Add unit tests, remove auth key, cleanup

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

cmd/notary-server/config.go

@@ -80,9 +80,12 @@ func getStore(configuration *viper.Viper, hRegister healthRegister) (
 		if err != nil {
 			return nil, err
 		}
-		s, err := storage.NewSQLStorage(storeConfig.Backend, storeConfig.Source)
-		store = *storage.NewTUFMetaStorage(s)
-		hRegister("DB operational", s.CheckHealth, time.Second*60)
+		var s *storage.SQLStorage
+		s, err = storage.NewSQLStorage(storeConfig.Backend, storeConfig.Source)
+		if err == nil {
+			store = *storage.NewTUFMetaStorage(s)
+			hRegister("DB operational", s.CheckHealth, time.Second*60)
+		}
 	case notary.RethinkDBBackend:
 		var sess *gorethink.Session
 		storeConfig, err := utils.ParseRethinkDBStorage(configuration)

cmd/notary-signer/main.go

@@ -93,39 +93,26 @@ func setUpCryptoservices(configuration *viper.Viper, allowedBackends []string) (
 		if err != nil {
 			return nil, err
 		}
-		sess, err = rethinkdb.Connection(storeConfig.CA, storeConfig.Source)
-		if err == nil {
-			defaultAlias := configuration.GetString("storage.default_alias")
-			if defaultAlias == "" {
-				// backwards compatibility - support this environment variable
-				defaultAlias = configuration.GetString(defaultAliasEnv)
-			}
-
-			if defaultAlias == "" {
-				return nil, fmt.Errorf("must provide a default alias for the key DB")
-			}
-			logrus.Debug("Default Alias: ", defaultAlias)
-			s := keydbstore.NewRethinkDBKeyStore(passphraseRetriever, defaultAlias, sess)
-			keyStore = s
-			health.RegisterPeriodicFunc("DB operational", s.CheckHealth, time.Minute)
+		defaultAlias, err := getDefaultAlias(configuration)
+		if err != nil {
+			return nil, err
 		}
-
+		sess, err = rethinkdb.Connection(storeConfig.CA, storeConfig.Source)
+		if err != nil {
+			return nil, err
+		}
+		s := keydbstore.NewRethinkDBKeyStore(passphraseRetriever, defaultAlias, sess)
+		health.RegisterPeriodicFunc("DB operational", s.CheckHealth, time.Minute)
+		keyStore = s
 	case notary.MySQLBackend, notary.SQLiteBackend:
 		storeConfig, err := utils.ParseSQLStorage(configuration)
 		if err != nil {
 			return nil, err
 		}
-		defaultAlias := configuration.GetString("storage.default_alias")
-		if defaultAlias == "" {
-			// backwards compatibility - support this environment variable
-			defaultAlias = configuration.GetString(defaultAliasEnv)
+		defaultAlias, err := getDefaultAlias(configuration)
+		if err != nil {
+			return nil, err
 		}
-
-		if defaultAlias == "" {
-			return nil, fmt.Errorf("must provide a default alias for the key DB")
-		}
-		logrus.Debug("Default Alias: ", defaultAlias)
-
 		dbStore, err := keydbstore.NewKeyDBStore(
 			passphraseRetriever, defaultAlias, storeConfig.Backend, storeConfig.Source)
 		if err != nil {
@@ -152,6 +139,20 @@ func setUpCryptoservices(configuration *viper.Viper, allowedBackends []string) (
 	return cryptoServices, nil
 }
 
+func getDefaultAlias(configuration *viper.Viper) (string, error) {
+	defaultAlias := configuration.GetString("storage.default_alias")
+	if defaultAlias == "" {
+		// backwards compatibility - support this environment variable
+		defaultAlias = configuration.GetString(defaultAliasEnv)
+	}
+
+	if defaultAlias == "" {
+		return "", fmt.Errorf("must provide a default alias for the key DB")
+	}
+	logrus.Debug("Default Alias: ", defaultAlias)
+	return defaultAlias, nil
+}
+
 // set up the GRPC server
 func setupGRPCServer(grpcAddr string, tlsConfig *tls.Config,
 	cryptoServices signer.CryptoServiceIndex) (*grpc.Server, net.Listener, error) {

cmd/notary-signer/main_test.go

@@ -113,6 +113,23 @@ func TestSetupCryptoServicesDBStoreNoDefaultAlias(t *testing.T) {
 	require.Contains(t, err.Error(), "must provide a default alias for the key DB")
 }
 
+// If a default alias is not provided to a rethinkdb backend, an error is returned.
+func TestSetupCryptoServicesRethinkDBStoreNoDefaultAlias(t *testing.T) {
+	_, err := setUpCryptoservices(
+		configure(fmt.Sprintf(
+			`{"storage": {
+				"backend": "%s",
+				"db_url": "host:port",
+				"tls_ca_file": "/tls/ca.pem",
+				"database": "rethinkdbtest"
+				}
+			}`,
+			notary.RethinkDBBackend)),
+		[]string{notary.RethinkDBBackend})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "must provide a default alias for the key DB")
+}
+
 // If a default alias *is* provided to a valid DB backend, a valid
 // CryptoService is returned.  (This depends on ParseStorage, which is tested
 // separately, so this doesn't test all the possible cases of storage

fixtures/server-config.rethink.json

@@ -18,7 +18,6 @@
 	},
 	"storage": {
 		"backend": "rethinkdb",
-		"auth_key": "devauthkey",
 		"db_url": "rdb-proxy.rdb",
 		"database": "notaryserver",
 		"tls_ca_file": "/tls/ca.pem"

fixtures/signer-config.rethink.json

@@ -11,7 +11,6 @@
 	},
 	"storage": {
 		"backend": "rethinkdb",
-		"auth_key": "devauthkey",
 		"db_url": "rdb-proxy.rdb",
 		"database": "notarysigner",
 		"tls_ca_file": "/tls/ca.pem"

server/storage/database_test.go

@@ -19,16 +19,7 @@ import (
 // SampleTUF returns a sample TUFFile with the given Version (ID will have
 // to be set independently)
 func SampleTUF(version int) TUFFile {
-	data := []byte("1")
-	checksum := sha256.Sum256(data)
-	hexChecksum := hex.EncodeToString(checksum[:])
-	return TUFFile{
-		Gun:     "testGUN",
-		Role:    "root",
-		Version: version,
-		Sha256:  hexChecksum,
-		Data:    []byte("1"),
-	}
+	return SampleCustomTUF(data.CanonicalRootRole, "testGUN", []byte("1"), version)
 }
 
 func SampleCustomTUF(role, gun string, data []byte, version int) TUFFile {

utils/configuration.go

@@ -26,9 +26,8 @@ type Storage struct {
 // RethinkDBStorage is configuration about a RethinkDB backend service
 type RethinkDBStorage struct {
 	Storage
-	CA      string
-	AuthKey string
-	DBName  string
+	CA     string
+	DBName string
 }
 
 // GetPathRelativeToConfig gets a configuration key which is a path, and if
@@ -117,9 +116,8 @@ func ParseRethinkDBStorage(configuration *viper.Viper) (*RethinkDBStorage, error
 			Backend: configuration.GetString("storage.backend"),
 			Source:  configuration.GetString("storage.db_url"),
 		},
-		CA:      configuration.GetString("storage.tls_ca_file"),
-		AuthKey: configuration.GetString("storage.auth_key"),
-		DBName:  configuration.GetString("storage.database"),
+		CA:     configuration.GetString("storage.tls_ca_file"),
+		DBName: configuration.GetString("storage.database"),
 	}
 
 	switch {
@@ -138,11 +136,6 @@ func ParseRethinkDBStorage(configuration *viper.Viper) (*RethinkDBStorage, error
 			"cowardly refusal to connect to %s without a CA cert",
 			store.Backend,
 		)
-	case store.AuthKey == "":
-		return nil, fmt.Errorf(
-			"cowardly refusal to connect to %s without an AuthKey",
-			store.Backend,
-		)
 	case store.DBName == "":
 		return nil, fmt.Errorf(
 			"%s requires a specific database to connect to",

utils/configuration_test.go

@@ -173,7 +173,7 @@ func TestParseInvalidStorageBackend(t *testing.T) {
 }
 
 // If there is no DB url for non-memory backends, an error is returned.
-func TestParseInvalidStorageNoDBSource(t *testing.T) {
+func TestParseInvalidSQLStorageNoDBSource(t *testing.T) {
 	invalids := []string{
 		`{"storage": {"backend": "%s"}}`,
 		`{"storage": {"backend": "%s", "db_url": ""}}`,
@@ -208,6 +208,67 @@ func TestParseSQLStorageDBStore(t *testing.T) {
 	require.Equal(t, expected, *store)
 }
 
+// ParseRethinkDBStorage will reject non rethink databases
+func TestParseRethinkStorageDBStoreInvalidBackend(t *testing.T) {
+	config := configure(`{
+		"storage": {
+			"backend": "mysql",
+			"db_url": "username:password@tcp(hostname:1234)/dbname",
+			"tls_ca_file": "/tls/ca.pem",
+			"database": "rethinkdbtest"
+		}
+	}`)
+
+	_, err := ParseRethinkDBStorage(config)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "not a supported RethinkDB backend")
+}
+
+// ParseRethinkDBStorage will require a db_url for rethink databases
+func TestParseRethinkStorageDBStoreEmptyDBUrl(t *testing.T) {
+	config := configure(`{
+		"storage": {
+			"backend": "rethinkdb",
+			"tls_ca_file": "/tls/ca.pem",
+			"database": "rethinkdbtest"
+		}
+	}`)
+
+	_, err := ParseRethinkDBStorage(config)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "must provide a non-empty host:port")
+}
+
+// ParseRethinkDBStorage will require a dbname for rethink databases
+func TestParseRethinkStorageDBStoreEmptyDBName(t *testing.T) {
+	config := configure(`{
+		"storage": {
+			"backend": "rethinkdb",
+			"db_url": "username:password@tcp(hostname:1234)/dbname",
+			"tls_ca_file": "/tls/ca.pem"
+		}
+	}`)
+
+	_, err := ParseRethinkDBStorage(config)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "requires a specific database to connect to")
+}
+
+// ParseRethinkDBStorage will require a CA cert for rethink databases
+func TestParseRethinkStorageDBStoreEmptyCA(t *testing.T) {
+	config := configure(`{
+		"storage": {
+			"backend": "rethinkdb",
+			"db_url": "username:password@tcp(hostname:1234)/dbname",
+			"database": "rethinkdbtest"
+		}
+	}`)
+
+	_, err := ParseRethinkDBStorage(config)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "cowardly refusal to connect to rethinkdb without a CA cert")
+}
+
 func TestParseSQLStorageWithEnvironmentVariables(t *testing.T) {
 	config := configure(`{
 		"storage": {