Support deprecating old root keys after rotation #942

ecordell · 2016-09-07T21:28:04Z

This adds support for root key rotation as described in #835.

Previously, old root keys needed to sign for all subsequent root.json files in perpetuity. This changes root rotation so that new keys are trusted if signed by the previous root key/threshold. To support this behavior, root files can now be requested by version number from the server.

This also adds an additional command line flag for rotate, --key, which allows specifying keys to rotate the root to. This supports specifying multiple keys, but all must be available to sign locally (i.e. this doesn't support threshold/witness behavior at the moment).

Example:

# test/collection root.json signed by A, B
$ notary key rotate test/collection root --key=C --key=D
# test/collection root.json now signed by A, B, C, D
$ notary key rotate test/collection root --key=E
# test/collection root.json now signed by C, D, E

Closes #1014
Closes #835

docker-jenkins · 2016-09-07T21:28:06Z

Can one of the admins verify this patch?

HuKeping · 2016-09-08T07:37:25Z

cmd/notary/keys.go

@@ -13,11 +13,12 @@ import (
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/utils"

+	"os"


would you mind if moving this package up to those canonical packages

Will do - could've sworn gofmt does this for you.

riyazdf · 2016-09-09T22:06:53Z

client/client.go

@@ -906,7 +909,7 @@ func (r *NotaryRepository) bootstrapClient(checkInitialized bool) (*TUFClient, e
 // RotateKey removes all existing keys associated with the role, and either
 // creates and adds one new key or delegates managing the key to the server.
 // These changes are staged in a changelist until publish is called.
-func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error {
+func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool, keyList []string) error {


can we document what keyList is in the function description?

riyazdf · 2016-09-09T23:02:02Z

thanks @ecordell for your work on this! I have a couple of things:

I want to echo my comment about whether we should allow non-root roles to rotate to multiple keys -- it's fine with me but if so we should make sure it's adequately tested in unit and integration tests. I'm also fine with leaving the scope of this PR to only root-roles and expanding on this later.
nit: can we rename the PR to something like Support specifying specific keys on root rotation? It's a little confusing because root key rotation is technically already supported :)

ecordell · 2016-09-13T15:10:56Z

@riyazdf thanks for taking a look!

Responded inline, but currently this will fail for non-root roles because the non-root keys are not generally encrypted. I dislike that this is an implicit restriction so I think we should choose one way or the other and I'll update.
I'll update the title

cyli · 2017-01-06T18:04:24Z

tuf/signed/sign_test.go

@@ -88,6 +92,13 @@ func (mts *MockCryptoService) GetKey(keyID string) data.PublicKey {
 	return nil
 }

+func (mts *MockCryptoService) GetKeyInfo(keyID string) (trustmanager.KeyInfo, error) {


This function can probably be removed too.

cyli · 2017-01-06T18:05:39Z

signer/client/signer_trust.go

@@ -203,6 +204,11 @@ func (trust *NotarySigner) GetKey(keyid string) data.PublicKey {
 	return pubKey
 }

+// GetKeyInfo returns the gun and role given a keyID


This function can probably be removed too.

cyli · 2017-01-06T18:05:44Z

signer/keydbstore/rethink_keydbstore.go

@@ -215,6 +215,16 @@ func (rdb *RethinkDBKeyStore) GetKey(keyID string) data.PublicKey {
 	return data.NewPublicKey(dbPrivateKey.Algorithm, dbPrivateKey.Public)
 }

+// GetKeyInfo returns the gun and role given a KeyID, and does not activate the key


This function can probably be removed too.

cyli · 2017-01-06T18:05:51Z

signer/keydbstore/sql_keydbstore.go

@@ -228,6 +228,15 @@ func (s *SQLKeyDBStore) GetKey(keyID string) data.PublicKey {
 	return data.NewPublicKey(privKey.Algorithm, []byte(privKey.Public))
 }

+// GetKeyInfo returns role and GUN info of a key by ID


This function can probably be removed too.

cyli · 2017-01-06T18:16:46Z

@ecordell
Thanks for adding the version skip test! (2) TestRootRoleInvariant tests that the latest root does not validate, but not intermediate downloaded roots, and (3) TestUpdateFailsIfServerRootKeyChangedWithoutMultiSign tests that if the cached root is expired, then an update succeeds so long as the new root is valid.

We were thinking for (3) something like having the local cache at v1 (either expired or not), and downloading an expired v2 and a valid v3? It should successfully update to v3.

Similarly for (2), except v2 is not properly rotated. The only reason being because some checks are skipped for intermediate roots (like checksum and expiry), and this would validate that the root signing is validated for the intermediate roots.

ecordell · 2017-01-07T00:42:15Z

@cyli I gotcha - I added those extra tests to be safe, thanks for clarifying.

I also removed those other GetKeyInfo methods - somehow I missed them, sorry about that.

cyli · 2017-01-07T01:10:33Z

@ecordell Thanks for adding them!

I also removed those other GetKeyInfo methods - somehow I missed them, sorry about that.

Not at all, easy to lose track :)

cyli · 2017-01-07T01:10:36Z

jenkins, test this please

cyli · 2017-01-07T01:17:38Z

LGTM! Thank you so much for all your work and patience on this @ecordell!

Signed-off-by: Evan Cordell <[email protected]>

ecordell · 2017-01-09T15:33:42Z

Thanks! I've rebased on master, so hopefully everything will be good shortly.

cyli · 2017-01-09T17:54:59Z

Jenkins, test this please

endophage · 2017-01-09T23:16:59Z

tuf/tuf.go

-
-	return oldKeys
-}
-
 func oldRootVersionName(version int) string {


Followup PR: I think we can remove this function. I don't think it's used anywhere with the new root rotation logic.

endophage

LGTM.

@ecordell thank you for all the hard work and perseverance on this. Conceptually and concretely I think this root rotation methodology is an outstanding improvement to both the TUF spec and Notary.

riyazdf

couple of nits between myself and @endophage's comment that can be followed up in a subsequent PR.

Overall LGTM, thank you for continuously working through this PR! 🎉

riyazdf · 2017-01-09T23:43:17Z

client/client.go

@@ -700,11 +714,84 @@ func signRootIfNecessary(updates map[string][]byte, repo *tuf.Repo, initialPubli
 	return nil
 }

+// Fetch back a `legacyVersions` number of roots files, collect the root public keys
+// This includes old `root` roles as well as legacy versioned root roles, e.g. `1.root`
+func (r *NotaryRepository) oldKeysForLegacyClientSupport(legacyVersions int, initialPublish bool) (data.KeyList, error) {


followup PR: I'm pretty happy with this function but would like to add some unit tests around overly large or negative legacyVersions and the skipping logic

HuKeping · 2017-01-10T03:36:52Z

Thanks for your patience and great work on this @ecordell !

ecordell · 2017-01-10T15:32:41Z

Thanks for all of the reviews! I can't merge myself if someone with write access would like to :) Glad to finally have this wrapped up.

GordonTheTurtle added status/0-triage dco/no labels Sep 7, 2016

ecordell mentioned this pull request Sep 7, 2016

Implement Threshold Signing mechanisms on the client/server #841

Open

ecordell force-pushed the root-key-rotation branch from 86ce5c3 to bea7cac Compare September 7, 2016 21:33

GordonTheTurtle removed the dco/no label Sep 7, 2016

HuKeping reviewed Sep 8, 2016
View reviewed changes

ecordell force-pushed the root-key-rotation branch from bea7cac to 24273a3 Compare September 8, 2016 21:58

riyazdf reviewed Sep 9, 2016
View reviewed changes

ecordell force-pushed the root-key-rotation branch from 24273a3 to 377176c Compare September 13, 2016 15:06

ecordell changed the title ~~Support root key rotation~~ Support deprecating old root keys after rotation Sep 13, 2016

ecordell force-pushed the root-key-rotation branch 9 times, most recently from 4bc591c to 5ec5862 Compare September 19, 2016 11:21

cyli reviewed Jan 6, 2017

View reviewed changes

ecordell force-pushed the root-key-rotation branch from 7a15b51 to 1dc0dc9 Compare January 7, 2017 00:39

GordonTheTurtle added the dco/no label Jan 9, 2017

ecordell added 7 commits January 9, 2017 10:32

Allow specifying key(s) when rotating root

9800984

Signed-off-by: Evan Cordell <[email protected]>

server: add support for serving root files by version

205e8a5

Signed-off-by: Evan Cordell <[email protected]>

client: add support for rotating root keys

a5ffe53

Signed-off-by: Evan Cordell <[email protected]>

minor refactoring: root key rotation

22bedc7

Signed-off-by: Evan Cordell <[email protected]>

Support legacy clients

e58a1a8

Signed-off-by: Evan Cordell <[email protected]>

clean up root rotation

edfd88c

Signed-off-by: Evan Cordell <[email protected]>

Additional root rotation tests

bebce70

Signed-off-by: Evan Cordell <[email protected]>

ecordell force-pushed the root-key-rotation branch from 61a274c to bebce70 Compare January 9, 2017 15:33

GordonTheTurtle removed the dco/no label Jan 9, 2017

endophage reviewed Jan 9, 2017

View reviewed changes

endophage approved these changes Jan 9, 2017

View reviewed changes

riyazdf approved these changes Jan 9, 2017

View reviewed changes

endophage merged commit eae6372 into notaryproject:master Jan 10, 2017

ecordell deleted the root-key-rotation branch January 10, 2017 19:39

cyli mentioned this pull request Jan 11, 2017

method to export timestamp key from notary-signer #1078

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support deprecating old root keys after rotation #942

Support deprecating old root keys after rotation #942

ecordell commented Sep 7, 2016 •

edited

Loading

docker-jenkins commented Sep 7, 2016

HuKeping Sep 8, 2016

ecordell Sep 8, 2016

riyazdf Sep 9, 2016

riyazdf commented Sep 9, 2016

ecordell commented Sep 13, 2016

cyli Jan 6, 2017

cyli Jan 6, 2017

cyli Jan 6, 2017

cyli Jan 6, 2017

cyli commented Jan 6, 2017

ecordell commented Jan 7, 2017

cyli commented Jan 7, 2017

cyli commented Jan 7, 2017

cyli commented Jan 7, 2017

ecordell commented Jan 9, 2017

cyli commented Jan 9, 2017

endophage Jan 9, 2017

endophage left a comment

riyazdf left a comment •

edited

Loading

riyazdf Jan 9, 2017

HuKeping commented Jan 10, 2017

ecordell commented Jan 10, 2017

Support deprecating old root keys after rotation #942

Support deprecating old root keys after rotation #942

Conversation

ecordell commented Sep 7, 2016 • edited Loading

docker-jenkins commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riyazdf commented Sep 9, 2016

ecordell commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Jan 6, 2017

ecordell commented Jan 7, 2017

cyli commented Jan 7, 2017

cyli commented Jan 7, 2017

cyli commented Jan 7, 2017

ecordell commented Jan 9, 2017

cyli commented Jan 9, 2017

Choose a reason for hiding this comment

endophage left a comment

Choose a reason for hiding this comment

riyazdf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuKeping commented Jan 10, 2017

ecordell commented Jan 10, 2017

ecordell commented Sep 7, 2016 •

edited

Loading

riyazdf left a comment •

edited

Loading