fixing error message and moving signing operations up a level

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

cryptoservice/crypto_service.go

@@ -116,12 +116,10 @@ func (cs *CryptoService) RemoveKey(keyID string) (err error) {
 // signatures is adequate.
 func (cs *CryptoService) Sign(keyIDs []string, payload []byte) ([]data.Signature, error) {
 	signatures := make([]data.Signature, 0, len(keyIDs))
-	for _, keyid := range keyIDs {
+	for _, keyID := range keyIDs {
-		keyName := keyid
+		privKey, _, err := cs.GetPrivateKey(keyID)
-
-		privKey, _, err := cs.GetPrivateKey(keyName)
 		if err != nil {
-			logrus.Debugf("error attempting to retrieve private key: %s, %v", keyid, err)
+			logrus.Debugf("error attempting to retrieve private key: %s, %v", keyID, err)
 			continue
 		}
 
@@ -129,15 +127,15 @@ func (cs *CryptoService) Sign(keyIDs []string, payload []byte) ([]data.Signature
 		sig, err := privKey.Sign(rand.Reader, payload, nil)
 		if err != nil {
 			logrus.Debugf("ignoring error attempting to %s sign with keyID: %s, %v",
-				privKey.Algorithm(), keyid, err)
+				privKey.Algorithm(), keyID, err)
 			continue
 		}
 
-		logrus.Debugf("appending %s signature with Key ID: %s", privKey.Algorithm(), keyid)
+		logrus.Debugf("appending %s signature with Key ID: %s", privKey.Algorithm(), keyID)
 
 		// Append signatures to result array
 		signatures = append(signatures, data.Signature{
-			KeyID:     keyid,
+			KeyID:     keyID,
 			Method:    sigAlgo,
 			Signature: sig[:],
 		})

tuf/signed/ed25519.go

@@ -7,6 +7,7 @@ import (
 	"io/ioutil"
 
 	"github.com/agl/ed25519"
+	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 )
 
@@ -83,18 +84,13 @@ func (e *Ed25519) Create(role, algorithm string) (data.PublicKey, error) {
 		return nil, errors.New("only ED25519 supported by this cryptoservice")
 	}
 
-	pub, priv, err := ed25519.GenerateKey(rand.Reader)
+	private, err := trustmanager.GenerateED25519Key(rand.Reader)
-	if err != nil {
-		return nil, err
-	}
-	public := data.NewED25519PublicKey(pub[:])
-	private, err := data.NewED25519PrivateKey(*public, priv[:])
 	if err != nil {
 		return nil, err
 	}
 
 	e.addKey(role, private)
-	return public, nil
+	return data.PublicKeyFromPrivate(private), nil
 }
 
 // PublicKeys returns a map of public keys for the ids provided, when those IDs are found
@@ -116,7 +112,10 @@ func (e *Ed25519) GetKey(keyID string) data.PublicKey {
 
 // GetPrivateKey returns a single private key based on the ID
 func (e *Ed25519) GetPrivateKey(keyID string) (data.PrivateKey, string, error) {
-	return e.keys[keyID].privKey, "", nil
+	if k, ok := e.keys[keyID]; ok {
+		return k.privKey, k.role, nil
+	}
+	return nil, "", errors.New("Key not found " + keyID)
 }
 
 // ImportRootKey adds an Ed25519 key to the store as a root key

tuf/signed/errors.go

@@ -2,6 +2,7 @@ package signed
 
 import (
 	"fmt"
+	"strings"
 )
 
 // ErrInsufficientSignatures - do not have enough signatures on a piece of
@@ -59,3 +60,13 @@ type ErrInvalidKeyLength struct {
 func (e ErrInvalidKeyLength) Error() string {
 	return fmt.Sprintf("key length is not supported: %s", e.msg)
 }
+
+// ErrNoKeys indicates no signing keys were found when trying to sign
+type ErrNoKeys struct {
+	keyIDs []string
+}
+
+func (e ErrNoKeys) Error() string {
+	return fmt.Sprintf("could not find necessary signing keys, at least one of these keys must be available: %s",
+		strings.Join(e.keyIDs, ", "))
+}

tuf/signed/sign.go

@@ -12,6 +12,7 @@ package signed
 // for which the root key is wrapped using an x509 certificate.
 
 import (
+	"crypto/rand"
 	"fmt"
 
 	"github.com/Sirupsen/logrus"
@@ -19,62 +20,62 @@ import (
 	"github.com/docker/notary/tuf/utils"
 )
 
-type idPair struct {
-	scopedKeyID    string
-	canonicalKeyID string
-}
-
 // Sign takes a data.Signed and a key, calculated and adds the signature
 // to the data.Signed
 func Sign(service CryptoService, s *data.Signed, keys ...data.PublicKey) error {
 	logrus.Debugf("sign called with %d keys", len(keys))
 	signatures := make([]data.Signature, 0, len(s.Signatures)+1)
-	keyIDMemb := make(map[string]struct{})
+	signingKeyIDs := make(map[string]struct{})
-	keyIDs := make(
+	ids := make([]string, 0, len(keys))
-		[]idPair,
-		0,
-		len(keys),
-	)
 
+	privKeys := make(map[string]data.PrivateKey)
+
+	// Get all the private key objects related to the public keys
 	for _, key := range keys {
-		keyID, err := utils.CanonicalKeyID(key)
+		canonicalID, err := utils.CanonicalKeyID(key)
+		ids = append(ids, canonicalID)
 		if err != nil {
 			continue
 		}
-		keyIDMemb[key.ID()] = struct{}{}
+		k, _, err := service.GetPrivateKey(canonicalID)
-		keyIDs = append(keyIDs, idPair{
+		if err != nil {
-			scopedKeyID:    key.ID(),
+			continue
-			canonicalKeyID: keyID,
+		}
+		privKeys[key.ID()] = k
+	}
+
+	// Check to ensure we have at least on signing key
+	if len(privKeys) == 0 {
+		return ErrNoKeys{keyIDs: ids}
+	}
+
+	// Do signing and generate list of signatures
+	for keyID, pk := range privKeys {
+		sig, err := pk.Sign(rand.Reader, s.Signed, nil)
+		if err != nil {
+			logrus.Debugf("Failed to sign with key: %s. Reason: %v", keyID, err)
+			continue
+		}
+		signingKeyIDs[keyID] = struct{}{}
+		signatures = append(signatures, data.Signature{
+			KeyID:     keyID,
+			Method:    pk.SignatureAlgorithm(),
+			Signature: sig[:],
 		})
 	}
 
-	// we need to ask the signer to sign with the canonical key ID
+	// Check we produced at least on signature
-	// (ID of the TUF key's public key bytes only), but
-	// we need to translate back to the scoped key ID (the hash of the TUF key
-	// with the full PEM bytes) before giving the
-	// signature back to TUF.
-	for _, pair := range keyIDs {
-		newSigs, err := service.Sign([]string{pair.canonicalKeyID}, s.Signed)
-		if err != nil {
-			return err
-		}
-		// we only asked to sign with 1 key ID, so there will either be 1
-		// or zero signatures
-		if len(newSigs) == 1 {
-			newSig := newSigs[0]
-			newSig.KeyID = pair.scopedKeyID
-			signatures = append(signatures, newSig)
-		}
-	}
 	if len(signatures) < 1 {
 		return ErrInsufficientSignatures{
 			Name: fmt.Sprintf(
 				"Cryptoservice failed to produce any signatures for keys with IDs: %v",
-				keyIDs),
+				ids),
 		}
 	}
+
 	for _, sig := range s.Signatures {
-		if _, ok := keyIDMemb[sig.KeyID]; ok {
+		if _, ok := signingKeyIDs[sig.KeyID]; ok {
+			// key is in the set of key IDs for which a signature has been created
 			continue
 		}
 		signatures = append(signatures, sig)

tuf/signed/sign_test.go

@@ -2,7 +2,6 @@ package signed
 
 import (
 	"crypto/rand"
-	"crypto/rsa"
 	"crypto/x509"
 	"encoding/pem"
 	"errors"
@@ -17,13 +16,10 @@ import (
 const (
 	testKeyPEM1 = "-----BEGIN PUBLIC KEY-----\nMIIBojANBgkqhkiG9w0BAQEFAAOCAY8AMIIBigKCAYEAnKuXZeefa2LmgxaL5NsM\nzKOHNe+x/nL6ik+lDBCTV6OdcwAhHQS+PONGhrChIUVR6Vth3hUCrreLzPO73Oo5\nVSCuRJ53UronENl6lsa5mFKP8StYLvIDITNvkoT3j52BJIjyNUK9UKY9As2TNqDf\nBEPIRp28ev/NViwGOEkBu2UAbwCIdnDXm8JQErCZA0Ydm7PKGgjLbFsFGrVzqXHK\n6pdzJXlhr9yap3UpgQ/iO9JtoEYB2EXsnSrPc9JRjR30bNHHtnVql3fvinXrAEwq\n3xmN4p+R4VGzfdQN+8Kl/IPjqWB535twhFYEG/B7Ze8IwbygBjK3co/KnOPqMUrM\nBI8ztvPiogz+MvXb8WvarZ6TMTh8ifZI96r7zzqyzjR1hJulEy3IsMGvz8XS2J0X\n7sXoaqszEtXdq5ef5zKVxkiyIQZcbPgmpHLq4MgfdryuVVc/RPASoRIXG4lKaTJj\n1ANMFPxDQpHudCLxwCzjCb+sVa20HBRPTnzo8LSZkI6jAgMBAAE=\n-----END PUBLIC KEY-----"
 	testKeyID1  = "51324b59d4888faa91219ebbe5a3876bb4efb21f0602ddf363cd4c3996ded3d4"
-
-	testKeyPEM2 = "-----BEGIN PUBLIC KEY-----\nMIIBojANBgkqhkiG9w0BAQEFAAOCAY8AMIIBigKCAYEArvqUPYb6JJROPJQglPTj\n5uDrsxQKl34Mo+3pSlBVuD6puE4lDnG649a2YksJy+C8ZIPJgokn5w+C3alh+dMe\nzbdWHHxrY1h9CLpYz5cbMlE16303ubkt1rvwDqEezG0HDBzPaKj4oP9YJ9x7wbsq\ndvFcy+Qc3wWd7UWcieo6E0ihbJkYcY8chRXVLg1rL7EfZ+e3bq5+ojA2ECM5JqzZ\nzgDpqCv5hTCYYZp72MZcG7dfSPAHrcSGIrwg7whzz2UsEtCOpsJTuCl96FPN7kAu\n4w/WyM3+SPzzr4/RQXuY1SrLCFD8ebM2zHt/3ATLhPnGmyG5I0RGYoegFaZ2AViw\nlqZDOYnBtgDvKP0zakMtFMbkh2XuNBUBO7Sjs0YcZMjLkh9gYUHL1yWS3Aqus1Lw\nlI0gHS22oyGObVBWkZEgk/Foy08sECLGao+5VvhmGpfVuiz9OKFUmtPVjWzRE4ng\niekEu4drSxpH41inLGSvdByDWLpcTvWQI9nkgclh3AT/AgMBAAE=\n-----END PUBLIC KEY-----"
-	testKeyID2  = "26f2f5c0fbfa98823bf1ad39d5f3b32575895793baf80f1df675597d5b95dba8"
 )
 
 type FailingCryptoService struct {
-	testKey data.PublicKey
+	testKey data.PrivateKey
 }
 
 func (mts *FailingCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) {
@@ -50,13 +46,16 @@ func (mts *FailingCryptoService) ListAllKeys() map[string]string {
 
 func (mts *FailingCryptoService) GetKey(keyID string) data.PublicKey {
 	if keyID == "testID" {
-		return mts.testKey
+		return data.PublicKeyFromPrivate(mts.testKey)
 	}
 	return nil
 }
 
 func (mts *FailingCryptoService) GetPrivateKey(keyID string) (data.PrivateKey, string, error) {
-	return nil, "", errors.New("Not implemented")
+	if mts.testKey != nil {
+		return mts.testKey, "testRole", nil
+	}
+	return nil, "", errors.New("Key not found " + keyID)
 }
 
 func (mts *FailingCryptoService) RemoveKey(keyID string) error {
@@ -68,7 +67,7 @@ func (mts *FailingCryptoService) ImportRootKey(r io.Reader) error {
 }
 
 type MockCryptoService struct {
-	testKey data.PublicKey
+	testKey data.PrivateKey
 }
 
 func (mts *MockCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) {
@@ -85,7 +84,7 @@ func (mts *MockCryptoService) Create(_ string, _ string) (data.PublicKey, error)
 
 func (mts *MockCryptoService) GetKey(keyID string) data.PublicKey {
 	if keyID == "testID" {
-		return mts.testKey
+		return data.PublicKeyFromPrivate(mts.testKey)
 	}
 	return nil
 }
@@ -104,7 +103,7 @@ func (mts *MockCryptoService) ListAllKeys() map[string]string {
 }
 
 func (mts *MockCryptoService) GetPrivateKey(keyID string) (data.PrivateKey, string, error) {
-	return nil, "", errors.New("Not implemented")
+	return mts.testKey, "testRole", nil
 }
 
 func (mts *MockCryptoService) RemoveKey(keyID string) error {
@@ -133,7 +132,7 @@ func (mts *StrictMockCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Sign
 
 func (mts *StrictMockCryptoService) GetKey(keyID string) data.PublicKey {
 	if keyID == mts.testKey.ID() {
-		return mts.testKey
+		return data.PublicKeyFromPrivate(mts.testKey)
 	}
 	return nil
 }
@@ -157,70 +156,80 @@ func (mts *StrictMockCryptoService) ImportRootKey(r io.Reader) error {
 
 // Test signing and ensure the expected signature is added
 func TestBasicSign(t *testing.T) {
-	testKey, _ := pem.Decode([]byte(testKeyPEM1))
+	cs := NewEd25519()
-	k := data.NewPublicKey(data.RSAKey, testKey.Bytes)
+	key, err := cs.Create("root", data.ED25519Key)
-	mockCryptoService := &MockCryptoService{testKey: k}
+	assert.NoError(t, err)
-	key, err := mockCryptoService.Create("root", data.ED25519Key)
-	if err != nil {
-		t.Fatal(err)
-	}
 	testData := data.Signed{}
 
-	Sign(mockCryptoService, &testData, key)
+	err = Sign(cs, &testData, key)
+	assert.NoError(t, err)
 
 	if len(testData.Signatures) != 1 {
 		t.Fatalf("Incorrect number of signatures: %d", len(testData.Signatures))
 	}
 
-	if testData.Signatures[0].KeyID != testKeyID1 {
+	if testData.Signatures[0].KeyID != key.ID() {
 		t.Fatalf("Wrong signature ID returned: %s", testData.Signatures[0].KeyID)
 	}
 }
 
-// Test signing with the same key multiple times only registers a single signature
+// Signing with the same key multiple times should not produce multiple sigs
-// for the key (N.B. MockCryptoService.Sign will still be called again, but Signer.Sign
+// with the same key ID
-// should be cleaning previous signatures by the KeyID when asked to sign again)
 func TestReSign(t *testing.T) {
-	testKey, _ := pem.Decode([]byte(testKeyPEM1))
+	cs := NewEd25519()
-	k := data.NewPublicKey(data.RSAKey, testKey.Bytes)
+	key, err := cs.Create("root", data.ED25519Key)
-	mockCryptoService := &MockCryptoService{testKey: k}
+	assert.NoError(t, err)
 	testData := data.Signed{}
 
-	Sign(mockCryptoService, &testData, k)
+	Sign(cs, &testData, key)
-	Sign(mockCryptoService, &testData, k)
+	Sign(cs, &testData, key)
 
 	if len(testData.Signatures) != 1 {
 		t.Fatalf("Incorrect number of signatures: %d", len(testData.Signatures))
 	}
 
-	if testData.Signatures[0].KeyID != testKeyID1 {
+	if testData.Signatures[0].KeyID != key.ID() {
 		t.Fatalf("Wrong signature ID returned: %s", testData.Signatures[0].KeyID)
 	}
 
 }
 
+// Should not remove signatures for valid keys that were not resigned with
 func TestMultiSign(t *testing.T) {
-	mockCryptoService := &MockCryptoService{}
+	cs := NewEd25519()
 	testData := data.Signed{}
 
-	testKey, _ := pem.Decode([]byte(testKeyPEM1))
+	key1, err := cs.Create("root", data.ED25519Key)
-	key := data.NewPublicKey(data.RSAKey, testKey.Bytes)
+	assert.NoError(t, err)
-	Sign(mockCryptoService, &testData, key)
+	Sign(cs, &testData, key1)
 
-	testKey, _ = pem.Decode([]byte(testKeyPEM2))
+	// reinitializing cs means it won't know about key1. We want
-	key = data.NewPublicKey(data.RSAKey, testKey.Bytes)
+	// to attempt to sign passing both key1 and key2, while expecting
-	Sign(mockCryptoService, &testData, key)
+	// that the signature for key1 is left intact and the signature
+	// for key2 is added
+	cs = NewEd25519()
+	key2, err := cs.Create("root", data.ED25519Key)
+	assert.NoError(t, err)
+	Sign(
+		cs,
+		&testData,
+		key1,
+		key2,
+	)
 
 	if len(testData.Signatures) != 2 {
 		t.Fatalf("Incorrect number of signatures: %d", len(testData.Signatures))
 	}
 
-	keyIDs := map[string]struct{}{testKeyID1: {}, testKeyID2: {}}
+	keyIDs := map[string]struct{}{key1.ID(): {}, key2.ID(): {}}
+	count := 0
 	for _, sig := range testData.Signatures {
+		count++
 		if _, ok := keyIDs[sig.KeyID]; !ok {
 			t.Fatalf("Got a signature we didn't expect: %s", sig.KeyID)
 		}
 	}
+	assert.Equal(t, 2, count)
 
 }
 
@@ -240,32 +249,18 @@ func TestSignReturnsNoSigs(t *testing.T) {
 	}
 }
 
-func TestCreate(t *testing.T) {
-	testKey, _ := pem.Decode([]byte(testKeyPEM1))
-	k := data.NewPublicKey(data.RSAKey, testKey.Bytes)
-	mockCryptoService := &MockCryptoService{testKey: k}
-
-	key, err := mockCryptoService.Create("root", data.ED25519Key)
-
-	if err != nil {
-		t.Fatal(err)
-	}
-	if key.ID() != testKeyID1 {
-		t.Fatalf("Expected key ID not found: %s", key.ID())
-	}
-}
-
 func TestSignWithX509(t *testing.T) {
 	// generate a key becase we need a cert
-	privKey, err := rsa.GenerateKey(rand.Reader, 1024)
+	privKey, err := trustmanager.GenerateRSAKey(rand.Reader, 1024)
 	assert.NoError(t, err)
 
 	// make a RSA x509 key
 	template, err := trustmanager.NewCertificate("test")
 	assert.NoError(t, err)
 
+	signer := privKey.CryptoSigner()
 	derBytes, err := x509.CreateCertificate(
-		rand.Reader, template, template, &privKey.PublicKey, privKey)
+		rand.Reader, template, template, signer.Public(), signer)
 	assert.NoError(t, err)
 
 	cert, err := x509.ParseCertificate(derBytes)
@@ -274,14 +269,9 @@ func TestSignWithX509(t *testing.T) {
 	tufRSAx509Key := trustmanager.CertToKey(cert)
 	assert.NoError(t, err)
 
-	// make a data.PublicKey from the generated private key
-	pubBytes, err := x509.MarshalPKIXPublicKey(&privKey.PublicKey)
-	assert.NoError(t, err)
-	tufRSAKey := data.NewPublicKey(data.RSAKey, pubBytes)
-
 	// test signing against a service that only recognizes a RSAKey (not
 	// RSAx509 key)
-	mockCryptoService := &StrictMockCryptoService{MockCryptoService{tufRSAKey}}
+	mockCryptoService := &StrictMockCryptoService{MockCryptoService{privKey}}
 	testData := data.Signed{}
 	err = Sign(mockCryptoService, &testData, tufRSAx509Key)
 	assert.NoError(t, err)

tuf/signed/verify.go

@@ -65,10 +65,10 @@ func VerifyRoot(s *data.Signed, minVersion int, keys map[string]data.PublicKey)
 // Verify checks the signatures and metadata (expiry, version) for the signed role
 // data
 func Verify(s *data.Signed, role string, minVersion int, db *keys.KeyDB) error {
-	if err := VerifySignatures(s, role, db); err != nil {
+	if err := verifyMeta(s, role, minVersion); err != nil {
 		return err
 	}
-	return verifyMeta(s, role, minVersion)
+	return VerifySignatures(s, role, db)
 }
 
 func verifyMeta(s *data.Signed, role string, minVersion int) error {

tuf/signed/verify_test.go

@@ -1,16 +1,160 @@
 package signed
 
 import (
+	"errors"
 	"testing"
 	"time"
 
+	"github.com/jfrazelle/go/canonical/json"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/keys"
-	"github.com/jfrazelle/go/canonical/json"
 )
 
+func TestRoleNoKeys(t *testing.T) {
+	cs := NewEd25519()
+	k, err := cs.Create("root", data.ED25519Key)
+	assert.NoError(t, err)
+	r, err := data.NewRole(
+		"root",
+		1,
+		[]string{},
+		nil,
+		nil,
+	)
+	assert.NoError(t, err)
+	db := keys.NewDB()
+	assert.NoError(t, err)
+	err = db.AddRole(r)
+	assert.NoError(t, err)
+	meta := &data.SignedCommon{Type: "Root", Version: 1, Expires: data.DefaultExpires("root")}
+
+	b, err := json.MarshalCanonical(meta)
+	assert.NoError(t, err)
+	s := &data.Signed{Signed: b}
+	Sign(cs, s, k)
+	err = Verify(s, "root", 1, db)
+	assert.IsType(t, ErrRoleThreshold{}, err)
+}
+
+func TestNotEnoughSigs(t *testing.T) {
+	cs := NewEd25519()
+	k, err := cs.Create("root", data.ED25519Key)
+	assert.NoError(t, err)
+	r, err := data.NewRole(
+		"root",
+		2,
+		[]string{k.ID()},
+		nil,
+		nil,
+	)
+	assert.NoError(t, err)
+	db := keys.NewDB()
+	assert.NoError(t, err)
+	db.AddKey(k)
+	err = db.AddRole(r)
+	assert.NoError(t, err)
+	meta := &data.SignedCommon{Type: "Root", Version: 1, Expires: data.DefaultExpires("root")}
+
+	b, err := json.MarshalCanonical(meta)
+	assert.NoError(t, err)
+	s := &data.Signed{Signed: b}
+	Sign(cs, s, k)
+	err = Verify(s, "root", 1, db)
+	assert.IsType(t, ErrRoleThreshold{}, err)
+}
+
+func TestMoreThanEnoughSigs(t *testing.T) {
+	cs := NewEd25519()
+	k1, err := cs.Create("root", data.ED25519Key)
+	assert.NoError(t, err)
+	k2, err := cs.Create("root", data.ED25519Key)
+	assert.NoError(t, err)
+	r, err := data.NewRole(
+		"root",
+		1,
+		[]string{k1.ID(), k2.ID()},
+		nil,
+		nil,
+	)
+	assert.NoError(t, err)
+	db := keys.NewDB()
+	assert.NoError(t, err)
+	db.AddKey(k1)
+	db.AddKey(k2)
+	err = db.AddRole(r)
+	assert.NoError(t, err)
+	meta := &data.SignedCommon{Type: "Root", Version: 1, Expires: data.DefaultExpires("root")}
+
+	b, err := json.MarshalCanonical(meta)
+	assert.NoError(t, err)
+	s := &data.Signed{Signed: b}
+	Sign(cs, s, k1, k2)
+	assert.Equal(t, 2, len(s.Signatures))
+	err = Verify(s, "root", 1, db)
+	assert.NoError(t, err)
+}
+
+func TestDuplicateSigs(t *testing.T) {
+	cs := NewEd25519()
+	k, err := cs.Create("root", data.ED25519Key)
+	assert.NoError(t, err)
+	r, err := data.NewRole(
+		"root",
+		2,
+		[]string{k.ID()},
+		nil,
+		nil,
+	)
+	assert.NoError(t, err)
+	db := keys.NewDB()
+	assert.NoError(t, err)
+	db.AddKey(k)
+	err = db.AddRole(r)
+	assert.NoError(t, err)
+	meta := &data.SignedCommon{Type: "Root", Version: 1, Expires: data.DefaultExpires("root")}
+
+	b, err := json.MarshalCanonical(meta)
+	assert.NoError(t, err)
+	s := &data.Signed{Signed: b}
+	Sign(cs, s, k)
+	s.Signatures = append(s.Signatures, s.Signatures[0])
+	err = Verify(s, "root", 1, db)
+	assert.IsType(t, ErrRoleThreshold{}, err)
+}
+
+func TestUnknownKeyBelowThreshold(t *testing.T) {
+	cs := NewEd25519()
+	k, err := cs.Create("root", data.ED25519Key)
+	assert.NoError(t, err)
+	unknown, err := cs.Create("root", data.ED25519Key)
+	assert.NoError(t, err)
+	r, err := data.NewRole(
+		"root",
+		2,
+		[]string{k.ID()},
+		nil,
+		nil,
+	)
+	assert.NoError(t, err)
+	db := keys.NewDB()
+	assert.NoError(t, err)
+	db.AddKey(k)
+	db.AddKey(unknown)
+	err = db.AddRole(r)
+	assert.NoError(t, err)
+	meta := &data.SignedCommon{Type: "Root", Version: 1, Expires: data.DefaultExpires("root")}
+
+	b, err := json.MarshalCanonical(meta)
+	assert.NoError(t, err)
+	s := &data.Signed{Signed: b}
+	Sign(cs, s, k, unknown)
+	s.Signatures = append(s.Signatures)
+	err = Verify(s, "root", 1, db)
+	assert.IsType(t, ErrRoleThreshold{}, err)
+}
+
 func Test(t *testing.T) {
 	cryptoService := NewEd25519()
 	type test struct {
@@ -37,87 +181,11 @@ func Test(t *testing.T) {
 		{
 			name: "unknown role",
 			role: "foo",
-			err:  ErrUnknownRole,
+			err:  errors.New("tuf: meta file has wrong type"),
-		},
-		//{
-		//	name: "wrong signature method",
-		//	mut:  func(t *test) { t.s.Signatures[0].Method = "foo" },
-		//	err:  ErrWrongMethod,
-		//},
-		//	{
-		//		name: "signature wrong length",
-		//		mut:  func(t *test) { t.s.Signatures[0].Signature = []byte{0} },
-		//		err:  ErrInvalid,
-		//	},
-		{
-			name: "key missing from role",
-			mut:  func(t *test) { t.roles["root"].KeyIDs = nil },
-			err:  ErrRoleThreshold{},
-		},
-		//	{
-		//		name: "invalid signature",
-		//		mut:  func(t *test) { t.s.Signatures[0].Signature = make([]byte, ed25519.SignatureSize) },
-		//		err:  ErrInvalid,
-		//	},
-		{
-			name: "not enough signatures",
-			mut:  func(t *test) { t.roles["root"].Threshold = 2 },
-			err:  ErrRoleThreshold{},
 		},
 		{
 			name: "exactly enough signatures",
 		},
-		{
-			name: "more than enough signatures",
-			mut: func(t *test) {
-				k, _ := cryptoService.Create("root", data.ED25519Key)
-				Sign(cryptoService, t.s, k)
-				t.keys = append(t.keys, k)
-				t.roles["root"].KeyIDs = append(t.roles["root"].KeyIDs, k.ID())
-			},
-		},
-		{
-			name: "duplicate key id",
-			mut: func(t *test) {
-				t.roles["root"].Threshold = 2
-				t.s.Signatures = append(t.s.Signatures, t.s.Signatures[0])
-			},
-			err: ErrRoleThreshold{},
-		},
-		{
-			name: "unknown key",
-			mut: func(t *test) {
-				k, _ := cryptoService.Create("root", data.ED25519Key)
-				Sign(cryptoService, t.s, k)
-			},
-		},
-		{
-			name: "unknown key below threshold",
-			mut: func(t *test) {
-				k, _ := cryptoService.Create("root", data.ED25519Key)
-				Sign(cryptoService, t.s, k)
-				t.roles["root"].Threshold = 2
-			},
-			err: ErrRoleThreshold{},
-		},
-		{
-			name: "unknown keys in db",
-			mut: func(t *test) {
-				k, _ := cryptoService.Create("root", data.ED25519Key)
-				Sign(cryptoService, t.s, k)
-				t.keys = append(t.keys, k)
-			},
-		},
-		{
-			name: "unknown keys in db below threshold",
-			mut: func(t *test) {
-				k, _ := cryptoService.Create("root", data.ED25519Key)
-				Sign(cryptoService, t.s, k)
-				t.keys = append(t.keys, k)
-				t.roles["root"].Threshold = 2
-			},
-			err: ErrRoleThreshold{},
-		},
 		{
 			name: "wrong type",
 			typ:  "bar",
@@ -136,6 +204,7 @@ func Test(t *testing.T) {
 		},
 	}
 	for _, run := range tests {
+		db := keys.NewDB()
 		if run.role == "" {
 			run.role = "root"
 		}
@@ -151,6 +220,16 @@ func Test(t *testing.T) {
 		}
 		if run.keys == nil && run.s == nil {
 			k, _ := cryptoService.Create("root", data.ED25519Key)
+			db.AddKey(k)
+			r, err := data.NewRole(
+				"root",
+				1,
+				[]string{k.ID()},
+				nil,
+				nil,
+			)
+			assert.NoError(t, err)
+			db.AddRole(r)
 			meta := &data.SignedCommon{Type: run.typ, Version: run.ver, Expires: *run.exp}
 
 			b, err := json.MarshalCanonical(meta)
@@ -158,32 +237,11 @@ func Test(t *testing.T) {
 			s := &data.Signed{Signed: b}
 			Sign(cryptoService, s, k)
 			run.s = s
-			run.keys = []data.PublicKey{k}
-		}
-		if run.roles == nil {
-			run.roles = map[string]*data.Role{
-				"root": {
-					RootRole: data.RootRole{
-						KeyIDs:    []string{run.keys[0].ID()},
-						Threshold: 1,
-					},
-					Name: "root",
-				},
-			}
 		}
 		if run.mut != nil {
 			run.mut(&run)
 		}
 
-		db := keys.NewDB()
-		for _, k := range run.keys {
-			db.AddKey(k)
-		}
-		for _, r := range run.roles {
-			err := db.AddRole(r)
-			assert.NoError(t, err)
-		}
-
 		err := Verify(run.s, run.role, minVer, db)
 		if e, ok := run.err.(ErrExpired); ok {
 			assertErrExpired(t, err, e)