Fix server validation and client update tests to also test threshold when testing

root rotation with the previous root role.

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

client/client_update_test.go

@@ -1322,13 +1322,8 @@ func TestValidateRootRotationWithOldRole(t *testing.T) {
 	repo := newBlankRepo(t, ts.URL)
 	defer os.RemoveAll(repo.baseDir)
 
-	pubKey1, err := testutils.CreateKey(serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalRootRole)
-	require.NoError(t, err)
-	pubKey2, err := testutils.CreateKey(serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalRootRole)
-	require.NoError(t, err)
-
-	// update the root metadata to have 2 keys and threshold 1, and then sign
-	// only with one of the keys
+	// update the root metadata to have 3 keys and threshold 2, and then sign
+	// only with two of the keys
 	rootBytes, err := serverSwizzler.MetadataCache.GetMeta(data.CanonicalRootRole, -1)
 	require.NoError(t, err)
 	signedRoot := data.SignedRoot{}
@@ -1340,14 +1335,24 @@ func TestValidateRootRotationWithOldRole(t *testing.T) {
 		Threshold: 1,
 		KeyIDs:    signedRoot.Signed.Roles[data.CanonicalRootRole].KeyIDs,
 	}
+
+	threeKeys := make([]data.PublicKey, 3)
+	keyIDs := make([]string, len(threeKeys))
+	for i := 0; i < len(threeKeys); i++ {
+		threeKeys[i], err = testutils.CreateKey(
+			serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalRootRole)
+		require.NoError(t, err)
+		keyIDs[i] = threeKeys[i].ID()
+		signedRoot.Signed.Keys[keyIDs[i]] = threeKeys[i]
+	}
 	signedRoot.Signed.Version++
-	signedRoot.Signed.Keys[pubKey1.ID()] = pubKey1
-	signedRoot.Signed.Keys[pubKey2.ID()] = pubKey2
-	signedRoot.Signed.Roles[data.CanonicalRootRole].KeyIDs = []string{pubKey1.ID(), pubKey2.ID()}
+	signedRoot.Signed.Roles[data.CanonicalRootRole].KeyIDs = keyIDs
+	signedRoot.Signed.Roles[data.CanonicalRootRole].Threshold = 2
 
 	signedObj, err := signedRoot.ToSigned()
 	require.NoError(t, err)
-	require.NoError(t, signed.Sign(serverSwizzler.CryptoService, signedObj, []data.PublicKey{pubKey1}, 1, nil))
+	// sign with the first two keys
+	require.NoError(t, signed.Sign(serverSwizzler.CryptoService, signedObj, threeKeys[:2], 2, nil))
 	rootBytes, err = json.Marshal(signedObj)
 	require.NoError(t, err)
 	require.NoError(t, serverSwizzler.MetadataCache.SetMeta(data.CanonicalRootRole, rootBytes))
@@ -1359,19 +1364,35 @@ func TestValidateRootRotationWithOldRole(t *testing.T) {
 	// Updating success
 	require.NoError(t, repo.Update(false))
 
-	// update one of the keys
-	pubKey3, err := testutils.CreateKey(
+	// replace the first key with a different key and change the threshold back to 1
+	replacementKey, err := testutils.CreateKey(
 		serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalRootRole)
 	require.NoError(t, err)
 	signedRoot.Signed.Version++
-	signedRoot.Signed.Keys[pubKey3.ID()] = pubKey3
-	signedRoot.Signed.Roles[data.CanonicalRootRole].KeyIDs = []string{pubKey1.ID(), pubKey2.ID(), pubKey3.ID()}
+	signedRoot.Signed.Keys[replacementKey.ID()] = replacementKey
+	signedRoot.Signed.Roles[data.CanonicalRootRole].KeyIDs = append(keyIDs[1:], replacementKey.ID())
+	signedRoot.Signed.Roles[data.CanonicalRootRole].Threshold = 1
 
-	// sign with pubkey2 only, which satisfies both the old and new role
+	// signing with just the second key will not satisfy the first role, because that one
+	// has a threshold of 2, although it will satisfy the new role
 	signedObj, err = signedRoot.ToSigned()
 	require.NoError(t, err)
 	require.NoError(t, signed.Sign(
-		serverSwizzler.CryptoService, signedObj, []data.PublicKey{pubKey2}, 1, nil))
+		serverSwizzler.CryptoService, signedObj, threeKeys[1:2], 1, nil))
+	rootBytes, err = json.Marshal(signedObj)
+	require.NoError(t, err)
+	require.NoError(t, serverSwizzler.MetadataCache.SetMeta(data.CanonicalRootRole, rootBytes))
+
+	// update the hashes on both snapshot and timestamp
+	require.NoError(t, serverSwizzler.UpdateSnapshotHashes())
+	require.NoError(t, serverSwizzler.UpdateTimestampHash())
+
+	// Updating failure
+	require.Error(t, repo.Update(false))
+
+	// sign with the second and third keys, which satisfies both the old and new role
+	require.NoError(t, signed.Sign(
+		serverSwizzler.CryptoService, signedObj, threeKeys[1:], 2, nil))
 	rootBytes, err = json.Marshal(signedObj)
 	require.NoError(t, err)
 	require.NoError(t, serverSwizzler.MetadataCache.SetMeta(data.CanonicalRootRole, rootBytes))
@@ -1383,8 +1404,8 @@ func TestValidateRootRotationWithOldRole(t *testing.T) {
 	// Updating success
 	require.NoError(t, repo.Update(false))
 
-	// Update snapshot key and sign with ONLY pubkey3, which satisfies only the latest root
-	// role, and none of the previous
+	// Update snapshot key and sign with ONLY the replacement key,
+	// which satisfies only the latest root role, and none of the previous
 	signedRoot.Signed.Version++
 	snapKey, err := testutils.CreateKey(
 		serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalSnapshotRole)
@@ -1395,7 +1416,7 @@ func TestValidateRootRotationWithOldRole(t *testing.T) {
 	signedObj, err = signedRoot.ToSigned()
 	require.NoError(t, err)
 	require.NoError(t, signed.Sign(
-		serverSwizzler.CryptoService, signedObj, []data.PublicKey{pubKey3}, 1, nil))
+		serverSwizzler.CryptoService, signedObj, []data.PublicKey{replacementKey}, 1, nil))
 	rootBytes, err = json.Marshal(signedObj)
 	require.NoError(t, err)
 	require.NoError(t, serverSwizzler.MetadataCache.SetMeta(data.CanonicalRootRole, rootBytes))

server/handlers/validation_test.go

@@ -517,48 +517,81 @@ func TestValidateRootRotationMultipleKeysThreshold1(t *testing.T) {
 	require.NoError(t, err)
 }
 
-// A root rotation must be signed with old and new root keys, otherwise the
-// new root fails to validate
-func TestRootRotationNotSignedWithOldKeys(t *testing.T) {
+// A root rotation must be signed with old and new root keys such that it satisfies
+// the old and new roles, otherwise the new root fails to validate
+func TestRootRotationNotSignedWithOldKeysForOldRole(t *testing.T) {
 	repo, crypto, err := testutils.EmptyRepo("docker.com/notary")
 	require.NoError(t, err)
 	store := storage.NewMemStorage()
+	serverCrypto := copyKeys(t, crypto, data.CanonicalTimestampRole)
+
+	oldRootKeyID := repo.Root.Signed.Roles[data.CanonicalRootRole].KeyIDs[0]
+
+	// make the original root have 2 keys with a threshold of 2
+	pairedRootKeys := make([]data.PublicKey, 2)
+	for i := 0; i < len(pairedRootKeys); i++ {
+		pairedRootKeys[i], err = crypto.Create("root", "", data.ED25519Key)
+		require.NoError(t, err)
+	}
+	require.NoError(t, repo.ReplaceBaseKeys(data.CanonicalRootRole, pairedRootKeys...))
+	repo.Root.Signed.Roles[data.CanonicalRootRole].Threshold = 2
 
 	r, tg, sn, ts, err := testutils.Sign(repo)
 	require.NoError(t, err)
+	require.Len(t, r.Signatures, 3)
 	root, targets, snapshot, timestamp, err := getUpdates(r, tg, sn, ts)
 	require.NoError(t, err)
 
-	store.UpdateCurrent("testGUN", root)
+	updates := []storage.MetaUpdate{root, targets, snapshot, timestamp}
+	require.NoError(t, store.UpdateMany("testGUN", updates))
 
-	rootKey, err := crypto.Create("root", "testGUN", data.ED25519Key)
+	finalRootKey, err := crypto.Create("root", "testGUN", data.ED25519Key)
 	require.NoError(t, err)
-	rootRole, err := data.NewRole("root", 1, []string{rootKey.ID()}, nil)
-	require.NoError(t, err)
-
-	repo.Root.Signed.Roles["root"] = &rootRole.RootRole
-	repo.Root.Signed.Keys[rootKey.ID()] = rootKey
+	repo.Root.Signed.Roles[data.CanonicalRootRole].Threshold = 1
+	require.NoError(t, repo.ReplaceBaseKeys(data.CanonicalRootRole, finalRootKey))
 
 	r, err = repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole))
 	require.NoError(t, err)
-	err = signed.Sign(crypto, r, []data.PublicKey{rootKey}, 1, nil)
-	require.NoError(t, err)
+	origSigs := r.Signatures
 
-	rt, err := data.RootFromSigned(r)
-	require.NoError(t, err)
-	repo.SetRoot(rt)
+	// make sure it's signed with only one of the previous keys and the new key
+	sigs := make([]data.Signature, 0, 2)
+	for _, sig := range origSigs {
+		if sig.KeyID == pairedRootKeys[0].ID() || sig.KeyID == finalRootKey.ID() {
+			sigs = append(sigs, sig)
+		}
+	}
+	require.Len(t, sigs, 2)
+	repo.Root.Signatures = sigs
+	r.Signatures = sigs
 
 	sn, err = repo.SignSnapshot(data.DefaultExpires(data.CanonicalSnapshotRole))
 	require.NoError(t, err)
 	root, targets, snapshot, timestamp, err = getUpdates(r, tg, sn, ts)
 	require.NoError(t, err)
 
-	updates := []storage.MetaUpdate{root, targets, snapshot, timestamp}
-
-	serverCrypto := copyKeys(t, crypto, data.CanonicalTimestampRole)
-	_, err = validateUpdate(serverCrypto, "testGUN", updates, store)
+	_, err = validateUpdate(serverCrypto, "testGUN", []storage.MetaUpdate{root, snapshot}, store)
 	require.Error(t, err)
-	require.Contains(t, err.Error(), "new root was not signed with at least 1 old keys")
+	require.Contains(t, err.Error(), "new root was not signed with at least 2 old keys")
+
+	// now sign with both of the pair and the new one
+	sigs = make([]data.Signature, 0, 3)
+	for _, sig := range origSigs {
+		if sig.KeyID != oldRootKeyID {
+			sigs = append(sigs, sig)
+		}
+	}
+	require.Len(t, sigs, 3)
+	repo.Root.Signatures = sigs
+	r.Signatures = sigs
+
+	sn, err = repo.SignSnapshot(data.DefaultExpires(data.CanonicalSnapshotRole))
+	require.NoError(t, err)
+	root, targets, snapshot, timestamp, err = getUpdates(r, tg, sn, ts)
+	require.NoError(t, err)
+
+	_, err = validateUpdate(serverCrypto, "testGUN", []storage.MetaUpdate{root, snapshot}, store)
+	require.NoError(t, err)
 }
 
 // An update is not valid without the root metadata.