From d62ac788a3863a874f04ea1dfd31014e6e486bfc Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 16 Nov 2015 18:31:31 -0800 Subject: [PATCH] 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 --- cmd/notary-server/main.go | 89 ++++++++------- cmd/notary-server/main_test.go | 192 ++++++++++++++++++++++++++------- 2 files changed, 209 insertions(+), 72 deletions(-) diff --git a/cmd/notary-server/main.go b/cmd/notary-server/main.go index 34f8131398..50fac39bd5 100644 --- a/cmd/notary-server/main.go +++ b/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,45 +208,13 @@ func main() { } utils.SetUpBugsnag(bugsnagConf) - keyAlgo := mainViper.GetString("trust_service.key_algorithm") - if keyAlgo == "" { - logrus.Fatal("no key algorithm configured.") - os.Exit(1) + trust, keyAlgo, err := getTrustService(mainViper, + client.NewNotarySigner, health.RegisterPeriodicFunc) + if err != nil { + logrus.Fatal(err.Error()) } 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( - 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 { logrus.Fatal(err.Error()) diff --git a/cmd/notary-server/main_test.go b/cmd/notary-server/main_test.go index 82e0fad2ab..0330a82153 100644 --- a/cmd/notary-server/main_test.go +++ b/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 { - jsonConfig := fmt.Sprintf( - `{"trust_service": {"hostname": "notary-signer", %s}}`, - trustConfig) + var registerCalled = 0 + var fakeRegister = func(_ string, _ func() error, _ time.Duration) { + registerCalled++ + } + + 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 -// the provided serverName is still returned. -func TestGrpcTLSNoConfig(t *testing.T) { - tlsConfig, err := grpcTLS( - configure(`{"trust_service": {"hostname": "notary-signer"}}`)) +// If no TLS configuration is provided for the host server, no TLS config will +// be set for the trust service. +func TestGetTrustServiceNoTLSConfig(t *testing.T) { + config := `{ + "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, -// and that it gets the path relative to the config file -func TestGrpcTLSSuccess(t *testing.T) { +// the correct tls.Config is returned based on all the configuration parameters +func TestGetTrustServiceTLSSuccess(t *testing.T) { keypair, err := tls.LoadX509KeyPair(Cert, Key) assert.NoError(t, err, "Unable to load cert and key for testing") - configJSON := `{ - "trust_service": { - "hostname": "notary-server", - "tls_client_cert": "notary-server.crt", - "tls_client_key": "notary-server.key" - } - }` - config := configure(configJSON) - config.SetConfigFile("../../fixtures/config.json") - tlsConfig, err := grpcTLS(config) + tlspart := fmt.Sprintf(`"tls_client_cert": "%s", "tls_client_key": "%s"`, + Cert, Key) + + 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(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 grpcTLS depends upon +// The rest of the functionality of getTrustService depends upon // utils.ConfigureServerTLS, so this test just asserts that if it fails, -// the error is propogated. -func TestGrpcTLSFailure(t *testing.T) { - config := fmt.Sprintf( - `{"trust_service": { - "hostname": "notary-server", - "tls_client_cert": "no-exist", - "tls_client_key": "%s"}}`, +// the error is propagated. +func TestGetTrustServiceTLSFailure(t *testing.T) { + tlspart := fmt.Sprintf(`"tls_client_cert": "none", "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