Reload tls config #5419

hanshasselberg · 2019-03-04T14:12:17Z

This PR introduces reloading tls configuration. The interesting thing about this is that it is not about reloading the configuration, which is easy. But about making sure the reload actually has an effect.

describe in which cases it works
- reload certificates
- reload CAs
- reload verify_incoming, verify_outgoing, verify_server_hostname
describe limitations
- it is not possible to turn TLS on or off! you want to have it enabled and then reload
document accordingly on website

hanshasselberg · 2019-03-06T12:30:43Z

agent/agent.go

@@ -3559,6 +3550,10 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error {
 	// the checks and service registrations.
 	a.loadTokens(newCfg)

+	if err := a.tlsConfigurator.Update(newCfg.ToTLSUtilConfig()); err != nil {
+		return fmt.Errorf("Failed reloading tls configuration: %s", err)
+	}


This is one of the cornerstones of this PR, the config is being updated here! And every tls.Config created afterwards will have the updates.

hanshasselberg · 2019-03-06T12:31:27Z

agent/config/runtime.go

 		VerifyIncoming:           c.VerifyIncoming,
 		VerifyIncomingRPC:        c.VerifyIncomingRPC,
 		VerifyIncomingHTTPS:      c.VerifyIncomingHTTPS,
 		VerifyOutgoing:           c.VerifyOutgoing,
+		VerifyServerHostname:     c.VerifyServerHostname,


Ups, how could I forget that?!

hanshasselberg · 2019-03-06T12:32:54Z

tlsutil/config.go

 	}
+	return c, nil
+}


NewConfigurator uses Update to set the config because Update also performs all the checks!

By the time it passed the checks in Update there is nothing left that could go wrong with generating a tls.Config.

hanshasselberg · 2019-03-06T12:35:00Z

tlsutil/config.go

+	tlsConfig.ClientCAs = c.cas
+	tlsConfig.RootCAs = c.cas
+
+	tlsConfig.MinVersion = TLSLookup[c.base.TLSMinVersion]


This is possible because TLSLookup also contains "" with golang's default. And because check makes sure the version correctly matches.

That PR comment would probably make a good code comment for future readers!

hanshasselberg · 2019-03-06T12:37:31Z

tlsutil/config.go

+	config := c.commonTLSConfig(c.base.VerifyIncomingRPC)
+	config.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) {
+		return c.IncomingRPCConfig(), nil
+	}


Using GetConfigForClient is another cornerstone of this PR, it will query for a new configuration for each new client. It enables reloading tls config for the RPC server that accepts connections in consul.

hanshasselberg · 2019-03-06T12:39:05Z

tlsutil/config.go

