diff --git a/cryptoservice/crypto_service_test.go b/cryptoservice/crypto_service_test.go index 832197a542..41b9f45cc4 100644 --- a/cryptoservice/crypto_service_test.go +++ b/cryptoservice/crypto_service_test.go @@ -1,7 +1,9 @@ package cryptoservice import ( + "crypto/rand" "fmt" + "path/filepath" "runtime" "testing" @@ -49,6 +51,35 @@ 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 + 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()) + 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 +112,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 +162,79 @@ 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)) + + 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) + 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, c.errorMsg("added keys wrong")) + } + + keyList := cryptoService.ListKeys("root") + 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, c.errorMsg("Unexpected key %s", k)) + } + + keyMap := cryptoService.ListAllKeys() + 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) + 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 @@ -112,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)) } @@ -126,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) } } } 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/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 diff --git a/trustmanager/yubikey/yubikeystore.go b/trustmanager/yubikey/yubikeystore.go index 5df3f430ed..3eacbf215d 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,54 +631,64 @@ 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) + added, err := s.addKey(keyID, role, privKey) + if err != nil { + return err + } + 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, backup bool) 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) - 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, 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 @@ -763,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, false) + _, 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 672a61eb35..621b8dd11e 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,76 @@ 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) +} + +// 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() { @@ -790,6 +861,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 TestYubiRetrySignUntilFail(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: sigAttempts + 1, + } + + 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, sigAttempts, 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 +1126,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 +}