From 8d9d6c6a09c7fd22fefa2f35d45f30672fefe5b1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 21 Jun 2021 11:42:32 -0400 Subject: [PATCH 1/5] tlsutil: unexport two types These types are only used internally and should not be exported. Also remove some unnecessary function wrapping. --- tlsutil/config.go | 22 +++++++++------------- tlsutil/config_test.go | 11 ++++++----- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 037f5978f2..4342ca78c1 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -34,8 +34,8 @@ 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{ +// 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, @@ -43,9 +43,6 @@ var TLSLookup = map[string]uint16{ "tls13": tls.VersionTLS13, } -// TLSVersions has all the keys from the map above. -var TLSVersions = strings.Join(tlsVersions(), ", ") - // Config used to create tls.Config type Config struct { // VerifyIncoming is used to verify the authenticity of incoming @@ -133,7 +130,7 @@ type Config struct { func tlsVersions() []string { versions := []string{} - for v := range TLSLookup { + for v := range tlsLookup { if v != "" { versions = append(versions, v) } @@ -364,8 +361,9 @@ func pool(pems []string) (*x509.CertPool, error) { 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 { - return fmt.Errorf("TLSMinVersion: value %s not supported, please specify one of [%s]", config.TLSMinVersion, TLSVersions) + 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) } } @@ -517,10 +515,10 @@ 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 + // 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] + tlsConfig.MinVersion = tlsLookup[c.base.TLSMinVersion] // Set ClientAuth if necessary if verifyIncoming { @@ -794,9 +792,7 @@ func (c *Configurator) OutgoingALPNRPCWrapper() ALPNWrapper { return nil } - return func(dc, nodeName, alpnProto string, conn net.Conn) (net.Conn, error) { - return c.wrapALPNTLSClient(dc, nodeName, alpnProto, conn) - } + return c.wrapALPNTLSClient } // AutoEncryptCertNotAfter returns NotAfter from the auto_encrypt cert. In case diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 1282cda958..9081ece248 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -708,12 +708,12 @@ 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"]) + require.Equal(t, c.commonTLSConfig(false).MinVersion, tlsLookup["tls10"]) for _, version := range tlsVersions() { require.NoError(t, c.Update(Config{TLSMinVersion: version})) require.Equal(t, c.commonTLSConfig(false).MinVersion, - TLSLookup[version]) + tlsLookup[version]) } require.Error(t, c.Update(Config{TLSMinVersion: "tlsBOGUS"})) @@ -930,12 +930,12 @@ func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { 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, tlsLookup[c.base.TLSMinVersion], tlsConf.MinVersion) require.Equal(t, c.base.ServerName, tlsConf.ServerName) tlsConf = c.OutgoingTLSConfigForCheck(true, "servername2") require.Equal(t, true, tlsConf.InsecureSkipVerify) - require.Equal(t, TLSLookup[c.base.TLSMinVersion], tlsConf.MinVersion) + require.Equal(t, tlsLookup[c.base.TLSMinVersion], tlsConf.MinVersion) require.Equal(t, "servername2", tlsConf.ServerName) } @@ -1141,5 +1141,6 @@ func TestConfigurator_AutoEncrytCertExpired(t *testing.T) { func TestConfig_tlsVersions(t *testing.T) { require.Equal(t, []string{"tls10", "tls11", "tls12", "tls13"}, tlsVersions()) - require.Equal(t, strings.Join(tlsVersions(), ", "), TLSVersions) + expected := "tls10, tls11, tls12, tls13" + require.Equal(t, expected, strings.Join(tlsVersions(), ", ")) } From 08cd772626a5a99a1c4c6119bedae98513d122ae Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 21 Jun 2021 11:51:05 -0400 Subject: [PATCH 2/5] tlsutil: remove unused method Method was only used in tests, and an equivalent function alraedy exists. --- tlsutil/config.go | 5 ----- tlsutil/config_test.go | 36 ++++++++++++++++-------------------- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 4342ca78c1..10040735b2 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -139,11 +139,6 @@ func tlsVersions() []string { return versions } -// KeyPair is used to open and parse a certificate and key file -func (c *Config) KeyPair() (*tls.Certificate, error) { - return loadKeyPair(c.CertFile, c.KeyFile) -} - // SpecificDC is used to invoke a static datacenter // and turns a DCWrapper into a Wrapper type. func SpecificDC(dc string, tlsWrap DCWrapper) Wrapper { diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 9081ece248..17710bc14b 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -405,7 +405,7 @@ func TestConfig_ParseCiphers(t *testing.T) { require.Equal(t, []uint16{}, v) } -func TestConfigurator_loadKeyPair(t *testing.T) { +func TestLoadKeyPair(t *testing.T) { type variant struct { cert, key string shoulderr bool @@ -422,24 +422,20 @@ func TestConfigurator_loadKeyPair(t *testing.T) { false, false}, } for i, v := range variants { - info := fmt.Sprintf("case %d", i) - cert1, err1 := loadKeyPair(v.cert, v.key) - config := &Config{CertFile: v.cert, KeyFile: v.key} - cert2, err2 := config.KeyPair() - if v.shoulderr { - require.Error(t, err1, info) - require.Error(t, err2, info) - } else { - require.NoError(t, err1, info) - require.NoError(t, err2, info) - } - if v.isnil { - require.Nil(t, cert1, info) - require.Nil(t, cert2, info) - } else { - require.NotNil(t, cert1, info) - require.NotNil(t, cert2, info) - } + t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) { + cert, err := loadKeyPair(v.cert, v.key) + if v.shoulderr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + if v.isnil { + require.Nil(t, cert) + } else { + require.NotNil(t, cert) + } + }) } } @@ -518,7 +514,7 @@ func TestConfigurator_ErrorPropagation(t *testing.T) { var err3 error if !v.excludeCheck { - cert, err := v.config.KeyPair() + cert, err := loadKeyPair(v.config.CertFile, v.config.KeyFile) require.NoError(t, err, info) pems, err := LoadCAs(v.config.CAFile, v.config.CAPath) require.NoError(t, err, info) From a4432bb0b498ee6e42bfc1a0b95e1d9cb0baaedf Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 21 Jun 2021 12:20:39 -0400 Subject: [PATCH 3/5] tlsutil: un-ptr and add godoc to autoTLs struct the autoTLS field on Configurator is only set once. By making it a value receiver it should be allocated as a single block of memory along with Configurator. Also add godoc to document what it is used for. --- tlsutil/config.go | 13 +++++++------ tlsutil/config_test.go | 24 ++++++++++++------------ 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 10040735b2..9b93bcc104 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -150,6 +150,8 @@ func SpecificDC(dc string, tlsWrap DCWrapper) Wrapper { } } +// autoTLS stores configuration that is received from the auto-encrypt or +// auto-config features. type autoTLS struct { manualCAPems []string connectCAPems []string @@ -157,7 +159,7 @@ type autoTLS struct { verifyServerHostname bool } -func (a *autoTLS) caPems() []string { +func (a autoTLS) caPems() []string { return append(a.manualCAPems, a.connectCAPems...) } @@ -172,7 +174,7 @@ type Configurator struct { // lock synchronizes access to all fields on this struct except for logger and version. lock sync.RWMutex base *Config - autoTLS *autoTLS + autoTLS autoTLS manual *manual peerDatacenterUseTLS map[string]bool caPool *x509.CertPool @@ -197,7 +199,6 @@ func NewConfigurator(config Config, logger hclog.Logger) (*Configurator, error) c := &Configurator{ logger: logger.Named(logging.TLSUtil), manual: &manual{}, - autoTLS: &autoTLS{}, peerDatacenterUseTLS: map[string]bool{}, } err := c.Update(config) @@ -274,7 +275,7 @@ func (c *Configurator) UpdateAutoTLSCA(connectCAPems []string) error { return nil } -// UpdateAutoTLSCert +// UpdateAutoTLSCert receives the updated Auto-Encrypt certificate. func (c *Configurator) UpdateAutoTLSCert(pub, priv string) error { cert, err := tls.X509KeyPair([]byte(pub), []byte(priv)) if err != nil { @@ -290,8 +291,8 @@ func (c *Configurator) UpdateAutoTLSCert(pub, priv string) error { return nil } -// UpdateAutoTLS sets everything under autoEncrypt. This is being called on the -// client when it received its cert from AutoEncrypt/AutoConfig endpoints. +// UpdateAutoTLS receives updates from Auto-Config, only expected to be called on +// client agents. func (c *Configurator) UpdateAutoTLS(manualCAPems, connectCAPems []string, pub, priv string, verifyServerHostname bool) error { cert, err := tls.X509KeyPair([]byte(pub), []byte(priv)) if err != nil { diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 17710bc14b..e681036dba 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -506,7 +506,7 @@ func TestConfigurator_ErrorPropagation(t *testing.T) { variants = append(variants, variant{Config{TLSMinVersion: v}, false, false}) } - c := Configurator{autoTLS: &autoTLS{}, manual: &manual{}} + c := Configurator{manual: &manual{}} for i, v := range variants { info := fmt.Sprintf("case %d, config: %+v", i, v.config) _, err1 := NewConfigurator(v.config, nil) @@ -716,7 +716,7 @@ func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { } func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { - c := Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := Configurator{base: &Config{}} type variant struct { verify bool expected tls.ClientAuthType @@ -731,7 +731,7 @@ func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { } func TestConfigurator_OutgoingRPCTLSDisabled(t *testing.T) { - c := Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := Configurator{base: &Config{}} type variant struct { verify bool autoEncryptTLS bool @@ -909,7 +909,7 @@ func TestConfigurator_IncomingALPNRPCConfig(t *testing.T) { } func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { - c := Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := Configurator{base: &Config{}} require.Equal(t, []string{"h2", "http/1.1"}, c.IncomingHTTPSConfig().NextProtos) } @@ -917,7 +917,7 @@ func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { c := Configurator{base: &Config{ TLSMinVersion: "tls12", EnableAgentTLSForChecks: false, - }, autoTLS: &autoTLS{}} + }} tlsConf := c.OutgoingTLSConfigForCheck(true, "") require.Equal(t, true, tlsConf.InsecureSkipVerify) require.Equal(t, uint16(0), tlsConf.MinVersion) @@ -936,7 +936,7 @@ func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { } func TestConfigurator_OutgoingRPCConfig(t *testing.T) { - c := &Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := &Configurator{base: &Config{}} require.Nil(t, c.OutgoingRPCConfig()) c, err := NewConfigurator(Config{ @@ -954,7 +954,7 @@ func TestConfigurator_OutgoingRPCConfig(t *testing.T) { } func TestConfigurator_OutgoingALPNRPCConfig(t *testing.T) { - c := &Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := &Configurator{base: &Config{}} require.Nil(t, c.OutgoingALPNRPCConfig()) c, err := NewConfigurator(Config{ @@ -974,7 +974,7 @@ func TestConfigurator_OutgoingALPNRPCConfig(t *testing.T) { } func TestConfigurator_OutgoingRPCWrapper(t *testing.T) { - c := &Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := &Configurator{base: &Config{}} wrapper := c.OutgoingRPCWrapper() require.NotNil(t, wrapper) conn := &net.TCPConn{} @@ -996,7 +996,7 @@ func TestConfigurator_OutgoingRPCWrapper(t *testing.T) { } func TestConfigurator_OutgoingALPNRPCWrapper(t *testing.T) { - c := &Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := &Configurator{base: &Config{}} wrapper := c.OutgoingRPCWrapper() require.NotNil(t, wrapper) conn := &net.TCPConn{} @@ -1071,7 +1071,7 @@ func TestConfigurator_ServerNameOrNodeName(t *testing.T) { } func TestConfigurator_VerifyOutgoing(t *testing.T) { - c := Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := Configurator{base: &Config{}} type variant struct { verify bool autoEncryptTLS bool @@ -1104,7 +1104,7 @@ func TestConfigurator_Domain(t *testing.T) { } func TestConfigurator_VerifyServerHostname(t *testing.T) { - c := Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := Configurator{base: &Config{}} require.False(t, c.VerifyServerHostname()) c.base.VerifyServerHostname = true @@ -1121,7 +1121,7 @@ func TestConfigurator_VerifyServerHostname(t *testing.T) { } func TestConfigurator_AutoEncrytCertExpired(t *testing.T) { - c := Configurator{base: &Config{}, autoTLS: &autoTLS{}} + c := Configurator{base: &Config{}} require.True(t, c.AutoEncryptCertExpired()) cert, err := loadKeyPair("../test/key/something_expired.cer", "../test/key/something_expired.key") From 6289b682478068d7a627b6c28b3d8e836822803c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 21 Jun 2021 12:29:07 -0400 Subject: [PATCH 4/5] tlsutil: document Configurator and some of its fields --- tlsutil/config.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 9b93bcc104..d9b20e2ab1 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -168,16 +168,20 @@ type manual struct { cert *tls.Certificate } -// Configurator holds a Config and is responsible for generating all the -// *tls.Config necessary for Consul. Except the one in the api package. +// Configurator provides tls.Config and net.Dial wrappers to enable TLS for +// clients and servers, for both HTTPS and RPC requests. +// Configurator receives an initial TLS configuration from agent configuration, +// and receives updates from config reloads, auto-encrypt, and auto-config. type Configurator struct { // lock synchronizes access to all fields on this struct except for logger and version. - lock sync.RWMutex - base *Config - autoTLS autoTLS - manual *manual + lock sync.RWMutex + base *Config + autoTLS autoTLS + manual *manual + caPool *x509.CertPool + // peerDatacenterUseTLS is a map of DC name to a bool indicating if the DC + // uses TLS for RPC requests. peerDatacenterUseTLS map[string]bool - caPool *x509.CertPool // logger is not protected by a lock. It must never be changed after // Configurator is created. From 1ba5acb2844f434fd4cc7b95d9cac921cbf71073 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 21 Jun 2021 12:36:52 -0400 Subject: [PATCH 5/5] tlsutil: un-ptr and document the manual struct --- tlsutil/config.go | 5 +++-- tlsutil/config_test.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index d9b20e2ab1..8d66cf9750 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -163,6 +163,8 @@ func (a autoTLS) caPems() []string { return append(a.manualCAPems, a.connectCAPems...) } +// manual stores the TLS CA and cert received from Configurator.Update which +// generally comes from the agent configuration. type manual struct { caPems []string cert *tls.Certificate @@ -177,7 +179,7 @@ type Configurator struct { lock sync.RWMutex base *Config autoTLS autoTLS - manual *manual + manual manual caPool *x509.CertPool // peerDatacenterUseTLS is a map of DC name to a bool indicating if the DC // uses TLS for RPC requests. @@ -202,7 +204,6 @@ func NewConfigurator(config Config, logger hclog.Logger) (*Configurator, error) c := &Configurator{ logger: logger.Named(logging.TLSUtil), - manual: &manual{}, peerDatacenterUseTLS: map[string]bool{}, } err := c.Update(config) diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index e681036dba..0811c00ac8 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -506,7 +506,7 @@ func TestConfigurator_ErrorPropagation(t *testing.T) { variants = append(variants, variant{Config{TLSMinVersion: v}, false, false}) } - c := Configurator{manual: &manual{}} + c := Configurator{} for i, v := range variants { info := fmt.Sprintf("case %d, config: %+v", i, v.config) _, err1 := NewConfigurator(v.config, nil)