Merge pull request #56 from docker/last-stuff

Some more tests, one minor change

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

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

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)
 		}
 	}
 }

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/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

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(
+// Only add if we haven't seen the key already.  Return whether the key was
-	keyID, role string, privKey data.PrivateKey, backup bool) error {
+// 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) {

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
+}