From 96bfaac05f9373c39fb8f0e9432d2a2be0d96d18 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 11 Nov 2015 21:03:07 -0800 Subject: [PATCH 1/6] Add tests for verifying signatures before returning a signature. Signed-off-by: Ying Li Signed-off-by: David Lawrence Signed-off-by: Ying Li (github: endophage) --- trustmanager/yubikey/yubikeystore_test.go | 113 ++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/trustmanager/yubikey/yubikeystore_test.go b/trustmanager/yubikey/yubikeystore_test.go index 672a61eb35..7605ce66a0 100644 --- a/trustmanager/yubikey/yubikeystore_test.go +++ b/trustmanager/yubikey/yubikeystore_test.go @@ -790,6 +790,95 @@ func TestYubiSignCleansUpOnError(t *testing.T) { []string{"Logout"}, false) } +// If Sign gives us an invalid signature, we retry until successful up to +// a maximum of 5 times. +func TestYubiRetrySignUntilSuccess(t *testing.T) { + if !YubikeyAccessible() { + t.Skip("Must have Yubikey access.") + } + clearAllKeys(t) + + SetYubikeyKeyMode(KeymodeNone) + defer func() { + SetYubikeyKeyMode(KeymodeTouch | KeymodePinOnce) + }() + + store, err := NewYubiKeyStore(trustmanager.NewKeyMemoryStore(ret), ret) + assert.NoError(t, err) + + key, err := testAddKey(t, store) + assert.NoError(t, err) + + message := []byte("Hello there") + goodSig, err := key.Sign(rand.Reader, message, nil) + assert.NoError(t, err) + + privKey, _, err := store.GetKey(key.ID()) + assert.NoError(t, err) + + yubiPrivateKey, ok := privKey.(*YubiPrivateKey) + assert.True(t, ok) + + badSigner := &SignInvalidSigCtx{ + Ctx: *pkcs11.New(pkcs11Lib), + goodSig: goodSig, + failNum: 2, + } + + yubiPrivateKey.setLibLoader(func(string) IPKCS11Ctx { return badSigner }) + + sig, err := yubiPrivateKey.Sign(rand.Reader, message, nil) + assert.NoError(t, err) + // because the SignInvalidSigCtx returns the good signature, we can just + // deep equal instead of verifying + assert.True(t, reflect.DeepEqual(goodSig, sig)) + assert.Equal(t, 3, badSigner.signCalls) +} + +// 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) { + if !YubikeyAccessible() { + t.Skip("Must have Yubikey access.") + } + clearAllKeys(t) + + SetYubikeyKeyMode(KeymodeNone) + defer func() { + SetYubikeyKeyMode(KeymodeTouch | KeymodePinOnce) + }() + + store, err := NewYubiKeyStore(trustmanager.NewKeyMemoryStore(ret), ret) + assert.NoError(t, err) + + key, err := testAddKey(t, store) + assert.NoError(t, err) + + message := []byte("Hello there") + goodSig, err := key.Sign(rand.Reader, message, nil) + assert.NoError(t, err) + + privKey, _, err := store.GetKey(key.ID()) + assert.NoError(t, err) + + yubiPrivateKey, ok := privKey.(*YubiPrivateKey) + assert.True(t, ok) + + badSigner := &SignInvalidSigCtx{ + Ctx: *pkcs11.New(pkcs11Lib), + goodSig: goodSig, + failNum: 5, + } + + yubiPrivateKey.setLibLoader(func(string) IPKCS11Ctx { return badSigner }) + + _, err = yubiPrivateKey.Sign(rand.Reader, message, nil) + 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) +} + // ----- Stubbed pkcs11 for testing error conditions ------ // This is just a passthrough to the underlying pkcs11 library, with optional // error injection. This is to ensure that if errors occur during the process @@ -966,3 +1055,27 @@ func (s *StubCtx) Sign(sh pkcs11.SessionHandle, message []byte) ([]byte, error) } return sig, sigErr } + +// a different stub Ctx object in which Sign returns an invalid signature some +// number of times +type SignInvalidSigCtx struct { + pkcs11.Ctx + + // Signature verification is to mitigate against hardware failure while + // signing - which might occur during testing. So to prevent spurious + // errors, return a real known good signature in the success case. + goodSig []byte + + failNum int // number of calls to fail before succeeding + signCalls int // number of calls to Sign so far +} + +func (s *SignInvalidSigCtx) Sign(sh pkcs11.SessionHandle, message []byte) ([]byte, error) { + s.signCalls++ + s.Ctx.Sign(sh, message) // clear out the SignInit + + if s.signCalls > s.failNum { + return s.goodSig, nil + } + return []byte("12345"), nil +} From 6cf0643d7d1fa6446c0ebf6a6fdc45bcf1885e7e Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 11 Nov 2015 21:13:35 -0800 Subject: [PATCH 2/6] Roll back an add key to the yubikey if we can't back it up. Signed-off-by: Ying Li Signed-off-by: David Lawrence Signed-off-by: Ying Li (github: endophage) --- trustmanager/yubikey/yubikeystore.go | 31 ++++++++---------- trustmanager/yubikey/yubikeystore_test.go | 38 +++++++++++++++++++++++ 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/trustmanager/yubikey/yubikeystore.go b/trustmanager/yubikey/yubikeystore.go index 5df3f430ed..1b933ac1ae 100644 --- a/trustmanager/yubikey/yubikeystore.go +++ b/trustmanager/yubikey/yubikeystore.go @@ -200,7 +200,6 @@ func addECDSAKey( pkcs11KeyID []byte, passRetriever passphrase.Retriever, role string, - backupStore trustmanager.KeyStore, ) error { logrus.Debugf("Attempting to add key to yubikey with ID: %s", privKey.ID()) @@ -253,13 +252,6 @@ func addECDSAKey( return fmt.Errorf("error importing: %v", err) } - if backupStore != nil { - err = backupStore.AddKey(privKey.ID(), role, privKey) - if err != nil { - return ErrBackupFailed{err: err.Error()} - } - } - return nil } @@ -639,11 +631,19 @@ 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) + 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()} + } + return nil } -func (s *YubiKeyStore) addKey( - keyID, role string, privKey data.PrivateKey, backup bool) error { +func (s *YubiKeyStore) addKey(keyID, role string, privKey data.PrivateKey) 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) @@ -670,13 +670,8 @@ func (s *YubiKeyStore) addKey( } logrus.Debugf("Attempting to store key using yubikey slot %v", slot) - backupStore := s.backupStore - if !backup { - backupStore = nil - } - err = addECDSAKey( - ctx, session, privKey, slot, s.passRetriever, role, backupStore) + ctx, session, privKey, slot, s.passRetriever, role) if err == nil { s.keys[privKey.ID()] = yubiSlot{ role: role, @@ -763,7 +758,7 @@ 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, false) + return s.addKey(privKey.ID(), "root", privKey) } func cleanup(ctx IPKCS11Ctx, session pkcs11.SessionHandle) { diff --git a/trustmanager/yubikey/yubikeystore_test.go b/trustmanager/yubikey/yubikeystore_test.go index 7605ce66a0..a38b0c697d 100644 --- a/trustmanager/yubikey/yubikeystore_test.go +++ b/trustmanager/yubikey/yubikeystore_test.go @@ -4,6 +4,7 @@ package yubikey import ( "crypto/rand" + "errors" "fmt" "reflect" "testing" @@ -209,6 +210,43 @@ func TestYubiAddKeyCanAddToMiddleSlot(t *testing.T) { } } +type nonworkingBackup struct { + trustmanager.KeyMemoryStore +} + +// AddKey stores the contents of a PEM-encoded private key as a PEM block +func (s *nonworkingBackup) AddKey(name, alias string, privKey data.PrivateKey) error { + return errors.New("Nope!") +} + +// If, when adding a key to the Yubikey, we can't back up the key, it should +// be removed from the Yubikey too because otherwise there is no way for +// the user to later get a backup of the key. +func TestYubiAddKeyRollsBackIfCannotBackup(t *testing.T) { + if !YubikeyAccessible() { + t.Skip("Must have Yubikey access.") + } + clearAllKeys(t) + + SetYubikeyKeyMode(KeymodeNone) + defer func() { + SetYubikeyKeyMode(KeymodeTouch | KeymodePinOnce) + }() + + backup := &nonworkingBackup{ + KeyMemoryStore: *trustmanager.NewKeyMemoryStore(ret), + } + store, err := NewYubiKeyStore(backup, ret) + assert.NoError(t, err) + + _, err = testAddKey(t, store) + assert.Error(t, err) + assert.IsType(t, ErrBackupFailed{}, err) + + // there should be no keys on the yubikey + assert.Len(t, cleanListKeys(t), 0) +} + // RemoveKey removes a key from the yubikey, but not from the backup store. func TestYubiRemoveKey(t *testing.T) { if !YubikeyAccessible() { From efff7219555934db7c3e42919350a872efb43fd1 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 11 Nov 2015 21:46:48 -0800 Subject: [PATCH 3/6] Add tests for multi-keystore crypto services. Signed-off-by: Ying Li Signed-off-by: David Lawrence Signed-off-by: Ying Li (github: endophage) --- cryptoservice/crypto_service_test.go | 129 +++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/cryptoservice/crypto_service_test.go b/cryptoservice/crypto_service_test.go index 832197a542..dc03a94095 100644 --- a/cryptoservice/crypto_service_test.go +++ b/cryptoservice/crypto_service_test.go @@ -1,6 +1,7 @@ package cryptoservice import ( + "crypto/rand" "fmt" "runtime" "testing" @@ -49,6 +50,29 @@ func (c CryptoServiceTester) TestCreateAndGetKey(t *testing.T) { assert.Equal(t, c.role, alias) } +// If there are multiple keystores, ensure that a key is only added to one - +// the first in the list of keyStores (which is in order of preference) +func (c CryptoServiceTester) TestCreateAndGetWhenMultipleKeystores(t *testing.T) { + cryptoService := c.cryptoServiceFactory() + cryptoService.keyStores = append(cryptoService.keyStores, + trustmanager.NewKeyMemoryStore(passphraseRetriever)) + + // Test Create + tufKey, err := cryptoService.Create(c.role, c.keyAlgo) + assert.NoError(t, err, c.errorMsg("error creating key")) + + // Only the first keystore should have the key + _, _, err = cryptoService.keyStores[0].GetKey(tufKey.ID()) + assert.NoError(t, err) + _, _, err = cryptoService.keyStores[1].GetKey(tufKey.ID()) + assert.Error(t, err) + + // GetKey works across multiple keystores + retrievedKey := cryptoService.GetKey(tufKey.ID()) + assert.NotNil(t, retrievedKey, + c.errorMsg("Could not find key ID %s", tufKey.ID())) +} + // asserts that getting key fails for a non-existent key func (c CryptoServiceTester) TestGetNonexistentKey(t *testing.T) { cryptoService := c.cryptoServiceFactory() @@ -81,6 +105,41 @@ func (c CryptoServiceTester) TestSignWithKey(t *testing.T) { c.errorMsg("verification failed for %s key type", c.keyAlgo)) } +// asserts that signing, if there are no matching keys, produces no signatures +func (c CryptoServiceTester) TestSignNoMatchingKeys(t *testing.T) { + cryptoService := c.cryptoServiceFactory() + content := []byte("this is a secret") + + privKey, err := trustmanager.GenerateECDSAKey(rand.Reader) + assert.NoError(t, err, c.errorMsg("error creating key")) + + // Test Sign with key that is not in the cryptoservice + signatures, err := cryptoService.Sign([]string{privKey.ID()}, content) + assert.NoError(t, err, c.errorMsg("signing failed")) + assert.Len(t, signatures, 0, c.errorMsg("wrong number of signatures")) +} + +// If there are multiple keystores, even if all of them have the same key, +// only one signature is returned. +func (c CryptoServiceTester) TestSignWhenMultipleKeystores(t *testing.T) { + cryptoService := c.cryptoServiceFactory() + cryptoService.keyStores = append(cryptoService.keyStores, + trustmanager.NewKeyMemoryStore(passphraseRetriever)) + content := []byte("this is a secret") + + privKey, err := trustmanager.GenerateECDSAKey(rand.Reader) + assert.NoError(t, err, c.errorMsg("error creating key")) + + for _, store := range cryptoService.keyStores { + err := store.AddKey(privKey.ID(), "root", privKey) + assert.NoError(t, err) + } + + signatures, err := cryptoService.Sign([]string{privKey.ID()}, content) + assert.NoError(t, err, c.errorMsg("signing failed")) + assert.Len(t, signatures, 1, c.errorMsg("wrong number of signatures")) +} + // asserts that removing key that exists succeeds func (c CryptoServiceTester) TestRemoveCreatedKey(t *testing.T) { cryptoService := c.cryptoServiceFactory() @@ -96,6 +155,76 @@ func (c CryptoServiceTester) TestRemoveCreatedKey(t *testing.T) { assert.Nil(t, retrievedKey, c.errorMsg("remove didn't work")) } +// asserts that removing key will remove it from all keystores +func (c CryptoServiceTester) TestRemoveFromMultipleKeystores(t *testing.T) { + cryptoService := c.cryptoServiceFactory() + cryptoService.keyStores = append(cryptoService.keyStores, + trustmanager.NewKeyMemoryStore(passphraseRetriever)) + + privKey, err := trustmanager.GenerateECDSAKey(rand.Reader) + assert.NoError(t, err, c.errorMsg("error creating key")) + + for _, store := range cryptoService.keyStores { + err := store.AddKey(privKey.ID(), "root", privKey) + assert.NoError(t, err) + } + + assert.NotNil(t, cryptoService.GetKey(privKey.ID())) + + // Remove removes it from all key stores + err = cryptoService.RemoveKey(privKey.ID()) + assert.NoError(t, err, c.errorMsg("could not remove key")) + + for _, store := range cryptoService.keyStores { + _, _, err := store.GetKey(privKey.ID()) + assert.Error(t, err) + } +} + +// asserts that listing keys works with multiple keystores, and that the +// same keys are deduplicated +func (c CryptoServiceTester) TestListFromMultipleKeystores(t *testing.T) { + cryptoService := c.cryptoServiceFactory() + cryptoService.keyStores = append(cryptoService.keyStores, + trustmanager.NewKeyMemoryStore(passphraseRetriever)) + + var expectedKeysIDs map[string]bool // just want to be able to index by key + + for i := 0; i < 3; i++ { + privKey, err := trustmanager.GenerateECDSAKey(rand.Reader) + assert.NoError(t, err, c.errorMsg("error creating key")) + expectedKeysIDs[privKey.ID()] = true + + // adds one different key to each keystore, and then one key to + // both keystores + for j, store := range cryptoService.keyStores { + if i == j || i == 2 { + store.AddKey(privKey.ID(), "root", privKey) + } + } + + // sanity check - each should have 2 + for _, store := range cryptoService.keyStores { + assert.Len(t, store.ListKeys(), 2) + } + } + + keyList := cryptoService.ListKeys("root") + assert.Len(t, keyList, 3) + for _, k := range keyList { + _, ok := expectedKeysIDs[k] + assert.True(t, ok) + } + + keyMap := cryptoService.ListAllKeys() + assert.Len(t, keyMap, 3) + for k, role := range keyMap { + _, ok := expectedKeysIDs[k] + assert.True(t, ok) + assert.Equal(t, "root", role) + } +} + // 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 From 43f2d40e43408850e1f17158495a4770ef202c9b Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 11 Nov 2015 22:07:49 -0800 Subject: [PATCH 4/6] Make our CI pick up trustmanager/yubikey again Signed-off-by: Ying Li Signed-off-by: David Lawrence Signed-off-by: Ying Li (github: endophage) --- trustmanager/yubikey/non_pkcs11.go | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 trustmanager/yubikey/non_pkcs11.go diff --git a/trustmanager/yubikey/non_pkcs11.go b/trustmanager/yubikey/non_pkcs11.go new file mode 100644 index 0000000000..77c07e6fe2 --- /dev/null +++ b/trustmanager/yubikey/non_pkcs11.go @@ -0,0 +1,9 @@ +// go list ./... and go test ./... will not pick up this package without this +// file, because go ? ./... does not honor build tags. + +// e.g. "go list -tags pkcs11 ./..." will not list this package if all the +// files in it have a build tag. + +// See https://github.com/golang/go/issues/11246 + +package yubikey From 87231d9a5d9fbeea3a3f5696229783a055aa2c06 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 11 Nov 2015 22:25:27 -0800 Subject: [PATCH 5/6] 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 ------ From 5d0893ef2ac4ef6e3f27a33ecc24ecf1ccc41a9e Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 11 Nov 2015 22:48:00 -0800 Subject: [PATCH 6/6] Oops, it'd be helpful if we actually ran the new CryptoService tests. Signed-off-by: Ying Li Signed-off-by: David Lawrence Signed-off-by: Ying Li (github: endophage) --- cryptoservice/crypto_service_test.go | 45 ++++++++++++++++++---------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/cryptoservice/crypto_service_test.go b/cryptoservice/crypto_service_test.go index dc03a94095..41b9f45cc4 100644 --- a/cryptoservice/crypto_service_test.go +++ b/cryptoservice/crypto_service_test.go @@ -3,6 +3,7 @@ package cryptoservice import ( "crypto/rand" "fmt" + "path/filepath" "runtime" "testing" @@ -62,10 +63,16 @@ func (c CryptoServiceTester) TestCreateAndGetWhenMultipleKeystores(t *testing.T) assert.NoError(t, err, c.errorMsg("error creating key")) // Only the first keystore should have the key - _, _, err = cryptoService.keyStores[0].GetKey(tufKey.ID()) - assert.NoError(t, err) - _, _, err = cryptoService.keyStores[1].GetKey(tufKey.ID()) - assert.Error(t, err) + keyPath := tufKey.ID() + if c.role != data.CanonicalRootRole && cryptoService.gun != "" { + keyPath = filepath.Join(cryptoService.gun, keyPath) + } + _, _, err = cryptoService.keyStores[0].GetKey(keyPath) + assert.NoError(t, err, c.errorMsg( + "First keystore does not have the key %s", keyPath)) + _, _, err = cryptoService.keyStores[1].GetKey(keyPath) + assert.Error(t, err, c.errorMsg( + "Second keystore has the key %s", keyPath)) // GetKey works across multiple keystores retrievedKey := cryptoService.GetKey(tufKey.ID()) @@ -188,7 +195,7 @@ func (c CryptoServiceTester) TestListFromMultipleKeystores(t *testing.T) { cryptoService.keyStores = append(cryptoService.keyStores, trustmanager.NewKeyMemoryStore(passphraseRetriever)) - var expectedKeysIDs map[string]bool // just want to be able to index by key + expectedKeysIDs := make(map[string]bool) // just want to be able to index by key for i := 0; i < 3; i++ { privKey, err := trustmanager.GenerateECDSAKey(rand.Reader) @@ -202,22 +209,25 @@ func (c CryptoServiceTester) TestListFromMultipleKeystores(t *testing.T) { store.AddKey(privKey.ID(), "root", privKey) } } - - // sanity check - each should have 2 - for _, store := range cryptoService.keyStores { - assert.Len(t, store.ListKeys(), 2) - } + } + // sanity check - each should have 2 + for _, store := range cryptoService.keyStores { + assert.Len(t, store.ListKeys(), 2, c.errorMsg("added keys wrong")) } keyList := cryptoService.ListKeys("root") - assert.Len(t, keyList, 3) + assert.Len(t, keyList, 4, + c.errorMsg( + "ListKeys should have 4 keys (not necesarily unique) but does not: %v", keyList)) for _, k := range keyList { _, ok := expectedKeysIDs[k] - assert.True(t, ok) + assert.True(t, ok, c.errorMsg("Unexpected key %s", k)) } keyMap := cryptoService.ListAllKeys() - assert.Len(t, keyMap, 3) + assert.Len(t, keyMap, 3, + c.errorMsg("ListAllKeys should have 3 unique keys but does not: %v", keyMap)) + for k, role := range keyMap { _, ok := expectedKeysIDs[k] assert.True(t, ok) @@ -241,7 +251,7 @@ func (c CryptoServiceTester) errorMsg(message string, args ...interface{}) strin } func testCryptoService(t *testing.T, gun string) { - getCryptoService := func() *CryptoService { + getTestingCryptoService := func() *CryptoService { return NewCryptoService( gun, trustmanager.NewKeyMemoryStore(passphraseRetriever)) } @@ -255,14 +265,19 @@ func testCryptoService(t *testing.T, gun string) { for _, role := range roles { for algo := range algoToSigType { cst := CryptoServiceTester{ - cryptoServiceFactory: getCryptoService, + cryptoServiceFactory: getTestingCryptoService, role: role, keyAlgo: algo, } cst.TestCreateAndGetKey(t) + cst.TestCreateAndGetWhenMultipleKeystores(t) cst.TestGetNonexistentKey(t) cst.TestSignWithKey(t) + cst.TestSignNoMatchingKeys(t) + cst.TestSignWhenMultipleKeystores(t) cst.TestRemoveCreatedKey(t) + cst.TestRemoveFromMultipleKeystores(t) + cst.TestListFromMultipleKeystores(t) } } }