Finish TAP-12 futureproofing

[TAP-12] would allow arbitrary key IDs (rather than requiring the SHA256
of the public key itself). Before, we claimed (in a comment) to support
it but did not in practice: no error, but keys would not be added.

For security, it's important that once we allow arbitrary key IDs that
there are no duplicate key IDs to prevent "confusion attacks."

Here, we:

- actually add keys that don't match, don't just silently ignore
- add a test that duplicate key IDs cause an error

[TAP-12]: https://github.com/theupdateframework/taps/blob/master/tap12.md

client/client.go

@@ -513,15 +513,7 @@ func (c *Client) verifyRoot(aJSON []byte, bJSON []byte) (*data.Root, error) {
 	ndb := verify.NewDB()
 	for id, k := range aRoot.Keys {
 		if err := ndb.AddKey(id, k); err != nil {
-			// TUF is considering in TAP-12 removing the
-			// requirement that the keyid hash algorithm be derived
-			// from the public key. So to be forwards compatible,
-			// we ignore `ErrWrongID` errors.
-			//
-			// TAP-12: https://github.com/theupdateframework/taps/blob/master/tap12.md
-			if _, ok := err.(verify.ErrWrongID); !ok {
-				return nil, err
-			}
+			return nil, err
 		}
 	}
 	for name, role := range aRoot.Roles {

repo.go

@@ -127,15 +127,7 @@ func (r *Repo) topLevelKeysDB() (*verify.DB, error) {
 	}
 	for id, k := range root.Keys {
 		if err := db.AddKey(id, k); err != nil {
-			// TUF is considering in TAP-12 removing the
-			// requirement that the keyid hash algorithm be derived
-			// from the public key. So to be forwards compatible,
-			// we ignore `ErrWrongID` errors.
-			//
-			// TAP-12: https://github.com/theupdateframework/taps/blob/master/tap12.md
-			if _, ok := err.(verify.ErrWrongID); !ok {
-				return nil, err
-			}
+			return nil, err
 		}
 	}
 	for name, role := range root.Roles {

verify/db.go

@@ -62,13 +62,14 @@ func NewDelegationsVerifier(d *data.Delegations) (DelegationsVerifier, error) {
 }
 
 func (db *DB) AddKey(id string, k *data.PublicKey) error {
-	if !k.ContainsID(id) {
-		// TUF is considering in TAP-12 removing the
-		// requirement that the keyid hash algorithm be derived
-		// from the public key. So to be forwards compatible,
-		// we ignore `ErrWrongID` errors.
-		//
-		// TAP-12: https://github.com/theupdateframework/taps/blob/master/tap12.md
+	// TUF is considering in TAP-12 removing the
+	// requirement that the keyid hash algorithm be derived
+	// from the public key. So to be forwards compatible,
+	// we allow any key ID, rather than check k.ContainsID(id)
+	//
+	// TAP-12: https://github.com/theupdateframework/taps/blob/master/tap12.md
+	if _, exists := db.verifiers[id]; exists {
+		return ErrRepeatID{}
 	}
 	verifier, err := keys.GetVerifier(k)
 	if err != nil {

verify/db_test.go

@@ -1,10 +1,12 @@
 package verify
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/theupdateframework/go-tuf/data"
+	"github.com/theupdateframework/go-tuf/pkg/keys"
 )
 
 func TestDelegationsVerifier(t *testing.T) {
@@ -33,13 +35,6 @@ func TestDelegationsVerifier(t *testing.T) {
 			}},
 			initErr: ErrInvalidThreshold,
 		},
-		{
-			testName: "invalid keys",
-			delegations: &data.Delegations{Keys: map[string]*data.PublicKey{
-				"a": &data.PublicKey{Type: data.KeySchemeEd25519},
-			}},
-			initErr: ErrWrongID{},
-		},
 	}
 
 	for _, tt := range verifierTests {
@@ -55,3 +50,17 @@ func TestDelegationsVerifier(t *testing.T) {
 		})
 	}
 }
+
+func TestDoubleAdd(t *testing.T) {
+	db := NewDB()
+	key, _ := keys.GenerateEd25519Key()
+
+	if err := db.AddKey("keyid", key.PublicData()); err != nil {
+		t.Error("AddKey got err: ", err)
+	}
+
+	expected := ErrRepeatID{}
+	if err := db.AddKey("keyid", key.PublicData()); err != expected {
+		t.Errorf("AddKey with repeat ID: want %v, got: \"%v\"", reflect.TypeOf(expected).Name(), err)
+	}
+}

verify/errors.go

@@ -20,10 +20,10 @@ var (
 	ErrInvalidThreshold     = errors.New("tuf: invalid role threshold")
 )
 
-type ErrWrongID struct{}
+type ErrRepeatID struct{}
 
-func (ErrWrongID) Error() string {
-	return "tuf: key id mismatch"
+func (ErrRepeatID) Error() string {
+	return "tuf: duplicate key id"
 }
 
 type ErrUnknownRole struct {