Connect: Verify the leaf cert to determine its readiness. (#4540)

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. The readyCh will be closed whenever a parseable certificate is set and the ca roots are set. This does not mean that the certificate is valid but that it has been setup and is generally valid. The Ready function will now do x509 certificate verification which will in addition to verifying the signatures with the installed CA roots will also verify the certificate isn't expired or not set to become valid in the future. 

The correct way to use these functions is to wait for the ReadyWait chan to be closed and then periodically check the readiness to determine if the certificate is currently useable.

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
 }

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.
@@ -267,12 +291,17 @@ 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,
 	}
 	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
+		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
@@ -334,19 +363,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 +413,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
 }

connect/tls_test.go

@@ -302,7 +302,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])
@@ -368,7 +368,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")
@@ -382,6 +382,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{}) {