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.

Author: schmichael

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()

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
 

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

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) {