Add Reason to ErrInvalidKeys (#237)

Leads to better error messages throughout. Also improve repo_test
coverage to exercise all of the ErrInvalidKeys paths.

Signed-off-by: Zachary Newman <[email protected]>

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 {

repo.go

@@ -152,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
@@ -168,15 +173,15 @@ 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 {
 		return err
 	}
 	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.
@@ -283,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 {
@@ -316,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) {
@@ -414,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) {
@@ -517,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)
@@ -553,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.
@@ -563,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

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) {
@@ -1386,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)
 
@@ -1598,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")
@@ -1607,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)
@@ -1727,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)
@@ -1749,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