From 87231d9a5d9fbeea3a3f5696229783a055aa2c06 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 11 Nov 2015 22:25:27 -0800 Subject: [PATCH] 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 Signed-off-by: David Lawrence Signed-off-by: Ying Li (github: endophage) --- trustmanager/keystore.go | 2 ++ trustmanager/yubikey/yubikeystore.go | 34 ++++++++++++-------- trustmanager/yubikey/yubikeystore_test.go | 39 +++++++++++++++++++++-- 3 files changed, 59 insertions(+), 16 deletions(-) diff --git a/trustmanager/keystore.go b/trustmanager/keystore.go index 88a0e3d774..2d41e32b5e 100644 --- a/trustmanager/keystore.go +++ b/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 diff --git a/trustmanager/yubikey/yubikeystore.go b/trustmanager/yubikey/yubikeystore.go index 1b933ac1ae..3eacbf215d 100644 --- a/trustmanager/yubikey/yubikeystore.go +++ b/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 err != nil { - defer s.RemoveKey(keyID) - return ErrBackupFailed{err: err.Error()} + if added { + err = s.backupStore.AddKey(privKey.ID(), role, privKey) + if err != nil { + 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) { diff --git a/trustmanager/yubikey/yubikeystore_test.go b/trustmanager/yubikey/yubikeystore_test.go index a38b0c697d..621b8dd11e 100644 --- a/trustmanager/yubikey/yubikeystore_test.go +++ b/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 ------