Refactor reloadableTLSConfig and verifyier shenanigans into simpler dynamicTLSConfig

connect/service.go

@@ -42,11 +42,8 @@ type Service struct {
 	// connections will fail to verify.
 	client *api.Client
 
-	// serverTLSCfg is the (reloadable) TLS config we use for serving.
+	// tlsCfg is the dynamic TLS config
-	serverTLSCfg *reloadableTLSConfig
+	tlsCfg *dynamicTLSConfig
-
-	// clientTLSCfg is the (reloadable) TLS config we use for dialling.
-	clientTLSCfg *reloadableTLSConfig
 
 	// httpResolverFromAddr is a function that returns a Resolver from a string
 	// address for HTTP clients. It's privately pluggable to make testing easier
@@ -73,9 +70,8 @@ func NewServiceWithLogger(serviceID string, client *api.Client,
 		serviceID: serviceID,
 		client:    client,
 		logger:    logger,
+		tlsCfg:    newDynamicTLSConfig(defaultTLSConfig()),
 	}
-	s.serverTLSCfg = newReloadableTLSConfig(defaultTLSConfig(newServerSideVerifier(client, serviceID)))
-	s.clientTLSCfg = newReloadableTLSConfig(defaultTLSConfig(clientSideVerifier))
 
 	// TODO(banks) run the background certificate sync
 	return s, nil
@@ -94,13 +90,7 @@ func NewDevServiceFromCertFiles(serviceID string, client *api.Client,
 	if err != nil {
 		return nil, err
 	}
-
+	s.tlsCfg = newDynamicTLSConfig(tlsCfg)
-	// Note that newReloadableTLSConfig makes a copy so we can re-use the same
-	// base for both client and server with swapped verifiers.
-	setVerifier(tlsCfg, newServerSideVerifier(client, serviceID))
-	s.serverTLSCfg = newReloadableTLSConfig(tlsCfg)
-	setVerifier(tlsCfg, clientSideVerifier)
-	s.clientTLSCfg = newReloadableTLSConfig(tlsCfg)
 	return s, nil
 }
 
@@ -115,7 +105,7 @@ func NewDevServiceFromCertFiles(serviceID string, client *api.Client,
 // error during renewal. The listener will be able to accept connections again
 // once connectivity is restored provided the client's Token is valid.
 func (s *Service) ServerTLSConfig() *tls.Config {
-	return s.serverTLSCfg.TLSConfig()
+	return s.tlsCfg.Get(newServerSideVerifier(s.client, s.serviceID))
 }
 
 // Dial connects to a remote Connect-enabled server. The passed Resolver is used
@@ -138,7 +128,7 @@ func (s *Service) Dial(ctx context.Context, resolver Resolver) (net.Conn, error)
 		return nil, err
 	}
 
-	tlsConn := tls.Client(tcpConn, s.clientTLSCfg.TLSConfig())
+	tlsConn := tls.Client(tcpConn, s.tlsCfg.Get(clientSideVerifier))
 	// Set deadline for Handshake to complete.
 	deadline, ok := ctx.Deadline()
 	if ok {

connect/testing.go

@@ -20,17 +20,15 @@ import (
 func TestService(t testing.T, service string, ca *structs.CARoot) *Service {
 	t.Helper()
 
-	// Don't need to talk to client since we are setting TLSConfig locally
+	// Don't need to talk to client since we are setting TLSConfig locally. This
+	// will cause server verification to skip AuthZ too.
 	svc, err := NewService(service, nil)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	// verify server without AuthZ call
+	// Override the tlsConfig hackily.
-	svc.serverTLSCfg = newReloadableTLSConfig(
+	svc.tlsCfg = newDynamicTLSConfig(TestTLSConfig(t, service, ca))
-		TestTLSConfigWithVerifier(t, service, ca, newServerSideVerifier(nil, service)))
-	svc.clientTLSCfg = newReloadableTLSConfig(
-		TestTLSConfigWithVerifier(t, service, ca, clientSideVerifier))
 
 	return svc
 }
@@ -39,17 +37,7 @@ func TestService(t testing.T, service string, ca *structs.CARoot) *Service {
 func TestTLSConfig(t testing.T, service string, ca *structs.CARoot) *tls.Config {
 	t.Helper()
 
-	// Insecure default (nil verifier)
+	cfg := defaultTLSConfig()
-	return TestTLSConfigWithVerifier(t, service, ca, nil)
-}
-
-// TestTLSConfigWithVerifier returns a *tls.Config suitable for use during
-// tests, it will use the given verifierFunc to verify tls certificates.
-func TestTLSConfigWithVerifier(t testing.T, service string, ca *structs.CARoot,
-	verifier verifierFunc) *tls.Config {
-	t.Helper()
-
-	cfg := defaultTLSConfig(verifier)
 	cfg.Certificates = []tls.Certificate{TestSvcKeyPair(t, service, ca)}
 	cfg.RootCAs = TestCAPool(t, ca)
 	cfg.ClientCAs = TestCAPool(t, ca)

connect/tls.go

@@ -5,7 +5,6 @@ import (
 	"crypto/x509"
 	"errors"
 	"io/ioutil"
-	"log"
 	"sync"
 
 	"github.com/hashicorp/consul/agent/connect"
@@ -14,13 +13,18 @@ import (
 
 // verifierFunc is a function that can accept rawCertificate bytes from a peer
 // and verify them against a given tls.Config. It's called from the
-// tls.Config.VerifyPeerCertificate hook. We don't pass verifiedChains since
+// tls.Config.VerifyPeerCertificate hook.
-// that is always nil in our usage. Implementations can use the roots provided
+//
-// in the cfg to verify the certs.
+// We don't pass verifiedChains since that is always nil in our usage.
+// Implementations can use the roots provided in the cfg to verify the certs.
+//
+// The passed *tls.Config may have a nil VerifyPeerCertificates function but
+// will have correct roots, leaf and other fields.
 type verifierFunc func(cfg *tls.Config, rawCerts [][]byte) error
 
-// defaultTLSConfig returns the standard config.
+// defaultTLSConfig returns the standard config with no peer verifier. It is
-func defaultTLSConfig(v verifierFunc) *tls.Config {
+// insecure to use it as-is.
+func defaultTLSConfig() *tls.Config {
 	cfg := &tls.Config{
 		MinVersion: tls.VersionTLS12,
 		ClientAuth: tls.RequireAndVerifyClientCert,
@@ -45,70 +49,11 @@ func defaultTLSConfig(v verifierFunc) *tls.Config {
 		// See: https://github.com/golang/go/blob/917c33fe8672116b04848cf11545296789cafd3b/src/net/http/server.go#L2724-L2731
 		NextProtos: []string{"h2"},
 	}
-	setVerifier(cfg, v)
 	return cfg
 }
 
-// setVerifier takes a *tls.Config and set's it's VerifyPeerCertificates hook to
-// use the passed verifierFunc.
-func setVerifier(cfg *tls.Config, v verifierFunc) {
-	if v != nil {
-		cfg.VerifyPeerCertificate = func(rawCerts [][]byte, chains [][]*x509.Certificate) error {
-			return v(cfg, rawCerts)
-		}
-	}
-}
-
-// reloadableTLSConfig exposes a tls.Config that can have it's certificates
-// reloaded. On a server, this uses GetConfigForClient to pass the current
-// tls.Config or client certificate for each acceptted connection. On a client,
-// this uses GetClientCertificate to provide the current client certificate.
-type reloadableTLSConfig struct {
-	mu sync.Mutex
-
-	// cfg is the current config to use for new connections
-	cfg *tls.Config
-}
-
-// newReloadableTLSConfig returns a reloadable config currently set to base.
-func newReloadableTLSConfig(base *tls.Config) *reloadableTLSConfig {
-	c := &reloadableTLSConfig{}
-	c.SetTLSConfig(base)
-	return c
-}
-
-// TLSConfig returns a *tls.Config that will dynamically load certs. It's
-// suitable for use in either a client or server.
-func (c *reloadableTLSConfig) TLSConfig() *tls.Config {
-	c.mu.Lock()
-	cfgCopy := c.cfg
-	c.mu.Unlock()
-	return cfgCopy
-}
-
-// SetTLSConfig sets the config used for future connections. It is safe to call
-// from any goroutine.
-func (c *reloadableTLSConfig) SetTLSConfig(cfg *tls.Config) error {
-	copy := cfg.Clone()
-	copy.GetClientCertificate = func(*tls.CertificateRequestInfo) (*tls.Certificate, error) {
-		current := c.TLSConfig()
-		if len(current.Certificates) < 1 {
-			return nil, errors.New("tls: no certificates configured")
-		}
-		return &current.Certificates[0], nil
-	}
-	copy.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) {
-		return c.TLSConfig(), nil
-	}
-
-	c.mu.Lock()
-	defer c.mu.Unlock()
-	c.cfg = copy
-	return nil
-}
-
 // devTLSConfigFromFiles returns a default TLS Config but with certs and CAs
-// based on local files for dev.
+// based on local files for dev. No verification is setup.
 func devTLSConfigFromFiles(caFile, certFile,
 	keyFile string) (*tls.Config, error) {
 
@@ -126,9 +71,7 @@ func devTLSConfigFromFiles(caFile, certFile,
 		return nil, err
 	}
 
-	// Insecure no verification
+	cfg := defaultTLSConfig()
-	cfg := defaultTLSConfig(nil)
-
 	cfg.Certificates = []tls.Certificate{cert}
 	cfg.RootCAs = roots
 	cfg.ClientCAs = roots
@@ -210,7 +153,6 @@ func newServerSideVerifier(client *api.Client, serviceID string) verifierFunc {
 		if !resp.Authorized {
 			return errors.New("connect: authz denied: " + resp.Reason)
 		}
-		log.Println("[DEBUG] authz result", resp)
 		return nil
 	}
 }
@@ -265,3 +207,101 @@ func verifyChain(tlsCfg *tls.Config, rawCerts [][]byte, client bool) (*x509.Cert
 	_, err := certs[0].Verify(opts)
 	return certs[0], err
 }
+
+// dynamicTLSConfig represents the state for returning a tls.Config that can
+// have root and leaf certificates updated dynamically with all existing clients
+// and servers automatically picking up the changes. It requires initialising
+// with a valid base config from which all the non-certificate and verification
+// params are used. The base config passed should not be modified externally as
+// it is assumed to be serialised by the embedded mutex.
+type dynamicTLSConfig struct {
+	base *tls.Config
+
+	sync.Mutex
+	leaf  *tls.Certificate
+	roots *x509.CertPool
+}
+
+// newDynamicTLSConfig returns a dynamicTLSConfig constructed from base.
+// base.Certificates[0] is used as the initial leaf and base.RootCAs is used as
+// the initial roots.
+func newDynamicTLSConfig(base *tls.Config) *dynamicTLSConfig {
+	cfg := &dynamicTLSConfig{
+		base: base,
+	}
+	if len(base.Certificates) > 0 {
+		cfg.leaf = &base.Certificates[0]
+	}
+	if base.RootCAs != nil {
+		cfg.roots = base.RootCAs
+	}
+	return cfg
+}
+
+// Get fetches the lastest tls.Config with all the hooks attached to keep it
+// loading the most recent roots and certs even after future changes to cfg.
+//
+// The verifierFunc passed will be attached to the config returned such that it
+// runs with the _latest_ config object returned passed to it. That means that a
+// client can use this config for a long time and will still verify against the
+// latest roots even though the roots in the struct is has can't change.
+func (cfg *dynamicTLSConfig) Get(v verifierFunc) *tls.Config {
+	cfg.Lock()
+	defer cfg.Unlock()
+	copy := cfg.base.Clone()
+	copy.RootCAs = cfg.roots
+	copy.ClientCAs = cfg.roots
+	if v != nil {
+		copy.VerifyPeerCertificate = func(rawCerts [][]byte, chains [][]*x509.Certificate) error {
+			return v(cfg.Get(nil), rawCerts)
+		}
+	}
+	copy.GetCertificate = func(_ *tls.ClientHelloInfo) (*tls.Certificate, error) {
+		leaf := cfg.Leaf()
+		if leaf == nil {
+			return nil, errors.New("tls: no certificates configured")
+		}
+		return leaf, nil
+	}
+	copy.GetClientCertificate = func(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+		leaf := cfg.Leaf()
+		if leaf == nil {
+			return nil, errors.New("tls: no certificates configured")
+		}
+		return leaf, nil
+	}
+	copy.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) {
+		return cfg.Get(v), nil
+	}
+	return copy
+}
+
+// SetRoots sets new roots.
+func (cfg *dynamicTLSConfig) SetRoots(roots *x509.CertPool) error {
+	cfg.Lock()
+	defer cfg.Unlock()
+	cfg.roots = roots
+	return nil
+}
+
+// SetLeaf sets a new leaf.
+func (cfg *dynamicTLSConfig) SetLeaf(leaf *tls.Certificate) error {
+	cfg.Lock()
+	defer cfg.Unlock()
+	cfg.leaf = leaf
+	return nil
+}
+
+// Roots returns the current CA root CertPool.
+func (cfg *dynamicTLSConfig) Roots() *x509.CertPool {
+	cfg.Lock()
+	defer cfg.Unlock()
+	return cfg.roots
+}
+
+// Leaf returns the current Leaf certificate.
+func (cfg *dynamicTLSConfig) Leaf() *tls.Certificate {
+	cfg.Lock()
+	defer cfg.Unlock()
+	return cfg.leaf
+}

connect/tls_test.go

@@ -12,53 +12,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestReloadableTLSConfig(t *testing.T) {
-	require := require.New(t)
-	base := defaultTLSConfig(nil)
-
-	c := newReloadableTLSConfig(base)
-
-	// The dynamic config should be the one we loaded (with some different hooks)
-	got := c.TLSConfig()
-	expect := base.Clone()
-	// Equal and even cmp.Diff fail on tls.Config due to unexported fields in
-	// each. Compare a few things to prove it's returning the bits we
-	// specifically set.
-	require.Equal(expect.Certificates, got.Certificates)
-	require.Equal(expect.RootCAs, got.RootCAs)
-	require.Equal(expect.ClientCAs, got.ClientCAs)
-	require.Equal(expect.InsecureSkipVerify, got.InsecureSkipVerify)
-	require.Equal(expect.MinVersion, got.MinVersion)
-	require.Equal(expect.CipherSuites, got.CipherSuites)
-	require.NotNil(got.GetClientCertificate)
-	require.NotNil(got.GetConfigForClient)
-	require.Contains(got.NextProtos, "h2")
-
-	ca := connect.TestCA(t, nil)
-
-	// Now change the config as if we just loaded certs from Consul
-	new := TestTLSConfig(t, "web", ca)
-	err := c.SetTLSConfig(new)
-	require.Nil(err)
-
-	// Change the passed config to ensure SetTLSConfig made a copy otherwise this
-	// is racey.
-	expect = new.Clone()
-	new.Certificates = nil
-
-	// The dynamic config should be the one we loaded (with some different hooks)
-	got = c.TLSConfig()
-	require.Equal(expect.Certificates, got.Certificates)
-	require.Equal(expect.RootCAs, got.RootCAs)
-	require.Equal(expect.ClientCAs, got.ClientCAs)
-	require.Equal(expect.InsecureSkipVerify, got.InsecureSkipVerify)
-	require.Equal(expect.MinVersion, got.MinVersion)
-	require.Equal(expect.CipherSuites, got.CipherSuites)
-	require.NotNil(got.GetClientCertificate)
-	require.NotNil(got.GetConfigForClient)
-	require.Contains(got.NextProtos, "h2")
-}
-
 func Test_verifyServerCertMatchesURI(t *testing.T) {
 	ca1 := connect.TestCA(t, nil)
 
@@ -288,3 +241,120 @@ func TestServerSideVerifier(t *testing.T) {
 		})
 	}
 }
+
+// requireEqualTLSConfig compares tlsConfig fields we care about. Equal and even
+// cmp.Diff fail on tls.Config due to unexported fields in each. expectLeaf
+// allows expecting a leaf cert different from the one in expect
+func requireEqualTLSConfig(t *testing.T, expect, got *tls.Config) {
+	require := require.New(t)
+	require.Equal(expect.RootCAs, got.RootCAs)
+	require.Equal(expect.ClientCAs, got.ClientCAs)
+	require.Equal(expect.InsecureSkipVerify, got.InsecureSkipVerify)
+	require.Equal(expect.MinVersion, got.MinVersion)
+	require.Equal(expect.CipherSuites, got.CipherSuites)
+	require.NotNil(got.GetCertificate)
+	require.NotNil(got.GetClientCertificate)
+	require.NotNil(got.GetConfigForClient)
+	require.Contains(got.NextProtos, "h2")
+
+	var expectLeaf *tls.Certificate
+	var err error
+	if expect.GetCertificate != nil {
+		expectLeaf, err = expect.GetCertificate(nil)
+		require.Nil(err)
+	} else if len(expect.Certificates) > 0 {
+		expectLeaf = &expect.Certificates[0]
+	}
+
+	gotLeaf, err := got.GetCertificate(nil)
+	require.Nil(err)
+	require.Equal(expectLeaf, gotLeaf)
+
+	gotLeaf, err = got.GetClientCertificate(nil)
+	require.Nil(err)
+	require.Equal(expectLeaf, gotLeaf)
+}
+
+// requireCorrectVerifier invokes got.VerifyPeerCertificate and expects the
+// tls.Config arg to be returned on the provided channel. This ensures the
+// correct verifier func was attached to got.
+//
+// It then ensures that the tls.Config passed to the verifierFunc was actually
+// the same as the expected current value.
+func requireCorrectVerifier(t *testing.T, expect, got *tls.Config,
+	ch chan *tls.Config) {
+
+	err := got.VerifyPeerCertificate(nil, nil)
+	require.Nil(t, err)
+	verifierCfg := <-ch
+	// The tls.Cfg passed to verifyFunc should be the expected (current) value.
+	requireEqualTLSConfig(t, expect, verifierCfg)
+}
+
+func TestDynamicTLSConfig(t *testing.T) {
+	require := require.New(t)
+
+	ca1 := connect.TestCA(t, nil)
+	ca2 := connect.TestCA(t, nil)
+	baseCfg := TestTLSConfig(t, "web", ca1)
+	newCfg := TestTLSConfig(t, "web", ca2)
+
+	c := newDynamicTLSConfig(baseCfg)
+
+	// Should set them from the base config
+	require.Equal(c.Leaf(), &baseCfg.Certificates[0])
+	require.Equal(c.Roots(), baseCfg.RootCAs)
+
+	// Create verifiers we can assert are set and run correctly.
+	v1Ch := make(chan *tls.Config, 1)
+	v2Ch := make(chan *tls.Config, 1)
+	v3Ch := make(chan *tls.Config, 1)
+	verify1 := func(cfg *tls.Config, rawCerts [][]byte) error {
+		v1Ch <- cfg
+		return nil
+	}
+	verify2 := func(cfg *tls.Config, rawCerts [][]byte) error {
+		v2Ch <- cfg
+		return nil
+	}
+	verify3 := func(cfg *tls.Config, rawCerts [][]byte) error {
+		v3Ch <- cfg
+		return nil
+	}
+
+	// The dynamic config should be the one we loaded (with some different hooks)
+	gotBefore := c.Get(verify1)
+	requireEqualTLSConfig(t, baseCfg, gotBefore)
+	requireCorrectVerifier(t, baseCfg, gotBefore, v1Ch)
+
+	// Now change the roots as if we just loaded new roots from Consul
+	err := c.SetRoots(newCfg.RootCAs)
+	require.Nil(err)
+
+	// The dynamic config should have the new roots, but old leaf
+	gotAfter := c.Get(verify2)
+	expect := newCfg.Clone()
+	expect.GetCertificate = func(_ *tls.ClientHelloInfo) (*tls.Certificate, error) {
+		return &baseCfg.Certificates[0], nil
+	}
+	requireEqualTLSConfig(t, expect, gotAfter)
+	requireCorrectVerifier(t, expect, gotAfter, v2Ch)
+
+	// The old config fetched before should still call it's own verify func, but
+	// that verifier should be passed the new config (expect).
+	requireCorrectVerifier(t, expect, gotBefore, v1Ch)
+
+	// Now change the leaf
+	err = c.SetLeaf(&newCfg.Certificates[0])
+	require.Nil(err)
+
+	// The dynamic config should have the new roots, AND new leaf
+	gotAfterLeaf := c.Get(verify3)
+	requireEqualTLSConfig(t, newCfg, gotAfterLeaf)
+	requireCorrectVerifier(t, newCfg, gotAfterLeaf, v3Ch)
+
+	// Both older configs should still call their own verify funcs, but those
+	// verifiers should be passed the new config.
+	requireCorrectVerifier(t, newCfg, gotBefore, v1Ch)
+	requireCorrectVerifier(t, newCfg, gotAfter, v2Ch)
+}