Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Require TLS for server RPC when enabled #2526

Merged
merged 3 commits into from
Apr 7, 2017
Merged

Require TLS for server RPC when enabled #2526

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11206c7
Require TLS for server RPC when enabled
schmichael Apr 6, 2017
985bde8
Improve test timings
schmichael Apr 6, 2017
df30a11
Create AssertUntil helper func
schmichael Apr 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is 1234 coming from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thin air. I would love a better way of testing "ensure X doesn't happen" (where "X" in this case is successfully making the RPC call).

I guess I should put a sleep in this loop as well so it's not spinning as fast as possible.

I'll poke around for a minute and try to come up with something a little better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you may just want to add a method to testutil that takes a total duration and a wait between duration and runs a function to assert. Then run for 2 seconds hitting the api every 250ms or something ensuring it doesn't succeed.

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()
Expand Down
3 changes: 0 additions & 3 deletions nomad/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions nomad/rpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
48 changes: 47 additions & 1 deletion nomad/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down