Clean up unused CRL entries when issuer is removed (hashicorp#23007)

* Clean up unused CRL entries when issuer is removed

When a issuer is removed, the space utilized by its CRL was not freed,
both from the CRL config mapping issuer IDs to CRL IDs and from the
CRL storage entry. We thus implement a two step cleanup, wherein
orphaned CRL IDs are removed from the config and any remaining full
CRL entries are removed from disk.

This relates to a Consul<->Vault interop issue (hashicorp#22980), wherein Consul
creates a new issuer on every leadership election, causing this config
to grow. Deleting issuers manually does not entirely solve this problem
as the config does not fully reclaim space used in this entry.

Notably, an observation that when deleting issuers, the CRL was rebuilt
on secondary clusters (due to the invalidation not caring about type of
the operation); for consistency and to clean up the unified CRLs, we
also need to run the rebuild on the active primary cluster that deleted
the issuer as well.

This approach does allow cleanup on existing impacted clusters by simply
rebuilding the CRL.

Co-authored-by: Steven Clark <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>

* Add test case on CRL removal

Signed-off-by: Alexander Scheel <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>

---------

Signed-off-by: Alexander Scheel <[email protected]>
Co-authored-by: Steven Clark <[email protected]>

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.
+```