diff --git a/client/client.go b/client/client.go index 774da357fb..72b2529340 100644 --- a/client/client.go +++ b/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 { - return fmt.Errorf( - "notary does not currently support rotating the %s key", role) - } - if serverManagesKey && role == data.CanonicalTargetsRole { - return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole} + switch { + // We currently support locally managing targets keys, remotely managing + // timestamp keys, and locally or remotely managing snapshot keys. + case role == data.CanonicalTargetsRole: + if serverManagesKey { + 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 - err error + pubKey data.PublicKey + 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) diff --git a/client/client_test.go b/client/client_test.go index 6083dc21fd..8f40bd91be 100644 --- a/client/client_test.go +++ b/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), - // (timestamp, false), (targets, true) + // the equivalent of: (root, true), (root, false), (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, diff --git a/cmd/notary/keys.go b/cmd/notary/keys.go index e461a8ba9d..3cb9cf4c23 100644 --- a/cmd/notary/keys.go +++ b/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 { diff --git a/cmd/notary/keys_test.go b/cmd/notary/keys_test.go index bbb7bfb696..308b43f255 100644 --- a/cmd/notary/keys_test.go +++ b/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(), - "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 +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) - 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 +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 - 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" + 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") + ret := passphrase.ConstantRetriever("pass") - ts, initialKeys := setUpRepo(t, tempBaseDir, gun, ret) - defer ts.Close() + 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) + + 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