Update the client to load the root in order to remotely rotate the key.

This also changes server key rotation behavior to just be get key behavior,
until such time as the server key rotation behavior is implemented.

We also now allow the client to remotely rotate timestmap keys.

Signed-off-by: Ying Li <[email protected]>

client/backwards_compatibility_test.go

@@ -12,6 +12,7 @@ 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"
@@ -106,8 +107,13 @@ func Test0Dot1RepoFormat(t *testing.T) {
 	// one and try to create a new one from scratch, which will be the wrong version
 	require.NoError(t, repo.fileStore.RemoveMeta(data.CanonicalTimestampRole))
 
-	// rotate the timestamp key, since the server doesn't have that one
-	require.NoError(t, repo.RotateKey(data.CanonicalTimestampRole, true))
+	// rotate the timestamp key, since the server doesn't have that one (we can't just
+	// call RotateKey because the server will verify that it is signed by the root, and
+	// the server doesn't know about this one)
+	timestampPubKey, err := getRemoteKey(ts.URL, gun, data.CanonicalTimestampRole, http.DefaultTransport)
+	require.NoError(t, err)
+	require.NoError(
+		t, repo.rootFileKeyChange(data.CanonicalTimestampRole, changelist.ActionCreate, timestampPubKey))
 	require.NoError(t, repo.Publish())
 
 	targets, err = repo.ListTargets()

client/client.go

@@ -522,11 +522,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
@@ -619,7 +618,7 @@ func (r *NotaryRepository) Publish() error {
 // 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 {
 	kdb := keys.NewDB()
 	tufRepo := tuf.NewRepo(kdb, r.CryptoService)
@@ -848,7 +847,7 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error {
 		return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole}
 	}
 	if !serverManagesKey && role == data.CanonicalTimestampRole {
-		return ErrInvalidLocalRole{Role: data.CanonicalTargetsRole}
+		return ErrInvalidLocalRole{Role: data.CanonicalTimestampRole}
 	}
 
 	var (
@@ -857,18 +856,48 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error {
 		err          error
 	)
 	if serverManagesKey {
-		pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip, true)
-		errFmtString = "server unable to rotate key: %s"
+		if err := r.bootstrapRepo(); err != nil {
+			return err
+		}
+
+		// get root keys - since we bootstrapped the repo, a valid root should always specify
+		// the root role
+		rootRole, ok := r.tufRepo.Root.Signed.Roles[data.CanonicalRootRole]
+		if !ok {
+			return fmt.Errorf("update resulted in an invalid state with an invalid root")
+		}
+		rootKeys := make([]data.PublicKey, 0, len(rootRole.KeyIDs))
+		for _, kid := range rootRole.KeyIDs {
+			k, ok := r.tufRepo.Root.Signed.Keys[kid]
+			if ok {
+				// this should always be the case, as key IDs need to correspond to keys in the
+				// key list, but just in case: skip if it's not
+				rootKeys = append(rootKeys, k)
+			}
+		}
+
+		errFmtString = "unable to rotate remote key: %s"
+		remote, err := getRemoteStore(r.baseURL, r.gun, r.roundTrip)
+		if err == nil {
+			pubKey, err = remote.RotateKey(role, r.CryptoService, rootKeys...)
+		}
 	} else {
 		pubKey, err = r.CryptoService.Create(role, data.ECDSAKey)
 		errFmtString = "unable to generate key: %s"
 	}
 
 	if err != nil {
-		return fmt.Errof(errFmtString, err)
+		return fmt.Errorf(errFmtString, err)
+	}
+
+	if err = r.rootFileKeyChange(role, changelist.ActionCreate, pubKey); err != nil {
+		return err
 	}
 
-	return r.rootFileKeyChange(role, changelist.ActionCreate, pubKey)
+	if serverManagesKey {
+		return r.Publish()
+	}
+	return nil
 }
 
 func (r *NotaryRepository) rootFileKeyChange(role, action string, key data.PublicKey) error {

client/client_test.go

@@ -2570,50 +2570,37 @@ func testRotateKeySuccess(t *testing.T, serverManagesSnapshotInit bool,
 	}
 }
 
