From e13f4af06b82858f4dc2b2877d414134091d84ad Mon Sep 17 00:00:00 2001
From: Kyle Havlovitz <kylehav@gmail.com>
Date: Fri, 9 Oct 2020 04:35:42 -0700
Subject: [PATCH] connect: Check for expired root cert when cross-signing

---
 agent/connect/ca/provider_consul.go |  7 +++----
 agent/connect/ca/provider_vault.go  | 15 ++++++++++++++-
 agent/consul/leader_connect.go      |  4 ++--
 agent/consul/leader_connect_test.go |  7 +++++--
 4 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go
index fa13ed7bec..55f9052fb5 100644
--- a/agent/connect/ca/provider_consul.go
+++ b/agent/connect/ca/provider_consul.go
@@ -21,14 +21,13 @@ import (
 	"github.com/hashicorp/go-hclog"
 )
 
-const (
-
+var (
 	// NotBefore will be CertificateTimeDriftBuffer in the past to account for
 	// time drift between different servers.
 	CertificateTimeDriftBuffer = time.Minute
-)
 
-var ErrNotInitialized = errors.New("provider not initialized")
+	ErrNotInitialized = errors.New("provider not initialized")
+)
 
 type ConsulProvider struct {
 	Delegate ConsulProviderStateDelegate
diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go
index 7c5fe47c8b..0eee7d6d20 100644
--- a/agent/connect/ca/provider_vault.go
+++ b/agent/connect/ca/provider_vault.go
@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"strings"
+	"time"
 
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/structs"
@@ -476,8 +477,20 @@ func (v *VaultProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
 // CrossSignCA takes a CA certificate and cross-signs it to form a trust chain
 // back to our active root.
 func (v *VaultProvider) CrossSignCA(cert *x509.Certificate) (string, error) {
+	rootPEM, err := v.ActiveRoot()
+	if err != nil {
+		return "", err
+	}
+	rootCert, err := connect.ParseCert(rootPEM)
+	if err != nil {
+		return "", fmt.Errorf("error parsing root cert: %v", err)
+	}
+	if rootCert.NotAfter.Before(time.Now()) {
+		return "", fmt.Errorf("root certificate is expired")
+	}
+
 	var pemBuf bytes.Buffer
-	err := pem.Encode(&pemBuf, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
+	err = pem.Encode(&pemBuf, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
 	if err != nil {
 		return "", err
 	}
diff --git a/agent/consul/leader_connect.go b/agent/consul/leader_connect.go
index 6befad04dd..c81b2120b3 100644
--- a/agent/consul/leader_connect.go
+++ b/agent/consul/leader_connect.go
@@ -516,6 +516,7 @@ func (s *Server) persistNewRoot(provider ca.Provider, newActiveRoot *structs.CAR
 // which means it must never take that lock itself or call anything that does.
 func (s *Server) getIntermediateCAPrimary(provider ca.Provider, newActiveRoot *structs.CARoot) error {
 	connectLogger := s.loggers.Named(logging.Connect)
+	// Generate and sign an intermediate cert using the root CA.
 	intermediatePEM, err := provider.GenerateIntermediate()
 	if err != nil {
 		return fmt.Errorf("error generating new intermediate cert: %v", err)
@@ -531,7 +532,7 @@ func (s *Server) getIntermediateCAPrimary(provider ca.Provider, newActiveRoot *s
 	newActiveRoot.IntermediateCerts = append(newActiveRoot.IntermediateCerts, intermediatePEM)
 	newActiveRoot.SigningKeyID = connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId)
 
-	connectLogger.Info("generated new intermediate certificate in primary datacenter")
+	connectLogger.Info("generated new intermediate certificate for primary datacenter")
 	return nil
 }
 
@@ -738,7 +739,6 @@ func (s *Server) intermediateCertRenewalWatch(ctx context.Context) error {
 
 				if lessThanHalfTimePassed(time.Now(), intermediateCert.NotBefore.Add(ca.CertificateTimeDriftBuffer),
 					intermediateCert.NotAfter) {
-					//connectLogger.Info("checked time passed", intermediateCert.NotBefore.Add(ca.CertificateTimeDriftBuffer), intermediateCert.NotAfter)
 					return nil
 				}
 
diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go
index 8690f3c994..6cad61bf63 100644
--- a/agent/consul/leader_connect_test.go
+++ b/agent/consul/leader_connect_test.go
@@ -185,11 +185,14 @@ func TestLeader_PrimaryCA_IntermediateRenew(t *testing.T) {
 	// no parallel execution because we change globals
 	origInterval := structs.IntermediateCertRenewInterval
 	origMinTTL := structs.MinLeafCertTTL
+	origDriftBuffer := ca.CertificateTimeDriftBuffer
 	defer func() {
 		structs.IntermediateCertRenewInterval = origInterval
 		structs.MinLeafCertTTL = origMinTTL
+		ca.CertificateTimeDriftBuffer = origDriftBuffer
 	}()
 
+	ca.CertificateTimeDriftBuffer = 30 * time.Second
 	structs.IntermediateCertRenewInterval = time.Millisecond
 	structs.MinLeafCertTTL = time.Second
 	require := require.New(t)
@@ -207,7 +210,7 @@ func TestLeader_PrimaryCA_IntermediateRenew(t *testing.T) {
 				"Token":               testVault.RootToken,
 				"RootPKIPath":         "pki-root/",
 				"IntermediatePKIPath": "pki-intermediate/",
-				"LeafCertTTL":         "5s",
+				"LeafCertTTL":         "1s",
 				// The retry loop only retries for 7sec max and
 				// the ttl needs to be below so that it
 				// triggers definitely.
@@ -215,7 +218,7 @@ func TestLeader_PrimaryCA_IntermediateRenew(t *testing.T) {
 				// valid from 1minute in the past, we need to
 				// account for that, otherwise it will be
 				// expired immediately.
-				"IntermediateCertTTL": "15s",
+				"IntermediateCertTTL": "5s",
 			},
 		}
 	})