Per-DB signer tests

buildscripts/dbtests.sh

@@ -52,5 +52,5 @@ trap cleanup SIGINT SIGTERM EXIT
 docker-compose -p "${project}_${db}" -f "${composeFile}" run --no-deps -d ${dbContainerOpts}
 docker-compose -p "${project}_${db}" -f "${composeFile}" run --no-deps \
     -e NOTARY_BUILDTAGS="${db}db" -e DBURL="${DBURL}" \
-    -e PKGS="github.com/docker/notary/server/storage" \
+    -e PKGS="github.com/docker/notary/server/storage github.com/docker/notary/signer/keydbstore" \
     client bash -c "${clientCmd}"

cmd/notary-signer/config.go

@@ -135,7 +135,11 @@ func setUpCryptoservices(configuration *viper.Viper, allowedBackends []string) (
 		}
 		s := keydbstore.NewRethinkDBKeyStore(storeConfig.DBName, storeConfig.Username, storeConfig.Password, passphraseRetriever, defaultAlias, sess)
 		health.RegisterPeriodicFunc("DB operational", time.Minute, s.CheckHealth)
-		keyStore = s
+		if doBootstrap {
+			keyStore = s
+		} else {
+			keyStore = keydbstore.NewCachedKeyStore(s)
+		}
 	case notary.MySQLBackend, notary.SQLiteBackend:
 		storeConfig, err := utils.ParseSQLStorage(configuration)
 		if err != nil {
@@ -145,15 +149,15 @@ func setUpCryptoservices(configuration *viper.Viper, allowedBackends []string) (
 		if err != nil {
 			return nil, err
 		}
-		dbStore, err := keydbstore.NewKeyDBStore(
+		dbStore, err := keydbstore.NewSQLKeyDBStore(
 			passphraseRetriever, defaultAlias, storeConfig.Backend, storeConfig.Source)
 		if err != nil {
 			return nil, fmt.Errorf("failed to create a new keydbstore: %v", err)
 		}
 
 		health.RegisterPeriodicFunc(
 			"DB operational", time.Minute, dbStore.HealthCheck)
-		keyStore = dbStore
+		keyStore = keydbstore.NewCachedKeyStore(dbStore)
 	}
 
 	if doBootstrap {

server/storage/mysql_test.go

@@ -37,16 +37,19 @@ func init() {
 	}
 
 	sqldbSetup = func(t *testing.T) (*SQLStorage, func()) {
-		var cleanup = func() {
+		var cleanup1 = func() {
 			gormDB, err := gorm.Open("mysql", dburl)
 			require.NoError(t, err)
 
 			// drop all tables, if they exist
 			gormDB.DropTable(&TUFFile{})
 			gormDB.DropTable(&Key{})
 		}
-		cleanup()
+		cleanup1()
 		dbStore := SetupSQLDB(t, "mysql", dburl)
-		return dbStore, cleanup
+		return dbStore, func() {
+			dbStore.DB.Close()
+			cleanup1()
+		}
 	}
 }

server/storage/rethink_realdb_test.go

@@ -30,7 +30,7 @@ func rethinkSessionSetup(t *testing.T) (*gorethink.Session, string) {
 
 func rethinkDBSetup(t *testing.T) (RethinkDB, func()) {
 	session, _ := rethinkSessionSetup(t)
-	dbName := "testdb"
+	dbName := "servertestdb"
 	var cleanup = func() { gorethink.DBDrop(dbName).Exec(session) }
 
 	cleanup()
@@ -41,32 +41,36 @@ func rethinkDBSetup(t *testing.T) (RethinkDB, func()) {
 	return NewRethinkDBStorage(dbName, "", "", session), cleanup
 }
 
-func TestBootstrapSetsUsernamePassword(t *testing.T) {
+func TestRethinkBootstrapSetsUsernamePassword(t *testing.T) {
 	adminSession, source := rethinkSessionSetup(t)
-	dbname, username, password := "testdb", "testuser", "testpassword"
-	otherDB, otherUser, otherPass := "otherdb", "otheruser", "otherpassword"
+	dbname, username, password := "servertestdb", "testuser", "testpassword"
+	otherDB, otherUser, otherPass := "otherservertestdb", "otheruser", "otherpassword"
 
 	// create a separate user with access to a different DB
 	require.NoError(t, rethinkdb.SetupDB(adminSession, otherDB, nil))
+	defer gorethink.DBDrop(otherDB).Exec(adminSession)
 	require.NoError(t, rethinkdb.CreateAndGrantDBUser(adminSession, otherDB, otherUser, otherPass))
 
 	// Bootstrap
 	s := NewRethinkDBStorage(dbname, username, password, adminSession)
 	require.NoError(t, s.Bootstrap())
+	defer gorethink.DBDrop(dbname).Exec(adminSession)
 
 	// A user with an invalid password cannot connect to rethink DB at all
 	_, err := rethinkdb.UserConnection(tlsOpts, source, username, "wrongpass")
 	require.Error(t, err)
 
 	// the other user cannot access rethink
 	userSession, err := rethinkdb.UserConnection(tlsOpts, source, otherUser, otherPass)
+	require.NoError(t, err)
 	s = NewRethinkDBStorage(dbname, otherUser, otherPass, userSession)
 	_, _, err = s.GetCurrent("gun", data.CanonicalRootRole)
 	require.Error(t, err)
 	require.IsType(t, gorethink.RQLRuntimeError{}, err)
 
 	// our user can access the DB though
 	userSession, err = rethinkdb.UserConnection(tlsOpts, source, username, password)
+	require.NoError(t, err)
 	s = NewRethinkDBStorage(dbname, username, password, userSession)
 	_, _, err = s.GetCurrent("gun", data.CanonicalRootRole)
 	require.Error(t, err)

server/storage/sqlite_test.go

@@ -18,7 +18,10 @@ func sqlite3Setup(t *testing.T) (*SQLStorage, func()) {
 	require.NoError(t, err)
 
 	dbStore := SetupSQLDB(t, "sqlite3", tempBaseDir+"test_db")
-	var cleanup = func() { os.RemoveAll(tempBaseDir) }
+	var cleanup = func() {
+		dbStore.DB.Close()
+		os.RemoveAll(tempBaseDir)
+	}
 	return dbStore, cleanup
 }
 

signer/keydbstore/cachedkeystore.go

@@ -0,0 +1,76 @@
+package keydbstore
+
+import (
+	"sync"
+
+	"github.com/docker/notary/trustmanager"
+	"github.com/docker/notary/tuf/data"
+)
+
+// Note:  once trustmanager's file KeyStore has been flattened, this can be moved to trustmanager
+
+type cachedKeyStore struct {
+	trustmanager.KeyStore
+	lock       *sync.Mutex
+	cachedKeys map[string]*cachedKey
+}
+
+type cachedKey struct {
+	role string
+	key  data.PrivateKey
+}
+
+// NewCachedKeyStore returns a new trustmanager.KeyStore that includes caching
+func NewCachedKeyStore(baseStore trustmanager.KeyStore) trustmanager.KeyStore {
+	return &cachedKeyStore{
+		KeyStore:   baseStore,
+		lock:       &sync.Mutex{},
+		cachedKeys: make(map[string]*cachedKey),
+	}
+}
+
+// AddKey stores the contents of a private key. Both role and gun are ignored,
+// we always use Key IDs as name, and don't support aliases
+func (s *cachedKeyStore) AddKey(keyInfo trustmanager.KeyInfo, privKey data.PrivateKey) error {
+	if err := s.KeyStore.AddKey(keyInfo, privKey); err != nil {
+		return err
+	}
+
+	// Add the private key to our cache
+	s.lock.Lock()
+	defer s.lock.Unlock()
+	s.cachedKeys[privKey.ID()] = &cachedKey{
+		role: keyInfo.Role,
+		key:  privKey,
+	}
+
+	return nil
+}
+
+// GetKey returns the PrivateKey given a KeyID
+func (s *cachedKeyStore) GetKey(keyID string) (data.PrivateKey, string, error) {
+	cachedKeyEntry, ok := s.cachedKeys[keyID]
+	if ok {
+		return cachedKeyEntry.key, cachedKeyEntry.role, nil
+	}
+
+	// retrieve the key from the underlying store and put it into the cache
+	privKey, role, err := s.KeyStore.GetKey(keyID)
+	if err == nil {
+		s.lock.Lock()
+		defer s.lock.Unlock()
+		// Add the key to cache
+		s.cachedKeys[privKey.ID()] = &cachedKey{key: privKey, role: role}
+		return privKey, role, nil
+	}
+	return nil, "", err
+}
+
+// RemoveKey removes the key from the keyfilestore
+func (s *cachedKeyStore) RemoveKey(keyID string) error {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
+	delete(s.cachedKeys, keyID)
+	return s.KeyStore.RemoveKey(keyID)
+}

signer/keydbstore/cachedkeystore_test.go

@@ -0,0 +1,141 @@
+package keydbstore
+
+import (
+	"crypto/rand"
+	"fmt"
+	"testing"
+
+	"github.com/docker/notary/trustmanager"
+	"github.com/docker/notary/tuf/data"
+	"github.com/docker/notary/tuf/utils"
+	"github.com/stretchr/testify/require"
+)
+
+// gets a key from the DB store, and asserts that the key is the expected key
+func requireGetKeySuccess(t *testing.T, dbStore trustmanager.KeyStore, expectedRole string, expectedKey data.PrivateKey) {
+	retrKey, role, err := dbStore.GetKey(expectedKey.ID())
+	require.NoError(t, err)
+	require.Equal(t, expectedRole, role)
+	require.Equal(t, retrKey.Private(), expectedKey.Private())
+	require.Equal(t, retrKey.Algorithm(), expectedKey.Algorithm())
+	require.Equal(t, retrKey.Public(), expectedKey.Public())
+	require.Equal(t, retrKey.SignatureAlgorithm(), expectedKey.SignatureAlgorithm())
+}
+
+// closes the DB connection first so we can test that the successful get was
+// from the cache
+func requireGetKeySuccessFromCache(t *testing.T, cachedStore, underlyingStore trustmanager.KeyStore, expectedRole string, expectedKey data.PrivateKey) {
+	require.NoError(t, underlyingStore.RemoveKey(expectedKey.ID()))
+	requireGetKeySuccess(t, cachedStore, expectedRole, expectedKey)
+}
+
+func requireGetKeyFailure(t *testing.T, dbStore trustmanager.KeyStore, keyID string) {
+	_, _, err := dbStore.GetKey(keyID)
+	require.IsType(t, trustmanager.ErrKeyNotFound{}, err)
+}
+
+type unAddableKeyStore struct {
+	trustmanager.KeyStore
+}
+
+func (u unAddableKeyStore) AddKey(_ trustmanager.KeyInfo, _ data.PrivateKey) error {
+	return fmt.Errorf("Can't add to keystore!")
+}
+
+type unRemoveableKeyStore struct {
+	trustmanager.KeyStore
+	failToRemove bool
+}
+
+func (u unRemoveableKeyStore) RemoveKey(keyID string) error {
+	if u.failToRemove {
+		return fmt.Errorf("Can't remove from keystore!")
+	}
+	return u.KeyStore.RemoveKey(keyID)
+}
+
+// Getting a key, on succcess, populates the cache.
+func TestGetSuccessPopulatesCache(t *testing.T) {
+	underlying := trustmanager.NewKeyMemoryStore(constRetriever)
+	cached := NewCachedKeyStore(underlying)
+
+	testKey, err := utils.GenerateECDSAKey(rand.Reader)
+	require.NoError(t, err)
+
+	// nothing there yet
+	requireGetKeyFailure(t, cached, testKey.ID())
+
+	// Add key to underlying store only
+	err = underlying.AddKey(trustmanager.KeyInfo{Role: data.CanonicalTimestampRole, Gun: "gun"}, testKey)
+	require.NoError(t, err)
+
+	// getting for the first time is successful, and after that getting from cache should be too
+	requireGetKeySuccess(t, cached, data.CanonicalTimestampRole, testKey)
+	requireGetKeySuccessFromCache(t, cached, underlying, data.CanonicalTimestampRole, testKey)
+}
+
+// Creating a key, on succcess, populates the cache, but does not do so on failure
+func TestAddKeyPopulatesCacheIfSuccessful(t *testing.T) {
+	var underlying trustmanager.KeyStore
+	underlying = trustmanager.NewKeyMemoryStore(constRetriever)
+	cached := NewCachedKeyStore(underlying)
+
+	testKeys := make([]data.PrivateKey, 2)
+	for i := 0; i < 2; i++ {
+		privKey, err := utils.GenerateECDSAKey(rand.Reader)
+		require.NoError(t, err)
+		testKeys[i] = privKey
+	}
+
+	// Writing in the keystore succeeds
+	err := cached.AddKey(trustmanager.KeyInfo{Role: data.CanonicalTimestampRole, Gun: "gun"}, testKeys[0])
+	require.NoError(t, err)
+
+	// Now even if it's deleted from the underlying database, it's fine because it's cached
+	requireGetKeySuccessFromCache(t, cached, underlying, data.CanonicalTimestampRole, testKeys[0])
+
+	// Writing in the keystore fails
+	underlying = unAddableKeyStore{KeyStore: underlying}
+	cached = NewCachedKeyStore(underlying)
+	err = cached.AddKey(trustmanager.KeyInfo{Role: data.CanonicalTimestampRole, Gun: "gun"}, testKeys[1])
+	require.Error(t, err)
+
+	// And now it can't be found in either DB
+	requireGetKeyFailure(t, cached, testKeys[1].ID())
+}
+
+// Deleting a key, no matter whether we succeed in the underlying layer or not, evicts the cached key.
+func TestDeleteKeyRemovesKeyFromCache(t *testing.T) {
+	underlying := trustmanager.NewKeyMemoryStore(constRetriever)
+	cached := NewCachedKeyStore(underlying)
+
+	testKey, err := utils.GenerateECDSAKey(rand.Reader)
+	require.NoError(t, err)
+
+	// Write the key, which puts it in the cache
+	err = cached.AddKey(trustmanager.KeyInfo{Role: data.CanonicalTimestampRole, Gun: "gun"}, testKey)
+	require.NoError(t, err)
+
+	// Deleting removes the key from the cache and the underlying store
+	err = cached.RemoveKey(testKey.ID())
+	require.NoError(t, err)
+	requireGetKeyFailure(t, cached, testKey.ID())
+
+	// Now set up an underlying store where the key can't be deleted
+	failingUnderlying := unRemoveableKeyStore{KeyStore: underlying, failToRemove: true}
+	cached = NewCachedKeyStore(failingUnderlying)
+	err = cached.AddKey(trustmanager.KeyInfo{Role: data.CanonicalTimestampRole, Gun: "gun"}, testKey)
+	require.NoError(t, err)
+
+	// Deleting fails to remove the key from the underlying store
+	err = cached.RemoveKey(testKey.ID())
+	require.Error(t, err)
+	requireGetKeySuccess(t, failingUnderlying, data.CanonicalTimestampRole, testKey)
+
+	// now actually remove the key from the underlying store to test that it's gone from the cache
+	failingUnderlying.failToRemove = false
+	require.NoError(t, failingUnderlying.RemoveKey(testKey.ID()))
+
+	// and it's not in the cache
+	requireGetKeyFailure(t, cached, testKey.ID())
+}