Make all key rotations publish immediately, not just remote key rotat…

…ions

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

client/client.go

@@ -875,44 +875,47 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error {
 	switch {
 	// We currently support locally or remotely managing snapshot keys...
 	case role == data.CanonicalSnapshotRole:
+		break
+
 	// locally managing targets keys only
-	case role == data.CanonicalTargetsRole:
-		if serverManagesKey {
-			return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole}
-		}
+	case role == data.CanonicalTargetsRole && !serverManagesKey:
+		break
+	case role == data.CanonicalTargetsRole && serverManagesKey:
+		return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole}
+
 	// and remotely managing timestamp keys only
-	case role == data.CanonicalTimestampRole:
-		if !serverManagesKey {
-			return ErrInvalidLocalRole{Role: data.CanonicalTimestampRole}
-		}
+	case role == data.CanonicalTimestampRole && serverManagesKey:
+		break
+	case role == data.CanonicalTimestampRole && !serverManagesKey:
+		return ErrInvalidLocalRole{Role: data.CanonicalTimestampRole}
+
 	default:
 		return fmt.Errorf("notary does not currently permit rotating the %s key", role)
 	}
 
-	if serverManagesKey {
-		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)
+	var (
+		pubKey    data.PublicKey
+		err       error
+		errFmtMsg string
+	)
+	switch serverManagesKey {
+	case true:
+		pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
+		errFmtMsg = "unable to rotate remote key: %s"
+	default:
+		pubKey, err = r.CryptoService.Create(role, data.ECDSAKey)
+		errFmtMsg = "unable to generate key: %s"
 	}
 
-	pubKey, err := r.CryptoService.Create(role, data.ECDSAKey)
 	if err != nil {
-		return fmt.Errorf("unable to generate key: %s", err)
+		return fmt.Errorf(errFmtMsg, err)
 	}
 
