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

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

@@ -21,7 +21,6 @@ import (
 	"github.com/Sirupsen/logrus"
 	ctxu "github.com/docker/distribution/context"
 	"github.com/docker/go/canonical/json"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/net/context"
 
@@ -2580,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)
@@ -2632,14 +2590,13 @@ func TestRemoteRotationError(t *testing.T) {
 
 	// server has died, so this should fail
 	err := repo.RotateKey(data.CanonicalTimestampRole, true)
-	assert.Error(t, err)
-	assert.Contains(t, err.Error(), "unable to rotate remote key")
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "unable to rotate remote key")
 }
 
 // Rotates the keys.  After the rotation, downloading the latest metadata
 // and require that the keys have changed
-func requireRotationSuccessful(t *testing.T, repo1 *NotaryRepository,
-	keysToRotate map[string]bool, alreadyPublished bool) {
+func requireRotationSuccessful(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
@@ -2649,35 +2606,26 @@ func requireRotationSuccessful(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")
-		require.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 {
 		require.NoError(t, repo1.RotateKey(role, serverManaged))
 	}
 
-	// Publish
-	err := repo1.Publish()
-	require.NoError(t, err)
+	changesPost := getChanges(t, repo1)
+	require.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)
 		require.NoError(t, err)
 
 		for role, isRemoteKey := range keysToRotate {
@@ -2705,12 +2653,6 @@ func requireRotationSuccessful(t *testing.T, repo1 *NotaryRepository,
 				require.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)
-		require.Len(t, changes, 0, "wrong number of changelist files found")
 	}
 }
 
@@ -2726,11 +2668,16 @@ 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")
 	requireRotationSuccessful(t, repo, map[string]bool{
 		data.CanonicalTargetsRole:  false,
-		data.CanonicalSnapshotRole: false}, false)
+		data.CanonicalSnapshotRole: false})
+
+	require.NoError(t, repo.Publish())
+	_, err := repo.GetTargetByName("latest")
+	require.NoError(t, err)
 }
 
 // Initialize a repo, locally signed snapshots
@@ -2778,12 +2725,13 @@ func testRotateKeySuccess(t *testing.T, serverManagesSnapshotInit bool,
 	// rotating the keys.
 	addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt")
 
+	requireRotationSuccessful(t, repo, keysToRotate)
+
 	// Publish
 	require.NoError(t, repo.Publish())
-
 	// Get root.json and capture targets + snapshot key IDs
-	repo.GetTargetByName("latest") // force a pull
-	requireRotationSuccessful(t, repo, keysToRotate, true)
+	_, err := repo.GetTargetByName("latest")
+	require.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 is immediately published.  No other changes, even if they are staged, will be 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

@@ -328,7 +328,7 @@ func setUpRepo(t *testing.T, tempBaseDir, gun string, ret passphrase.Retriever)
 
 	cryptoService := cryptoservice.NewCryptoService(
 		"", trustmanager.NewKeyMemoryStore(ret))
-	ts := httptest.NewServer(server.RootHandler(nil, ctx, cryptoService))
+	ts := httptest.NewServer(server.RootHandler(nil, ctx, cryptoService, nil, nil))
 
 	repo, err := client.NewNotaryRepository(
 		tempBaseDir, gun, ts.URL, http.DefaultTransport, ret)
@@ -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)
-	}
-	// there should be 2 keys left
-	assert.Len(t, newKeys, 2)
-	// one for each role
-	var targetsFound, snapshotFound bool
-	for _, role := range newKeys {
-		switch role {
-		case data.CanonicalTargetsRole:
-			targetsFound = true
-		case data.CanonicalSnapshotRole:
-			snapshotFound = true
+		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")
 		}
 	}
-	assert.True(t, targetsFound, "targets key was not created")
-	assert.True(t, snapshotFound, "snapshot key was not created")
+
+	found := make(map[string]bool)
+	for _, role := range newKeys {
+		found[role] = true
+	}
+	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) {