import refactor

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

cmd/notary/integration_test.go

@@ -803,7 +803,7 @@ func assertNumKeys(t *testing.T, tempDir string, numRoot, numSigning int,
 		_, err := os.Stat(filepath.Join(
 			tempDir, "private", "root_keys", rootKeyID+".key"))
 		// os.IsExist checks to see if the error is because a file already
-		// exist, and hence doesn't actually the right funciton to use here
+		// exists, and hence it isn't actually the right function to use here
 		assert.Equal(t, rootOnDisk, !os.IsNotExist(err))
 
 		// this function is declared is in the build-tagged setup files
@@ -945,7 +945,7 @@ func TestClientKeyBackupAndRestore(t *testing.T) {
 	assert.NoError(t, err)
 	// all keys should be there, including root because the root key was backed up to disk,
 	// and export just backs up all the keys on disk
-	assertNumKeys(t, dirs[1], 1, 4, true)
+	assertNumKeys(t, dirs[1], 1, 4, !rootOnHardware())
 
 	// can list and publish to both repos using restored keys
 	for _, gun := range []string{"gun1", "gun2"} {

cryptoservice/import_export.go

@@ -11,7 +11,6 @@ import (
 	"path/filepath"
 	"strings"
 
-	"github.com/docker/notary"
 	"github.com/docker/notary/passphrase"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
@@ -102,49 +101,22 @@ func (cs *CryptoService) ExportKeyReencrypt(dest io.Writer, keyID string, newPas
 	return nil
 }
 
-// ImportRootKey imports a root in PEM format key from an io.Reader
-// It prompts for the key's passphrase to verify the data and to determine
-// the key ID.
-func (cs *CryptoService) ImportRootKey(source io.Reader) error {
-	pemBytes, err := ioutil.ReadAll(source)
-	if err != nil {
-		return err
-	}
-	return cs.ImportRoleKey(pemBytes, data.CanonicalRootRole, nil)
-}
-
 // ImportRoleKey imports a private key in PEM format key from a byte array
 // It prompts for the key's passphrase to verify the data and to determine
 // the key ID.
 func (cs *CryptoService) ImportRoleKey(pemBytes []byte, role string, newPassphraseRetriever passphrase.Retriever) error {
-	var alias string
 	var err error
+	keyGun := cs.gun
 	if role == data.CanonicalRootRole {
-		alias = role
+		keyGun = ""
 		if err = checkRootKeyIsEncrypted(pemBytes); err != nil {
 			return err
 		}
-	} else {
-		// Parse the private key to get the key ID so that we can import it to the correct location
-		privKey, err := trustmanager.ParsePEMPrivateKey(pemBytes, "")
-		if err != nil {
-			privKey, _, err = trustmanager.GetPasswdDecryptBytes(newPassphraseRetriever, pemBytes, role, string(role))
-			if err != nil {
-				return err
-			}
-		}
-		// Since we're importing a non-root role, we need to pass the path as an alias
-		alias = filepath.Join(notary.NonRootKeysSubdir, cs.gun, privKey.ID())
-		// We also need to ensure that the role is properly set in the PEM headers
-		pemBytes, err = trustmanager.KeyToPEM(privKey, role)
-		if err != nil {
-			return err
-		}
 	}
 
 	for _, ks := range cs.keyStores {
 		// don't redeclare err, we want the value carried out of the loop
-		if err = ks.ImportKey(pemBytes, alias); err == nil {
+		if err = ks.ImportKey(pemBytes, role, keyGun); err == nil {
 			return nil //bail on the first keystore we import to
 		}
 	}
@@ -215,11 +187,12 @@ func (cs *CryptoService) ImportKeysZip(zipReader zip.Reader) error {
 	}
 
 	for keyName, pemBytes := range newKeys {
+		_, keyInfo := trustmanager.KeyInfoFromPEM(pemBytes, keyName)
 		// try to import the key to all key stores. As long as one of them
 		// succeeds, consider it a success
 		var tmpErr error
 		for _, ks := range cs.keyStores {
-			if err := ks.ImportKey(pemBytes, keyName); err != nil {
+			if err := ks.ImportKey(pemBytes, keyInfo.Role, keyInfo.Gun); err != nil {
 				tmpErr = err
 			} else {
 				tmpErr = nil

cryptoservice/import_export_test.go

@@ -300,9 +300,11 @@ func TestImportExportRootKey(t *testing.T) {
 	keyReader, err := os.Open(tempKeyFilePath)
 	assert.NoError(t, err, "could not open key file")
 
-	err = cs2.ImportRootKey(keyReader)
-	assert.NoError(t, err)
+	pemImportBytes, err := ioutil.ReadAll(keyReader)
 	keyReader.Close()
+	assert.NoError(t, err)
+	err = cs2.ImportRoleKey(pemImportBytes, data.CanonicalRootRole, nil)
+	assert.NoError(t, err)
 
 	// Look for repo's root key in repo2
 	// There should be a file named after the key ID of the root key we
@@ -320,11 +322,15 @@ func TestImportExportRootKey(t *testing.T) {
 	decryptedPEMBytes, err := trustmanager.KeyToPEM(privKey, "root")
 	assert.NoError(t, err, "could not convert key to PEM")
 
-	err = cs2.ImportRootKey(bytes.NewReader(decryptedPEMBytes))
+	pemImportBytes, err = ioutil.ReadAll(bytes.NewReader(decryptedPEMBytes))
+	assert.NoError(t, err)
+	err = cs2.ImportRoleKey(pemImportBytes, data.CanonicalRootRole, nil)
 	assert.EqualError(t, err, ErrRootKeyNotEncrypted.Error())
 
 	// Try to import garbage and make sure it doesn't succeed
-	err = cs2.ImportRootKey(strings.NewReader("this is not PEM"))
+	pemImportBytes, err = ioutil.ReadAll(strings.NewReader("this is not PEM"))
+	assert.NoError(t, err)
+	err = cs2.ImportRoleKey(pemImportBytes, data.CanonicalRootRole, nil)
 	assert.EqualError(t, err, ErrNoValidPrivateKey.Error())
 
 	// Should be able to unlock the root key with the old password
@@ -368,9 +374,11 @@ func TestImportExportRootKeyReencrypt(t *testing.T) {
 	keyReader, err := os.Open(tempKeyFilePath)
 	assert.NoError(t, err, "could not open key file")
 
-	err = cs2.ImportRootKey(keyReader)
-	assert.NoError(t, err)
+	pemImportBytes, err := ioutil.ReadAll(keyReader)
 	keyReader.Close()
+	assert.NoError(t, err)
+	err = cs2.ImportRoleKey(pemImportBytes, data.CanonicalRootRole, nil)
+	assert.NoError(t, err)
 
 	// Look for repo's root key in repo2
 	// There should be a file named after the key ID of the root key we

signer/client/signer_trust.go

@@ -202,9 +202,3 @@ func (trust *NotarySigner) CheckHealth(timeout time.Duration) error {
 
 	return err
 }
-
-// ImportRootKey satisfies the CryptoService interface. It should not be implemented
-// for a NotarySigner.
-func (trust *NotarySigner) ImportRootKey(r io.Reader) error {
-	return errors.New("Importing a root key to NotarySigner is not supported")
-}

trustmanager/keyfilestore.go

@@ -70,12 +70,37 @@ func generateKeyInfoMap(s LimitedFileStore) map[string]KeyInfo {
 			logrus.Error(err)
 			continue
 		}
-		keyID, keyInfo := getImportedKeyInfo(d, keyPath)
+		keyID, keyInfo := KeyInfoFromPEM(d, keyPath)
 		keyInfoMap[keyID] = keyInfo
 	}
 	return keyInfoMap
 }
 
+// Attempts to infer the keyID, role, and GUN from the specified key path.
+// Note that non-root roles can only be inferred if this is a legacy style filename: KEYID_ROLE.key
+func inferKeyInfoFromKeyPath(keyPath string) (string, string, string) {
+	var keyID, role, gun string
+	keyID = filepath.Base(keyPath)
+	underscoreIndex := strings.LastIndex(keyID, "_")
+
+	// 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
+	if underscoreIndex != -1 {
+		role = keyID[underscoreIndex+1:]
+		keyID = keyID[:underscoreIndex]
+	}
+
+	if filepath.HasPrefix(keyPath, notary.RootKeysSubdir+"/") {
+		return keyID, data.CanonicalRootRole, ""
+	}
+
+	keyPath = strings.TrimPrefix(keyPath, notary.NonRootKeysSubdir+"/")
+	gun = getGunFromFullID(keyPath)
+	return keyID, role, gun
+}
+
 func getGunFromFullID(fullKeyID string) string {
 	keyGun := filepath.Dir(fullKeyID)
 	// If the gun is empty, Dir will return .
@@ -182,14 +207,16 @@ 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.
-func (s *KeyFileStore) ImportKey(pemBytes []byte, alias string) error {
-	err := importKey(s, s.Retriever, s.cachedKeys, alias, pemBytes)
+// with the given key ID, role, and gun.
+func (s *KeyFileStore) ImportKey(pemBytes []byte, role, gun string) error {
+	if data.IsDelegation(role) {
+		gun = ""
+	}
+	keyID, err := importKey(s, s.Retriever, s.cachedKeys, role, gun, pemBytes)
 	if err != nil {
 		return err
 	}
-	keyID, keyInfo := getImportedKeyInfo(pemBytes, alias)
-	s.keyInfoMap[keyID] = keyInfo
+	s.keyInfoMap[keyID] = KeyInfo{Role: role, Gun: gun}
 	return nil
 }
 
@@ -291,45 +318,28 @@ func (s *KeyMemoryStore) ExportKey(name string) ([]byte, error) {
 
 // ImportKey imports the private key in the encrypted bytes into the keystore
 // with the given key ID and alias.
-func (s *KeyMemoryStore) ImportKey(pemBytes []byte, alias string) error {
-	err := importKey(s, s.Retriever, s.cachedKeys, alias, pemBytes)
+func (s *KeyMemoryStore) ImportKey(pemBytes []byte, role, gun string) error {
+	if data.IsDelegation(role) {
+		gun = ""
+	}
+	keyID, err := importKey(s, s.Retriever, s.cachedKeys, role, gun, pemBytes)
 	if err != nil {
 		return err
 	}
-	keyID, keyInfo := getImportedKeyInfo(pemBytes, alias)
-	s.keyInfoMap[keyID] = keyInfo
+	s.keyInfoMap[keyID] = KeyInfo{Role: role, Gun: gun}
 	return nil
 }
 
-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, notary.RootKeysSubdir+"/") {
-		role = data.CanonicalRootRole
-		gun = ""
-		filename = strings.TrimPrefix(filename, notary.RootKeysSubdir+"/")
-	} else {
-		filename = strings.TrimPrefix(filename, notary.NonRootKeysSubdir+"/")
-		gun = getGunFromFullID(filename)
-	}
-	keyIDFull := filepath.Base(filename)
-	underscoreIndex := strings.LastIndex(keyIDFull, "_")
-	if underscoreIndex == -1 {
+// KeyInfoFromPEM attempts to get a keyID and KeyInfo from the filename and PEM bytes of a key
+func KeyInfoFromPEM(pemBytes []byte, filename string) (string, KeyInfo) {
+	keyID, role, gun := inferKeyInfoFromKeyPath(filename)
+	if role == "" {
 		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
-		// 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 := keyIDFull
-		keyID = legacyID[:underscoreIndex]
-		role = legacyID[underscoreIndex+1:]
 	}
 	return keyID, KeyInfo{Gun: gun, Role: role}
 }
@@ -521,20 +531,23 @@ func encryptAndAddKey(s LimitedFileStore, passwd string, cachedKeys map[string]*
 	return s.Add(filepath.Join(getSubdir(role), name), pemPrivKey)
 }
 
-func importKey(s LimitedFileStore, passphraseRetriever passphrase.Retriever, cachedKeys map[string]*cachedKey, alias string, pemBytes []byte) error {
-
-	if alias != data.CanonicalRootRole {
-		return s.Add(alias, pemBytes)
-	}
-
-	privKey, passphrase, err := GetPasswdDecryptBytes(
-		passphraseRetriever, pemBytes, "", "imported "+alias)
-
+func importKey(s LimitedFileStore, passphraseRetriever passphrase.Retriever, cachedKeys map[string]*cachedKey, role, gun string, pemBytes []byte) (string, error) {
+	// See if the key is encrypted. If its encrypted we'll fail to parse the private key on this first check
+	privKey, err := ParsePEMPrivateKey(pemBytes, "")
 	if err != nil {
-		return err
+		var password string
+		privKey, password, err = GetPasswdDecryptBytes(passphraseRetriever, pemBytes, "", "imported "+role)
+		if err != nil {
+			return "", err
+		}
+		if role == data.CanonicalRootRole {
+			return privKey.ID(), encryptAndAddKey(s, password, cachedKeys, privKey.ID(), role, privKey)
+		}
 	}
-
-	var name string
-	name = privKey.ID()
-	return encryptAndAddKey(s, passphrase, cachedKeys, name, alias, privKey)
+	pemBytesWithRole, err := KeyToPEM(privKey, role)
+	if err != nil {
+		return privKey.ID(), err
+	}
+	s.Add(filepath.Join(getSubdir(role), gun, privKey.ID()), pemBytesWithRole)
+	return privKey.ID(), nil
 }

trustmanager/keyfilestore_test.go

@@ -771,10 +771,10 @@ func assertExportKeySuccess(
 func assertImportKeySuccess(
 	t *testing.T, s KeyStore, expectedKey data.PrivateKey) {
 
-	pemBytes, err := EncryptPrivateKey(expectedKey, "root", cannedPassphrase)
+	pemBytes, err := EncryptPrivateKey(expectedKey, data.CanonicalRootRole, cannedPassphrase)
 	assert.NoError(t, err)
 
-	err = s.ImportKey(pemBytes, "root")
+	err = s.ImportKey(pemBytes, data.CanonicalRootRole, "")
 	assert.NoError(t, err)
 
 	reimportedKey, reimportedAlias, err := s.GetKey(expectedKey.ID())

trustmanager/keystore.go

@@ -48,7 +48,7 @@ type KeyStore interface {
 	ListKeys() map[string]KeyInfo
 	RemoveKey(name string) error
 	ExportKey(name string) ([]byte, error)
-	ImportKey(pemBytes []byte, alias string) error
+	ImportKey(pemBytes []byte, role, gun string) error
 	Name() string
 }
 

trustmanager/yubikey/yubikeystore.go

@@ -762,19 +762,19 @@ func (s *YubiKeyStore) ExportKey(keyID string) ([]byte, error) {
 	return nil, errors.New("Keys cannot be exported from a Yubikey.")
 }
 
-// ImportKey imports a root key into a Yubikey
-func (s *YubiKeyStore) ImportKey(pemBytes []byte, keyPath string) error {
-	logrus.Debugf("Attempting to import: %s key inside of YubiKeyStore", keyPath)
-	if keyPath != data.CanonicalRootRole {
+// ImportKey imports only root keys into a Yubikey, ignoring the specified gun
+func (s *YubiKeyStore) ImportKey(pemBytes []byte, role, gun string) error {
+	logrus.Debugf("Attempting to import: %s key inside of YubiKeyStore", role)
+	if role != data.CanonicalRootRole {
 		return fmt.Errorf("yubikey only supports storing root keys")
 	}
 	privKey, _, err := trustmanager.GetPasswdDecryptBytes(
 		s.passRetriever, pemBytes, "", "imported root")
 	if err != nil {
-		logrus.Debugf("Failed to get and retrieve a key from: %s", keyPath)
+		logrus.Debugf("Failed to get and retrieve a key from: %s", role)
 		return err
 	}
-	_, err = s.addKey(privKey.ID(), "root", privKey)
+	_, err = s.addKey(privKey.ID(), role, privKey)
 	return err
 }
 

trustmanager/yubikey/yubikeystore_test.go

@@ -339,10 +339,10 @@ func TestYubiImportNewKey(t *testing.T) {
 	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
 	assert.NoError(t, err)
 
-	pemBytes, err := trustmanager.EncryptPrivateKey(privKey, "root", "passphrase")
+	pemBytes, err := trustmanager.EncryptPrivateKey(privKey, data.CanonicalRootRole, "passphrase")
 	assert.NoError(t, err)
 
-	err = store.ImportKey(pemBytes, "root")
+	err = store.ImportKey(pemBytes, data.CanonicalRootRole, "")
 	assert.NoError(t, err)
 
 	// key is not in backup store
@@ -388,9 +388,9 @@ func TestYubiImportExistingKey(t *testing.T) {
 	assert.NotNil(t, k)
 
 	// import the key, which should have already been added to the yubikey
-	pemBytes, err := trustmanager.EncryptPrivateKey(key, "root", "passphrase")
+	pemBytes, err := trustmanager.EncryptPrivateKey(key, data.CanonicalRootRole, "passphrase")
 	assert.NoError(t, err)
-	err = newStore.ImportKey(pemBytes, "root")
+	err = newStore.ImportKey(pemBytes, data.CanonicalRootRole, "")
 	assert.NoError(t, err)
 
 	// key is not in backup store
@@ -418,10 +418,10 @@ func TestYubiImportNonRootKey(t *testing.T) {
 	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
 	assert.NoError(t, err)
 
-	pemBytes, err := trustmanager.EncryptPrivateKey(privKey, "root", "passphrase")
+	pemBytes, err := trustmanager.EncryptPrivateKey(privKey, data.CanonicalRootRole, "passphrase")
 	assert.NoError(t, err)
 
-	err = store.ImportKey(pemBytes, privKey.ID())
+	err = store.ImportKey(pemBytes, data.CanonicalTargetsRole, "")
 	assert.Error(t, err)
 
 	// key is not in backup store
@@ -690,10 +690,10 @@ func TestYubiImportKeyCleansUpOnError(t *testing.T) {
 	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
 	assert.NoError(t, err)
 
-	pemBytes, err := trustmanager.EncryptPrivateKey(privKey, "root", "passphrase")
+	pemBytes, err := trustmanager.EncryptPrivateKey(privKey, data.CanonicalRootRole, "passphrase")
 	assert.NoError(t, err)
 
-	var _importkey = func() error { return store.ImportKey(pemBytes, "root") }
+	var _importkey = func() error { return store.ImportKey(pemBytes, data.CanonicalRootRole, "") }
 
 	testYubiFunctionCleansUpOnLoginError(t, store, _importkey)
 	// all the PKCS11 functions ImportKey depends on that aren't the login/logout

tuf/signed/ed25519.go

@@ -3,10 +3,7 @@ package signed
 import (
 	"crypto/rand"
 	"errors"
-	"io"
-	"io/ioutil"
 
-	"github.com/agl/ed25519"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 )
@@ -108,22 +105,3 @@ func (e *Ed25519) GetPrivateKey(keyID string) (data.PrivateKey, string, error) {
 	}
 	return nil, "", trustmanager.ErrKeyNotFound{KeyID: keyID}
 }
-
-// ImportRootKey adds an Ed25519 key to the store as a root key
-func (e *Ed25519) ImportRootKey(r io.Reader) error {
-	raw, err := ioutil.ReadAll(r)
-	if err != nil {
-		return err
-	}
-	dataSize := ed25519.PublicKeySize + ed25519.PrivateKeySize
-	if len(raw) < dataSize || len(raw) > dataSize {
-		return errors.New("Wrong length of data for Ed25519 Key Import")
-	}
-	public := data.NewED25519PublicKey(raw[:ed25519.PublicKeySize])
-	private, err := data.NewED25519PrivateKey(*public, raw[ed25519.PublicKeySize:])
-	e.keys[private.ID()] = edCryptoKey{
-		role:    "root",
-		privKey: private,
-	}
-	return nil
-}

tuf/signed/interface.go

@@ -2,7 +2,6 @@ package signed
 
 import (
 	"github.com/docker/notary/tuf/data"
-	"io"
 )
 
 // KeyService provides management of keys locally. It will never
@@ -33,10 +32,6 @@ type KeyService interface {
 
 	// ListAllKeys returns a map of all available signing key IDs to role
 	ListAllKeys() map[string]string
-
-	// ImportRootKey imports a root key to the highest priority keystore associated with
-	// the cryptoservice
-	ImportRootKey(source io.Reader) error
 }
 
 // CryptoService is deprecated and all instances of its use should be

tuf/signed/sign_test.go

@@ -3,7 +3,6 @@ package signed
 import (
 	"crypto/rand"
 	"encoding/pem"
-	"io"
 	"testing"
 
 	"github.com/docker/notary/cryptoservice"
@@ -60,10 +59,6 @@ func (mts *FailingCryptoService) RemoveKey(keyID string) error {
 	return nil
 }
 
-func (mts *FailingCryptoService) ImportRootKey(r io.Reader) error {
-	return nil
-}
-
 type MockCryptoService struct {
 	testKey data.PrivateKey
 }
@@ -104,10 +99,6 @@ func (mts *MockCryptoService) RemoveKey(keyID string) error {
 	return nil
 }
 
-func (mts *MockCryptoService) ImportRootKey(r io.Reader) error {
-	return nil
-}
-
 var _ CryptoService = &MockCryptoService{}
 
 type StrictMockCryptoService struct {
@@ -138,10 +129,6 @@ func (mts *StrictMockCryptoService) AddKey(key data.PrivateKey, role string) err
 	return nil
 }
 
-func (mts *StrictMockCryptoService) ImportRootKey(r io.Reader) error {
-	return nil
-}
-
 // Test signing and ensure the expected signature is added
 func TestBasicSign(t *testing.T) {
 	cs := NewEd25519()