-	cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist"))
-	if err != nil {
+	cl := changelist.NewMemChangelist()
+	if err := r.rootFileKeyChange(cl, role, changelist.ActionCreate, pubKey); err != nil {
 		return err
 	}
-	defer cl.Close()
-
-	return r.rootFileKeyChange(cl, role, changelist.ActionCreate, pubKey)
+	return r.publish(cl)
 }
 
 func (r *NotaryRepository) rootFileKeyChange(cl changelist.Changelist, role, action string, key data.PublicKey) error {

client/client_test.go

@@ -2579,47 +2579,6 @@ 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)
@@ -2637,8 +2596,7 @@ func TestRemoteRotationError(t *testing.T) {
 
 // Rotates the keys.  After the rotation, downloading the latest metadata
 // and assert that the keys have changed
-func assertRotationSuccessful(t *testing.T, repo1 *NotaryRepository,
-	keysToRotate map[string]bool, alreadyPublished bool) {
+func assertRotationSuccessful(t *testing.T, repo1 *NotaryRepository, keysToRotate map[string]bool) {
 	// Create 2 new repos:  1 will download repo data before the publish,
 	// and one only downloads after the publish. This reflects a client
 	// that has some previous trust data (but is not the publisher), and a
@@ -2648,35 +2606,26 @@ func assertRotationSuccessful(t *testing.T, repo1 *NotaryRepository,
 
 	repos := []*NotaryRepository{repo1, repo2}
 
-	if alreadyPublished {
-		repo3, _ := newRepoToTestRepo(t, repo1, true)
-		defer os.RemoveAll(repo2.baseDir)
-
-		// force a pull on repo3
-		_, err := repo3.GetTargetByName("latest")
-		assert.NoError(t, err)
-
-		repos = append(repos, repo3)
-	}
-
 	oldKeyIDs := make(map[string][]string)
 	for role := range keysToRotate {
 		keyIDs := repo1.tufRepo.Root.Signed.Roles[role].KeyIDs
 		oldKeyIDs[role] = keyIDs
 	}
 
+	// Confirm no changelists get published
+	changesPre := getChanges(t, repo1)
+
 	// Do rotation
 	for role, serverManaged := range keysToRotate {
 		assert.NoError(t, repo1.RotateKey(role, serverManaged))
 	}
 
-	// Publish
-	err := repo1.Publish()
-	assert.NoError(t, err)
+	changesPost := getChanges(t, repo1)
+	assert.Equal(t, changesPre, changesPost)
 
 	// Download data from remote and check that keys have changed
 	for _, repo := range repos {
-		_, err := repo.GetTargetByName("latest") // force a pull
+		_, err := repo.Update(true)
 		assert.NoError(t, err)
 
 		for role, isRemoteKey := range keysToRotate {
@@ -2704,12 +2653,6 @@ func assertRotationSuccessful(t *testing.T, repo1 *NotaryRepository,
 				assert.NotNil(t, key)
 			}
 		}
-
-		// Confirm changelist dir empty (on repo1, it should be empty after
-		// after publishing changes, on repo2, there should never have been
-		// any changelists)
-		changes := getChanges(t, repo)
-		assert.Len(t, changes, 0, "wrong number of changelist files found")
 	}
 }
 
@@ -2725,11 +2668,17 @@ func TestRotateBeforePublishFromRemoteKeyToLocalKey(t *testing.T) {
 	defer os.RemoveAll(repo.baseDir)
 
 	// Adding a target will allow us to confirm the repository is still valid
-	// after rotating the keys.
+	// after rotating the keys when we publish (and that rotation doesn't publish
+	// non-key-rotation changes)
 	addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt")
+
 	assertRotationSuccessful(t, repo, map[string]bool{
 		data.CanonicalTargetsRole:  false,
-		data.CanonicalSnapshotRole: false}, false)
+		data.CanonicalSnapshotRole: false})
+
+	assert.NoError(t, repo.Publish())
+	_, err := repo.GetTargetByName("latest")
+	assert.NoError(t, err)
 }
 
 // Initialize a repo, locally signed snapshots
@@ -2777,12 +2726,13 @@ func testRotateKeySuccess(t *testing.T, serverManagesSnapshotInit bool,
 	// rotating the keys.
 	addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt")
 
+	assertRotationSuccessful(t, repo, keysToRotate)
+
 	// Publish
 	assert.NoError(t, repo.Publish())
-
 	// Get root.json and capture targets + snapshot key IDs
-	repo.GetTargetByName("latest") // force a pull
-	assertRotationSuccessful(t, repo, keysToRotate, true)
+	_, err := repo.GetTargetByName("latest")
+	assert.NoError(t, err)
 
 	var keysToExpectCreated []string
 	for role, serverManaged := range keysToRotate {

cmd/notary/integration_test.go

@@ -871,28 +871,13 @@ func TestClientKeyGenerationRotation(t *testing.T) {
 	assertSuccessfullyPublish(t, tempDir, server.URL, "gun", target, tempfiles[0])
 
 	// rotate the signing keys
-	_, err = runCommand(t, tempDir, "key", "rotate", "gun", data.CanonicalSnapshotRole)
+	_, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", data.CanonicalSnapshotRole)
 	assert.NoError(t, err)
-	_, err = runCommand(t, tempDir, "key", "rotate", "gun", data.CanonicalTargetsRole)
+	_, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", data.CanonicalTargetsRole)
 	assert.NoError(t, err)
-	root, sign := assertNumKeys(t, tempDir, 1, 4, true)
+	root, sign := assertNumKeys(t, tempDir, 1, 2, true)
 	assert.Equal(t, origRoot[0], root[0])
-	// there should be the new keys and the old keys
-	for _, origKey := range origSign {
-		found := false
-		for _, key := range sign {
-			if key == origKey {
-				found = true
-			}
-		}
-		assert.True(t, found, "Old key not found in list of old and new keys")
-	}
 
-	// publish the key rotation
-	_, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun")
-	assert.NoError(t, err)
-	root, sign = assertNumKeys(t, tempDir, 1, 2, true)
-	assert.Equal(t, origRoot[0], root[0])
 	// just do a cursory rotation check that the keys aren't equal anymore
 	for _, origKey := range origSign {
 		for _, key := range sign {

cmd/notary/keys.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"net/http"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -38,7 +37,7 @@ var cmdKeyListTemplate = usageTemplate{
 var cmdRotateKeyTemplate = usageTemplate{
 	Use:   "rotate [ GUN ] [ key role ]",
 	Short: "Rotate a signing (non-root) key of the given type for the given Globally Unique Name and role.",
-	Long:  "Generates a new signing key (non-root) for the given Globally Unique Name and role.  Rotating to a server-managed key is an online-only operation: a new key is requested from the server rather than generated, and if successful, the key rotation is immediately published.  Rotating to a locally-managed key is an offline operation only: `notary publish` must be executed manually afterward to publish to the remote server.\nThe role must be one of \"snapshot\", \"targets\", or \"timestamp\".",
+	Long:  "Generates a new key for the given Globally Unique Name and role (one of \"snapshot\", \"targets\", or \"timestamp\").  If rotating to a server-managed key, a new key is requested from the server rather than generated.  If the generation or key request is successful, the key rotation only is immediately published.",
 }
 
 var cmdKeyGenerateRootKeyTemplate = usageTemplate{
@@ -415,15 +414,11 @@ func (k *keyCommander) keysRotate(cmd *cobra.Command, args []string) error {
 	gun := args[0]
 	rotateKeyRole := args[1]
 
-	var rt http.RoundTripper
-	if k.rotateKeyServerManaged {
-		// this does not actually push the changes, just creates the keys, but
-		// it creates a key remotely so it needs a transport
-		rt, err = getTransport(config, gun, false)
-		if err != nil {
-			return err
-		}
+	rt, err := getTransport(config, gun, false)
+	if err != nil {
+		return err
 	}
+
 	nRepo, err := notaryclient.NewNotaryRepository(
 		config.GetString("trust_dir"), gun, getRemoteTrustServer(config),
 		rt, k.getRetriever())

cmd/notary/keys_test.go

@@ -414,14 +414,13 @@ func TestRotateKeyBothKeys(t *testing.T) {
 	ret := passphrase.ConstantRetriever("pass")
 
 	ts, initialKeys := setUpRepo(t, tempBaseDir, gun, ret)
-	// we won't need this anymore since we are creating keys locally
-	ts.Close()
+	defer ts.Close()
 
 	k := &keyCommander{
 		configGetter: func() (*viper.Viper, error) {
 			v := viper.New()
 			v.SetDefault("trust_dir", tempBaseDir)
-			// won't need a remote server URL, since we are creating local keys
+			v.SetDefault("remote_server.url", ts.URL)
 			return v, nil
 		},
 		getRetriever: func() passphrase.Retriever { return ret },
@@ -434,30 +433,32 @@ func TestRotateKeyBothKeys(t *testing.T) {
 
 	cl, err := repo.GetChangelist()
 	assert.NoError(t, err, "unable to get changelist: %v", err)
-	assert.Len(t, cl.List(), 2)
+	assert.Len(t, cl.List(), 0)
 
-	// two new keys have been created, and the old keys should still be there
+	// two new keys have been created, and the old keys should still be gone
 	newKeys := repo.CryptoService.ListAllKeys()
+	// there should be 3 keys - snapshot, targets, and root
+	assert.Len(t, newKeys, 3)
+
+	// the old snapshot/targets keys should be gone
 	for keyID, role := range initialKeys {
 		r, ok := newKeys[keyID]
-		assert.True(t, ok, "original key %s missing", keyID)
-		assert.Equal(t, role, r)
-		delete(newKeys, keyID)
+		switch r {
+		case data.CanonicalSnapshotRole, data.CanonicalTargetsRole:
+			assert.False(t, ok, "original key %s still there", keyID)
+		case data.CanonicalRootRole:
+			assert.Equal(t, role, r)
+			assert.True(t, ok, "old root key has changed")
+		}
 	}
-	// there should be 2 keys left
-	assert.Len(t, newKeys, 2)
-	// one for each role
-	var targetsFound, snapshotFound bool
+
+	found := make(map[string]bool)
 	for _, role := range newKeys {
-		switch role {
-		case data.CanonicalTargetsRole:
-			targetsFound = true
-		case data.CanonicalSnapshotRole:
-			snapshotFound = true
-		}
+		found[role] = true
 	}
-	assert.True(t, targetsFound, "targets key was not created")
-	assert.True(t, snapshotFound, "snapshot key was not created")
+	assert.True(t, found[data.CanonicalTargetsRole], "targets key was not created")
+	assert.True(t, found[data.CanonicalSnapshotRole], "snapshot key was not created")
+	assert.True(t, found[data.CanonicalRootRole], "root key was removed somehow")
 }
 
 func TestChangeKeyPassphraseInvalidID(t *testing.T) {