addressing review comments

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

cryptoservice/crypto_service.go

@@ -3,7 +3,6 @@ package cryptoservice
 import (
 	"crypto/rand"
 	"fmt"
-	"path/filepath"
 
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/notary/trustmanager"
@@ -113,18 +112,22 @@ func (cs *CryptoService) RemoveKey(keyID string) (err error) {
 // AddKey adds a private key to a specified role.
 // The GUN is inferred from the cryptoservice itself for non-root roles
 func (cs *CryptoService) AddKey(key data.PrivateKey, role string) (err error) {
-	keyID := key.ID()
+	// First check if this key already exists in any of our keystores
-	if role != data.CanonicalRootRole {
-		keyID = filepath.Join(cs.gun, key.ID())
-	}
 	for _, ks := range cs.keyStores {
-		if keyInfo, err := ks.GetKeyInfo(keyID); err == nil {
+		if keyInfo, err := ks.GetKeyInfo(key.ID()); err == nil {
 			if keyInfo.Role != role {
 				return fmt.Errorf("key with same ID already exists for role: %s", keyInfo.Role)
 			}
-			continue
+			logrus.Debugf("key with same ID %s and role %s already exists", key.ID(), keyInfo.Role)
+			return nil
+		}
+	}
+	// If the key didn't exist in any of our keystores, add and return on the first successful keystore
+	for _, ks := range cs.keyStores {
+		// Try to add to this keystore, return if successful
+		if err = ks.AddKey(key, trustmanager.KeyInfo{Role: role, Gun: cs.gun}); err == nil {
+			return nil
 		}
-		ks.AddKey(key, trustmanager.KeyInfo{Role: role, Gun: cs.gun})
 	}
 	return // returns whatever the final values were
 }

cryptoservice/crypto_service_test.go

@@ -300,6 +300,53 @@ func (c CryptoServiceTester) TestListFromMultipleKeystores(t *testing.T) {
 	}
 }
 
+// asserts that adding a key adds to all keystores
+// and adding an existing key either succeeds if the role matches or fails if it does not
+func (c CryptoServiceTester) TestAddKey(t *testing.T) {
+	cryptoService := c.cryptoServiceFactory()
+	cryptoService.keyStores = append(cryptoService.keyStores,
+		trustmanager.NewKeyMemoryStore(passphraseRetriever))
+
+	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
+	assert.NoError(t, err)
+
+	// Add the key to the targets role
+	assert.NoError(t, cryptoService.AddKey(privKey, data.CanonicalTargetsRole))
+
+	// Check that we added the key and its info to only the first keystore
+	retrievedKey, retrievedRole, err := cryptoService.keyStores[0].GetKey(privKey.ID())
+	assert.NoError(t, err)
+	assert.Equal(t, privKey.Private(), retrievedKey.Private())
+	assert.Equal(t, data.CanonicalTargetsRole, retrievedRole)
+
+	retrievedKeyInfo, err := cryptoService.keyStores[0].GetKeyInfo(privKey.ID())
+	assert.NoError(t, err)
+	assert.Equal(t, data.CanonicalTargetsRole, retrievedKeyInfo.Role)
+	assert.Equal(t, cryptoService.gun, retrievedKeyInfo.Gun)
+
+	// The key should not exist in the second keystore
+	_, _, err = cryptoService.keyStores[1].GetKey(privKey.ID())
+	assert.Error(t, err)
+	_, err = cryptoService.keyStores[1].GetKeyInfo(privKey.ID())
+	assert.Error(t, err)
+
+	// We should be able to successfully get the key from the cryptoservice level
+	retrievedKey, retrievedRole, err = cryptoService.GetPrivateKey(privKey.ID())
+	assert.NoError(t, err)
+	assert.Equal(t, privKey.Private(), retrievedKey.Private())
+	assert.Equal(t, data.CanonicalTargetsRole, retrievedRole)
+	retrievedKeyInfo, err = cryptoService.GetKeyInfo(privKey.ID())
+	assert.NoError(t, err)
+	assert.Equal(t, data.CanonicalTargetsRole, retrievedKeyInfo.Role)
+	assert.Equal(t, cryptoService.gun, retrievedKeyInfo.Gun)
+
+	// Add the same key to the targets role, since the info is the same we should have no error
+	assert.NoError(t, cryptoService.AddKey(privKey, data.CanonicalTargetsRole))
+
+	// Try to add the same key to the snapshot role, which should error due to the role mismatch
+	assert.Error(t, cryptoService.AddKey(privKey, data.CanonicalSnapshotRole))
+}
+
 // 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
@@ -330,6 +377,7 @@ func testCryptoService(t *testing.T, gun string) {
 				keyAlgo: algo,
 				gun:     gun,
 			}
+			cst.TestAddKey(t)
 			cst.TestCreateAndGetKey(t)
 			cst.TestCreateAndGetWhenMultipleKeystores(t)
 			cst.TestGetNonexistentKey(t)

cryptoservice/import_export_test.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/docker/notary"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 	"github.com/stretchr/testify/assert"
@@ -80,13 +81,13 @@ func TestImportExportZip(t *testing.T) {
 		if alias == data.CanonicalRootRole {
 			continue
 		}
-		relKeyPath := filepath.Join("tuf_keys", privKeyName+".key")
+		relKeyPath := filepath.Join(notary.NonRootKeysSubdir, privKeyName+".key")
 		passphraseByFile[relKeyPath] = exportPassphrase
 	}
 
 	// Add root key to the map. This will use the export passphrase because it
 	// will be reencrypted.
-	relRootKey := filepath.Join("root_keys", rootKeyID+".key")
+	relRootKey := filepath.Join(notary.RootKeysSubdir, rootKeyID+".key")
 	passphraseByFile[relRootKey] = exportPassphrase
 
 	// Iterate through the files in the archive, checking that the files
@@ -141,8 +142,8 @@ func TestImportExportZip(t *testing.T) {
 		if alias == data.CanonicalRootRole {
 			continue
 		}
-		relKeyPath := filepath.Join("tuf_keys", privKeyName+".key")
+		relKeyPath := filepath.Join(notary.NonRootKeysSubdir, privKeyName+".key")
-		privKeyFileName := filepath.Join(tempBaseDir2, "private", relKeyPath)
+		privKeyFileName := filepath.Join(tempBaseDir2, notary.PrivDir, relKeyPath)
 		_, err = os.Stat(privKeyFileName)
 		assert.NoError(t, err, "missing private key for role %s: %s", alias, privKeyName)
 	}
@@ -151,7 +152,7 @@ func TestImportExportZip(t *testing.T) {
 	// There should be a file named after the key ID of the root key we
 	// passed in.
 	rootKeyFilename := rootKeyID + ".key"
-	_, err = os.Stat(filepath.Join(tempBaseDir2, "private", "root_keys", rootKeyFilename))
+	_, err = os.Stat(filepath.Join(tempBaseDir2, notary.PrivDir, notary.RootKeysSubdir, rootKeyFilename))
 	assert.NoError(t, err, "missing root key")
 }
 
@@ -199,7 +200,7 @@ func TestImportExportGUN(t *testing.T) {
 		if alias == data.CanonicalRootRole {
 			continue
 		}
-		relKeyPath := filepath.Join("tuf_keys", gun, privKeyName+".key")
+		relKeyPath := filepath.Join(notary.NonRootKeysSubdir, gun, privKeyName+".key")
 
 		passphraseByFile[relKeyPath] = exportPassphrase
 	}
@@ -258,8 +259,8 @@ func TestImportExportGUN(t *testing.T) {
 		if alias == data.CanonicalRootRole {
 			continue
 		}
-		relKeyPath := filepath.Join("tuf_keys", gun, privKeyName+".key")
+		relKeyPath := filepath.Join(notary.NonRootKeysSubdir, gun, privKeyName+".key")
-		privKeyFileName := filepath.Join(tempBaseDir2, "private", relKeyPath)
+		privKeyFileName := filepath.Join(tempBaseDir2, notary.PrivDir, relKeyPath)
 		_, err = os.Stat(privKeyFileName)
 		assert.NoError(t, err)
 	}
@@ -307,7 +308,7 @@ func TestImportExportRootKey(t *testing.T) {
 	// There should be a file named after the key ID of the root key we
 	// imported.
 	rootKeyFilename := rootKeyID + ".key"
-	_, err = os.Stat(filepath.Join(tempBaseDir2, "private", "root_keys", rootKeyFilename))
+	_, err = os.Stat(filepath.Join(tempBaseDir2, notary.PrivDir, notary.RootKeysSubdir, rootKeyFilename))
 	assert.NoError(t, err, "missing root key")
 
 	// Try to import a decrypted version of the root key and make sure it
@@ -375,7 +376,7 @@ func TestImportExportRootKeyReencrypt(t *testing.T) {
 	// There should be a file named after the key ID of the root key we
 	// imported.
 	rootKeyFilename := rootKeyID + ".key"
-	_, err = os.Stat(filepath.Join(tempBaseDir2, "private", "root_keys", rootKeyFilename))
+	_, err = os.Stat(filepath.Join(tempBaseDir2, notary.PrivDir, notary.RootKeysSubdir, rootKeyFilename))
 	assert.NoError(t, err, "missing root key")
 
 	// Should be able to unlock the root key with the new password
@@ -430,7 +431,7 @@ func TestImportExportNonRootKey(t *testing.T) {
 	// There should be a file named after the key ID of the targets key we
 	// imported.
 	targetsKeyFilename := targetsKeyID + ".key"
-	_, err = os.Stat(filepath.Join(tempBaseDir2, "private", "tuf_keys", "docker.com/notary", targetsKeyFilename))
+	_, err = os.Stat(filepath.Join(tempBaseDir2, notary.PrivDir, notary.NonRootKeysSubdir, "docker.com/notary", targetsKeyFilename))
 	assert.NoError(t, err, "missing targets key")
 
 	// Check that the key is the same
@@ -485,7 +486,7 @@ func TestImportExportNonRootKeyReencrypt(t *testing.T) {
 	// There should be a file named after the key ID of the snapshot key we
 	// imported.
 	snapshotKeyFilename := snapshotKeyID + ".key"
-	_, err = os.Stat(filepath.Join(tempBaseDir2, "private", "tuf_keys", "docker.com/notary", snapshotKeyFilename))
+	_, err = os.Stat(filepath.Join(tempBaseDir2, notary.PrivDir, notary.NonRootKeysSubdir, "docker.com/notary", snapshotKeyFilename))
 	assert.NoError(t, err, "missing snapshot key")
 
 	// Should be able to unlock the root key with the new password

trustmanager/keyfilestore.go

@@ -65,45 +65,13 @@ func NewKeyFileStore(baseDir string, passphraseRetriever passphrase.Retriever) (
 func generateKeyInfoMap(s LimitedFileStore) map[string]KeyInfo {
 	keyInfoMap := make(map[string]KeyInfo)
 	for _, keyPath := range s.ListFiles() {
-		// Remove the prefix of the directory from the filename for GUN/role/ID parsing
-		var keyIDAndGun, keyRole string
-		if strings.HasPrefix(keyPath, notary.RootKeysSubdir+"/") {
-			keyIDAndGun = strings.TrimPrefix(keyPath, notary.RootKeysSubdir+"/")
-		} else {
-			keyIDAndGun = strings.TrimPrefix(keyPath, notary.NonRootKeysSubdir+"/")
-		}
-
-		// Separate the ID and GUN (can be empty) from the filepath
-		keyIDAndGun = strings.TrimSpace(keyIDAndGun)
-		keyGun := getGunFromFullID(keyIDAndGun)
-		keyID := filepath.Base(keyIDAndGun)
-
-		// If the key does not have a _, we'll attempt to
-		// read the role from PEM headers
-		underscoreIndex := strings.LastIndex(keyID, "_")
-		if underscoreIndex == -1 {
 		d, err := s.Get(keyPath)
 		if err != nil {
 			logrus.Error(err)
 			continue
 		}
-			block, _ := pem.Decode(d)
+		keyID, keyInfo := getImportedKeyInfo(d, keyPath)
-			if block == nil {
+		keyInfoMap[keyID] = keyInfo
-				continue
-			}
-			if role, ok := block.Headers["role"]; ok {
-				keyRole = role
-			}
-		} else {
-			// This is the legacy KEYID_ROLE filename
-			// The keyID is the first part of the keyname
-			// The keyRole is the second part of the keyname
-			// in a key named abcde_root, abcde is the keyID and root is the KeyAlias
-			legacyID := keyID
-			keyID = legacyID[:underscoreIndex]
-			keyRole = legacyID[underscoreIndex+1:]
-		}
-		keyInfoMap[keyID] = KeyInfo{Gun: keyGun, Role: keyRole}
 	}
 	return keyInfoMap
 }
@@ -173,9 +141,9 @@ func (s *KeyFileStore) GetKey(name string) (data.PrivateKey, string, error) {
 	return getKey(s, s.Retriever, s.cachedKeys, name)
 }
 
-// ListKeys returns a list of unique PublicKeys present on the KeyFileStore.
+// ListKeys returns a list of unique PublicKeys present on the KeyFileStore, by returning a copy of the keyInfoMap
 func (s *KeyFileStore) ListKeys() map[string]KeyInfo {
-	return s.keyInfoMap
+	return copyKeyInfoMap(s.keyInfoMap)
 }
 
 // RemoveKey removes the key from the keyfilestore
@@ -215,7 +183,6 @@ func (s *KeyFileStore) ExportKey(name string) ([]byte, error) {
 
 // ImportKey imports the private key in the encrypted bytes into the keystore
 // with the given key ID and alias.
-// This is only used for root, so no need to touch the keyInfoMap
 func (s *KeyFileStore) ImportKey(pemBytes []byte, alias string) error {
 	err := importKey(s, s.Retriever, s.cachedKeys, alias, pemBytes)
 	if err != nil {
@@ -276,9 +243,18 @@ func (s *KeyMemoryStore) GetKey(name string) (data.PrivateKey, string, error) {
 	return getKey(s, s.Retriever, s.cachedKeys, name)
 }
 
-// ListKeys returns a list of unique PublicKeys present on the KeyFileStore.
+// ListKeys returns a list of unique PublicKeys present on the KeyFileStore, by returning a copy of the keyInfoMap
 func (s *KeyMemoryStore) ListKeys() map[string]KeyInfo {
-	return s.keyInfoMap
+	return copyKeyInfoMap(s.keyInfoMap)
+}
+
+// copyKeyInfoMap returns a deep copy of the passed-in keyInfoMap
+func copyKeyInfoMap(keyInfoMap map[string]KeyInfo) map[string]KeyInfo {
+	copyMap := make(map[string]KeyInfo)
+	for keyID, keyInfo := range keyInfoMap {
+		copyMap[keyID] = KeyInfo{Role: keyInfo.Role, Gun: keyInfo.Gun}
+	}
+	return copyMap
 }
 
 // RemoveKey removes the key from the keystore
@@ -328,21 +304,23 @@ func (s *KeyMemoryStore) ImportKey(pemBytes []byte, alias string) error {
 func getImportedKeyInfo(pemBytes []byte, filename string) (string, KeyInfo) {
 	var keyID, gun, role string
 	// Try to read the role and gun from the filepath and PEM headers
-	if filepath.HasPrefix(filename, "root_keys") {
+	if filepath.HasPrefix(filename, notary.RootKeysSubdir+"/") {
 		role = data.CanonicalRootRole
 		gun = ""
-		filename = strings.TrimPrefix(filename, "root_keys")
+		filename = strings.TrimPrefix(filename, notary.RootKeysSubdir+"/")
 	} else {
-		filename = strings.TrimPrefix(filename, "tuf_keys")
+		filename = strings.TrimPrefix(filename, notary.NonRootKeysSubdir+"/")
 		gun = getGunFromFullID(filename)
 	}
 	keyIDFull := filepath.Base(filename)
 	underscoreIndex := strings.LastIndex(keyIDFull, "_")
 	if underscoreIndex == -1 {
 		block, _ := pem.Decode(pemBytes)
+		if block != nil {
 			if keyRole, ok := block.Headers["role"]; ok {
 				role = keyRole
 			}
+		}
 		keyID = filepath.Base(keyIDFull)
 	} else {
 		// This is the legacy KEYID_ROLE filename
@@ -458,7 +436,7 @@ func removeKey(s LimitedFileStore, cachedKeys map[string]*cachedKey, name string
 
 // Assumes 2 subdirectories, 1 containing root keys and 1 containing tuf keys
 func getSubdir(alias string) string {
-	if alias == "root" {
+	if alias == data.CanonicalRootRole {
 		return notary.RootKeysSubdir
 	}
 	return notary.NonRootKeysSubdir

trustmanager/keyfilestore_test.go

@@ -169,11 +169,9 @@ func TestGet(t *testing.T) {
 
 	// Root role needs to go in the rootKeySubdir to be read.
 	// All other roles can go in the nonRootKeysSubdir, possibly under a GUN
-	rootKeysSubdirWithGUN := filepath.Clean(filepath.Join(notary.RootKeysSubdir, gun))
 	nonRootKeysSubdirWithGUN := filepath.Clean(filepath.Join(notary.NonRootKeysSubdir, gun))
 
 	testGetKeyWithRole(t, "", data.CanonicalRootRole, notary.RootKeysSubdir, true)
-	testGetKeyWithRole(t, gun, data.CanonicalRootRole, rootKeysSubdirWithGUN, true)
 	for _, role := range nonRootRolesToTest {
 		testGetKeyWithRole(t, "", role, notary.NonRootKeysSubdir, true)
 		testGetKeyWithRole(t, gun, role, nonRootKeysSubdirWithGUN, true)
@@ -185,7 +183,6 @@ func TestGet(t *testing.T) {
 	testGetKeyWithRole(t, gun, data.CanonicalRootRole, nonRootKeysSubdirWithGUN, false)
 	for _, role := range nonRootRolesToTest {
 		testGetKeyWithRole(t, "", role, notary.RootKeysSubdir, false)
-		testGetKeyWithRole(t, gun, role, rootKeysSubdirWithGUN, false)
 	}
 }
 
@@ -367,6 +364,14 @@ func TestListKeys(t *testing.T) {
 	// Check to see if the keystore still lists two keys
 	keyMap := store.ListKeys()
 	assert.Len(t, keyMap, len(roles))
+
+	// Check that ListKeys() returns a copy of the state
+	// so modifying its returned information does not change the underlying store's keyInfo
+	for keyID := range keyMap {
+		delete(keyMap, keyID)
+		_, err = store.GetKeyInfo(keyID)
+		assert.NoError(t, err)
+	}
 }
 
 func TestAddGetKeyMemStore(t *testing.T) {