Clean up unused CRL entries when issuer is removed

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))
+}

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
 }
 

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

changelog/23007.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Fix removal of issuers to clean up unreferenced CRLs.
+```