Nomad agent reload TLS configuration on SIGHUP (#3479)

* Allow server TLS configuration to be reloaded via SIGHUP

* dynamic tls reloading for nomad agents

* code cleanup and refactoring

* ensure keyloader is initialized, add comments

* allow downgrading from TLS

* initalize keyloader if necessary

* integration test for tls reload

* fix up test to assert success on reloaded TLS configuration

* failure in loading a new TLS config should remain at current

Reload only the config if agent is already using TLS

* reload agent configuration before specific server/client

lock keyloader before loading/caching a new certificate

* introduce a get-or-set method for keyloader

* fixups from code review

* fix up linting errors

* fixups from code review

* add lock for config updates; improve copy of tls config

* GetCertificate only reloads certificates dynamically for the server

* config updates/copies should be on agent

* improve http integration test

* simplify agent reloading storing a local copy of config

* reuse the same keyloader when reloading

* Test that server and client get reloaded but keep keyloader

* Keyloader exposes GetClientCertificate as well for outgoing connections

* Fix spelling

* correct changelog style

command/agent/agent.go

@@ -43,7 +43,9 @@ const (
 // scheduled to, and are responsible for interfacing with
 // servers to run allocations.
 type Agent struct {
-	config    *Config
+	config     *Config
+	configLock sync.Mutex
+
 	logger    *log.Logger
 	logOutput io.Writer
 
@@ -724,6 +726,40 @@ func (a *Agent) Stats() map[string]map[string]string {
 	return stats
 }
 
+// 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 {
+	a.configLock.Lock()
+	defer a.configLock.Unlock()
+
+	if newConfig.TLSConfig != nil {
+
+		// TODO(chelseakomlo) In a later PR, we will introduce the ability to reload
+		// TLS configuration if the agent is not running with TLS enabled.
+		if a.config.TLSConfig != nil {
+			// Reload the certificates on the keyloader and on success store the
+			// updated TLS config. It is important to reuse the same keyloader
+			// as this allows us to dynamically reload configurations not only
+			// on the Agent but on the Server and Client too (they are
+			// referencing the same keyloader).
+			keyloader := a.config.TLSConfig.GetKeyLoader()
+			_, err := keyloader.LoadKeyPair(newConfig.TLSConfig.CertFile, newConfig.TLSConfig.KeyFile)
+			if err != nil {
+				return err
+			}
+			a.config.TLSConfig = newConfig.TLSConfig
+			a.config.TLSConfig.KeyLoader = keyloader
+		}
+	}
+
+	return nil
+}
+
+// GetConfigCopy creates a replica of the agent's config, excluding locks
+func (a *Agent) GetConfig() *Config {
+	return a.config
+}
+
 // setupConsul creates the Consul client and starts its main Run loop.
 func (a *Agent) setupConsul(consulConfig *config.ConsulConfig) error {
 	apiConf, err := consulConfig.ApiConfig()

command/agent/agent_test.go

@@ -22,7 +22,7 @@ func tmpDir(t testing.TB) string {
 	return dir
 }
 
-func TestAgent_RPCPing(t *testing.T) {
+func TestAgent_RPC_Ping(t *testing.T) {
 	t.Parallel()
 	agent := NewTestAgent(t, t.Name(), nil)
 	defer agent.Shutdown()
@@ -559,3 +559,190 @@ func TestAgent_HTTPCheckPath(t *testing.T) {
 		t.Errorf("expected client check path to be %q but found %q", expected, check.Path)
 	}
 }
+
+// This test asserts that the keyloader embedded in the TLS config is shared
+// across the Agent, Server, and Client. This is essential for certificate
+// reloading to work.
+func TestServer_Reload_TLS_Shared_Keyloader(t *testing.T) {
+	t.Parallel()
+	assert := assert.New(t)
+
+	// We will start out with a bad cert and then reload with a good one.
+	const (
+		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"
+	)
+
+	agent := NewTestAgent(t, t.Name(), func(c *Config) {
+		c.TLSConfig = &sconfig.TLSConfig{
+			EnableHTTP:           true,
+			EnableRPC:            true,
+			VerifyServerHostname: true,
+			CAFile:               cafile,
+			CertFile:             foocert,
+			KeyFile:              fookey,
+		}
+	})
+	defer agent.Shutdown()
+
+	originalKeyloader := agent.Config.TLSConfig.GetKeyLoader()
+	originalCert, err := originalKeyloader.GetOutgoingCertificate(nil)
+	assert.NotNil(originalKeyloader)
+	if assert.Nil(err) {
+		assert.NotNil(originalCert)
+	}
+
+	// Switch to the correct certificates and reload
+	newConfig := &Config{
+		TLSConfig: &sconfig.TLSConfig{
+			EnableHTTP:           true,
+			EnableRPC:            true,
+			VerifyServerHostname: true,
+			CAFile:               cafile,
+			CertFile:             foocert2,
+			KeyFile:              fookey2,
+		},
+	}
+
+	assert.Nil(agent.Reload(newConfig))
+	assert.Equal(agent.Config.TLSConfig.CertFile, newConfig.TLSConfig.CertFile)
+	assert.Equal(agent.Config.TLSConfig.KeyFile, newConfig.TLSConfig.KeyFile)
+	assert.Equal(agent.Config.TLSConfig.GetKeyLoader(), originalKeyloader)
+
+	// Assert is passed through on the server correctly
+	if assert.NotNil(agent.server.GetConfig().TLSConfig) {
+		serverKeyloader := agent.server.GetConfig().TLSConfig.GetKeyLoader()
+		assert.Equal(serverKeyloader, originalKeyloader)
+		newCert, err := serverKeyloader.GetOutgoingCertificate(nil)
+		assert.Nil(err)
+		assert.NotEqual(originalCert, newCert)
+	}
+
+	// Assert is passed through on the client correctly
+	if assert.NotNil(agent.client.GetConfig().TLSConfig) {
+		clientKeyloader := agent.client.GetConfig().TLSConfig.GetKeyLoader()
+		assert.Equal(clientKeyloader, originalKeyloader)
+		newCert, err := clientKeyloader.GetOutgoingCertificate(nil)
+		assert.Nil(err)
+		assert.NotEqual(originalCert, newCert)
+	}
+}
+
+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-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"
+	)
+
+	agentConfig := &Config{
+		TLSConfig: &sconfig.TLSConfig{
+			EnableHTTP:           true,
+			EnableRPC:            true,
+			VerifyServerHostname: true,
+			CAFile:               cafile,
+			CertFile:             foocert,
+			KeyFile:              fookey,
+		},
+	}
+
+	agent := &Agent{
+		config: agentConfig,
+	}
+
+	newConfig := &Config{
+		TLSConfig: &sconfig.TLSConfig{
+			EnableHTTP:           true,
+			EnableRPC:            true,
+			VerifyServerHostname: true,
+			CAFile:               cafile,
+			CertFile:             foocert2,
+			KeyFile:              fookey2,
+		},
+	}
+
+	originalKeyloader := agentConfig.TLSConfig.GetKeyLoader()
+	assert.NotNil(originalKeyloader)
+
+	err := agent.Reload(newConfig)
+	assert.Nil(err)
+	assert.Equal(agent.config.TLSConfig.CertFile, newConfig.TLSConfig.CertFile)
+	assert.Equal(agent.config.TLSConfig.KeyFile, newConfig.TLSConfig.KeyFile)
+	assert.Equal(agent.config.TLSConfig.GetKeyLoader(), originalKeyloader)
+}
+
+func TestServer_Reload_TLS_Certificate_Invalid(t *testing.T) {
+	t.Parallel()
+	assert := assert.New(t)
+
+	const (
+		cafile   = "../../helper/tlsutil/testdata/ca.pem"
+		foocert  = "../../helper/tlsutil/testdata/nomad-bad.pem"
+		fookey   = "../../helper/tlsutil/testdata/nomad-bad-key.pem"
+		foocert2 = "invalid_cert_path"
+		fookey2  = "invalid_key_path"
+	)
+
+	agentConfig := &Config{
+		TLSConfig: &sconfig.TLSConfig{
+			EnableHTTP:           true,
+			EnableRPC:            true,
+			VerifyServerHostname: true,
+			CAFile:               cafile,
+			CertFile:             foocert,
+			KeyFile:              fookey,
+		},
+	}
+
+	agent := &Agent{
+		config: agentConfig,
+	}
+
+	newConfig := &Config{
+		TLSConfig: &sconfig.TLSConfig{
+			EnableHTTP:           true,
+			EnableRPC:            true,
+			VerifyServerHostname: true,
+			CAFile:               cafile,
+			CertFile:             foocert2,
+			KeyFile:              fookey2,
+		},
+	}
+
+	err := agent.Reload(newConfig)
+	assert.NotNil(err)
+	assert.NotEqual(agent.config.TLSConfig.CertFile, newConfig.TLSConfig.CertFile)
+	assert.NotEqual(agent.config.TLSConfig.KeyFile, newConfig.TLSConfig.KeyFile)
+}
+
+func Test_GetConfig(t *testing.T) {
+	assert := assert.New(t)
+
+	agentConfig := &Config{
+		Telemetry:      &Telemetry{},
+		Client:         &ClientConfig{},
+		Server:         &ServerConfig{},
+		ACL:            &ACLConfig{},
+		Ports:          &Ports{},
+		Addresses:      &Addresses{},
+		AdvertiseAddrs: &AdvertiseAddrs{},
+		Vault:          &sconfig.VaultConfig{},
+		Consul:         &sconfig.ConsulConfig{},
+		Sentinel:       &sconfig.SentinelConfig{},
+	}
+
+	agent := &Agent{
+		config: agentConfig,
+	}
+
+	actualAgentConfig := agent.GetConfig()
+	assert.Equal(actualAgentConfig, agentConfig)
+}

command/agent/command.go

@@ -23,8 +23,8 @@ import (
 	checkpoint "github.com/hashicorp/go-checkpoint"
 	gsyslog "github.com/hashicorp/go-syslog"
 	"github.com/hashicorp/logutils"
-	"github.com/hashicorp/nomad/helper/flag-helpers"
-	"github.com/hashicorp/nomad/helper/gated-writer"
+	flaghelper "github.com/hashicorp/nomad/helper/flag-helpers"
+	gatedwriter "github.com/hashicorp/nomad/helper/gated-writer"
 	"github.com/hashicorp/nomad/nomad/structs/config"
 	"github.com/hashicorp/nomad/version"
 	"github.com/mitchellh/cli"
@@ -529,11 +529,11 @@ func (c *Command) Run(args []string) int {
 	go c.retryJoin(config)
 
 	// Wait for exit
-	return c.handleSignals(config)
+	return c.handleSignals()
 }
 
 // handleSignals blocks until we get an exit-causing signal
-func (c *Command) handleSignals(config *Config) int {
+func (c *Command) handleSignals() int {
 	signalCh := make(chan os.Signal, 4)
 	signal.Notify(signalCh, os.Interrupt, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGPIPE)
 
@@ -557,17 +557,15 @@ WAIT:
 
 	// Check if this is a SIGHUP
 	if sig == syscall.SIGHUP {
-		if conf := c.handleReload(config); conf != nil {
-			*config = *conf
-		}
+		c.handleReload()
 		goto WAIT
 	}
 
 	// Check if we should do a graceful leave
 	graceful := false
-	if sig == os.Interrupt && config.LeaveOnInt {
+	if sig == os.Interrupt && c.agent.GetConfig().LeaveOnInt {
 		graceful = true
-	} else if sig == syscall.SIGTERM && config.LeaveOnTerm {
+	} else if sig == syscall.SIGTERM && c.agent.GetConfig().LeaveOnTerm {
 		graceful = true
 	}
 
@@ -599,12 +597,12 @@ WAIT:
 }
 
 // handleReload is invoked when we should reload our configs, e.g. SIGHUP
-func (c *Command) handleReload(config *Config) *Config {
+func (c *Command) handleReload() {
 	c.Ui.Output("Reloading configuration...")
 	newConf := c.readConfig()
 	if newConf == nil {
 		c.Ui.Error(fmt.Sprintf("Failed to reload configs"))
-		return config
+		return
 	}
 
 	// Change the log level
@@ -617,7 +615,13 @@ func (c *Command) handleReload(config *Config) *Config {
 			minLevel, c.logFilter.Levels))
 
 		// Keep the current log level
-		newConf.LogLevel = config.LogLevel
+		newConf.LogLevel = c.agent.GetConfig().LogLevel
+	}
+
+	// Reloads configuration for an agent running in both client and server mode
+	err := c.agent.Reload(newConf)
+	if err != nil {
+		c.agent.logger.Printf("[ERR] agent: failed to reload the config: %v", err)
 	}
 
 	if s := c.agent.Server(); s != nil {
@@ -630,8 +634,6 @@ func (c *Command) handleReload(config *Config) *Config {
 			}
 		}
 	}
-
-	return newConf
 }
 
 // setupTelemetry is used ot setup the telemetry sub-systems

command/agent/config.go

@@ -680,8 +680,7 @@ func (c *Config) Merge(b *Config) *Config {
 
 	// Apply the TLS Config
 	if result.TLSConfig == nil && b.TLSConfig != nil {
-		tlsConfig := *b.TLSConfig
-		result.TLSConfig = &tlsConfig
+		result.TLSConfig = b.TLSConfig.Copy()
 	} else if b.TLSConfig != nil {
 		result.TLSConfig = result.TLSConfig.Merge(b.TLSConfig)
 	}

command/agent/config_parse.go

@@ -8,7 +8,7 @@ import (
 	"path/filepath"
 	"time"
 
-	"github.com/hashicorp/go-multierror"
+	multierror "github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/hcl"
 	"github.com/hashicorp/hcl/hcl/ast"
 	"github.com/hashicorp/nomad/helper"
@@ -749,6 +749,7 @@ func parseTLSConfig(result **config.TLSConfig, list *ast.ObjectList) error {
 	if err := mapstructure.WeakDecode(m, &tlsConfig); err != nil {
 		return err
 	}
+
 	*result = &tlsConfig
 	return nil
 }

command/agent/http.go

@@ -14,7 +14,7 @@ import (
 	"time"
 
 	"github.com/NYTimes/gziphandler"
-	"github.com/elazarl/go-bindata-assetfs"
+	assetfs "github.com/elazarl/go-bindata-assetfs"
 	"github.com/hashicorp/nomad/helper/tlsutil"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/rs/cors"
@@ -75,6 +75,7 @@ func NewHTTPServer(agent *Agent, config *Config) (*HTTPServer, error) {
 			CAFile:               config.TLSConfig.CAFile,
 			CertFile:             config.TLSConfig.CertFile,
 			KeyFile:              config.TLSConfig.KeyFile,
+			KeyLoader:            config.TLSConfig.GetKeyLoader(),
 		}
 		tlsConfig, err := tlsConf.IncomingTLSConfig()
 		if err != nil {

command/agent/http_test.go

@@ -529,6 +529,108 @@ func checkIndex(resp *httptest.ResponseRecorder) error {
 	return nil
 }
 
+func TestHTTP_VerifyHTTPSClient_AfterConfigReload(t *testing.T) {
+	t.Parallel()
+	assert := assert.New(t)
+
+	const (
+		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"
+	)
+
+	agentConfig := &Config{
+		TLSConfig: &config.TLSConfig{
+			EnableHTTP:        true,
+			VerifyHTTPSClient: true,
+			CAFile:            cafile,
+			CertFile:          foocert,
+			KeyFile:           fookey,
+		},
+	}
+
+	newConfig := &Config{
+		TLSConfig: &config.TLSConfig{
+			EnableHTTP:        true,
+			VerifyHTTPSClient: true,
+			CAFile:            cafile,
+			CertFile:          foocert2,
+			KeyFile:           fookey2,
+		},
+	}
+
+	s := makeHTTPServer(t, func(c *Config) {
+		c.TLSConfig = agentConfig.TLSConfig
+	})
+	defer s.Shutdown()
+
+	// Make an initial request that should fail.
+	// Requests that specify a valid hostname, CA cert, and client
+	// certificate succeed.
+	tlsConf := &tls.Config{
+		ServerName: "client.regionFoo.nomad",
+		RootCAs:    x509.NewCertPool(),
+		GetClientCertificate: func(*tls.CertificateRequestInfo) (*tls.Certificate, error) {
+			c, err := tls.LoadX509KeyPair(foocert, fookey)
+			if err != nil {
+				return nil, err
+			}
+			return &c, nil
+		},
+	}
+
+	// HTTPS request should succeed
+	httpsReqURL := fmt.Sprintf("https://%s/v1/agent/self", s.Agent.config.AdvertiseAddrs.HTTP)
+
+	cacertBytes, err := ioutil.ReadFile(cafile)
+	assert.Nil(err)
+	tlsConf.RootCAs.AppendCertsFromPEM(cacertBytes)
+
+	transport := &http.Transport{TLSClientConfig: tlsConf}
+	client := &http.Client{Transport: transport}
+	req, err := http.NewRequest("GET", httpsReqURL, nil)
+	assert.Nil(err)
+
+	// Check that we get an error that the certificate isn't valid for the
+	// region we are contacting.
+	_, err = client.Do(req)
+	assert.Contains(err.Error(), "certificate is valid for")
+
+	// Reload the TLS configuration==
+	assert.Nil(s.Agent.Reload(newConfig))
+
+	// Requests that specify a valid hostname, CA cert, and client
+	// certificate succeed.
+	tlsConf = &tls.Config{
+		ServerName: "client.regionFoo.nomad",
+		RootCAs:    x509.NewCertPool(),
+		GetClientCertificate: func(*tls.CertificateRequestInfo) (*tls.Certificate, error) {
+			c, err := tls.LoadX509KeyPair(foocert2, fookey2)
+			if err != nil {
+				return nil, err
+			}
+			return &c, nil
+		},
+	}
+
+	cacertBytes, err = ioutil.ReadFile(cafile)
+	assert.Nil(err)
+	tlsConf.RootCAs.AppendCertsFromPEM(cacertBytes)
+
+	transport = &http.Transport{TLSClientConfig: tlsConf}
+	client = &http.Client{Transport: transport}
+	req, err = http.NewRequest("GET", httpsReqURL, nil)
+	assert.Nil(err)
+
+	resp, err := client.Do(req)
+	if assert.Nil(err) {
+		resp.Body.Close()
+		assert.Equal(resp.StatusCode, 200)
+	}
+}
+
 // getIndex parses X-Nomad-Index
 func getIndex(t *testing.T, resp *httptest.ResponseRecorder) uint64 {
 	header := resp.Header().Get("X-Nomad-Index")