From cc7210848c47b7f32605a31b055950bce128eefc Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 1 Feb 2016 12:46:34 -0800 Subject: [PATCH 1/8] Add a server endpoint to force rotate remote-managed keys. Signed-off-by: Ying Li --- server/errors/errors.go | 6 ++ server/handlers/default.go | 83 +++++++++++++++++-------- server/handlers/default_test.go | 105 ++++++++++++++++++++------------ server/server.go | 5 ++ server/server_test.go | 29 +++++++++ 5 files changed, 161 insertions(+), 67 deletions(-) diff --git a/server/errors/errors.go b/server/errors/errors.go index ad92ccf89..e3a09f98b 100644 --- a/server/errors/errors.go +++ b/server/errors/errors.go @@ -86,5 +86,11 @@ var ( Description: "No key algorihtm has been configured for the server and it has been asked to perform an operation that requires generation.", HTTPStatusCode: http.StatusInternalServerError, }) + ErrCannotRotateKey = errcode.Register(errGroup, errcode.ErrorDescriptor{ + Value: "CANNOT_ROTATE_KEY", + Message: "Key has already been rotated recently.", + Description: "The key has been rotated too recently, and cannot be rotated again at this time.", + HTTPStatusCode: 429, // 429 is Too Many Requests - this will be added as a status constant in Go 1.6 + }) ErrUnknown = errcode.ErrorCodeUnknown ) diff --git a/server/handlers/default.go b/server/handlers/default.go index 96a601412..97a04b3bb 100644 --- a/server/handlers/default.go +++ b/server/handlers/default.go @@ -143,70 +143,99 @@ func DeleteHandler(ctx context.Context, w http.ResponseWriter, r *http.Request) return nil } +// RotateKeyHandler returns a new public key for the specified role, creating a new key-pair +// if one has not be rotated yet within a certain time period. +func RotateKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Request) error { + defer r.Body.Close() + vars := mux.Vars(r) + return rotateKeyHandler(ctx, w, vars) +} + // GetKeyHandler returns a public key for the specified role, creating a new key-pair // it if it doesn't yet exist func GetKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Request) error { defer r.Body.Close() vars := mux.Vars(r) - return getKeyHandler(ctx, w, r, vars) + return getKeyHandler(ctx, w, vars) +} + +type serverKeyInfo struct { + gun string + role string + store storage.MetaStore + crypto signed.CryptoService + keyAlgo string } -func getKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { +func parseKeyParams(ctx context.Context, vars map[string]string) (*serverKeyInfo, error) { gun, ok := vars["imageName"] if !ok || gun == "" { - return errors.ErrUnknown.WithDetail("no gun") + return nil, errors.ErrUnknown.WithDetail("no gun") } role, ok := vars["tufRole"] if !ok || role == "" { - return errors.ErrUnknown.WithDetail("no role") + return nil, errors.ErrUnknown.WithDetail("no role") + } + if role != data.CanonicalTimestampRole && role != data.CanonicalSnapshotRole { + return nil, errors.ErrInvalidRole.WithDetail(role) } - - logger := ctxu.GetLoggerWithField(ctx, gun, "gun") s := ctx.Value("metaStore") store, ok := s.(storage.MetaStore) if !ok || store == nil { - logger.Error("500 GET storage not configured") - return errors.ErrNoStorage.WithDetail(nil) + return nil, errors.ErrNoStorage.WithDetail(nil) } + c := ctx.Value("cryptoService") crypto, ok := c.(signed.CryptoService) if !ok || crypto == nil { - logger.Error("500 GET crypto service not configured") - return errors.ErrNoCryptoService.WithDetail(nil) + return nil, errors.ErrNoCryptoService.WithDetail(nil) } + algo := ctx.Value("keyAlgorithm") keyAlgo, ok := algo.(string) - if !ok || keyAlgo == "" { - logger.Error("500 GET key algorithm not configured") - return errors.ErrNoKeyAlgorithm.WithDetail(nil) + if !ok || keyAlgo != data.ECDSAKey && keyAlgo != data.RSAKey && keyAlgo != data.ED25519Key { + return nil, errors.ErrNoKeyAlgorithm.WithDetail(nil) + } + + return &serverKeyInfo{ + gun: gun, + role: role, + store: store, + crypto: crypto, + keyAlgo: keyAlgo, + }, nil +} + +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) +} + +func getKeyHandler(ctx context.Context, w io.Writer, vars map[string]string) error { + s, err := parseKeyParams(ctx, vars) + if err != nil { + return err } - keyAlgorithm := keyAlgo + var key data.PublicKey - var ( - key data.PublicKey - err error - ) - switch role { + switch s.role { // parseKeyParams ensures it's only timestamp or snapshot case data.CanonicalTimestampRole: - key, err = timestamp.GetOrCreateTimestampKey(gun, store, crypto, keyAlgorithm) + key, err = timestamp.GetOrCreateTimestampKey(s.gun, s.store, s.crypto, s.keyAlgo) case data.CanonicalSnapshotRole: - key, err = snapshot.GetOrCreateSnapshotKey(gun, store, crypto, keyAlgorithm) - default: - logger.Errorf("400 GET %s key: %v", role, err) - return errors.ErrInvalidRole.WithDetail(role) + key, err = snapshot.GetOrCreateSnapshotKey(s.gun, s.store, s.crypto, s.keyAlgo) } if err != nil { - logger.Errorf("500 GET %s key: %v", role, err) return errors.ErrUnknown.WithDetail(err) } out, err := json.Marshal(key) if err != nil { - logger.Errorf("500 GET %s key", role) return errors.ErrUnknown.WithDetail(err) } - logger.Debugf("200 GET %s key", role) w.Write(out) return nil } diff --git a/server/handlers/default_test.go b/server/handlers/default_test.go index 4669b90e8..7eb50dd6b 100644 --- a/server/handlers/default_test.go +++ b/server/handlers/default_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "io" "io/ioutil" "net/http" "net/http/httptest" @@ -19,10 +20,10 @@ import ( "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/store" "github.com/docker/notary/tuf/validation" + "github.com/stretchr/testify/assert" "github.com/docker/notary/tuf/testutils" "github.com/docker/notary/utils" - "github.com/stretchr/testify/assert" ) type handlerState struct { @@ -75,9 +76,11 @@ func TestMainHandlerNotGet(t *testing.T) { } } -// GetKeyHandler needs to have access to a metadata store and cryptoservice, -// a key algorithm -func TestGetKeyHandlerInvalidConfiguration(t *testing.T) { +type simplerHandler func(context.Context, io.Writer, map[string]string) error + +// GetKeyHandler and RotateKeyHandler needs to have access to a metadata store +// and cryptoservice, a key algorithm +func TestKeyHandlersInvalidConfiguration(t *testing.T) { noStore := defaultState() noStore.store = nil @@ -106,68 +109,90 @@ func TestGetKeyHandlerInvalidConfiguration(t *testing.T) { "imageName": "gun", "tufRole": data.CanonicalTimestampRole, } - req := &http.Request{Body: ioutil.NopCloser(bytes.NewBuffer(nil))} - for errString, states := range invalidStates { - for _, s := range states { - err := getKeyHandler(getContext(s), httptest.NewRecorder(), req, vars) - assert.Error(t, err) - assert.Contains(t, err.Error(), errString) + var buf bytes.Buffer + for _, keyHandler := range []simplerHandler{getKeyHandler, rotateKeyHandler} { + for errString, states := range invalidStates { + for _, s := range states { + err := keyHandler(getContext(s), &buf, vars) + assert.Error(t, err) + assert.Contains(t, err.Error(), errString) + } } } } -// GetKeyHandler needs to be set up such that an imageName and tufRole are both -// provided and non-empty. -func TestGetKeyHandlerNoRoleOrRepo(t *testing.T) { +// GetKeyHandler and RotateKeyHandler needs to be set up such that an imageName +// and tufRole are both provided and non-empty. +func TestKeyHandlersNoRoleOrRepo(t *testing.T) { state := defaultState() - req := &http.Request{Body: ioutil.NopCloser(bytes.NewBuffer(nil))} - - for _, key := range []string{"imageName", "tufRole"} { - vars := map[string]string{ - "imageName": "gun", - "tufRole": data.CanonicalTimestampRole, - } - // not provided - delete(vars, key) - err := getKeyHandler(getContext(state), httptest.NewRecorder(), req, vars) - assert.Error(t, err) - assert.Contains(t, err.Error(), "unknown") + for _, keyHandler := range []simplerHandler{getKeyHandler, rotateKeyHandler} { + for _, key := range []string{"imageName", "tufRole"} { + vars := map[string]string{ + "imageName": "gun", + "tufRole": data.CanonicalTimestampRole, + } + var buf bytes.Buffer + + // not provided + delete(vars, key) + err := keyHandler(getContext(state), &buf, vars) + assert.Error(t, err) + assert.Contains(t, err.Error(), "unknown") - // empty - vars[key] = "" - err = getKeyHandler(getContext(state), httptest.NewRecorder(), req, vars) - assert.Error(t, err) - assert.Contains(t, err.Error(), "unknown") + // empty + vars[key] = "" + err = keyHandler(getContext(state), &buf, vars) + assert.Error(t, err) + assert.Contains(t, err.Error(), "unknown") + } } } -// Getting a key for a non-supported role results in a 400. -func TestGetKeyHandlerInvalidRole(t *testing.T) { +// Getting or rotating a key for a non-supported role results in a 400. +func TestKeyHandlersInvalidRole(t *testing.T) { state := defaultState() vars := map[string]string{ "imageName": "gun", "tufRole": data.CanonicalRootRole, } - req := &http.Request{Body: ioutil.NopCloser(bytes.NewBuffer(nil))} + for _, keyHandler := range []simplerHandler{getKeyHandler, rotateKeyHandler} { + var buf bytes.Buffer - err := getKeyHandler(getContext(state), httptest.NewRecorder(), req, vars) - assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid role") + err := keyHandler(getContext(state), &buf, vars) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid role") + } } // Getting the key for a valid role and gun succeeds func TestGetKeyHandlerCreatesOnce(t *testing.T) { state := defaultState() roles := []string{data.CanonicalTimestampRole, data.CanonicalSnapshotRole} - req := &http.Request{Body: ioutil.NopCloser(bytes.NewBuffer(nil))} for _, role := range roles { vars := map[string]string{"imageName": "gun", "tufRole": role} - recorder := httptest.NewRecorder() - err := getKeyHandler(getContext(state), recorder, req, vars) + var buf bytes.Buffer + err := getKeyHandler(getContext(state), &buf, vars) assert.NoError(t, err) - assert.True(t, len(recorder.Body.String()) > 0) + assert.NotEmpty(t, buf.Bytes()) + } +} + +// 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) } } diff --git a/server/server.go b/server/server.go index 98d1d9c68..6a4bc9fff 100644 --- a/server/server.go +++ b/server/server.go @@ -102,6 +102,11 @@ func RootHandler(ac auth.AccessController, ctx context.Context, trust signed.Cry prometheus.InstrumentHandlerWithOpts( prometheusOpts("GetKey"), hand(handlers.GetKeyHandler, "push", "pull"))) + r.Methods("POST").Path( + "/v2/{imageName:.*}/_trust/tuf/{tufRole:snapshot|timestamp}.key").Handler( + prometheus.InstrumentHandlerWithOpts( + prometheusOpts("RotateKey"), + hand(handlers.RotateKeyHandler, "push", "pull"))) r.Methods("DELETE").Path("/v2/{imageName:.*}/_trust/tuf/").Handler( prometheus.InstrumentHandlerWithOpts( prometheusOpts("DeleteTuf"), diff --git a/server/server_test.go b/server/server_test.go index 81d9724f6..a45009bb2 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -1,6 +1,7 @@ package server import ( + "bytes" "crypto/sha256" "encoding/hex" "encoding/json" @@ -90,6 +91,34 @@ func TestGetKeysEndpoint(t *testing.T) { } } +// RotateKey supports only the timestamp and snapshot key endpoints +func TestRotateKeyEndpoint(t *testing.T) { + ctx := context.WithValue( + context.Background(), "metaStore", storage.NewMemStorage()) + ctx = context.WithValue(ctx, "keyAlgorithm", data.ED25519Key) + + handler := RootHandler(nil, ctx, signed.NewEd25519()) + ts := httptest.NewServer(handler) + defer ts.Close() + + rolesToStatus := map[string]int{ + data.CanonicalTimestampRole: 429, // not implemented yet + data.CanonicalSnapshotRole: 429, // not implemented yet + data.CanonicalTargetsRole: http.StatusNotFound, + data.CanonicalRootRole: http.StatusNotFound, + "somerandomrole": http.StatusNotFound, + } + + var buf bytes.Buffer + for role, expectedStatus := range rolesToStatus { + res, err := http.Post( + fmt.Sprintf("%s/v2/gun/_trust/tuf/%s.key", ts.URL, role), + "text/plain", &buf) + assert.NoError(t, err) + assert.Equal(t, expectedStatus, res.StatusCode) + } +} + // This just checks the URL routing is working correctly. // More detailed tests for this path including negative // tests are located in /server/handlers/ From e4bb888560609128070888128712b13eb09806fd Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 1 Feb 2016 15:26:04 -0800 Subject: [PATCH 2/8] Update the httpstore to be able to rotate keys. This also changes GetKey and RotateKey to return a data.PublicKey, rather than bytes. Signed-off-by: Ying Li --- tuf/store/httpstore.go | 34 +++++++++++-- tuf/store/httpstore_test.go | 93 +++++++++++++++++++++++++++++++++- tuf/store/interfaces.go | 5 +- tuf/store/memorystore.go | 11 ++-- tuf/store/memorystore_test.go | 7 +++ tuf/store/offlinestore.go | 9 +++- tuf/store/offlinestore_test.go | 4 ++ 7 files changed, 152 insertions(+), 11 deletions(-) diff --git a/tuf/store/httpstore.go b/tuf/store/httpstore.go index 8b0d85011..2e6c59021 100644 --- a/tuf/store/httpstore.go +++ b/tuf/store/httpstore.go @@ -24,6 +24,7 @@ import ( "github.com/Sirupsen/logrus" "github.com/docker/notary" + "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/validation" ) @@ -131,6 +132,8 @@ func translateStatusToError(resp *http.Response, resource string) error { return ErrMetaNotFound{Resource: resource} case http.StatusBadRequest: return tryUnmarshalError(resp, ErrInvalidOperation{}) + case 429: // HTTP over limit error - will be added as a constnat in Go 1.6 + return ErrInvalidOperation{fmt.Sprintf("%s rate limited", resource)} default: return ErrServerUnavailable{code: resp.StatusCode} } @@ -271,12 +274,29 @@ func (s HTTPStore) buildURL(uri string) (*url.URL, error) { } // GetKey retrieves a public key from the remote server -func (s HTTPStore) GetKey(role string) ([]byte, error) { +func (s HTTPStore) GetKey(role string) (data.PublicKey, error) { + return s.requestKey(role, false) +} + +// RotateKey rotates a key on the remote server and returns the new public key +func (s HTTPStore) RotateKey(role string) (data.PublicKey, error) { + return s.requestKey(role, true) +} + +func (s HTTPStore) requestKey(role string, rotate bool) (data.PublicKey, error) { url, err := s.buildKeyURL(role) if err != nil { return nil, err } - req, err := http.NewRequest("GET", url.String(), nil) + + method := "GET" + resource := role + " key" + if rotate { + method = "POST" + resource = resource + " rotation" + } + + req, err := http.NewRequest(method, url.String(), nil) if err != nil { return nil, err } @@ -285,12 +305,18 @@ func (s HTTPStore) GetKey(role string) ([]byte, error) { return nil, err } defer resp.Body.Close() - if err := translateStatusToError(resp, role+" key"); err != nil { + if err := translateStatusToError(resp, resource); err != nil { return nil, err } body, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, err } - return body, nil + + pubKey, err := data.UnmarshalPublicKey(body) + if err != nil { + return nil, err + } + + return pubKey, nil } diff --git a/tuf/store/httpstore_test.go b/tuf/store/httpstore_test.go index 7ce3fefb5..e24a9e6d9 100644 --- a/tuf/store/httpstore_test.go +++ b/tuf/store/httpstore_test.go @@ -7,17 +7,17 @@ import ( "fmt" "io" "io/ioutil" + "net" "net/http" "net/http/httptest" "strings" "testing" - "github.com/stretchr/testify/assert" - "github.com/docker/go/canonical/json" "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/validation" + "github.com/stretchr/testify/assert" ) const testRoot = `{"signed":{"_type":"Root","consistent_snapshot":false,"expires":"2025-07-17T16:19:21.101698314-07:00","keys":{"1ca15c7f4b2b0c6efce202a545e7267152da28ab7c91590b3b60bdb4da723aad":{"keytype":"ecdsa","keyval":{"private":null,"public":"MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEb0720c99Cj6ZmuDlznEZ52NA6YpeY9Sj45z51XvPnG63Bi2RSBezMJlPzbSfP39mXKXqOJyT+z9BZhi3FVWczg=="}},"b1d6813b55442ecbfb1f4b40eb1fcdb4290e53434cfc9ba2da24c26c9143873b":{"keytype":"ecdsa-x509","keyval":{"private":null,"public":"LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUJVekNCKzZBREFnRUNBaEFCWDNKLzkzaW8zbHcrZUsvNFhvSHhNQW9HQ0NxR1NNNDlCQU1DTUJFeER6QU4KQmdOVkJBTVRCbVY0Y0dseVpUQWVGdzB4TlRBM01qQXlNekU1TVRkYUZ3MHlOVEEzTVRjeU16RTVNVGRhTUJFeApEekFOQmdOVkJBTVRCbVY0Y0dseVpUQlpNQk1HQnlxR1NNNDlBZ0VHQ0NxR1NNNDlBd0VIQTBJQUJFTDhOTFhQCitreUJZYzhYY0FTMXB2S2l5MXRQUDlCZHJ1dEdrWlR3Z0dEYTM1THMzSUFXaWlrUmlPbGRuWmxVVEE5cG5JekoKOFlRQThhTjQ1TDQvUlplak5UQXpNQTRHQTFVZER3RUIvd1FFQXdJQW9EQVRCZ05WSFNVRUREQUtCZ2dyQmdFRgpCUWNEQXpBTUJnTlZIUk1CQWY4RUFqQUFNQW9HQ0NxR1NNNDlCQU1DQTBjQU1FUUNJRVJ1ZUVURG5xMlRqRFBmClhGRStqUFJqMEtqdXdEOG9HSmtoVGpMUDAycjhBaUI5cUNyL2ZqSXpJZ1NQcTJVSXZqR0hlYmZOYXh1QlpZZUUKYW8xNjd6dHNYZz09Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K"}},"fbddae7f25a6c23ca735b017206a849d4c89304a4d8de4dcc4b3d6f3eb22ce3b":{"keytype":"ecdsa","keyval":{"private":null,"public":"MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE/xS5fBHK2HKmlGcvAr06vwPITvmxWP4P3CMDCgY25iSaIiM21OiXA1/Uvo3Pa3xh5G3cwCtDvi+4FpflW2iB/w=="}},"fd75751f010c3442e23b3e3e99a1442a112f2f21038603cb8609d8b17c9e912a":{"keytype":"ed25519","keyval":{"private":null,"public":"rc+glN01m+q8jmX8SolGsjTfk6NMhUQTWyj10hjmne0="}}},"roles":{"root":{"keyids":["b1d6813b55442ecbfb1f4b40eb1fcdb4290e53434cfc9ba2da24c26c9143873b"],"threshold":1},"snapshot":{"keyids":["1ca15c7f4b2b0c6efce202a545e7267152da28ab7c91590b3b60bdb4da723aad"],"threshold":1},"targets":{"keyids":["fbddae7f25a6c23ca735b017206a849d4c89304a4d8de4dcc4b3d6f3eb22ce3b"],"threshold":1},"timestamp":{"keyids":["fd75751f010c3442e23b3e3e99a1442a112f2f21038603cb8609d8b17c9e912a"],"threshold":1}},"version":2},"signatures":[{"keyid":"b1d6813b55442ecbfb1f4b40eb1fcdb4290e53434cfc9ba2da24c26c9143873b","method":"ecdsa","sig":"A2lNVwxHBnD9ViFtRre8r5oG6VvcvJnC6gdvvxv/Jyag40q/fNMjllCqyHrb+6z8XDZcrTTDsFU1R3/e+92d1A=="}]}` @@ -324,3 +324,92 @@ func TestErrServerUnavailable(t *testing.T) { } } } + +// If successful, GetKey and RotateKey both return public keys +func TestGetKeyAndRotateKeySuccess(t *testing.T) { + c := signed.NewEd25519() + role := data.CanonicalSnapshotRole + key, err := c.Create(role, data.ED25519Key) + assert.NoError(t, err) + + keyJSON, err := json.Marshal(key) + assert.NoError(t, err) + + // Set up a simple handler and server for our store + handler := func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(keyJSON)) + } + server := httptest.NewServer(http.HandlerFunc(handler)) + defer server.Close() + store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport) + assert.NoError(t, err) + + gotten, err := store.GetKey(role) + assert.NoError(t, err) + assert.Equal(t, key.ID(), gotten.ID()) + + rotated, err := store.RotateKey(role) + assert.NoError(t, err) + assert.Equal(t, key.ID(), rotated.ID()) +} + +// GetKey and RotateKey both fail if the key returned is invalid JSON +func TestGetKeyAndRotateKeyInvalidJSON(t *testing.T) { + role := data.CanonicalSnapshotRole + + // Set up a simple handler and server for our store + handler := func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("{")) + } + server := httptest.NewServer(http.HandlerFunc(handler)) + defer server.Close() + store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport) + assert.NoError(t, err) + + _, err = store.GetKey(role) + assert.Error(t, err) + assert.IsType(t, &json.SyntaxError{}, err) + + _, err = store.RotateKey(role) + assert.Error(t, err) + assert.IsType(t, &json.SyntaxError{}, err) +} + +// GetKey and RotateKey both fail if it cannot make the request +func TestGetKeyAndRotateKeyServerUnreachable(t *testing.T) { + role := data.CanonicalSnapshotRole + + // nonexistent URL + store, err := NewHTTPStore("http://localhost:90861", "metadata", "json", "key", http.DefaultTransport) + assert.NoError(t, err) + + _, err = store.GetKey(role) + assert.Error(t, err) + assert.IsType(t, &net.OpError{}, err) + + _, err = store.RotateKey(role) + assert.Error(t, err) + assert.IsType(t, &net.OpError{}, err) +} + +// GetKey and RotateKey both fail with ErrInvalidOperation if a 429 is returned +func TestGetKeyAndRotateKeyServerLimitError(t *testing.T) { + role := data.CanonicalSnapshotRole + + // Set up a simple handler and server for our store + handler := func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(429) + } + server := httptest.NewServer(http.HandlerFunc(handler)) + defer server.Close() + store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport) + assert.NoError(t, err) + + _, err = store.GetKey(role) + assert.Error(t, err) + assert.IsType(t, ErrInvalidOperation{}, err) + + _, err = store.RotateKey(role) + assert.Error(t, err) + assert.IsType(t, ErrInvalidOperation{}, err) +} diff --git a/tuf/store/interfaces.go b/tuf/store/interfaces.go index dd307168b..9db9976f1 100644 --- a/tuf/store/interfaces.go +++ b/tuf/store/interfaces.go @@ -1,5 +1,7 @@ package store +import "github.com/docker/notary/tuf/data" + // MetadataStore must be implemented by anything that intends to interact // with a store of TUF files type MetadataStore interface { @@ -12,7 +14,8 @@ type MetadataStore interface { // PublicKeyStore must be implemented by a key service type PublicKeyStore interface { - GetKey(role string) ([]byte, error) + GetKey(role string) (data.PublicKey, error) + RotateKey(role string) (data.PublicKey, error) } // LocalStore represents a local TUF sture diff --git a/tuf/store/memorystore.go b/tuf/store/memorystore.go index 493bb6f0b..1288dde8f 100644 --- a/tuf/store/memorystore.go +++ b/tuf/store/memorystore.go @@ -94,9 +94,14 @@ func (m *MemoryStore) RemoveMeta(name string) error { return nil } -// GetKey returns the public key for the given role -func (m *MemoryStore) GetKey(role string) ([]byte, error) { - return nil, fmt.Errorf("GetKey is not implemented for the MemoryStore") +// GetKey returns the latest key for a given role +func (m *MemoryStore) GetKey(role string) (data.PublicKey, error) { + return nil, fmt.Errorf("GetKey is not implemented for the memoryStore") +} + +// RotateKey rotates a key for a given role +func (m *MemoryStore) RotateKey(role string) (data.PublicKey, error) { + return nil, fmt.Errorf("RotateKey is not implemented for the memoryStore") } // RemoveAll clears the existing memory store by setting this store as new empty one diff --git a/tuf/store/memorystore_test.go b/tuf/store/memorystore_test.go index 64eee5163..01650f63e 100644 --- a/tuf/store/memorystore_test.go +++ b/tuf/store/memorystore_test.go @@ -3,6 +3,7 @@ package store import ( "testing" + "github.com/docker/notary/tuf/data" "github.com/stretchr/testify/require" ) @@ -31,4 +32,10 @@ func TestMemoryStore(t *testing.T) { _, err = s.GetMeta("exists", 0) require.Error(t, err) require.IsType(t, ErrMetaNotFound{}, err) + + _, err = s.GetKey(data.CanonicalSnapshotRole) + require.Error(t, err) + + _, err = s.RotateKey(data.CanonicalSnapshotRole) + require.Error(t, err) } diff --git a/tuf/store/offlinestore.go b/tuf/store/offlinestore.go index b0f057b2b..51ef44d18 100644 --- a/tuf/store/offlinestore.go +++ b/tuf/store/offlinestore.go @@ -2,6 +2,8 @@ package store import ( "io" + + "github.com/docker/notary/tuf/data" ) // ErrOffline is used to indicate we are operating offline @@ -38,7 +40,12 @@ func (es OfflineStore) RemoveMeta(name string) error { } // GetKey returns ErrOffline -func (es OfflineStore) GetKey(role string) ([]byte, error) { +func (es OfflineStore) GetKey(role string) (data.PublicKey, error) { + return nil, err +} + +// RotateKey returns ErrOffline +func (es OfflineStore) RotateKey(role string) (data.PublicKey, error) { return nil, err } diff --git a/tuf/store/offlinestore_test.go b/tuf/store/offlinestore_test.go index 66de915e7..c60e38a68 100644 --- a/tuf/store/offlinestore_test.go +++ b/tuf/store/offlinestore_test.go @@ -24,6 +24,10 @@ func TestOfflineStore(t *testing.T) { require.Error(t, err) require.IsType(t, ErrOffline{}, err) + _, err = s.RotateKey("") + require.Error(t, err) + require.IsType(t, ErrOffline{}, err) + _, err = s.GetTarget("") require.Error(t, err) require.IsType(t, ErrOffline{}, err) From f23645113a919a3b5be958db9d4174778de520a1 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 1 Feb 2016 15:26:37 -0800 Subject: [PATCH 3/8] Update the client to attempt to use the remote rotation endpoint when rotating keys. Signed-off-by: Ying Li --- client/backwards_compatibility_test.go | 7 +- client/client.go | 28 +++++-- client/client_test.go | 101 ++++++++++++++++++------- client/helpers.go | 13 ++-- const.go | 3 + server/errors/errors.go | 5 +- server/server_test.go | 5 +- tuf/store/httpstore.go | 2 +- tuf/store/httpstore_test.go | 7 +- 9 files changed, 119 insertions(+), 52 deletions(-) diff --git a/client/backwards_compatibility_test.go b/client/backwards_compatibility_test.go index d78f313f9..578348b32 100644 --- a/client/backwards_compatibility_test.go +++ b/client/backwards_compatibility_test.go @@ -12,7 +12,6 @@ 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" @@ -108,11 +107,7 @@ func Test0Dot1RepoFormat(t *testing.T) { require.NoError(t, repo.fileStore.RemoveMeta(data.CanonicalTimestampRole)) // rotate the timestamp key, since the server doesn't have that 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.RotateKey(data.CanonicalTimestampRole, true)) require.NoError(t, repo.Publish()) targets, err = repo.ListTargets() diff --git a/client/client.go b/client/client.go index 444063d00..cf5c8e5f0 100644 --- a/client/client.go +++ b/client/client.go @@ -55,6 +55,17 @@ func (err ErrInvalidRemoteRole) Error() string { "notary does not support 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 support the client managing the %s key", err.Role) +} + // ErrRepositoryNotExist is returned when an action is taken on a remote // repository that doesn't exist type ErrRepositoryNotExist struct { @@ -829,25 +840,32 @@ 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 { + if role == data.CanonicalRootRole { return fmt.Errorf( "notary does not currently support rotating the %s key", role) } if serverManagesKey && role == data.CanonicalTargetsRole { return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole} } + if !serverManagesKey && role == data.CanonicalTimestampRole { + return ErrInvalidLocalRole{Role: data.CanonicalTargetsRole} + } var ( - pubKey data.PublicKey - err error + pubKey data.PublicKey + errFmtString string + err error ) if serverManagesKey { - pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip) + pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip, true) + errFmtString = "server unable to rotate key: %s" } else { pubKey, err = r.CryptoService.Create(role, data.ECDSAKey) + errFmtString = "unable to generate key: %s" } + if err != nil { - return err + return fmt.Errof(errFmtString, err) } return r.rootFileKeyChange(role, changelist.ActionCreate, pubKey) diff --git a/client/client_test.go b/client/client_test.go index 1803dd6b6..44bee358f 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/gorilla/mux" "github.com/stretchr/testify/assert" "golang.org/x/net/context" @@ -93,13 +94,13 @@ func (p *passRoleRecorder) assertAsked(t *testing.T, expected []string, args ... var passphraseRetriever = passphrase.ConstantRetriever(password) func simpleTestServer(t *testing.T, roles ...string) ( - *httptest.Server, *http.ServeMux, map[string]data.PrivateKey) { + *httptest.Server, *mux.Router, map[string]data.PrivateKey) { if len(roles) == 0 { roles = []string{data.CanonicalTimestampRole, data.CanonicalSnapshotRole} } keys := make(map[string]data.PrivateKey) - mux := http.NewServeMux() + m := mux.NewRouter() for _, role := range roles { key, err := trustmanager.GenerateECDSAKey(rand.Reader) @@ -112,15 +113,15 @@ func simpleTestServer(t *testing.T, roles ...string) ( keyJSON := string(jsonBytes) // TUF will request /v2/docker.com/notary/_trust/tuf/.key - mux.HandleFunc( - fmt.Sprintf("/v2/docker.com/notary/_trust/tuf/%s.key", role), + m.Methods("GET").Path( + fmt.Sprintf("/v2/docker.com/notary/_trust/tuf/%s.key", role)).HandlerFunc( func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, keyJSON) }) } - ts := httptest.NewServer(mux) - return ts, mux, keys + ts := httptest.NewServer(m) + return ts, m, keys } func fullTestServer(t *testing.T) *httptest.Server { @@ -1019,7 +1020,7 @@ func testListEmptyTargets(t *testing.T, rootType string) { // reads data from the repository in order to fake data being served via // the ServeMux. -func fakeServerData(t *testing.T, repo *NotaryRepository, mux *http.ServeMux, +func fakeServerData(t *testing.T, repo *NotaryRepository, m *mux.Router, keys map[string]data.PrivateKey) { timestampKey, ok := keys[data.CanonicalTimestampRole] @@ -1085,54 +1086,54 @@ func fakeServerData(t *testing.T, repo *NotaryRepository, mux *http.ServeMux, cksmBytes = sha256.Sum256(level2JSON) level2Checksum := hex.EncodeToString(cksmBytes[:]) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/root.json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/root.json", func(w http.ResponseWriter, r *http.Request) { assert.NoError(t, err) fmt.Fprint(w, string(rootFileBytes)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/root."+rootChecksum+".json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/root."+rootChecksum+".json", func(w http.ResponseWriter, r *http.Request) { assert.NoError(t, err) fmt.Fprint(w, string(rootFileBytes)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/timestamp.json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/timestamp.json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(timestampJSON)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/snapshot.json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/snapshot.json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(snapshotJSON)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/snapshot."+snapshotChecksum+".json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/snapshot."+snapshotChecksum+".json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(snapshotJSON)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets.json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets.json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(targetsJSON)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets."+targetsChecksum+".json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets."+targetsChecksum+".json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(targetsJSON)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level1.json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level1.json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(level1JSON)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level1."+level1Checksum+".json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level1."+level1Checksum+".json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(level1JSON)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level2.json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level2.json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(level2JSON)) }) - mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level2."+level2Checksum+".json", + m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level2."+level2Checksum+".json", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, string(level2JSON)) }) @@ -1146,7 +1147,7 @@ func (k targetSorter) Swap(i, j int) { k[i], k[j] = k[j], k[i] } func (k targetSorter) Less(i, j int) bool { return k[i].Name < k[j].Name } func testListTarget(t *testing.T, rootType string) { - ts, mux, keys := simpleTestServer(t) + ts, m, keys := simpleTestServer(t) defer ts.Close() repo, _ := initializeRepo(t, rootType, "docker.com/notary", ts.URL, false) @@ -1170,7 +1171,7 @@ func testListTarget(t *testing.T, rootType string) { err = applyChangelist(repo.tufRepo, cl) assert.NoError(t, err, "could not apply changelist") - fakeServerData(t, repo, mux, keys) + fakeServerData(t, repo, m, keys) targets, err := repo.ListTargets(data.CanonicalTargetsRole) assert.NoError(t, err) @@ -1202,7 +1203,7 @@ func testListTarget(t *testing.T, rootType string) { } func testListTargetWithDelegates(t *testing.T, rootType string) { - ts, mux, keys := simpleTestServer(t) + ts, m, keys := simpleTestServer(t) defer ts.Close() repo, _ := initializeRepo(t, rootType, "docker.com/notary", ts.URL, false) @@ -1253,7 +1254,7 @@ func testListTargetWithDelegates(t *testing.T, rootType string) { _, ok = repo.tufRepo.Targets["targets/level2"].Signed.Targets["level2"] assert.True(t, ok) - fakeServerData(t, repo, mux, keys) + fakeServerData(t, repo, m, keys) // test default listing targets, err := repo.ListTargets() @@ -2392,14 +2393,16 @@ 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 { + if role == data.CanonicalSnapshotRole { // remote or local can manage snapshot continue } for _, serverManagesKey := range []bool{true, false} { - if role == data.CanonicalTargetsRole && !serverManagesKey { + if role == data.CanonicalTargetsRole && !serverManagesKey { // only local can manage targets + continue + } + if role == data.CanonicalTimestampRole && serverManagesKey { // only remote can manage timestamp continue } err := repo.RotateKey(role, serverManagesKey) @@ -2567,6 +2570,52 @@ 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) + 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]) +} + +// 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) + + // simpleTestServer has no rotate key endpoint, so this should fail + assert.Error(t, repo.RotateKey(data.CanonicalTimestampRole, true)) +} + // If there is no local cache, notary operations return the remote error code func TestRemoteServerUnavailableNoLocalCache(t *testing.T) { tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-") diff --git a/client/helpers.go b/client/helpers.go index 41b0b686c..237bf11d4 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -248,17 +248,16 @@ func getRemoteKey(url, gun, role string, rt http.RoundTripper) (data.PublicKey, if err != nil { return nil, err } - rawPubKey, err := remote.GetKey(role) - if err != nil { - return nil, err - } + return remote.GetKey(role) +} - pubKey, err := data.UnmarshalPublicKey(rawPubKey) +// 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 pubKey, nil + return remote.RotateKey(role) } // add a key to a KeyDB, and create a role for the key and add it. diff --git a/const.go b/const.go index 3841bc8c8..b1c917554 100644 --- a/const.go +++ b/const.go @@ -18,4 +18,7 @@ const ( Sha256HexSize = 64 // TrustedCertsDir is the directory, under the notary repo base directory, where trusted certs are stored TrustedCertsDir = "trusted_certificates" + + // HTTPStatusTooManyRequests is the http status 429 - will be an exported constant in http library in Go 1.6 + HTTPStatusTooManyRequests = 429 ) diff --git a/server/errors/errors.go b/server/errors/errors.go index e3a09f98b..9c44a9bec 100644 --- a/server/errors/errors.go +++ b/server/errors/errors.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/docker/distribution/registry/api/errcode" + "github.com/docker/notary" ) // The notary API is on version 1, but URLs start with /v2/ to be consistent @@ -83,14 +84,14 @@ var ( ErrNoKeyAlgorithm = errcode.Register(errGroup, errcode.ErrorDescriptor{ Value: "NO_KEYALGORITHM", Message: "The server does not have a key algorithm configured.", - Description: "No key algorihtm has been configured for the server and it has been asked to perform an operation that requires generation.", + Description: "No key algorithm has been configured for the server and it has been asked to perform an operation that requires generation.", HTTPStatusCode: http.StatusInternalServerError, }) ErrCannotRotateKey = errcode.Register(errGroup, errcode.ErrorDescriptor{ Value: "CANNOT_ROTATE_KEY", Message: "Key has already been rotated recently.", Description: "The key has been rotated too recently, and cannot be rotated again at this time.", - HTTPStatusCode: 429, // 429 is Too Many Requests - this will be added as a status constant in Go 1.6 + HTTPStatusCode: notary.HTTPStatusTooManyRequests, }) ErrUnknown = errcode.ErrorCodeUnknown ) diff --git a/server/server_test.go b/server/server_test.go index a45009bb2..dbc818124 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -14,6 +14,7 @@ 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" @@ -102,8 +103,8 @@ func TestRotateKeyEndpoint(t *testing.T) { defer ts.Close() rolesToStatus := map[string]int{ - data.CanonicalTimestampRole: 429, // not implemented yet - data.CanonicalSnapshotRole: 429, // not implemented yet + data.CanonicalTimestampRole: notary.HTTPStatusTooManyRequests, // not implemented yet + data.CanonicalSnapshotRole: notary.HTTPStatusTooManyRequests, // not implemented yet data.CanonicalTargetsRole: http.StatusNotFound, data.CanonicalRootRole: http.StatusNotFound, "somerandomrole": http.StatusNotFound, diff --git a/tuf/store/httpstore.go b/tuf/store/httpstore.go index 2e6c59021..bbe7afe2d 100644 --- a/tuf/store/httpstore.go +++ b/tuf/store/httpstore.go @@ -132,7 +132,7 @@ func translateStatusToError(resp *http.Response, resource string) error { return ErrMetaNotFound{Resource: resource} case http.StatusBadRequest: return tryUnmarshalError(resp, ErrInvalidOperation{}) - case 429: // HTTP over limit error - will be added as a constnat in Go 1.6 + case notary.HTTPStatusTooManyRequests: return ErrInvalidOperation{fmt.Sprintf("%s rate limited", resource)} default: return ErrServerUnavailable{code: resp.StatusCode} diff --git a/tuf/store/httpstore_test.go b/tuf/store/httpstore_test.go index e24a9e6d9..fe4418353 100644 --- a/tuf/store/httpstore_test.go +++ b/tuf/store/httpstore_test.go @@ -14,6 +14,7 @@ import ( "testing" "github.com/docker/go/canonical/json" + "github.com/docker/notary" "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/validation" @@ -325,7 +326,7 @@ func TestErrServerUnavailable(t *testing.T) { } } -// If successful, GetKey and RotateKey both return public keys +// GetKey and RotateKey both succeed if valid public keys are returned from the server with a 200 status func TestGetKeyAndRotateKeySuccess(t *testing.T) { c := signed.NewEd25519() role := data.CanonicalSnapshotRole @@ -392,13 +393,13 @@ func TestGetKeyAndRotateKeyServerUnreachable(t *testing.T) { assert.IsType(t, &net.OpError{}, err) } -// GetKey and RotateKey both fail with ErrInvalidOperation if a 429 is returned +// GetKey and RotateKey both fail with ErrInvalidOperation if a notary.HTTPStatusTooManyRequests is returned func TestGetKeyAndRotateKeyServerLimitError(t *testing.T) { role := data.CanonicalSnapshotRole // Set up a simple handler and server for our store handler := func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(429) + w.WriteHeader(notary.HTTPStatusTooManyRequests) } server := httptest.NewServer(http.HandlerFunc(handler)) defer server.Close() From cc2ee80d541aaa2594de007876996559c04e6d8d Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 8 Feb 2016 12:26:14 -0800 Subject: [PATCH 4/8] Update the TUF remote store's RotateKey function to expect a CryptoService and root pubkeys. So that the rotate request gets signed. Signed-off-by: Ying Li --- tuf/store/httpstore.go | 46 +++++++++++++---- tuf/store/httpstore_test.go | 91 ++++++++++++++++++++++++++++++++-- tuf/store/interfaces.go | 7 ++- tuf/store/memorystore.go | 3 +- tuf/store/memorystore_test.go | 2 +- tuf/store/offlinestore.go | 3 +- tuf/store/offlinestore_test.go | 2 +- 7 files changed, 134 insertions(+), 20 deletions(-) diff --git a/tuf/store/httpstore.go b/tuf/store/httpstore.go index bbe7afe2d..9e378cc46 100644 --- a/tuf/store/httpstore.go +++ b/tuf/store/httpstore.go @@ -21,10 +21,12 @@ import ( "net/http" "net/url" "path" + "time" "github.com/Sirupsen/logrus" "github.com/docker/notary" "github.com/docker/notary/tuf/data" + "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/validation" ) @@ -273,17 +275,41 @@ func (s HTTPStore) buildURL(uri string) (*url.URL, error) { return s.baseURL.ResolveReference(sub), nil } -// GetKey retrieves a public key from the remote server +// GetKey retrieves the most recently created (whether it is signed in yet or not) +// public key for the given role from the remote server func (s HTTPStore) GetKey(role string) (data.PublicKey, error) { - return s.requestKey(role, false) + return s.requestKey(role, nil) } -// RotateKey rotates a key on the remote server and returns the new public key -func (s HTTPStore) RotateKey(role string) (data.PublicKey, error) { - return s.requestKey(role, true) +// RotateKey rotates a key on the remote server and returns the new public key. This requires +// a request with a short expiry time (to limit replay), signed by at least one root key. +// Signing with the root key proves that the client making the request has the capability of +// actually rotating the key, and is not just making a spurious rotation request. +func (s HTTPStore) RotateKey(role string, cs signed.CryptoService, roots ...data.PublicKey) (data.PublicKey, error) { + req := &data.SignedCommon{ + Type: role, + Expires: time.Now().Add(5 * time.Minute), + } + reqJSON, err := json.Marshal(req) + if err != nil { + return nil, err + } + signedReq := &data.Signed{Signed: reqJSON} + if err := signed.Sign(cs, signedReq, roots...); err != nil { + return nil, err + } + + requestBody, err := json.Marshal(signedReq) + if err != nil { + return nil, err + } + + return s.requestKey(role, bytes.NewBuffer(requestBody)) } -func (s HTTPStore) requestKey(role string, rotate bool) (data.PublicKey, error) { +// requestKey either sends a get or a post request, depending on whether there +// is a body. +func (s HTTPStore) requestKey(role string, body io.Reader) (data.PublicKey, error) { url, err := s.buildKeyURL(role) if err != nil { return nil, err @@ -291,12 +317,12 @@ func (s HTTPStore) requestKey(role string, rotate bool) (data.PublicKey, error) method := "GET" resource := role + " key" - if rotate { + if body != nil { method = "POST" resource = resource + " rotation" } - req, err := http.NewRequest(method, url.String(), nil) + req, err := http.NewRequest(method, url.String(), body) if err != nil { return nil, err } @@ -308,12 +334,12 @@ func (s HTTPStore) requestKey(role string, rotate bool) (data.PublicKey, error) if err := translateStatusToError(resp, resource); err != nil { return nil, err } - body, err := ioutil.ReadAll(resp.Body) + respBody, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, err } - pubKey, err := data.UnmarshalPublicKey(body) + pubKey, err := data.UnmarshalPublicKey(respBody) if err != nil { return nil, err } diff --git a/tuf/store/httpstore_test.go b/tuf/store/httpstore_test.go index fe4418353..363c8cfe5 100644 --- a/tuf/store/httpstore_test.go +++ b/tuf/store/httpstore_test.go @@ -12,10 +12,12 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/docker/go/canonical/json" "github.com/docker/notary" "github.com/docker/notary/tuf/data" + "github.com/docker/notary/tuf/keys" "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/validation" "github.com/stretchr/testify/assert" @@ -349,7 +351,7 @@ func TestGetKeyAndRotateKeySuccess(t *testing.T) { assert.NoError(t, err) assert.Equal(t, key.ID(), gotten.ID()) - rotated, err := store.RotateKey(role) + rotated, err := store.RotateKey(role, c, key) assert.NoError(t, err) assert.Equal(t, key.ID(), rotated.ID()) } @@ -358,6 +360,10 @@ func TestGetKeyAndRotateKeySuccess(t *testing.T) { func TestGetKeyAndRotateKeyInvalidJSON(t *testing.T) { role := data.CanonicalSnapshotRole + c := signed.NewEd25519() + root, err := c.Create(data.CanonicalRootRole, data.ED25519Key) + assert.NoError(t, err) + // Set up a simple handler and server for our store handler := func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("{")) @@ -371,7 +377,7 @@ func TestGetKeyAndRotateKeyInvalidJSON(t *testing.T) { assert.Error(t, err) assert.IsType(t, &json.SyntaxError{}, err) - _, err = store.RotateKey(role) + _, err = store.RotateKey(role, c, root) assert.Error(t, err) assert.IsType(t, &json.SyntaxError{}, err) } @@ -380,6 +386,10 @@ func TestGetKeyAndRotateKeyInvalidJSON(t *testing.T) { func TestGetKeyAndRotateKeyServerUnreachable(t *testing.T) { role := data.CanonicalSnapshotRole + c := signed.NewEd25519() + root, err := c.Create(data.CanonicalRootRole, data.ED25519Key) + assert.NoError(t, err) + // nonexistent URL store, err := NewHTTPStore("http://localhost:90861", "metadata", "json", "key", http.DefaultTransport) assert.NoError(t, err) @@ -388,7 +398,7 @@ func TestGetKeyAndRotateKeyServerUnreachable(t *testing.T) { assert.Error(t, err) assert.IsType(t, &net.OpError{}, err) - _, err = store.RotateKey(role) + _, err = store.RotateKey(role, c, root) assert.Error(t, err) assert.IsType(t, &net.OpError{}, err) } @@ -397,6 +407,10 @@ func TestGetKeyAndRotateKeyServerUnreachable(t *testing.T) { func TestGetKeyAndRotateKeyServerLimitError(t *testing.T) { role := data.CanonicalSnapshotRole + c := signed.NewEd25519() + root, err := c.Create(data.CanonicalRootRole, data.ED25519Key) + assert.NoError(t, err) + // Set up a simple handler and server for our store handler := func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(notary.HTTPStatusTooManyRequests) @@ -410,7 +424,76 @@ func TestGetKeyAndRotateKeyServerLimitError(t *testing.T) { assert.Error(t, err) assert.IsType(t, ErrInvalidOperation{}, err) - _, err = store.RotateKey(role) + _, err = store.RotateKey(role, c, root) assert.Error(t, err) assert.IsType(t, ErrInvalidOperation{}, err) } + +// RotateKey sends a signed request to the the server to rotate a key +func TestRotateKeySendsSignedRequest(t *testing.T) { + role := data.CanonicalSnapshotRole + + c := signed.NewEd25519() + root, err := c.Create(data.CanonicalRootRole, data.ED25519Key) + assert.NoError(t, err) + snapshot, err := c.Create(role, data.ED25519Key) + assert.NoError(t, err) + + keyJSON, err := json.Marshal(snapshot) + assert.NoError(t, err) + + kdb := keys.NewDB() + kdb.AddKey(root) + roleObj, err := data.NewRole(data.CanonicalRootRole, 1, []string{root.ID()}, nil, nil) + assert.NoError(t, err) + kdb.AddRole(roleObj) + + // Set up handler that asserts information about the request + m := http.NewServeMux() + m.HandleFunc("/metadata/snapshot.key", func(w http.ResponseWriter, r *http.Request) { + decoder := json.NewDecoder(r.Body) + s := data.Signed{} + assert.NoError(t, decoder.Decode(&s)) + assert.NoError(t, signed.VerifySignatures(&s, data.CanonicalRootRole, kdb)) + common := data.SignedCommon{} + assert.NoError(t, json.Unmarshal(s.Signed, &common)) + assert.Equal(t, data.CanonicalSnapshotRole, common.Type) + assert.True(t, common.Expires.After(time.Now())) + assert.False(t, common.Expires.After(time.Now().Add(5*time.Minute))) + + w.Write(keyJSON) + }) + server := httptest.NewServer(m) + defer server.Close() + + store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport) + assert.NoError(t, err) + + k, err := store.RotateKey(role, c, root) + assert.NoError(t, err) + assert.Equal(t, snapshot.ID(), k.ID()) +} + +// RotateKey fails to even send a request if we can't sign. +func TestRotateKeyDoesntSendRequestIfCannotSign(t *testing.T) { + role := data.CanonicalSnapshotRole + + c := signed.NewEd25519() + root, err := c.Create(data.CanonicalRootRole, data.ED25519Key) + assert.NoError(t, err) + + // Set up handler that asserts information about the request + m := http.NewServeMux() + m.HandleFunc("/metadata/snapshot.key", func(w http.ResponseWriter, r *http.Request) { + assert.Fail(t, "No request should ever have been made.") + }) + server := httptest.NewServer(m) + defer server.Close() + + store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport) + assert.NoError(t, err) + + _, err = store.RotateKey(role, signed.NewEd25519(), root) + assert.Error(t, err) + assert.IsType(t, signed.ErrNoKeys{}, err) +} diff --git a/tuf/store/interfaces.go b/tuf/store/interfaces.go index 9db9976f1..96bb3c3e4 100644 --- a/tuf/store/interfaces.go +++ b/tuf/store/interfaces.go @@ -1,6 +1,9 @@ package store -import "github.com/docker/notary/tuf/data" +import ( + "github.com/docker/notary/tuf/data" + "github.com/docker/notary/tuf/signed" +) // MetadataStore must be implemented by anything that intends to interact // with a store of TUF files @@ -15,7 +18,7 @@ type MetadataStore interface { // PublicKeyStore must be implemented by a key service type PublicKeyStore interface { GetKey(role string) (data.PublicKey, error) - RotateKey(role string) (data.PublicKey, error) + RotateKey(role string, cs signed.CryptoService, roots ...data.PublicKey) (data.PublicKey, error) } // LocalStore represents a local TUF sture diff --git a/tuf/store/memorystore.go b/tuf/store/memorystore.go index 1288dde8f..be91e7af3 100644 --- a/tuf/store/memorystore.go +++ b/tuf/store/memorystore.go @@ -6,6 +6,7 @@ import ( "github.com/docker/notary" "github.com/docker/notary/tuf/data" + "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/utils" ) @@ -100,7 +101,7 @@ func (m *MemoryStore) GetKey(role string) (data.PublicKey, error) { } // RotateKey rotates a key for a given role -func (m *MemoryStore) RotateKey(role string) (data.PublicKey, error) { +func (m *MemoryStore) RotateKey(string, signed.CryptoService, ...data.PublicKey) (data.PublicKey, error) { return nil, fmt.Errorf("RotateKey is not implemented for the memoryStore") } diff --git a/tuf/store/memorystore_test.go b/tuf/store/memorystore_test.go index 01650f63e..8d4dabfee 100644 --- a/tuf/store/memorystore_test.go +++ b/tuf/store/memorystore_test.go @@ -36,6 +36,6 @@ func TestMemoryStore(t *testing.T) { _, err = s.GetKey(data.CanonicalSnapshotRole) require.Error(t, err) - _, err = s.RotateKey(data.CanonicalSnapshotRole) + _, err = s.RotateKey(data.CanonicalSnapshotRole, nil) require.Error(t, err) } diff --git a/tuf/store/offlinestore.go b/tuf/store/offlinestore.go index 51ef44d18..5bc92c1a5 100644 --- a/tuf/store/offlinestore.go +++ b/tuf/store/offlinestore.go @@ -4,6 +4,7 @@ import ( "io" "github.com/docker/notary/tuf/data" + "github.com/docker/notary/tuf/signed" ) // ErrOffline is used to indicate we are operating offline @@ -45,7 +46,7 @@ func (es OfflineStore) GetKey(role string) (data.PublicKey, error) { } // RotateKey returns ErrOffline -func (es OfflineStore) RotateKey(role string) (data.PublicKey, error) { +func (es OfflineStore) RotateKey(string, signed.CryptoService, ...data.PublicKey) (data.PublicKey, error) { return nil, err } diff --git a/tuf/store/offlinestore_test.go b/tuf/store/offlinestore_test.go index c60e38a68..e832f6c33 100644 --- a/tuf/store/offlinestore_test.go +++ b/tuf/store/offlinestore_test.go @@ -24,7 +24,7 @@ func TestOfflineStore(t *testing.T) { require.Error(t, err) require.IsType(t, ErrOffline{}, err) - _, err = s.RotateKey("") + _, err = s.RotateKey("", nil) require.Error(t, err) require.IsType(t, ErrOffline{}, err) From 413e1cdb0418cac9eb66b777896f3fa0d6fa7619 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 8 Feb 2016 14:49:56 -0800 Subject: [PATCH 5/8] 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 578348b32..ea513cd4e 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 cf5c8e5f0..110014778 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 44bee358f..a01e6102e 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 237bf11d4..bee7147b8 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 353ec750c..5ddfdd3b3 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 95bf0979c..e411319c9 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 97a04b3bb..d17173fa4 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 7eb50dd6b..92575e886 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 dbc818124..4971425ef 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, From e9182f7124dc29b0a78facbe2b33d40dd029ecd4 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 8 Feb 2016 17:03:09 -0800 Subject: [PATCH 6/8] Refactor getting signing keys for a role into a function on tuf.Repo. Since it already does something like that to find signing keys for signing roles. Also, include some better comments, debug logging, and error reporting. Signed-off-by: Ying Li --- client/backwards_compatibility_test.go | 12 +++------- client/client.go | 29 ++++++++++++----------- client/client_test.go | 15 +++++++++++- tuf/tuf.go | 32 ++++++++++++++++---------- 4 files changed, 53 insertions(+), 35 deletions(-) diff --git a/client/backwards_compatibility_test.go b/client/backwards_compatibility_test.go index ea513cd4e..833c2aec4 100644 --- a/client/backwards_compatibility_test.go +++ b/client/backwards_compatibility_test.go @@ -12,7 +12,6 @@ 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" @@ -107,14 +106,9 @@ 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 (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()) + // We need to rotate the timestamp key, since the server doesn't have the one + // specified by the metadata. + require.NoError(t, repo.RotateKey(data.CanonicalTimestampRole, true)) targets, err = repo.ListTargets() require.NoError(t, err) diff --git a/client/client.go b/client/client.go index 110014778..36ffe7b82 100644 --- a/client/client.go +++ b/client/client.go @@ -843,6 +843,8 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error { return fmt.Errorf( "notary does not currently support rotating the %s key", role) } + // We currently support locally managing targets keys, remotely managing + // timestamp keys, and locally or remotely managing timestamp keys. if serverManagesKey && role == data.CanonicalTargetsRole { return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole} } @@ -855,31 +857,32 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error { errFmtString string err error ) + // Key rotation is an offline operation unless we are rotating a remote key, in which case we + // need to ensure the rotation can actually happen: the user needs to have the root key, and + // we want to publish right away so that the remote key doesn't expire if serverManagesKey { - if err := r.bootstrapRepo(); err != nil { + if err = r.bootstrapRepo(); err != nil { + if _, ok := err.(store.ErrMetaNotFound); ok { + err = ErrRepoNotInitialized{} + } + logrus.Debug("Repository should be valid and initialized before rotating a remote key:", err) 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) - } + rootKeys, err := r.tufRepo.SigningKeysForRole(data.CanonicalRootRole) + if err != nil { + logrus.Debug("The root is invalid, because it does not contain any signing keys") + return err } 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 if _, ok := err.(signed.ErrNoKeys); ok { + err = fmt.Errorf("root signing key unavailable so unable to rotate key anyway") } } else { pubKey, err = r.CryptoService.Create(role, data.ECDSAKey) diff --git a/client/client_test.go b/client/client_test.go index a01e6102e..a8e7f420b 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2585,7 +2585,7 @@ func TestRemoteRotationNonRateLimitError(t *testing.T) { // The rotator is not the owner of the repository, they cannot rotate the remote // key -func TestRemoteRotationNonPermitted(t *testing.T) { +func TestRemoteRotationNoRootKey(t *testing.T) { ts := fullTestServer(t) defer ts.Close() @@ -2603,6 +2603,19 @@ func TestRemoteRotationNonPermitted(t *testing.T) { assert.IsType(t, signed.ErrNoKeys{}, err) } +// The repo hasn't been initialized, so we can't rotate +func TestRemoteRotationNonexistentRepo(t *testing.T) { + ts, _, _ := simpleTestServer(t) + defer ts.Close() + + repo := newBlankRepo(t, ts.URL) + defer os.RemoveAll(repo.baseDir) + + err := repo.RotateKey(data.CanonicalTimestampRole, true) + assert.Error(t, err) + assert.IsType(t, ErrRepoNotInitialized{}, err) +} + // If there is no local cache, notary operations return the remote error code func TestRemoteServerUnavailableNoLocalCache(t *testing.T) { tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-") diff --git a/tuf/tuf.go b/tuf/tuf.go index 8001c45de..0da0963e9 100644 --- a/tuf/tuf.go +++ b/tuf/tuf.go @@ -649,12 +649,11 @@ func (tr *Repo) SignRoot(expires time.Time) (*data.Signed, error) { logrus.Debug("signing root...") tr.Root.Signed.Expires = expires tr.Root.Signed.Version++ - root := tr.keysDB.GetRole(data.CanonicalRootRole) signed, err := tr.Root.ToSigned() if err != nil { return nil, err } - signed, err = tr.sign(signed, *root) + signed, err = tr.sign(signed, data.CanonicalRootRole) if err != nil { return nil, err } @@ -678,8 +677,7 @@ func (tr *Repo) SignTargets(role string, expires time.Time) (*data.Signed, error logrus.Debug("errored getting targets data.Signed object") return nil, err } - targets := tr.keysDB.GetRole(role) - signed, err = tr.sign(signed, *targets) + signed, err = tr.sign(signed, role) if err != nil { logrus.Debug("errored signing ", role) return nil, err @@ -717,8 +715,7 @@ func (tr *Repo) SignSnapshot(expires time.Time) (*data.Signed, error) { if err != nil { return nil, err } - snapshot := tr.keysDB.GetRole(data.CanonicalSnapshotRole) - signed, err = tr.sign(signed, *snapshot) + signed, err = tr.sign(signed, data.CanonicalSnapshotRole) if err != nil { return nil, err } @@ -743,8 +740,7 @@ func (tr *Repo) SignTimestamp(expires time.Time) (*data.Signed, error) { if err != nil { return nil, err } - timestamp := tr.keysDB.GetRole(data.CanonicalTimestampRole) - signed, err = tr.sign(signed, *timestamp) + signed, err = tr.sign(signed, data.CanonicalTimestampRole) if err != nil { return nil, err } @@ -753,9 +749,14 @@ func (tr *Repo) SignTimestamp(expires time.Time) (*data.Signed, error) { return signed, nil } -func (tr Repo) sign(signedData *data.Signed, role data.Role) (*data.Signed, error) { - ks := make([]data.PublicKey, 0, len(role.KeyIDs)) - for _, kid := range role.KeyIDs { +// SigningKeysForRole returns the public keys necessary for signing a particular role +func (tr *Repo) SigningKeysForRole(role string) ([]data.PublicKey, error) { + roleObj := tr.keysDB.GetRole(role) + if roleObj == nil { + return nil, data.ErrInvalidRole{Role: role, Reason: "does not exist"} + } + ks := make([]data.PublicKey, 0, len(roleObj.KeyIDs)) + for _, kid := range roleObj.KeyIDs { k := tr.keysDB.GetKey(kid) if k == nil { continue @@ -765,9 +766,16 @@ func (tr Repo) sign(signedData *data.Signed, role data.Role) (*data.Signed, erro if len(ks) < 1 { return nil, keys.ErrInvalidKey } - err := signed.Sign(tr.cryptoService, signedData, ks...) + return ks, nil +} + +func (tr Repo) sign(signedData *data.Signed, role string) (*data.Signed, error) { + signingKeys, err := tr.SigningKeysForRole(role) if err != nil { return nil, err } + if err := signed.Sign(tr.cryptoService, signedData, signingKeys...); err != nil { + return nil, err + } return signedData, nil } From 5ea520878ee91abac3e8c15422993f33cdd9f235 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 8 Feb 2016 17:11:17 -0800 Subject: [PATCH 7/8] Better server error logging. Signed-off-by: Ying Li --- server/handlers/default.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/handlers/default.go b/server/handlers/default.go index d17173fa4..cfee1b186 100644 --- a/server/handlers/default.go +++ b/server/handlers/default.go @@ -183,19 +183,19 @@ func parseKeyParams(ctx context.Context, vars map[string]string) (*serverKeyInfo s := ctx.Value("metaStore") store, ok := s.(storage.MetaStore) if !ok || store == nil { - return nil, errors.ErrNoStorage.WithDetail(nil) + return nil, errors.ErrNoStorage.WithDetail("metadata store not configured") } c := ctx.Value("cryptoService") crypto, ok := c.(signed.CryptoService) if !ok || crypto == nil { - return nil, errors.ErrNoCryptoService.WithDetail(nil) + return nil, errors.ErrNoCryptoService.WithDetail("crypto service not configured") } algo := ctx.Value("keyAlgorithm") keyAlgo, ok := algo.(string) if !ok || keyAlgo != data.ECDSAKey && keyAlgo != data.RSAKey && keyAlgo != data.ED25519Key { - return nil, errors.ErrNoKeyAlgorithm.WithDetail(nil) + return nil, errors.ErrNoKeyAlgorithm.WithDetail("key algorithm not configured") } return &serverKeyInfo{ From 4f39c93ce5791369b94d88aec5e6cec8b49ce16d Mon Sep 17 00:00:00 2001 From: Ying Li Date: Tue, 9 Feb 2016 18:34:17 -0800 Subject: [PATCH 8/8] Addressing minor review comments: - better comments - more test assertions - better error messages - stylistic improvements - variable renaming Signed-off-by: Ying Li --- client/client.go | 20 ++++++++++---------- client/client_test.go | 6 ++++-- cmd/notary/keys_test.go | 3 ++- server/errors/errors.go | 10 +++++----- server/handlers/default.go | 7 +++---- server/handlers/default_test.go | 2 +- tuf/store/httpstore.go | 13 +++---------- tuf/store/offlinestore.go | 18 +++++++++--------- 8 files changed, 37 insertions(+), 42 deletions(-) diff --git a/client/client.go b/client/client.go index 36ffe7b82..b2c0540df 100644 --- a/client/client.go +++ b/client/client.go @@ -52,7 +52,7 @@ 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 @@ -63,7 +63,7 @@ type ErrInvalidLocalRole struct { func (err ErrInvalidLocalRole) Error() string { return fmt.Sprintf( - "notary does not support the client managing the %s key", err.Role) + "notary does not permit the client managing the %s key", err.Role) } // ErrRepositoryNotExist is returned when an action is taken on a remote @@ -839,16 +839,16 @@ 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 { + switch { + case role == data.CanonicalRootRole: return fmt.Errorf( - "notary does not currently support rotating the %s key", role) - } + "notary does not currently permit rotating the %s key", role) + // We currently support locally managing targets keys, remotely managing - // timestamp keys, and locally or remotely managing timestamp keys. - if serverManagesKey && role == data.CanonicalTargetsRole { + // timestamp keys, and locally or remotely managing snapshot keys. + case serverManagesKey && role == data.CanonicalTargetsRole: return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole} - } - if !serverManagesKey && role == data.CanonicalTimestampRole { + case !serverManagesKey && role == data.CanonicalTimestampRole: return ErrInvalidLocalRole{Role: data.CanonicalTimestampRole} } @@ -882,7 +882,7 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error { if err == nil { pubKey, err = remote.RotateKey(role, r.CryptoService, rootKeys...) } else if _, ok := err.(signed.ErrNoKeys); ok { - err = fmt.Errorf("root signing key unavailable so unable to rotate key anyway") + err = fmt.Errorf("root signing key unavailable so unable to rotate key") } } else { pubKey, err = r.CryptoService.Create(role, data.ECDSAKey) diff --git a/client/client_test.go b/client/client_test.go index a8e7f420b..1fcecc8b4 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -248,7 +248,7 @@ func TestInitRepositoryManagedRolesIncludingRoot(t *testing.T) { assert.IsType(t, ErrInvalidRemoteRole{}, err) // Just testing the error message here in this one case assert.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.assertCreated(t, nil) } @@ -2580,7 +2580,9 @@ func TestRemoteRotationNonRateLimitError(t *testing.T) { defer os.RemoveAll(repo.baseDir) // simpleTestServer has no rotate key endpoint, so this should fail - assert.Error(t, repo.RotateKey(data.CanonicalTimestampRole, true)) + err := repo.RotateKey(data.CanonicalTimestampRole, true) + assert.Error(t, err) + assert.IsType(t, store.ErrMetaNotFound{}, err) } // The rotator is not the owner of the repository, they cannot rotate the remote diff --git a/cmd/notary/keys_test.go b/cmd/notary/keys_test.go index e411319c9..9d989caa5 100644 --- a/cmd/notary/keys_test.go +++ b/cmd/notary/keys_test.go @@ -233,11 +233,12 @@ func TestRemoveMultikeysRemoveOnlyChosenKey(t *testing.T) { } } -// Non-roles, and root 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, "notevenARole", + "targets/a", } for _, role := range invalids { for _, serverManaged := range []bool{true, false} { diff --git a/server/errors/errors.go b/server/errors/errors.go index 9c44a9bec..714066289 100644 --- a/server/errors/errors.go +++ b/server/errors/errors.go @@ -82,15 +82,15 @@ var ( HTTPStatusCode: http.StatusInternalServerError, }) ErrNoKeyAlgorithm = errcode.Register(errGroup, errcode.ErrorDescriptor{ - Value: "NO_KEYALGORITHM", + Value: "NO_KEY_ALGORITHM", Message: "The server does not have a key algorithm configured.", Description: "No key algorithm has been configured for the server and it has been asked to perform an operation that requires generation.", HTTPStatusCode: http.StatusInternalServerError, }) - ErrCannotRotateKey = errcode.Register(errGroup, errcode.ErrorDescriptor{ - Value: "CANNOT_ROTATE_KEY", - Message: "Key has already been rotated recently.", - Description: "The key has been rotated too recently, and cannot be rotated again at this time.", + ErrKeyRotationLimited = errcode.Register(errGroup, errcode.ErrorDescriptor{ + Value: "ROTATE_KEY_RATE_LIMITED", + Message: "Cannot rotate, last key rotation too recent.", + Description: "Cannot rotate key because the last key rotation was too recent", HTTPStatusCode: notary.HTTPStatusTooManyRequests, }) ErrUnknown = errcode.ErrorCodeUnknown diff --git a/server/handlers/default.go b/server/handlers/default.go index cfee1b186..0ed726df0 100644 --- a/server/handlers/default.go +++ b/server/handlers/default.go @@ -176,9 +176,6 @@ func parseKeyParams(ctx context.Context, vars map[string]string) (*serverKeyInfo if !ok || role == "" { return nil, errors.ErrUnknown.WithDetail("no role") } - if role != data.CanonicalTimestampRole && role != data.CanonicalSnapshotRole { - return nil, errors.ErrInvalidRole.WithDetail(role) - } s := ctx.Value("metaStore") store, ok := s.(storage.MetaStore) @@ -194,7 +191,7 @@ func parseKeyParams(ctx context.Context, vars map[string]string) (*serverKeyInfo algo := ctx.Value("keyAlgorithm") keyAlgo, ok := algo.(string) - if !ok || keyAlgo != data.ECDSAKey && keyAlgo != data.RSAKey && keyAlgo != data.ED25519Key { + if !ok || keyAlgo == "" { return nil, errors.ErrNoKeyAlgorithm.WithDetail("key algorithm not configured") } @@ -223,6 +220,8 @@ func getKeyHandler(ctx context.Context, w io.Writer, vars map[string]string) err key, err = timestamp.GetOrCreateTimestampKey(s.gun, s.store, s.crypto, s.keyAlgo) case data.CanonicalSnapshotRole: key, err = snapshot.GetOrCreateSnapshotKey(s.gun, s.store, s.crypto, s.keyAlgo) + default: + return errors.ErrInvalidRole.WithDetail(s.role) } if err != nil { return errors.ErrUnknown.WithDetail(err) diff --git a/server/handlers/default_test.go b/server/handlers/default_test.go index 92575e886..8dda4b938 100644 --- a/server/handlers/default_test.go +++ b/server/handlers/default_test.go @@ -102,7 +102,7 @@ func TestKeyHandlersInvalidConfiguration(t *testing.T) { invalidStates := map[string][]handlerState{ "no storage": {noStore, invalidStore}, "no cryptoservice": {noCrypto, invalidCrypto}, - "no keyalgorithm": {noKeyAlgo, invalidKeyAlgo}, + "no key algorithm": {noKeyAlgo, invalidKeyAlgo}, } vars := map[string]string{ diff --git a/tuf/store/httpstore.go b/tuf/store/httpstore.go index 9e378cc46..532ae92d3 100644 --- a/tuf/store/httpstore.go +++ b/tuf/store/httpstore.go @@ -278,7 +278,7 @@ func (s HTTPStore) buildURL(uri string) (*url.URL, error) { // GetKey retrieves the most recently created (whether it is signed in yet or not) // public key for the given role from the remote server func (s HTTPStore) GetKey(role string) (data.PublicKey, error) { - return s.requestKey(role, nil) + return s.requestKey(role, "GET", fmt.Sprintf("%s key", role), nil) } // RotateKey rotates a key on the remote server and returns the new public key. This requires @@ -304,24 +304,17 @@ func (s HTTPStore) RotateKey(role string, cs signed.CryptoService, roots ...data return nil, err } - return s.requestKey(role, bytes.NewBuffer(requestBody)) + return s.requestKey(role, "POST", fmt.Sprintf("%s key rotation", role), bytes.NewBuffer(requestBody)) } // requestKey either sends a get or a post request, depending on whether there // is a body. -func (s HTTPStore) requestKey(role string, body io.Reader) (data.PublicKey, error) { +func (s HTTPStore) requestKey(role, method, resource string, body io.Reader) (data.PublicKey, error) { url, err := s.buildKeyURL(role) if err != nil { return nil, err } - method := "GET" - resource := role + " key" - if body != nil { - method = "POST" - resource = resource + " rotation" - } - req, err := http.NewRequest(method, url.String(), body) if err != nil { return nil, err diff --git a/tuf/store/offlinestore.go b/tuf/store/offlinestore.go index 5bc92c1a5..624ff52be 100644 --- a/tuf/store/offlinestore.go +++ b/tuf/store/offlinestore.go @@ -14,7 +14,7 @@ func (e ErrOffline) Error() string { return "client is offline" } -var err = ErrOffline{} +var errOffline = ErrOffline{} // OfflineStore is to be used as a placeholder for a nil store. It simply // returns ErrOffline for every operation @@ -22,40 +22,40 @@ type OfflineStore struct{} // GetMeta returns ErrOffline func (es OfflineStore) GetMeta(name string, size int64) ([]byte, error) { - return nil, err + return nil, errOffline } // SetMeta returns ErrOffline func (es OfflineStore) SetMeta(name string, blob []byte) error { - return err + return errOffline } // SetMultiMeta returns ErrOffline func (es OfflineStore) SetMultiMeta(map[string][]byte) error { - return err + return errOffline } // RemoveMeta returns ErrOffline func (es OfflineStore) RemoveMeta(name string) error { - return err + return errOffline } // GetKey returns ErrOffline func (es OfflineStore) GetKey(role string) (data.PublicKey, error) { - return nil, err + return nil, errOffline } // RotateKey returns ErrOffline func (es OfflineStore) RotateKey(string, signed.CryptoService, ...data.PublicKey) (data.PublicKey, error) { - return nil, err + return nil, errOffline } // GetTarget returns ErrOffline func (es OfflineStore) GetTarget(path string) (io.ReadCloser, error) { - return nil, err + return nil, errOffline } // RemoveAll return ErrOffline func (es OfflineStore) RemoveAll() error { - return err + return errOffline }