Add a max_crl_size parameter to CRL config (#28654)

* wip

* Unit test the CRL limit, wire up config

* Bigger error

* API docs

* wording

* max_crl_entries, + ignore 0 or < -1 values to the config endpoint

* changelog

* rename field in docs

* Update website/content/api-docs/secret/pki/index.mdx

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>

* Update website/content/api-docs/secret/pki/index.mdx

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>

---------

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>

builtin/logical/pki/backend_test.go

@@ -6103,6 +6103,7 @@ func TestPKI_EmptyCRLConfigUpgraded(t *testing.T) {
 	require.Equal(t, resp.Data["auto_rebuild_grace_period"], pki_backend.DefaultCrlConfig.AutoRebuildGracePeriod)
 	require.Equal(t, resp.Data["enable_delta"], pki_backend.DefaultCrlConfig.EnableDelta)
 	require.Equal(t, resp.Data["delta_rebuild_interval"], pki_backend.DefaultCrlConfig.DeltaRebuildInterval)
+	require.Equal(t, resp.Data["max_crl_entries"], pki_backend.DefaultCrlConfig.MaxCRLEntries)
 }
 
 func TestPKI_ListRevokedCerts(t *testing.T) {

builtin/logical/pki/crl_test.go

@@ -285,7 +285,7 @@ func crlEnableDisableTestForBackend(t *testing.T, b *backend, s logical.Storage,
 	}
 
 	serials := make(map[int]string)
-	for i := 0; i < 6; i++ {
+	for i := 0; i < 7; i++ {
 		resp, err := CBWrite(b, s, "issue/test", map[string]interface{}{
 			"common_name": "test.foobar.com",
 		})
@@ -323,11 +323,15 @@ func crlEnableDisableTestForBackend(t *testing.T, b *backend, s logical.Storage,
 		}
 	}
 
-	revoke := func(serialIndex int) {
+	revoke := func(serialIndex int, errorText ...string) {
 		_, err = CBWrite(b, s, "revoke", map[string]interface{}{
 			"serial_number": serials[serialIndex],
 		})
-		if err != nil {
+		if err != nil && len(errorText) == 1 {
+			if strings.Contains(err.Error(), errorText[0]) {
+				err = nil
+				return
+			}
 			t.Fatal(err)
 		}
 
@@ -377,6 +381,24 @@ func crlEnableDisableTestForBackend(t *testing.T, b *backend, s logical.Storage,
 
 	crlCreationTime2 := getParsedCrlFromBackend(t, b, s, "crl").TBSCertList.ThisUpdate
 	require.NotEqual(t, crlCreationTime1, crlCreationTime2)
+
+	// Set a limit, and test that it blocks building an over-large CRL
+	CBWrite(b, s, "config/crl", map[string]interface{}{
+		"max_crl_entries": 6,
+	})
+	revoke(6, "revocation list size (7) exceeds configured maximum (6)")
+	test(6)
+
+	_, err = CBRead(b, s, "crl/rotate")
+	require.Error(t, err)
+	require.True(t, strings.Contains(err.Error(), "revocation list size (7) exceeds configured maximum (6)"))
+
+	// Set unlimited, and try again
+	CBWrite(b, s, "config/crl", map[string]interface{}{
+		"max_crl_entries": -1,
+	})
+	_, err = CBRead(b, s, "crl/rotate")
+	require.NoError(t, err)
 }
 
 func TestBackend_Secondary_CRL_Rebuilding(t *testing.T) {

builtin/logical/pki/crl_util.go

@@ -1768,6 +1768,20 @@ func buildAnyCRLsWithCerts(
 				internalCRLConfig.LastModified = time.Now().UTC()
 			}
 
+			// Enforce the max CRL size guard before building the actual CRL
+			if globalCRLConfig.MaxCRLEntries > -1 {
+				limit := maxCRLEntriesOrDefault(globalCRLConfig.MaxCRLEntries)
+				revokedCount := len(revokedCerts)
+				if revokedCount > limit {
+					// Also log a nasty error to get the operator's attention
+					sc.Logger().Error("CRL was not updated, as it exceeds the configured max size.  The CRL now does not contain all revoked certificates!  This may be indicative of a runaway issuance/revocation pattern.", "limit", limit)
+					return nil, fmt.Errorf("error building CRL: revocation list size (%d) exceeds configured maximum (%d)", revokedCount, limit)
+				}
+				if revokedCount > int(float32(limit)*0.90) {
+					sc.Logger().Warn("warning, revoked certificate count is within 10% of the configured maximum CRL size", "revoked_certs", revokedCount, "limit", limit)
+				}
+			}
+
 			// Lastly, build the CRL.
 			nextUpdate, err := buildCRL(sc, globalCRLConfig, forceNew, representative, revokedCerts, crlIdentifier, crlNumber, isUnified, isDelta, lastCompleteNumber)
 			if err != nil {

builtin/logical/pki/path_config_crl.go

@@ -16,15 +16,7 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
-func pathConfigCRL(b *backend) *framework.Path {
+var configCRLFields = map[string]*framework.FieldSchema{
-	return &framework.Path{
-		Pattern: "config/crl",
-
-		DisplayAttrs: &framework.DisplayAttributes{
-			OperationPrefix: operationPrefixPKI,
-		},
-
-		Fields: map[string]*framework.FieldSchema{
 	"expiry": {
 		Type: framework.TypeString,
 		Description: `The amount of time the generated CRL should be
@@ -81,8 +73,22 @@ disable for CRLs and ocsp_disable for OCSP.`,
 existing CRL and OCSP paths will return the unified CRL instead of a response based on cluster-local data`,
 		Default: "false",
 	},
+	"max_crl_entries": {
+		Type:        framework.TypeInt,
+		Description: `The maximum number of entries the CRL can contain.  This is meant as a guard against accidental runaway revocations overloading Vault storage.  If this limit is exceeded writing the CRL will fail.  If set to -1 this limit is disabled.`,
+		Default:     pki_backend.DefaultCrlConfig.MaxCRLEntries,
+	},
+}
+
+func pathConfigCRL(b *backend) *framework.Path {
+	return &framework.Path{
+		Pattern: "config/crl",
+
+		DisplayAttrs: &framework.DisplayAttributes{
+			OperationPrefix: operationPrefixPKI,
 		},
 
+		Fields: configCRLFields,
 		Operations: map[logical.Operation]framework.OperationHandler{
 			logical.ReadOperation: &framework.PathOperation{
 				DisplayAttrs: &framework.DisplayAttributes{
@@ -92,69 +98,7 @@ existing CRL and OCSP paths will return the unified CRL instead of a response ba
 				Responses: map[int][]framework.Response{
 					http.StatusOK: {{
 						Description: "OK",
-						Fields: map[string]*framework.FieldSchema{
+						Fields:      configCRLFields,
-							"expiry": {
-								Type: framework.TypeString,
-								Description: `The amount of time the generated CRL should be
-valid; defaults to 72 hours`,
-								Required: true,
-							},
-							"disable": {
-								Type:        framework.TypeBool,
-								Description: `If set to true, disables generating the CRL entirely.`,
-								Required:    true,
-							},
-							"ocsp_disable": {
-								Type:        framework.TypeBool,
-								Description: `If set to true, ocsp unauthorized responses will be returned.`,
-								Required:    true,
-							},
-							"ocsp_expiry": {
-								Type: framework.TypeString,
-								Description: `The amount of time an OCSP response will be valid (controls 
-the NextUpdate field); defaults to 12 hours`,
-								Required: true,
-							},
-							"auto_rebuild": {
-								Type:        framework.TypeBool,
-								Description: `If set to true, enables automatic rebuilding of the CRL`,
-								Required:    true,
-							},
-							"auto_rebuild_grace_period": {
-								Type:        framework.TypeString,
-								Description: `The time before the CRL expires to automatically rebuild it, when enabled. Must be shorter than the CRL expiry. Defaults to 12h.`,
-								Required:    true,
-							},
-							"enable_delta": {
-								Type:        framework.TypeBool,
-								Description: `Whether to enable delta CRLs between authoritative CRL rebuilds`,
-								Required:    true,
-							},
-							"delta_rebuild_interval": {
-								Type:        framework.TypeString,
-								Description: `The time between delta CRL rebuilds if a new revocation has occurred. Must be shorter than the CRL expiry. Defaults to 15m.`,
-								Required:    true,
-							},
-							"cross_cluster_revocation": {
-								Type: framework.TypeBool,
-								Description: `Whether to enable a global, cross-cluster revocation queue.
-Must be used with auto_rebuild=true.`,
-								Required: true,
-							},
-							"unified_crl": {
-								Type: framework.TypeBool,
-								Description: `If set to true enables global replication of revocation entries,
-also enabling unified versions of OCSP and CRLs if their respective features are enabled.
-disable for CRLs and ocsp_disable for OCSP.`,
-								Required: true,
-							},
-							"unified_crl_on_existing_paths": {
-								Type: framework.TypeBool,
-								Description: `If set to true, 
-existing CRL and OCSP paths will return the unified CRL instead of a response based on cluster-local data`,
-								Required: true,
-							},
-						},
 					}},
 				},
 			},
@@ -167,65 +111,7 @@ existing CRL and OCSP paths will return the unified CRL instead of a response ba
 				Responses: map[int][]framework.Response{
 					http.StatusOK: {{
 						Description: "OK",
-						Fields: map[string]*framework.FieldSchema{
+						Fields:      configCRLFields,
-							"expiry": {
-								Type: framework.TypeString,
-								Description: `The amount of time the generated CRL should be
-valid; defaults to 72 hours`,
-								Default: "72h",
-							},
-							"disable": {
-								Type:        framework.TypeBool,
-								Description: `If set to true, disables generating the CRL entirely.`,
-							},
-							"ocsp_disable": {
-								Type:        framework.TypeBool,
-								Description: `If set to true, ocsp unauthorized responses will be returned.`,
-							},
-							"ocsp_expiry": {
-								Type: framework.TypeString,
-								Description: `The amount of time an OCSP response will be valid (controls 
-the NextUpdate field); defaults to 12 hours`,
-								Default: "1h",
-							},
-							"auto_rebuild": {
-								Type:        framework.TypeBool,
-								Description: `If set to true, enables automatic rebuilding of the CRL`,
-							},
-							"auto_rebuild_grace_period": {
-								Type:        framework.TypeString,
-								Description: `The time before the CRL expires to automatically rebuild it, when enabled. Must be shorter than the CRL expiry. Defaults to 12h.`,
-								Default:     "12h",
-							},
-							"enable_delta": {
-								Type:        framework.TypeBool,
-								Description: `Whether to enable delta CRLs between authoritative CRL rebuilds`,
-							},
-							"delta_rebuild_interval": {
-								Type:        framework.TypeString,
-								Description: `The time between delta CRL rebuilds if a new revocation has occurred. Must be shorter than the CRL expiry. Defaults to 15m.`,
-								Default:     "15m",
-							},
-							"cross_cluster_revocation": {
-								Type: framework.TypeBool,
-								Description: `Whether to enable a global, cross-cluster revocation queue.
-Must be used with auto_rebuild=true.`,
-								Required: false,
-							},
-							"unified_crl": {
-								Type: framework.TypeBool,
-								Description: `If set to true enables global replication of revocation entries,
-also enabling unified versions of OCSP and CRLs if their respective features are enabled.
-disable for CRLs and ocsp_disable for OCSP.`,
-								Required: false,
-							},
-							"unified_crl_on_existing_paths": {
-								Type: framework.TypeBool,
-								Description: `If set to true, 
-existing CRL and OCSP paths will return the unified CRL instead of a response based on cluster-local data`,
-								Required: false,
-							},
-						},
 					}},
 				},
 				// Read more about why these flags are set in backend.go.
@@ -326,6 +212,13 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra
 		config.UnifiedCRLOnExistingPaths = unifiedCrlOnExistingPathsRaw.(bool)
 	}
 
+	if maxCRLEntriesRaw, ok := d.GetOk("max_crl_entries"); ok {
+		v := maxCRLEntriesRaw.(int)
+		if v == -1 || v > 0 {
+			config.MaxCRLEntries = v
+		}
+	}
+
 	if config.UnifiedCRLOnExistingPaths && !config.UnifiedCRL {
 		return logical.ErrorResponse("unified_crl_on_existing_paths cannot be enabled if unified_crl is disabled"), nil
 	}
@@ -408,6 +301,13 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra
 	return resp, nil
 }
 
+func maxCRLEntriesOrDefault(size int) int {
+	if size == 0 {
+		return pki_backend.DefaultCrlConfig.MaxCRLEntries
+	}
+	return size
+}
+
 func genResponseFromCrlConfig(config *pki_backend.CrlConfig) *logical.Response {
 	return &logical.Response{
 		Data: map[string]interface{}{
@@ -422,6 +322,7 @@ func genResponseFromCrlConfig(config *pki_backend.CrlConfig) *logical.Response {
 			"cross_cluster_revocation":      config.UseGlobalQueue,
 			"unified_crl":                   config.UnifiedCRL,
 			"unified_crl_on_existing_paths": config.UnifiedCRLOnExistingPaths,
+			"max_crl_entries":               maxCRLEntriesOrDefault(config.MaxCRLEntries),
 		},
 	}
 }

builtin/logical/pki/pki_backend/crl_config.go

@@ -19,6 +19,7 @@ type CrlConfig struct {
 	UseGlobalQueue            bool   `json:"cross_cluster_revocation"`
 	UnifiedCRL                bool   `json:"unified_crl"`
 	UnifiedCRLOnExistingPaths bool   `json:"unified_crl_on_existing_paths"`
+	MaxCRLEntries             int    `json:"max_crl_entries"`
 }
 
 // Implicit default values for the config if it does not exist.
@@ -35,4 +36,5 @@ var DefaultCrlConfig = CrlConfig{
 	UseGlobalQueue:            false,
 	UnifiedCRL:                false,
 	UnifiedCRLOnExistingPaths: false,
+	MaxCRLEntries:             100000,
 }

changelog/28654.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+secrets/pki: Add a CRL entry limit to prevent runaway revocations from overloading Vault, reconfigurable with max_crl_entries on the CRL config.
+```

website/content/api-docs/secret/pki/index.mdx

@@ -3928,7 +3928,8 @@ $ curl \
     "delta_rebuild_interval": "15m",
     "cross_cluster_revocation": true,
     "unified_crl": true,
-    "unified_crl_on_existing_paths": true
+    "unified_crl_on_existing_paths": true,
+    "max_crl_entries": 100000
   },
   "auth": null
 }
@@ -4037,6 +4038,11 @@ the CRL.
   without having to re-issue certificates or update scripts pulling
   a single CRL.
 
+- `max_crl_entries` `(int: 100000)` -
+  The maximum number of entries a CRL can contain.  This option exists to
+  prevent accidental runaway issuance/revocation from overloading Vault. 
+  If set to -1, the limit is disabled.
+
 #### Sample payload
 
 ```json
@@ -4052,6 +4058,7 @@ the CRL.
   "cross_cluster_revocation": true,
   "unified_crl": true,
   "unified_crl_on_existing_paths": true,
+  "max_crl_entries": 100000
 }
 ```