+	config := c.commonTLSConfig(c.base.VerifyIncomingHTTPS)
+	config.GetConfigForClient = func(hello *tls.ClientHelloInfo) (*tls.Config, error) {
+		config := c.IncomingHTTPSConfig()
+		config.NextProtos = hello.SupportedProtos


We have a test that uses a http2 client and for whatever reason, using GetConfigForClient made that test fail. Until I put in the supported protocols.

Could this cause issues if a client requests protos ["foo", "http"]? Wouldn't copying the values indicate to the Go TLS library that "foo" is the preferred protocol.?

hanshasselberg · 2019-03-06T12:40:49Z

tlsutil/config.go

-	tlsConfig, err := c.OutgoingRPCConfig()
-	if err != nil {
-		return nil, err
-	}


And this is another important line because creating the actual configuration is postponed until later. Otherwise it outgoing connections would never pick up changes in the configuration.

hanshasselberg · 2019-03-06T12:41:33Z

tlsutil/config.go

+	verifyServerHostname := c.base.VerifyServerHostname
+	verifyOutgoing := c.base.VerifyOutgoing
+	domain := c.base.Domain
+	c.RUnlock()


Carefully read everything we need from the config and guard that with a lock.

banks

This looks awesome @i0rek! 🎉

Locking issue needs a fix but it should be trivial.

I think the caveats/limitations are reasonable for now but agree we should document them.

One thing I didn't see in the tests here (but did skim so could be wrong) - do we have tests that cover actual verification logic for certs?

Like attempt to use the TLS config to actuall establish TLS connects as both the client and server with both good and bad certs/hostnames etc and check the desired outcome?

That seems like a pretty important thing to have confidence that the security properties we are assuming actually hold through the refactor etc.

banks · 2019-03-06T13:22:01Z

tlsutil/config.go

-	defer c.Unlock()
-	return c.checks[id]
+	c.RLock()
+	config := c.OutgoingRPCConfig()


Hmm OutgoingRPCConfig also takes an RLock on the same lock.

Multiple Read locks are allowed so this won't obvious deadlock and tests will pass, but there is a subtle bug here due to Go's RWMutex implementation that I happened to read about the other day.

See https://stackoverflow.com/questions/30547916/goroutine-blocks-when-calling-rwmutex-rlock-twice-after-an-rwmutex-unlock

This means that it is always unsafe to call RLock on a RWMutex that the same goroutine already has read locked.

I don't know if -race is smart enough to spot that but we should avoid it here explicitly - maybe with an internal outgoingRPCConfigLocked implementation?

Great catch! -race is not smart enough. :(
I am restructuring the code a little so that all reading happens from independent small functions that can be locked without having to think about nested locking.

hanshasselberg · 2019-03-08T14:39:52Z

@banks All the existing tests that check with a server and a client are still there and I also added one:

consul/tlsutil/config_test.go

Lines 57 to 226 in cab155a

    
           func TestConfigurator_outgoingWrapper_OK(t *testing.T) { 
        
           	config := Config{ 
        
           		CAFile:               "../test/hostname/CertAuth.crt", 
        
           		CertFile:             "../test/hostname/Alice.crt", 
        
           		KeyFile:              "../test/hostname/Alice.key", 
        
           		VerifyServerHostname: true, 
        
           		VerifyOutgoing:       true, 
        
           		Domain:               "consul", 
        
           	} 
        
           	client, errc := startTLSServer(&config) 
        
           	if client == nil { 
        
           		t.Fatalf("startTLSServer err: %v", <-errc) 
        
           	} 
        
           	c, err := NewConfigurator(config, nil) 
        
           	require.NoError(t, err) 
        
           	wrap := c.OutgoingRPCWrapper() 
        
           	require.NotNil(t, wrap) 
        
           	tlsClient, err := wrap("dc1", client) 
        
           	require.NoError(t, err) 
        
           	defer tlsClient.Close() 
        
           	err = tlsClient.(*tls.Conn).Handshake() 
        
           	require.NoError(t, err) 
        
           	err = <-errc 
        
           	require.NoError(t, err) 
        
           } 
        
           func TestConfigurator_outgoingWrapper_noverify_OK(t *testing.T) { 
        
           	config := Config{ 
        
           		CAFile:   "../test/hostname/CertAuth.crt", 
        
           		CertFile: "../test/hostname/Alice.crt", 
        
           		KeyFile:  "../test/hostname/Alice.key", 
        
           		Domain:   "consul", 
        
           	} 
        
           	client, errc := startTLSServer(&config) 
        
           	if client == nil { 
        
           		t.Fatalf("startTLSServer err: %v", <-errc) 
        
           	} 
        
           	c, err := NewConfigurator(config, nil) 
        
           	require.NoError(t, err) 
        
           	wrap := c.OutgoingRPCWrapper() 
        
           	require.NotNil(t, wrap) 
        
           	tlsClient, err := wrap("dc1", client) 
        
           	require.NoError(t, err) 
        
           	defer tlsClient.Close() 
        
           	err = tlsClient.(*tls.Conn).Handshake() 
        
           	require.NoError(t, err) 
        
           	err = <-errc 
        
           	require.NoError(t, err) 
        
           } 
        
           func TestConfigurator_outgoingWrapper_BadDC(t *testing.T) { 
        
           	config := Config{ 
        
           		CAFile:               "../test/hostname/CertAuth.crt", 
        
           		CertFile:             "../test/hostname/Alice.crt", 
        
           		KeyFile:              "../test/hostname/Alice.key", 
        
           		VerifyServerHostname: true, 
        
           		VerifyOutgoing:       true, 
        
           		Domain:               "consul", 
        
           	} 
        
           	client, errc := startTLSServer(&config) 
        
           	if client == nil { 
        
           		t.Fatalf("startTLSServer err: %v", <-errc) 
        
           	} 
        
           	c, err := NewConfigurator(config, nil) 
        
           	require.NoError(t, err) 
        
           	wrap := c.OutgoingRPCWrapper() 
        
           	tlsClient, err := wrap("dc2", client) 
        
           	require.NoError(t, err) 
        
           	err = tlsClient.(*tls.Conn).Handshake() 
        
           	_, ok := err.(x509.HostnameError) 
        
           	require.True(t, ok) 
        
           	tlsClient.Close() 
        
           	<-errc 
        
           } 
        
           func TestConfigurator_outgoingWrapper_BadCert(t *testing.T) { 
        
           	config := Config{ 
        
           		CAFile:               "../test/ca/root.cer", 
        
           		CertFile:             "../test/key/ourdomain.cer", 
        
           		KeyFile:              "../test/key/ourdomain.key", 
        
           		VerifyServerHostname: true, 
        
           		VerifyOutgoing:       true, 
        
           		Domain:               "consul", 
        
           	} 
        
           	client, errc := startTLSServer(&config) 
        
           	if client == nil { 
        
           		t.Fatalf("startTLSServer err: %v", <-errc) 
        
           	} 
        
           	c, err := NewConfigurator(config, nil) 
        
           	require.NoError(t, err) 
        
           	wrap := c.OutgoingRPCWrapper() 
        
           	tlsClient, err := wrap("dc1", client) 
        
           	require.NoError(t, err) 
        
           	err = tlsClient.(*tls.Conn).Handshake() 
        
           	if _, ok := err.(x509.HostnameError); !ok { 
        
           		t.Fatalf("should get hostname err: %v", err) 
        
           	} 
        
           	tlsClient.Close() 
        
           	<-errc 
        
           } 
        
           func TestConfigurator_wrapTLS_OK(t *testing.T) { 
        
           	config := Config{ 
        
           		CAFile:         "../test/ca/root.cer", 
        
           		CertFile:       "../test/key/ourdomain.cer", 
        
           		KeyFile:        "../test/key/ourdomain.key", 
        
           		VerifyOutgoing: true, 
        
           	} 
        
           	client, errc := startTLSServer(&config) 
        
           	if client == nil { 
        
           		t.Fatalf("startTLSServer err: %v", <-errc) 
        
           	} 
        
           	c, err := NewConfigurator(config, nil) 
        
           	require.NoError(t, err) 
        
           	tlsClient, err := c.wrapTLSClient("dc1", client) 
        
           	require.NoError(t, err) 
        
           	tlsClient.Close() 
        
           	err = <-errc 
        
           	require.NoError(t, err) 
        
           } 
        
           func TestConfigurator_wrapTLS_BadCert(t *testing.T) { 
        
           	serverConfig := &Config{ 
        
           		CertFile: "../test/key/ssl-cert-snakeoil.pem", 
        
           		KeyFile:  "../test/key/ssl-cert-snakeoil.key", 
        
           	} 
        
           	client, errc := startTLSServer(serverConfig) 
        
           	if client == nil { 
        
           		t.Fatalf("startTLSServer err: %v", <-errc) 
        
           	} 
        
           	clientConfig := Config{ 
        
           		CAFile:         "../test/ca/root.cer", 
        
           		VerifyOutgoing: true, 
        
           	} 
        
           	c, err := NewConfigurator(clientConfig, nil) 
        
           	require.NoError(t, err) 
        
           	tlsClient, err := c.wrapTLSClient("dc1", client) 
        
           	require.Error(t, err) 
        
           	require.Nil(t, tlsClient) 
        
           	err = <-errc 
        
           	require.NoError(t, err) 
        
           }

.

mkeeler

This looks great and is going to make quite a few of our users very happy.

Just a couple requests for comments (as I started wondering how things worked and eventually tracked it down to dead code only there for easing testing) and a couple of other question.

mkeeler · 2019-03-11T16:29:05Z

agent/consul/client.go

@@ -89,7 +89,11 @@ type Client struct {
 // NewClient is used to construct a new Consul client from the
 // configuration, potentially returning an error
 func NewClient(config *Config) (*Client, error) {
-	return NewClientLogger(config, nil, tlsutil.NewConfigurator(config.ToTLSUtilConfig()))
+	c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil)


Might be useful to note here that a normal Consul Agent doesn't use this method and instead will pass in its own TLS configurator.

At first I was thinking that the agent and server/client would have different configurators (and thus reloading would not work) but realized that you have it passing them to NewClientLogger and NewServerLogger. Adding a comment here to mention whats going on would probably be good.

mkeeler · 2019-03-11T16:29:41Z

agent/consul/server.go

@@ -253,7 +253,11 @@ type Server struct {
 }

 func NewServer(config *Config) (*Server, error) {
-	return NewServerLogger(config, nil, new(token.Store), tlsutil.NewConfigurator(config.ToTLSUtilConfig()))
+	c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil)


Same thing here. A comment about this configurator not being used for a normal Consul agent would be helpful.

tlsutil/config.go

mkeeler · 2019-03-11T19:11:28Z

tlsutil/config.go

+	config := c.commonTLSConfig(c.base.VerifyIncomingHTTPS)
+	config.GetConfigForClient = func(hello *tls.ClientHelloInfo) (*tls.Config, error) {
+		config := c.IncomingHTTPSConfig()
+		config.NextProtos = hello.SupportedProtos


Could this cause issues if a client requests protos ["foo", "http"]? Wouldn't copying the values indicate to the Go TLS library that "foo" is the preferred protocol.?

hanshasselberg · 2019-03-12T10:26:49Z

@mkeeler thank you for the review, I addressed all your points.

mkeeler · 2019-03-12T15:05:15Z

@i0rek Might want to check on those travis tests before merging. Looks like the tlsutil tests are failing to build.

tlsutil/config_test.go:671:19: undefined: nextProtos

hanshasselberg added 22 commits March 6, 2019 12:46

copy config

d881d63

wip

8bcab53

GetConfigForClient

76249dc

better test

12076fa

guard config reading

f76c13e

getclientconfigfor

a064f14

remove comment

c996dd6

fix tests

6017229

check config

066acf6

add test for failing tlsconfig reload

61b6c50

extra test for test

cd35331

add logger so that we can debug log

dd93d73

that refactor....

b111993

woot

10aef9c

fix small things

fccacc7

improvements

a8c3f05

small things

cb3c407

correct locking in wrap

ae321db

small things

b789253

fix that test 🎉

93f9adc

remove that name

b2f7f19

merge conflict.

16c1c80

hanshasselberg force-pushed the reload_tls_config branch from 42cc35b to 16c1c80 Compare March 6, 2019 12:01

hanshasselberg changed the base branch from no_server_name_rpc to master March 6, 2019 12:02

another bad merge

792889a

hanshasselberg marked this pull request as ready for review March 6, 2019 12:26

hanshasselberg commented Mar 6, 2019

View reviewed changes

hanshasselberg requested a review from a team March 6, 2019 12:45

banks requested changes Mar 6, 2019

View reviewed changes

hanshasselberg added 3 commits March 6, 2019 17:30

improve locking

967fb90

more tests

b25c9df

more tests

cab155a

add note about tls reloading to the docs.

3d7fc9e

hanshasselberg requested a review from banks March 8, 2019 14:44

mkeeler requested changes Mar 11, 2019

View reviewed changes

hanshasselberg added 3 commits March 11, 2019 20:53

add comments to explain these functions are only used for testing.

7f78eb8

actually check the cas and not only for values in the configuration

2efe7d2

set next protos, but do not accept everything from the client

282bd93

mkeeler approved these changes Mar 12, 2019

View reviewed changes

fix test :)

421cacf

banks approved these changes Mar 12, 2019

View reviewed changes

hanshasselberg merged commit 7e11dd8 into master Mar 13, 2019

hanshasselberg deleted the reload_tls_config branch March 13, 2019 09:29

hanshasselberg mentioned this pull request Mar 13, 2019

SSL certificate reload without restart #2584

Closed

weihanglo mentioned this pull request May 5, 2022

[BUG] Requesting "additional-ca" and then requesting "network.harvesterhci.io.clusternetworks" will return a 503 error harvester/harvester#2205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reload tls config #5419

Reload tls config #5419

hanshasselberg commented Mar 4, 2019 •

edited by ramramhariram

Loading

hanshasselberg Mar 6, 2019 •

edited

Loading

hanshasselberg Mar 6, 2019

hanshasselberg Mar 6, 2019

hanshasselberg Mar 6, 2019

hanshasselberg Mar 6, 2019 •

edited

Loading

banks Mar 6, 2019

hanshasselberg Mar 6, 2019

hanshasselberg Mar 6, 2019 •

edited

Loading

hanshasselberg Mar 6, 2019

mkeeler Mar 11, 2019

hanshasselberg Mar 6, 2019

hanshasselberg Mar 6, 2019

banks left a comment

banks Mar 6, 2019

hanshasselberg Mar 6, 2019 •

edited

Loading

hanshasselberg commented Mar 8, 2019

mkeeler left a comment

mkeeler Mar 11, 2019

hanshasselberg Mar 11, 2019

mkeeler Mar 11, 2019

hanshasselberg Mar 11, 2019

mkeeler Mar 11, 2019

hanshasselberg commented Mar 12, 2019

mkeeler commented Mar 12, 2019 •

edited

Loading

Reload tls config #5419

Reload tls config #5419

Conversation

hanshasselberg commented Mar 4, 2019 • edited by ramramhariram Loading

hanshasselberg Mar 6, 2019 • 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

hanshasselberg Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanshasselberg Mar 6, 2019 • 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

banks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanshasselberg Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

hanshasselberg commented Mar 8, 2019

mkeeler 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

Choose a reason for hiding this comment

hanshasselberg commented Mar 12, 2019

mkeeler commented Mar 12, 2019 • edited Loading

hanshasselberg commented Mar 4, 2019 •

edited by ramramhariram

Loading

hanshasselberg Mar 6, 2019 •

edited

Loading

hanshasselberg Mar 6, 2019 •

edited

Loading

hanshasselberg Mar 6, 2019 •

edited

Loading

hanshasselberg Mar 6, 2019 •

edited

Loading

mkeeler commented Mar 12, 2019 •

edited

Loading