-// If remote rotate key was rate limited, just get the latest created key and
-// use that.
-func TestRemoteRotationRateLimited(t *testing.T) {
-	ts := fullTestServer(t)
+// If remotely rotating key fails for a non-rate-limit reason, fail the rotation
+// entirely
+func TestRemoteRotationNonRateLimitError(t *testing.T) {
+	ts, _, _ := simpleTestServer(t)
 	defer ts.Close()
 
 	repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, true)
 	defer os.RemoveAll(repo.baseDir)
 
-	role := data.CanonicalSnapshotRole
-
-	// original key is the key on the repo
-	origKey, err := getRemoteKey(ts.URL, repo.gun, role, repo.roundTrip)
-	assert.NoError(t, err)
-	assert.Len(t, repo.tufRepo.Root.Signed.Roles[role].KeyIDs, 1)
-	assert.Equal(t, origKey.ID(), repo.tufRepo.Root.Signed.Roles[role].KeyIDs[0])
-
-	// rotate keys, and assert that the first rotation doesn't fail but doesn't rotate,
-	// because the original key has not actually been published yet.
-	assert.NoError(t, repo.RotateKey(role, true))
-
-	rotateKey1, err := getRemoteKey(ts.URL, repo.gun, role, repo.roundTrip)
-	assert.NoError(t, err)
-	assert.Equal(t, origKey.ID(), rotateKey1.ID())
-
-	// publish
-	assert.NoError(t, repo.Publish())
-
-	// the final key is the original key, since it was not rotated
-	assert.Len(t, repo.tufRepo.Root.Signed.Roles[role].KeyIDs, 1)
-	assert.Equal(t, rotateKey1.ID(), repo.tufRepo.Root.Signed.Roles[role].KeyIDs[0])
+	// simpleTestServer has no rotate key endpoint, so this should fail
+	assert.Error(t, repo.RotateKey(data.CanonicalTimestampRole, true))
 }
 
-// If remotely rotating key fails for a non-rate-limit reason, fail the rotation
-// entirely
-func TestRemoteRotationNonRateLimitError(t *testing.T) {
-	ts, _, _ := simpleTestServer(t)
+// The rotator is not the owner of the repository, they cannot rotate the remote
+// key
+func TestRemoteRotationNonPermitted(t *testing.T) {
+	ts := fullTestServer(t)
 	defer ts.Close()
 
 	repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, true)
 	defer os.RemoveAll(repo.baseDir)
+	assert.NoError(t, repo.Publish())
 
-	// simpleTestServer has no rotate key endpoint, so this should fail
-	assert.Error(t, repo.RotateKey(data.CanonicalTimestampRole, true))
+	newRepo, _ := newRepoToTestRepo(t, repo, true)
+	defer os.RemoveAll(newRepo.baseDir)
+	_, err := newRepo.ListTargets()
+	assert.NoError(t, err)
+
+	err = newRepo.RotateKey(data.CanonicalSnapshotRole, true)
+	assert.Error(t, err)
+	assert.IsType(t, signed.ErrNoKeys{}, err)
 }
 
 // If there is no local cache, notary operations return the remote error code

client/helpers.go

@@ -251,15 +251,6 @@ func getRemoteKey(url, gun, role string, rt http.RoundTripper) (data.PublicKey,
 	return remote.GetKey(role)
 }
 
