diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go index ea9b3af15..24d048b4f 100644 --- a/builtin/logical/pki/crl_test.go +++ b/builtin/logical/pki/crl_test.go @@ -12,12 +12,15 @@ import ( "testing" "time" - "github.com/hashicorp/go-secure-stdlib/parseutil" "github.com/hashicorp/vault/api" + "github.com/hashicorp/vault/helper/constants" vaulthttp "github.com/hashicorp/vault/http" "github.com/hashicorp/vault/sdk/helper/testhelpers/schema" "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/vault" + + "github.com/hashicorp/go-secure-stdlib/parseutil" + "github.com/stretchr/testify/require" ) @@ -1436,3 +1439,89 @@ hbiiPARizZA/Tsna/9ox1qDT require.NotNil(t, resp) require.Empty(t, resp.Warnings) } + +func TestCRLIssuerRemoval(t *testing.T) { + t.Parallel() + + ctx := context.Background() + b, s := CreateBackendWithStorage(t) + + if constants.IsEnterprise { + // We don't really care about the whole cross cluster replication + // stuff, but we do want to enable unified CRLs if we can, so that + // unified CRLs get built. + _, err := CBWrite(b, s, "config/crl", map[string]interface{}{ + "cross_cluster_revocation": true, + }) + require.NoError(t, err, "failed enabling unified CRLs on enterprise") + } + + // Create a single root, configure delta CRLs, and rotate CRLs to prep a + // starting state. + _, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{ + "common_name": "Root R1", + "key_type": "ec", + }) + require.NoError(t, err) + _, err = CBWrite(b, s, "config/crl", map[string]interface{}{ + "enable_delta": true, + "auto_rebuild": true, + }) + require.NoError(t, err) + _, err = CBRead(b, s, "crl/rotate") + require.NoError(t, err) + + // List items in storage under both CRL paths so we know what is there in + // the "good" state. + crlList, err := s.List(ctx, "crls/") + require.NoError(t, err) + require.Contains(t, crlList, "config") + require.Greater(t, len(crlList), 1) + + unifiedCRLList, err := s.List(ctx, "unified-crls/") + require.NoError(t, err) + require.Contains(t, unifiedCRLList, "config") + require.Greater(t, len(unifiedCRLList), 1) + + // Now, create a bunch of issuers, generate CRLs, and remove them. + var keyIDs []string + var issuerIDs []string + for i := 1; i <= 25; i++ { + resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{ + "common_name": fmt.Sprintf("Root X%v", i), + "key_type": "ec", + }) + require.NoError(t, err) + require.NotNil(t, resp) + + key := string(resp.Data["key_id"].(keyID)) + keyIDs = append(keyIDs, key) + issuer := string(resp.Data["issuer_id"].(issuerID)) + issuerIDs = append(issuerIDs, issuer) + } + _, err = CBRead(b, s, "crl/rotate") + require.NoError(t, err) + for _, issuer := range issuerIDs { + _, err := CBDelete(b, s, "issuer/"+issuer) + require.NoError(t, err) + } + for _, key := range keyIDs { + _, err := CBDelete(b, s, "key/"+key) + require.NoError(t, err) + } + + // Finally list storage entries again to ensure they are cleaned up. + afterCRLList, err := s.List(ctx, "crls/") + require.NoError(t, err) + for _, entry := range crlList { + require.Contains(t, afterCRLList, entry) + } + require.Equal(t, len(afterCRLList), len(crlList)) + + afterUnifiedCRLList, err := s.List(ctx, "unified-crls/") + require.NoError(t, err) + for _, entry := range unifiedCRLList { + require.Contains(t, afterUnifiedCRLList, entry) + } + require.Equal(t, len(afterUnifiedCRLList), len(unifiedCRLList)) +} diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index 759436f60..e0970e020 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/builtin/logical/pki/path_fetch_issuers.go @@ -1117,6 +1117,18 @@ func (b *backend) pathDeleteIssuer(ctx context.Context, req *logical.Request, da response.AddWarning(msg) } + // Finally, we need to rebuild both the local and the unified CRLs. This + // will free up any now unnecessary space used in both the CRL config + // and for the underlying CRL. + warnings, err := b.crlBuilder.rebuild(sc, true) + if err != nil { + return nil, err + } + + for index, warning := range warnings { + response.AddWarning(fmt.Sprintf("Warning %d during CRL rebuild: %v", index+1, warning)) + } + return response, nil } diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index 52b0faf2c..e329a2bcd 100644 --- a/builtin/logical/pki/storage.go +++ b/builtin/logical/pki/storage.go @@ -930,7 +930,91 @@ func areCertificatesEqual(cert1 *x509.Certificate, cert2 *x509.Certificate) bool return bytes.Equal(cert1.Raw, cert2.Raw) } +func (sc *storageContext) _cleanupInternalCRLMapping(mapping *internalCRLConfigEntry, path string) error { + // Track which CRL IDs are presently referred to by issuers; any other CRL + // IDs are subject to cleanup. + // + // Unused IDs both need to be removed from this map (cleaning up the size + // of this storage entry) but also the full CRLs removed from disk. + presentMap := make(map[crlID]bool) + for _, id := range mapping.IssuerIDCRLMap { + presentMap[id] = true + } + + // Identify which CRL IDs exist and are candidates for removal; + // theoretically these three maps should be in sync, but were added + // at different times. + toRemove := make(map[crlID]bool) + for id := range mapping.CRLNumberMap { + if !presentMap[id] { + toRemove[id] = true + } + } + for id := range mapping.LastCompleteNumberMap { + if !presentMap[id] { + toRemove[id] = true + } + } + for id := range mapping.CRLExpirationMap { + if !presentMap[id] { + toRemove[id] = true + } + } + + // Depending on which path we're writing this config to, we need to + // remove CRLs from the relevant folder too. + isLocal := path == storageLocalCRLConfig + baseCRLPath := "crls/" + if !isLocal { + baseCRLPath = "unified-crls/" + } + + for id := range toRemove { + // Clean up space in this mapping... + delete(mapping.CRLNumberMap, id) + delete(mapping.LastCompleteNumberMap, id) + delete(mapping.CRLExpirationMap, id) + + // And clean up space on disk from the fat CRL mapping. + crlPath := baseCRLPath + string(id) + deltaCRLPath := crlPath + "-delta" + if err := sc.Storage.Delete(sc.Context, crlPath); err != nil { + return fmt.Errorf("failed to delete unreferenced CRL %v: %w", id, err) + } + if err := sc.Storage.Delete(sc.Context, deltaCRLPath); err != nil { + return fmt.Errorf("failed to delete unreferenced delta CRL %v: %w", id, err) + } + } + + // Lastly, some CRLs could've been partially removed from the map but + // not from disk. Check to see if we have any dangling CRLs and remove + // them too. + list, err := sc.Storage.List(sc.Context, baseCRLPath) + if err != nil { + return fmt.Errorf("failed listing all CRLs: %w", err) + } + for _, crl := range list { + if crl == "config" || strings.HasSuffix(crl, "/") { + continue + } + + if presentMap[crlID(crl)] { + continue + } + + if err := sc.Storage.Delete(sc.Context, baseCRLPath+"/"+crl); err != nil { + return fmt.Errorf("failed cleaning up orphaned CRL %v: %w", crl, err) + } + } + + return nil +} + func (sc *storageContext) _setInternalCRLConfig(mapping *internalCRLConfigEntry, path string) error { + if err := sc._cleanupInternalCRLMapping(mapping, path); err != nil { + return fmt.Errorf("failed to clean up internal CRL mapping: %w", err) + } + json, err := logical.StorageEntryJSON(path, mapping) if err != nil { return err diff --git a/changelog/23007.txt b/changelog/23007.txt new file mode 100644 index 000000000..02fee8c15 --- /dev/null +++ b/changelog/23007.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: Fix removal of issuers to clean up unreferenced CRLs. +```