Fixed cryptoservice.Create to call keyStore.AddKey with a GUN only if

it is not a root role.

Updated the cryptoservice tests to test all key algorithms, all roles,
and cryptoservices without a GUN.  This then also found bugs in
cryptoservice.GetKey, cryptoservice.RemoveKey, and
cryptoservice.GetPrivateKey, which weren't really being exercised
previously.

Signed-off-by: Ying Li <ying.li@docker.com>
Signed-off-by: David Lawrence <david.lawrence@docker.com>

Signed-off-by: Ying Li <ying.li@docker.com> (github: endophage)

cryptoservice/certificate_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestUnlockedSigner(t *testing.T) {
+func TestGenerateCertificate(t *testing.T) {
 	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
 	assert.NoError(t, err, "could not generate key")
 

cryptoservice/crypto_service.go

@@ -53,8 +53,15 @@ func (ccs *CryptoService) Create(role, algorithm string) (data.PublicKey, error)
 	logrus.Debugf("generated new %s key for role: %s and keyID: %s", algorithm, role, privKey.ID())
 
 	// Store the private key into our keystore with the name being: /GUN/ID.key with an alias of role
+	var keyPath string
+	if role == data.CanonicalRootRole {
+		keyPath = privKey.ID()
+	} else {
+		keyPath = filepath.Join(ccs.gun, privKey.ID())
+	}
+
 	for _, ks := range ccs.keyStores {
-		err = ks.AddKey(filepath.Join(ccs.gun, privKey.ID()), role, privKey)
+		err = ks.AddKey(keyPath, role, privKey)
 		if err == nil {
 			return data.PublicKeyFromPrivate(privKey), nil
 		}
@@ -66,40 +73,47 @@ func (ccs *CryptoService) Create(role, algorithm string) (data.PublicKey, error)
 
 }
 
-// GetPrivateKey returns a private key by ID
+// GetPrivateKey returns a private key by ID. It tries to get the key first
+// without a GUN (in which case it's a root key).  If that fails, try to get
+// the key with the GUN (non-root key).
+// If that fails, then we don't have the key.
 func (ccs *CryptoService) GetPrivateKey(keyID string) (k data.PrivateKey, id string, err error) {
+	keyPaths := []string{keyID, filepath.Join(ccs.gun, keyID)}
 	for _, ks := range ccs.keyStores {
-		k, id, err = ks.GetKey(keyID)
+		for _, keyPath := range keyPaths {
-		if k == nil || err != nil {
+			k, id, err = ks.GetKey(keyPath)
+			if err != nil {
 				continue
 			}
 			return
 		}
+	}
 	return // returns whatever the final values were
 }
 
 // GetKey returns a key by ID
 func (ccs *CryptoService) GetKey(keyID string) data.PublicKey {
-	for _, ks := range ccs.keyStores {
+	privKey, _, err := ccs.GetPrivateKey(keyID)
-		k, _, err := ks.GetKey(keyID)
+	if err != nil {
-		if k == nil || err != nil {
+		return nil
-			continue
 	}
-		return data.PublicKeyFromPrivate(k)
+	return data.PublicKeyFromPrivate(privKey)
-
-	}
-	return nil // returns whatever the final values were
 }
 
 // RemoveKey deletes a key by ID
 func (ccs *CryptoService) RemoveKey(keyID string) (err error) {
+	keyPaths := []string{keyID, filepath.Join(ccs.gun, keyID)}
 	for _, ks := range ccs.keyStores {
-		e := ks.RemoveKey(keyID)
+		for _, keyPath := range keyPaths {
-		if e != nil {
+			_, _, err = ks.GetKey(keyPath)
-			err = e
+			if err != nil {
+				continue
+			}
+			err = ks.RemoveKey(keyPath)
+			return
 		}
 	}
-	return // returns last error if any
+	return // returns whatever the final values were
 }
 
 // Sign returns the signatures for the payload with a set of keyIDs. It ignores
@@ -110,25 +124,18 @@ func (ccs *CryptoService) Sign(keyIDs []string, payload []byte) ([]data.Signatur
 	for _, keyid := range keyIDs {
 		keyName := keyid
 
-		// Try to get the key first without a GUN (in which case it's a root
-		// key).  If that fails, try to get the key with the GUN (non-root
-		// key).  If that fails, then we don't have the key.
 		privKey, _, err := ccs.GetPrivateKey(keyName)
 		if err != nil {
 			logrus.Debugf("error attempting to retrieve private key: %s, %v", keyid, err)
-			keyName = filepath.Join(ccs.gun, keyid)
+			continue
-			privKey, _, err = ccs.GetPrivateKey(keyName)
-			if err != nil {
-				logrus.Debugf("error attempting to retrieve key ID: %s, %v", keyid, err)
-				return nil, err
-			}
 		}
 
 		sigAlgo := privKey.SignatureAlgorithm()
 		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)
+			logrus.Debugf("ignoring error attempting to %s sign with keyID: %s, %v",
-			return nil, err
+				privKey.Algorithm(), keyid, err)
+			continue
 		}
 
 		logrus.Debugf("appending %s signature with Key ID: %s", privKey.Algorithm(), keyid)

cryptoservice/crypto_service_test.go

@@ -1,51 +1,147 @@
 package cryptoservice
 
 import (
-	"github.com/stretchr/testify/assert"
+	"fmt"
+	"runtime"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/signed"
 )
 
-func TestCryptoService(t *testing.T) {
+var algoToSigType = map[string]data.SigAlgorithm{
-	testCryptoService(t, data.ECDSAKey, signed.ECDSAVerifier{})
+	data.ECDSAKey:   data.ECDSASignature,
-	testCryptoService(t, data.ED25519Key, signed.Ed25519Verifier{})
+	data.ED25519Key: data.EDDSASignature,
-	if !testing.Short() {
+	data.RSAKey:     data.RSAPSSSignature,
-		testCryptoService(t, data.RSAKey, signed.RSAPSSVerifier{})
-	}
 }
 
 var passphraseRetriever = func(string, string, bool, int) (string, bool, error) { return "", false, nil }
 
-func testCryptoService(t *testing.T, keyAlgo string, verifier signed.Verifier) {
+type CryptoServiceTester struct {
-	content := []byte("this is a secret")
+	cryptoServiceFactory func() *CryptoService
+	role                 string
+	keyAlgo              string
+}
 
-	keyStore := trustmanager.NewKeyMemoryStore(passphraseRetriever)
+// asserts that created key exists
-	cryptoService := NewCryptoService("", keyStore)
+func (c CryptoServiceTester) TestCreateAndGetKey(t *testing.T) {
+	cryptoService := c.cryptoServiceFactory()
 
 	// Test Create
-	tufKey, err := cryptoService.Create("", keyAlgo)
+	tufKey, err := cryptoService.Create(c.role, c.keyAlgo)
-	assert.NoError(t, err, "error creating key")
+	assert.NoError(t, err, c.errorMsg("error creating key"))
-
-	// Test Sign
-	signatures, err := cryptoService.Sign([]string{tufKey.ID()}, content)
-	assert.NoError(t, err, "signing failed")
-	assert.Len(t, signatures, 1, "wrong number of signatures")
-
-	err = verifier.Verify(tufKey, signatures[0].Signature, content)
-	assert.NoError(t, err, "verification failed for %s key type", keyAlgo)
 
 	// Test GetKey
 	retrievedKey := cryptoService.GetKey(tufKey.ID())
-	assert.Equal(t, tufKey.Public(), retrievedKey.Public(), "retrieved key didn't match")
+	assert.NotNil(t, retrievedKey,
+		c.errorMsg("Could not find key ID %s", tufKey.ID()))
+	assert.Equal(t, tufKey.Public(), retrievedKey.Public(),
+		c.errorMsg("retrieved public key didn't match"))
 
-	assert.Nil(t, cryptoService.GetKey("boguskeyid"), "non-nil result for bogus keyid")
+	// Test GetPrivateKey
+	retrievedKey, alias, err := cryptoService.GetPrivateKey(tufKey.ID())
+	assert.NoError(t, err)
+	assert.Equal(t, tufKey.ID(), retrievedKey.ID(),
+		c.errorMsg("retrieved private key didn't have the right ID"))
+	assert.Equal(t, c.role, alias)
+}
+
+// asserts that getting key fails for a non-existent key
+func (c CryptoServiceTester) TestGetNonexistentKey(t *testing.T) {
+	cryptoService := c.cryptoServiceFactory()
+
+	assert.Nil(t, cryptoService.GetKey("boguskeyid"),
+		c.errorMsg("non-nil result for bogus keyid"))
+
+	_, _, err := cryptoService.GetPrivateKey("boguskeyid")
+	assert.NotNil(t, err)
+}
+
+// asserts that signing with a created key creates a valid signature
+func (c CryptoServiceTester) TestSignWithKey(t *testing.T) {
+	cryptoService := c.cryptoServiceFactory()
+	content := []byte("this is a secret")
+
+	tufKey, err := cryptoService.Create(c.role, c.keyAlgo)
+	assert.NoError(t, err, c.errorMsg("error creating key"))
+
+	// Test Sign
+	signatures, err := cryptoService.Sign([]string{tufKey.ID()}, content)
+	assert.NoError(t, err, c.errorMsg("signing failed"))
+	assert.Len(t, signatures, 1, c.errorMsg("wrong number of signatures"))
+
+	verifier, ok := signed.Verifiers[algoToSigType[c.keyAlgo]]
+	assert.True(t, ok, c.errorMsg("Unknown verifier for algorithm"))
+
+	err = verifier.Verify(tufKey, signatures[0].Signature, content)
+	assert.NoError(t, err,
+		c.errorMsg("verification failed for %s key type", c.keyAlgo))
+}
+
+// asserts that removing key that exists succeeds
+func (c CryptoServiceTester) TestRemoveCreatedKey(t *testing.T) {
+	cryptoService := c.cryptoServiceFactory()
+
+	tufKey, err := cryptoService.Create(c.role, c.keyAlgo)
+	assert.NoError(t, err, c.errorMsg("error creating key"))
+	assert.NotNil(t, cryptoService.GetKey(tufKey.ID()))
 
 	// Test RemoveKey
 	err = cryptoService.RemoveKey(tufKey.ID())
-	assert.NoError(t, err, "could not remove key")
+	assert.NoError(t, err, c.errorMsg("could not remove key"))
-	retrievedKey = cryptoService.GetKey(tufKey.ID())
+	retrievedKey := cryptoService.GetKey(tufKey.ID())
-	assert.Nil(t, retrievedKey, "remove didn't work")
+	assert.Nil(t, retrievedKey, c.errorMsg("remove didn't work"))
+}
+
+// Prints out an error message with information about the key algorithm,
+// role, and test name. Ideally we could generate different tests given
+// data, without having to put for loops in one giant test function, but
+// that involves a lot of boilerplate.  So as a compromise, everything will
+// still be run in for loops in one giant test function, but we can at
+// least provide an error message stating what data/helper test function
+// failed.
+func (c CryptoServiceTester) errorMsg(message string, args ...interface{}) string {
+	pc := make([]uintptr, 10) // at least 1 entry needed
+	runtime.Callers(2, pc)    // the caller of errorMsg
+	f := runtime.FuncForPC(pc[0])
+	return fmt.Sprintf("%s (role: %s, keyAlgo: %s): %s", f.Name(), c.role,
+		c.keyAlgo, fmt.Sprintf(message, args...))
+}
+
+func testCryptoService(t *testing.T, gun string) {
+	getCryptoService := func() *CryptoService {
+		return NewCryptoService(
+			gun, trustmanager.NewKeyMemoryStore(passphraseRetriever))
+	}
+	roles := []string{
+		data.CanonicalRootRole,
+		data.CanonicalTargetsRole,
+		data.CanonicalSnapshotRole,
+		data.CanonicalTimestampRole,
+	}
+
+	for _, role := range roles {
+		for algo := range algoToSigType {
+			cst := CryptoServiceTester{
+				cryptoServiceFactory: getCryptoService,
+				role:                 role,
+				keyAlgo:              algo,
+			}
+			cst.TestCreateAndGetKey(t)
+			cst.TestGetNonexistentKey(t)
+			cst.TestSignWithKey(t)
+			cst.TestRemoveCreatedKey(t)
+		}
+	}
+}
+
+func TestCryptoServiceWithNonEmptyGUN(t *testing.T) {
+	testCryptoService(t, "org/repo")
+}
+
+func TestCryptoServiceWithEmptyGUN(t *testing.T) {
+	testCryptoService(t, "")
 }