Repository: theupdateframework/go-tuf (go)

Issue:
Implement TAP-12 for future-proofing
With the python-tuf implementation, I think that we can move TAP 12 to accepted pretty quickly. Unless there are strong objections to the TAP that will lead to major changes (which I haven't seen), I'd argue for future-proofing go-tuf by implementing TAP 12 now. It may make the implementation temporarily out of sync with the specification, but not in a major way, and not in a way that affects security (as described in more detail in [the TAP](https://github.com/theupdateframework/taps/blob/master/tap12.md)).

_Originally posted by @mnm678 in https://github.com/theupdateframework/go-tuf/issues/228#issuecomment-1059332867_

@mnm678: I think I agree with you, but I want to clarify what that would mean:

- [X] We can just keep *producing* keyIDs using SHA2, no change needed there
- [ ] We need to double-check that we don't add multiple distinct keys with the same keyID (this is done, but we should allow adding multiple *identical* keys to prevent errors)
- [ ] Make sure keys are managed locally to each metadata file, not globally (I think this is the case but need to double-check)
- [ ] During signature verification, need to enforce the uniqueness of each *key*

Pull request description:
feat: Implement TAP-12 support
Please fill in the fields below to submit a pull request.  The more information that is provided, the better.

Fixes #232
Release Notes: Add support for repositories that implement TAP-12 (allowing arbitrary key IDs). go-tuf itself will continue to emit pre-TAP-12-compliant key IDs.

**Types of changes**:
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

**Description of the changes being introduced by the pull request**:

Allows arbitrary key IDs in repos that we read, per [TAP-12](https://github.com/theupdateframework/taps/blob/master/tap12.md). (We will still only produce key IDs that are the SHA2 of the public key.)

**Please verify and check that the pull request fulfills the following requirements**:

- [X] Tests have been added for the bug fix or new feature
- [ ] (N/A)  Docs have been added for the bug fix or new feature


Patch:
diff --git a/client/client.go b/client/client.go
index 0241a74..b7e30b2 100644
--- a/client/client.go
+++ b/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 {
diff --git a/repo.go b/repo.go
index 7064782..2354ef4 100644
--- a/repo.go
+++ b/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 {
diff --git a/verify/db.go b/verify/db.go
index 657c2c5..b67ff26 100644
--- a/verify/db.go
+++ b/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{}{}
 	}
 
diff --git a/verify/errors.go b/verify/errors.go
index f94321e..f71d4bd 100644
--- a/verify/errors.go
+++ b/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 {


Test patch:
diff --git a/verify/db_test.go b/verify/db_test.go
index 84031ff..2e26b3c 100644
--- a/verify/db_test.go
+++ b/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")
+}