Update the CLI and client to no longer reject remote timestamp rotations.

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

client/client.go

@@ -46,7 +46,18 @@ type ErrInvalidRemoteRole struct {
 
 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
+// an unsupported key type
+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
@@ -543,11 +554,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
@@ -640,7 +650,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 {
 	tufRepo := tuf.NewRepo(r.CryptoService)
 
@@ -858,25 +868,36 @@ 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 {
+	switch {
-		return fmt.Errorf(
+	// We currently support locally managing targets keys, remotely managing
-			"notary does not currently support rotating the %s key", role)
+	// timestamp keys, and locally or remotely managing snapshot keys.
-	}
+	case role == data.CanonicalTargetsRole:
-	if serverManagesKey && role == data.CanonicalTargetsRole {
+		if serverManagesKey {
-		return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole}
+			return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole}
+		}
+	case role == data.CanonicalTimestampRole:
+		if !serverManagesKey {
+			return ErrInvalidLocalRole{Role: data.CanonicalTimestampRole}
+		}
+	case role == data.CanonicalSnapshotRole:
+	default:
+		return fmt.Errorf("notary does not currently permit rotating the %s key", role)
 	}
 
 	var (
-		pubKey data.PublicKey
+		pubKey       data.PublicKey
-		err    error
+		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"
 	}
 	if err != nil {
-		return err
+		return fmt.Errorf(errFmtString, err)
 	}
 
 	return r.rootFileKeyChange(role, changelist.ActionCreate, pubKey)

client/client_test.go

@@ -21,6 +21,7 @@ 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"
 
@@ -247,7 +248,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 +2554,20 @@ 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),
+	// the equivalent of: (root, true), (root, false), (timestamp, false), (targets, true)
-	// (timestamp, false), (targets, true)
 	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,6 +2576,21 @@ 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)
+	assert.Error(t, err)
+	assert.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,

cmd/notary/keys.go

@@ -38,7 +38,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{
@@ -420,13 +420,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,12 +233,12 @@ func TestRemoveMultikeysRemoveOnlyChosenKey(t *testing.T) {
 	}
 }
 
-// Non-roles, root, and timestamp can't be rotated
+// Non-roles, root, and delegation keys can't be rotated with this command line
 func TestRotateKeyInvalidRoles(t *testing.T) {
 	invalids := []string{
 		data.CanonicalRootRole,
-		data.CanonicalTimestampRole,
 		"notevenARole",
+		"targets/a",
 	}
 	for _, role := range invalids {
 		for _, serverManaged := range []bool{true, false} {
@@ -260,8 +266,20 @@ func TestRotateKeyTargetCannotBeServerManaged(t *testing.T) {
 	}
 	err := k.keysRotate(&cobra.Command{}, []string{"gun"})
 	assert.Error(t, err)
-	assert.Contains(t, err.Error(),
+	assert.IsType(t, client.ErrInvalidRemoteRole{}, err)
-		"remote signing/key management is only supported for the snapshot key")
+}
+
+// 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 +298,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)
+	// Set up server
-	key, err := trustmanager.GenerateECDSAKey(rand.Reader)
+	ctx := context.WithValue(
-	assert.NoError(t, err)
+		context.Background(), "metaStore", storage.NewMemStorage())
-	pubKey := data.PublicKeyFromPrivate(key)
+
-	jsonBytes, err := json.MarshalCanonical(&pubKey)
+	// Do not pass one of the const KeyAlgorithms here as the value! Passing a
-	assert.NoError(t, err)
+	// string is in itself good test that we are handling it correctly as we
-	keyJSON := string(jsonBytes)
+	// will be receiving a string from the configuration.
-	ts := httptest.NewServer(http.HandlerFunc(
+	ctx = context.WithValue(ctx, "keyAlgorithm", "ecdsa")
-		func(w http.ResponseWriter, r *http.Request) {
+
-			fmt.Fprint(w, keyJSON)
+	// 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 +333,61 @@ 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
+	for _, role := range []string{data.CanonicalSnapshotRole, data.CanonicalTimestampRole} {
-	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
+		// Temporary directory where test files will be created
-	defer os.RemoveAll(tempBaseDir)
+		tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
-	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+		defer os.RemoveAll(tempBaseDir)
-	gun := "docker.com/notary"
+		assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+		gun := "docker.com/notary"
 
-	ret := passphrase.ConstantRetriever("pass")
+		ret := passphrase.ConstantRetriever("pass")
 
-	ts, initialKeys := setUpRepo(t, tempBaseDir, gun, ret)
+		ts, initialKeys := setUpRepo(t, tempBaseDir, gun, ret)
-	defer ts.Close()
+		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)
+
+		repo, err := client.NewNotaryRepository(tempBaseDir, gun, ts.URL, http.DefaultTransport, 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, "expected a single key rotation change")
+
+		assert.NoError(t, repo.Publish())
+
+		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