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

@@ -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
 }

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{}) {