From eb0895c5fb04547ecba7da9c5a7cd7982a7a7a77 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Tue, 5 Mar 2019 21:35:43 +0100 Subject: [PATCH] tlsutil: don't use `server_name` config for RPC connections (#5394) * server name only for outgoing https for checks --- tlsutil/config.go | 81 +++++++++++++++++++++--------------------- tlsutil/config_test.go | 50 +++++++++++++++----------- 2 files changed, 70 insertions(+), 61 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 758c353422..19d08aad3e 100644 --- a/tlsutil/config.go +++ b/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. - // This means that TCP requests are forbidden, only allowing for TLS. TLS connections - // must match a provided certificate authority. This can be used to force client auth. + // VerifyIncoming is used to verify the authenticity of incoming + // connections. This means that TCP requests are forbidden, only + // 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. - // This means that TCP requests are forbidden, only allowing for TLS. TLS connections - // must match a provided certificate authority. This can be used to force client auth. + // VerifyIncomingRPC is used to verify the authenticity of incoming RPC + // connections. This means that TCP requests are forbidden, only + // 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. - // This means that TCP requests are forbidden, only allowing for TLS. TLS connections - // must match a provided certificate authority. This can be used to force client auth. + // VerifyIncomingHTTPS is used to verify the authenticity of incoming + // HTTPS connections. This means that TCP requests are forbidden, only + // 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. - // This means that TLS requests are used, and TCP requests are not made. TLS connections - // must match a provided certificate authority. This is used to verify authenticity of - // server nodes. + // VerifyOutgoing is used to verify the authenticity of outgoing + // connections. This means that TLS requests are used, and TCP + // requests are not made. TLS connections must match a provided + // certificate authority. This is used to verify authenticity of server + // nodes. VerifyOutgoing bool - // VerifyServerHostname is used to enable hostname verification of servers. This - // ensures that the certificate presented is valid for server... - // This prevents a compromised client from being restarted as a server, and then - // intercepting request traffic as well as being added as a raft peer. This should be - // enabled by default with VerifyOutgoing, but for legacy reasons we cannot break - // existing clients. + // VerifyServerHostname is used to enable hostname verification of + // servers. This ensures that the certificate presented is valid for + // server... This prevents a compromised client + // from being restarted as a server, and then intercepting request + // traffic as well as being added as a raft peer. This should be + // 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 - // or VerifyOutgoing to verify the TLS connection. + // CAFile is a path to a certificate authority file. This is used with + // VerifyIncoming or VerifyOutgoing to verify the TLS connection. CAFile string - // CAPath is a path to a directory containing certificate authority files. This is used - // with VerifyIncoming or VerifyOutgoing to verify the TLS connection. + // CAPath is a path to a directory containing certificate authority + // 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. - // Must be provided to serve TLS connections. + // CertFile is used to provide a TLS certificate that is used for + // 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. - // Must be provided to serve TLS connections. + // KeyFile is used to provide a TLS key that is used for serving TLS + // 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 - // over the client ciphersuites. + // PreferServerCipherSuites specifies whether to prefer the server's + // 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.skipBuiltinVerify(), - ServerName: c.base.ServerName, - } - - if tlsConfig.ServerName == "" { - tlsConfig.ServerName = c.base.NodeName + InsecureSkipVerify: !c.base.VerifyServerHostname, } // 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 } diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 5945871f5d..de213c0def 100644 --- a/tlsutil/config_test.go +++ b/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.False(t, tlsConf.InsecureSkipVerify) + require.Empty(t, tlsConf.ServerName) + 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) }