From e8cdc32f0be11cb3ccf4fc2bc0eadb701638a62d Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 18 Mar 2016 17:18:58 -0700 Subject: [PATCH] Clean up after rebase and address review comments Signed-off-by: Ying Li --- server/handlers/validation.go | 55 ++++++++++++------------------ server/handlers/validation_test.go | 21 ++++++++---- server/snapshot/snapshot_test.go | 2 +- server/timestamp/timestamp_test.go | 2 +- 4 files changed, 38 insertions(+), 42 deletions(-) diff --git a/server/handlers/validation.go b/server/handlers/validation.go index 0a07db5e4c..cd853961a8 100644 --- a/server/handlers/validation.go +++ b/server/handlers/validation.go @@ -240,34 +240,35 @@ func generateSnapshot(gun string, repo *tuf.Repo, store storage.MetaStore) (*sto func generateTimestamp(gun string, repo *tuf.Repo, store storage.MetaStore) (*storage.MetaUpdate, error) { var prev *data.SignedTimestamp _, currentJSON, err := store.GetCurrent(gun, data.CanonicalTimestampRole) - if err == nil { + + switch err.(type) { + case nil: prev = new(data.SignedTimestamp) - if err = json.Unmarshal(currentJSON, prev); err != nil { + if err := json.Unmarshal(currentJSON, prev); err != nil { logrus.Error("Failed to unmarshal existing timestamp for GUN ", gun) return nil, err } - } - - if _, ok := err.(storage.ErrNotFound); !ok && err != nil { + case storage.ErrNotFound: + break // this is the first timestamp ever for the repo + default: return nil, err } metaUpdate, err := timestamp.NewTimestampUpdate(prev, repo) - if err != nil { - _, noSigs := err.(signed.ErrInsufficientSignatures) - _, noKeys := err.(signed.ErrNoKeys) + + switch err.(type) { + case nil: + return metaUpdate, nil + case signed.ErrInsufficientSignatures, signed.ErrNoKeys: // If we cannot sign the timestamp, then we don't have keys for the timestamp, // and the client screwed up their root - if noSigs || noKeys { - return nil, validation.ErrBadRoot{ - Msg: fmt.Sprintf("none of the following timestamp keys exist on the server: %s", - strings.Join(repo.Root.Signed.Roles[data.CanonicalTimestampRole].KeyIDs, ", ")), - } + return nil, validation.ErrBadRoot{ + Msg: fmt.Sprintf("none of the following timestamp keys exist on the server: %s", + strings.Join(repo.Root.Signed.Roles[data.CanonicalTimestampRole].KeyIDs, ", ")), } - + default: return nil, validation.ErrValidation{Msg: err.Error()} } - return metaUpdate, nil } // loadAndValidateSnapshot validates that the given snapshot update is valid. It also sets the new snapshot @@ -438,25 +439,11 @@ func checkAgainstOldRoot(oldRoot []byte, newRootRole data.BaseRole, newSigned *d return err } - // if the set of keys has changed between the old root and new root, then a root - // rotation may have taken place - rotation := len(oldRootRole.Keys) != len(newRootRole.Keys) - if !rotation { // if the number of keys is the same, we need to check every key - for kid := range newRootRole.Keys { - if _, ok := oldRootRole.Keys[kid]; !ok { - // if there is any difference in keys, a key rotation may have - // occurred. - rotation = true - } - } - } - - if rotation { - if err := signed.VerifyRoot(newSigned, oldRootRole.Threshold, oldRootRole.Keys); err != nil { - return validation.ErrBadRoot{Msg: fmt.Sprintf( - "rotation detected and new root was not signed with at least %d old keys", - oldRootRole.Threshold)} - } + // Always verify the new root against the old root + if err := signed.VerifyRoot(newSigned, oldRootRole.Threshold, oldRootRole.Keys); err != nil { + return validation.ErrBadRoot{Msg: fmt.Sprintf( + "rotation detected and new root was not signed with at least %d old keys", + oldRootRole.Threshold)} } return nil diff --git a/server/handlers/validation_test.go b/server/handlers/validation_test.go index 6c66bb8332..6a4c103031 100644 --- a/server/handlers/validation_test.go +++ b/server/handlers/validation_test.go @@ -3,7 +3,6 @@ package handlers import ( "bytes" "encoding/json" - "path" "reflect" "testing" "time" @@ -52,10 +51,10 @@ func copyKeys(t *testing.T, from signed.CryptoService, roles ...string) signed.C for _, keyID := range from.ListKeys(role) { key, _, err := from.GetPrivateKey(keyID) assert.NoError(t, err) - memKeyStore.AddKey(path.Base(keyID), role, key) + memKeyStore.AddKey(trustmanager.KeyInfo{Role: role}, key) } } - return cryptoservice.NewCryptoService("", memKeyStore) + return cryptoservice.NewCryptoService(memKeyStore) } // Returns a mapping of role name to `MetaUpdate` objects @@ -303,6 +302,8 @@ func TestValidateOldRootCorrupt(t *testing.T) { assert.IsType(t, &json.SyntaxError{}, err) } +// We cannot validate a new root if the old root is corrupt, because there might +// have been a root key rotation. func TestValidateOldRootCorruptRootRole(t *testing.T) { repo, cs, err := testutils.EmptyRepo("docker.com/notary") assert.NoError(t, err) @@ -333,6 +334,10 @@ func TestValidateOldRootCorruptRootRole(t *testing.T) { assert.IsType(t, data.ErrInvalidRole{}, err) } +// We cannot validate a new root if we cannot get the old root from the DB ( +// and cannot detect whether there was an old root or not), because there might +// have been an old root and we can't determine if the new root represents a +// root key rotation. func TestValidateRootGetCurrentRootBroken(t *testing.T) { repo, cs, err := testutils.EmptyRepo("docker.com/notary") assert.NoError(t, err) @@ -354,6 +359,7 @@ func TestValidateRootGetCurrentRootBroken(t *testing.T) { assert.IsType(t, data.ErrNoSuchRole{}, err) } +// A valid root rotation requires that the new root be signed with both old and new keys. func TestValidateRootRotation(t *testing.T) { repo, crypto, err := testutils.EmptyRepo("docker.com/notary") assert.NoError(t, err) @@ -400,7 +406,9 @@ func TestValidateRootRotation(t *testing.T) { assert.NoError(t, err) } -func TestInvalidRootRotation(t *testing.T) { +// A root rotation must be signed with old and new root keys, otherwise the +// new root fails to validate +func TestRootRotationNotSignedWithOldKeys(t *testing.T) { repo, crypto, err := testutils.EmptyRepo("docker.com/notary") assert.NoError(t, err) store := storage.NewMemStorage() @@ -412,7 +420,7 @@ func TestInvalidRootRotation(t *testing.T) { store.UpdateCurrent("testGUN", root) - rootKey, err := crypto.Create("root", data.ED25519Key) + rootKey, err := crypto.Create("root", "testGUN", data.ED25519Key) assert.NoError(t, err) rootRole, err := data.NewRole("root", 1, []string{rootKey.ID()}, nil) assert.NoError(t, err) @@ -442,6 +450,7 @@ func TestInvalidRootRotation(t *testing.T) { assert.Contains(t, err.Error(), "new root was not signed with at least 1 old keys") } +// An update is not valid without the root metadata. func TestValidateNoRoot(t *testing.T) { repo, cs, err := testutils.EmptyRepo("docker.com/notary") assert.NoError(t, err) @@ -695,7 +704,7 @@ func TestValidateRootInvalidTimestampKey(t *testing.T) { updates := []storage.MetaUpdate{root, targets, snapshot} serverCrypto := signed.NewEd25519() - _, err = serverCrypto.Create(data.CanonicalTimestampRole, data.ED25519Key) + _, err = serverCrypto.Create(data.CanonicalTimestampRole, "testGUN", data.ED25519Key) assert.NoError(t, err) _, err = validateUpdate(serverCrypto, "testGUN", updates, store) diff --git a/server/snapshot/snapshot_test.go b/server/snapshot/snapshot_test.go index 7c645d6e6a..9c830f5e22 100644 --- a/server/snapshot/snapshot_test.go +++ b/server/snapshot/snapshot_test.go @@ -138,7 +138,7 @@ func TestGetSnapshotNoPreviousSnapshot(t *testing.T) { } // create a key to be used by GetOrCreateSnapshot - key, err := crypto.Create(data.CanonicalSnapshotRole, data.ECDSAKey) + key, err := crypto.Create(data.CanonicalSnapshotRole, "gun", data.ECDSAKey) assert.NoError(t, err) assert.NoError(t, store.SetKey("gun", data.CanonicalSnapshotRole, key.Algorithm(), key.Public())) diff --git a/server/timestamp/timestamp_test.go b/server/timestamp/timestamp_test.go index ab3c51d91e..4808f80ddb 100644 --- a/server/timestamp/timestamp_test.go +++ b/server/timestamp/timestamp_test.go @@ -77,7 +77,7 @@ func TestGetTimestampNoPreviousTimestamp(t *testing.T) { } // create a key to be used by GetOrCreateTimestamp - key, err := crypto.Create(data.CanonicalTimestampRole, data.ECDSAKey) + key, err := crypto.Create(data.CanonicalTimestampRole, "gun", data.ECDSAKey) assert.NoError(t, err) assert.NoError(t, store.SetKey("gun", data.CanonicalTimestampRole, key.Algorithm(), key.Public()))