feat: Implement TAP-12 support (#310)

* docs: Add DCO instructions

Signed-off-by: Zachary Newman <z@znewman.net>

* feat: implement TAP-12 support

Main changes:

- allow IDs that aren't the SHA2 of the public key
- but disallow multiple distinct keys with the same ID
- test for TAP-12 compliance
  - Adding keys should disallow different keys with the same ID, but allow everything else
  - Verification should ensure that we have unique keys for each signature

Fixes #232.

Signed-off-by: Zachary Newman <z@znewman.net>

* feat: add Key ID to ErrRepeatID

Signed-off-by: Zachary Newman <z@znewman.net>

* test: add test for keyIDs case for delegations DB

Signed-off-by: Zachary Newman <z@znewman.net>

* test: clarify DB tests for TAP-12

Signed-off-by: Zachary Newman <z@znewman.net>

client/client.go

@@ -499,15 +499,7 @@ func (c *Client) loadAndVerifyRootMeta(rootJSON []byte, ignoreExpiredCheck bool)
 	ndb := verify.NewDB()
 	for id, k := range root.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 err
-			}
+			return err
 		}
 	}
 	for name, role := range root.Roles {
@@ -555,15 +547,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

@@ -103,15 +103,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

@@ -53,13 +53,23 @@ func NewDBFromDelegations(d *data.Delegations) (*DB, error) {
 }
 
 func (db *DB) AddKey(id string, k *data.PublicKey) error {
-	if !k.ContainsID(id) {
-		return ErrWrongID{}
-	}
 	verifier, err := keys.GetVerifier(k)
 	if err != nil {
 		return ErrInvalidKey
 	}
+
+	// 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 checking k.ContainsID(id)
+	//
+	// AddKey should be idempotent, so we allow re-adding the same PublicKey.
+	//
+	// TAP-12: https://github.com/theupdateframework/taps/blob/master/tap12.md
+	if oldVerifier, exists := db.verifiers[id]; exists && oldVerifier.Public() != verifier.Public() {
+		return ErrRepeatID{id}
+	}
+
 	db.verifiers[id] = verifier
 	return nil
 }
@@ -74,9 +84,6 @@ func (db *DB) AddRole(name string, r *data.Role) error {
 		Threshold: r.Threshold,
 	}
 	for _, id := range r.KeyIDs {
-		if len(id) != data.KeyIDLength {
-			return ErrInvalidKeyID
-		}
 		role.KeyIDs[id] = struct{}{}
 	}
 

verify/db_test.go

@@ -5,9 +5,12 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/theupdateframework/go-tuf/data"
+	"github.com/theupdateframework/go-tuf/pkg/keys"
 )
 
 func TestDelegationsDB(t *testing.T) {
+	key, err := keys.GenerateEd25519Key()
+	assert.Nil(t, err, "generating key failed")
 	var dbTests = []struct {
 		testName     string
 		delegations  *data.Delegations
@@ -34,11 +37,40 @@ func TestDelegationsDB(t *testing.T) {
 			initErr: ErrInvalidThreshold,
 		},
 		{
-			testName: "invalid keys",
-			delegations: &data.Delegations{Keys: map[string]*data.PublicKey{
-				"a": {Type: data.KeySchemeEd25519},
-			}},
-			initErr: ErrWrongID{},
+			testName: "standard (SHA256) key IDs supported",
+			delegations: &data.Delegations{
+				Keys: map[string]*data.PublicKey{
+					key.PublicData().IDs()[0]: key.PublicData(),
+				},
+				Roles: []data.DelegatedRole{{
+					Name:      "rolename",
+					KeyIDs:    key.PublicData().IDs(),
+					Threshold: 1,
+				},
+				},
+			},
+			// If we get to ErrNoSignatures, we've passed key loading; see
+			// delegations_test.go to see tests that delegation DB *fully* works
+			// with valid signatures set up.
+			unmarshalErr: ErrNoSignatures,
+		},
+		{
+			testName: "arbitrary (non-SHA256, per TAP-12) key IDs supported",
+			delegations: &data.Delegations{
+				Keys: map[string]*data.PublicKey{
+					"a": key.PublicData(),
+				},
+				Roles: []data.DelegatedRole{{
+					Name:      "rolename",
+					KeyIDs:    []string{"a"},
+					Threshold: 1,
+				},
+				},
+			},
+			// If we get to ErrNoSignatures, we've passed key loading; see
+			// delegations_test.go to see tests that delegation DB *fully* works
+			// with valid signatures set up.
+			unmarshalErr: ErrNoSignatures,
 		},
 	}
 
@@ -55,3 +87,46 @@ func TestDelegationsDB(t *testing.T) {
 		})
 	}
 }
