Use configuration option structures to set up client TLS and server TLS.

Test for if client cert is passed without a client key and vice versa.
Fail in ConfigureClientTLS if only one of client cert/key is passed.
Lint fixes.

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

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())
 	}

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(
+	tlsConfig, err := utils.ConfigureClientTLS(&utils.ClientTLSOpts{
-		rootCAFile, "", insecureSkipVerify, "", "")
+		RootCAFile:         rootCAFile,
+		InsecureSkipVerify: insecureSkipVerify,
+	})
 	if err != nil {
 		logrus.Fatal("Unable to configure TLS: ", err.Error())
 	}

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(
+		tlsConfig, err := utils.ConfigureServerTLS(&utils.ServerTLSOpts{
-			tlsCertFile, tlsKeyFile, false, "")
+			ServerCertFile: tlsCertFile,
+			ServerKeyFile:  tlsKeyFile,
+		})
 		if err != nil {
 			return err
 		}

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(
+	tlsConfig, err := utils.ConfigureClientTLS(&utils.ClientTLSOpts{
-		tlscafile, hostname, false, "", "")
+		RootCAFile: tlscafile,
+		ServerName: hostname,
+	})
 	if err != nil {
 		logrus.Fatal("Unable to set up TLS: ", err)
 	}

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
 	}
 }
 

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.NoError(t, err)
-	assert.Equal(t, len(distinguishingBytes), nBytes,
+	assert.Len(t, distinguishingBytes, nBytes)
-		"didn't write all bytes to temporary file")
 
 	nBytes, err = out.Write(certBytes)
-	assert.NoError(t, err, "could not write cert to the temporary file")
+	assert.NoError(t, err)
-	assert.Equal(t, len(certBytes), nBytes,
+	assert.Len(t, certBytes, nBytes)
-		"didn't write all bytes to temporary file")
 
 	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.NoError(t, err)
-	assert.Equal(t, store.GetCertificates(), []*x509.Certificate{expectedCert},
+	assert.Equal(t, []*x509.Certificate{expectedCert}, store.GetCertificates())
-		"did not load certificate already in the directory")
 
 	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())
 }
 

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) {
+func ConfigureServerTLS(opts *ServerTLSOpts) (*tls.Config, error) {
-	keypair, err := tls.LoadX509KeyPair(serverCert, serverKey)
+	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
 }
 
-// ConfigureClientTLS generates a tls configuration for clients using the
+// ClientTLSOpts is a struct that contains options to pass to
-// provided parameters.
+// ConfigureClientTLS
-func ConfigureClientTLS(rootCA, serverName string, insecureSkipVerify bool, clientCert, clientKey string) (*tls.Config, error) {
+type ClientTLSOpts struct {
-	tlsConfig := &tls.Config{
+	RootCAFile         string
-		InsecureSkipVerify: insecureSkipVerify,
+	ServerName         string
-		MinVersion:         tls.VersionTLS12,
+	InsecureSkipVerify bool
-		CipherSuites:       clientCipherSuites,
+	ClientCertFile     string
-		ServerName:         serverName,
+	ClientKeyFile      string
 }
 
-	if rootCA != "" {
+// ConfigureClientTLS generates a tls configuration for clients using the
-		rootCert, err := trustmanager.LoadCertFromFile(rootCA)
+// provided parameters.
+func ConfigureClientTLS(opts *ClientTLSOpts) (*tls.Config, error) {
+	tlsConfig := &tls.Config{
+		InsecureSkipVerify: opts.InsecureSkipVerify,
+		MinVersion:         tls.VersionTLS12,
+		CipherSuites:       clientCipherSuites,
+		ServerName:         opts.ServerName,
+	}
+
+	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 != "" {
+	if opts.ClientCertFile != "" || opts.ClientKeyFile != "" {
-		keypair, err := tls.LoadX509KeyPair(clientCert, clientKey)
+		keypair, err := tls.LoadX509KeyPair(
+			opts.ClientCertFile, opts.ClientKeyFile)
 		if err != nil {
 			return nil, err
 		}

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,23 +162,27 @@ func TestConfigClientTLSValidRootCA(t *testing.T) {
 }
 
 // An error is returned if a root CA is provided but not valid
-func TestConfigClientTLSInValidRootCA(t *testing.T) {
+func TestConfigClientTLSInvalidRootCA(t *testing.T) {
-	tlsConfig, err := ConfigureClientTLS("not-a-file.crt", "", false, "", "")
+	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++ {
+		for _, invalid := range []string{"not-a-file", ""} {
 			files := []string{ServerCert, ServerKey}
-		files[i] = "not-a-file.crt"
+			files[i] = invalid
-		tlsConfig, err := ConfigureClientTLS("", "", false, files[0], files[1])
+			tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{
+				ClientCertFile: files[0], ClientKeyFile: files[1]})
 			assert.Error(t, err)
 			assert.Nil(t, tlsConfig)
 		}
 	}
+}
 
 // The certificate is set if the client cert and client key are provided and
 // valid.
@@ -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)