From 08bdafbe783d761cd56b315c4de696a0341c4a09 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 6 Nov 2017 23:10:23 -0500 Subject: [PATCH] fixups from code review --- command/agent/agent.go | 2 +- command/agent/agent_test.go | 62 +++++++++---------------------------- command/agent/config.go | 1 - helper/tlsutil/config.go | 2 +- nomad/structs/config/tls.go | 36 ++++++++++++++------- 5 files changed, 41 insertions(+), 62 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index aac443cf03ce..380483d2813d 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -727,7 +727,7 @@ func (a *Agent) Stats() map[string]map[string]string { // Reload handles configuration changes for the agent. Provides a method that // is easier to unit test, as this action is invoked via SIGHUP. func (a *Agent) Reload(newConfig *Config) error { - if a.config != nil && newConfig.TLSConfig != nil { + if newConfig.TLSConfig != nil { // If the agent is already running with TLS enabled, we need to only reload // its certificates. diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 684f179b9bbe..d26c3227a6c0 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -560,55 +560,16 @@ func TestAgent_HTTPCheckPath(t *testing.T) { } } -func TestServer_Reload_TLS_UpgradeToTLS(t *testing.T) { +func TestServer_Reload_TLS_Certificate(t *testing.T) { t.Parallel() assert := assert.New(t) const ( - cafile = "../../helper/tlsutil/testdata/ca.pem" - foocert = "../../helper/tlsutil/testdata/nomad-foo.pem" - fookey = "../../helper/tlsutil/testdata/nomad-foo-key.pem" - ) - dir := tmpDir(t) - defer os.RemoveAll(dir) - - agentConfig := &Config{ - TLSConfig: &sconfig.TLSConfig{ - EnableHTTP: false, - }, - } - - agent := &Agent{ - config: agentConfig, - } - - newConfig := &Config{ - TLSConfig: &sconfig.TLSConfig{ - EnableHTTP: true, - EnableRPC: true, - VerifyServerHostname: true, - CAFile: cafile, - CertFile: foocert, - KeyFile: fookey, - }, - } - - assert.Nil(agentConfig.TLSConfig.GetKeyLoader().Certificate) - - err := agent.Reload(newConfig) - assert.Nil(err) - - assert.NotNil(agentConfig.TLSConfig.GetKeyLoader().Certificate) -} - -func TestServer_Reload_TLS_DowngradeFromTLS(t *testing.T) { - t.Parallel() - assert := assert.New(t) - - const ( - cafile = "../../helper/tlsutil/testdata/ca.pem" - foocert = "../../helper/tlsutil/testdata/nomad-foo.pem" - fookey = "../../helper/tlsutil/testdata/nomad-foo-key.pem" + cafile = "../../helper/tlsutil/testdata/ca.pem" + foocert = "../../helper/tlsutil/testdata/nomad-bad.pem" + fookey = "../../helper/tlsutil/testdata/nomad-bad-key.pem" + foocert2 = "../../helper/tlsutil/testdata/nomad-foo.pem" + fookey2 = "../../helper/tlsutil/testdata/nomad-foo-key.pem" ) dir := tmpDir(t) defer os.RemoveAll(dir) @@ -630,12 +591,17 @@ func TestServer_Reload_TLS_DowngradeFromTLS(t *testing.T) { newConfig := &Config{ TLSConfig: &sconfig.TLSConfig{ - EnableHTTP: false, + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert2, + KeyFile: fookey2, }, } err := agent.Reload(newConfig) assert.Nil(err) - - assert.Nil(agentConfig.TLSConfig.GetKeyLoader().Certificate) + assert.Equal(agent.config.TLSConfig.CertFile, newConfig.TLSConfig.CertFile) + assert.Equal(agent.config.TLSConfig.KeyFile, newConfig.TLSConfig.KeyFile) } diff --git a/command/agent/config.go b/command/agent/config.go index ebe53b2c5bc9..f5915585030f 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -340,7 +340,6 @@ func (c *Config) UpdateTLSConfig(newConfig *config.TLSConfig) error { keyLoader := c.TLSConfig.GetKeyLoader() _, err := keyLoader.LoadKeyPair(newConfig.CertFile, newConfig.KeyFile) - if err != nil { return err } diff --git a/helper/tlsutil/config.go b/helper/tlsutil/config.go index 359ea2879bff..e733b2860555 100644 --- a/helper/tlsutil/config.go +++ b/helper/tlsutil/config.go @@ -63,7 +63,7 @@ type Config struct { // Must be provided to serve TLS connections. KeyFile string - // Utility to dynamically reload TLS configuration. + // KeyLoader dynamically reloads TLS configuration. KeyLoader *config.KeyLoader } diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index 5030cb30d436..48b5f7e597db 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -8,7 +8,6 @@ import ( // TLSConfig provides TLS related configuration type TLSConfig struct { - configLock sync.Mutex // EnableHTTP enabled TLS for http traffic to the Nomad server and clients EnableHTTP bool `mapstructure:"http"` @@ -35,6 +34,8 @@ type TLSConfig struct { // KeyLoader is a helper to dynamically reload TLS configuration KeyLoader *KeyLoader + keyloaderLock sync.Mutex + // KeyFile is used to provide a TLS key that is used for serving TLS connections. // Must be provided to serve TLS connections. KeyFile string `mapstructure:"key_file"` @@ -50,15 +51,18 @@ type TLSConfig struct { type KeyLoader struct { cacheLock sync.Mutex - Certificate *tls.Certificate + certificate *tls.Certificate } // LoadKeyPair reloads the TLS certificate based on the specified certificate // and key file. If successful, stores the certificate for further use. func (k *KeyLoader) LoadKeyPair(certFile, keyFile string) (*tls.Certificate, error) { + k.cacheLock.Lock() + defer k.cacheLock.Unlock() + // Allow downgrading if certFile == "" && keyFile == "" { - k.Certificate = nil + k.certificate = nil return nil, nil } @@ -67,20 +71,25 @@ func (k *KeyLoader) LoadKeyPair(certFile, keyFile string) (*tls.Certificate, err return nil, fmt.Errorf("Failed to load cert/key pair: %v", err) } - k.cacheLock.Lock() - defer k.cacheLock.Unlock() - - k.Certificate = &cert - return k.Certificate, nil + k.certificate = &cert + return k.certificate, nil } +// GetOutgoingCertificate fetches the currently-loaded certificate. This +// currently does not consider information in the ClientHello and only returns +// the certificate that was last loaded. func (k *KeyLoader) GetOutgoingCertificate(*tls.ClientHelloInfo) (*tls.Certificate, error) { - return k.Certificate, nil + k.cacheLock.Lock() + defer k.cacheLock.Unlock() + + return k.certificate, nil } +// GetKeyLoader returns the keyloader for a TLSConfig object. If the keyloader +// has not been initialized, it will first do so. func (t *TLSConfig) GetKeyLoader() *KeyLoader { - t.configLock.Lock() - defer t.configLock.Unlock() + t.keyloaderLock.Lock() + defer t.keyloaderLock.Unlock() // If the keyloader has not yet been initialized, do it here if t.KeyLoader == nil { @@ -92,13 +101,18 @@ func (t *TLSConfig) GetKeyLoader() *KeyLoader { // Copy copies the fields of TLSConfig to another TLSConfig object. Required as // to not copy mutexes between objects. func (t *TLSConfig) Copy() *TLSConfig { + new := &TLSConfig{} new.EnableHTTP = t.EnableHTTP new.EnableRPC = t.EnableRPC new.VerifyServerHostname = t.VerifyServerHostname new.CAFile = t.CAFile new.CertFile = t.CertFile + + t.keyloaderLock.Lock() new.KeyLoader = t.KeyLoader + t.keyloaderLock.Unlock() + new.KeyFile = t.KeyFile new.RPCUpgradeMode = t.RPCUpgradeMode new.VerifyHTTPSClient = t.VerifyHTTPSClient