From 6e6d6b7e3384a55d31392349ff05e9bae51d7944 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 26 Mar 2018 15:55:32 -0400 Subject: [PATCH] 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) +}