Repository: theupdateframework/go-tuf
Pull request #237 (2022-03-20 13:39:12)

Add Reason to ErrInvalidKeys
Leads to better error messages throughout. Also improve repo_test
coverage to exercise all of the ErrInvalidKeys paths.

Signed-off-by: Zachary Newman <z@znewman.net>

coveralls: ## Pull Request Test Coverage Report for [Build 2011969605](https://coveralls.io/builds/47518676)

* **6** of **9**   **(66.67%)**  changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.7%**) to **71.375%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [errors.go](https://coveralls.io/builds/47518676/source?filename=errors.go#L54) | 0 | 1 | 0.0%
| [repo.go](https://coveralls.io/builds/47518676/source?filename=repo.go#L194) | 6 | 8 | 75.0%
<!-- | **Total:** | **6** | **9** | **66.67%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/47518676/badge)](https://coveralls.io/builds/47518676) |
| :-- | --: |
| Change from base [Build 2000571104](https://coveralls.io/builds/47471002): |  0.7% |
| Covered Lines: | 2279 |
| Relevant Lines: | 3193 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
new file mode 100644
index 0000000..eda42a0
--- /dev/null
+++ b/.github/PULL_REQUEST_TEMPLATE.md
@@ -0,0 +1,17 @@
+Please fill in the fields below to submit a pull request.  The more information that is provided, the better.
+
+Fixes #<Issue>
+Release Notes: <!-- What comments/remarks should we include in the release notes for this change? -->
+
+**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)
+- [  ] 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**:
+
+**Please verify and check that the pull request fulfills the following requirements**:
+
+- [ ] Tests have been added for the bug fix or new feature
+- [ ] Docs have been added for the bug fix or new feature
diff --git a/errors.go b/errors.go
index 33d67c4..1a8ab9f 100644
--- a/errors.go
+++ b/errors.go
@@ -46,11 +46,12 @@ func (e ErrInsufficientSignatures) Error() string {
 }
 
 type ErrInvalidRole struct {
-	Role string
+	Role   string
+	Reason string
 }
 
 func (e ErrInvalidRole) Error() string {
-	return fmt.Sprintf("tuf: invalid role %s", e.Role)
+	return fmt.Sprintf("tuf: invalid role %s: %s", e.Role, e.Reason)
 }
 
 type ErrInvalidExpires struct {
diff --git a/repo.go b/repo.go
index c436195..53a65a7 100644
--- a/repo.go
+++ b/repo.go
@@ -74,11 +74,14 @@ func (r *Repo) Init(consistentSnapshot bool) error {
 	root.ConsistentSnapshot = consistentSnapshot
 	// Set root version to 1 for a new root.
 	root.Version = 1
-	err = r.setTopLevelMeta("root.json", root)
-	if err == nil {
-		fmt.Println("Repository initialized")
+	if err = r.setTopLevelMeta("root.json", root); err != nil {
+		return err
 	}
-	return err
+	if err = r.writeTargetWithExpires(t, data.DefaultExpires("targets")); err != nil {
+		return err
+	}
+	fmt.Println("Repository initialized")
+	return nil
 }
 
 func (r *Repo) topLevelKeysDB() (*verify.DB, error) {
@@ -149,13 +152,18 @@ func (r *Repo) RootVersion() (int, error) {
 }
 
 func (r *Repo) GetThreshold(keyRole string) (int, error) {
+	if !roles.IsTopLevelRole(keyRole) {
+		// Delegations are not currently supported, so return an error if this is not a
+		// top-level metadata file.
+		return -1, ErrInvalidRole{keyRole, "only thresholds for top-level roles supported"}
+	}
 	root, err := r.root()
 	if err != nil {
 		return -1, err
 	}
 	role, ok := root.Roles[keyRole]
 	if !ok {
-		return -1, ErrInvalidRole{keyRole}
+		return -1, ErrInvalidRole{keyRole, "role missing from root metadata"}
 	}
 
 	return role.Threshold, nil
@@ -165,7 +173,7 @@ func (r *Repo) SetThreshold(keyRole string, t int) error {
 	if !roles.IsTopLevelRole(keyRole) {
 		// Delegations are not currently supported, so return an error if this is not a
 		// top-level metadata file.
-		return ErrInvalidRole{keyRole}
+		return ErrInvalidRole{keyRole, "only thresholds for top-level roles supported"}
 	}
 	root, err := r.root()
 	if err != nil {
@@ -173,7 +181,7 @@ func (r *Repo) SetThreshold(keyRole string, t int) error {
 	}
 	role, ok := root.Roles[keyRole]
 	if !ok {
-		return ErrInvalidRole{keyRole}
+		return ErrInvalidRole{keyRole, "role missing from root metadata"}
 	}
 	if role.Threshold == t {
 		// Change was a no-op.
@@ -280,7 +288,7 @@ func (r *Repo) timestamp() (*data.Timestamp, error) {
 
 func (r *Repo) ChangePassphrase(keyRole string) error {
 	if !roles.IsTopLevelRole(keyRole) {
-		return ErrInvalidRole{keyRole}
+		return ErrInvalidRole{keyRole, "only support passphrases for top-level roles"}
 	}
 
 	if p, ok := r.local.(PassphraseChanger); ok {
@@ -313,7 +321,7 @@ func (r *Repo) AddPrivateKey(role string, signer keys.Signer) error {
 
 func (r *Repo) AddPrivateKeyWithExpires(keyRole string, signer keys.Signer, expires time.Time) error {
 	if !roles.IsTopLevelRole(keyRole) {
-		return ErrInvalidRole{keyRole}
+		return ErrInvalidRole{keyRole, "only support adding keys for top-level roles"}
 	}
 
 	if !validExpires(expires) {
@@ -411,7 +419,7 @@ func (r *Repo) RevokeKey(role, id string) error {
 
 func (r *Repo) RevokeKeyWithExpires(keyRole, id string, expires time.Time) error {
 	if !roles.IsTopLevelRole(keyRole) {
-		return ErrInvalidRole{keyRole}
+		return ErrInvalidRole{keyRole, "only revocations for top-level roles supported"}
 	}
 
 	if !validExpires(expires) {
@@ -514,7 +522,7 @@ func (r *Repo) setTopLevelMeta(roleFilename string, meta interface{}) error {
 func (r *Repo) Sign(roleFilename string) error {
 	role := strings.TrimSuffix(roleFilename, ".json")
 	if !roles.IsTopLevelRole(role) {
-		return ErrInvalidRole{role}
+		return ErrInvalidRole{role, "only signing top-level metadata supported"}
 	}
 
 	s, err := r.SignedMeta(roleFilename)
@@ -550,7 +558,7 @@ func (r *Repo) Sign(roleFilename string) error {
 func (r *Repo) AddOrUpdateSignature(roleFilename string, signature data.Signature) error {
 	role := strings.TrimSuffix(roleFilename, ".json")
 	if !roles.IsTopLevelRole(role) {
-		return ErrInvalidRole{role}
+		return ErrInvalidRole{role, "only signing top-level metadata supported"}
 	}
 
 	// Check key ID is in valid for the role.
@@ -560,7 +568,7 @@ func (r *Repo) AddOrUpdateSignature(roleFilename string, signature data.Signatur
 	}
 	roleData := db.GetRole(role)
 	if roleData == nil {
-		return ErrInvalidRole{role}
+		return ErrInvalidRole{role, "role missing from top-level keys"}
 	}
 	if !roleData.ValidKey(signature.KeyID) {
 		return verify.ErrInvalidKey
@@ -738,7 +746,7 @@ func (r *Repo) writeTargetWithExpires(t *data.Targets, expires time.Time) error
 	}
 
 	err := r.setTopLevelMeta("targets.json", t)
-	if err == nil {
+	if err == nil && len(t.Targets) > 0 {
 		fmt.Println("Added/staged targets:")
 		for k := range t.Targets {
 			fmt.Println("*", k)


Test patch:
diff --git a/repo_test.go b/repo_test.go
index 1e46147..3364373 100644
--- a/repo_test.go
+++ b/repo_test.go
@@ -184,7 +184,7 @@ func (rs *RepoSuite) TestGenKey(c *C) {
 
 	// generate a key for an unknown role
 	_, err = r.GenKey("foo")
-	c.Assert(err, Equals, ErrInvalidRole{"foo"})
+	c.Assert(err, Equals, ErrInvalidRole{"foo", "only support adding keys for top-level roles"})
 
 	// generate a root key
 	ids := genKey(c, r, "root")
@@ -346,7 +346,7 @@ func (rs *RepoSuite) TestAddPrivateKey(c *C) {
 	signer, err := keys.GenerateEd25519Key()
 	c.Assert(err, IsNil)
 	err = r.AddPrivateKey("foo", signer)
-	c.Assert(err, Equals, ErrInvalidRole{"foo"})
+	c.Assert(err, Equals, ErrInvalidRole{"foo", "only support adding keys for top-level roles"})
 
 	// add a root key
 	ids := addPrivateKey(c, r, "root", signer)
@@ -511,7 +511,7 @@ func (rs *RepoSuite) TestRevokeKey(c *C) {
 	c.Assert(err, IsNil)
 
 	// revoking a key for an unknown role returns ErrInvalidRole
-	c.Assert(r.RevokeKey("foo", ""), DeepEquals, ErrInvalidRole{"foo"})
+	c.Assert(r.RevokeKey("foo", ""), DeepEquals, ErrInvalidRole{"foo", "only revocations for top-level roles supported"})
 
 	// revoking a key which doesn't exist returns ErrKeyNotFound
 	c.Assert(r.RevokeKey("root", "nonexistent"), DeepEquals, ErrKeyNotFound{"root", "nonexistent"})
@@ -632,6 +632,8 @@ func (rs *RepoSuite) TestSign(c *C) {
 	r, err := NewRepo(local)
 	c.Assert(err, IsNil)
 
+	c.Assert(r.Sign("foo.json"), Equals, ErrInvalidRole{"foo", "only signing top-level metadata supported"})
+
 	// signing with no keys returns ErrInsufficientKeys
 	c.Assert(r.Sign("root.json"), Equals, ErrInsufficientKeys{"root.json"})
 
@@ -676,6 +678,9 @@ func (rs *RepoSuite) TestSign(c *C) {
 	c.Assert(local.SaveSigner("root", newKey), IsNil)
 	c.Assert(r.Sign("root.json"), IsNil)
 	checkSigIDs(append(signer.PublicData().IDs(), newKey.PublicData().IDs()...)...)
+
+	// attempt to sign missing metadata
+	c.Assert(r.Sign("targets.json"), Equals, ErrMissingMetadata{"targets.json"})
 }
 
 func (rs *RepoSuite) TestCommit(c *C) {
@@ -687,13 +692,15 @@ func (rs *RepoSuite) TestCommit(c *C) {
 	// commit without root.json
 	c.Assert(r.Commit(), DeepEquals, ErrMissingMetadata{"root.json"})
 
-	// commit without targets.json
+	// Init should create targets.json, but not signed yet
+	r.Init(false)
+	c.Assert(r.Commit(), DeepEquals, ErrMissingMetadata{"snapshot.json"})
+
 	genKey(c, r, "root")
-	c.Assert(r.Commit(), DeepEquals, ErrMissingMetadata{"targets.json"})
 
 	// commit without snapshot.json
 	genKey(c, r, "targets")
-	c.Assert(r.AddTarget("foo.txt", nil), IsNil)
+	c.Assert(r.Sign("targets.json"), IsNil)
 	c.Assert(r.Commit(), DeepEquals, ErrMissingMetadata{"snapshot.json"})
 
 	// commit without timestamp.json
@@ -714,12 +721,12 @@ func (rs *RepoSuite) TestCommit(c *C) {
 	// commit with an invalid root hash in snapshot.json due to new key creation
 	genKey(c, r, "targets")
 	c.Assert(r.Sign("targets.json"), IsNil)
-	c.Assert(r.Commit(), DeepEquals, errors.New("tuf: invalid targets.json in snapshot.json: wrong length, expected 511 got 725"))
+	c.Assert(r.Commit(), DeepEquals, errors.New("tuf: invalid targets.json in snapshot.json: wrong length, expected 338 got 552"))
 
 	// commit with an invalid targets hash in snapshot.json
 	c.Assert(r.Snapshot(), IsNil)
 	c.Assert(r.AddTarget("bar.txt", nil), IsNil)
-	c.Assert(r.Commit(), DeepEquals, errors.New("tuf: invalid targets.json in snapshot.json: wrong length, expected 725 got 899"))
+	c.Assert(r.Commit(), DeepEquals, errors.New("tuf: invalid targets.json in snapshot.json: wrong length, expected 552 got 725"))
 
 	// commit with an invalid timestamp
 	c.Assert(r.Snapshot(), IsNil)
@@ -1384,6 +1391,12 @@ func (rs *RepoSuite) TestKeyPersistence(c *C) {
 	tmp = newTmpDir(c)
 	store = FileSystemStore(tmp.path, testPassphraseFunc)
 
+	// 1.5. Changing passphrase only works for top-level roles.
+	r, err := NewRepo(store)
+	c.Assert(err, IsNil)
+
+	c.Assert(r.ChangePassphrase("foo"), DeepEquals, ErrInvalidRole{"foo", "only support passphrases for top-level roles"})
+
 	// 2. Test changing the passphrase when the keys file does not exist - should FAIL
 	c.Assert(store.(PassphraseChanger).ChangePassphrase("root"), NotNil)
 
@@ -1596,6 +1609,11 @@ func (rs *RepoSuite) TestThreshold(c *C) {
 	r, err := NewRepo(local)
 	c.Assert(err, IsNil)
 
+	_, err = r.GetThreshold("root")
+	c.Assert(err, DeepEquals, ErrInvalidRole{"root", "role missing from root metadata"})
+	err = r.SetThreshold("root", 2)
+	c.Assert(err, DeepEquals, ErrInvalidRole{"root", "role missing from root metadata"})
+
 	// Add one key to each role
 	genKey(c, r, "root")
 	genKey(c, r, "targets")
@@ -1605,6 +1623,11 @@ func (rs *RepoSuite) TestThreshold(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(t, Equals, 1)
 
+	_, err = r.GetThreshold("foo")
+	c.Assert(err, DeepEquals, ErrInvalidRole{"foo", "only thresholds for top-level roles supported"})
+	err = r.SetThreshold("foo", 2)
+	c.Assert(err, DeepEquals, ErrInvalidRole{"foo", "only thresholds for top-level roles supported"})
+
 	// commit the metadata to the store.
 	c.Assert(r.AddTargets([]string{}, nil), IsNil)
 	c.Assert(r.Snapshot(), IsNil)
@@ -1725,6 +1748,10 @@ func (rs *RepoSuite) TestBadAddOrUpdateSignatures(c *C) {
 	// don't use consistent snapshots to make the checks simpler
 	c.Assert(r.Init(false), IsNil)
 
+	c.Assert(r.AddOrUpdateSignature("targets.json", data.Signature{
+		KeyID:     "foo",
+		Signature: nil}), Equals, ErrInvalidRole{"targets", "role missing from top-level keys"})
+
 	// generate root key offline and add as a verification key
 	rootKey, err := keys.GenerateEd25519Key()
 	c.Assert(err, IsNil)
@@ -1747,7 +1774,7 @@ func (rs *RepoSuite) TestBadAddOrUpdateSignatures(c *C) {
 	for _, id := range rootKey.PublicData().IDs() {
 		c.Assert(r.AddOrUpdateSignature("invalid_root.json", data.Signature{
 			KeyID:     id,
-			Signature: rootSig}), Equals, ErrInvalidRole{"invalid_root"})
+			Signature: rootSig}), Equals, ErrInvalidRole{"invalid_root", "only signing top-level metadata supported"})
 	}
 
 	// add a root signature with an key ID that is for the targets role