Nomad agent reload TLS configuration on SIGHUP #3479

chelseakomlo · 2017-11-01T02:26:28Z

This creates a KeyLoader object as part of an agent's configuration, to cache previously-loaded certificates and provide on-the-fly reloading.

This requires that a server already have been previously configured with TLS enabled.

slackpad · 2017-11-01T05:10:11Z

Ooooh - Consul would ❤️ a port of this one once it lands!

preetapan · 2017-11-01T13:32:24Z

command/agent/command.go

-	"github.com/hashicorp/nomad/helper/flag-helpers"
-	"github.com/hashicorp/nomad/helper/gated-writer"
+	flaghelper "github.com/hashicorp/nomad/helper/flag-helpers"
+	gatedwriter "github.com/hashicorp/nomad/helper/gated-writer"


Is this named import necessary?

These are automatically added from https://godoc.org/golang.org/x/tools/cmd/goimports

preetapan · 2017-11-01T13:40:46Z

nomad/structs/config/tls.go

+func (k *KeyLoader) LoadKeyPair(certFile, keyFile string) (*tls.Certificate, error) {
+	cert, err := tls.LoadX509KeyPair(certFile, keyFile)
+	if err != nil {
+		return nil, fmt.Errorf("Failed to load cert/key pair: %v", err)


Shouldn't k.Certificate be set to nil and the load error captured in a variable so that GetOutGoingCertificate starts to return nil after that? The error case I am thinking of is when you load a cert successfully, use it for a while, then need to reload because it expired and the new cert fails to load. In that case, if I understand this code correctly, its going to return the old cert in GetOutgoingCertificate

Great point. This also made me realize that we don't handle downgrading with this either (if that is something we want to handle), it is always assumed that the reload is either upgrading to TLS or changing certificates.

Oh ya, we need to handle downgrades too.

preetapan · 2017-11-01T17:18:39Z

command/agent/agent.go

+			return err
+		}
+	}
+


Shouldn't this handle the reverse case too? Something like

if a.config.TLSConfig != nil && newConfig.TLSConfig == nil { //unload certs and disable tls }

dadgar · 2017-11-02T18:05:45Z

--- FAIL: TestHTTP_VerifyHTTPSClient_AfterConfigReload (0.92s)
	assertions.go:239: 
                          
	Error Trace:	http_test.go:597
		
	Error:      	Not equal: 
		
	            	expected: ""
		
	            	actual: "Get https://127.0.0.1:12169/v1/agent/self: http: server gave HTTP response to HTTPS client"
	assertions.go:239: 
                          
	Error Trace:	http_test.go:598
		
	Error:      	Expected nil, but got: &url.Error{Op:"Get", URL:"https://127.0.0.1:12169/v1/agent/self", Err:(*errors.errorString)(0xc420a882f0)}
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x40 pc=0x10893f7]

goroutine 769 [running]:
testing.tRunner.func1(0xc420138a50)
	/home/travis/.gimme/versions/go1.9.linux.amd64/src/testing/testing.go:711 +0x2d2
panic(0x126e8a0, 0x1de06b0)
	/home/travis/.gimme/versions/go1.9.linux.amd64/src/runtime/panic.go:491 +0x283
github.com/hashicorp/nomad/command/agent.TestHTTP_VerifyHTTPSClient_AfterConfigReload(0xc420138a50)
	/home/travis/gopath/src/github.com/hashicorp/nomad/command/agent/http_test.go:600 +0x857
testing.tRunner(0xc420138a50, 0x14b08c8)
	/home/travis/.gimme/versions/go1.9.linux.amd64/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
	/home/travis/.gimme/versions/go1.9.linux.amd64/src/testing/testing.go:789 +0x2de

dadgar · 2017-11-02T17:47:13Z

command/agent/agent.go

@@ -719,6 +719,16 @@ func (a *Agent) Stats() map[string]map[string]string {
 	return stats
 }

+func (a *Agent) Reload(newConfig *Config) error {


Put a comment on this explaining how it is different than handleReload

dadgar · 2017-11-02T17:47:35Z

command/agent/command.go

@@ -656,6 +656,11 @@ func (c *Command) handleReload(config *Config) *Config {
 		}
 	}

+	err := c.agent.Reload(newConf)


Comment that you are reloading agent object that can affect both client and server. It is probably worth doing this before the Server reload in case these may affect the new config on the server

dadgar · 2017-11-02T17:49:33Z

command/agent/agent.go

