From 446025b587ca7dc82ed3eafd0ca421e3ec008887 Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Fri, 17 Aug 2018 15:55:54 -0400 Subject: [PATCH 1/2] Connect: Verify the leaf cert to determine if its ready. This improves the checking so that if a certificate were to expire or the roots changed then we will go into a non-ready state. This parses the x509 certificates from the TLS certificate when the leaf is set and whenever the leaf is set or the roots are set will perform x509 verification to ensure that the given roots can verify the leaf cert. Only when a valid roots/leaf cert pair are loaded will the notify function be called and the readyCh be closed. The Ready function will now also perform x509 verification of the certificate which will in addition to verifying the signatures also verify the cert has not expired. In this way repeated calls to Ready will return a non-Ready state when a certificate expires. --- connect/tls.go | 65 ++++++++++++++++++++++++++++++++++++++------- connect/tls_test.go | 11 ++++++++ 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/connect/tls.go b/connect/tls.go index 7d679e383807..d2f9603909c3 100644 --- a/connect/tls.go +++ b/connect/tls.go @@ -15,6 +15,30 @@ import ( "github.com/hashicorp/consul/api" ) +// parseLeafX509Cert will parse an X509 certificate +// from the TLS certificate and store the parsed +// value in the TLS certificate as the Leaf field. +func parseLeafX509Cert(leaf *tls.Certificate) error { + if leaf == nil { + // nothing to parse for nil cert + return nil + } + + if leaf.Leaf != nil { + // leaf cert was already parsed + return nil + } + + cert, err := x509.ParseCertificate(leaf.Certificate[0]) + + if err != nil { + return err + } + + leaf.Leaf = cert + return nil +} + // 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. @@ -273,6 +297,9 @@ func newDynamicTLSConfig(base *tls.Config) *dynamicTLSConfig { } if len(base.Certificates) > 0 { cfg.leaf = &base.Certificates[0] + // If this does error then future calls to Ready will fail + // It is better to handle not-Ready rather than failing + parseLeafX509Cert(cfg.leaf) } if base.RootCAs != nil { cfg.roots = base.RootCAs @@ -334,19 +361,39 @@ func (cfg *dynamicTLSConfig) SetRoots(roots *x509.CertPool) error { func (cfg *dynamicTLSConfig) SetLeaf(leaf *tls.Certificate) error { cfg.Lock() defer cfg.Unlock() + if err := parseLeafX509Cert(leaf); err != nil { + return err + } cfg.leaf = leaf + cfg.notify() return nil } // notify is called under lock during an update to check if we are now ready. func (cfg *dynamicTLSConfig) notify() { - if cfg.readyCh != nil && cfg.leaf != nil && cfg.roots != nil { + if cfg.readyCh != nil && cfg.leaf != nil && cfg.roots != nil && cfg.leaf.Leaf != nil { close(cfg.readyCh) cfg.readyCh = nil } } +func (cfg *dynamicTLSConfig) VerifyLeafWithRoots() error { + cfg.RLock() + defer cfg.RUnlock() + + if cfg.roots == nil { + return fmt.Errorf("No roots are set") + } else if cfg.leaf == nil { + return fmt.Errorf("No leaf certificate is set") + } else if cfg.leaf.Leaf == nil { + return fmt.Errorf("Leaf certificate has not been parsed") + } + + _, err := cfg.leaf.Leaf.Verify(x509.VerifyOptions{Roots: cfg.roots}) + return err +} + // Roots returns the current CA root CertPool. func (cfg *dynamicTLSConfig) Roots() *x509.CertPool { cfg.RLock() @@ -364,19 +411,19 @@ func (cfg *dynamicTLSConfig) Leaf() *tls.Certificate { // Ready returns whether or not both roots and a leaf certificate are // configured. If both are non-nil, they are assumed to be valid and usable. func (cfg *dynamicTLSConfig) Ready() bool { - cfg.RLock() - defer cfg.RUnlock() - return cfg.leaf != nil && cfg.roots != nil + // not locking because VerifyLeafWithRoots will do that + return cfg.VerifyLeafWithRoots() == nil } // ReadyWait returns a chan that is closed when the the Service becomes ready // for use for the first time. Note that if the Service is ready when it is // called it returns a nil chan. Ready means that it has root and leaf -// certificates configured which we assume are valid. The service may -// subsequently stop being "ready" if it's certificates expire or are revoked -// and an error prevents new ones being loaded but this method will not stop -// returning a nil chan in that case. It is only useful for initial startup. For -// ongoing health Ready() should be used. +// certificates configured but not that the combination is valid nor that +// the current time is within the validity window of the certificate. The +// service may subsequently stop being "ready" if it's certificates expire +// or are revoked and an error prevents new ones from being loaded but this +// method will not stop returning a nil chan in that case. It is only useful +// for initial startup. For ongoing health Ready() should be used. func (cfg *dynamicTLSConfig) ReadyWait() <-chan struct{} { return cfg.readyCh } diff --git a/connect/tls_test.go b/connect/tls_test.go index 5df491866f5d..61b35a50d869 100644 --- a/connect/tls_test.go +++ b/connect/tls_test.go @@ -379,6 +379,17 @@ func TestDynamicTLSConfig_Ready(t *testing.T) { require.NoError(err) assertNotBlocked(t, readyCh) require.True(c.Ready(), "should be ready") + + ca2 := connect.TestCA(t, nil) + ca2cfg := TestTLSConfig(t, "web", ca2) + + require.NoError(c.SetRoots(ca2cfg.RootCAs)) + assertNotBlocked(t, readyCh) + require.False(c.Ready(), "invalid leaf, should not be ready") + + require.NoError(c.SetRoots(baseCfg.RootCAs)) + assertNotBlocked(t, readyCh) + require.True(c.Ready(), "should be ready") } func assertBlocked(t *testing.T, ch <-chan struct{}) { From 0348f1d6fcc554910b4cbafd60abb5a7c068a2be Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Thu, 6 Sep 2018 15:29:56 -0400 Subject: [PATCH 2/2] Pass logger into newDynamicTLSConfig and log cert parsing errors. --- connect/service.go | 4 ++-- connect/tls.go | 6 ++++-- connect/tls_test.go | 4 ++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/connect/service.go b/connect/service.go index 7bdaf3c60ffc..74e6a52b5ea2 100644 --- a/connect/service.go +++ b/connect/service.go @@ -72,7 +72,7 @@ func NewServiceWithLogger(serviceName string, client *api.Client, service: serviceName, client: client, logger: logger, - tlsCfg: newDynamicTLSConfig(defaultTLSConfig()), + tlsCfg: newDynamicTLSConfig(defaultTLSConfig(), logger), httpResolverFromAddr: ConsulResolverFromAddrFunc(client), } @@ -121,7 +121,7 @@ func NewDevServiceWithTLSConfig(serviceName string, logger *log.Logger, s := &Service{ service: serviceName, logger: logger, - tlsCfg: newDynamicTLSConfig(tlsCfg), + tlsCfg: newDynamicTLSConfig(tlsCfg, logger), } return s, nil } diff --git a/connect/tls.go b/connect/tls.go index d2f9603909c3..7c513dc611f2 100644 --- a/connect/tls.go +++ b/connect/tls.go @@ -291,7 +291,7 @@ type tlsCfgUpdate struct { // 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 { +func newDynamicTLSConfig(base *tls.Config, logger *log.Logger) *dynamicTLSConfig { cfg := &dynamicTLSConfig{ base: base, } @@ -299,7 +299,9 @@ func newDynamicTLSConfig(base *tls.Config) *dynamicTLSConfig { cfg.leaf = &base.Certificates[0] // If this does error then future calls to Ready will fail // It is better to handle not-Ready rather than failing - parseLeafX509Cert(cfg.leaf) + if err := parseLeafX509Cert(cfg.leaf); err != nil && logger != nil { + logger.Printf("[ERR] Error parsing configured leaf certificate: %v", err) + } } if base.RootCAs != nil { cfg.roots = base.RootCAs diff --git a/connect/tls_test.go b/connect/tls_test.go index 61b35a50d869..591f4fb00afc 100644 --- a/connect/tls_test.go +++ b/connect/tls_test.go @@ -299,7 +299,7 @@ func TestDynamicTLSConfig(t *testing.T) { baseCfg := TestTLSConfig(t, "web", ca1) newCfg := TestTLSConfig(t, "web", ca2) - c := newDynamicTLSConfig(baseCfg) + c := newDynamicTLSConfig(baseCfg, nil) // Should set them from the base config require.Equal(c.Leaf(), &baseCfg.Certificates[0]) @@ -365,7 +365,7 @@ func TestDynamicTLSConfig_Ready(t *testing.T) { ca1 := connect.TestCA(t, nil) baseCfg := TestTLSConfig(t, "web", ca1) - c := newDynamicTLSConfig(defaultTLSConfig()) + c := newDynamicTLSConfig(defaultTLSConfig(), nil) readyCh := c.ReadyWait() assertBlocked(t, readyCh) require.False(c.Ready(), "no roots or leaf, should not be ready")