Merge pull request #39 from docker/fix-import

Do not back up a root key that is imported into Yubikey.

Signed-off-by: David Lawrence <david.lawrence@docker.com>

Signed-off-by: Diogo Mónica <diogo.monica@gmail.com> (github: endophage)

cmd/notary/integration_test.go

@@ -178,17 +178,18 @@ func GetKeys(t *testing.T, tempDir string) ([]string, []string) {
 
 // List keys, parses the output, and asserts something about the number of root
 // keys and number of signing keys, as well as returning them.
-func assertNumKeys(t *testing.T, tempDir string, numRoot, numSigning int) (
+func assertNumKeys(t *testing.T, tempDir string, numRoot, numSigning int,
-	[]string, []string) {
+	rootOnDisk bool) ([]string, []string) {
 
 	root, signing := GetKeys(t, tempDir)
 	assert.Len(t, root, numRoot)
 	assert.Len(t, signing, numSigning)
 	for _, rootKeyID := range root {
-		// it should always be present on disk
 		_, err := os.Stat(filepath.Join(
 			tempDir, "private", "root_keys", rootKeyID+"_root.key"))
-		assert.NoError(t, err)
+		// 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
+		assert.Equal(t, rootOnDisk, !os.IsNotExist(err))
 
 		// this function is declared is in the build-tagged setup files
 		verifyRootKeyOnHardware(t, rootKeyID)
@@ -241,17 +242,17 @@ func TestClientKeyGenerationRotation(t *testing.T) {
 	// -- tests --
 
 	// starts out with no keys
-	assertNumKeys(t, tempDir, 0, 0)
+	assertNumKeys(t, tempDir, 0, 0, true)
 
 	// generate root key produces a single root key and no other keys
 	_, err = runCommand(t, tempDir, "key", "generate", "ecdsa")
 	assert.NoError(t, err)
-	assertNumKeys(t, tempDir, 1, 0)
+	assertNumKeys(t, tempDir, 1, 0, true)
 
 	// initialize a repo, should have signing keys and no new root key
 	_, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun")
 	assert.NoError(t, err)
-	origRoot, origSign := assertNumKeys(t, tempDir, 1, 2)
+	origRoot, origSign := assertNumKeys(t, tempDir, 1, 2, true)
 
 	// publish using the original keys
 	assertSuccessfullyPublish(t, tempDir, server.URL, "gun", target, tempfiles[0])
@@ -259,7 +260,7 @@ func TestClientKeyGenerationRotation(t *testing.T) {
 	// rotate the signing keys
 	_, err = runCommand(t, tempDir, "key", "rotate", "gun")
 	assert.NoError(t, err)
-	root, sign := assertNumKeys(t, tempDir, 1, 4)
+	root, sign := assertNumKeys(t, tempDir, 1, 4, true)
 	assert.Equal(t, origRoot[0], root[0])
 	// there should be the new keys and the old keys
 	for _, origKey := range origSign {
@@ -275,7 +276,7 @@ func TestClientKeyGenerationRotation(t *testing.T) {
 	// publish the key rotation
 	_, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun")
 	assert.NoError(t, err)
-	root, sign = assertNumKeys(t, tempDir, 1, 2)
+	root, sign = assertNumKeys(t, tempDir, 1, 2, true)
 	assert.Equal(t, origRoot[0], root[0])
 	// just do a cursory rotation check that the keys aren't equal anymore
 	for _, origKey := range origSign {
@@ -332,7 +333,7 @@ func TestClientKeyImportExportRootAndSigning(t *testing.T) {
 		assertSuccessfullyPublish(
 			t, dirs[0], server.URL, gun, target, tempfiles[0])
 	}
-	assertNumKeys(t, dirs[0], 1, 4)
+	assertNumKeys(t, dirs[0], 1, 4, true)
 
 	// -- tests --
 	zipfile := tempfiles[0] + ".zip"
@@ -344,7 +345,7 @@ func TestClientKeyImportExportRootAndSigning(t *testing.T) {
 
 	_, err = runCommand(t, dirs[1], "key", "import", zipfile)
 	assert.NoError(t, err)
-	assertNumKeys(t, dirs[1], 1, 4) // all keys should be there
+	assertNumKeys(t, dirs[1], 1, 4, true) // all keys should be there
 
 	// can list and publish to both repos using imported keys
 	for _, gun := range []string{"gun1", "gun2"} {
@@ -365,16 +366,12 @@ func TestClientKeyImportExportRootAndSigning(t *testing.T) {
 
 	// this function is declared is in the build-tagged setup files
 	if rootOnHardware() {
-		// hardware root is still present, but two signing keys moved - we
+		// hardware root is still present, and the key will ONLY be on hardware
-		// can't call assertNumKeys, because in this case it will ONLY be on
+		// and not on disk
-		// hardware and not on disk
+		assertNumKeys(t, dirs[2], 1, 2, false)
-		root, signing := GetKeys(t, dirs[2])
-		assert.Len(t, root, 1)
-		verifyRootKeyOnHardware(t, root[0])
-		assert.Len(t, signing, 2)
 	} else {
 		// only 2 signing keys should be there, and no root key
-		assertNumKeys(t, dirs[2], 0, 2)
+		assertNumKeys(t, dirs[2], 0, 2, true)
 	}
 }
 
@@ -388,7 +385,7 @@ func exportRoot(t *testing.T, exportTo string) string {
 	// generate root key produces a single root key and no other keys
 	_, err = runCommand(t, tempDir, "key", "generate", "ecdsa")
 	assert.NoError(t, err)
-	oldRoot, _ := assertNumKeys(t, tempDir, 1, 0)
+	oldRoot, _ := assertNumKeys(t, tempDir, 1, 0, true)
 
 	// export does not require a password
 	oldRetriever := retriever
@@ -451,13 +448,16 @@ func TestClientKeyImportExportRootOnly(t *testing.T) {
 	// import the key
 	_, err = runCommand(t, tempDir, "key", "import-root", tempFile.Name())
 	assert.NoError(t, err)
-	newRoot, _ := assertNumKeys(t, tempDir, 1, 0)
+
+	// if there is hardware available, root will only be on hardware, and not
+	// on disk
+	newRoot, _ := assertNumKeys(t, tempDir, 1, 0, !rootOnHardware())
 	assert.Equal(t, rootKeyID, newRoot[0])
 
 	// Just to make sure, init a repo and publish
 	_, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun")
 	assert.NoError(t, err)
-	assertNumKeys(t, tempDir, 1, 2)
+	assertNumKeys(t, tempDir, 1, 2, !rootOnHardware())
 	assertSuccessfullyPublish(
 		t, tempDir, server.URL, "gun", target, tempFile.Name())
 }

cryptoservice/import_export.go

@@ -112,8 +112,8 @@ func (cs *CryptoService) ImportRootKey(source io.Reader) error {
 
 	for _, ks := range cs.keyStores {
 		// don't redeclare err, we want the value carried out of the loop
-		if err = ks.ImportKey(pemBytes, "root"); err != nil {
+		if err = ks.ImportKey(pemBytes, "root"); err == nil {
-			continue
+			return nil //bail on the first keystore we import to
 		}
 	}
 

passphrase/passphrase.go

@@ -124,12 +124,17 @@ func PromptRetrieverWithInOut(in io.Reader, out io.Writer, aliasMap map[string]s
 			}
 		}
 
+		withID := fmt.Sprintf(" with ID %s", shortName)
+		if shortName == "" {
+			withID = ""
+		}
+
 		if createNew {
-			fmt.Fprintf(out, "Enter passphrase for new %s key with ID %s: ", displayAlias, shortName)
+			fmt.Fprintf(out, "Enter passphrase for new %s key%s: ", displayAlias, withID)
 		} else if displayAlias == "yubikey" {
 			fmt.Fprintf(out, "Enter the %s for the attached Yubikey: ", keyName)
 		} else {
-			fmt.Fprintf(out, "Enter passphrase for %s key with ID %s: ", displayAlias, shortName)
+			fmt.Fprintf(out, "Enter passphrase for %s key%s: ", displayAlias, withID)
 		}
 
 		passphrase, err := stdin.ReadBytes('\n')
@@ -157,7 +162,7 @@ func PromptRetrieverWithInOut(in io.Reader, out io.Writer, aliasMap map[string]s
 			return "", false, ErrTooShort
 		}
 
-		fmt.Fprintf(out, "Repeat passphrase for new %s key with ID %s: ", displayAlias, shortName)
+		fmt.Fprintf(out, "Repeat passphrase for new %s key%s: ", displayAlias, withID)
 		confirmation, err := stdin.ReadBytes('\n')
 		fmt.Fprintln(out)
 		if err != nil {

trustmanager/keyfilestore.go

@@ -347,7 +347,8 @@ func importKey(s LimitedFileStore, passphraseRetriever passphrase.Retriever, cac
 		return s.Add(alias, pemBytes)
 	}
 
-	privKey, passphrase, err := GetPasswdDecryptBytes(passphraseRetriever, pemBytes, "imported", alias)
+	privKey, passphrase, err := GetPasswdDecryptBytes(
+		passphraseRetriever, pemBytes, "", "imported "+alias)
 
 	if err != nil {
 		return err

trustmanager/yubikeystore.go

@@ -199,9 +199,11 @@ func addECDSAKey(
 		return fmt.Errorf("error importing: %v", err)
 	}
 
-	err = backupStore.AddKey(privKey.ID(), role, privKey)
+	if backupStore != nil {
-	if err != nil {
+		err = backupStore.AddKey(privKey.ID(), role, privKey)
-		return ErrBackupFailed{err: err.Error()}
+		if err != nil {
+			return ErrBackupFailed{err: err.Error()}
+		}
 	}
 
 	return nil
@@ -553,6 +555,11 @@ func (s *YubiKeyStore) ListKeys() map[string]string {
 
 // AddKey puts a key inside the Yubikey, as well as writing it to the backup store
 func (s *YubiKeyStore) AddKey(keyID, role string, privKey data.PrivateKey) error {
+	return s.addKey(keyID, role, privKey, true)
+}
+
+func (s *YubiKeyStore) addKey(
+	keyID, role string, privKey data.PrivateKey, backup bool) error {
 	// We only allow adding root keys for now
 	if role != data.CanonicalRootRole {
 		return fmt.Errorf("yubikey only supports storing root keys, got %s for key: %s\n", role, keyID)
@@ -576,7 +583,14 @@ func (s *YubiKeyStore) AddKey(keyID, role string, privKey data.PrivateKey) error
 		return err
 	}
 	logrus.Debugf("Using yubikey slot %v", slot)
-	err = addECDSAKey(ctx, session, privKey, slot, s.passRetriever, role, s.backupStore)
+
+	backupStore := s.backupStore
+	if !backup {
+		backupStore = nil
+	}
+
+	err = addECDSAKey(
+		ctx, session, privKey, slot, s.passRetriever, role, backupStore)
 	if err == nil {
 		s.keys[privKey.ID()] = yubiSlot{
 			role:   role,
@@ -636,18 +650,21 @@ func (s *YubiKeyStore) RemoveKey(keyID string) error {
 	return err
 }
 
+// ExportKey doesn't work, because you can't export data from a Yubikey
 func (s *YubiKeyStore) ExportKey(keyID string) ([]byte, error) {
 	logrus.Debugf("Attempting to export: %s key inside of YubiKeyStore", keyID)
 	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, keyID string) error {
 	logrus.Debugf("Attempting to import: %s key inside of YubiKeyStore", keyID)
-	privKey, _, err := GetPasswdDecryptBytes(s.passRetriever, pemBytes, "imported", "root")
+	privKey, _, err := GetPasswdDecryptBytes(
+		s.passRetriever, pemBytes, "", "imported root")
 	if err != nil {
 		return err
 	}
-	return s.AddKey(privKey.ID(), "root", privKey)
+	return s.addKey(privKey.ID(), "root", privKey, false)
 }
 
 func cleanup(ctx *pkcs11.Ctx, session pkcs11.SessionHandle) {

trustmanager/yubikeystore_test.go

@@ -78,3 +78,42 @@ func TestAddKeyToNextEmptyYubikeySlot(t *testing.T) {
 	_, err = testAddKey(t, store)
 	assert.NoError(t, err)
 }
+
+// ImportKey imports a key as root without adding it to the backup store
+func TestImportKey(t *testing.T) {
+	if !YubikeyAccessible() {
+		t.Skip("Must have Yubikey access.")
+	}
+	clearAllKeys(t)
+
+	ret := passphrase.ConstantRetriever("passphrase")
+	backup := NewKeyMemoryStore(ret)
+	store, err := NewYubiKeyStore(backup, ret)
+	assert.NoError(t, err)
+	SetYubikeyKeyMode(KeymodeNone)
+	defer func() {
+		SetYubikeyKeyMode(KeymodeTouch | KeymodePinOnce)
+	}()
+
+	// generate key and import it
+	privKey, err := GenerateECDSAKey(rand.Reader)
+	assert.NoError(t, err)
+
+	pemBytes, err := EncryptPrivateKey(privKey, "passphrase")
+	assert.NoError(t, err)
+
+	err = store.ImportKey(pemBytes, privKey.ID())
+	assert.NoError(t, err)
+
+	// key is not in backup store
+	_, _, err = backup.GetKey(privKey.ID())
+	assert.Error(t, err)
+
+	// ensure key is in Yubikey -  create a new store, to make sure we're not
+	// just using the keys cache
+	store, err = NewYubiKeyStore(NewKeyMemoryStore(ret), ret)
+	gottenKey, role, err := store.GetKey(privKey.ID())
+	assert.NoError(t, err)
+	assert.Equal(t, data.CanonicalRootRole, role)
+	assert.Equal(t, privKey.Public(), gottenKey.Public())
+}