Fix new bug where adding a duplicate key to a yubikey added to the backup.

Added a test for this case as well - thanks @endophage!

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)

trustmanager/keystore.go

@@ -40,6 +40,8 @@ const (
 
 // KeyStore is a generic interface for private key storage
 type KeyStore interface {
+	// Add Key adds a key to the KeyStore, and if the key already exists,
+	// succeeds.  Otherwise, returns an error if it cannot add.
 	AddKey(name, alias string, privKey data.PrivateKey) error
 	GetKey(name string) (data.PrivateKey, string, error)
 	ListKeys() map[string]string

trustmanager/yubikey/yubikeystore.go

@@ -631,42 +631,49 @@ 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 {
-	err := s.addKey(keyID, role, privKey)
+	added, err := s.addKey(keyID, role, privKey)
 	if err != nil {
 		return err
 	}
-	err = s.backupStore.AddKey(privKey.ID(), role, privKey)
+	if added {
-	if err != nil {
+		err = s.backupStore.AddKey(privKey.ID(), role, privKey)
-		defer s.RemoveKey(keyID)
+		if err != nil {
-		return ErrBackupFailed{err: err.Error()}
+			defer s.RemoveKey(keyID)
+			return ErrBackupFailed{err: err.Error()}
+		}
 	}
 	return nil
 }
 
-func (s *YubiKeyStore) addKey(keyID, role string, privKey data.PrivateKey) error {
+// Only add if we haven't seen the key already.  Return whether the key was
+// added.
+func (s *YubiKeyStore) addKey(keyID, role string, privKey data.PrivateKey) (
+	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", role, keyID)
+		return false, fmt.Errorf(
+			"yubikey only supports storing root keys, got %s for key: %s", role, keyID)
 	}
 
 	ctx, session, err := SetupHSMEnv(pkcs11Lib, s.libLoader)
 	if err != nil {
 		logrus.Debugf("Failed to initialize PKCS11 environment: %s", err.Error())
-		return err
+		return false, err
 	}
 	defer cleanup(ctx, session)
 
 	if k, ok := s.keys[keyID]; ok {
 		if k.role == role {
 			// already have the key and it's associated with the correct role
-			return nil
+			return false, nil
 		}
 	}
 
 	slot, err := getNextEmptySlot(ctx, session)
 	if err != nil {
 		logrus.Debugf("Failed to get an empty yubikey slot: %s", err.Error())
-		return err
+		return false, err
 	}
 	logrus.Debugf("Attempting to store key using yubikey slot %v", slot)
 
@@ -677,11 +684,11 @@ func (s *YubiKeyStore) addKey(keyID, role string, privKey data.PrivateKey) error
 			role:   role,
 			slotID: slot,
 		}
-		return nil
+		return true, nil
 	}
 	logrus.Debugf("Failed to add key to yubikey: %v", err)
 
-	return err
+	return false, err
 }
 
 // GetKey retrieves a key from the Yubikey only (it does not look inside the
@@ -758,7 +765,8 @@ func (s *YubiKeyStore) ImportKey(pemBytes []byte, keyPath string) error {
 	if keyPath != data.CanonicalRootRole {
 		return fmt.Errorf("yubikey only supports storing root keys")
 	}
-	return s.addKey(privKey.ID(), "root", privKey)
+	_, err = s.addKey(privKey.ID(), "root", privKey)
+	return err
 }
 
 func cleanup(ctx IPKCS11Ctx, session pkcs11.SessionHandle) {

trustmanager/yubikey/yubikeystore_test.go

@@ -247,6 +247,39 @@ func TestYubiAddKeyRollsBackIfCannotBackup(t *testing.T) {
 	assert.Len(t, cleanListKeys(t), 0)
 }
 
+// If, when adding a key to the Yubikey, and it already exists, we succeed
+// without adding it to the backup store.
+func TestYubiAddDuplicateKeySucceedsButDoesNotBackup(t *testing.T) {
+	if !YubikeyAccessible() {
+		t.Skip("Must have Yubikey access.")
+	}
+	clearAllKeys(t)
+
+	SetYubikeyKeyMode(KeymodeNone)
+	defer func() {
+		SetYubikeyKeyMode(KeymodeTouch | KeymodePinOnce)
+	}()
+
+	origStore, err := NewYubiKeyStore(trustmanager.NewKeyMemoryStore(ret), ret)
+	assert.NoError(t, err)
+
+	key, err := testAddKey(t, origStore)
+	assert.NoError(t, err)
+
+	backup := trustmanager.NewKeyMemoryStore(ret)
+	cleanStore, err := NewYubiKeyStore(backup, ret)
+	assert.NoError(t, err)
+	assert.Len(t, cleanStore.ListKeys(), 1)
+
+	err = cleanStore.AddKey(key.ID(), "root", key)
+	assert.NoError(t, err)
+
+	// there should be just 1 key on the yubikey
+	assert.Len(t, cleanListKeys(t), 1)
+	// nothing was added to the backup
+	assert.Len(t, backup.ListKeys(), 0)
+}
+
 // RemoveKey removes a key from the yubikey, but not from the backup store.
 func TestYubiRemoveKey(t *testing.T) {
 	if !YubikeyAccessible() {
@@ -875,7 +908,7 @@ func TestYubiRetrySignUntilSuccess(t *testing.T) {
 
 // If Sign gives us an invalid signature, we retry until up to a maximum of 5
 // times, and if it's still invalid, fail.
-func TestYubiRetrySign5TimesAndFail(t *testing.T) {
+func TestYubiRetrySignUntilFail(t *testing.T) {
 	if !YubikeyAccessible() {
 		t.Skip("Must have Yubikey access.")
 	}
@@ -905,7 +938,7 @@ func TestYubiRetrySign5TimesAndFail(t *testing.T) {
 	badSigner := &SignInvalidSigCtx{
 		Ctx:     *pkcs11.New(pkcs11Lib),
 		goodSig: goodSig,
-		failNum: 5,
+		failNum: sigAttempts + 1,
 	}
 
 	yubiPrivateKey.setLibLoader(func(string) IPKCS11Ctx { return badSigner })
@@ -914,7 +947,7 @@ func TestYubiRetrySign5TimesAndFail(t *testing.T) {
 	assert.Error(t, err)
 	// because the SignInvalidSigCtx returns the good signature, we can just
 	// deep equal instead of verifying
-	assert.Equal(t, 5, badSigner.signCalls)
+	assert.Equal(t, sigAttempts, badSigner.signCalls)
 }
 
 // -----  Stubbed pkcs11 for testing error conditions ------