-// Rotates a public key from a remote store, given a gun and role
-func rotateRemoteKey(url, gun, role string, rt http.RoundTripper) (data.PublicKey, error) {
-	remote, err := getRemoteStore(url, gun, rt)
-	if err != nil {
-		return nil, err
-	}
-	return remote.RotateKey(role)
-}
-
 // add a key to a KeyDB, and create a role for the key and add it.
 func addKeyForRole(kdb *keys.KeyDB, role string, key data.PublicKey) error {
 	theRole, err := data.NewRole(role, 1, []string{key.ID()}, nil, nil)

cmd/notary/keys.go

@@ -37,7 +37,7 @@ var cmdKeyListTemplate = usageTemplate{
 var cmdRotateKeyTemplate = usageTemplate{
 	Use:   "rotate [ GUN ]",
 	Short: "Rotate the signing (non-root) keys for the given Globally Unique Name.",
-	Long:  "Removes all the old signing (non-root) keys for the given Globally Unique Name, and generates new ones.  This only makes local changes - please use then `notary publish` to push the key rotation changes to the remote server.",
+	Long:  "Removes old signing (non-root) keys for the given Globally Unique Name, and generates new ones.  If rotating to a server-managed key, the key rotation is automatically published.  If rotating to locally-managed key(s), only local, non-online changes are made - please use then `notary publish` to push the key rotation changes to the remote server.",
 }
 
 var cmdKeyGenerateRootKeyTemplate = usageTemplate{
@@ -361,13 +361,11 @@ func (k *keyCommander) keysRotate(cmd *cobra.Command, args []string) error {
 		rolesToRotate = []string{data.CanonicalSnapshotRole}
 	case data.CanonicalTargetsRole:
 		rolesToRotate = []string{data.CanonicalTargetsRole}
+	case data.CanonicalTimestampRole:
+		rolesToRotate = []string{data.CanonicalTimestampRole}
 	default:
 		return fmt.Errorf("key rotation not supported for %s keys", k.rotateKeyRole)
 	}
-	if k.rotateKeyServerManaged && rotateKeyRole != data.CanonicalSnapshotRole {
-		return fmt.Errorf(
-			"remote signing/key management is only supported for the snapshot key")
-	}
 
 	config, err := k.configGetter()
 	if err != nil {

cmd/notary/keys_test.go

@@ -11,10 +11,16 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/docker/go/canonical/json"
+	"golang.org/x/net/context"
+
+	"github.com/Sirupsen/logrus"
+	ctxu "github.com/docker/distribution/context"
 	"github.com/docker/notary"
 	"github.com/docker/notary/client"
+	"github.com/docker/notary/cryptoservice"
 	"github.com/docker/notary/passphrase"
+	"github.com/docker/notary/server"
+	"github.com/docker/notary/server/storage"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 	"github.com/spf13/cobra"
@@ -227,11 +233,10 @@ func TestRemoveMultikeysRemoveOnlyChosenKey(t *testing.T) {
 	}
 }
 
-// Non-roles, root, and timestamp can't be rotated
+// Non-roles, and root can't be rotated
 func TestRotateKeyInvalidRoles(t *testing.T) {
 	invalids := []string{
 		data.CanonicalRootRole,
-		data.CanonicalTimestampRole,
 		"notevenARole",
 	}
 	for _, role := range invalids {
@@ -260,8 +265,20 @@ func TestRotateKeyTargetCannotBeServerManaged(t *testing.T) {
 	}
 	err := k.keysRotate(&cobra.Command{}, []string{"gun"})
 	assert.Error(t, err)
-	assert.Contains(t, err.Error(),
-		"remote signing/key management is only supported for the snapshot key")
+	assert.IsType(t, client.ErrInvalidRemoteRole{}, err)
+}
+
+// Cannot rotate a timestamp key and require that it is locally managed it
+func TestRotateKeyTimestampCannotBeLocallyManaged(t *testing.T) {
+	k := &keyCommander{
+		configGetter:           func() (*viper.Viper, error) { return viper.New(), nil },
+		getRetriever:           func() passphrase.Retriever { return passphrase.ConstantRetriever("pass") },
+		rotateKeyRole:          data.CanonicalTimestampRole,
+		rotateKeyServerManaged: false,
+	}
+	err := k.keysRotate(&cobra.Command{}, []string{"gun"})
+	assert.Error(t, err)
+	assert.IsType(t, client.ErrInvalidLocalRole{}, err)
 }
 
 // rotate key must be provided with a gun
@@ -280,17 +297,23 @@ func TestRotateKeyNoGUN(t *testing.T) {
 func setUpRepo(t *testing.T, tempBaseDir, gun string, ret passphrase.Retriever) (
 	*httptest.Server, map[string]string) {
 
-	// server that always returns 200 (and a key)
-	key, err := trustmanager.GenerateECDSAKey(rand.Reader)
-	assert.NoError(t, err)
-	pubKey := data.PublicKeyFromPrivate(key)
-	jsonBytes, err := json.MarshalCanonical(&pubKey)
-	assert.NoError(t, err)
-	keyJSON := string(jsonBytes)
-	ts := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, r *http.Request) {
-			fmt.Fprint(w, keyJSON)
-		}))
+	// Set up server
+	ctx := context.WithValue(
+		context.Background(), "metaStore", storage.NewMemStorage())
+
+	// Do not pass one of the const KeyAlgorithms here as the value! Passing a
+	// string is in itself good test that we are handling it correctly as we
+	// will be receiving a string from the configuration.
+	ctx = context.WithValue(ctx, "keyAlgorithm", "ecdsa")
+
+	// Eat the logs instead of spewing them out
+	l := logrus.New()
+	l.Out = bytes.NewBuffer(nil)
+	ctx = ctxu.WithLogger(ctx, logrus.NewEntry(l))
+
+	cryptoService := cryptoservice.NewCryptoService(
+		"", trustmanager.NewKeyMemoryStore(ret))
+	ts := httptest.NewServer(server.RootHandler(nil, ctx, cryptoService))
 
 	repo, err := client.NewNotaryRepository(
 		tempBaseDir, gun, ts.URL, http.DefaultTransport, ret)
@@ -309,39 +332,59 @@ func setUpRepo(t *testing.T, tempBaseDir, gun string, ret passphrase.Retriever)
 // that the correct config variables are passed for the client to request a key
 // from the remote server.
 func TestRotateKeyRemoteServerManagesKey(t *testing.T) {
-	// Temporary directory where test files will be created
-	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
-	defer os.RemoveAll(tempBaseDir)
-	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
-	gun := "docker.com/notary"
-
-	ret := passphrase.ConstantRetriever("pass")
+	for _, role := range []string{data.CanonicalSnapshotRole, data.CanonicalTimestampRole} {
+		// Temporary directory where test files will be created
+		tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
+		defer os.RemoveAll(tempBaseDir)
+		assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+		gun := "docker.com/notary"
+
+		ret := passphrase.ConstantRetriever("pass")
+
+		ts, initialKeys := setUpRepo(t, tempBaseDir, gun, ret)
+		defer ts.Close()
+
+		k := &keyCommander{
+			configGetter: func() (*viper.Viper, error) {
+				v := viper.New()
+				v.SetDefault("trust_dir", tempBaseDir)
+				v.SetDefault("remote_server.url", ts.URL)
+				return v, nil
+			},
+			getRetriever:           func() passphrase.Retriever { return ret },
+			rotateKeyRole:          role,
+			rotateKeyServerManaged: true,
+		}
+		err = k.keysRotate(&cobra.Command{}, []string{gun})
+		assert.NoError(t, err)
 
-	ts, initialKeys := setUpRepo(t, tempBaseDir, gun, ret)
-	defer ts.Close()
+		repo, err := client.NewNotaryRepository(tempBaseDir, gun, ts.URL, nil, ret)
+		assert.NoError(t, err, "error creating repo: %s", err)
+
+		cl, err := repo.GetChangelist()
+		assert.NoError(t, err, "unable to get changelist: %v", err)
+		assert.Len(t, cl.List(), 0, "The key rotation should have been published")
+
+		finalKeys := repo.CryptoService.ListAllKeys()
+		assert.Len(t, initialKeys, 3)
+		// no keys have been created, since a remote key was specified
+		if role == data.CanonicalSnapshotRole {
+			assert.Len(t, finalKeys, 2)
+			for k, r := range initialKeys {
+				if r != data.CanonicalSnapshotRole {
+					_, ok := finalKeys[k]
+					assert.True(t, ok)
+				}
+			}
+		} else {
+			assert.Len(t, finalKeys, 3)
+			for k := range initialKeys {
+				_, ok := finalKeys[k]
+				assert.True(t, ok)
+			}
+		}
 
-	k := &keyCommander{
-		configGetter: func() (*viper.Viper, error) {
-			v := viper.New()
-			v.SetDefault("trust_dir", tempBaseDir)
-			v.SetDefault("remote_server.url", ts.URL)
-			return v, nil
-		},
-		getRetriever:           func() passphrase.Retriever { return ret },
-		rotateKeyRole:          data.CanonicalSnapshotRole,
-		rotateKeyServerManaged: true,
 	}
-	err = k.keysRotate(&cobra.Command{}, []string{gun})
-	assert.NoError(t, err)
-
-	repo, err := client.NewNotaryRepository(tempBaseDir, gun, ts.URL, nil, ret)
-	assert.NoError(t, err, "error creating repo: %s", err)
-
-	cl, err := repo.GetChangelist()
-	assert.NoError(t, err, "unable to get changelist: %v", err)
-	assert.Len(t, cl.List(), 1)
-	// no keys have been created, since a remote key was specified
-	assert.Equal(t, initialKeys, repo.CryptoService.ListAllKeys())
 }
 
 // The command line uses NotaryRepository's RotateKey - this is just testing