From 11206c7c3e81e6f0ed86eb797d074fd683dbfff0 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 5 Apr 2017 20:50:35 -0700 Subject: [PATCH] Require TLS for server RPC when enabled Fixes #2525 We used to be checking a RequireTLS field that was never set. Instead we can just check the TLSConfig.EnableRPC field and require TLS if it's enabled. Added a few unfortunately slow integration tests to assert the intended behavior of misconfigured RPC TLS. Also disable a lot of noisy test logging when -v isn't specified. --- client/client_test.go | 93 +++++++++++++++++++++++++++++++++++++++++++ nomad/config.go | 3 -- nomad/rpc.go | 4 +- nomad/server_test.go | 48 +++++++++++++++++++++- 4 files changed, 142 insertions(+), 6 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 6f883b129216..d08a5e9f0bf0 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -21,6 +21,7 @@ import ( "github.com/hashicorp/nomad/nomad" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" + nconfig "github.com/hashicorp/nomad/nomad/structs/config" "github.com/hashicorp/nomad/testutil" "github.com/mitchellh/hashstructure" @@ -382,6 +383,98 @@ func TestClient_Drivers_WhitelistBlacklistCombination(t *testing.T) { } } +// TestClient_MixedTLS asserts that when a server is running with TLS enabled +// it will reject any RPC connections from clients that lack TLS. See #2525 +func TestClient_MixedTLS(t *testing.T) { + const ( + cafile = "../helper/tlsutil/testdata/ca.pem" + foocert = "../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../helper/tlsutil/testdata/nomad-foo-key.pem" + ) + s1, addr := testServer(t, func(c *nomad.Config) { + c.TLSConfig = &nconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + }) + defer s1.Shutdown() + testutil.WaitForLeader(t, s1.RPC) + + c1 := testClient(t, func(c *config.Config) { + c.Servers = []string{addr} + }) + defer c1.Shutdown() + + req := structs.NodeSpecificRequest{ + NodeID: c1.Node().ID, + QueryOptions: structs.QueryOptions{Region: "global"}, + } + var out structs.SingleNodeResponse + deadline := time.Now().Add(1234 * time.Millisecond) + for time.Now().Before(deadline) { + err := c1.RPC("Node.GetNode", &req, &out) + if err == nil { + t.Fatalf("client RPC succeeded when it should have failed:\n%+v", out) + } + } +} + +// TestClient_BadTLS asserts that when a client and server are running with TLS +// enabled -- but their certificates are signed by different CAs -- they're +// unable to communicate. +func TestClient_BadTLS(t *testing.T) { + const ( + cafile = "../helper/tlsutil/testdata/ca.pem" + foocert = "../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../helper/tlsutil/testdata/nomad-foo-key.pem" + badca = "../helper/tlsutil/testdata/ca-bad.pem" + badcert = "../helper/tlsutil/testdata/nomad-bad.pem" + badkey = "../helper/tlsutil/testdata/nomad-bad-key.pem" + ) + s1, addr := testServer(t, func(c *nomad.Config) { + c.TLSConfig = &nconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + }) + defer s1.Shutdown() + testutil.WaitForLeader(t, s1.RPC) + + c1 := testClient(t, func(c *config.Config) { + c.Servers = []string{addr} + c.TLSConfig = &nconfig.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: badca, + CertFile: badcert, + KeyFile: badkey, + } + }) + defer c1.Shutdown() + + req := structs.NodeSpecificRequest{ + NodeID: c1.Node().ID, + QueryOptions: structs.QueryOptions{Region: "global"}, + } + var out structs.SingleNodeResponse + deadline := time.Now().Add(1234 * time.Millisecond) + for time.Now().Before(deadline) { + err := c1.RPC("Node.GetNode", &req, &out) + if err == nil { + t.Fatalf("client RPC succeeded when it should have failed:\n%+v", out) + } + } +} + func TestClient_Register(t *testing.T) { s1, _ := testServer(t, nil) defer s1.Shutdown() diff --git a/nomad/config.go b/nomad/config.go index 3a54217d0dc1..e843ba89503b 100644 --- a/nomad/config.go +++ b/nomad/config.go @@ -92,9 +92,6 @@ type Config struct { // RaftTimeout is applied to any network traffic for raft. Defaults to 10s. RaftTimeout time.Duration - // RequireTLS ensures that all RPC traffic is protected with TLS - RequireTLS bool - // SerfConfig is the configuration for the serf cluster SerfConfig *serf.Config diff --git a/nomad/rpc.go b/nomad/rpc.go index 7aaf225c094b..9ba2156d19c6 100644 --- a/nomad/rpc.go +++ b/nomad/rpc.go @@ -97,8 +97,8 @@ func (s *Server) handleConn(conn net.Conn, isTLS bool) { return } - // Enforce TLS if VerifyIncoming is set - if s.config.RequireTLS && !isTLS && RPCType(buf[0]) != rpcTLS { + // Enforce TLS if EnableRPC is set + if s.config.TLSConfig.EnableRPC && !isTLS && RPCType(buf[0]) != rpcTLS { s.logger.Printf("[WARN] nomad.rpc: Non-TLS connection attempted with RequireTLS set") conn.Close() return diff --git a/nomad/server_test.go b/nomad/server_test.go index 94d245cfd1c4..902498a1d4ae 100644 --- a/nomad/server_test.go +++ b/nomad/server_test.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/nomad/command/agent/consul" "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/nomad/structs/config" "github.com/hashicorp/nomad/testutil" ) @@ -62,6 +63,11 @@ func testServer(t *testing.T, cb func(*Config)) *Server { f := false config.VaultConfig.Enabled = &f + // Squelch output when -v isn't specified + if !testing.Verbose() { + config.LogOutput = ioutil.Discard + } + // Invoke the callback if any if cb != nil { cb(config) @@ -71,7 +77,7 @@ func testServer(t *testing.T, cb func(*Config)) *Server { config.RaftConfig.StartAsLeader = !config.DevDisableBootstrap shutdownCh := make(chan struct{}) - logger := log.New(config.LogOutput, "", log.LstdFlags) + logger := log.New(config.LogOutput, fmt.Sprintf("[%s] ", config.NodeName), log.LstdFlags) consulSyncer, err := consul.NewSyncer(config.ConsulConfig, shutdownCh, logger) if err != nil { t.Fatalf("err: %v", err) @@ -107,6 +113,46 @@ func TestServer_RPC(t *testing.T) { } } +func TestServer_RPC_MixedTLS(t *testing.T) { + const ( + cafile = "../helper/tlsutil/testdata/ca.pem" + foocert = "../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../helper/tlsutil/testdata/nomad-foo-key.pem" + ) + s1 := testServer(t, func(c *Config) { + c.BootstrapExpect = 3 + c.DevDisableBootstrap = true + c.TLSConfig = &config.TLSConfig{ + EnableHTTP: true, + EnableRPC: true, + VerifyServerHostname: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + }) + defer s1.Shutdown() + + cb := func(c *Config) { + c.BootstrapExpect = 3 + c.DevDisableBootstrap = true + } + s2 := testServer(t, cb) + defer s2.Shutdown() + s3 := testServer(t, cb) + defer s3.Shutdown() + + testJoin(t, s1, s2, s3) + testutil.WaitForLeader(t, s2.RPC) + testutil.WaitForLeader(t, s3.RPC) + + // s1 shouldn't be able to join + leader := "" + if err := s1.RPC("Status.Leader", &structs.GenericRequest{}, &leader); err == nil { + t.Errorf("expected a connection error from TLS server but received none; found leader: %q", leader) + } +} + func TestServer_Regions(t *testing.T) { // Make the servers s1 := testServer(t, func(c *Config) {