agent: honor when ca is set but verify_outgoing is disabled (#4826)

* honor when verify_outgoing is false but ca is set

* Remove code that exists only for tests

* fix formatting

tlsutil/config.go

@@ -121,6 +121,10 @@ func (c *Config) KeyPair() (*tls.Certificate, error) {
 	return &cert, err
 }
 
+func (c *Config) skipBuiltinVerify() bool {
+	return c.VerifyServerHostname == false && c.ServerName == ""
+}
+
 // OutgoingTLSConfig generates a TLS configuration for outgoing
 // requests. It will return a nil config if this configuration should
 // not use TLS for outgoing connections.
@@ -131,16 +135,8 @@ func (c *Config) OutgoingTLSConfig() (*tls.Config, error) {
 	// Create the tlsConfig
 	tlsConfig := &tls.Config{
 		RootCAs:            x509.NewCertPool(),
-		InsecureSkipVerify: true,
-	}
-	if c.ServerName != "" {
-		tlsConfig.ServerName = c.ServerName
-		tlsConfig.InsecureSkipVerify = false
-	}
-	if c.VerifyServerHostname {
-		// ServerName is filled in dynamically based on the target DC
-		tlsConfig.ServerName = "VerifyServerHostname"
-		tlsConfig.InsecureSkipVerify = false
+		InsecureSkipVerify: c.skipBuiltinVerify(),
+		ServerName:         c.ServerName,
 	}
 	if len(c.CipherSuites) != 0 {
 		tlsConfig.CipherSuites = c.CipherSuites
@@ -198,20 +194,15 @@ func (c *Config) OutgoingTLSWrapper() (DCWrapper, error) {
 		return nil, nil
 	}
 
-	// Strip the trailing '.' from the domain if any
-	domain := strings.TrimSuffix(c.Domain, ".")
-
-	wrapper := func(dc string, c net.Conn) (net.Conn, error) {
-		return WrapTLSClient(c, tlsConfig)
-	}
-
 	// Generate the wrapper based on hostname verification
-	if c.VerifyServerHostname {
-		wrapper = func(dc string, conn net.Conn) (net.Conn, error) {
-			conf := tlsConfig.Clone()
-			conf.ServerName = "server." + dc + "." + domain
-			return WrapTLSClient(conn, conf)
+	wrapper := func(dc string, conn net.Conn) (net.Conn, error) {
+		if c.VerifyServerHostname {
+			// Strip the trailing '.' from the domain if any
+			domain := strings.TrimSuffix(c.Domain, ".")
+			tlsConfig = tlsConfig.Clone()
+			tlsConfig.ServerName = "server." + dc + "." + domain
 		}
+		return c.wrapTLSClient(conn, tlsConfig)
 	}
 
 	return wrapper, nil
@@ -238,7 +229,7 @@ func SpecificDC(dc string, tlsWrap DCWrapper) Wrapper {
 // node names, we don't verify the certificate DNS names. Since go 1.3
 // no longer supports this mode of operation, we have to do it
 // manually.
-func WrapTLSClient(conn net.Conn, tlsConfig *tls.Config) (net.Conn, error) {
+func (c *Config) wrapTLSClient(conn net.Conn, tlsConfig *tls.Config) (net.Conn, error) {
 	var err error
 	var tlsConn *tls.Conn
 
@@ -250,6 +241,11 @@ func WrapTLSClient(conn net.Conn, tlsConfig *tls.Config) (net.Conn, error) {
 		return tlsConn, nil
 	}
 
+	// If verification is not turned on, don't do it.
+	if !c.VerifyOutgoing {
+		return tlsConn, nil
+	}
+
 	if err = tlsConn.Handshake(); err != nil {
 		tlsConn.Close()
 		return nil, err

tlsutil/config_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/yamux"
+	"github.com/stretchr/testify/require"
 )
 
 func TestConfig_AppendCA_None(t *testing.T) {
@@ -127,6 +128,23 @@ func TestConfig_OutgoingTLS_VerifyOutgoing(t *testing.T) {
 	}
 }
 
+func TestConfig_SkipBuiltinVerify(t *testing.T) {
+	type variant struct {
+		config Config
+		result bool
+	}
+	table := []variant{
+		variant{Config{ServerName: "", VerifyServerHostname: true}, false},
+		variant{Config{ServerName: "", VerifyServerHostname: false}, true},
+		variant{Config{ServerName: "consul", VerifyServerHostname: true}, false},
+		variant{Config{ServerName: "consul", VerifyServerHostname: false}, false},
+	}
+
+	for _, v := range table {
+		require.Equal(t, v.result, v.config.skipBuiltinVerify())
+	}
+}
+
 func TestConfig_OutgoingTLS_ServerName(t *testing.T) {
 	conf := &Config{
 		VerifyOutgoing: true,
@@ -167,9 +185,6 @@ func TestConfig_OutgoingTLS_VerifyHostname(t *testing.T) {
 	if len(tls.RootCAs.Subjects()) != 1 {
 		t.Fatalf("expect root cert")
 	}
-	if tls.ServerName != "VerifyServerHostname" {
-		t.Fatalf("expect server name")
-	}
 	if tls.InsecureSkipVerify {
 		t.Fatalf("should not skip built-in verification")
 	}
@@ -377,7 +392,7 @@ func TestConfig_wrapTLS_OK(t *testing.T) {
 		t.Fatalf("OutgoingTLSConfig err: %v", err)
 	}
 
-	tlsClient, err := WrapTLSClient(client, clientConfig)
+	tlsClient, err := config.wrapTLSClient(client, clientConfig)
 	if err != nil {
 		t.Fatalf("wrapTLS err: %v", err)
 	} else {
@@ -410,7 +425,7 @@ func TestConfig_wrapTLS_BadCert(t *testing.T) {
 		t.Fatalf("OutgoingTLSConfig err: %v", err)
 	}
 
-	tlsClient, err := WrapTLSClient(client, clientTLSConfig)
+	tlsClient, err := clientConfig.wrapTLSClient(client, clientTLSConfig)
 	if err == nil {
 		t.Fatalf("wrapTLS no err")
 	}