+
+// Test key database for compliance with TAP-12.
+//
+// Previously, every key's key ID was the SHA256 of the public key. TAP-12
+// allows arbitrary key IDs, with no loss in security.
+//
+//
+// TAP-12: https://github.com/theupdateframework/taps/blob/master/tap12.md
+func TestTAP12(t *testing.T) {
+	db := NewDB()
+	// Need to use a signer type that supports random signatures.
+	key1, _ := keys.GenerateRsaKey()
+	key2, _ := keys.GenerateRsaKey()
+	msg := []byte("{}")
+	sig1, _ := key1.SignMessage(msg)
+	sig1Duplicate, _ := key1.SignMessage(msg)
+	assert.NotEqual(t, sig1, sig1Duplicate, "Signatures should be random")
+	sig2, _ := key2.SignMessage(msg)
+
+	// Idempotent: adding the same key with the same ID is okay.
+	assert.Nil(t, db.AddKey("key1", key1.PublicData()), "initial add")
+	assert.Nil(t, db.AddKey("key1", key1.PublicData()), "re-add")
+	// Adding a different key is allowed, unless the key ID is the same.
+	assert.Nil(t, db.AddKey("key2", key2.PublicData()), "different key with different ID")
+	assert.ErrorIs(t, db.AddKey("key1", key2.PublicData()), ErrRepeatID{"key1"}, "different key with same key ID")
+	assert.Nil(t, db.AddKey("key1-duplicate", key1.PublicData()), "same key with different ID should succeed")
+	assert.Nil(t, db.AddRole("diffkeys", &data.Role{
+		KeyIDs:    []string{"key1", "key2"},
+		Threshold: 2,
+	}), "adding role")
+	assert.Nil(t, db.AddRole("samekeys", &data.Role{
+		KeyIDs:    []string{"key1", "key1-alt"},
+		Threshold: 2,
+	}), "adding role")
+	assert.Nil(t, db.VerifySignatures(&data.Signed{
+		Signed:     msg,
+		Signatures: []data.Signature{{KeyID: "key1", Signature: sig1}, {KeyID: "key2", Signature: sig2}},
+	}, "diffkeys"), "Signature with different keys: ")
+	assert.ErrorIs(t, db.VerifySignatures(&data.Signed{
+		Signed:     msg,
+		Signatures: []data.Signature{{KeyID: "key1", Signature: sig1}, {KeyID: "key1-alt", Signature: sig1Duplicate}},
+	}, "samekeys"), ErrRoleThreshold{2, 1}, "Threshold signing with repeat key")
+}

verify/errors.go

@@ -21,10 +21,12 @@ var (
 	ErrMissingTargetFile    = errors.New("tuf: missing previously listed targets metadata file")
 )
 
-type ErrWrongID struct{}
+type ErrRepeatID struct {
+	KeyID string
+}
 
-func (ErrWrongID) Error() string {
-	return "tuf: key id mismatch"
+func (e ErrRepeatID) Error() string {
+	return fmt.Sprintf("tuf: duplicate key id (%s)", e.KeyID)
 }
 
 type ErrUnknownRole struct {