From 8e83cf8d8a77c70ba69865d81376eda53c0499b9 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 21 Mar 2018 12:46:05 -0400 Subject: [PATCH] Allow TLS configurations for HTTP and RPC connections to be reloaded separately --- command/agent/agent.go | 28 +++++++-- command/agent/agent_test.go | 104 +++++++++++++++++++++++++++++-- command/agent/command.go | 6 +- nomad/server.go | 2 +- nomad/server_test.go | 58 ++++++++++++++++- nomad/structs/config/tls.go | 13 ++-- nomad/structs/config/tls_test.go | 14 ++--- 7 files changed, 197 insertions(+), 28 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 6965cb2db..ff62f6266 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -777,14 +777,34 @@ func (a *Agent) Stats() map[string]map[string]string { // ShouldReload determines if we should reload the configuration and agent // connections. If the TLS Configuration has not changed, we shouldn't reload. -func (a *Agent) ShouldReload(newConfig *Config) (bool, bool) { +func (a *Agent) ShouldReload(newConfig *Config) (bool, bool, bool) { + var shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC bool + a.configLock.Lock() defer a.configLock.Unlock() - if a.config.TLSConfig.Equals(newConfig.TLSConfig) { - return false, false + + var tlsInfoChanged bool + if !a.config.TLSConfig.CertificateInfoIsEqual(newConfig.TLSConfig) { + tlsInfoChanged = true } - return true, true // requires a reload of both agent and http server + // Allow the ability to only reload HTTP connections + if a.config.TLSConfig.EnableHTTP != newConfig.TLSConfig.EnableHTTP || tlsInfoChanged { + shouldReloadHTTP = true + } + + // Allow the ability to only reload HTTP connections + if a.config.TLSConfig.EnableRPC != newConfig.TLSConfig.EnableRPC || tlsInfoChanged { + shouldReloadRPC = true + } + + // Always reload the agent if either HTTP or RPC connections need to reload, + // or if the TLS configuration itself has changed + if shouldReloadHTTP || shouldReloadRPC || tlsInfoChanged { + shouldReloadAgent = true + } + + return shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC } // Reload handles configuration changes for the agent. Provides a method that diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 7c2e163f0..2df13df26 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -771,9 +771,104 @@ func TestServer_ShouldReload_ReturnFalseForNoChanges(t *testing.T) { config: agentConfig, } - shouldReloadAgent, shouldReloadHTTPServer := agent.ShouldReload(sameAgentConfig) + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) assert.False(shouldReloadAgent) - assert.False(shouldReloadHTTPServer) + assert.False(shouldReloadHTTP) + assert.False(shouldReloadRPC) +} + +func TestServer_ShouldReload_ReturnTrueForOnlyHTTPChanges(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) + + logger := log.New(ioutil.Discard, "", 0) + + agentConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: false, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + }, + } + + sameAgentConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + }, + } + + agent := &Agent{ + logger: logger, + config: agentConfig, + } + + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + assert.True(shouldReloadAgent) + assert.True(shouldReloadHTTP) + assert.False(shouldReloadRPC) +} + +func TestServer_ShouldReload_ReturnTrueForOnlyRPCChanges(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) + + logger := log.New(ioutil.Discard, "", 0) + + agentConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: false, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + }, + } + + sameAgentConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + }, + } + + agent := &Agent{ + logger: logger, + config: agentConfig, + } + + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + assert.True(shouldReloadAgent) + assert.False(shouldReloadHTTP) + assert.True(shouldReloadRPC) } func TestServer_ShouldReload_ReturnTrueForConfigChanges(t *testing.T) { @@ -819,7 +914,8 @@ func TestServer_ShouldReload_ReturnTrueForConfigChanges(t *testing.T) { config: agentConfig, } - shouldReloadAgent, shouldReloadHTTPServer := agent.ShouldReload(newConfig) + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(newConfig) assert.True(shouldReloadAgent) - assert.True(shouldReloadHTTPServer) + assert.True(shouldReloadHTTP) + assert.True(shouldReloadRPC) } diff --git a/command/agent/command.go b/command/agent/command.go index 91c44f8c4..2eb015a8c 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -640,7 +640,7 @@ func (c *Command) handleReload() { newConf.LogLevel = c.agent.GetConfig().LogLevel } - shouldReloadAgent, shouldReloadHTTPServer := c.agent.ShouldReload(newConf) + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := c.agent.ShouldReload(newConf) if shouldReloadAgent { c.agent.logger.Printf("[DEBUG] agent: starting reload of agent config") err := c.agent.Reload(newConf) @@ -648,7 +648,9 @@ func (c *Command) handleReload() { c.agent.logger.Printf("[ERR] agent: failed to reload the config: %v", err) return } + } + if shouldReloadRPC { if s := c.agent.Server(); s != nil { sconf, err := convertServerConfig(newConf, c.logOutput) c.agent.logger.Printf("[DEBUG] agent: starting reload of server config") @@ -681,7 +683,7 @@ func (c *Command) handleReload() { // we error in either of the above cases. For example, reloading the http // server to a TLS connection could succeed, while reloading the server's rpc // connections could fail. - if shouldReloadHTTPServer { + if shouldReloadHTTP { err := c.reloadHTTPServer() if err != nil { c.agent.logger.Printf("[ERR] http: failed to reload the config: %v", err) diff --git a/nomad/server.go b/nomad/server.go index 68789da4a..d4c5ac64f 100644 --- a/nomad/server.go +++ b/nomad/server.go @@ -658,7 +658,7 @@ func (s *Server) Reload(newConfig *Config) error { } } - if !newConfig.TLSConfig.Equals(s.config.TLSConfig) { + if !newConfig.TLSConfig.CertificateInfoIsEqual(s.config.TLSConfig) || newConfig.TLSConfig.EnableRPC != s.config.TLSConfig.EnableRPC { if err := s.reloadTLSConnections(newConfig.TLSConfig); err != nil { s.logger.Printf("[ERR] nomad: error reloading server TLS configuration: %s", err) multierror.Append(&mErr, err) diff --git a/nomad/server_test.go b/nomad/server_test.go index 826885773..a1f72ae11 100644 --- a/nomad/server_test.go +++ b/nomad/server_test.go @@ -271,7 +271,7 @@ func TestServer_Reload_TLSConnections_PlaintextToTLS(t *testing.T) { err := s1.reloadTLSConnections(newTLSConfig) assert.Nil(err) - assert.True(s1.config.TLSConfig.Equals(newTLSConfig)) + assert.True(s1.config.TLSConfig.CertificateInfoIsEqual(newTLSConfig)) codec := rpcClient(t, s1) @@ -319,7 +319,61 @@ func TestServer_Reload_TLSConnections_TLSToPlaintext_RPC(t *testing.T) { err := s1.reloadTLSConnections(newTLSConfig) assert.Nil(err) - assert.True(s1.config.TLSConfig.Equals(newTLSConfig)) + assert.True(s1.config.TLSConfig.CertificateInfoIsEqual(newTLSConfig)) + + codec := rpcClient(t, s1) + + node := mock.Node() + req := &structs.NodeRegisterRequest{ + Node: node, + WriteRequest: structs.WriteRequest{Region: "global"}, + } + + var resp structs.GenericResponse + err = msgpackrpc.CallWithCodec(codec, "Node.Register", req, &resp) + assert.Nil(err) +} + +// Tests that the server will successfully reload its network connections, +// downgrading only RPC connections +func TestServer_Reload_TLSConnections_TLSToPlaintext_OnlyRPC(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) + + s1 := TestServer(t, func(c *Config) { + c.DataDir = path.Join(dir, "nodeB") + c.TLSConfig = &config.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + }) + defer s1.Shutdown() + + newTLSConfig := &config.TLSConfig{ + EnableHTTP: true, + EnableRPC: false, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + + err := s1.reloadTLSConnections(newTLSConfig) + assert.Nil(err) + assert.True(s1.config.TLSConfig.CertificateInfoIsEqual(newTLSConfig)) codec := rpcClient(t, s1) diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index be1e8f42a..aad798e0a 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -186,20 +186,17 @@ func (t *TLSConfig) Merge(b *TLSConfig) *TLSConfig { return result } -// Equals compares the fields of two TLS configuration objects, returning a -// boolean indicating if they are the same. +// CertificateInfoIsEqual compares the fields of two TLS configuration objects +// for the fields that are specific to configuring a TLS connection // It is possible for either the calling TLSConfig to be nil, or the TLSConfig // that it is being compared against, so we need to handle both places. See // server.go Reload for example. -func (t *TLSConfig) Equals(newConfig *TLSConfig) bool { +func (t *TLSConfig) CertificateInfoIsEqual(newConfig *TLSConfig) bool { if t == nil || newConfig == nil { return t == newConfig } - return t.EnableRPC == newConfig.EnableRPC && - t.CAFile == newConfig.CAFile && + return t.CAFile == newConfig.CAFile && t.CertFile == newConfig.CertFile && - t.KeyFile == newConfig.KeyFile && - t.RPCUpgradeMode == newConfig.RPCUpgradeMode && - t.VerifyHTTPSClient == newConfig.VerifyHTTPSClient + t.KeyFile == newConfig.KeyFile } diff --git a/nomad/structs/config/tls_test.go b/nomad/structs/config/tls_test.go index e6dc36363..47e3e9993 100644 --- a/nomad/structs/config/tls_test.go +++ b/nomad/structs/config/tls_test.go @@ -26,32 +26,32 @@ func TestTLSConfig_Merge(t *testing.T) { assert.Equal(b, new) } -func TestTLS_Equals_TrueWhenEmpty(t *testing.T) { +func TestTLS_CertificateInfoIsEqual_TrueWhenEmpty(t *testing.T) { assert := assert.New(t) a := &TLSConfig{} b := &TLSConfig{} - assert.True(a.Equals(b)) + assert.True(a.CertificateInfoIsEqual(b)) } -func TestTLS_Equals_FalseWhenUnequal(t *testing.T) { +func TestTLS_CertificateInfoIsEqual_FalseWhenUnequal(t *testing.T) { assert := assert.New(t) a := &TLSConfig{CAFile: "abc", CertFile: "def", KeyFile: "ghi"} b := &TLSConfig{CAFile: "jkl", CertFile: "def", KeyFile: "ghi"} - assert.False(a.Equals(b)) + assert.False(a.CertificateInfoIsEqual(b)) } -func TestTLS_Equals_TrueWhenEqual(t *testing.T) { +func TestTLS_CertificateInfoIsEqual_TrueWhenEqual(t *testing.T) { assert := assert.New(t) a := &TLSConfig{CAFile: "abc", CertFile: "def", KeyFile: "ghi"} b := &TLSConfig{CAFile: "abc", CertFile: "def", KeyFile: "ghi"} - assert.True(a.Equals(b)) + assert.True(a.CertificateInfoIsEqual(b)) } func TestTLS_Copy(t *testing.T) { assert := assert.New(t) a := &TLSConfig{CAFile: "abc", CertFile: "def", KeyFile: "ghi"} aCopy := a.Copy() - assert.True(a.Equals(aCopy)) + assert.True(a.CertificateInfoIsEqual(aCopy)) } // GetKeyLoader should always return an initialized KeyLoader for a TLSConfig