From 0a14a3e17c46d752d898ceefa6eab495323e43bf Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 17 Jun 2021 15:41:01 -0400 Subject: [PATCH 1/5] inline assignment --- agent/config/builder.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index ed34f19a00..a35dc856ff 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -669,7 +669,6 @@ func (b *builder) Build() (rt RuntimeConfig, err error) { // autoEncrypt and autoConfig implicitly turns on connect which is why // they need to be above other settings that rely on connect. - autoEncryptTLS := boolVal(c.AutoEncrypt.TLS) autoEncryptDNSSAN := []string{} for _, d := range c.AutoEncrypt.DNSSAN { autoEncryptDNSSAN = append(autoEncryptDNSSAN, d) @@ -685,13 +684,8 @@ func (b *builder) Build() (rt RuntimeConfig, err error) { } autoEncryptAllowTLS := boolVal(c.AutoEncrypt.AllowTLS) - - if autoEncryptAllowTLS { - connectEnabled = true - } - autoConfig := b.autoConfigVal(c.AutoConfig) - if autoConfig.Enabled { + if autoEncryptAllowTLS || autoConfig.Enabled { connectEnabled = true } @@ -984,7 +978,7 @@ func (b *builder) Build() (rt RuntimeConfig, err error) { Checks: checks, ClientAddrs: clientAddrs, ConfigEntryBootstrap: configEntries, - AutoEncryptTLS: autoEncryptTLS, + AutoEncryptTLS: boolVal(c.AutoEncrypt.TLS), AutoEncryptDNSSAN: autoEncryptDNSSAN, AutoEncryptIPSAN: autoEncryptIPSAN, AutoEncryptAllowTLS: autoEncryptAllowTLS, From 6f5198431328e85cacd880b61234b9662d5a9514 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 17 Jun 2021 18:48:44 -0400 Subject: [PATCH 2/5] tlsutil: un-embed the RWMutex Embedded structs make code harder to navidate because an IDE can not show all uses of the methods of that field separate from other uses. Generally embedding of structs should only be used to satisfy an interface, and in this case the Configurator type does not need to implement the RWMutex interface. --- tlsutil/config.go | 99 ++++++++++++++++++++++++----------------------- 1 file changed, 50 insertions(+), 49 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index ab6213cce4..63c1d7d9d3 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -176,7 +176,8 @@ type manual struct { // Configurator holds a Config and is responsible for generating all the // *tls.Config necessary for Consul. Except the one in the api package. type Configurator struct { - sync.RWMutex + // lock synchronizes access to all fields on this struct + lock sync.RWMutex base *Config autoTLS *autoTLS manual *manual @@ -211,15 +212,15 @@ func NewConfigurator(config Config, logger hclog.Logger) (*Configurator, error) // CAPems returns the currently loaded CAs in PEM format. func (c *Configurator) CAPems() []string { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return append(c.manual.caPems, c.autoTLS.caPems()...) } // ManualCAPems returns the currently loaded CAs in PEM format. func (c *Configurator) ManualCAPems() []string { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return c.manual.caPems } @@ -227,10 +228,10 @@ func (c *Configurator) ManualCAPems() []string { // *tls.Config. // This function acquires a write lock because it writes the new config. func (c *Configurator) Update(config Config) error { - c.Lock() + c.lock.Lock() // order of defers matters because log acquires a RLock() defer c.log("Update") - defer c.Unlock() + defer c.lock.Unlock() cert, err := loadKeyPair(config.CertFile, config.KeyFile) if err != nil { @@ -260,18 +261,18 @@ func (c *Configurator) Update(config Config) error { // certificates. // Or it is being called on the client side when CA changes are detected. func (c *Configurator) UpdateAutoTLSCA(connectCAPems []string) error { - c.Lock() + c.lock.Lock() // order of defers matters because log acquires a RLock() defer c.log("UpdateAutoEncryptCA") - defer c.Unlock() + defer c.lock.Unlock() pool, err := pool(append(c.manual.caPems, append(c.autoTLS.manualCAPems, connectCAPems...)...)) if err != nil { - c.RUnlock() + c.lock.RUnlock() return err } if err = c.check(*c.base, pool, c.manual.cert); err != nil { - c.RUnlock() + c.lock.RUnlock() return err } c.autoTLS.connectCAPems = connectCAPems @@ -289,8 +290,8 @@ func (c *Configurator) UpdateAutoTLSCert(pub, priv string) error { return fmt.Errorf("Failed to load cert/key pair: %v", err) } - c.Lock() - defer c.Unlock() + c.lock.Lock() + defer c.lock.Unlock() c.autoTLS.cert = &cert c.version++ @@ -307,8 +308,8 @@ func (c *Configurator) UpdateAutoTLS(manualCAPems, connectCAPems []string, pub, return fmt.Errorf("Failed to load cert/key pair: %v", err) } - c.Lock() - defer c.Unlock() + c.lock.Lock() + defer c.lock.Unlock() pool, err := pool(append(c.manual.caPems, append(manualCAPems, connectCAPems...)...)) if err != nil { @@ -324,15 +325,15 @@ func (c *Configurator) UpdateAutoTLS(manualCAPems, connectCAPems []string, pub, } func (c *Configurator) UpdateAreaPeerDatacenterUseTLS(peerDatacenter string, useTLS bool) { - c.Lock() - defer c.Unlock() + c.lock.Lock() + defer c.lock.Unlock() c.version++ c.peerDatacenterUseTLS[peerDatacenter] = useTLS } func (c *Configurator) getAreaForPeerDatacenterUseTLS(peerDatacenter string) bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() if v, ok := c.peerDatacenterUseTLS[peerDatacenter]; ok { return v } @@ -340,8 +341,8 @@ func (c *Configurator) getAreaForPeerDatacenterUseTLS(peerDatacenter string) boo } func (c *Configurator) Base() Config { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return *c.base } @@ -472,8 +473,8 @@ func (c *Configurator) commonTLSConfig(verifyIncoming bool) *tls.Config { // this needs to be outside of RLock because it acquires an RLock itself verifyServerHostname := c.VerifyServerHostname() - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() tlsConfig := &tls.Config{ InsecureSkipVerify: !verifyServerHostname, } @@ -529,8 +530,8 @@ func (c *Configurator) commonTLSConfig(verifyIncoming bool) *tls.Config { // This function acquires a read lock because it reads from the config. func (c *Configurator) Cert() *tls.Certificate { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() cert := c.manual.cert if cert == nil { cert = c.autoTLS.cert @@ -540,15 +541,15 @@ func (c *Configurator) Cert() *tls.Certificate { // This function acquires a read lock because it reads from the config. func (c *Configurator) VerifyIncomingRPC() bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return c.base.verifyIncomingRPC() } // This function acquires a read lock because it reads from the config. func (c *Configurator) outgoingRPCTLSDisabled() bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() // if AutoEncrypt enabled, always use TLS if c.base.AutoTLS { @@ -569,15 +570,15 @@ func (c *Configurator) MutualTLSCapable() bool { // This function acquires a read lock because it reads from the config. func (c *Configurator) mutualTLSCapable() bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return c.caPool != nil && (c.autoTLS.cert != nil || c.manual.cert != nil) } // This function acquires a read lock because it reads from the config. func (c *Configurator) verifyOutgoing() bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() // If AutoEncryptTLS is enabled and there is a CA, then verify // outgoing. @@ -601,36 +602,36 @@ func (c *Configurator) ServerSNI(dc, nodeName string) string { // This function acquires a read lock because it reads from the config. func (c *Configurator) domain() string { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return c.base.Domain } // This function acquires a read lock because it reads from the config. func (c *Configurator) verifyIncomingRPC() bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return c.base.verifyIncomingRPC() } // This function acquires a read lock because it reads from the config. func (c *Configurator) verifyIncomingHTTPS() bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return c.base.verifyIncomingHTTPS() } // This function acquires a read lock because it reads from the config. func (c *Configurator) enableAgentTLSForChecks() bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return c.base.EnableAgentTLSForChecks } // This function acquires a read lock because it reads from the config. func (c *Configurator) serverNameOrNodeName() string { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() if c.base.ServerName != "" { return c.base.ServerName } @@ -639,8 +640,8 @@ func (c *Configurator) serverNameOrNodeName() string { // This function acquires a read lock because it reads from the config. func (c *Configurator) VerifyServerHostname() bool { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() return c.base.VerifyServerHostname || c.autoTLS.verifyServerHostname } @@ -799,8 +800,8 @@ func (c *Configurator) OutgoingALPNRPCWrapper() ALPNWrapper { // AutoEncryptCertNotAfter returns NotAfter from the auto_encrypt cert. In case // there is no cert, it will return a time in the past. func (c *Configurator) AutoEncryptCertNotAfter() time.Time { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() tlsCert := c.autoTLS.cert if tlsCert == nil || tlsCert.Certificate == nil { return time.Now().AddDate(0, 0, -1) @@ -820,8 +821,8 @@ func (c *Configurator) AutoEncryptCertExpired() bool { // This function acquires a read lock because it reads from the config. func (c *Configurator) log(name string) { if c.logger != nil { - c.RLock() - defer c.RUnlock() + c.lock.RLock() + defer c.lock.RUnlock() c.logger.Trace(name, "version", c.version) } } From b3fa778d9139a029d85fa2b5cf00ef8f108b9fed Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 17 Jun 2021 18:59:53 -0400 Subject: [PATCH 3/5] tlsutil: fix a panic UpdateAutoTLSCA would panic if either of the calls errored, because the read lock was being unlocked incorrectly. --- tlsutil/config.go | 2 -- tlsutil/config_test.go | 15 ++++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 63c1d7d9d3..1a26f7113a 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -268,11 +268,9 @@ func (c *Configurator) UpdateAutoTLSCA(connectCAPems []string) error { pool, err := pool(append(c.manual.caPems, append(c.autoTLS.manualCAPems, connectCAPems...)...)) if err != nil { - c.lock.RUnlock() return err } if err = c.check(*c.base, pool, c.manual.cert); err != nil { - c.lock.RUnlock() return err } c.autoTLS.connectCAPems = connectCAPems diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 7287d8628b..571ff8113e 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -11,9 +11,11 @@ import ( "strings" "testing" - "github.com/hashicorp/consul/sdk/testutil" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/yamux" "github.com/stretchr/testify/require" + + "github.com/hashicorp/consul/sdk/testutil" ) func startRPCTLSServer(config *Config) (net.Conn, chan error) { @@ -831,6 +833,17 @@ func TestConfigurator_MutualTLSCapable(t *testing.T) { }) } +func TestConfigurator_UpdateAutoTLSCA_DoesNotPanic(t *testing.T) { + config := Config{ + Domain: "consul", + } + c, err := NewConfigurator(config, hclog.New(nil)) + require.NoError(t, err) + + err = c.UpdateAutoTLSCA([]string{"invalid pem"}) + require.Error(t, err) +} + func TestConfigurator_VerifyIncomingRPC(t *testing.T) { c := Configurator{base: &Config{ VerifyIncomingRPC: true, From bcf23cd1b442bbfa6a7d84189acc47df2e81e9d6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 17 Jun 2021 19:05:21 -0400 Subject: [PATCH 4/5] tlsutil: Un-method Configurator.check The method receiver was never used. Also rename it and add a godoc comment. --- tlsutil/config.go | 8 +++++--- tlsutil/config_test.go | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 1a26f7113a..d72d0397c3 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -245,7 +245,7 @@ func (c *Configurator) Update(config Config) error { if err != nil { return err } - if err = c.check(config, pool, cert); err != nil { + if err = validateConfig(config, pool, cert); err != nil { return err } c.base = &config @@ -270,7 +270,7 @@ func (c *Configurator) UpdateAutoTLSCA(connectCAPems []string) error { if err != nil { return err } - if err = c.check(*c.base, pool, c.manual.cert); err != nil { + if err = validateConfig(*c.base, pool, c.manual.cert); err != nil { return err } c.autoTLS.connectCAPems = connectCAPems @@ -357,7 +357,9 @@ func pool(pems []string) (*x509.CertPool, error) { return pool, nil } -func (c *Configurator) check(config Config, pool *x509.CertPool, cert *tls.Certificate) 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 { diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 571ff8113e..2608d28030 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -524,7 +524,7 @@ func TestConfigurator_ErrorPropagation(t *testing.T) { require.NoError(t, err, info) pool, err := pool(pems) require.NoError(t, err, info) - err3 = c.check(v.config, pool, cert) + err3 = validateConfig(v.config, pool, cert) } if v.shouldErr { require.Error(t, err1, info) From bca33d818fc33764d086b9dc33beacb72a092a75 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 17 Jun 2021 19:35:58 -0400 Subject: [PATCH 5/5] tlsutil: remove the RLock from log The log method only needed the lock because it accessed version. By using an atomic instead of a lock, we can remove the risk that the comments call out, making log safer to use. Also updates the log name to match the function names, and adds some comments about how the lock is used. --- tlsutil/config.go | 43 +++++++++++++++++++++--------------------- tlsutil/config_test.go | 11 +++++------ 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index d72d0397c3..037f5978f2 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -11,6 +11,7 @@ import ( "sort" "strings" "sync" + "sync/atomic" "time" "github.com/hashicorp/go-hclog" @@ -176,16 +177,20 @@ type manual struct { // Configurator holds a Config and is responsible for generating all the // *tls.Config necessary for Consul. Except the one in the api package. type Configurator struct { - // lock synchronizes access to all fields on this struct + // lock synchronizes access to all fields on this struct except for logger and version. lock sync.RWMutex base *Config autoTLS *autoTLS manual *manual peerDatacenterUseTLS map[string]bool + caPool *x509.CertPool - caPool *x509.CertPool - logger hclog.Logger - version int + // logger is not protected by a lock. It must never be changed after + // Configurator is created. + logger hclog.Logger + // version is increased each time the Configurator is updated. Must be accessed + // using sync/atomic. + version uint64 } // NewConfigurator creates a new Configurator and sets the provided @@ -229,8 +234,6 @@ func (c *Configurator) ManualCAPems() []string { // This function acquires a write lock because it writes the new config. func (c *Configurator) Update(config Config) error { c.lock.Lock() - // order of defers matters because log acquires a RLock() - defer c.log("Update") defer c.lock.Unlock() cert, err := loadKeyPair(config.CertFile, config.KeyFile) @@ -252,7 +255,8 @@ func (c *Configurator) Update(config Config) error { c.manual.cert = cert c.manual.caPems = pems c.caPool = pool - c.version++ + atomic.AddUint64(&c.version, 1) + c.log("Update") return nil } @@ -262,8 +266,6 @@ func (c *Configurator) Update(config Config) error { // Or it is being called on the client side when CA changes are detected. func (c *Configurator) UpdateAutoTLSCA(connectCAPems []string) error { c.lock.Lock() - // order of defers matters because log acquires a RLock() - defer c.log("UpdateAutoEncryptCA") defer c.lock.Unlock() pool, err := pool(append(c.manual.caPems, append(c.autoTLS.manualCAPems, connectCAPems...)...)) @@ -275,14 +277,13 @@ func (c *Configurator) UpdateAutoTLSCA(connectCAPems []string) error { } c.autoTLS.connectCAPems = connectCAPems c.caPool = pool - c.version++ + atomic.AddUint64(&c.version, 1) + c.log("UpdateAutoTLSCA") return nil } // UpdateAutoTLSCert func (c *Configurator) UpdateAutoTLSCert(pub, priv string) error { - // order of defers matters because log acquires a RLock() - defer c.log("UpdateAutoEncryptCert") cert, err := tls.X509KeyPair([]byte(pub), []byte(priv)) if err != nil { return fmt.Errorf("Failed to load cert/key pair: %v", err) @@ -292,15 +293,14 @@ func (c *Configurator) UpdateAutoTLSCert(pub, priv string) error { defer c.lock.Unlock() c.autoTLS.cert = &cert - c.version++ + atomic.AddUint64(&c.version, 1) + c.log("UpdateAutoTLSCert") return nil } // UpdateAutoTLS sets everything under autoEncrypt. This is being called on the // client when it received its cert from AutoEncrypt/AutoConfig endpoints. func (c *Configurator) UpdateAutoTLS(manualCAPems, connectCAPems []string, pub, priv string, verifyServerHostname bool) error { - // order of defers matters because log acquires a RLock() - defer c.log("UpdateAutoEncrypt") cert, err := tls.X509KeyPair([]byte(pub), []byte(priv)) if err != nil { return fmt.Errorf("Failed to load cert/key pair: %v", err) @@ -318,14 +318,16 @@ func (c *Configurator) UpdateAutoTLS(manualCAPems, connectCAPems []string, pub, c.autoTLS.cert = &cert c.caPool = pool c.autoTLS.verifyServerHostname = verifyServerHostname - c.version++ + atomic.AddUint64(&c.version, 1) + c.log("UpdateAutoTLS") return nil } func (c *Configurator) UpdateAreaPeerDatacenterUseTLS(peerDatacenter string, useTLS bool) { c.lock.Lock() defer c.lock.Unlock() - c.version++ + atomic.AddUint64(&c.version, 1) + c.log("UpdateAreaPeerDatacenterUseTLS") c.peerDatacenterUseTLS[peerDatacenter] = useTLS } @@ -818,12 +820,9 @@ func (c *Configurator) AutoEncryptCertExpired() bool { return c.AutoEncryptCertNotAfter().Before(time.Now()) } -// This function acquires a read lock because it reads from the config. func (c *Configurator) log(name string) { - if c.logger != nil { - c.lock.RLock() - defer c.lock.RUnlock() - c.logger.Trace(name, "version", c.version) + if c.logger != nil && c.logger.IsTrace() { + c.logger.Trace(name, "version", atomic.LoadUint64(&c.version)) } } diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 2608d28030..1282cda958 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -1026,11 +1026,10 @@ func TestConfigurator_UpdateChecks(t *testing.T) { require.NoError(t, err) require.NoError(t, c.Update(Config{})) require.Error(t, c.Update(Config{VerifyOutgoing: true})) - require.Error(t, c.Update(Config{VerifyIncoming: true, - CAFile: "../test/ca/root.cer"})) + require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) require.False(t, c.base.VerifyIncoming) require.False(t, c.base.VerifyOutgoing) - require.Equal(t, c.version, 2) + require.Equal(t, uint64(2), c.version) } func TestConfigurator_UpdateSetsStuff(t *testing.T) { @@ -1039,10 +1038,10 @@ func TestConfigurator_UpdateSetsStuff(t *testing.T) { require.Nil(t, c.caPool) require.Nil(t, c.manual.cert) require.Equal(t, c.base, &Config{}) - require.Equal(t, 1, c.version) + require.Equal(t, uint64(1), c.version) require.Error(t, c.Update(Config{VerifyOutgoing: true})) - require.Equal(t, c.version, 1) + require.Equal(t, uint64(1), c.version) config := Config{ CAFile: "../test/ca/root.cer", @@ -1054,7 +1053,7 @@ func TestConfigurator_UpdateSetsStuff(t *testing.T) { require.Len(t, c.caPool.Subjects(), 1) require.NotNil(t, c.manual.cert) require.Equal(t, c.base, &config) - require.Equal(t, 2, c.version) + require.Equal(t, uint64(2), c.version) } func TestConfigurator_ServerNameOrNodeName(t *testing.T) {