From c7bccd79e38e76a6000296a437027036a048a333 Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Wed, 17 Feb 2016 10:23:45 -0800 Subject: [PATCH] addressing review comments Signed-off-by: Riyaz Faizullabhoy --- cryptoservice/crypto_service.go | 19 ++++--- cryptoservice/crypto_service_test.go | 48 ++++++++++++++++++ cryptoservice/import_export_test.go | 25 ++++----- trustmanager/keyfilestore.go | 76 ++++++++++------------------ trustmanager/keyfilestore_test.go | 11 ++-- 5 files changed, 107 insertions(+), 72 deletions(-) diff --git a/cryptoservice/crypto_service.go b/cryptoservice/crypto_service.go index 0cc7d6c621..e95ba5b15c 100644 --- a/cryptoservice/crypto_service.go +++ b/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() - if role != data.CanonicalRootRole { - keyID = filepath.Join(cs.gun, key.ID()) - } + // First check if this key already exists in any of our keystores 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 } diff --git a/cryptoservice/crypto_service_test.go b/cryptoservice/crypto_service_test.go index dbeb3a4e5b..e4ef5f18e7 100644 --- a/cryptoservice/crypto_service_test.go +++ b/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) diff --git a/cryptoservice/import_export_test.go b/cryptoservice/import_export_test.go index 9497665f89..43412f76db 100644 --- a/cryptoservice/import_export_test.go +++ b/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") - privKeyFileName := filepath.Join(tempBaseDir2, "private", relKeyPath) + relKeyPath := filepath.Join(notary.NonRootKeysSubdir, privKeyName+".key") + 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") - privKeyFileName := filepath.Join(tempBaseDir2, "private", relKeyPath) + relKeyPath := filepath.Join(notary.NonRootKeysSubdir, gun, privKeyName+".key") + 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 diff --git a/trustmanager/keyfilestore.go b/trustmanager/keyfilestore.go index d05df9205c..344b4273c0 100644 --- a/trustmanager/keyfilestore.go +++ b/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+"/") + d, err := s.Get(keyPath) + if err != nil { + logrus.Error(err) + continue } - - // 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) - if block == nil { - 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} + keyID, keyInfo := getImportedKeyInfo(d, keyPath) + keyInfoMap[keyID] = keyInfo } 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,20 +304,22 @@ 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 keyRole, ok := block.Headers["role"]; ok { - role = keyRole + if block != nil { + if keyRole, ok := block.Headers["role"]; ok { + role = keyRole + } } keyID = filepath.Base(keyIDFull) } else { @@ -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 diff --git a/trustmanager/keyfilestore_test.go b/trustmanager/keyfilestore_test.go index cabebcf043..4fdb8e3a06 100644 --- a/trustmanager/keyfilestore_test.go +++ b/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) {