From 2766043527fd4b6132374d7f08f30fe23cacaf35 Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Wed, 13 Sep 2023 17:39:15 -0400 Subject: [PATCH] [1.14.x] Vault CA provider clean up previous default issuers (#18773) (#18785) * Vault CA provider clean up previous default issuers (#18773) (cherry picked from commit 4dfca64ded20512089c015f4913e969989934431) --- .changelog/18773.txt | 3 ++ agent/connect/ca/provider_vault.go | 46 ++++++++++++++++++++ agent/connect/ca/provider_vault_test.go | 57 ++++++++++++++++++++++--- 3 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 .changelog/18773.txt diff --git a/.changelog/18773.txt b/.changelog/18773.txt new file mode 100644 index 0000000000..1d59fe98f0 --- /dev/null +++ b/.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)] +``` diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index 2f1862da66..105a909cc4 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -639,6 +639,9 @@ func (v *VaultProvider) GenerateIntermediate() (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 { @@ -676,6 +679,15 @@ 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 @@ -684,6 +696,34 @@ func (v *VaultProvider) setDefaultIntermediateIssuer(vaultResp *vaultapi.Secret, 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 } @@ -893,6 +933,12 @@ func (v *VaultProvider) writeNamespaced(namespace string, resource string, data return v.client.Logical().Write(resource, data) } +//nolint:unparam +func (v *VaultProvider) deleteNamespaced(namespace string, resource string) (*vaultapi.Secret, error) { + defer v.setNamespace(namespace)() + return v.client.Logical().Delete(resource) +} + func (v *VaultProvider) setNamespace(namespace string) func() { if namespace != "" { v.clientMutex.Lock() diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index ab0575eabe..97b22e6970 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/go-hclog" vaultapi "github.com/hashicorp/vault/api" vaultconst "github.com/hashicorp/vault/sdk/helper/consts" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent/connect" @@ -543,12 +544,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{ @@ -1125,6 +1120,56 @@ func TestVaultCAProvider_GenerateIntermediate(t *testing.T) { require.NotEqual(t, orig, new) } +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) + + if res == nil { + t.Skip("Vault version < 1.11 does not have multi issuers functionality") + } + + // 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.GenerateIntermediate() + 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)