diff --git a/cmd/notary-signer/main.go b/cmd/notary-signer/main.go index 23f03b3f17..4a86dde90b 100644 --- a/cmd/notary-signer/main.go +++ b/cmd/notary-signer/main.go @@ -102,7 +102,10 @@ func main() { log.Fatalf("Certificate and key are mandatory") } - tlsConfig, err := utils.ConfigureServerTLS(certFile, keyFile, false, "") + tlsConfig, err := utils.ConfigureServerTLS(&utils.ServerTLSOpts{ + ServerCertFile: certFile, + ServerKeyFile: keyFile, + }) if err != nil { logrus.Fatalf("Unable to set up TLS: %s", err.Error()) } diff --git a/cmd/notary/tuf.go b/cmd/notary/tuf.go index 1a80c8464e..b3cd58e348 100644 --- a/cmd/notary/tuf.go +++ b/cmd/notary/tuf.go @@ -371,8 +371,10 @@ func getTransport(gun string, readOnly bool) http.RoundTripper { if mainViper.IsSet("remote_server.skipTLSVerify") { insecureSkipVerify = mainViper.GetBool("remote_server.skipTLSVerify") } - tlsConfig, err := utils.ConfigureClientTLS( - rootCAFile, "", insecureSkipVerify, "", "") + tlsConfig, err := utils.ConfigureClientTLS(&utils.ClientTLSOpts{ + RootCAFile: rootCAFile, + InsecureSkipVerify: insecureSkipVerify, + }) if err != nil { logrus.Fatal("Unable to configure TLS: ", err.Error()) } diff --git a/server/server.go b/server/server.go index e265ae3e88..cc8e29ded6 100644 --- a/server/server.go +++ b/server/server.go @@ -41,8 +41,10 @@ func Run(ctx context.Context, addr, tlsCertFile, tlsKeyFile string, trust signed } if tlsCertFile != "" && tlsKeyFile != "" { - tlsConfig, err := utils.ConfigureServerTLS( - tlsCertFile, tlsKeyFile, false, "") + tlsConfig, err := utils.ConfigureServerTLS(&utils.ServerTLSOpts{ + ServerCertFile: tlsCertFile, + ServerKeyFile: tlsKeyFile, + }) if err != nil { return err } diff --git a/signer/signer_trust.go b/signer/signer_trust.go index 1454597297..448a1767da 100644 --- a/signer/signer_trust.go +++ b/signer/signer_trust.go @@ -31,8 +31,10 @@ type NotarySigner struct { func NewNotarySigner(hostname string, port string, tlscafile string) *NotarySigner { var opts []grpc.DialOption netAddr := net.JoinHostPort(hostname, port) - tlsConfig, err := utils.ConfigureClientTLS( - tlscafile, hostname, false, "", "") + tlsConfig, err := utils.ConfigureClientTLS(&utils.ClientTLSOpts{ + RootCAFile: tlscafile, + ServerName: hostname, + }) if err != nil { logrus.Fatal("Unable to set up TLS: ", err) } diff --git a/signer/signer_trust_test.go b/signer/signer_trust_test.go index bcbba05bd6..4b5ccef40d 100644 --- a/signer/signer_trust_test.go +++ b/signer/signer_trust_test.go @@ -40,7 +40,7 @@ func stubHealthFunction(t *testing.T, status map[string]string, err error) rpcHe _, withDeadline := ctx.Deadline() assert.True(t, withDeadline) - return &pb.HealthStatus{status}, err + return &pb.HealthStatus{Status: status}, err } } diff --git a/trustmanager/x509filestore_test.go b/trustmanager/x509filestore_test.go index 95be916107..58decfc748 100644 --- a/trustmanager/x509filestore_test.go +++ b/trustmanager/x509filestore_test.go @@ -27,39 +27,36 @@ func TestNewX509FileStore(t *testing.T) { // not overwrite any of the. func TestNewX509FileStoreLoadsExistingCerts(t *testing.T) { tempDir, err := ioutil.TempDir("", "cert-test") - assert.NoError(t, err, "couldn't open temp directory") + assert.NoError(t, err) defer os.RemoveAll(tempDir) certBytes, err := ioutil.ReadFile("../fixtures/root-ca.crt") - assert.NoError(t, err, "couldn't read fixtures/root-ca.crt") + assert.NoError(t, err) out, err := os.Create(filepath.Join(tempDir, "root-ca.crt")) - assert.NoError(t, err, "couldn't create a file in the temp dir") + assert.NoError(t, err) // to distinguish it from the canonical format distinguishingBytes := []byte{'\n', '\n', '\n', '\n', '\n', '\n'} nBytes, err := out.Write(distinguishingBytes) - assert.NoError(t, err, "could not write newlines to the temporary file") - assert.Equal(t, len(distinguishingBytes), nBytes, - "didn't write all bytes to temporary file") + assert.NoError(t, err) + assert.Len(t, distinguishingBytes, nBytes) nBytes, err = out.Write(certBytes) - assert.NoError(t, err, "could not write cert to the temporary file") - assert.Equal(t, len(certBytes), nBytes, - "didn't write all bytes to temporary file") + assert.NoError(t, err) + assert.Len(t, certBytes, nBytes) err = out.Close() - assert.NoError(t, err, "could not close temporary file") + assert.NoError(t, err) store, err := NewX509FileStore(tempDir) - assert.NoError(t, err, "failed to create a new X509FileStore") + assert.NoError(t, err) expectedCert, err := LoadCertFromFile("../fixtures/root-ca.crt") - assert.NoError(t, err, "could not load root-ca.crt") - assert.Equal(t, store.GetCertificates(), []*x509.Certificate{expectedCert}, - "did not load certificate already in the directory") + assert.NoError(t, err) + assert.Equal(t, []*x509.Certificate{expectedCert}, store.GetCertificates()) outBytes, err := ioutil.ReadFile(filepath.Join(tempDir, "root-ca.crt")) - assert.NoError(t, err, "couldn't read temporary file") + assert.NoError(t, err) assert.Equal(t, distinguishingBytes, outBytes[:6], "original file overwritten") assert.Equal(t, certBytes, outBytes[6:], "original file overwritten") } @@ -131,11 +128,11 @@ func TestNewX509FileStoreEmpty(t *testing.T) { defer os.RemoveAll(tempDir) store, err := NewX509FileStore(tempDir) - assert.NoError(t, err, "failed to create a new X509FileStore: %v", store) + assert.NoError(t, err) assert.True(t, store.Empty()) err = store.AddCertFromFile("../fixtures/root-ca.crt") - assert.NoError(t, err, "failed to add certificate from file") + assert.NoError(t, err) assert.False(t, store.Empty()) } diff --git a/utils/tls_config.go b/utils/tls_config.go index 9e9347d087..0b1e70eac5 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -26,12 +26,22 @@ var serverCipherSuites = append(clientCipherSuites, []uint16{ tls.TLS_RSA_WITH_AES_128_CBC_SHA, }...) +// ServerTLSOpts generates a tls configuration for servers using the +// provided parameters. +type ServerTLSOpts struct { + ServerCertFile string + ServerKeyFile string + RequireClientAuth bool + ClientCADirectory string +} + // ConfigureServerTLS specifies a set of ciphersuites, the server cert and key, // and optionally client authentication. Note that a tls configuration is // constructed that either requires and verifies client authentication or // doesn't deal with client certs at all. Nothing in the middle. -func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir string) (*tls.Config, error) { - keypair, err := tls.LoadX509KeyPair(serverCert, serverKey) +func ConfigureServerTLS(opts *ServerTLSOpts) (*tls.Config, error) { + keypair, err := tls.LoadX509KeyPair( + opts.ServerCertFile, opts.ServerKeyFile) if err != nil { return nil, err } @@ -44,26 +54,26 @@ func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir Rand: rand.Reader, } - if clientAuth { + if opts.RequireClientAuth { tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert } - if caCertDir != "" { + if opts.ClientCADirectory != "" { // Check to see if the given directory exists - fi, err := os.Stat(caCertDir) + fi, err := os.Stat(opts.ClientCADirectory) if err != nil { return nil, err } if !fi.IsDir() { - return nil, fmt.Errorf("No such directory: %s", caCertDir) + return nil, fmt.Errorf("No such directory: %s", opts.ClientCADirectory) } - certStore, err := trustmanager.NewX509FileStore(caCertDir) + certStore, err := trustmanager.NewX509FileStore(opts.ClientCADirectory) if err != nil { return nil, err } if certStore.Empty() { - return nil, fmt.Errorf("No certificates in %s", caCertDir) + return nil, fmt.Errorf("No certificates in %s", opts.ClientCADirectory) } tlsConfig.ClientCAs = certStore.GetCertificatePool() } @@ -71,18 +81,28 @@ func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir return tlsConfig, nil } +// ClientTLSOpts is a struct that contains options to pass to +// ConfigureClientTLS +type ClientTLSOpts struct { + RootCAFile string + ServerName string + InsecureSkipVerify bool + ClientCertFile string + ClientKeyFile string +} + // ConfigureClientTLS generates a tls configuration for clients using the // provided parameters. -func ConfigureClientTLS(rootCA, serverName string, insecureSkipVerify bool, clientCert, clientKey string) (*tls.Config, error) { +func ConfigureClientTLS(opts *ClientTLSOpts) (*tls.Config, error) { tlsConfig := &tls.Config{ - InsecureSkipVerify: insecureSkipVerify, + InsecureSkipVerify: opts.InsecureSkipVerify, MinVersion: tls.VersionTLS12, CipherSuites: clientCipherSuites, - ServerName: serverName, + ServerName: opts.ServerName, } - if rootCA != "" { - rootCert, err := trustmanager.LoadCertFromFile(rootCA) + if opts.RootCAFile != "" { + rootCert, err := trustmanager.LoadCertFromFile(opts.RootCAFile) if err != nil { return nil, fmt.Errorf( "Could not load root ca file. %s", err.Error()) @@ -92,8 +112,9 @@ func ConfigureClientTLS(rootCA, serverName string, insecureSkipVerify bool, clie tlsConfig.RootCAs = rootPool } - if clientCert != "" && clientKey != "" { - keypair, err := tls.LoadX509KeyPair(clientCert, clientKey) + if opts.ClientCertFile != "" || opts.ClientKeyFile != "" { + keypair, err := tls.LoadX509KeyPair( + opts.ClientCertFile, opts.ClientKeyFile) if err != nil { return nil, err } diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go index 70821e2664..328ebc49d9 100644 --- a/utils/tls_config_test.go +++ b/utils/tls_config_test.go @@ -44,7 +44,12 @@ func TestConfigServerTLSFailsIfUnableToLoadCerts(t *testing.T) { files := []string{ServerCert, ServerKey, tempDir} files[i] = "not-real-file" - result, err := ConfigureServerTLS(files[0], files[1], true, files[2]) + result, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: files[0], + ServerKeyFile: files[1], + RequireClientAuth: true, + ClientCADirectory: files[2], + }) assert.Nil(t, result) assert.Error(t, err) } @@ -56,7 +61,10 @@ func TestConfigServerTLSServerCertsOnly(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, "") + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + }) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) @@ -70,7 +78,11 @@ func TestConfigServerTLSWithEmptyCACertDir(t *testing.T) { tempDir, err := ioutil.TempDir("/tmp", "cert-test") assert.NoError(t, err, "couldn't open temp directory") - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, tempDir) + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + ClientCADirectory: tempDir, + }) assert.Nil(t, tlsConfig) assert.Error(t, err) } @@ -83,7 +95,11 @@ func TestConfigServerTLSWithCACerts(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, tempDir) + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + ClientCADirectory: tempDir, + }) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) @@ -98,7 +114,11 @@ func TestConfigServerTLSClientAuthEnabled(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, true, "") + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + RequireClientAuth: true, + }) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) @@ -109,7 +129,8 @@ func TestConfigServerTLSClientAuthEnabled(t *testing.T) { // The skipVerify boolean gets set on the tls.Config's InsecureSkipBoolean func TestConfigClientTLSNoVerify(t *testing.T) { for _, skip := range []bool{true, false} { - tlsConfig, err := ConfigureClientTLS("", "", skip, "", "") + tlsConfig, err := ConfigureClientTLS( + &ClientTLSOpts{InsecureSkipVerify: skip}) assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) assert.Equal(t, skip, tlsConfig.InsecureSkipVerify) @@ -121,7 +142,7 @@ func TestConfigClientTLSNoVerify(t *testing.T) { // The skipVerify boolean gets set on the tls.Config's InsecureSkipBoolean func TestConfigClientServerName(t *testing.T) { for _, name := range []string{"", "myname"} { - tlsConfig, err := ConfigureClientTLS("", name, false, "", "") + tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{ServerName: name}) assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) assert.Equal(t, false, tlsConfig.InsecureSkipVerify) @@ -132,7 +153,7 @@ func TestConfigClientServerName(t *testing.T) { // The RootCA is set if it is provided and valid func TestConfigClientTLSValidRootCA(t *testing.T) { - tlsConfig, err := ConfigureClientTLS(RootCA, "", false, "", "") + tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{RootCAFile: RootCA}) assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) assert.Equal(t, false, tlsConfig.InsecureSkipVerify) @@ -141,21 +162,25 @@ func TestConfigClientTLSValidRootCA(t *testing.T) { } // An error is returned if a root CA is provided but not valid -func TestConfigClientTLSInValidRootCA(t *testing.T) { - tlsConfig, err := ConfigureClientTLS("not-a-file.crt", "", false, "", "") +func TestConfigClientTLSInvalidRootCA(t *testing.T) { + tlsConfig, err := ConfigureClientTLS( + &ClientTLSOpts{RootCAFile: "not-a-file"}) assert.Error(t, err) assert.Nil(t, tlsConfig) } // An error is returned if either the client cert or the key are provided -// but invalid. +// but invalid or blank. func TestConfigClientTLSClientCertOrKeyInvalid(t *testing.T) { for i := 0; i < 2; i++ { - files := []string{ServerCert, ServerKey} - files[i] = "not-a-file.crt" - tlsConfig, err := ConfigureClientTLS("", "", false, files[0], files[1]) - assert.Error(t, err) - assert.Nil(t, tlsConfig) + for _, invalid := range []string{"not-a-file", ""} { + files := []string{ServerCert, ServerKey} + files[i] = invalid + tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{ + ClientCertFile: files[0], ClientKeyFile: files[1]}) + assert.Error(t, err) + assert.Nil(t, tlsConfig) + } } } @@ -165,7 +190,8 @@ func TestConfigClientTLSValidClientCertAndKey(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureClientTLS("", "", false, ServerCert, ServerKey) + tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{ + ClientCertFile: ServerCert, ClientKeyFile: ServerKey}) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.Equal(t, false, tlsConfig.InsecureSkipVerify)