tlsutil: don't use `server_name` config for RPC connections (#5394)

* server name only for outgoing https for checks

tlsutil/config.go

@@ -30,52 +30,58 @@ var TLSLookup = map[string]uint16{
 
 // Config used to create tls.Config
 type Config struct {
-	// VerifyIncoming is used to verify the authenticity of incoming connections.
+	// VerifyIncoming is used to verify the authenticity of incoming
-	// This means that TCP requests are forbidden, only allowing for TLS. TLS connections
+	// connections.  This means that TCP requests are forbidden, only
-	// must match a provided certificate authority. This can be used to force client auth.
+	// allowing for TLS. TLS connections must match a provided certificate
+	// authority. This can be used to force client auth.
 	VerifyIncoming bool
 
-	// VerifyIncomingRPC is used to verify the authenticity of incoming RPC connections.
+	// VerifyIncomingRPC is used to verify the authenticity of incoming RPC
-	// This means that TCP requests are forbidden, only allowing for TLS. TLS connections
+	// connections.  This means that TCP requests are forbidden, only
-	// must match a provided certificate authority. This can be used to force client auth.
+	// allowing for TLS. TLS connections must match a provided certificate
+	// authority. This can be used to force client auth.
 	VerifyIncomingRPC bool
 
-	// VerifyIncomingHTTPS is used to verify the authenticity of incoming HTTPS connections.
+	// VerifyIncomingHTTPS is used to verify the authenticity of incoming
-	// This means that TCP requests are forbidden, only allowing for TLS. TLS connections
+	// HTTPS connections.  This means that TCP requests are forbidden, only
-	// must match a provided certificate authority. This can be used to force client auth.
+	// allowing for TLS. TLS connections must match a provided certificate
+	// authority. This can be used to force client auth.
 	VerifyIncomingHTTPS bool
 
-	// VerifyOutgoing is used to verify the authenticity of outgoing connections.
+	// VerifyOutgoing is used to verify the authenticity of outgoing
-	// This means that TLS requests are used, and TCP requests are not made. TLS connections
+	// connections.  This means that TLS requests are used, and TCP
-	// must match a provided certificate authority. This is used to verify authenticity of
+	// requests are not made. TLS connections must match a provided
-	// server nodes.
+	// certificate authority. This is used to verify authenticity of server
+	// nodes.
 	VerifyOutgoing bool
 
-	// VerifyServerHostname is used to enable hostname verification of servers. This
+	// VerifyServerHostname is used to enable hostname verification of
-	// ensures that the certificate presented is valid for server.<datacenter>.<domain>.
+	// servers. This ensures that the certificate presented is valid for
-	// This prevents a compromised client from being restarted as a server, and then
+	// server.<datacenter>.<domain>.  This prevents a compromised client
-	// intercepting request traffic as well as being added as a raft peer. This should be
+	// from being restarted as a server, and then intercepting request
-	// enabled by default with VerifyOutgoing, but for legacy reasons we cannot break
+	// traffic as well as being added as a raft peer. This should be
-	// existing clients.
+	// enabled by default with VerifyOutgoing, but for legacy reasons we
+	// cannot break existing clients.
 	VerifyServerHostname bool
 
 	// UseTLS is used to enable outgoing TLS connections to Consul servers.
 	UseTLS bool
 
-	// CAFile is a path to a certificate authority file. This is used with VerifyIncoming
+	// CAFile is a path to a certificate authority file. This is used with
-	// or VerifyOutgoing to verify the TLS connection.
+	// VerifyIncoming or VerifyOutgoing to verify the TLS connection.
 	CAFile string
 
-	// CAPath is a path to a directory containing certificate authority files. This is used
+	// CAPath is a path to a directory containing certificate authority
-	// with VerifyIncoming or VerifyOutgoing to verify the TLS connection.
+	// files. This is used with VerifyIncoming or VerifyOutgoing to verify
+	// the TLS connection.
 	CAPath string
 
-	// CertFile is used to provide a TLS certificate that is used for serving TLS connections.
+	// CertFile is used to provide a TLS certificate that is used for
-	// Must be provided to serve TLS connections.
+	// serving TLS connections.  Must be provided to serve TLS connections.
 	CertFile string
 
-	// KeyFile is used to provide a TLS key that is used for serving TLS connections.
+	// KeyFile is used to provide a TLS key that is used for serving TLS
-	// Must be provided to serve TLS connections.
+	// connections.  Must be provided to serve TLS connections.
 	KeyFile string
 
 	// Node name is the name we use to advertise. Defaults to hostname.
@@ -94,8 +100,8 @@ type Config struct {
 	// CipherSuites is the list of TLS cipher suites to use.
 	CipherSuites []uint16
 
-	// PreferServerCipherSuites specifies whether to prefer the server's ciphersuite
+	// PreferServerCipherSuites specifies whether to prefer the server's
-	// over the client ciphersuites.
+	// ciphersuite over the client ciphersuites.
 	PreferServerCipherSuites bool
 
 	// EnableAgentTLSForChecks is used to apply the agent's TLS settings in
@@ -118,10 +124,6 @@ func (c *Config) KeyPair() (*tls.Certificate, error) {
 	return &cert, err
 }
 
-func (c *Config) skipBuiltinVerify() bool {
-	return c.VerifyServerHostname == false && c.ServerName == ""
-}
-
 // SpecificDC is used to invoke a static datacenter
 // and turns a DCWrapper into a Wrapper type.
 func SpecificDC(dc string, tlsWrap DCWrapper) Wrapper {
@@ -224,13 +226,7 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls.
 	}
 
 	tlsConfig := &tls.Config{
-		ClientAuth:         tls.NoClientCert,
+		InsecureSkipVerify: !c.base.VerifyServerHostname,
-		InsecureSkipVerify: c.base.skipBuiltinVerify(),
-		ServerName:         c.base.ServerName,
-	}
-
-	if tlsConfig.ServerName == "" {
-		tlsConfig.ServerName = c.base.NodeName
 	}
 
 	// Set the cipher suites
@@ -321,6 +317,11 @@ func (c *Configurator) OutgoingTLSConfigForCheck(id string) (*tls.Config, error)
 		return nil, err
 	}
 	tlsConfig.InsecureSkipVerify = c.getSkipVerifyForCheck(id)
+	tlsConfig.ServerName = c.base.ServerName
+	if tlsConfig.ServerName == "" {
+		tlsConfig.ServerName = c.base.NodeName
+	}
+
 	return tlsConfig, nil
 }
 

tlsutil/config_test.go

@@ -73,7 +73,7 @@ func TestConfigurator_OutgoingTLS_VerifyOutgoing(t *testing.T) {
 	require.True(t, tlsConf.InsecureSkipVerify)
 }
 
-func TestConfigurator_OutgoingTLS_ServerName(t *testing.T) {
+func TestConfigurator_OutgoingRPC_ServerName(t *testing.T) {
 	conf := &Config{
 		VerifyOutgoing: true,
 		CAFile:         "../test/ca/root.cer",
@@ -84,8 +84,8 @@ func TestConfigurator_OutgoingTLS_ServerName(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, tlsConf)
 	require.Len(t, tlsConf.RootCAs.Subjects(), 1)
-	require.Equal(t, tlsConf.ServerName, "consul.example.com")
+	require.Empty(t, tlsConf.ServerName)
-	require.False(t, tlsConf.InsecureSkipVerify)
+	require.True(t, tlsConf.InsecureSkipVerify)
 }
 
 func TestConfigurator_OutgoingTLS_VerifyHostname(t *testing.T) {
@@ -133,23 +133,6 @@ func TestConfigurator_OutgoingTLS_TLSMinVersion(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 startTLSServer(config *Config) (net.Conn, chan error) {
 	errc := make(chan error, 1)
 
@@ -501,7 +484,7 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) {
 		c := NewConfigurator(v.config)
 		tlsConf, err := c.commonTLSConfig(false)
 		require.NoError(t, err)
-		require.Equal(t, v.result, tlsConf.ServerName)
+		require.Empty(t, tlsConf.ServerName)
 	}
 }
 
@@ -615,6 +598,16 @@ func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) {
 	tlsConf, err = c.commonTLSConfig(true)
 	require.NoError(t, err)
 	require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth)
+
+	c.Update(&Config{VerifyServerHostname: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})
+	tlsConf, err = c.commonTLSConfig(false)
+	require.NoError(t, err)
+	require.True(t, tlsConf.InsecureSkipVerify)
+
+	c.Update(&Config{VerifyServerHostname: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})
+	tlsConf, err = c.commonTLSConfig(false)
+	require.NoError(t, err)
+	require.False(t, tlsConf.InsecureSkipVerify)
 }
 
 func TestConfigurator_IncomingRPCConfig(t *testing.T) {
@@ -708,4 +701,19 @@ func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) {
 	tlsConf, err = c.OutgoingTLSConfigForCheck("c1")
 	require.NoError(t, err)
 	require.False(t, tlsConf.InsecureSkipVerify)
+
+	c.Update(&Config{EnableAgentTLSForChecks: true, NodeName: "node", ServerName: "server"})
+	tlsConf, err = c.OutgoingTLSConfigForCheck("")
+	require.NoError(t, err)
+	require.Equal(t, "server", tlsConf.ServerName)
+
+	c.Update(&Config{EnableAgentTLSForChecks: true, ServerName: "server"})
+	tlsConf, err = c.OutgoingTLSConfigForCheck("")
+	require.NoError(t, err)
+	require.Equal(t, "server", tlsConf.ServerName)
+
+	c.Update(&Config{EnableAgentTLSForChecks: true, NodeName: "node"})
+	tlsConf, err = c.OutgoingTLSConfigForCheck("")
+	require.NoError(t, err)
+	require.Equal(t, "node", tlsConf.ServerName)
 }