From 113454645dd572047cc9517a9990d66911018627 Mon Sep 17 00:00:00 2001
From: Kyle Havlovitz <kylehav@gmail.com>
Date: Wed, 31 Aug 2022 11:20:29 -0700
Subject: [PATCH] Prune old expired intermediate certs when appending a new one

---
 agent/consul/leader_connect_ca.go      | 22 ++++++++++++++++++++++
 agent/consul/leader_connect_ca_test.go |  1 -
 agent/consul/leader_connect_test.go    | 24 ++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go
index 5e22681645..d2cd021134 100644
--- a/agent/consul/leader_connect_ca.go
+++ b/agent/consul/leader_connect_ca.go
@@ -1100,6 +1100,28 @@ func setLeafSigningCert(caRoot *structs.CARoot, pem string) error {
 
 	caRoot.IntermediateCerts = append(caRoot.IntermediateCerts, pem)
 	caRoot.SigningKeyID = connect.EncodeSigningKeyID(cert.SubjectKeyId)
+	return pruneExpiredIntermediates(caRoot)
+}
+
+// pruneExpiredIntermediates removes expired intermediate certificates
+// from the given CARoot.
+func pruneExpiredIntermediates(caRoot *structs.CARoot) error {
+	var newIntermediates []string
+	now := time.Now()
+	for i, intermediatePEM := range caRoot.IntermediateCerts {
+		cert, err := connect.ParseCert(intermediatePEM)
+		if err != nil {
+			return fmt.Errorf("error parsing leaf signing cert: %w", err)
+		}
+
+		// Only keep the intermediate cert if it's still valid, or if it's the most
+		// recently added (and thus the active signing cert).
+		if cert.NotAfter.After(now) || i == len(caRoot.IntermediateCerts) {
+			newIntermediates = append(newIntermediates, intermediatePEM)
+		}
+	}
+
+	caRoot.IntermediateCerts = newIntermediates
 	return nil
 }
 
diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go
index 37756eb204..91095be8e6 100644
--- a/agent/consul/leader_connect_ca_test.go
+++ b/agent/consul/leader_connect_ca_test.go
@@ -435,7 +435,6 @@ func TestCAManager_SignCertificate_WithExpiredCert(t *testing.T) {
 		errorMsg              string
 	}{
 		{"intermediate valid", time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), false, ""},
-		{"intermediate expired", time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), time.Now().AddDate(-2, 0, 0), time.Now().AddDate(0, 0, -1), true, "intermediate expired: certificate expired, expiration date"},
 		{"root expired", time.Now().AddDate(-2, 0, 0), time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), true, "root expired: certificate expired, expiration date"},
 		// a cert that is not yet valid is ok, assume it will be valid soon enough
 		{"intermediate in the future", time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), time.Now().AddDate(0, 0, 1), time.Now().AddDate(0, 0, 2), false, ""},
diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go
index d9b3863865..c8b361b03b 100644
--- a/agent/consul/leader_connect_test.go
+++ b/agent/consul/leader_connect_test.go
@@ -401,6 +401,18 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) {
 	err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert)
 	require.NoError(t, err)
 	verifyLeafCert(t, activeRoot, cert.CertPEM)
+
+	// Wait for the primary's old intermediate to be pruned after expiring.
+	oldIntermediate := activeRoot.IntermediateCerts[0]
+	retry.Run(t, func(r *retry.R) {
+		store := s1.caManager.delegate.State()
+		_, storedRoot, err := store.CARootActive(nil)
+		r.Check(err)
+
+		if storedRoot.IntermediateCerts[0] == oldIntermediate {
+			r.Fatal("old intermediate should be gone")
+		}
+	})
 }
 
 func patchIntermediateCertRenewInterval(t *testing.T) {
@@ -516,6 +528,18 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) {
 	err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert)
 	require.NoError(t, err)
 	verifyLeafCert(t, activeRoot, cert.CertPEM)
+
+	// Wait for dc2's old intermediate to be pruned after expiring.
+	oldIntermediate := activeRoot.IntermediateCerts[0]
+	retry.Run(t, func(r *retry.R) {
+		store := s2.caManager.delegate.State()
+		_, storedRoot, err := store.CARootActive(nil)
+		r.Check(err)
+
+		if storedRoot.IntermediateCerts[0] == oldIntermediate {
+			r.Fatal("old intermediate should be gone")
+		}
+	})
 }
 
 func TestConnectCA_ConfigurationSet_RootRotation_Secondary(t *testing.T) {