From a920936c869e2bc92d7d538f9781c0e7364e96bd Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 24 Jun 2021 12:51:40 -0400 Subject: [PATCH 1/3] tlsutil: convert tests for OutgoingTLSConfigForCheck to a table In preparation for adding more test cases. --- tlsutil/config_test.go | 98 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 80 insertions(+), 18 deletions(-) diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 19b04d4a67..91d4f1aaee 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -11,6 +11,8 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/hashicorp/go-hclog" "github.com/hashicorp/yamux" "github.com/stretchr/testify/require" @@ -913,26 +915,86 @@ func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { require.Equal(t, []string{"h2", "http/1.1"}, c.IncomingHTTPSConfig().NextProtos) } -func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { - c := Configurator{base: &Config{ - TLSMinVersion: "tls12", - EnableAgentTLSForChecks: false, - }} - tlsConf := c.OutgoingTLSConfigForCheck(true, "") - require.Equal(t, true, tlsConf.InsecureSkipVerify) - require.Equal(t, uint16(0), tlsConf.MinVersion) +func TestConfigurator_OutgoingTLSConfigForCheck(t *testing.T) { + type testCase struct { + name string + conf func() (*Configurator, error) + skipVerify bool + serverName string + expected *tls.Config + } - c.base.EnableAgentTLSForChecks = true - c.base.ServerName = "servername" - tlsConf = c.OutgoingTLSConfigForCheck(true, "") - require.Equal(t, true, tlsConf.InsecureSkipVerify) - require.Equal(t, tlsLookup[c.base.TLSMinVersion], tlsConf.MinVersion) - require.Equal(t, c.base.ServerName, tlsConf.ServerName) + cmpTLSConfig := cmp.Options{ + cmpopts.IgnoreFields(tls.Config{}, "GetCertificate", "GetClientCertificate"), + cmpopts.IgnoreUnexported(tls.Config{}), + } - tlsConf = c.OutgoingTLSConfigForCheck(true, "servername2") - require.Equal(t, true, tlsConf.InsecureSkipVerify) - require.Equal(t, tlsLookup[c.base.TLSMinVersion], tlsConf.MinVersion) - require.Equal(t, "servername2", tlsConf.ServerName) + run := func(t *testing.T, tc testCase) { + configurator, err := tc.conf() + require.NoError(t, err) + c := configurator.OutgoingTLSConfigForCheck(tc.skipVerify, tc.serverName) + assertDeepEqual(t, tc.expected, c, cmpTLSConfig) + } + + testCases := []testCase{ + { + name: "default tls, skip verify, no server name", + conf: func() (*Configurator, error) { + return NewConfigurator(Config{ + TLSMinVersion: "tls12", + EnableAgentTLSForChecks: false, + }, nil) + }, + skipVerify: true, + expected: &tls.Config{InsecureSkipVerify: true}, + }, + { + name: "agent tls, skip verify, default server name", + conf: func() (*Configurator, error) { + return NewConfigurator(Config{ + TLSMinVersion: "tls12", + EnableAgentTLSForChecks: true, + ServerName: "servername", + }, nil) + }, + skipVerify: true, + expected: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + ServerName: "servername", + }, + }, + { + name: "agent tls, skip verify, with server name override", + conf: func() (*Configurator, error) { + return NewConfigurator(Config{ + TLSMinVersion: "tls12", + EnableAgentTLSForChecks: true, + ServerName: "servername", + }, nil) + }, + skipVerify: true, + serverName: "override", + expected: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + ServerName: "override", + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + run(t, tc) + }) + } +} + +func assertDeepEqual(t *testing.T, x, y interface{}, opts ...cmp.Option) { + t.Helper() + if diff := cmp.Diff(x, y, opts...); diff != "" { + t.Fatalf("assertion failed: values are not equal\n--- expected\n+++ actual\n%v", diff) + } } func TestConfigurator_OutgoingRPCConfig(t *testing.T) { From 486b97e2c9d76621d4f97077328676a41948026b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 24 Jun 2021 13:36:58 -0400 Subject: [PATCH 2/3] tlsutil: fix default server name for health checks Don't use the agent node name or agent server name when EnableAgentTLSForChecks=false. --- .changelog/10490.txt | 3 +++ tlsutil/config.go | 7 +++---- tlsutil/config_test.go | 28 ++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 .changelog/10490.txt diff --git a/.changelog/10490.txt b/.changelog/10490.txt new file mode 100644 index 0000000000..ec84cd951e --- /dev/null +++ b/.changelog/10490.txt @@ -0,0 +1,3 @@ +```release-note:bug +checks: fixes the default ServerName used with TLS health checks. +``` diff --git a/tlsutil/config.go b/tlsutil/config.go index 70d987f118..69b358fed0 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -720,10 +720,6 @@ func (c *Configurator) IncomingHTTPSConfig() *tls.Config { func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool, serverName string) *tls.Config { c.log("OutgoingTLSConfigForCheck") - if serverName == "" { - serverName = c.serverNameOrNodeName() - } - if !c.enableAgentTLSForChecks() { return &tls.Config{ InsecureSkipVerify: skipVerify, @@ -731,6 +727,9 @@ func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool, serverName str } } + if serverName == "" { + serverName = c.serverNameOrNodeName() + } config := c.commonTLSConfig(false) config.InsecureSkipVerify = skipVerify config.ServerName = serverName diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 91d4f1aaee..f63a02d051 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -948,6 +948,34 @@ func TestConfigurator_OutgoingTLSConfigForCheck(t *testing.T) { skipVerify: true, expected: &tls.Config{InsecureSkipVerify: true}, }, + { + name: "default tls, skip verify, default server name", + conf: func() (*Configurator, error) { + return NewConfigurator(Config{ + TLSMinVersion: "tls12", + EnableAgentTLSForChecks: false, + ServerName: "servername", + }, nil) + }, + skipVerify: true, + expected: &tls.Config{InsecureSkipVerify: true}, + }, + { + name: "default tls, skip verify, check server name", + conf: func() (*Configurator, error) { + return NewConfigurator(Config{ + TLSMinVersion: "tls12", + EnableAgentTLSForChecks: false, + ServerName: "servername", + }, nil) + }, + skipVerify: true, + serverName: "check-server-name", + expected: &tls.Config{ + InsecureSkipVerify: true, + ServerName: "check-server-name", + }, + }, { name: "agent tls, skip verify, default server name", conf: func() (*Configurator, error) { From d09027caf606532330b837114843b7e7fdea68f9 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 24 Jun 2021 13:43:15 -0400 Subject: [PATCH 3/3] tlsutils: more test cases for OutgoingTLSConfigForCheck --- tlsutil/config_test.go | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index f63a02d051..42116c985c 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -937,6 +937,13 @@ func TestConfigurator_OutgoingTLSConfigForCheck(t *testing.T) { } testCases := []testCase{ + { + name: "default tls", + conf: func() (*Configurator, error) { + return NewConfigurator(Config{}, nil) + }, + expected: &tls.Config{}, + }, { name: "default tls, skip verify, no server name", conf: func() (*Configurator, error) { @@ -955,6 +962,7 @@ func TestConfigurator_OutgoingTLSConfigForCheck(t *testing.T) { TLSMinVersion: "tls12", EnableAgentTLSForChecks: false, ServerName: "servername", + NodeName: "nodename", }, nil) }, skipVerify: true, @@ -977,19 +985,34 @@ func TestConfigurator_OutgoingTLSConfigForCheck(t *testing.T) { }, }, { - name: "agent tls, skip verify, default server name", + name: "agent tls, default server name", conf: func() (*Configurator, error) { return NewConfigurator(Config{ TLSMinVersion: "tls12", EnableAgentTLSForChecks: true, + NodeName: "nodename", ServerName: "servername", }, nil) }, + expected: &tls.Config{ + MinVersion: tls.VersionTLS12, + ServerName: "servername", + }, + }, + { + name: "agent tls, skip verify, node name for server name", + conf: func() (*Configurator, error) { + return NewConfigurator(Config{ + TLSMinVersion: "tls12", + EnableAgentTLSForChecks: true, + NodeName: "nodename", + }, nil) + }, skipVerify: true, expected: &tls.Config{ InsecureSkipVerify: true, MinVersion: tls.VersionTLS12, - ServerName: "servername", + ServerName: "nodename", }, }, {