Vault CA provider clean up previous default issuers

.changelog/18773.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+ca: Vault provider now cleans up the previous Vault issuer and key when generating a new leaf signing certificate [[GH-18779](https://github.com/hashicorp/consul/issues/18779)]
+```

agent/connect/ca/provider_vault.go

@@ -12,7 +12,6 @@ import (
 	"io"
 	"net/http"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/hashicorp/go-hclog"
@@ -58,9 +57,7 @@ type VaultProvider struct {
 	config *structs.VaultCAProviderConfig
 
 	client *vaultapi.Client
-	// We modify the namespace on the fly to override default namespace for rootCertificate and intermediateCertificate. Can't guarantee
-	// all operations (specifically Sign) are not called re-entrantly, so we add this for safety.
-	clientMutex   sync.Mutex
+
 	baseNamespace string
 
 	stopWatcher func()
@@ -521,9 +518,7 @@ func (v *VaultProvider) ActiveLeafSigningCert() (string, error) {
 // because the endpoint only returns the raw PEM contents of the CA cert
 // and not the typical format of the secrets endpoints.
 func (v *VaultProvider) getCA(namespace, path string) (string, error) {
-	defer v.setNamespace(namespace)()
-
-	resp, err := v.client.Logical().ReadRaw(path + "/ca/pem")
+	resp, err := v.client.WithNamespace(namespace).Logical().ReadRaw(path + "/ca/pem")
 	if resp != nil {
 		defer resp.Body.Close()
 	}
@@ -549,9 +544,7 @@ func (v *VaultProvider) getCA(namespace, path string) (string, error) {
 
 // TODO: refactor to remove duplication with getCA
 func (v *VaultProvider) getCAChain(namespace, path string) (string, error) {
-	defer v.setNamespace(namespace)()
-
-	resp, err := v.client.Logical().ReadRaw(path + "/ca_chain")
+	resp, err := v.client.WithNamespace(namespace).Logical().ReadRaw(path + "/ca_chain")
 	if resp != nil {
 		defer resp.Body.Close()
 	}
@@ -622,6 +615,9 @@ func (v *VaultProvider) GenerateLeafSigningCert() (string, error) {
 // should contain a "mapping" data field we can use to cross-reference
 // with the keyId returned when calling [/intermediate/generate/internal].
 //
+// After a new default issuer is written, this function also cleans up
+// the previous default issuer along with its associated key.
+//
 // [/intermediate/set-signed]: https://developer.hashicorp.com/vault/api-docs/secret/pki#import-ca-certificates-and-keys
 // [/intermediate/generate/internal]: https://developer.hashicorp.com/vault/api-docs/secret/pki#generate-intermediate-csr
 func (v *VaultProvider) setDefaultIntermediateIssuer(vaultResp *vaultapi.Secret, keyId string) error {
@@ -659,14 +655,50 @@ func (v *VaultProvider) setDefaultIntermediateIssuer(vaultResp *vaultapi.Secret,
 		return fmt.Errorf("could not read from /config/issuers: %w", err)
 	}
 	issuersConf := resp.Data
+	prevIssuer, ok := issuersConf["default"].(string)
+	if !ok {
+		return fmt.Errorf("unexpected type for 'default' value in Vault response from /pki/config/issuers")
+	}
+
+	if prevIssuer == intermediateId {
+		return nil
+	}
+
 	// Overwrite the default issuer
 	issuersConf["default"] = intermediateId
-
 	_, err = v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"config/issuers", issuersConf)
 	if err != nil {
 		return fmt.Errorf("could not write default issuer to /config/issuers: %w", err)
 	}
 
+	// Find the key_id of the previous issuer. In Consul, issuers have 1:1 relationship with
+	// keys so we can delete issuer first then the key.
+	resp, err = v.readNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"issuer/"+prevIssuer)
+	if err != nil {
+		return fmt.Errorf("could not read issuer %q: %w", prevIssuer, err)
+	}
+	prevKeyId, ok := resp.Data["key_id"].(string)
+	if !ok {
+		return fmt.Errorf("unexpected type for 'key_id' value in Vault response")
+	}
+
+	// Delete the previously known default issuer to prevent the number of unused
+	// issuers from increasing too much.
+	_, err = v.deleteNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"issuer/"+prevIssuer)
+	if err != nil {
+		v.logger.Warn("Could not delete previous issuer. Manually delete from Vault to prevent the list of issuers from growing too large.",
+			"prev_issuer_id", prevIssuer,
+			"error", err)
+	}
+
+	// Keys can only be deleted if there are no more issuers referencing them.
+	_, err = v.deleteNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"key/"+prevKeyId)
+	if err != nil {
+		v.logger.Warn("Could not delete previous key. Manually delete from Vault to prevent the list of keys from growing too large.",
+			"prev_key_id", prevKeyId,
+			"error", err)
+	}
+
 	return nil
 }
 
@@ -819,41 +851,27 @@ func (v *VaultProvider) Stop() {
 
 // We use raw path here
 func (v *VaultProvider) mountNamespaced(namespace, path string, mountInfo *vaultapi.MountInput) error {
-	defer v.setNamespace(namespace)()
-	return v.client.Sys().Mount(path, mountInfo)
+	return v.client.WithNamespace(namespace).Sys().Mount(path, mountInfo)
 }
 
 func (v *VaultProvider) tuneMountNamespaced(namespace, path string, mountConfig *vaultapi.MountConfigInput) error {
-	defer v.setNamespace(namespace)()
-	return v.client.Sys().TuneMount(path, *mountConfig)
+	return v.client.WithNamespace(namespace).Sys().TuneMount(path, *mountConfig)
 }
 
 func (v *VaultProvider) unmountNamespaced(namespace, path string) error {
-	defer v.setNamespace(namespace)()
-	return v.client.Sys().Unmount(path)
+	return v.client.WithNamespace(namespace).Sys().Unmount(path)
 }
 
 func (v *VaultProvider) readNamespaced(namespace string, resource string) (*vaultapi.Secret, error) {
-	defer v.setNamespace(namespace)()
-	return v.client.Logical().Read(resource)
+	return v.client.WithNamespace(namespace).Logical().Read(resource)
 }
 
 func (v *VaultProvider) writeNamespaced(namespace string, resource string, data map[string]interface{}) (*vaultapi.Secret, error) {
-	defer v.setNamespace(namespace)()
-	return v.client.Logical().Write(resource, data)
+	return v.client.WithNamespace(namespace).Logical().Write(resource, data)
 }
 
-func (v *VaultProvider) setNamespace(namespace string) func() {
-	if namespace != "" {
-		v.clientMutex.Lock()
-		v.client.SetNamespace(namespace)
-		return func() {
-			v.client.SetNamespace(v.baseNamespace)
-			v.clientMutex.Unlock()
-		}
-	} else {
-		return func() {}
-	}
+func (v *VaultProvider) deleteNamespaced(namespace string, resource string) (*vaultapi.Secret, error) {
+	return v.client.WithNamespace(namespace).Logical().Delete(resource)
 }
 
 // autotidyIssuers sets Vault's auto-tidy to remove expired issuers

agent/connect/ca/provider_vault_test.go

@@ -18,6 +18,7 @@ import (
 	vaultapi "github.com/hashicorp/vault/api"
 	"github.com/hashicorp/vault/api/auth/gcp"
 	vaultconst "github.com/hashicorp/vault/sdk/helper/consts"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/hashicorp/consul/agent/connect"
@@ -574,12 +575,6 @@ func TestVaultCAProvider_CrossSignCA(t *testing.T) {
 	run := func(t *testing.T, tc CASigningKeyTypes, withSudo, expectFailure bool) {
 		t.Parallel()
 
-		if tc.SigningKeyType != tc.CSRKeyType {
-			// TODO: uncomment since the bug is closed
-			// See https://github.com/hashicorp/vault/issues/7709
-			t.Skip("Vault doesn't support cross-signing different key types yet.")
-		}
-
 		testVault1 := NewTestVaultServer(t)
 
 		attr1 := &VaultTokenAttributes{
@@ -1195,6 +1190,51 @@ func TestVaultCAProvider_AutoTidyExpiredIssuers(t *testing.T) {
 	require.Contains(t, errStr, "permission denied")
 }
 
+func TestVaultCAProvider_DeletePreviousIssuerAndKey(t *testing.T) {
+	SkipIfVaultNotPresent(t)
+	t.Parallel()
+
+	testVault := NewTestVaultServer(t)
+	attr := &VaultTokenAttributes{
+		RootPath:         "pki-root",
+		IntermediatePath: "pki-intermediate",
+		ConsulManaged:    true,
+	}
+	token := CreateVaultTokenWithAttrs(t, testVault.client, attr)
+	provider := createVaultProvider(t, true, testVault.Addr, token,
+		map[string]any{
+			"RootPKIPath":         "pki-root/",
+			"IntermediatePKIPath": "pki-intermediate/",
+		})
+	res, err := testVault.Client().Logical().List("pki-intermediate/issuers")
+	require.NoError(t, err)
+	// Why 2 issuers? There is always an initial issuer that
+	// gets created before we manage the lifecycle of issuers.
+	// Since we're asserting that the number doesn't grow
+	// this isn't too much of a concern.
+	//
+	// Note the key "keys" refers to the IDs of the resource based
+	// on the endpoint the response is from.
+	require.Len(t, res.Data["keys"], 2)
+
+	res, err = testVault.Client().Logical().List("pki-intermediate/keys")
+	require.NoError(t, err)
+	require.Len(t, res.Data["keys"], 1)
+
+	for i := 0; i < 3; i++ {
+		_, err := provider.GenerateLeafSigningCert()
+		require.NoError(t, err)
+
+		res, err := testVault.Client().Logical().List("pki-intermediate/issuers")
+		require.NoError(t, err)
+		require.Len(t, res.Data["keys"], 2)
+
+		res, err = testVault.Client().Logical().List("pki-intermediate/keys")
+		require.NoError(t, err)
+		assert.Len(t, res.Data["keys"], 1)
+	}
+}
+
 func TestVaultCAProvider_GenerateIntermediate_inSecondary(t *testing.T) {
 	SkipIfVaultNotPresent(t)