From 37f8880291cc96a2334027c5a437f1aa0f246f98 Mon Sep 17 00:00:00 2001 From: Mike Morris Date: Thu, 18 Nov 2021 15:50:17 -0500 Subject: [PATCH] tlsutil: initial implementation of types/TLSVersion tlsutil: add test for parsing deprecated agent TLS version strings tlsutil: return TLSVersionInvalid with error --- tlsutil/config.go | 64 ++++++++++++++++++++++++++++-------------- tlsutil/config_test.go | 21 +++++++++++--- 2 files changed, 60 insertions(+), 25 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 4d0bd05259..6dde95658b 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -18,6 +18,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/hashicorp/consul/logging" + "github.com/hashicorp/consul/types" ) // ALPNWrapper is a function that is used to wrap a non-TLS connection and @@ -35,13 +36,35 @@ type DCWrapper func(dc string, conn net.Conn) (net.Conn, error) // a constant value. This is usually done by currying DCWrapper. type Wrapper func(conn net.Conn) (net.Conn, error) -// tlsLookup maps the tls_min_version configuration to the internal value -var tlsLookup = map[string]uint16{ - "": tls.VersionTLS10, // default in golang - "tls10": tls.VersionTLS10, - "tls11": tls.VersionTLS11, - "tls12": tls.VersionTLS12, - "tls13": tls.VersionTLS13, +// GoTLSVersions maps types.TLSVersion to the Go internal value +var GoTLSVersions = map[types.TLSVersion]uint16{ + types.TLSVersionAuto: tls.VersionTLS10, // default in golang + types.TLSv1_0: tls.VersionTLS10, + types.TLSv1_1: tls.VersionTLS11, + types.TLSv1_2: tls.VersionTLS12, + types.TLSv1_3: tls.VersionTLS13, +} + +// tlsLookup maps a TLS version configuration string to the internal value +func tlsLookup(tlsVersionString string) (types.TLSVersion, error) { + // Handle empty string case for unspecified config + if tlsVersionString == "" { + return types.TLSVersionAuto, nil + } + + if tlsVersion, ok := types.TLSVersions[tlsVersionString]; ok { + return tlsVersion, nil + } else { + // NOTE: This inner check for deprecated values should eventually be removed + if tlsVersion, ok := types.DeprecatedAgentTLSVersions[tlsVersionString]; ok { + // TODO: log warning about deprecated config + return tlsVersion, nil + } else { + // Only suggest non-deprecated values if configured value is invalid + versions := strings.Join(tlsVersions(), ", ") + return types.TLSVersionInvalid, fmt.Errorf("TLSMinVersion: value %s not supported, please specify one of [%s]", tlsVersionString, versions) + } + } } // Config used to create tls.Config @@ -131,10 +154,8 @@ type Config struct { func tlsVersions() []string { versions := []string{} - for v := range tlsLookup { - if v != "" { - versions = append(versions, v) - } + for v := range types.TLSVersions { + versions = append(versions, v) } sort.Strings(versions) return versions @@ -371,12 +392,11 @@ func newX509CertPool(groups ...[]string) (*x509.CertPool, error) { // validateConfig checks that config is valid and does not conflict with the pool // or cert. func validateConfig(config Config, pool *x509.CertPool, cert *tls.Certificate) error { - // Check if a minimum TLS version was set - if config.TLSMinVersion != "" { - if _, ok := tlsLookup[config.TLSMinVersion]; !ok { - versions := strings.Join(tlsVersions(), ", ") - return fmt.Errorf("TLSMinVersion: value %s not supported, please specify one of [%s]", config.TLSMinVersion, versions) - } + // Check if a minimum TLS version was set, + // returns TLSVersionAuto if config.TLSMinVersion is empty + _, err := tlsLookup(config.TLSMinVersion) + if err != nil { + return err } // Ensure we have a CA if VerifyOutgoing is set @@ -515,10 +535,12 @@ func (c *Configurator) commonTLSConfig(verifyIncoming bool) *tls.Config { tlsConfig.ClientCAs = c.caPool tlsConfig.RootCAs = c.caPool - // This is possible because tlsLookup also contains "" with golang's - // default (tls10). And because the initial check makes sure the - // version correctly matches. - tlsConfig.MinVersion = tlsLookup[c.base.TLSMinVersion] + // Error handling is not needed here because tlsLookup handles "" as + // TLSVersionAuto with GoTLSVersions mapping TLSVersionAuto to Go's + // default (tls10) and because the initial check in validateConfig makes + // sure the version is not invalid. + tlsVersion, _ := tlsLookup(c.base.TLSMinVersion) + tlsConfig.MinVersion = GoTLSVersions[tlsVersion] // Set ClientAuth if necessary if verifyIncoming { diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index cad4baac5e..3b03348b0a 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/require" "github.com/hashicorp/consul/sdk/testutil" + "github.com/hashicorp/consul/types" ) func startRPCTLSServer(config *Config) (net.Conn, chan error) { @@ -719,12 +720,24 @@ func TestConfigurator_CommonTLSConfigCAs(t *testing.T) { func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { c, err := NewConfigurator(Config{TLSMinVersion: ""}, nil) require.NoError(t, err) - require.Equal(t, c.commonTLSConfig(false).MinVersion, tlsLookup["tls10"]) + tlsVersion, _ := tlsLookup("TLSv1_0") + require.Equal(t, c.commonTLSConfig(false).MinVersion, GoTLSVersions[tlsVersion]) for _, version := range tlsVersions() { require.NoError(t, c.Update(Config{TLSMinVersion: version})) + tlsVersion, _ := tlsLookup(version) require.Equal(t, c.commonTLSConfig(false).MinVersion, - tlsLookup[version]) + GoTLSVersions[tlsVersion]) + } + + // NOTE: checks for deprecated TLS version string warnings, + // should be removed when removing support for these config values + for version := range types.DeprecatedAgentTLSVersions { + // TODO: check for warning log message? how? + require.NoError(t, c.Update(Config{TLSMinVersion: version})) + tlsVersion, _ := tlsLookup(version) + require.Equal(t, c.commonTLSConfig(false).MinVersion, + GoTLSVersions[tlsVersion]) } require.Error(t, c.Update(Config{TLSMinVersion: "tlsBOGUS"})) @@ -1439,7 +1452,7 @@ func certChain(t *testing.T, certs ...string) []*x509.Certificate { } func TestConfig_tlsVersions(t *testing.T) { - require.Equal(t, []string{"tls10", "tls11", "tls12", "tls13"}, tlsVersions()) - expected := "tls10, tls11, tls12, tls13" + require.Equal(t, []string{"TLS_AUTO", "TLSv1_0", "TLSv1_1", "TLSv1_2", "TLSv1_3"}, tlsVersions()) + expected := "TLS_AUTO, TLSv1_0, TLSv1_1, TLSv1_2, TLSv1_3" require.Equal(t, expected, strings.Join(tlsVersions(), ", ")) }