+func (a *Agent) Reload(newConfig *Config) error {
+	if a.config != nil && newConfig.TLSConfig != nil {
+		if err := a.config.SetTLSConfig(newConfig.TLSConfig); err != nil {
+			return err


What happens if there is an error. Shouldn't this never back to the existing TLS config

Just to confirm- I think we decided that if there is an error in loading a new TLS configuration, the existing (old) TLS config will continue to be used.

Yeah I agree with that desired behavior but I am not sure that is what is happening. You have reset the key loader by the time there is an error: https://github.com/hashicorp/nomad/pull/3479/files#diff-fdee252bfeea98d87971d63faa2fd863R355

dadgar · 2017-11-02T18:03:41Z

nomad/structs/config/tls.go

+	}
+
+	k.Certificate = &cert
+	return k.Certificate, nil


Doesn't this have to be locked?

dadgar · 2017-11-03T18:18:12Z

command/agent/agent.go

-			return err
+
+		// If the agent is already running with TLS enabled, we need to only reload
+		// its certificates. In a later PR, we will introduce the ability to reload


Change the in a later PR to:

// TODO(chelsea). so you can find it. Comments should be long lived

dadgar · 2017-11-03T18:19:09Z

command/agent/config.go

 func (c *Config) SetTLSConfig(newConfig *config.TLSConfig) error {
-	c.TLSConfig = newConfig
-	c.TLSConfig.KeyLoader = &config.KeyLoader{}
+	if c.TLSConfig == nil {


Maybe SetTLSConfig isn't the right name since I would expect this to be fine

dadgar · 2017-11-03T18:20:08Z

nomad/config.go

-			KeyLoader: &config.KeyLoader{},
-		},
-		ReplicationBackoff: 30 * time.Second,
-		SentinelGCInterval: 30 * time.Second,


Accidentally deleted these?

dadgar · 2017-11-03T18:20:34Z

nomad/structs/config/tls.go

+func (t *TLSConfig) GetKeyLoader() *KeyLoader {
+	// If the keyloader has not yet been initialized, do it here
+	if t.KeyLoader == nil {
+		t.KeyLoader = &KeyLoader{}


This cleans up so much! Probably should lock this too

Ah this would have to be outside the if statement. It is ensuring that if you have two concurrent calls where the keyloader isn't made they both return the same object.

Ah, of course. Will fix that up.

dadgar

Lint failure:

command/agent/http_test.go:563:2:warning: ineffectual assignment to req (ineffassign)

Are tests failing because of changes in this PR?

schmichael

Looks good! Just some comment style nitpicks and locking fun.

schmichael · 2017-11-06T23:22:45Z

command/agent/agent.go

+// Reload handles configuration changes for the agent. Provides a method that
+// is easier to unit test, as this action is invoked via SIGHUP.
+func (a *Agent) Reload(newConfig *Config) error {
+	if a.config != nil && newConfig.TLSConfig != nil {


a.config won't be nil here.

schmichael · 2017-11-06T23:25:02Z

command/agent/config.go

+
+	keyLoader := c.TLSConfig.GetKeyLoader()
+	_, err := keyLoader.LoadKeyPair(newConfig.CertFile, newConfig.KeyFile)
+


extra space

schmichael · 2017-11-06T23:26:53Z

helper/tlsutil/config.go

@@ -60,6 +62,9 @@ type Config struct {
 	// KeyFile is used to provide a TLS key that is used for serving TLS connections.
 	// Must be provided to serve TLS connections.
 	KeyFile string
+
+	// Utility to dynamically reload TLS configuration.


Go's style is to put the name of the var/func as the first word in the description so that searching for terms jumps to their comment first (and some tools even take advantage of this idiom!):

// KeyLoader dynamically reloads TLS configuration

or similar.

schmichael · 2017-11-06T23:40:07Z

nomad/server.go

 	lru "github.com/hashicorp/golang-lru"
 	"github.com/hashicorp/nomad/command/agent/consul"
 	"github.com/hashicorp/nomad/helper/tlsutil"
 	"github.com/hashicorp/nomad/nomad/deploymentwatcher"
 	"github.com/hashicorp/nomad/nomad/state"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/raft"
-	"github.com/hashicorp/raft-boltdb"
+	raftboltdb "github.com/hashicorp/raft-boltdb"


huh, I wonder why your goimports disagrees with our goimports? Are you using this? https://godoc.org/golang.org/x/tools/cmd/goimports

I'm using vim-go! Standardizing on an import format would be great.

schmichael · 2017-11-06T23:42:53Z

nomad/structs/config/tls.go

+	return k.Certificate, nil
+}
+
+func (k *KeyLoader) GetOutgoingCertificate(*tls.ClientHelloInfo) (*tls.Certificate, error) {


Add a comment to exported funcs

schmichael · 2017-11-06T23:43:32Z

nomad/structs/config/tls.go

+	new.VerifyServerHostname = t.VerifyServerHostname
+	new.CAFile = t.CAFile
+	new.CertFile = t.CertFile
+	new.KeyLoader = t.KeyLoader


Do you need to guard access to KeyLoader with configLock?

schmichael · 2017-11-06T23:44:14Z

nomad/structs/config/tls.go

 // TLSConfig provides TLS related configuration
 type TLSConfig struct {
+	configLock sync.Mutex


It looks like this only guards KeyLoader, so perhaps rename it keyLoaderLock and put the field below keyloader (we try to put lock field declarations near what they lock).

schmichael · 2017-11-06T23:44:40Z

nomad/structs/config/tls.go

@@ -38,9 +48,66 @@ type TLSConfig struct {
 	VerifyHTTPSClient bool `mapstructure:"verify_https_client"`
 }

+type KeyLoader struct {
+	cacheLock   sync.Mutex
+	Certificate *tls.Certificate


Let's not export this if it can only be accessed using a private lock.

schmichael · 2017-11-06T23:47:35Z

nomad/structs/config/tls.go

+func (k *KeyLoader) LoadKeyPair(certFile, keyFile string) (*tls.Certificate, error) {
+	// Allow downgrading
+	if certFile == "" && keyFile == "" {
+		k.Certificate = nil


Needs locking

schmichael · 2017-11-06T23:48:48Z

nomad/structs/config/tls.go

+}
+
+func (k *KeyLoader) GetOutgoingCertificate(*tls.ClientHelloInfo) (*tls.Certificate, error) {
+	return k.Certificate, nil


Lock around certificate access

schmichael · 2017-11-13T18:53:53Z

command/agent/config.go

+// current configuration and return an error.
+// This only allows reloading the certificate and keyfile- other TLSConfig
+// fields are ignored.
+func (c *Config) UpdateTLSConfig(newConfig *config.TLSConfig) error {


Does this method need locking? If readers are accessing c.TLSConfig.CertFile concurrently with Reload()s then there's a race.

Good point, added.

dadgar · 2017-11-13T18:58:27Z

nomad/structs/config/tls.go

+func (t *TLSConfig) Copy() *TLSConfig {
+
+	new := &TLSConfig{}
+	new.EnableHTTP = t.EnableHTTP


*new = *t is the pattern for getting all non-pointer values.

This will also copy lock values, which is a warning in govet. If there is another way that is more idiomatic that doesn't copy lock values, would definitely prefer that.

dadgar · 2017-11-13T18:59:18Z

nomad/structs/config/tls.go

+	new.CertFile = t.CertFile
+
+	t.keyloaderLock.Lock()
+	new.KeyLoader = t.KeyLoader


Do you really want to do this? Or you want a copy of the object?

Good point. I'm not sure it matters that much, as it seems Merge is called for two configurations before Keyloader would be initialized, but doing a copy would probably be safer. I'll make that change.

Reload only the config if agent is already using TLS

lock keyloader before loading/caching a new certificate

Implementation is based heavily on similar functionality in Nomad. hashicorp/nomad#3479

github-actions · 2023-03-17T02:14:25Z

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.

preetapan reviewed Nov 1, 2017

View reviewed changes

chelseakomlo changed the title ~~Nomad agent reload on TLS sighup~~ Nomad agent reload TLS configuration on SIGHIP Nov 1, 2017

chelseakomlo changed the title ~~Nomad agent reload TLS configuration on SIGHIP~~ Nomad agent reload TLS configuration on SIGHUP Nov 1, 2017

preetapan reviewed Nov 1, 2017

View reviewed changes

chelseakomlo mentioned this pull request Nov 1, 2017

WIP: Allow server TLS configuration to be reloaded via SIGHUP #3470

Closed

preetapan reviewed Nov 1, 2017

View reviewed changes

chelseakomlo changed the title ~~Nomad agent reload TLS configuration on SIGHUP~~ WIP: Nomad agent reload TLS configuration on SIGHUP Nov 1, 2017

dadgar requested changes Nov 2, 2017

View reviewed changes

chelseakomlo changed the title ~~WIP: Nomad agent reload TLS configuration on SIGHUP~~ Nomad agent reload TLS configuration on SIGHUP Nov 2, 2017

chelseakomlo force-pushed the f-client-reload-tls-sighup branch from de37397 to 61bfd0c Compare November 3, 2017 15:35

chelseakomlo mentioned this pull request Nov 3, 2017

Reload server TLS configuration on the fly #3489

Closed

dadgar requested changes Nov 3, 2017

View reviewed changes

chelseakomlo force-pushed the f-client-reload-tls-sighup branch from c2b0b49 to 3a998ec Compare November 3, 2017 21:14

dadgar approved these changes Nov 3, 2017

View reviewed changes

dadgar requested changes Nov 3, 2017

View reviewed changes

schmichael requested changes Nov 6, 2017

View reviewed changes

chelseakomlo force-pushed the f-client-reload-tls-sighup branch 2 times, most recently from 3e401ab to 08bdafb Compare November 12, 2017 03:27

SoMuchToGrok mentioned this pull request Nov 13, 2017

SSL certificate reload without restart hashicorp/consul#2584

Closed

schmichael reviewed Nov 13, 2017

View reviewed changes

dadgar approved these changes Nov 13, 2017

View reviewed changes

chelseakomlo force-pushed the f-client-reload-tls-sighup branch 4 times, most recently from 121e169 to 1712d6a Compare November 14, 2017 16:46

chelseakomlo added 3 commits November 14, 2017 12:09

Allow server TLS configuration to be reloaded via SIGHUP

25a86ac

dynamic tls reloading for nomad agents

c65e23b

code cleanup and refactoring

c9acc5f

chelseakomlo added 10 commits November 14, 2017 12:09

initalize keyloader if necessary

50a1336

integration test for tls reload

ff1d0dd

fix up test to assert success on reloaded TLS configuration

d7ff9f4

failure in loading a new TLS config should remain at current

0be15b0

Reload only the config if agent is already using TLS

reload agent configuration before specific server/client

0c4336e

lock keyloader before loading/caching a new certificate

introduce a get-or-set method for keyloader

2990eb8

fixups from code review

b2ef194

fix up linting errors

fea24fe

fixups from code review

a40aafb

add lock for config updates; improve copy of tls config

2cade96

chelseakomlo force-pushed the f-client-reload-tls-sighup branch from 1712d6a to 2cade96 Compare November 14, 2017 17:15

chelseakomlo added 2 commits November 14, 2017 14:10

GetCertificate only reloads certificates dynamically for the server

12cb657

config updates/copies should be on agent

885c175

chelseakomlo force-pushed the f-client-reload-tls-sighup branch from ce8a664 to 885c175 Compare November 14, 2017 22:17

chelseakomlo and others added 7 commits November 14, 2017 17:47

improve http integration test

d46eb3b

simplify agent reloading storing a local copy of config

3c7bb25

reuse the same keyloader when reloading

ae63199

Test that server and client get reloaded but keep keyloader

d7d25b4

Keyloader exposes GetClientCertificate as well for outgoing connections

9f954dc

Fix spelling

98d6358

correct changelog style

75a0c60

dadgar merged commit fa9fd44 into master Nov 15, 2017

dadgar deleted the f-client-reload-tls-sighup branch November 15, 2017 01:53

dadgar mentioned this pull request Nov 17, 2017

[feature] Support graceful reloading of TLS configuration #3247

Closed

akshayganeshen mentioned this pull request Jun 11, 2018

Support reloading TLS configuration hashicorp/consul#4204

Closed

8 tasks

hanshasselberg pushed a commit to hashicorp/consul that referenced this pull request Feb 15, 2019

Add TLS key-pair reload.

527b639

Implementation is based heavily on similar functionality in Nomad. hashicorp/nomad#3479

hanshasselberg pushed a commit to hashicorp/consul that referenced this pull request Feb 18, 2019

Add TLS key-pair reload.

595cfe5

Implementation is based heavily on similar functionality in Nomad. hashicorp/nomad#3479

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

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


		keyLoader := c.TLSConfig.GetKeyLoader()
		_, err := keyLoader.LoadKeyPair(newConfig.CertFile, newConfig.KeyFile)

Nomad agent reload TLS configuration on SIGHUP #3479

Nomad agent reload TLS configuration on SIGHUP #3479

Conversation

chelseakomlo commented Nov 1, 2017 • edited

slackpad commented Nov 1, 2017

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 commented Nov 2, 2017

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 • edited

Choose a reason for hiding this comment

schmichael 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

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

github-actions bot commented Mar 17, 2023

chelseakomlo commented Nov 1, 2017 •

edited

dadgar left a comment •

edited