diff --git a/client/backwards_compatibility_test.go b/client/backwards_compatibility_test.go index 578348b320..ea513cd4ec 100644 --- a/client/backwards_compatibility_test.go +++ b/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() diff --git a/client/client.go b/client/client.go index cf5c8e5f0c..1100147788 100644 --- a/client/client.go +++ b/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 { diff --git a/client/client_test.go b/client/client_test.go index 44bee358f3..a01e6102e8 100644 --- a/client/client_test.go +++ b/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 diff --git a/client/helpers.go b/client/helpers.go index 237bf11d41..bee7147b8b 100644 --- a/client/helpers.go +++ b/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) diff --git a/cmd/notary/keys.go b/cmd/notary/keys.go index 353ec750c0..5ddfdd3b35 100644 --- a/cmd/notary/keys.go +++ b/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 { diff --git a/cmd/notary/keys_test.go b/cmd/notary/keys_test.go index 95bf0979cd..e411319c9f 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,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 diff --git a/server/handlers/default.go b/server/handlers/default.go index 97a04b3bbd..d17173fa41 100644 --- a/server/handlers/default.go +++ b/server/handlers/default.go @@ -208,11 +208,7 @@ func parseKeyParams(ctx context.Context, vars map[string]string) (*serverKeyInfo } func rotateKeyHandler(ctx context.Context, w io.Writer, vars map[string]string) error { - if _, err := parseKeyParams(ctx, vars); err != nil { - return err - } - // Not implemented yet. - return errors.ErrCannotRotateKey.WithDetail(nil) + return getKeyHandler(ctx, w, vars) } func getKeyHandler(ctx context.Context, w io.Writer, vars map[string]string) error { diff --git a/server/handlers/default_test.go b/server/handlers/default_test.go index 7eb50dd6b6..92575e8861 100644 --- a/server/handlers/default_test.go +++ b/server/handlers/default_test.go @@ -179,23 +179,6 @@ func TestGetKeyHandlerCreatesOnce(t *testing.T) { } } -// If we cannot rotate the key, we get an error cannot rotate key back -func TestRotateKeyHandlerCannotRotateKey(t *testing.T) { - state := defaultState() - roles := []string{data.CanonicalTimestampRole, data.CanonicalSnapshotRole} - - for _, role := range roles { - vars := map[string]string{"imageName": "gun", "tufRole": role} - var buf bytes.Buffer - err := rotateKeyHandler(getContext(state), &buf, vars) - assert.Error(t, err) - assert.Empty(t, len(buf.Bytes())) - errCode, ok := err.(errcode.Error) - assert.True(t, ok) - assert.Equal(t, errors.ErrCannotRotateKey, errCode.Code) - } -} - func TestGetHandlerRoot(t *testing.T) { metaStore := storage.NewMemStorage() _, repo, _, err := testutils.EmptyRepo("gun") diff --git a/server/server_test.go b/server/server_test.go index dbc818124c..4971425efc 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -14,7 +14,6 @@ import ( "testing" _ "github.com/docker/distribution/registry/auth/silly" - "github.com/docker/notary" "github.com/docker/notary/server/storage" "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/signed" @@ -103,8 +102,8 @@ func TestRotateKeyEndpoint(t *testing.T) { defer ts.Close() rolesToStatus := map[string]int{ - data.CanonicalTimestampRole: notary.HTTPStatusTooManyRequests, // not implemented yet - data.CanonicalSnapshotRole: notary.HTTPStatusTooManyRequests, // not implemented yet + data.CanonicalTimestampRole: http.StatusOK, // just returning same as GetKey endpoint right now + data.CanonicalSnapshotRole: http.StatusOK, // just returning same as GetKey endpoint right now data.CanonicalTargetsRole: http.StatusNotFound, data.CanonicalRootRole: http.StatusNotFound, "somerandomrole": http.StatusNotFound,