diff --git a/client/client_test.go b/client/client_test.go index 678746af78..6f482d5cc4 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2580,9 +2580,17 @@ func TestRotateKeyInvalidRole(t *testing.T) { require.Error(t, repo.RotateKey("targets/releases", false), "Rotating a delegation key should fail") + // rotating a delegation key to the server also fails + require.Error(t, repo.RotateKey("targets/releases", true), + "Rotating a delegation key should fail") + // rotating a not a real role key fails require.Error(t, repo.RotateKey("nope", false), "Rotating a non-real role key should fail") + + // rotating a not a real role key to the server also fails + require.Error(t, repo.RotateKey("nope", false), + "Rotating a non-real role key should fail") } // If remotely rotating key fails, the failure is propagated @@ -2595,9 +2603,59 @@ func TestRemoteRotationError(t *testing.T) { ts.Close() // server has died, so this should fail + for _, role := range []string{data.CanonicalSnapshotRole, data.CanonicalTimestampRole} { + err := repo.RotateKey(role, true) + require.Error(t, err) + require.Contains(t, err.Error(), "unable to rotate remote key") + } +} + +// If remotely rotating key fails for any reason, fail the rotation entirely +func TestRemoteRotationEndpointError(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 + for _, role := range []string{data.CanonicalSnapshotRole, data.CanonicalTimestampRole} { + err := repo.RotateKey(role, true) + require.Error(t, err) + require.IsType(t, store.ErrMetaNotFound{}, err) + } +} + +// The rotator is not the owner of the repository, they cannot rotate the remote +// key +func TestRemoteRotationNoRootKey(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) + require.NoError(t, repo.Publish()) + + newRepo, _ := newRepoToTestRepo(t, repo, true) + defer os.RemoveAll(newRepo.baseDir) + _, err := newRepo.ListTargets() + require.NoError(t, err) + + err = newRepo.RotateKey(data.CanonicalSnapshotRole, true) + require.Error(t, err) + require.IsType(t, signed.ErrInsufficientSignatures{}, 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) require.Error(t, err) - require.Contains(t, err.Error(), "unable to rotate remote key") + require.IsType(t, ErrRepoNotInitialized{}, err) } // Rotates the keys. After the rotation, downloading the latest metadata @@ -2732,6 +2790,14 @@ func TestRotateKeyAfterPublishServerManagementChange(t *testing.T) { data.CanonicalTargetsRole: false, data.CanonicalRootRole: false, }) + // check that the snapshot remote rotation creates new keys + testRotateKeySuccess(t, false, map[string]bool{ + data.CanonicalSnapshotRole: true, + }) + // check that the timestamp remote rotation creates new keys + testRotateKeySuccess(t, false, map[string]bool{ + data.CanonicalTimestampRole: true, + }) // reclaim snapshot key management from the server testRotateKeySuccess(t, true, map[string]bool{ data.CanonicalSnapshotRole: false, diff --git a/docs/advanced_usage.md b/docs/advanced_usage.md index a3e1de39b2..8c5c6fd148 100644 --- a/docs/advanced_usage.md +++ b/docs/advanced_usage.md @@ -87,7 +87,7 @@ subsection. ### Rotate keys -In case of potential compromise, notary provides a CLI command for rotating keys. Currently, you can use the `notary key rotate` command to rotate the targets or snapshot keys. +In case of potential compromise, notary provides a CLI command for rotating keys. Currently, you can use the `notary key rotate` command to rotate the root, targets, snapshot, or timestamp keys. While the snapshot key is managed by the notary client by default, use the `notary key rotate snapshot -r` command to rotate the snapshot key to the server, such that the @@ -98,7 +98,8 @@ snapshot key to push their updates to the collection. Note that new collections created by a Docker 1.11 Engine client will have the server manage the snapshot key by default. To reclaim control of the snapshot key on the client, use the `notary key rotate` command without the `-r` flag. -The targets key must be locally managed - to rotate the targets key, for instance in case of compromise, use the `notary key rotate targets` command without the `-r` flag. +The root and targets key must be locally managed - to rotate either the root or targets key, for instance in case of compromise, use the `notary key rotate` command without the `-r` flag. +The timestamp key must be remotely managed - to rotate the timestamp key use the `notary key rotate timestamp -r` command. ### Use a Yubikey diff --git a/server/errors/errors.go b/server/errors/errors.go index a06c1caf2c..3ccb7d3f36 100644 --- a/server/errors/errors.go +++ b/server/errors/errors.go @@ -92,11 +92,5 @@ var ( Description: "The server does not support actions on images of this name", HTTPStatusCode: http.StatusBadRequest, }) - 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: http.StatusTooManyRequests, - }) ErrUnknown = errcode.ErrorCodeUnknown ) diff --git a/server/handlers/default_test.go b/server/handlers/default_test.go index 29646a737b..31c5f1a5b3 100644 --- a/server/handlers/default_test.go +++ b/server/handlers/default_test.go @@ -76,9 +76,11 @@ func TestMainHandlerNotGet(t *testing.T) { } } -// GetKeyHandler needs to have access to a metadata store and cryptoservice, +type simplerHandler func(context.Context, http.ResponseWriter, *http.Request, map[string]string) error + +// GetKeyHandler and RotateKeyHandler needs to have access to a metadata store and cryptoservice, // a key algorithm -func TestGetKeyHandlerInvalidConfiguration(t *testing.T) { +func TestKeyHandlersInvalidConfiguration(t *testing.T) { noStore := defaultState() noStore.store = nil @@ -108,38 +110,41 @@ func TestGetKeyHandlerInvalidConfiguration(t *testing.T) { "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) - require.Error(t, err) - require.Contains(t, err.Error(), errString) + for _, keyHandler := range []simplerHandler{getKeyHandler, rotateKeyHandler} { + for errString, states := range invalidStates { + for _, s := range states { + err := keyHandler(getContext(s), httptest.NewRecorder(), req, vars) + require.Error(t, err) + require.Contains(t, err.Error(), errString) + } } } } -// GetKeyHandler needs to be set up such that an imageName and tufRole are both +// GetKeyHandler and RotateKeyHandler need to be set up such that an imageName and tufRole are both // provided and non-empty. -func TestGetKeyHandlerNoRoleOrRepo(t *testing.T) { +func TestKeyHandlersNoRoleOrRepo(t *testing.T) { state := defaultState() req := &http.Request{Body: ioutil.NopCloser(bytes.NewBuffer(nil))} + for _, keyHandler := range []simplerHandler{getKeyHandler, rotateKeyHandler} { + for _, key := range []string{"imageName", "tufRole"} { + vars := map[string]string{ + "imageName": "gun", + "tufRole": data.CanonicalTimestampRole, + } + + // not provided + delete(vars, key) + err := keyHandler(getContext(state), httptest.NewRecorder(), req, vars) + require.Error(t, err) + require.Contains(t, err.Error(), "unknown") - for _, key := range []string{"imageName", "tufRole"} { - vars := map[string]string{ - "imageName": "gun", - "tufRole": data.CanonicalTimestampRole, + // empty + vars[key] = "" + err = keyHandler(getContext(state), httptest.NewRecorder(), req, vars) + require.Error(t, err) + require.Contains(t, err.Error(), "unknown") } - - // not provided - delete(vars, key) - err := getKeyHandler(getContext(state), httptest.NewRecorder(), req, vars) - require.Error(t, err) - require.Contains(t, err.Error(), "unknown") - - // empty - vars[key] = "" - err = getKeyHandler(getContext(state), httptest.NewRecorder(), req, vars) - require.Error(t, err) - require.Contains(t, err.Error(), "unknown") } } @@ -172,6 +177,55 @@ func TestGetKeyHandlerCreatesOnce(t *testing.T) { } } +// Rotating a key for a non-supported role results in a 400. +func TestRotateKeyHandlerInvalidRole(t *testing.T) { + state := defaultState() + for _, role := range []string{data.CanonicalRootRole, data.CanonicalTargetsRole, "targets/a", "invalidrole"} { + vars := map[string]string{ + "imageName": "gun", + "tufRole": role, + } + req := &http.Request{Body: ioutil.NopCloser(bytes.NewBuffer(nil))} + + err := rotateKeyHandler(getContext(state), httptest.NewRecorder(), req, vars) + require.Error(t, err) + require.Contains(t, err.Error(), "invalid role") + } +} + +// Rotating the key fails if we don't pass a valid key algorithm, or if it isn't Ed25519 +func TestRotateKeyHandlerNonED25519(t *testing.T) { + 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() + invalidKeyAlgoState := defaultState() + invalidKeyAlgoState.keyAlgo = "notactuallyakeyalgorithm" + err := rotateKeyHandler(getContext(invalidKeyAlgoState), recorder, req, vars) + require.Error(t, err) + invalidKeyAlgoState.keyAlgo = data.ECDSAKey + err = rotateKeyHandler(getContext(invalidKeyAlgoState), recorder, req, vars) + require.Error(t, err) + } +} + +// Rotating the key for a valid role and gun succeeds +func TestRotateKeyHandlerSuccessfulRotation(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 := rotateKeyHandler(getContext(state), recorder, req, vars) + require.NoError(t, err) + require.True(t, len(recorder.Body.String()) > 0) + } +} + 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 ea117f3990..8f2d93d182 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -304,3 +304,32 @@ func verifyGetResponse(t *testing.T, r *http.Response, expectedBytes []byte) { require.NotEqual(t, "", r.Header.Get("Last-Modified")) require.Equal(t, "", r.Header.Get("Pragma")) } + +// RotateKey supports only timestamp and snapshot key rotation +func TestRotateKeyEndpoint(t *testing.T) { + ctx := context.WithValue( + context.Background(), "metaStore", storage.NewMemStorage()) + ctx = context.WithValue(ctx, "keyAlgorithm", data.ED25519Key) + + ccc := utils.NewCacheControlConfig(10, false) + handler := RootHandler(nil, ctx, signed.NewEd25519(), ccc, ccc, nil) + ts := httptest.NewServer(handler) + defer ts.Close() + + rolesToStatus := map[string]int{ + 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, + "targets/delegation": 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) + require.NoError(t, err) + require.Equal(t, expectedStatus, res.StatusCode) + } +} diff --git a/signer/keydbstore/cachedkeystore_test.go b/signer/keydbstore/cachedkeystore_test.go index 450813933b..a040d13304 100644 --- a/signer/keydbstore/cachedkeystore_test.go +++ b/signer/keydbstore/cachedkeystore_test.go @@ -74,7 +74,7 @@ func TestGetSuccessPopulatesCache(t *testing.T) { requireGetKeySuccessFromCache(t, cached, underlying, data.CanonicalTimestampRole, testKey) } -// Creating a key, on succcess, populates the cache, but does not do so on failure +// Creating a key, on success, populates the cache, but does not do so on failure func TestAddKeyPopulatesCacheIfSuccessful(t *testing.T) { var underlying trustmanager.KeyStore underlying = trustmanager.NewKeyMemoryStore(constRetriever) diff --git a/signer/keydbstore/keydbstore_test.go b/signer/keydbstore/keydbstore_test.go index 4f6ace6cb8..58d265980a 100644 --- a/signer/keydbstore/keydbstore_test.go +++ b/signer/keydbstore/keydbstore_test.go @@ -36,6 +36,7 @@ type keyRotator interface { type keyActivator interface { trustmanager.KeyStore MarkActive(keyID string) error + GetPendingKey(trustmanager.KeyInfo) (data.PublicKey, error) } // A key can only be added to the DB once. Returns a list of expected keys, and which keys are expected to exist. @@ -139,3 +140,37 @@ func testMarkKeyActive(t *testing.T, dbStore keyActivator) (data.PrivateKey, dat return testKeys[0], testKeys[1] } + +func testGetPendingKey(t *testing.T, dbStore keyActivator) (data.PrivateKey, data.PrivateKey) { + // Create a test key and add it to the db such that it will be pending (never marked active) + keyInfo := trustmanager.KeyInfo{Role: data.CanonicalSnapshotRole, Gun: "gun"} + pendingTestKey, err := utils.GenerateECDSAKey(rand.Reader) + require.NoError(t, err) + requireGetKeyFailure(t, dbStore, pendingTestKey.ID()) + err = dbStore.AddKey(keyInfo, pendingTestKey) + require.NoError(t, err) + requireGetKeySuccess(t, dbStore, data.CanonicalSnapshotRole, pendingTestKey) + + retrievedKey, err := dbStore.GetPendingKey(keyInfo) + require.NoError(t, err) + require.Equal(t, pendingTestKey.Public(), retrievedKey.Public()) + + // Now create an active key with the same keyInfo + activeTestKey, err := utils.GenerateECDSAKey(rand.Reader) + require.NoError(t, err) + requireGetKeyFailure(t, dbStore, activeTestKey.ID()) + err = dbStore.AddKey(keyInfo, activeTestKey) + require.NoError(t, err) + requireGetKeySuccess(t, dbStore, data.CanonicalSnapshotRole, activeTestKey) + + // Mark as active + require.NoError(t, dbStore.MarkActive(activeTestKey.ID())) + + // We should still get back the original pending key on GetPendingKey + retrievedKey, err = dbStore.GetPendingKey(keyInfo) + require.NoError(t, err) + require.NotEqual(t, activeTestKey.Public(), retrievedKey.Public()) + require.Equal(t, pendingTestKey.Public(), retrievedKey.Public()) + + return pendingTestKey, activeTestKey +} diff --git a/signer/keydbstore/rethink_realkeydbstore_test.go b/signer/keydbstore/rethink_realkeydbstore_test.go index 8eb7569750..51917891f9 100644 --- a/signer/keydbstore/rethink_realkeydbstore_test.go +++ b/signer/keydbstore/rethink_realkeydbstore_test.go @@ -209,3 +209,16 @@ func TestRethinkMarkKeyActive(t *testing.T) { require.True(t, rdbKeys[activeKey.ID()].LastUsed.Equal(rdbNow)) require.True(t, rdbKeys[nonActiveKey.ID()].LastUsed.Equal(time.Time{})) } + +func TestRethinkGetPendingKey(t *testing.T) { + dbStore, cleanup := rethinkDBSetup(t, "signerActivationTests") + defer cleanup() + + pendingKey, activeKey := testGetPendingKey(t, dbStore) + + rdbKeys := requireExpectedRDBKeys(t, dbStore, []data.PrivateKey{pendingKey, activeKey}) + + // check that activation updates the activated key but not the unactivated key + require.True(t, rdbKeys[activeKey.ID()].LastUsed.Equal(rdbNow)) + require.True(t, rdbKeys[pendingKey.ID()].LastUsed.Equal(time.Time{})) +} diff --git a/signer/keydbstore/sql_keydbstore_test.go b/signer/keydbstore/sql_keydbstore_test.go index ee3bdfe375..4831bd2614 100644 --- a/signer/keydbstore/sql_keydbstore_test.go +++ b/signer/keydbstore/sql_keydbstore_test.go @@ -157,3 +157,16 @@ func TestSQLMarkKeyActive(t *testing.T) { require.True(t, gormKeys[activeKey.ID()].LastUsed.Equal(gormActiveTime)) require.True(t, gormKeys[nonActiveKey.ID()].LastUsed.Equal(time.Time{})) } + +func TestSQLGetPendingKey(t *testing.T) { + dbStore, cleanup := sqldbSetup(t) + defer cleanup() + + pendingKey, activeKey := testGetPendingKey(t, dbStore) + + gormKeys := requireExpectedGORMKeys(t, dbStore, []data.PrivateKey{pendingKey, activeKey}) + + // check that activation updates the activated key but not the pending key + require.True(t, gormKeys[activeKey.ID()].LastUsed.Equal(gormActiveTime)) + require.True(t, gormKeys[pendingKey.ID()].LastUsed.Equal(time.Time{})) +} diff --git a/storage/httpstore_test.go b/storage/httpstore_test.go index 359fe82f2b..09bceb6b01 100644 --- a/storage/httpstore_test.go +++ b/storage/httpstore_test.go @@ -206,6 +206,20 @@ func TestHTTPStoreRemoveAll(t *testing.T) { require.NoError(t, err) } +func TestHTTPStoreRotateKey(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(testRootKey)) + } + server := httptest.NewServer(http.HandlerFunc(handler)) + defer server.Close() + store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport) + require.NoError(t, err) + + pubKeyBytes, err := store.RotateKey(data.CanonicalSnapshotRole) + require.NoError(t, err) + require.Equal(t, pubKeyBytes, []byte(testRootKey)) +} + func TestHTTPOffline(t *testing.T) { s, err := NewHTTPStore("https://localhost/", "", "", "", nil) require.NoError(t, err) diff --git a/storage/offlinestore_test.go b/storage/offlinestore_test.go index 659211e422..57f9073e4a 100644 --- a/storage/offlinestore_test.go +++ b/storage/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.RemoveAll() require.Error(t, err) require.IsType(t, ErrOffline{}, err)