diff --git a/client/client.go b/client/client.go index 0241a74cb..b7e30b245 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 7064782ae..2354ef4f9 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 657c2c558..2a43e9f65 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{} + } + 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/db_test.go b/verify/db_test.go index 84031ffba..e1e9e18e5 100644 --- a/verify/db_test.go +++ b/verify/db_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/theupdateframework/go-tuf/data" + "github.com/theupdateframework/go-tuf/pkg/keys" ) func TestDelegationsDB(t *testing.T) { @@ -33,13 +34,6 @@ func TestDelegationsDB(t *testing.T) { }}, initErr: ErrInvalidThreshold, }, - { - testName: "invalid keys", - delegations: &data.Delegations{Keys: map[string]*data.PublicKey{ - "a": {Type: data.KeySchemeEd25519}, - }}, - initErr: ErrWrongID{}, - }, } for _, tt := range dbTests { @@ -55,3 +49,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{}, "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") +} diff --git a/verify/errors.go b/verify/errors.go index f94321e29..523fb58f2 100644 --- a/verify/errors.go +++ b/verify/errors.go @@ -21,10 +21,10 @@ var ( ErrMissingTargetFile = errors.New("tuf: missing previously listed targets metadata file") ) -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 {