Client/Server TLS dynamic reload #3492

chelseakomlo · 2017-11-02T18:24:34Z

This PR includes:

Upgrading/downgrading from non-TLS connections to TLS via SIGHUP
Closing long-lived connections on reload (HTTP, RPC, Raft)

dadgar · 2017-11-15T00:09:56Z

command/agent/agent.go

+				a.logger.Printf("[WARN] agent: Issue reloading the server's TLS Configuration, consider a full system restart: %v", err.Error())
+				return err, false
+			}
+		} else if c := a.Client(); c != nil {


Could be running as both so don't use else if use another if

preetapan

Made one pass through

preetapan · 2017-11-21T17:24:02Z

client/client.go

@@ -363,6 +364,25 @@ func (c *Client) init() error {
 	return nil
 }

+// ReloadTLSConnectoins allows a client to reload RPC connections if the


preetapan · 2017-11-21T18:35:58Z

client/client.go

+
+	return nil
+}
+


Is the only place that can throw any errors here the OutgoingTLSWrapper method?

preetapan · 2017-11-21T18:39:11Z

client/client_test.go

+		QueryOptions: structs.QueryOptions{Region: "dc1"},
+	}
+	var out structs.SingleNodeResponse
+	testutil.AssertUntil(100*time.Millisecond,


what is this trying to establish, looks like it always expects Node.GetNode to fail?

This makes several checks until either the request succeeds or it hits the predetermined timeout.

preetapan · 2017-11-21T18:41:02Z

command/agent/command.go

+	if err != nil {
+		return err
+	}
+	c.httpServer = http


This assignment seems unsafe without a mutex around it, what if two different calls to this reload method happen close enough in time in quick succession?

Great catch, fixing.

preetapan · 2017-11-21T19:37:49Z

client/client.go

+	c.config.TLSConfig = newConfig
+	c.configLock.Unlock()
+
+	if c.config.TLSConfig.EnableRPC {


Why is this only checking for enableRPC and not for enableHttp?

HTTP connections are handled at the HTTP server level

preetapan · 2017-11-21T19:42:15Z

command/agent/command.go

+			c.agent.logger.Printf("[ERR] agent: failed to reload http server: %v", err)
+			return
+		}
+		c.agent.logger.Println("[INFO] agent: successfully restarted the HTTP server")


This logic looks identical to the reloadHTTPServerOnConfigChange method, and that method is unused, did you forget to make the replacement maybe?

Thanks! I did, fixed that up.

preetapan · 2017-11-21T19:45:32Z

nomad/pool.go

+
+	oldPool := p.pool
+	for _, conn := range oldPool {
+		conn.Close()


this is ignoring the error from conn.Close(), but so is the rest of this file. Need some context from @dadgar or @schmichael as to why close errors are ignored.

As far as I know there's nothing useful to do with Close errors. In some circumstances logging them might be useful, but I can't imagine what actionable information errors from this Close could contain? I don't think it's even useful for catching programming errors like closing closed connections as there's always the possibility the connection may close concurrently due to peer disconnects or other issues.

If somebody has an example of a useful Close error I'd love to know it!

preetapan · 2017-11-21T22:32:13Z

vendor/github.com/hashicorp/raft/net_transport.go

+		close(n.shutdownCh)
+		n.shutdown = true
+	}
+


Create an issue in the raft library and open a PR for this there and vendor the changes in. Otherwise, this will drift when we update raft.

fix up linting

dadgar · 2018-01-09T17:50:42Z

website/source/guides/securing-nomad.html.md

@@ -472,6 +472,17 @@ NOTE: Dynamically reloading certificates will _not_ close existing connections.
 If you need to rotate certificates due to a security incident, you will still
 need to completely shutdown and restart the Nomad agent.

+## Migrating a cluster to TLS
+
+Nomad supports dynamically reloading a server to move from plaintext to TLS.


Nomad supports dynamically reloading it's TLS configuration. To reload Nomad's configuration, first update the configuration file and then send the Nomad agent a SIGHUP signal. Note that this will only reload a subset of the configuration file, including the TLS configuration.

When reloading the configuration, if there is a change to the TLS configuration, the agent will reload all network connections and when establishing new connections, will use the new configuration. This process
works for both upgrading and downgrading TLS (but we recommend upgrading).

dadgar · 2018-01-09T17:53:45Z

client/client.go

+	var tlsWrap tlsutil.RegionWrapper
+	if newConfig != nil && newConfig.EnableRPC {
+		tw, err := c.config.NewTLSConfiguration(newConfig).OutgoingTLSWrapper()
+


Remove blank line here

dadgar · 2018-01-09T17:56:23Z

client/client_test.go

+	assert := assert.New(t)
+
+	s1, addr := testServer(t, func(c *nomad.Config) {
+		c.Region = "foo"


I believe this is the wrong region: https://github.com/hashicorp/nomad/pull/3721/files#diff-769638ff36c019b3e4c9eab992c23f60R190

Further I don't think it is necessary to set it for this test

dadgar · 2018-01-09T17:57:12Z

client/client_test.go

+		CertFile:             foocert,
+		KeyFile:              fookey,
+	}
+


Don't you want to assert the node registered using plaintext?

dadgar · 2018-01-09T17:58:47Z

client/client_test.go

+	defer c1.Shutdown()
+
+	newConfig := &nconfig.TLSConfig{}
+


Assert it isn't registered

dadgar · 2018-01-09T18:02:07Z

client/config/config.go

+// NewTLSConfiguration returns a TLSUtil Config for a new TLS config object
+// This allows a TLSConfig object to be created without first explicitly
+// setting it
+func (c *Config) NewTLSConfiguration(tlsConfig *config.TLSConfig) *tlsutil.Config {


Can you remove this and put it back to how it was. Further if you wanted to have these separate, this one shouldn't be a method on the client Config. It should be func NewTLSConfiguration

dadgar · 2018-01-09T19:38:12Z

command/agent/command.go

-	err := c.agent.Reload(newConf)
-	if err != nil {
-		c.agent.logger.Printf("[ERR] agent: failed to reload the config: %v", err)
+	shouldReload := c.agent.ShouldReload(newConf)


Should this maybe return a separate boolean for whether the http server should be reloaded? As we add stuff, you could potential reload the agent w/o needing to reload the HTTP server

dadgar · 2018-01-09T19:39:26Z

command/agent/agent_test.go

+
+func TestServer_Reload_TLS_WithNilConfiguration(t *testing.T) {
+	t.Parallel()
+	assert := assert.New(t)


Moving forward can you use https://godoc.org/github.com/stretchr/testify/require. It will hard fail on the first failed assertion

dadgar · 2018-01-09T19:51:58Z

command/agent/http_test.go

+	}
+
+	s := makeHTTPServer(t, func(c *Config) {
+		c.TLSConfig = newConfig.TLSConfig


Don't you have to set the region to regionFoo? I don't understand the test setup or how it is passing. The region is wrong and the server starts with TLS and it is being reloaded with the same config as it is originally started as

dadgar · 2018-01-09T19:52:18Z

nomad/config.go

+// newTLSConfig  returns a TLSUtil Config based on the server configuration
+// This is useful for creating a TLSConfig object without explictely setting it
+// on the config.
+func (c *Config) newTLSConfig(newConf *config.TLSConfig) *tlsutil.Config {


Bump, lets put this back to how it was

dadgar · 2018-01-09T21:45:18Z

nomad/server.go

+	s.listenerCh = make(chan struct{})
+	list, err := net.ListenTCP("tcp", s.config.RPCAddr)
+	if err != nil || list == nil {
+		s.logger.Printf("[ERR] nomad: No TLS listener to reload")


Is the list == nil a valid case? Why not just check and log the error?

dadgar · 2018-01-09T21:50:08Z

nomad/server.go

+
+	if !newConfig.TLSConfig.Equals(s.config.TLSConfig) {
+		if err := s.reloadTLSConnections(newConfig.TLSConfig); err != nil {
+			s.logger.Printf("[DEBUG] nomad: reloading server TLS configuration")


I think you wanted to log the error?

dadgar · 2018-01-09T21:51:48Z

nomad/server.go

@@ -776,6 +884,7 @@ func (s *Server) setupRPC(tlsWrap tlsutil.RegionWrapper) error {
 		return err
 	}
 	s.rpcListener = list
+	s.listenerCh = make(chan struct{})


Use createRPCListener? There is duplicated code

dadgar · 2018-01-09T21:53:58Z

nomad/server.go

+	tlsConf := s.config.newTLSConfig(newTLSConfig)
+	incomingTLS, tlsWrap, err := getTLSConf(newTLSConfig.EnableRPC, tlsConf)
+	if err != nil {
+		s.logger.Printf("[ERR] nomad: unable to reset TLS context")


Log the errors. Can you search through the PR and make sure all your logs have the errors logged.
"unable to reset TLS context" is slightly vague. Can you make it more specific to the fact that it is creating the TLS config that failed.

dadgar · 2018-01-09T21:55:10Z

nomad/server.go

+	s.configLock.Unlock()
+
+	if s.rpcCancel == nil {
+		s.logger.Printf("[ERR] nomad: No TLS Context to reset")


This isn't about the context, it is there is no RPC servers. Structure so that the log reuses the returned error

dadgar · 2018-01-09T22:00:13Z

nomad/server.go

+	// CLose existing streams
+	wrapper := tlsutil.RegionSpecificWrapper(s.config.Region, tlsWrap)
+	s.raftLayer = NewRaftLayer(s.rpcAdvertise, wrapper)
+


s.raftTransport.Reload(s.raftLayer)?

Calls into questions the tests, if they are passing

dadgar · 2018-01-09T22:00:57Z

nomad/server.go

+	s.startRPCListener()
+
+	time.Sleep(3 * time.Second)
+	if err != nil {


Why is this not attached to the call site that returns the error

dadgar · 2018-01-09T22:01:49Z

nomad/server.go

+	s.connPool.ReloadTLS(tlsWrap)
+
+	// reinitialize our rpc listener
+	s.rpcListener.Close()


Can you do all the RPC stuff and then (with a separate comment) start the Raft stuff

dadgar · 2018-01-09T22:04:35Z

vendor/github.com/hashicorp/raft/net_transport.go

 	return trans
 }

+// Pause closes the current stream for a NetworkTransport instance
+func (n *NetworkTransport) Pause() {


I would prefer these to be called CloseStream and UseStream and document that calling CloseStream must be done before giving a new stream layer

dadgar · 2018-01-09T22:04:44Z

vendor/github.com/hashicorp/raft/net_transport.go

 		// Accept incoming connections
 		conn, err := n.stream.Accept()
 		if err != nil {
 			if n.IsShutdown() {
 				return
 			}
+			// TODO Getting an error here


You probably need to check the ctx again as Accept is blocking

…http server

close second goroutine in raft-net

dadgar · 2018-01-18T19:13:21Z

nomad/server.go


-	time.Sleep(3 * time.Second)
+	time.Sleep(500 * time.Millisecond)


dadgar · 2018-01-18T19:13:58Z

nomad/server_test.go

-			assert.NotNil(out)
-			assert.Equal(out.CreateIndex, resp.JobModifyIndex)
+		for _, serv := range servers {
+			testutil.WaitForResult(func() (bool, error) {


wait for leader on positive cases

dadgar · 2018-01-18T19:14:57Z

vendor/vendor.json

-		{"path":"gopkg.in/tomb.v1","checksumSHA1":"TO8baX+t1Qs7EmOYth80MkbKzFo=","revision":"dd632973f1e7218eb1089048e0798ec9ae7dceb8","revisionTime":"2014-10-24T13:56:13Z"},
-		{"path":"gopkg.in/tomb.v2","checksumSHA1":"WiyCOMvfzRdymImAJ3ME6aoYUdM=","revision":"14b3d72120e8d10ea6e6b7f87f7175734b1faab8","revisionTime":"2014-06-26T14:46:23Z"},
-		{"path":"gopkg.in/yaml.v2","checksumSHA1":"12GqsW8PiRPnezDDy0v4brZrndM=","revision":"a5b47d31c556af34a302ce5d659e6fea44d90de0","revisionTime":"2016-09-28T15:37:09Z"}
+		{


make vendorfmt

preetapan · 2018-01-19T23:14:56Z

vendor/vendor.json

@@ -162,7 +162,7 @@
 		{"path":"github.com/hashicorp/logutils","revision":"0dc08b1671f34c4250ce212759ebd880f743d883"},
 		{"path":"github.com/hashicorp/memberlist","checksumSHA1":"1zk7IeGClUqBo+Phsx89p7fQ/rQ=","revision":"23ad4b7d7b38496cd64c241dfd4c60b7794c254a","revisionTime":"2017-02-08T21:15:06Z"},
 		{"path":"github.com/hashicorp/net-rpc-msgpackrpc","revision":"a14192a58a694c123d8fe5481d4a4727d6ae82f3"},
-		{"path":"github.com/hashicorp/raft","checksumSHA1":"ecpaHOImbL/NaivWrUDUUe5461E=","revision":"3a6f3bdfe4fc69e300c6d122b1a92051af6f0b95","revisionTime":"2017-08-07T22:22:24Z"},
+		{"path":"github.com/hashicorp/raft","checksumSHA1":"zkA9uvbj1BdlveyqXpVTh1N6ers=","revision":"077966dbc90f342107eb723ec52fdb0463ec789b","revisionTime":"2018-01-17T20:29:25Z","version":"=master","versionExact":"master"},


should the version be just "master" not "=master"?

Good point- this was from what I thought the govendor syntax needed to be. Fixed.

dadgar

Add to changelog please

github-actions · 2023-03-13T02:15:32Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

chelseakomlo force-pushed the f-client-tls-reload branch 5 times, most recently from 070e632 to e27c2db Compare November 3, 2017 20:48

chelseakomlo changed the title ~~WIP: Client TLS dynamic reload~~ Client TLS dynamic reload Nov 3, 2017

chelseakomlo force-pushed the f-client-tls-reload branch from e25aadb to f38e864 Compare November 3, 2017 22:35

chelseakomlo changed the title ~~Client TLS dynamic reload~~ Client/Server TLS dynamic reload Nov 4, 2017

chelseakomlo mentioned this pull request Nov 4, 2017

Reload server TLS configuration on the fly #3489

Closed

chelseakomlo force-pushed the f-client-tls-reload branch 2 times, most recently from be7abb1 to ed07520 Compare November 6, 2017 14:53

SoMuchToGrok mentioned this pull request Nov 6, 2017

[feature] Support graceful reloading of TLS configuration #3247

Closed

chelseakomlo changed the title ~~Client/Server TLS dynamic reload~~ WIP: Client/Server TLS dynamic reload Nov 6, 2017

chelseakomlo force-pushed the f-client-tls-reload branch 6 times, most recently from 392b066 to 576b595 Compare November 13, 2017 18:19

chelseakomlo force-pushed the f-client-tls-reload branch from 576b595 to b7d6488 Compare November 13, 2017 18:59

dadgar reviewed Nov 15, 2017

View reviewed changes

chelseakomlo force-pushed the f-client-tls-reload branch from b7d6488 to ad970c3 Compare November 20, 2017 23:38

chelseakomlo changed the title ~~WIP: Client/Server TLS dynamic reload~~ Client/Server TLS dynamic reload Nov 21, 2017

chelseakomlo requested a review from preetapan November 21, 2017 17:21

preetapan reviewed Nov 21, 2017

View reviewed changes

chelseakomlo force-pushed the f-client-tls-reload branch from 3c50f91 to 1288b53 Compare November 28, 2017 19:06

reload raft transport layer

11089b2

fix up linting

chelseakomlo force-pushed the f-client-tls-reload branch from c6c6318 to 11089b2 Compare January 8, 2018 19:52

add documentation

21c1c1d

dadgar requested changes Jan 9, 2018

View reviewed changes

chelseakomlo added 2 commits January 16, 2018 07:34

adding additional test assertions; differentiate reloading agent and …

bbc5686

…http server

refactor creating a new tls configuration

8de260f

tmessi mentioned this pull request Jan 16, 2018

[feature] Reload TLSConfig.CAFile on SIGHUP #3746

Closed

feedback from code review

d97c91c

chelseakomlo force-pushed the f-client-tls-reload branch 4 times, most recently from 098f300 to 1dab7b5 Compare January 17, 2018 00:02

fixing up raft reload tests

0805c41

close second goroutine in raft-net

chelseakomlo force-pushed the f-client-tls-reload branch from 1dab7b5 to 0805c41 Compare January 17, 2018 15:34

allow for similar error messages for closed connections

028ba9f

chelseakomlo force-pushed the f-client-tls-reload branch 2 times, most recently from a08ab45 to 231e9dd Compare January 18, 2018 13:01

dadgar requested changes Jan 18, 2018

View reviewed changes

chelseakomlo added 2 commits January 19, 2018 17:00

vendor raft to master branch

d443098

swap raft layer tls wrapper

5170301

chelseakomlo force-pushed the f-client-tls-reload branch from 231e9dd to 5170301 Compare January 19, 2018 22:07

preetapan reviewed Jan 19, 2018

View reviewed changes

specified version of raft should be master

cab0830

dadgar approved these changes Jan 22, 2018

View reviewed changes

add changelog

c6eee01

chelseakomlo merged commit 9d006ec into master Jan 23, 2018

chelseakomlo deleted the f-client-tls-reload branch January 23, 2018 10:51

github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023


		time.Sleep(3 * time.Second)
		time.Sleep(500 * time.Millisecond)

Client/Server TLS dynamic reload #3492

Client/Server TLS dynamic reload #3492

Conversation

chelseakomlo commented Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

preetapan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadgar left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2023

chelseakomlo commented Nov 2, 2017 •

edited

Loading

preetapan Nov 21, 2017 •

edited

Loading