Backport of Clean up unused CRL entries when issuer is removed into r…

…elease/1.12.x (#23028)

* backport of commit e2ff1f1 (#23030)

Co-authored-by: Alexander Scheel <[email protected]>

* backport of commit 293e8b8 (#23045)

Co-authored-by: Steven Clark <[email protected]>

* Remove unified CRLs references from v1.12.x

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

---------

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

builtin/logical/pki/crl_test.go

@@ -1318,3 +1318,67 @@ func requestCrlFromBackend(t *testing.T, s logical.Storage, b *backend) *logical
 	require.False(t, resp.IsError(), "crl error response: %v", resp)
 	return resp
 }
+
+func TestCRLIssuerRemoval(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	b, s := createBackendWithStorage(t)
+
+	// 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)
+
+	// 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))
+}

builtin/logical/pki/path_fetch_issuers.go

@@ -891,6 +891,14 @@ 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.
+	err = b.crlBuilder.rebuild(ctx, b, req, false)
+	if err != nil {
+		return nil, err
+	}
+
 	return response, nil
 }
 

builtin/logical/pki/storage.go

@@ -834,7 +834,86 @@ func areCertificatesEqual(cert1 *x509.Certificate, cert2 *x509.Certificate) bool
 	return bytes.Equal(cert1.Raw, cert2.Raw)
 }
 
+func (sc *storageContext) _cleanupInternalCRLMapping(mapping *localCRLConfigEntry) 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
+		}
+	}
+
+	// Vault 1.12.x only has local CRLs
+	baseCRLPath := "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) setLocalCRLConfig(mapping *localCRLConfigEntry) error {
+	if err := sc._cleanupInternalCRLMapping(mapping); err != nil {
+		return fmt.Errorf("failed to clean up internal CRL mapping: %w", err)
+	}
+
 	json, err := logical.StorageEntryJSON(storageLocalCRLConfig, 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.
+```