diff --git a/client/client.go b/client/client.go index 0241a74c..b7e30b24 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/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index be87d0ad..4336fec1 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -32,6 +32,7 @@ Pull requests should be targeted at the `master` branch. Before creating a pull 5. [Rebase](http://git-scm.com/book/en/Git-Branching-Rebasing) your local changes against the `master` branch. 6. Run the full project test suite with the `go test ./...` command and confirm that it passes (see [TESTING.md](TESTING.md) for details). 7. Run `go fmt ./...`. +8. You must agree to the [Developer Certificate of Origin](https://developercertificate.org/) for your contributions; use `git commit -s` ([detailed information here](https://wiki.linuxfoundation.org/dco)). When creating a PR: diff --git a/repo.go b/repo.go index 7064782a..2354ef4f 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 657c2c55..b67ff260 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/db_test.go b/verify/db_test.go index 84031ffb..2e26b3ce 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") +} diff --git a/verify/errors.go b/verify/errors.go index f94321e2..f71d4bda 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 {