diff --git a/connect/service.go b/connect/service.go index 51bad44f6..f9d6591c2 100644 --- a/connect/service.go +++ b/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. - serverTLSCfg *reloadableTLSConfig - - // clientTLSCfg is the (reloadable) TLS config we use for dialling. - clientTLSCfg *reloadableTLSConfig + // tlsCfg is the dynamic TLS config + tlsCfg *dynamicTLSConfig // 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 } - - // 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) + s.tlsCfg = newDynamicTLSConfig(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 { diff --git a/connect/testing.go b/connect/testing.go index c23b83701..491036aaf 100644 --- a/connect/testing.go +++ b/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 - svc.serverTLSCfg = newReloadableTLSConfig( - TestTLSConfigWithVerifier(t, service, ca, newServerSideVerifier(nil, service))) - svc.clientTLSCfg = newReloadableTLSConfig( - TestTLSConfigWithVerifier(t, service, ca, clientSideVerifier)) + // Override the tlsConfig hackily. + svc.tlsCfg = newDynamicTLSConfig(TestTLSConfig(t, service, ca)) 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) - 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 := defaultTLSConfig() cfg.Certificates = []tls.Certificate{TestSvcKeyPair(t, service, ca)} cfg.RootCAs = TestCAPool(t, ca) cfg.ClientCAs = TestCAPool(t, ca) diff --git a/connect/tls.go b/connect/tls.go index d23b49396..f5cb95a75 100644 --- a/connect/tls.go +++ b/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 -// that is always nil in our usage. Implementations can use the roots provided -// in the cfg to verify the certs. +// tls.Config.VerifyPeerCertificate hook. +// +// 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. -func defaultTLSConfig(v verifierFunc) *tls.Config { +// defaultTLSConfig returns the standard config with no peer verifier. It is +// 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 ¤t.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(nil) - + cfg := defaultTLSConfig() 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 +} diff --git a/connect/tls_test.go b/connect/tls_test.go index 82b89440f..aa1063f3e 100644 --- a/connect/tls_test.go +++ b/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) +}