From a8ddcdfc9844704f037de9747eed61d29834e587 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 8 Feb 2016 14:49:56 -0800 Subject: [PATCH] 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 --- client/backwards_compatibility_test.go | 10 +- client/client.go | 45 +++++++-- client/client_test.go | 51 ++++------ client/helpers.go | 9 -- cmd/notary/keys.go | 8 +- cmd/notary/keys_test.go | 135 ++++++++++++++++--------- server/handlers/default.go | 6 +- server/handlers/default_test.go | 17 ---- server/server_test.go | 5 +- 9 files changed, 159 insertions(+), 127 deletions(-) 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,