Fixed bug parsing trust service info in notary server.

Previously, if it wasn't a remote service, the config parser was
still setting the key algorithm to be whatever was configured.
Now, if we are using a local trust service, the algorithm is always
ED25519.

Also broke the trust parsing into its own function for testing.

Signed-off-by: Ying Li <ying.li@docker.com>

cmd/notary-server/main.go

@@ -18,6 +18,7 @@ import (
 	_ "github.com/docker/distribution/registry/auth/token"
 	"github.com/docker/notary/server/storage"
 	"github.com/docker/notary/signer/client"
+	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/signed"
 	_ "github.com/go-sql-driver/mysql"
 	"golang.org/x/net/context"
@@ -95,6 +96,7 @@ func grpcTLS(configuration *viper.Viper) (*tls.Config, error) {
 	return tlsConfig, nil
 }
 
+// parses the configuration and returns a backing store for the TUF files
 func getStore(configuration *viper.Viper, allowedBackends []string) (
 	storage.MetaStore, error) {
 
@@ -117,6 +119,53 @@ func getStore(configuration *viper.Viper, allowedBackends []string) (
 	return store, nil
 }
 
+// parses the configuration and determines which trust service and key algorithm
+// to return
+func getTrustService(configuration *viper.Viper,
+	signerFactory func(string, string, *tls.Config) *client.NotarySigner,
+	healthRegister func(string, func() error, time.Duration)) (
+	signed.CryptoService, string, error) {
+
+	if configuration.GetString("trust_service.type") != "remote" {
+		logrus.Info("Using local signing service")
+		return signed.NewEd25519(), data.ED25519Key, nil
+	}
+
+	keyAlgo := configuration.GetString("trust_service.key_algorithm")
+	if keyAlgo != data.ED25519Key && keyAlgo != data.ECDSAKey && keyAlgo != data.RSAKey {
+		return nil, "", fmt.Errorf("invalid key algorithm configured: %s", keyAlgo)
+	}
+
+	clientTLS, err := grpcTLS(configuration)
+	if err != nil {
+		return nil, "", err
+	}
+
+	logrus.Info("Using remote signing service")
+
+	notarySigner := signerFactory(
+		configuration.GetString("trust_service.hostname"),
+		configuration.GetString("trust_service.port"),
+		clientTLS,
+	)
+
+	minute := 1 * time.Minute
+	healthRegister(
+		"Trust operational",
+		// If the trust service fails, the server is degraded but not
+		// exactly unheatlthy, so always return healthy and just log an
+		// error.
+		func() error {
+			err := notarySigner.CheckHealth(minute)
+			if err != nil {
+				logrus.Error("Trust not fully operational: ", err.Error())
+			}
+			return nil
+		},
+		minute)
+	return notarySigner, keyAlgo, nil
+}
+
 func main() {
 	flag.Usage = usage
 	flag.Parse()
@@ -159,44 +208,12 @@ func main() {
 	}
 	utils.SetUpBugsnag(bugsnagConf)
 
-	keyAlgo := mainViper.GetString("trust_service.key_algorithm")
+	trust, keyAlgo, err := getTrustService(mainViper,
-	if keyAlgo == "" {
+		client.NewNotarySigner, health.RegisterPeriodicFunc)
-		logrus.Fatal("no key algorithm configured.")
-		os.Exit(1)
-	}
-	ctx = context.WithValue(ctx, "keyAlgorithm", keyAlgo)
-
-	var trust signed.CryptoService
-	if mainViper.GetString("trust_service.type") == "remote" {
-		logrus.Info("Using remote signing service")
-		clientTLS, err := grpcTLS(mainViper)
 	if err != nil {
 		logrus.Fatal(err.Error())
 	}
-		notarySigner := client.NewNotarySigner(
+	ctx = context.WithValue(ctx, "keyAlgorithm", keyAlgo)
-			mainViper.GetString("trust_service.hostname"),
-			mainViper.GetString("trust_service.port"),
-			clientTLS,
-		)
-		trust = notarySigner
-		minute := 1 * time.Minute
-		health.RegisterPeriodicFunc(
-			"Trust operational",
-			// If the trust service fails, the server is degraded but not
-			// exactly unheatlthy, so always return healthy and just log an
-			// error.
-			func() error {
-				err := notarySigner.CheckHealth(minute)
-				if err != nil {
-					logrus.Error("Trust not fully operational: ", err.Error())
-				}
-				return nil
-			},
-			minute)
-	} else {
-		logrus.Info("Using local signing service")
-		trust = signed.NewEd25519()
-	}
 
 	store, err := getStore(mainViper, []string{utils.MySQLBackend, utils.MemoryBackend})
 	if err != nil {

cmd/notary-server/main_test.go

@@ -8,8 +8,12 @@ import (
 	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/docker/notary/server/storage"
+	"github.com/docker/notary/signer/client"
+	"github.com/docker/notary/tuf/data"
+	"github.com/docker/notary/tuf/signed"
 	"github.com/docker/notary/utils"
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
@@ -89,74 +93,190 @@ func TestGetAddrAndTLSConfigSkipClientTLS(t *testing.T) {
 	assert.Nil(t, tlsConf.ClientCAs)
 }
 
+// Various configurations that result in a local trust service being returned,
+// with an ED22519 algorithm no matter what was specified.  No health function
+// is configured.
+func TestGetLocalTrustService(t *testing.T) {
+	localConfigs := []string{
+		`{"trust_service": {"type": "bruhaha", "key_algorithm": "rsa"}}`,
+		`{"trust_service": {"type": "local", "key_algorithm": "rsa"}}`,
+		`{}`,
+	}
+	var registerCalled = 0
+	var fakeRegister = func(_ string, _ func() error, _ time.Duration) {
+		registerCalled++
+	}
+
+	for _, config := range localConfigs {
+		trust, algo, err := getTrustService(configure(config),
+			client.NewNotarySigner, fakeRegister)
+		assert.NoError(t, err)
+		assert.IsType(t, &signed.Ed25519{}, trust)
+		assert.Equal(t, data.ED25519Key, algo)
+	}
+	// no health function ever registered
+	assert.Equal(t, 0, registerCalled)
+}
+
+// Various configurations that result in a local trust service being returned,
+// with an ED22519 algorithm no matter what was specified.  No health function
+// is configured.
+func TestGetTrustServiceInvalidKeyAlgorithm(t *testing.T) {
+	configTemplate := `
+	{
+		"trust_service": {
+			"type": "remote",
+			"hostname": "blah",
+			"port": "1234",
+			"key_algorithm": "%s"
+		}
+	}`
+	badKeyAlgos := []string{
+		fmt.Sprintf(configTemplate, ""),
+		fmt.Sprintf(configTemplate, data.ECDSAx509Key),
+		fmt.Sprintf(configTemplate, "random"),
+	}
+	var registerCalled = 0
+	var fakeRegister = func(_ string, _ func() error, _ time.Duration) {
+		registerCalled++
+	}
+
+	for _, config := range badKeyAlgos {
+		_, _, err := getTrustService(configure(config),
+			client.NewNotarySigner, fakeRegister)
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "invalid key algorithm")
+	}
+	// no health function ever registered
+	assert.Equal(t, 0, registerCalled)
+}
+
+// template to be used for testing TLS parsing with the trust service
+var trustTLSConfigTemplate = `
+	{
+		"trust_service": {
+			"type": "remote",
+			"hostname": "notary-signer",
+			"port": "1234",
+			"key_algorithm": "ecdsa",
+			%s
+		}
+	}`
+
 // Client cert and Key either both have to be empty or both have to be
 // provided.
-func TestGrpcTLSMissingCertOrKey(t *testing.T) {
+func TestGetTrustServiceTLSMissingCertOrKey(t *testing.T) {
 	configs := []string{
 		fmt.Sprintf(`"tls_client_cert": "%s"`, Cert),
 		fmt.Sprintf(`"tls_client_key": "%s"`, Key),
 	}
-	for _, trustConfig := range configs {
+	var registerCalled = 0
-		jsonConfig := fmt.Sprintf(
+	var fakeRegister = func(_ string, _ func() error, _ time.Duration) {
-			`{"trust_service": {"hostname": "notary-signer", %s}}`,
+		registerCalled++
-			trustConfig)
+	}
+
+	for _, clientTLSConfig := range configs {
+		jsonConfig := fmt.Sprintf(trustTLSConfigTemplate, clientTLSConfig)
 		config := configure(jsonConfig)
-		tlsConfig, err := grpcTLS(config)
+		_, _, err := getTrustService(config, client.NewNotarySigner,
+			fakeRegister)
 		assert.Error(t, err)
-		assert.Nil(t, tlsConfig)
 		assert.True(t,
 			strings.Contains(err.Error(), "Partial TLS configuration found."))
 	}
+	// no health function ever registered
+	assert.Equal(t, 0, registerCalled)
 }
 
-// If no TLS configuration is provided for the host server, a tls config with
+// If no TLS configuration is provided for the host server, no TLS config will
-// the provided serverName is still returned.
+// be set for the trust service.
-func TestGrpcTLSNoConfig(t *testing.T) {
+func TestGetTrustServiceNoTLSConfig(t *testing.T) {
-	tlsConfig, err := grpcTLS(
+	config := `{
-		configure(`{"trust_service": {"hostname": "notary-signer"}}`))
+		"trust_service": {
+			"type": "remote",
+			"hostname": "notary-signer",
+			"port": "1234",
+			"key_algorithm": "ecdsa"
+		}
+	}`
+	var registerCalled = 0
+	var fakeRegister = func(_ string, _ func() error, _ time.Duration) {
+		registerCalled++
+	}
+
+	var tlsConfig *tls.Config
+	var fakeNewSigner = func(_, _ string, c *tls.Config) *client.NotarySigner {
+		tlsConfig = c
+		return &client.NotarySigner{}
+	}
+
+	trust, algo, err := getTrustService(configure(config),
+		fakeNewSigner, fakeRegister)
 	assert.NoError(t, err)
+	assert.IsType(t, &client.NotarySigner{}, trust)
+	assert.Equal(t, "ecdsa", algo)
 	assert.Equal(t, "notary-signer", tlsConfig.ServerName)
 	assert.Nil(t, tlsConfig.RootCAs)
 	assert.Nil(t, tlsConfig.Certificates)
+	// health function registered
+	assert.Equal(t, 1, registerCalled)
 }
 
-// The rest of the functionality of grpcTLS depends upon
+// The rest of the functionality of getTrustService depends upon
 // utils.ConfigureClientTLS, so this test just asserts that if successful,
-// the correct tls.Config is returned based on all the configuration parameters,
+// the correct tls.Config is returned based on all the configuration parameters
-// and that it gets the path relative to the config file
+func TestGetTrustServiceTLSSuccess(t *testing.T) {
-func TestGrpcTLSSuccess(t *testing.T) {
 	keypair, err := tls.LoadX509KeyPair(Cert, Key)
 	assert.NoError(t, err, "Unable to load cert and key for testing")
 
-	configJSON := `{
+	tlspart := fmt.Sprintf(`"tls_client_cert": "%s", "tls_client_key": "%s"`,
-		"trust_service": {
+		Cert, Key)
-            "hostname": "notary-server",
+
-            "tls_client_cert": "notary-server.crt",
+	var registerCalled = 0
-            "tls_client_key": "notary-server.key"
+	var fakeRegister = func(_ string, _ func() error, _ time.Duration) {
-        }
+		registerCalled++
-    }`
-	config := configure(configJSON)
-	config.SetConfigFile("../../fixtures/config.json")
-	tlsConfig, err := grpcTLS(config)
-	assert.NoError(t, err)
-	assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates)
 	}
 
-// The rest of the functionality of grpcTLS depends upon
+	var tlsConfig *tls.Config
+	var fakeNewSigner = func(_, _ string, c *tls.Config) *client.NotarySigner {
+		tlsConfig = c
+		return &client.NotarySigner{}
+	}
+
+	trust, algo, err := getTrustService(
+		configure(fmt.Sprintf(trustTLSConfigTemplate, tlspart)),
+		fakeNewSigner, fakeRegister)
+	assert.NoError(t, err)
+	assert.IsType(t, &client.NotarySigner{}, trust)
+	assert.Equal(t, "ecdsa", algo)
+	assert.Equal(t, "notary-signer", tlsConfig.ServerName)
+	assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates)
+	// health function registered
+	assert.Equal(t, 1, registerCalled)
+}
+
+// The rest of the functionality of getTrustService depends upon
 // utils.ConfigureServerTLS, so this test just asserts that if it fails,
-// the error is propogated.
+// the error is propagated.
-func TestGrpcTLSFailure(t *testing.T) {
+func TestGetTrustServiceTLSFailure(t *testing.T) {
-	config := fmt.Sprintf(
+	tlspart := fmt.Sprintf(`"tls_client_cert": "none", "tls_client_key": "%s"`,
-		`{"trust_service": {
-            "hostname": "notary-server",
-            "tls_client_cert": "no-exist",
-            "tls_client_key": "%s"}}`,
 		Key)
-	tlsConfig, err := grpcTLS(configure(config))
+
+	var registerCalled = 0
+	var fakeRegister = func(_ string, _ func() error, _ time.Duration) {
+		registerCalled++
+	}
+
+	_, _, err := getTrustService(
+		configure(fmt.Sprintf(trustTLSConfigTemplate, tlspart)),
+		client.NewNotarySigner, fakeRegister)
+
 	assert.Error(t, err)
-	assert.Nil(t, tlsConfig)
 	assert.True(t, strings.Contains(err.Error(),
 		"Unable to configure TLS to the trust service"))
+
+	// no health function ever registered
+	assert.Equal(t, 0, registerCalled)
 }
 
 // Just to ensure that errors are propogated