Clean up after rebase and address review comments

Signed-off-by: Ying Li <ying.li@docker.com>

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
 		}
-	}
+	case storage.ErrNotFound:
-
+		break // this is the first timestamp ever for the repo
-	if _, ok := err.(storage.ErrNotFound); !ok && err != nil {
+	default:
 		return nil, err
 	}
 
 	metaUpdate, err := timestamp.NewTimestampUpdate(prev, repo)
-	if err != nil {
+
-		_, noSigs := err.(signed.ErrInsufficientSignatures)
+	switch err.(type) {
-		_, noKeys := err.(signed.ErrNoKeys)
+	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{
-			return nil, validation.ErrBadRoot{
+			Msg: fmt.Sprintf("none of the following timestamp keys exist on the server: %s",
-				Msg: fmt.Sprintf("none of the following timestamp keys exist on the server: %s",
+				strings.Join(repo.Root.Signed.Roles[data.CanonicalTimestampRole].KeyIDs, ", ")),
-					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
+	// Always verify the new root against the old root
-	// rotation may have taken place
+	if err := signed.VerifyRoot(newSigned, oldRootRole.Threshold, oldRootRole.Keys); err != nil {
-	rotation := len(oldRootRole.Keys) != len(newRootRole.Keys)
+		return validation.ErrBadRoot{Msg: fmt.Sprintf(
-	if !rotation { // if the number of keys is the same, we need to check every key
+			"rotation detected and new root was not signed with at least %d old keys",
-		for kid := range newRootRole.Keys {
+			oldRootRole.Threshold)}
-			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)}
-		}
 	}
 
 	return nil

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)

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()))
 

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()))