From fa5edc40af7efdc0de23fd56605f0ef00c4a1bed Mon Sep 17 00:00:00 2001 From: Ying Li Date: Tue, 1 Mar 2016 18:00:38 -0500 Subject: [PATCH] Publish only the key rotation changes after a remote key rotation Signed-off-by: Ying Li --- client/backwards_compatibility_test.go | 5 +- client/client.go | 73 +++++++++++++------------- client/client_test.go | 41 +++++++++++++++ cmd/notary/keys.go | 6 +-- 4 files changed, 80 insertions(+), 45 deletions(-) diff --git a/client/backwards_compatibility_test.go b/client/backwards_compatibility_test.go index d78f313f9..fd2c3c7db 100644 --- a/client/backwards_compatibility_test.go +++ b/client/backwards_compatibility_test.go @@ -12,7 +12,6 @@ import ( "testing" "time" - "github.com/docker/notary/client/changelist" "github.com/docker/notary/passphrase" "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/store" @@ -108,10 +107,8 @@ func Test0Dot1RepoFormat(t *testing.T) { require.NoError(t, repo.fileStore.RemoveMeta(data.CanonicalTimestampRole)) // rotate the timestamp key, since the server doesn't have that one - timestampPubKey, err := getRemoteKey(ts.URL, gun, data.CanonicalTimestampRole, http.DefaultTransport) + err = repo.RotateKey(data.CanonicalTimestampRole, true) require.NoError(t, err) - require.NoError( - t, repo.rootFileKeyChange(data.CanonicalTimestampRole, changelist.ActionCreate, timestampPubKey)) require.NoError(t, repo.Publish()) diff --git a/client/client.go b/client/client.go index 859c66b17..65b4cf7f4 100644 --- a/client/client.go +++ b/client/client.go @@ -531,6 +531,25 @@ func (r *NotaryRepository) ListRoles() ([]RoleWithSignatures, error) { // Publish pushes the local changes in signed material to the remote notary-server // Conceptually it performs an operation similar to a `git rebase` func (r *NotaryRepository) Publish() error { + cl, err := r.GetChangelist() + if err != nil { + return err + } + if err = r.publish(cl); err != nil { + return err + } + if err = cl.Clear(""); err != nil { + // This is not a critical problem when only a single host is pushing + // but will cause weird behaviour if changelist cleanup is failing + // and there are multiple hosts writing to the repo. + logrus.Warn("Unable to clear changelist. You may want to manually delete the folder ", filepath.Join(r.tufRepoPath, "changelist")) + } + return nil +} + +// publish pushes the changes in the given changelist to the remote notary-server +// Conceptually it performs an operation similar to a `git rebase` +func (r *NotaryRepository) publish(cl changelist.Changelist) error { var initialPublish bool // update first before publishing _, err := r.Update(true) @@ -558,10 +577,6 @@ func (r *NotaryRepository) Publish() error { return err } } - cl, err := r.GetChangelist() - if err != nil { - return err - } // apply the changelist to the repo err = applyChangelist(r.tufRepo, cl) if err != nil { @@ -632,18 +647,7 @@ func (r *NotaryRepository) Publish() error { return err } - err = remote.SetMultiMeta(updatedFiles) - if err != nil { - return err - } - err = cl.Clear("") - if err != nil { - // This is not a critical problem when only a single host is pushing - // but will cause weird behaviour if changelist cleanup is failing - // and there are multiple hosts writing to the repo. - logrus.Warn("Unable to clear changelist. You may want to manually delete the folder ", filepath.Join(r.tufRepoPath, "changelist")) - } - return nil + return remote.SetMultiMeta(updatedFiles) } // bootstrapRepo loads the repository from the local file system. This attempts @@ -885,32 +889,33 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error { return fmt.Errorf("notary does not currently permit rotating the %s key", role) } - var ( - pubKey data.PublicKey - err error - errFmtString string - ) if serverManagesKey { - pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip) - errFmtString = "unable to rotate remote key: %s" - } else { - pubKey, err = r.CryptoService.Create(role, data.ECDSAKey) - errFmtString = "unable to generate key: %s" + pubKey, err := getRemoteKey(r.baseURL, r.gun, role, r.roundTrip) + if err != nil { + return fmt.Errorf("unable to rotate remote key: %s", err) + } + cl := changelist.NewMemChangelist() + if err := r.rootFileKeyChange(cl, role, changelist.ActionCreate, pubKey); err != nil { + return err + } + return r.publish(cl) } + + pubKey, err := r.CryptoService.Create(role, data.ECDSAKey) if err != nil { - return fmt.Errorf(errFmtString, err) + return fmt.Errorf("unable to generate key: %s", err) } - return r.rootFileKeyChange(role, changelist.ActionCreate, pubKey) -} - -func (r *NotaryRepository) rootFileKeyChange(role, action string, key data.PublicKey) error { cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) if err != nil { return err } defer cl.Close() + return r.rootFileKeyChange(cl, role, changelist.ActionCreate, pubKey) +} + +func (r *NotaryRepository) rootFileKeyChange(cl changelist.Changelist, role, action string, key data.PublicKey) error { kl := make(data.KeyList, 0, 1) kl = append(kl, key) meta := changelist.TufRootData{ @@ -929,11 +934,7 @@ func (r *NotaryRepository) rootFileKeyChange(role, action string, key data.Publi role, metaJSON, ) - err = cl.Add(c) - if err != nil { - return err - } - return nil + return cl.Add(c) } // DeleteTrustData removes the trust data stored for this repo in the TUF cache and certificate store on the client side diff --git a/client/client_test.go b/client/client_test.go index 897969af8..fccc74221 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2580,6 +2580,47 @@ func TestRotateKeyInvalidRole(t *testing.T) { } } +// Initialize repo to have local signing of snapshots. Rotate the key to have +// the server manage the snapshot key. Assert that this publishes the key change +// but not any other changes. +func TestRemoteRotationOnlyPublishesKeyChanges(t *testing.T) { + ts := fullTestServer(t) + defer ts.Close() + + repo1, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, false) + defer os.RemoveAll(repo1.baseDir) + + oldSnapshotRole, err := repo1.tufRepo.GetBaseRole(data.CanonicalSnapshotRole) + assert.NoError(t, err) + + // Add and make sure it's not published when the key is rotated + addTarget(t, repo1, "latest", "../fixtures/intermediate-ca.crt") + cl, err := repo1.GetChangelist() + assert.NoError(t, err) + assert.Len(t, cl.List(), 1) + + assert.NoError(t, repo1.RotateKey(data.CanonicalSnapshotRole, true)) + assert.Len(t, cl.List(), 1, "rotating key published other changes") + + // ensure that the key rotation was public by pulling from a new repo + repo2, _ := newRepoToTestRepo(t, repo1, true) + defer os.RemoveAll(repo2.baseDir) + tgts, err := repo2.ListTargets() + assert.NoError(t, err) + assert.Len(t, tgts, 0, "No targets should have been published") + + newSnapshotRole, err := repo2.tufRepo.GetBaseRole(data.CanonicalSnapshotRole) + assert.NoError(t, err) + + // assert that the snapshot key has changed + assert.Len(t, oldSnapshotRole.Keys, 1) + assert.Len(t, newSnapshotRole.Keys, 1) + for k := range oldSnapshotRole.Keys { + _, ok := newSnapshotRole.Keys[k] + assert.False(t, ok) + } +} + // If remotely rotating key fails, the failure is propagated func TestRemoteRotationError(t *testing.T) { ts, _, _ := simpleTestServer(t) diff --git a/cmd/notary/keys.go b/cmd/notary/keys.go index f4debba3b..f4df2c708 100644 --- a/cmd/notary/keys.go +++ b/cmd/notary/keys.go @@ -430,11 +430,7 @@ func (k *keyCommander) keysRotate(cmd *cobra.Command, args []string) error { if err != nil { return err } - err = nRepo.RotateKey(rotateKeyRole, k.rotateKeyServerManaged) - if err == nil && k.rotateKeyServerManaged { - err = nRepo.Publish() - } - return err + return nRepo.RotateKey(rotateKeyRole, k.rotateKeyServerManaged) } func removeKeyInteractively(keyStores []trustmanager.KeyStore, keyID string,