From f8df2071efe2108441b7a3f4a83668e836a7e961 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 26 Mar 2018 15:55:32 -0400 Subject: [PATCH 1/9] check file contents when determining if agent should reload TLS configuration --- command/agent/agent.go | 4 + command/agent/agent_test.go | 243 +++++++++++++++++++++++-------- nomad/structs/config/tls.go | 64 +++++++- nomad/structs/config/tls_test.go | 129 ++++++++++++++-- 4 files changed, 369 insertions(+), 71 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 4916c014a..1bd162d8a 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -77,6 +77,9 @@ func NewAgent(config *Config, logOutput io.Writer, inmem *metrics.InmemSink) (*A InmemSink: inmem, } + // ensure that the TLS configuration is properly set up + a.config.TLSConfig.SetChecksum() + if err := a.setupConsul(config.Consul); err != nil { return nil, fmt.Errorf("Failed to initialize Consul client: %v", err) } @@ -826,6 +829,7 @@ func (a *Agent) Reload(newConfig *Config) error { } a.config.TLSConfig = newConfig.TLSConfig a.config.TLSConfig.KeyLoader = keyloader + a.config.TLSConfig.SetChecksum() return nil } diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 115655822..7b0e7c2b0 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -4,6 +4,7 @@ import ( "io/ioutil" "log" "os" + "path/filepath" "strings" "testing" "time" @@ -743,19 +744,6 @@ func TestServer_ShouldReload_ReturnFalseForNoChanges(t *testing.T) { dir := tmpDir(t) defer os.RemoveAll(dir) - logger := log.New(ioutil.Discard, "", 0) - - agentConfig := &Config{ - TLSConfig: &sconfig.TLSConfig{ - EnableHTTP: true, - EnableRPC: true, - VerifyServerHostname: true, - CAFile: cafile, - CertFile: foocert, - KeyFile: fookey, - }, - } - sameAgentConfig := &Config{ TLSConfig: &sconfig.TLSConfig{ EnableHTTP: true, @@ -767,10 +755,17 @@ func TestServer_ShouldReload_ReturnFalseForNoChanges(t *testing.T) { }, } - agent := &Agent{ - logger: logger, - config: agentConfig, - } + 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() shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) assert.False(shouldReloadAgent) @@ -790,9 +785,7 @@ func TestServer_ShouldReload_ReturnTrueForOnlyHTTPChanges(t *testing.T) { dir := tmpDir(t) defer os.RemoveAll(dir) - logger := log.New(ioutil.Discard, "", 0) - - agentConfig := &Config{ + sameAgentConfig := &Config{ TLSConfig: &sconfig.TLSConfig{ EnableHTTP: false, EnableRPC: true, @@ -803,21 +796,17 @@ func TestServer_ShouldReload_ReturnTrueForOnlyHTTPChanges(t *testing.T) { }, } - sameAgentConfig := &Config{ - TLSConfig: &sconfig.TLSConfig{ + agent := NewTestAgent(t, t.Name(), func(c *Config) { + c.TLSConfig = &sconfig.TLSConfig{ EnableHTTP: true, EnableRPC: true, VerifyServerHostname: true, CAFile: cafile, CertFile: foocert, KeyFile: fookey, - }, - } - - agent := &Agent{ - logger: logger, - config: agentConfig, - } + } + }) + defer agent.Shutdown() shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) require.True(shouldReloadAgent) @@ -837,19 +826,6 @@ func TestServer_ShouldReload_ReturnTrueForOnlyRPCChanges(t *testing.T) { 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, @@ -861,10 +837,17 @@ func TestServer_ShouldReload_ReturnTrueForOnlyRPCChanges(t *testing.T) { }, } - agent := &Agent{ - logger: logger, - config: agentConfig, - } + agent := NewTestAgent(t, t.Name(), func(c *Config) { + c.TLSConfig = &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: false, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + }) + defer agent.Shutdown() shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) assert.True(shouldReloadAgent) @@ -880,24 +863,23 @@ func TestServer_ShouldReload_ReturnTrueForConfigChanges(t *testing.T) { cafile = "../../helper/tlsutil/testdata/ca.pem" foocert = "../../helper/tlsutil/testdata/nomad-foo.pem" fookey = "../../helper/tlsutil/testdata/nomad-foo-key.pem" - foocert2 = "any_cert_path" - fookey2 = "any_key_path" + foocert2 = "../../helper/tlsutil/testdata/nomad-bad.pem" + fookey2 = "../../helper/tlsutil/testdata/nomad-bad-key.pem" ) dir := tmpDir(t) defer os.RemoveAll(dir) - logger := log.New(ioutil.Discard, "", 0) - - agentConfig := &Config{ - TLSConfig: &sconfig.TLSConfig{ + 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() newConfig := &Config{ TLSConfig: &sconfig.TLSConfig{ @@ -910,13 +892,156 @@ func TestServer_ShouldReload_ReturnTrueForConfigChanges(t *testing.T) { }, } - agent := &Agent{ - logger: logger, - config: agentConfig, - } - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(newConfig) assert.True(shouldReloadAgent) assert.True(shouldReloadHTTP) assert.True(shouldReloadRPC) } + +func TestServer_ShouldReload_ReturnTrueForFileChanges(t *testing.T) { + t.Parallel() + require := require.New(t) + + oldCertificate := ` + -----BEGIN CERTIFICATE----- + MIICrzCCAlagAwIBAgIUN+4rYZ6wqQCIBzYYd0sfX2e8hDowCgYIKoZIzj0EAwIw + eDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNh + biBGcmFuY2lzY28xEjAQBgNVBAoTCUhhc2hpQ29ycDEOMAwGA1UECxMFTm9tYWQx + GDAWBgNVBAMTD25vbWFkLmhhc2hpY29ycDAgFw0xNjExMTAxOTU2MDBaGA8yMTE2 + MTAxNzE5NTYwMFoweDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWEx + FjAUBgNVBAcTDVNhbiBGcmFuY2lzY28xEjAQBgNVBAoTCUhhc2hpQ29ycDEOMAwG + A1UECxMFTm9tYWQxGDAWBgNVBAMTD3JlZ2lvbkZvby5ub21hZDBZMBMGByqGSM49 + AgEGCCqGSM49AwEHA0IABOqGSFNjm+EBlLYlxmIP6SQTdX8U/6hbPWObB0ffkEO/ + CFweeYIVWb3FKNPqYAlhMqg1K0ileD0FbhEzarP0sL6jgbswgbgwDgYDVR0PAQH/ + BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjAMBgNVHRMBAf8E + AjAAMB0GA1UdDgQWBBQnMcjU4yI3k0AoMtapACpO+w9QMTAfBgNVHSMEGDAWgBQ6 + NWr8F5y2eFwqfoQdQPg0kWb9QDA5BgNVHREEMjAwghZzZXJ2ZXIucmVnaW9uRm9v + Lm5vbWFkghZjbGllbnQucmVnaW9uRm9vLm5vbWFkMAoGCCqGSM49BAMCA0cAMEQC + ICrvzc5NzqhdT/HkazAx5OOUU8hqoptnmhRmwn6X+0y9AiA8bNvMUxHz3ZLjGBiw + PLBDC2UaSDqJqiiYpYegLhbQtw== + -----END CERTIFICATE----- + ` + + content := []byte(oldCertificate) + dir, err := ioutil.TempDir("", "certificate") + if err != nil { + log.Fatal(err) + } + defer os.RemoveAll(dir) // clean up + + tmpfn := filepath.Join(dir, "testcert") + err = ioutil.WriteFile(tmpfn, content, 0666) + require.Nil(err) + + const ( + cafile = "../../helper/tlsutil/testdata/ca.pem" + key = "../../helper/tlsutil/testdata/nomad-foo-key.pem" + ) + + logger := log.New(ioutil.Discard, "", 0) + + agentConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: tmpfn, + KeyFile: key, + }, + } + + agent := &Agent{ + logger: logger, + config: agentConfig, + } + agent.config.TLSConfig.SetChecksum() + + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(agentConfig) + require.False(shouldReloadAgent) + require.False(shouldReloadHTTP) + require.False(shouldReloadRPC) + + newCertificate := ` + -----BEGIN CERTIFICATE----- + MIICtTCCAlqgAwIBAgIUQp/L2szbgE4b1ASlPOZMReFE27owCgYIKoZIzj0EAwIw + fDELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDVNh + biBGcmFuY2lzY28xEjAQBgNVBAoTCUhhc2hpQ29ycDEOMAwGA1UECxMFTm9tYWQx + HDAaBgNVBAMTE2JhZC5ub21hZC5oYXNoaWNvcnAwIBcNMTYxMTEwMjAxMDAwWhgP + MjExNjEwMTcyMDEwMDBaMHgxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9y + bmlhMRYwFAYDVQQHEw1TYW4gRnJhbmNpc2NvMRIwEAYDVQQKEwlIYXNoaUNvcnAx + DjAMBgNVBAsTBU5vbWFkMRgwFgYDVQQDEw9yZWdpb25CYWQubm9tYWQwWTATBgcq + hkjOPQIBBggqhkjOPQMBBwNCAAQk6oXJwlxNrKvl6kpeeR4NJc5EYFI2b3y7odjY + u55Jp4sI91JVDqnpyatkyGmavdAWa4t0U6HkeaWqKk16/ZcYo4G7MIG4MA4GA1Ud + DwEB/wQEAwIFoDAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwDAYDVR0T + AQH/BAIwADAdBgNVHQ4EFgQUxhzOftFR2L0QAPx8LOuP99WPbpgwHwYDVR0jBBgw + FoAUHPDLSgzlHqBEh+c4A7HeT0GWygIwOQYDVR0RBDIwMIIWc2VydmVyLnJlZ2lv + bkJhZC5ub21hZIIWY2xpZW50LnJlZ2lvbkJhZC5ub21hZDAKBggqhkjOPQQDAgNJ + ADBGAiEAq2rnBeX/St/8i9Cab7Yw0C7pjcaE+mrFYeQByng1Uc0CIQD/o4BrZdkX + Nm7QGTRZbUFZTHYZr0ULz08Iaz2aHQ6Mcw== + -----END CERTIFICATE----- + ` + + err = ioutil.WriteFile(tmpfn, []byte(newCertificate), 0644) + require.Nil(err) + + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC = agent.ShouldReload(agentConfig) + require.True(shouldReloadAgent) + require.True(shouldReloadHTTP) + require.True(shouldReloadRPC) +} + +func TestServer_ShouldReload_ShouldHandleMultipleChanges(t *testing.T) { + t.Parallel() + require := require.New(t) + + const ( + cafile = "../../helper/tlsutil/testdata/ca.pem" + foocert = "../../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../../helper/tlsutil/testdata/nomad-foo-key.pem" + foocert2 = "../../helper/tlsutil/testdata/nomad-bad.pem" + fookey2 = "../../helper/tlsutil/testdata/nomad-bad-key.pem" + ) + dir := tmpDir(t) + defer os.RemoveAll(dir) + + sameAgentConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + }, + } + + agent := NewTestAgent(t, t.Name(), func(c *Config) { + c.TLSConfig = &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert2, + KeyFile: fookey2, + } + }) + defer agent.Shutdown() + + { + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + require.True(shouldReloadAgent) + require.True(shouldReloadHTTP) + require.True(shouldReloadRPC) + } + + err := agent.Reload(sameAgentConfig) + require.Nil(err) + + { + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC := agent.ShouldReload(sameAgentConfig) + require.False(shouldReloadAgent) + require.False(shouldReloadHTTP) + require.False(shouldReloadRPC) + } +} diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index aad798e0a..b83eec772 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -1,8 +1,12 @@ package config import ( + "crypto/md5" "crypto/tls" + "encoding/hex" "fmt" + "io" + "os" "sync" ) @@ -47,6 +51,10 @@ type TLSConfig struct { // Verify connections to the HTTPS API VerifyHTTPSClient bool `mapstructure:"verify_https_client"` + + // Checksum is a MD5 hash of the certificate CA File, Certificate file, and + // key file. + Checksum string } type KeyLoader struct { @@ -138,6 +146,9 @@ func (t *TLSConfig) Copy() *TLSConfig { new.KeyFile = t.KeyFile new.RPCUpgradeMode = t.RPCUpgradeMode new.VerifyHTTPSClient = t.VerifyHTTPSClient + + new.SetChecksum() + return new } @@ -196,7 +207,54 @@ func (t *TLSConfig) CertificateInfoIsEqual(newConfig *TLSConfig) bool { return t == newConfig } - return t.CAFile == newConfig.CAFile && - t.CertFile == newConfig.CertFile && - t.KeyFile == newConfig.KeyFile + if t.IsEmpty() && newConfig.IsEmpty() { + return true + } + + newCertChecksum, err := createChecksumOfFiles(newConfig.CAFile, newConfig.CertFile, newConfig.KeyFile) + if err != nil { + return false + } + + return t.Checksum == newCertChecksum +} + +// SetChecksum generates and sets the checksum for a TLS configuration +func (t *TLSConfig) SetChecksum() error { + newCertChecksum, err := createChecksumOfFiles(t.CAFile, t.CertFile, t.KeyFile) + if err != nil { + return err + } + + t.Checksum = newCertChecksum + return nil +} + +func getFileChecksum(filepath string) (string, error) { + f, err := os.Open(filepath) + if err != nil { + return "", err + } + defer f.Close() + + h := md5.New() + if _, err := io.Copy(h, f); err != nil { + return "", err + } + + return hex.EncodeToString(h.Sum(nil)), nil +} + +func createChecksumOfFiles(inputs ...string) (string, error) { + h := md5.New() + + for _, input := range inputs { + checksum, err := getFileChecksum(input) + if err != nil { + return "", err + } + io.WriteString(h, checksum) + } + + return hex.EncodeToString(h.Sum(nil)), nil } diff --git a/nomad/structs/config/tls_test.go b/nomad/structs/config/tls_test.go index 47e3e9993..f51b655c1 100644 --- a/nomad/structs/config/tls_test.go +++ b/nomad/structs/config/tls_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestTLSConfig_Merge(t *testing.T) { @@ -34,22 +35,106 @@ func TestTLS_CertificateInfoIsEqual_TrueWhenEmpty(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.CertificateInfoIsEqual(b)) + require := require.New(t) + const ( + cafile = "../../../helper/tlsutil/testdata/ca.pem" + foocert = "../../../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../../../helper/tlsutil/testdata/nomad-foo-key.pem" + foocert2 = "../../../helper/tlsutil/testdata/nomad-bad.pem" + fookey2 = "../../../helper/tlsutil/testdata/nomad-bad-key.pem" + ) + + // Assert that both mismatching certificate and key files are considered + // unequal + { + a := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + a.SetChecksum() + + b := &TLSConfig{ + CAFile: cafile, + CertFile: foocert2, + KeyFile: fookey2, + } + require.False(a.CertificateInfoIsEqual(b)) + } + + // Assert that mismatching certificate are considered unequal + { + a := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + a.SetChecksum() + + b := &TLSConfig{ + CAFile: cafile, + CertFile: foocert2, + KeyFile: fookey, + } + require.False(a.CertificateInfoIsEqual(b)) + } + + // Assert that mismatching keys are considered unequal + { + a := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + a.SetChecksum() + + b := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey2, + } + require.False(a.CertificateInfoIsEqual(b)) + } } +// Certificate info should be equal when the CA file, certificate file, and key +// file all are equal 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.CertificateInfoIsEqual(b)) + require := require.New(t) + const ( + cafile = "../../../helper/tlsutil/testdata/ca.pem" + foocert = "../../../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../../../helper/tlsutil/testdata/nomad-foo-key.pem" + ) + a := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + a.SetChecksum() + + b := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + require.True(a.CertificateInfoIsEqual(b)) } func TestTLS_Copy(t *testing.T) { assert := assert.New(t) - a := &TLSConfig{CAFile: "abc", CertFile: "def", KeyFile: "ghi"} + const ( + cafile = "../../../helper/tlsutil/testdata/ca.pem" + foocert = "../../../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../../../helper/tlsutil/testdata/nomad-foo-key.pem" + ) + a := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + a.SetChecksum() + aCopy := a.Copy() assert.True(a.CertificateInfoIsEqual(aCopy)) } @@ -61,3 +146,29 @@ func TestTLS_GetKeyloader(t *testing.T) { a := &TLSConfig{} assert.NotNil(a.GetKeyLoader()) } + +func TestTLS_SetChecksum(t *testing.T) { + require := require.New(t) + const ( + cafile = "../../../helper/tlsutil/testdata/ca.pem" + foocert = "../../../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../../../helper/tlsutil/testdata/nomad-foo-key.pem" + foocert2 = "../../../helper/tlsutil/testdata/nomad-bad.pem" + fookey2 = "../../../helper/tlsutil/testdata/nomad-bad-key.pem" + ) + + a := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + a.SetChecksum() + oldChecksum := a.Checksum + + a.CertFile = foocert2 + a.KeyFile = fookey2 + + a.SetChecksum() + + require.NotEqual(oldChecksum, a.Checksum) +} From 7de20ac8069d1339eabc46c0fda577fc5c1b789a Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Tue, 27 Mar 2018 18:00:41 -0400 Subject: [PATCH 2/9] set server configuration checksum on reload --- client/client.go | 1 + nomad/server.go | 1 + nomad/server_test.go | 1 + 3 files changed, 3 insertions(+) diff --git a/client/client.go b/client/client.go index f1022f444..f20b85c54 100644 --- a/client/client.go +++ b/client/client.go @@ -414,6 +414,7 @@ func (c *Client) reloadTLSConnections(newConfig *nconfig.TLSConfig) error { // decide on what type of connections to accept c.configLock.Lock() c.config.TLSConfig = newConfig + c.config.TLSConfig.SetChecksum() c.configLock.Unlock() c.connPool.ReloadTLS(tlsWrap) diff --git a/nomad/server.go b/nomad/server.go index b61b0d53d..0323ebd14 100644 --- a/nomad/server.go +++ b/nomad/server.go @@ -471,6 +471,7 @@ func (s *Server) reloadTLSConnections(newTLSConfig *config.TLSConfig) error { // access to config information, such as rpc.go, where we decide on what kind // of network connections to accept depending on the server configuration s.config.TLSConfig = newTLSConfig + s.config.TLSConfig.SetChecksum() s.rpcTLS = incomingTLS s.connPool.ReloadTLS(tlsWrap) diff --git a/nomad/server_test.go b/nomad/server_test.go index 962abbc8d..3bdb58e7e 100644 --- a/nomad/server_test.go +++ b/nomad/server_test.go @@ -427,6 +427,7 @@ func TestServer_Reload_TLSConnections_PlaintextToTLS_OnlyRPC(t *testing.T) { err := s1.reloadTLSConnections(newTLSConfig) assert.Nil(err) + assert.True(s1.config.TLSConfig.EnableRPC) assert.True(s1.config.TLSConfig.CertificateInfoIsEqual(newTLSConfig)) codec := rpcClient(t, s1) From 6e317fa6c6bcd4972344ec206d62b73bd0195ba1 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 28 Mar 2018 09:41:17 -0400 Subject: [PATCH 3/9] set TLS checksum when parsing config Refactor checksum comparison, always set checksum if it is empty --- client/client.go | 1 - command/agent/agent.go | 3 --- command/agent/command.go | 7 +++++++ nomad/server.go | 1 - nomad/structs/config/tls.go | 23 ++++++++++++++++++----- nomad/structs/config/tls_test.go | 12 ++++++++++++ 6 files changed, 37 insertions(+), 10 deletions(-) diff --git a/client/client.go b/client/client.go index f20b85c54..f1022f444 100644 --- a/client/client.go +++ b/client/client.go @@ -414,7 +414,6 @@ func (c *Client) reloadTLSConnections(newConfig *nconfig.TLSConfig) error { // decide on what type of connections to accept c.configLock.Lock() c.config.TLSConfig = newConfig - c.config.TLSConfig.SetChecksum() c.configLock.Unlock() c.connPool.ReloadTLS(tlsWrap) diff --git a/command/agent/agent.go b/command/agent/agent.go index 1bd162d8a..453b2ccf2 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -77,9 +77,6 @@ func NewAgent(config *Config, logOutput io.Writer, inmem *metrics.InmemSink) (*A InmemSink: inmem, } - // ensure that the TLS configuration is properly set up - a.config.TLSConfig.SetChecksum() - if err := a.setupConsul(config.Consul); err != nil { return nil, fmt.Errorf("Failed to initialize Consul client: %v", err) } diff --git a/command/agent/command.go b/command/agent/command.go index 314ff444e..24d6df2a3 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -321,6 +321,13 @@ func (c *Command) readConfig() *Config { c.Ui.Error("WARNING: Bootstrap mode enabled! Potentially unsafe operation.") } + // Set up the TLS configuration properly if we have one. + // XXX chelseakomlo: set up a TLSConfig New method which would wrap + // constructor-type actions like this. + if !config.TLSConfig.IsEmpty() { + config.TLSConfig.SetChecksum() + } + return config } diff --git a/nomad/server.go b/nomad/server.go index 0323ebd14..b61b0d53d 100644 --- a/nomad/server.go +++ b/nomad/server.go @@ -471,7 +471,6 @@ func (s *Server) reloadTLSConnections(newTLSConfig *config.TLSConfig) error { // access to config information, such as rpc.go, where we decide on what kind // of network connections to accept depending on the server configuration s.config.TLSConfig = newTLSConfig - s.config.TLSConfig.SetChecksum() s.rpcTLS = incomingTLS s.connPool.ReloadTLS(tlsWrap) diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index b83eec772..9bd8e65af 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -209,14 +209,27 @@ func (t *TLSConfig) CertificateInfoIsEqual(newConfig *TLSConfig) bool { if t.IsEmpty() && newConfig.IsEmpty() { return true - } - - newCertChecksum, err := createChecksumOfFiles(newConfig.CAFile, newConfig.CertFile, newConfig.KeyFile) - if err != nil { + } else if t.IsEmpty() || newConfig.IsEmpty() { return false } - return t.Checksum == newCertChecksum + // Set the checksum if it hasn't yet been set (this should happen when the + // config is parsed but this provides safety in depth) + if newConfig.Checksum == "" { + err := newConfig.SetChecksum() + if err != nil { + return false + } + } + + if t.Checksum == "" { + err := t.SetChecksum() + if err != nil { + return false + } + } + + return t.Checksum == newConfig.Checksum } // SetChecksum generates and sets the checksum for a TLS configuration diff --git a/nomad/structs/config/tls_test.go b/nomad/structs/config/tls_test.go index f51b655c1..d75b1a2d7 100644 --- a/nomad/structs/config/tls_test.go +++ b/nomad/structs/config/tls_test.go @@ -95,6 +95,18 @@ func TestTLS_CertificateInfoIsEqual_FalseWhenUnequal(t *testing.T) { } require.False(a.CertificateInfoIsEqual(b)) } + + // Assert that mismatching empty types are considered unequal + { + a := &TLSConfig{} + + b := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey2, + } + require.False(a.CertificateInfoIsEqual(b)) + } } // Certificate info should be equal when the CA file, certificate file, and key From 2fc02c183221d4a5220a53e14602640445bddb5b Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 28 Mar 2018 13:18:13 -0400 Subject: [PATCH 4/9] fix up test for file content changes --- command/agent/agent_test.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 7b0e7c2b0..292623123 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -982,10 +982,22 @@ func TestServer_ShouldReload_ReturnTrueForFileChanges(t *testing.T) { -----END CERTIFICATE----- ` - err = ioutil.WriteFile(tmpfn, []byte(newCertificate), 0644) + os.Remove(tmpfn) + err = ioutil.WriteFile(tmpfn, []byte(newCertificate), 0666) require.Nil(err) - shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC = agent.ShouldReload(agentConfig) + newAgentConfig := &Config{ + TLSConfig: &sconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: tmpfn, + KeyFile: key, + }, + } + + shouldReloadAgent, shouldReloadHTTP, shouldReloadRPC = agent.ShouldReload(newAgentConfig) require.True(shouldReloadAgent) require.True(shouldReloadHTTP) require.True(shouldReloadRPC) From 065133d716a6f74429306097f144c9d69219946e Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 28 Mar 2018 13:29:53 -0400 Subject: [PATCH 5/9] check for nil, remove unnecessary set checksum call --- command/agent/agent.go | 1 - command/agent/command.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 453b2ccf2..4916c014a 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -826,7 +826,6 @@ func (a *Agent) Reload(newConfig *Config) error { } a.config.TLSConfig = newConfig.TLSConfig a.config.TLSConfig.KeyLoader = keyloader - a.config.TLSConfig.SetChecksum() return nil } diff --git a/command/agent/command.go b/command/agent/command.go index 24d6df2a3..c0d3ce6c7 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -324,7 +324,7 @@ func (c *Command) readConfig() *Config { // Set up the TLS configuration properly if we have one. // XXX chelseakomlo: set up a TLSConfig New method which would wrap // constructor-type actions like this. - if !config.TLSConfig.IsEmpty() { + if config.TLSConfig != nil && !config.TLSConfig.IsEmpty() { config.TLSConfig.SetChecksum() } From 074683c61643c1e3b9f231092141583ab650282c Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 28 Mar 2018 17:36:55 -0400 Subject: [PATCH 6/9] output warning for error in creating TLS checksum --- command/agent/command.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/command/agent/command.go b/command/agent/command.go index c0d3ce6c7..fb5c489d7 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -325,7 +325,9 @@ func (c *Command) readConfig() *Config { // XXX chelseakomlo: set up a TLSConfig New method which would wrap // constructor-type actions like this. if config.TLSConfig != nil && !config.TLSConfig.IsEmpty() { - config.TLSConfig.SetChecksum() + if err := config.TLSConfig.SetChecksum(); err != nil { + c.Ui.Error(fmt.Sprintf("WARNING: Error when parsing TLS configuration: %v", err)) + } } return config From e8af281dd3f63f3c1145e7c8c70bbf9638302619 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 28 Mar 2018 17:54:22 -0400 Subject: [PATCH 7/9] return error when setting checksum; don't reload --- command/agent/agent.go | 6 +++++- nomad/server.go | 8 ++++++- nomad/structs/config/tls.go | 14 ++++++------- nomad/structs/config/tls_test.go | 36 ++++++++++++++++++++++---------- 4 files changed, 44 insertions(+), 20 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 4916c014a..e633fb7db 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -781,7 +781,11 @@ func (a *Agent) ShouldReload(newConfig *Config) (agent, http, rpc bool) { a.configLock.Lock() defer a.configLock.Unlock() - if !a.config.TLSConfig.CertificateInfoIsEqual(newConfig.TLSConfig) { + isEqual, err := a.config.TLSConfig.CertificateInfoIsEqual(newConfig.TLSConfig) + if err != nil { + a.logger.Println("[INFO] agent: error when parsing TLS certificate %v", err) + return false, false, false + } else if !isEqual { return true, true, true } diff --git a/nomad/server.go b/nomad/server.go index b61b0d53d..cbcabc73e 100644 --- a/nomad/server.go +++ b/nomad/server.go @@ -665,7 +665,13 @@ func (s *Server) Reload(newConfig *Config) error { } } - if !newConfig.TLSConfig.CertificateInfoIsEqual(s.config.TLSConfig) || newConfig.TLSConfig.EnableRPC != s.config.TLSConfig.EnableRPC { + tlsInfoEqual, err := newConfig.TLSConfig.CertificateInfoIsEqual(s.config.TLSConfig) + if err != nil { + s.logger.Printf("[ERR] nomad: error parsing server TLS configuration: %s", err) + return err + } + + if !tlsInfoEqual || 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/structs/config/tls.go b/nomad/structs/config/tls.go index 9bd8e65af..f40866c01 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -202,15 +202,15 @@ func (t *TLSConfig) Merge(b *TLSConfig) *TLSConfig { // 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) CertificateInfoIsEqual(newConfig *TLSConfig) bool { +func (t *TLSConfig) CertificateInfoIsEqual(newConfig *TLSConfig) (bool, error) { if t == nil || newConfig == nil { - return t == newConfig + return t == newConfig, nil } if t.IsEmpty() && newConfig.IsEmpty() { - return true + return true, nil } else if t.IsEmpty() || newConfig.IsEmpty() { - return false + return false, nil } // Set the checksum if it hasn't yet been set (this should happen when the @@ -218,18 +218,18 @@ func (t *TLSConfig) CertificateInfoIsEqual(newConfig *TLSConfig) bool { if newConfig.Checksum == "" { err := newConfig.SetChecksum() if err != nil { - return false + return false, err } } if t.Checksum == "" { err := t.SetChecksum() if err != nil { - return false + return false, err } } - return t.Checksum == newConfig.Checksum + return t.Checksum == newConfig.Checksum, nil } // SetChecksum generates and sets the checksum for a TLS configuration diff --git a/nomad/structs/config/tls_test.go b/nomad/structs/config/tls_test.go index d75b1a2d7..390039319 100644 --- a/nomad/structs/config/tls_test.go +++ b/nomad/structs/config/tls_test.go @@ -28,10 +28,12 @@ func TestTLSConfig_Merge(t *testing.T) { } func TestTLS_CertificateInfoIsEqual_TrueWhenEmpty(t *testing.T) { - assert := assert.New(t) + require := require.New(t) a := &TLSConfig{} b := &TLSConfig{} - assert.True(a.CertificateInfoIsEqual(b)) + isEqual, err := a.CertificateInfoIsEqual(b) + require.Nil(err) + require.True(isEqual) } func TestTLS_CertificateInfoIsEqual_FalseWhenUnequal(t *testing.T) { @@ -59,7 +61,9 @@ func TestTLS_CertificateInfoIsEqual_FalseWhenUnequal(t *testing.T) { CertFile: foocert2, KeyFile: fookey2, } - require.False(a.CertificateInfoIsEqual(b)) + isEqual, err := a.CertificateInfoIsEqual(b) + require.Nil(err) + require.False(isEqual) } // Assert that mismatching certificate are considered unequal @@ -76,7 +80,9 @@ func TestTLS_CertificateInfoIsEqual_FalseWhenUnequal(t *testing.T) { CertFile: foocert2, KeyFile: fookey, } - require.False(a.CertificateInfoIsEqual(b)) + isEqual, err := a.CertificateInfoIsEqual(b) + require.Nil(err) + require.False(isEqual) } // Assert that mismatching keys are considered unequal @@ -93,7 +99,9 @@ func TestTLS_CertificateInfoIsEqual_FalseWhenUnequal(t *testing.T) { CertFile: foocert, KeyFile: fookey2, } - require.False(a.CertificateInfoIsEqual(b)) + isEqual, err := a.CertificateInfoIsEqual(b) + require.Nil(err) + require.False(isEqual) } // Assert that mismatching empty types are considered unequal @@ -105,7 +113,9 @@ func TestTLS_CertificateInfoIsEqual_FalseWhenUnequal(t *testing.T) { CertFile: foocert, KeyFile: fookey2, } - require.False(a.CertificateInfoIsEqual(b)) + isEqual, err := a.CertificateInfoIsEqual(b) + require.Nil(err) + require.False(isEqual) } } @@ -130,11 +140,13 @@ func TestTLS_CertificateInfoIsEqual_TrueWhenEqual(t *testing.T) { CertFile: foocert, KeyFile: fookey, } - require.True(a.CertificateInfoIsEqual(b)) + isEqual, err := a.CertificateInfoIsEqual(b) + require.Nil(err) + require.True(isEqual) } func TestTLS_Copy(t *testing.T) { - assert := assert.New(t) + require := require.New(t) const ( cafile = "../../../helper/tlsutil/testdata/ca.pem" foocert = "../../../helper/tlsutil/testdata/nomad-foo.pem" @@ -148,15 +160,17 @@ func TestTLS_Copy(t *testing.T) { a.SetChecksum() aCopy := a.Copy() - assert.True(a.CertificateInfoIsEqual(aCopy)) + isEqual, err := a.CertificateInfoIsEqual(aCopy) + require.Nil(err) + require.True(isEqual) } // GetKeyLoader should always return an initialized KeyLoader for a TLSConfig // object func TestTLS_GetKeyloader(t *testing.T) { - assert := assert.New(t) + require := require.New(t) a := &TLSConfig{} - assert.NotNil(a.GetKeyLoader()) + require.NotNil(a.GetKeyLoader()) } func TestTLS_SetChecksum(t *testing.T) { From 6665206c9ee3f2e4e1c51ed15a413f73481fea3e Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 28 Mar 2018 18:31:35 -0400 Subject: [PATCH 8/9] add test to assert invalid files return error --- nomad/structs/config/tls_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/nomad/structs/config/tls_test.go b/nomad/structs/config/tls_test.go index 390039319..698855e08 100644 --- a/nomad/structs/config/tls_test.go +++ b/nomad/structs/config/tls_test.go @@ -117,6 +117,24 @@ func TestTLS_CertificateInfoIsEqual_FalseWhenUnequal(t *testing.T) { require.Nil(err) require.False(isEqual) } + + // Assert that invalid files return an error + { + a := &TLSConfig{ + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey2, + } + + b := &TLSConfig{ + CAFile: cafile, + CertFile: "invalid_file", + KeyFile: fookey2, + } + isEqual, err := a.CertificateInfoIsEqual(b) + require.NotNil(err) + require.False(isEqual) + } } // Certificate info should be equal when the CA file, certificate file, and key From 5d866366a724facb2edf97f30242d1d92fa4b52a Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 28 Mar 2018 19:11:51 -0400 Subject: [PATCH 9/9] make check fix --- command/agent/agent.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index e633fb7db..b8b55c295 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -783,7 +783,7 @@ func (a *Agent) ShouldReload(newConfig *Config) (agent, http, rpc bool) { isEqual, err := a.config.TLSConfig.CertificateInfoIsEqual(newConfig.TLSConfig) if err != nil { - a.logger.Println("[INFO] agent: error when parsing TLS certificate %v", err) + a.logger.Printf("[INFO] agent: error when parsing TLS certificate %v", err) return false, false, false } else if !isEqual { return true, true, true