Merge pull request #3492 from hashicorp/f-client-tls-reload

Client/Server TLS dynamic reload

Author: chelseakomlo

CHANGELOG.md

@@ -14,6 +14,7 @@ IMPROVEMENTS:
 
 BUG FIXES:
  * core: Fix search endpoint forwarding for multi-region clusters [[GH-3680](https://github.com/hashicorp/nomad/issues/3680)]
+ * core: Allow upgrading/downgrading TLS via SIGHUP on both servers and clients [[GH-3492](https://github.com/hashicorp/nomad/issues/3492)]
  * core: Fix an issue in which batch jobs with queued placements and lost
    allocations could result in improper placement counts [[GH-3717](https://github.com/hashicorp/nomad/issues/3717)]
  * client: Migrated ephemeral_disk's maintain directory permissions [[GH-3723](https://github.com/hashicorp/nomad/issues/3723)]

client/client.go

@@ -30,6 +30,7 @@ import (
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/nomad"
 	"github.com/hashicorp/nomad/nomad/structs"
+	nconfig "github.com/hashicorp/nomad/nomad/structs/config"
 	vaultapi "github.com/hashicorp/vault/api"
 	"github.com/mitchellh/hashstructure"
 	"github.com/shirou/gopsutil/host"
@@ -364,6 +365,34 @@ func (c *Client) init() error {
 	return nil
 }
 
+// reloadTLSConnections allows a client to reload its TLS configuration on the
+// fly
+func (c *Client) reloadTLSConnections(newConfig *nconfig.TLSConfig) error {
+	var tlsWrap tlsutil.RegionWrapper
+	if newConfig != nil && newConfig.EnableRPC {
+		tw, err := tlsutil.NewTLSConfiguration(newConfig).OutgoingTLSWrapper()
+		if err != nil {
+			return err
+		}
+		tlsWrap = tw
+	}
+
+	// Keep the client configuration up to date as we use configuration values to
+	// decide on what type of connections to accept
+	c.configLock.Lock()
+	c.config.TLSConfig = newConfig
+	c.configLock.Unlock()
+
+	c.connPool.ReloadTLS(tlsWrap)
+
+	return nil
+}
+
+// Reload allows a client to reload its configuration on the fly
+func (c *Client) Reload(newConfig *config.Config) error {
+	return c.reloadTLSConnections(newConfig.TLSConfig)
+}
+
 // Leave is used to prepare the client to leave the cluster
 func (c *Client) Leave() error {
 	// TODO

client/client_test.go

@@ -1001,3 +1001,156 @@ func TestClient_ValidateMigrateToken_ACLDisabled(t *testing.T) {
 
 	assert.Equal(c.ValidateMigrateToken("", ""), true)
 }
+
+func TestClient_ReloadTLS_UpgradePlaintextToTLS(t *testing.T) {
+	t.Parallel()
+	assert := assert.New(t)
+
+	s1, addr := testServer(t, func(c *nomad.Config) {
+		c.Region = "regionFoo"
+	})
+	defer s1.Shutdown()
+	testutil.WaitForLeader(t, s1.RPC)
+
+	const (
+		cafile  = "../helper/tlsutil/testdata/ca.pem"
+		foocert = "../helper/tlsutil/testdata/nomad-foo.pem"
+		fookey  = "../helper/tlsutil/testdata/nomad-foo-key.pem"
+	)
+
+	c1 := testClient(t, func(c *config.Config) {
+		c.Servers = []string{addr}
+	})
+	defer c1.Shutdown()
+
+	// Registering a node over plaintext should succeed
+	{
+		req := structs.NodeSpecificRequest{
+			NodeID:       c1.Node().ID,
+			QueryOptions: structs.QueryOptions{Region: "regionFoo"},
+		}
+
+		testutil.WaitForResult(func() (bool, error) {
+			var out structs.SingleNodeResponse
+			err := c1.RPC("Node.GetNode", &req, &out)
+			if err != nil {
+				return false, fmt.Errorf("client RPC failed when it should have succeeded:\n%+v", err)
+			}
+			return true, nil
+		},
+			func(err error) {
+				t.Fatalf(err.Error())
+			},
+		)
+	}
+
+	newConfig := &nconfig.TLSConfig{
+		EnableHTTP:           true,
+		EnableRPC:            true,
+		VerifyServerHostname: true,
+		CAFile:               cafile,
+		CertFile:             foocert,
+		KeyFile:              fookey,
+	}
+
+	err := c1.reloadTLSConnections(newConfig)
+	assert.Nil(err)
+
+	// Registering a node over plaintext should fail after the node has upgraded
+	// to TLS
+	{
+		req := structs.NodeSpecificRequest{
+			NodeID:       c1.Node().ID,
+			QueryOptions: structs.QueryOptions{Region: "regionFoo"},
+		}
+		testutil.WaitForResult(func() (bool, error) {
+			var out structs.SingleNodeResponse
+			err := c1.RPC("Node.GetNode", &req, &out)
+			if err == nil {
+				return false, fmt.Errorf("client RPC succeeded when it should have failed:\n%+v", err)
+			}
+			return true, nil
+		},
+			func(err error) {
+				t.Fatalf(err.Error())
+			},
+		)
+	}
+}
+
+func TestClient_ReloadTLS_DowngradeTLSToPlaintext(t *testing.T) {
+	t.Parallel()
+	assert := assert.New(t)
+
+	s1, addr := testServer(t, func(c *nomad.Config) {
+		c.Region = "regionFoo"
+	})
+	defer s1.Shutdown()
+	testutil.WaitForLeader(t, s1.RPC)
+
+	const (
+		cafile  = "../helper/tlsutil/testdata/ca.pem"
+		foocert = "../helper/tlsutil/testdata/nomad-foo.pem"
+		fookey  = "../helper/tlsutil/testdata/nomad-foo-key.pem"
+	)
+
+	c1 := testClient(t, func(c *config.Config) {
+		c.Servers = []string{addr}
+		c.TLSConfig = &nconfig.TLSConfig{
+			EnableHTTP:           true,
+			EnableRPC:            true,
+			VerifyServerHostname: true,
+			CAFile:               cafile,
+			CertFile:             foocert,
+			KeyFile:              fookey,
+		}
+	})
+	defer c1.Shutdown()
+
+	// assert that when one node is running in encrypted mode, a RPC request to a
+	// node running in plaintext mode should fail
+	{
+		req := structs.NodeSpecificRequest{
+			NodeID:       c1.Node().ID,
+			QueryOptions: structs.QueryOptions{Region: "regionFoo"},
+		}
+		testutil.WaitForResult(func() (bool, error) {
+			var out structs.SingleNodeResponse
+			err := c1.RPC("Node.GetNode", &req, &out)
+			if err == nil {
+				return false, fmt.Errorf("client RPC succeeded when it should have failed :\n%+v", err)
+			}
+			return true, nil
+		},
+			func(err error) {
+				t.Fatalf(err.Error())
+			},
+		)
+	}
+
+	newConfig := &nconfig.TLSConfig{}
+
+	err := c1.reloadTLSConnections(newConfig)
+	assert.Nil(err)
+
+	// assert that when both nodes are in plaintext mode, a RPC request should
+	// succeed
+	{
+		req := structs.NodeSpecificRequest{
+			NodeID:       c1.Node().ID,
+			QueryOptions: structs.QueryOptions{Region: "regionFoo"},
+		}
+		testutil.WaitForResult(func() (bool, error) {
+			var out structs.SingleNodeResponse
+			err := c1.RPC("Node.GetNode", &req, &out)
+			if err != nil {
+				return false, fmt.Errorf("client RPC failed when it should have succeeded:\n%+v", err)
+			}
+			return true, nil
+		},
+			func(err error) {
+				t.Fatalf(err.Error())
+			},
+		)
+	}
+}

client/config/config.go

@@ -347,9 +347,10 @@ func (c *Config) ReadStringListToMapDefault(key, defaultValue string) map[string
 	return list
 }
 
-// TLSConfig returns a TLSUtil Config based on the client configuration
+// TLSConfiguration returns a TLSUtil Config based on the existing client
+// configuration
 func (c *Config) TLSConfiguration() *tlsutil.Config {
-	tlsConf := &tlsutil.Config{
+	return &tlsutil.Config{
 		VerifyIncoming:       true,
 		VerifyOutgoing:       true,
 		VerifyServerHostname: c.TLSConfig.VerifyServerHostname,
@@ -358,5 +359,4 @@ func (c *Config) TLSConfiguration() *tlsutil.Config {
 		KeyFile:              c.TLSConfig.KeyFile,
 		KeyLoader:            c.TLSConfig.GetKeyLoader(),
 	}
-	return tlsConf
 }

command/agent/agent.go

@@ -756,37 +756,66 @@ func (a *Agent) Stats() map[string]map[string]string {
 	return stats
 }
 
+// ShouldReload determines if we should reload the configuration and agent
+// connections. If the TLS Configuration has not changed, we shouldn't reload.
+func (a *Agent) ShouldReload(newConfig *Config) (bool, bool) {
+	a.configLock.Lock()
+	defer a.configLock.Unlock()
+	if a.config.TLSConfig.Equals(newConfig.TLSConfig) {
+		return false, false
+	}
+
+	return true, true // requires a reload of both agent and http server
+}
+
 // 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 {
 	a.configLock.Lock()
 	defer a.configLock.Unlock()
 
-	if newConfig.TLSConfig != nil {
-
-		// TODO(chelseakomlo) In a later PR, we will introduce the ability to reload
-		// TLS configuration if the agent is not running with TLS enabled.
-		if a.config.TLSConfig != nil {
-			// Reload the certificates on the keyloader and on success store the
-			// updated TLS config. It is important to reuse the same keyloader
-			// as this allows us to dynamically reload configurations not only
-			// on the Agent but on the Server and Client too (they are
-			// referencing the same keyloader).
-			keyloader := a.config.TLSConfig.GetKeyLoader()
-			_, err := keyloader.LoadKeyPair(newConfig.TLSConfig.CertFile, newConfig.TLSConfig.KeyFile)
-			if err != nil {
-				return err
-			}
-			a.config.TLSConfig = newConfig.TLSConfig
-			a.config.TLSConfig.KeyLoader = keyloader
+	if newConfig == nil || newConfig.TLSConfig == nil {
+		return fmt.Errorf("cannot reload agent with nil configuration")
+	}
+
+	// This is just a TLS configuration reload, we don't need to refresh
+	// existing network connections
+	if !a.config.TLSConfig.IsEmpty() && !newConfig.TLSConfig.IsEmpty() {
+
+		// Reload the certificates on the keyloader and on success store the
+		// updated TLS config. It is important to reuse the same keyloader
+		// as this allows us to dynamically reload configurations not only
+		// on the Agent but on the Server and Client too (they are
+		// referencing the same keyloader).
+		keyloader := a.config.TLSConfig.GetKeyLoader()
+		_, err := keyloader.LoadKeyPair(newConfig.TLSConfig.CertFile, newConfig.TLSConfig.KeyFile)
+		if err != nil {
+			return err
 		}
+		a.config.TLSConfig = newConfig.TLSConfig
+		a.config.TLSConfig.KeyLoader = keyloader
+		return nil
+	}
+
+	// Completely reload the agent's TLS configuration (moving from non-TLS to
+	// TLS, or vice versa)
+	// This does not handle errors in loading the new TLS configuration
+	a.config.TLSConfig = newConfig.TLSConfig.Copy()
+
+	if newConfig.TLSConfig.IsEmpty() {
+		a.logger.Println("[WARN] agent: Downgrading agent's existing TLS configuration to plaintext")
+	} else {
+		a.logger.Println("[INFO] agent: Upgrading from plaintext configuration to TLS")
 	}
 
 	return nil
 }
 
-// GetConfigCopy creates a replica of the agent's config, excluding locks
+// GetConfig creates a locked reference to the agent's config
 func (a *Agent) GetConfig() *Config {
+	a.configLock.Lock()
+	defer a.configLock.Unlock()
+
 	return a.config
 }