From bb4baeba956e1a36f117c80bd42f535e4938c015 Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Mon, 13 Mar 2023 17:32:59 -0400 Subject: [PATCH] Preserve CARoots when updating Vault CA configuration (#16592) If a CA config update did not cause a root change, the codepath would return early and skip some steps which preserve its intermediate certificates and signing key ID. This commit re-orders some code and prevents updates from generating new intermediate certificates. --- .changelog/16592.txt | 3 + agent/consul/leader_connect_ca.go | 37 +++++++----- agent/consul/leader_connect_ca_test.go | 83 ++++++++++++++++++-------- 3 files changed, 83 insertions(+), 40 deletions(-) create mode 100644 .changelog/16592.txt diff --git a/.changelog/16592.txt b/.changelog/16592.txt new file mode 100644 index 000000000..ba37d6901 --- /dev/null +++ b/.changelog/16592.txt @@ -0,0 +1,3 @@ +```release-note:bug +ca: Fixes a bug where updating Vault CA Provider config would cause TLS issues in the service mesh +``` diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 008289c94..abb92f54b 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -12,7 +12,7 @@ import ( "time" "github.com/hashicorp/go-hclog" - uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/go-uuid" "golang.org/x/time/rate" "github.com/hashicorp/consul/acl" @@ -272,7 +272,7 @@ func newCARoot(pemValue, provider, clusterID string) (*structs.CARoot, error) { ExternalTrustDomain: clusterID, NotBefore: primaryCert.NotBefore, NotAfter: primaryCert.NotAfter, - RootCert: pemValue, + RootCert: lib.EnsureTrailingNewline(pemValue), PrivateKeyType: keyType, PrivateKeyBits: keyBits, Active: true, @@ -887,6 +887,23 @@ func (c *CAManager) primaryUpdateRootCA(newProvider ca.Provider, args *structs.C return err } + // TODO: https://github.com/hashicorp/consul/issues/12386 + intermediate, err := newProvider.ActiveIntermediate() + if err != nil { + return fmt.Errorf("error fetching active intermediate: %w", err) + } + if intermediate == "" { + intermediate, err = newProvider.GenerateIntermediate() + if err != nil { + return fmt.Errorf("error generating intermediate: %w", err) + } + } + if intermediate != newRootPEM { + if err := setLeafSigningCert(newActiveRoot, intermediate); err != nil { + return err + } + } + // See if the provider needs to persist any state along with the config pState, err := newProvider.State() if err != nil { @@ -970,19 +987,9 @@ func (c *CAManager) primaryUpdateRootCA(newProvider ca.Provider, args *structs.C } // Add the cross signed cert to the new CA's intermediates (to be attached - // to leaf certs). - newActiveRoot.IntermediateCerts = []string{xcCert} - } - } - - // TODO: https://github.com/hashicorp/consul/issues/12386 - intermediate, err := newProvider.GenerateIntermediate() - if err != nil { - return err - } - if intermediate != newRootPEM { - if err := setLeafSigningCert(newActiveRoot, intermediate); err != nil { - return err + // to leaf certs). We do not want it to be the last cert if there are any + // existing intermediate certs so we push to the front. + newActiveRoot.IntermediateCerts = append([]string{xcCert}, newActiveRoot.IntermediateCerts...) } } diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 7e84a87b1..1e4e4d2af 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -25,7 +25,7 @@ import ( "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/connect" - ca "github.com/hashicorp/consul/agent/connect/ca" + "github.com/hashicorp/consul/agent/connect/ca" "github.com/hashicorp/consul/agent/consul/fsm" "github.com/hashicorp/consul/agent/consul/state" "github.com/hashicorp/consul/agent/structs" @@ -612,39 +612,72 @@ func TestCAManager_UpdateConfiguration_Vault_Primary(t *testing.T) { _, origRoot, err := s1.fsm.State().CARootActive(nil) require.NoError(t, err) require.Len(t, origRoot.IntermediateCerts, 1) + origRoot.CreateIndex = s1.caManager.providerRoot.CreateIndex + origRoot.ModifyIndex = s1.caManager.providerRoot.ModifyIndex + require.Equal(t, s1.caManager.providerRoot, origRoot) cert, err := connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(origRoot)) require.NoError(t, err) require.Equal(t, connect.HexString(cert.SubjectKeyId), origRoot.SigningKeyID) - vaultToken2 := ca.CreateVaultTokenWithAttrs(t, vault.Client(), &ca.VaultTokenAttributes{ - RootPath: "pki-root-2", - IntermediatePath: "pki-intermediate-2", - ConsulManaged: true, - }) - - err = s1.caManager.UpdateConfiguration(&structs.CARequest{ - Config: &structs.CAConfiguration{ - Provider: "vault", - Config: map[string]interface{}{ - "Address": vault.Addr, - "Token": vaultToken2, - "RootPKIPath": "pki-root-2/", - "IntermediatePKIPath": "pki-intermediate-2/", + t.Run("update config without changing root", func(t *testing.T) { + err = s1.caManager.UpdateConfiguration(&structs.CARequest{ + Config: &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vaultToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + "CSRMaxPerSecond": 100, + }, }, - }, + }) + require.NoError(t, err) + _, sameRoot, err := s1.fsm.State().CARootActive(nil) + require.NoError(t, err) + require.Len(t, sameRoot.IntermediateCerts, 1) + sameRoot.CreateIndex = s1.caManager.providerRoot.CreateIndex + sameRoot.ModifyIndex = s1.caManager.providerRoot.ModifyIndex + + cert, err := connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(sameRoot)) + require.NoError(t, err) + require.Equal(t, connect.HexString(cert.SubjectKeyId), sameRoot.SigningKeyID) + + require.Equal(t, origRoot, sameRoot) + require.Equal(t, sameRoot, s1.caManager.providerRoot) }) - require.NoError(t, err) - _, newRoot, err := s1.fsm.State().CARootActive(nil) - require.NoError(t, err) - require.Len(t, newRoot.IntermediateCerts, 2, - "expected one cross-sign cert and one local leaf sign cert") - require.NotEqual(t, origRoot.ID, newRoot.ID) + t.Run("update config and change root", func(t *testing.T) { + vaultToken2 := ca.CreateVaultTokenWithAttrs(t, vault.Client(), &ca.VaultTokenAttributes{ + RootPath: "pki-root-2", + IntermediatePath: "pki-intermediate-2", + ConsulManaged: true, + }) - cert, err = connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(newRoot)) - require.NoError(t, err) - require.Equal(t, connect.HexString(cert.SubjectKeyId), newRoot.SigningKeyID) + err = s1.caManager.UpdateConfiguration(&structs.CARequest{ + Config: &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vaultToken2, + "RootPKIPath": "pki-root-2/", + "IntermediatePKIPath": "pki-intermediate-2/", + }, + }, + }) + require.NoError(t, err) + + _, newRoot, err := s1.fsm.State().CARootActive(nil) + require.NoError(t, err) + require.Len(t, newRoot.IntermediateCerts, 2, + "expected one cross-sign cert and one local leaf sign cert") + require.NotEqual(t, origRoot.ID, newRoot.ID) + + cert, err = connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(newRoot)) + require.NoError(t, err) + require.Equal(t, connect.HexString(cert.SubjectKeyId), newRoot.SigningKeyID) + }) } func TestCAManager_Initialize_Vault_WithIntermediateAsPrimaryCA(t *testing.T) {