Publish only the key rotation changes after a remote key rotation

Signed-off-by: Ying Li <ying.li@docker.com>

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())
 

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

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)

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,