From 113454645dd572047cc9517a9990d66911018627 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Wed, 31 Aug 2022 11:20:29 -0700 Subject: [PATCH 1/3] 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) { From 7150ccad85e941e4ce56de1760526bc24cba0e0e Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Wed, 31 Aug 2022 11:43:21 -0700 Subject: [PATCH 2/3] Add changelog note --- .changelog/14429.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/14429.txt diff --git a/.changelog/14429.txt b/.changelog/14429.txt new file mode 100644 index 0000000000..4387d1ed40 --- /dev/null +++ b/.changelog/14429.txt @@ -0,0 +1,3 @@ +```release-note:bug +connect: Fixed an issue where intermediate certificates could build up in the root CA because they were never being pruned after expiring. +`` \ No newline at end of file From 0c2fb7252dd28b5108f8e9d544a893256729064f Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Thu, 1 Sep 2022 14:24:30 -0700 Subject: [PATCH 3/3] Prune intermediates before appending new one --- agent/consul/leader_connect_ca.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index d2cd021134..fe780f7f22 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -1098,9 +1098,13 @@ func setLeafSigningCert(caRoot *structs.CARoot, pem string) error { return fmt.Errorf("error parsing leaf signing cert: %w", err) } + if err := pruneExpiredIntermediates(caRoot); err != nil { + return err + } + caRoot.IntermediateCerts = append(caRoot.IntermediateCerts, pem) caRoot.SigningKeyID = connect.EncodeSigningKeyID(cert.SubjectKeyId) - return pruneExpiredIntermediates(caRoot) + return nil } // pruneExpiredIntermediates removes expired intermediate certificates @@ -1108,15 +1112,14 @@ func setLeafSigningCert(caRoot *structs.CARoot, pem string) error { func pruneExpiredIntermediates(caRoot *structs.CARoot) error { var newIntermediates []string now := time.Now() - for i, intermediatePEM := range caRoot.IntermediateCerts { + for _, 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) { + // Only keep the intermediate cert if it's still valid. + if cert.NotAfter.After(now) { newIntermediates = append(newIntermediates, intermediatePEM) } }