From 405be0354a334bf5a37df1c26124ca282e05229f Mon Sep 17 00:00:00 2001 From: Zachary Newman Date: Fri, 10 Jun 2022 12:14:26 -0400 Subject: [PATCH 1/5] docs: Add DCO instructions Signed-off-by: Zachary Newman --- docs/CONTRIBUTING.md | 1 + 1 file changed, 1 insertion(+) 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: From 2cca9d707e65394f84c60f1633087570a9a348b3 Mon Sep 17 00:00:00 2001 From: Zachary Newman Date: Wed, 8 Jun 2022 18:26:34 -0400 Subject: [PATCH 2/5] 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 --- client/client.go | 20 ++----------------- repo.go | 10 +--------- verify/db.go | 19 ++++++++++++------ verify/db_test.go | 51 ++++++++++++++++++++++++++++++++++++++++------- verify/errors.go | 6 +++--- 5 files changed, 63 insertions(+), 43 deletions(-) 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/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..2a43e9f6 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 84031ffb..e1e9e18e 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 f94321e2..523fb58f 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 { From af4a95eeb01ccbab4ee440e9abc4716cc8ab687f Mon Sep 17 00:00:00 2001 From: Zachary Newman Date: Thu, 9 Jun 2022 13:33:29 -0400 Subject: [PATCH 3/5] feat: add Key ID to ErrRepeatID Signed-off-by: Zachary Newman --- verify/db.go | 2 +- verify/db_test.go | 2 +- verify/errors.go | 8 +++++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/verify/db.go b/verify/db.go index 2a43e9f6..b67ff260 100644 --- a/verify/db.go +++ b/verify/db.go @@ -67,7 +67,7 @@ func (db *DB) AddKey(id string, k *data.PublicKey) error { // // TAP-12: https://github.com/theupdateframework/taps/blob/master/tap12.md if oldVerifier, exists := db.verifiers[id]; exists && oldVerifier.Public() != verifier.Public() { - return ErrRepeatID{} + return ErrRepeatID{id} } db.verifiers[id] = verifier diff --git a/verify/db_test.go b/verify/db_test.go index e1e9e18e..b45840f5 100644 --- a/verify/db_test.go +++ b/verify/db_test.go @@ -73,7 +73,7 @@ func TestTAP12(t *testing.T) { 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.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"}, diff --git a/verify/errors.go b/verify/errors.go index 523fb58f..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 ErrRepeatID struct{} +type ErrRepeatID struct { + KeyID string +} -func (ErrRepeatID) Error() string { - return "tuf: duplicate key id" +func (e ErrRepeatID) Error() string { + return fmt.Sprintf("tuf: duplicate key id (%s)", e.KeyID) } type ErrUnknownRole struct { From a494fc28347591b735b418f2ff85b26701e8a20f Mon Sep 17 00:00:00 2001 From: Zachary Newman Date: Thu, 9 Jun 2022 13:50:42 -0400 Subject: [PATCH 4/5] test: add test for keyIDs case for delegations DB Signed-off-by: Zachary Newman --- verify/db_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/verify/db_test.go b/verify/db_test.go index b45840f5..9fd1e796 100644 --- a/verify/db_test.go +++ b/verify/db_test.go @@ -9,6 +9,8 @@ import ( ) 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,6 +36,36 @@ func TestDelegationsDB(t *testing.T) { }}, initErr: ErrInvalidThreshold, }, + { + testName: "standard 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, + }, + }, + }, + unmarshalErr: ErrNoSignatures, + }, + { + testName: "arbitrary key IDs supported", + delegations: &data.Delegations{ + Keys: map[string]*data.PublicKey{ + "a": key.PublicData(), + }, + Roles: []data.DelegatedRole{{ + Name: "rolename", + KeyIDs: []string{"a"}, + Threshold: 1, + }, + }, + }, + unmarshalErr: ErrNoSignatures, + }, } for _, tt := range dbTests { From 8b9a9b0ee9ea8ad146f56b3deff93c6e35b47fcc Mon Sep 17 00:00:00 2001 From: Zachary Newman Date: Fri, 10 Jun 2022 10:05:48 -0400 Subject: [PATCH 5/5] test: clarify DB tests for TAP-12 Signed-off-by: Zachary Newman --- verify/db_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/verify/db_test.go b/verify/db_test.go index 9fd1e796..2e26b3ce 100644 --- a/verify/db_test.go +++ b/verify/db_test.go @@ -37,7 +37,7 @@ func TestDelegationsDB(t *testing.T) { initErr: ErrInvalidThreshold, }, { - testName: "standard key IDs supported", + testName: "standard (SHA256) key IDs supported", delegations: &data.Delegations{ Keys: map[string]*data.PublicKey{ key.PublicData().IDs()[0]: key.PublicData(), @@ -49,10 +49,13 @@ func TestDelegationsDB(t *testing.T) { }, }, }, + // 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 key IDs supported", + testName: "arbitrary (non-SHA256, per TAP-12) key IDs supported", delegations: &data.Delegations{ Keys: map[string]*data.PublicKey{ "a": key.PublicData(), @@ -64,6 +67,9 @@ func TestDelegationsDB(t *testing.T) { }, }, }, + // 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, }, }