diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index af079b13f7..9292892144 100644 --- a/builtin/logical/pki/backend_test.go +++ b/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) { diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go index 3c7848db78..7f26f49a78 100644 --- a/builtin/logical/pki/crl_test.go +++ b/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) { diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index def00a5f11..99ec95a797 100644 --- a/builtin/logical/pki/crl_util.go +++ b/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 { diff --git a/builtin/logical/pki/path_config_crl.go b/builtin/logical/pki/path_config_crl.go index 81815ff215..a9cdcf5ca1 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -16,6 +16,70 @@ import ( "github.com/hashicorp/vault/sdk/logical" ) +var configCRLFields = map[string]*framework.FieldSchema{ + "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.`, + }, + "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.`, + Default: "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`, + 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", @@ -24,65 +88,7 @@ func pathConfigCRL(b *backend) *framework.Path { OperationPrefix: operationPrefixPKI, }, - Fields: map[string]*framework.FieldSchema{ - "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.`, - }, - "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.`, - Default: "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`, - Default: "false", - }, - }, - + 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{ - "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, - }, - }, + Fields: configCRLFields, }}, }, }, @@ -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{ - "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, - }, - }, + Fields: configCRLFields, }}, }, // 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), }, } } diff --git a/builtin/logical/pki/pki_backend/crl_config.go b/builtin/logical/pki/pki_backend/crl_config.go index f37fbbbb33..99ccf6f120 100644 --- a/builtin/logical/pki/pki_backend/crl_config.go +++ b/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, } diff --git a/changelog/28654.txt b/changelog/28654.txt new file mode 100644 index 0000000000..35f7c619e4 --- /dev/null +++ b/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. +``` diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index 5bf656e407..ef39d0f997 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/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 } ```