Change the CLI to be able to rotate server managed keys

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

@@ -39,14 +39,25 @@ func (err ErrRepoNotInitialized) Error() string {
 }
 
 // ErrInvalidRemoteRole is returned when the server is requested to manage
-// an unsupported key type
+// a key type that is not permitted
 type ErrInvalidRemoteRole struct {
 	Role string
 }
 
 func (err ErrInvalidRemoteRole) Error() string {
 	return fmt.Sprintf(
-		"notary does not support the server managing the %s key", err.Role)
+		"notary does not permit the server managing the %s key", err.Role)
+}
+
+// ErrInvalidLocalRole is returned when the client wants to manage
+// a key type that is not permitted
+type ErrInvalidLocalRole struct {
+	Role string
+}
+
+func (err ErrInvalidLocalRole) Error() string {
+	return fmt.Sprintf(
+		"notary does not permit the client managing the %s key", err.Role)
 }
 
 // ErrRepositoryNotExist is returned when an action is taken on a remote
@@ -520,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)
@@ -543,15 +573,10 @@ func (r *NotaryRepository) Publish() error {
 			initialPublish = true
 		} else {
 			// We could not update, so we cannot publish.
-			logrus.Error("Could not publish Repository: ", err.Error())
+			logrus.Error("Could not publish Repository since we could not update: ", err.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 {
@@ -622,25 +647,14 @@ 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
 // to load metadata for all roles.  Since server snapshots are supported,
 // if the snapshot metadata fails to load, that's ok.
 // This can also be unified with some cache reading tools from tuf/client.
-// This assumes that bootstrapRepo is only used by Publish()
+// This assumes that bootstrapRepo is only used by Publish() or RotateKey()
 func (r *NotaryRepository) bootstrapRepo() error {
 	tufRepo := tuf.NewRepo(r.CryptoService)
 
@@ -858,37 +872,53 @@ func (r *NotaryRepository) validateRoot(rootJSON []byte) (*data.SignedRoot, erro
 // 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 {
-	if role == data.CanonicalRootRole || role == data.CanonicalTimestampRole {
-		return fmt.Errorf(
-			"notary does not currently support rotating the %s key", role)
-	}
-	if serverManagesKey && role == data.CanonicalTargetsRole {
+	switch {
+	// We currently support locally or remotely managing snapshot keys...
+	case role == data.CanonicalSnapshotRole:
+		break
+
+	// locally managing targets keys only
+	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 && 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)
 	}
 
 	var (
-		pubKey data.PublicKey
-		err    error
+		pubKey    data.PublicKey
+		err       error
+		errFmtMsg string
 	)
-	if serverManagesKey {
+	switch serverManagesKey {
+	case true:
 		pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
-	} else {
+		errFmtMsg = "unable to rotate remote key: %s"
+	default:
 		pubKey, err = r.CryptoService.Create(role, data.ECDSAKey)
+		errFmtMsg = "unable to generate key: %s"
 	}
+
 	if err != nil {
-		return err
+		return fmt.Errorf(errFmtMsg, 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 {
+	cl := changelist.NewMemChangelist()
+	if err := r.rootFileKeyChange(cl, role, changelist.ActionCreate, pubKey); err != nil {
 		return err
 	}
-	defer cl.Close()
+	return r.publish(cl)
+}
 
+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{
@@ -907,11 +937,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

@@ -247,7 +247,7 @@ func TestInitRepositoryManagedRolesIncludingRoot(t *testing.T) {
 	require.IsType(t, ErrInvalidRemoteRole{}, err)
 	// Just testing the error message here in this one case
 	require.Equal(t, err.Error(),
-		"notary does not support the server managing the root key")
+		"notary does not permit the server managing the root key")
 	// no key creation happened
 	rec.requireCreated(t, nil)
 }
@@ -2553,16 +2553,24 @@ func TestRotateKeyInvalidRole(t *testing.T) {
 	repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, false)
 	defer os.RemoveAll(repo.baseDir)
 
-	// the equivalent of: (root, true), (root, false), (timestamp, true),
-	// (timestamp, false), (targets, true)
+	// the equivalent of: remotely rotating the root key
+	// (RotateKey("root", true)), locally rotating the root key (RotateKey("root", false)),
+	// locally rotating the timestamp key (RotateKey("timestamp", false)),
+	// and remotely rotating the targets key (RotateKey(targets, true)), all of which should
+	// fail
 	for _, role := range data.BaseRoles {
 		if role == data.CanonicalSnapshotRole {
 			continue
 		}
 		for _, serverManagesKey := range []bool{true, false} {
+			// we support local rotation of the targets key and remote rotation of the
+			// timestamp key
 			if role == data.CanonicalTargetsRole && !serverManagesKey {
 				continue
 			}
+			if role == data.CanonicalTimestampRole && serverManagesKey {
+				continue
+			}
 			err := repo.RotateKey(role, serverManagesKey)
 			require.Error(t, err,
 				"Rotating a %s key with server-managing the key as %v should fail",
@@ -2571,10 +2579,24 @@ func TestRotateKeyInvalidRole(t *testing.T) {
 	}
 }
 
+// If remotely rotating key fails, the failure is propagated
+func TestRemoteRotationError(t *testing.T) {
+	ts, _, _ := simpleTestServer(t)
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, true)
+	defer os.RemoveAll(repo.baseDir)
+
+	ts.Close()
+
+	// server has died, so this should fail
+	err := repo.RotateKey(data.CanonicalTimestampRole, true)
+	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
@@ -2584,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 {
@@ -2640,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")
 	}
 }
 
@@ -2661,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
@@ -2713,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

@@ -589,7 +589,7 @@ func TestClientDelegationsPublishing(t *testing.T) {
 	assertNumKeys(t, tempDir, 1, 2, true)
 
 	// rotate the snapshot key to server
-	output, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", "-r", "--key-type", "snapshot")
+	output, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", "snapshot", "-r")
 	assert.NoError(t, err)
 
 	// publish repo
@@ -871,26 +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")
+	_, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", data.CanonicalSnapshotRole)
 	assert.NoError(t, err)
-	root, sign := assertNumKeys(t, tempDir, 1, 4, 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")
+	_, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", data.CanonicalTargetsRole)
 	assert.NoError(t, err)
-	root, sign = assertNumKeys(t, tempDir, 1, 2, true)
+	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 {