From c5369cbeb17aacfb1a15214dbf82f5ec327b47ca Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Tue, 8 Oct 2024 13:01:48 -0500 Subject: [PATCH 01/10] wip --- builtin/logical/pki/backend_test.go | 1 + builtin/logical/pki/crl_util.go | 12 + builtin/logical/pki/path_config_crl.go | 256 +++++------------- builtin/logical/pki/pki_backend/crl_config.go | 2 + 4 files changed, 90 insertions(+), 181 deletions(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index af079b13f781..3f80de3cc23b 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_size"], pki_backend.DefaultCrlConfig.MaxCRLSize) } func TestPKI_ListRevokedCerts(t *testing.T) { diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index def00a5f11c6..d26c5d31b281 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -1768,6 +1768,18 @@ func buildAnyCRLsWithCerts( internalCRLConfig.LastModified = time.Now().UTC() } + // Enforce the max CRL size guard before building the actual CRL + if globalCRLConfig.MaxCRLSize > -1 { + limit := maxCRLSizeOrDefault(globalCRLConfig.MaxCRLSize) + revokedCount := len(revokedCerts) + if revokedCount > 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 81815ff21592..b8b0469c8cd5 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -16,73 +16,79 @@ import ( "github.com/hashicorp/vault/sdk/logical" ) -func pathConfigCRL(b *backend) *framework.Path { - 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 +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 + 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. + 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, + }, + "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, + 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", - }, + Default: "false", + }, + "max_crl_size": { + 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. To disable set to -1.`, + Default: pki_backend.DefaultCrlConfig.MaxCRLSize, + }, +} + +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{ - "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. @@ -408,6 +294,13 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra return resp, nil } +func maxCRLSizeOrDefault(size int) int { + if size == 0 { + return pki_backend.DefaultCrlConfig.MaxCRLSize + } + return size +} + func genResponseFromCrlConfig(config *pki_backend.CrlConfig) *logical.Response { return &logical.Response{ Data: map[string]interface{}{ @@ -422,6 +315,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_size": maxCRLSizeOrDefault(config.MaxCRLSize), }, } } diff --git a/builtin/logical/pki/pki_backend/crl_config.go b/builtin/logical/pki/pki_backend/crl_config.go index f37fbbbb3321..6bb6e3372e49 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"` + MaxCRLSize int `json:"max_crl_size"` } // Implicit default values for the config if it does not exist. @@ -35,4 +36,5 @@ var DefaultCrlConfig = CrlConfig{ UseGlobalQueue: false, UnifiedCRL: false, UnifiedCRLOnExistingPaths: false, + MaxCRLSize: 100000, } From 4a97042cab0e80e9e69307e559c0ea0ebf22e9ed Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 9 Oct 2024 11:57:47 -0500 Subject: [PATCH 02/10] Unit test the CRL limit, wire up config --- builtin/logical/pki/crl_test.go | 28 +++++++++++++++++++++++--- builtin/logical/pki/path_config_crl.go | 4 ++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go index 3c7848db78de..31aed01bc9b0 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_size": 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_size": -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/path_config_crl.go b/builtin/logical/pki/path_config_crl.go index b8b0469c8cd5..16fe1bbcdbed 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -212,6 +212,10 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra config.UnifiedCRLOnExistingPaths = unifiedCrlOnExistingPathsRaw.(bool) } + if maxCRLSizeRaw, ok := d.GetOk("max_crl_size"); ok { + config.MaxCRLSize = maxCRLSizeRaw.(int) + } + if config.UnifiedCRLOnExistingPaths && !config.UnifiedCRL { return logical.ErrorResponse("unified_crl_on_existing_paths cannot be enabled if unified_crl is disabled"), nil } From 3fd3d5d741246de0004bb1db7d20a6a14714b9dc Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 9 Oct 2024 12:00:37 -0500 Subject: [PATCH 03/10] Bigger error --- builtin/logical/pki/crl_util.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index d26c5d31b281..f72820d275cd 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -1773,6 +1773,8 @@ func buildAnyCRLsWithCerts( limit := maxCRLSizeOrDefault(globalCRLConfig.MaxCRLSize) 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) { From 1eded6290f86f7a7e746fe0d3ae854ea4385438d Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 9 Oct 2024 13:28:39 -0500 Subject: [PATCH 04/10] API docs --- website/content/api-docs/secret/pki/index.mdx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index 5bf656e40784..ead288f24396 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_size": 100000 }, "auth": null } @@ -4037,6 +4038,11 @@ the CRL. without having to re-issue certificates or update scripts pulling a single CRL. +- `max_crl_size` `(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_size": 100000 } ``` From e96810f9e4c4f8dc431f59b667844391eec29b29 Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 9 Oct 2024 13:30:33 -0500 Subject: [PATCH 05/10] wording --- builtin/logical/pki/path_config_crl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/pki/path_config_crl.go b/builtin/logical/pki/path_config_crl.go index 16fe1bbcdbed..d04bdc76db87 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -75,7 +75,7 @@ existing CRL and OCSP paths will return the unified CRL instead of a response ba }, "max_crl_size": { 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. To disable set to -1.`, + 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.MaxCRLSize, }, } From 637305e1ae260c758834f246eac3c77d414debfd Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 9 Oct 2024 14:04:47 -0500 Subject: [PATCH 06/10] max_crl_entries, + ignore 0 or < -1 values to the config endpoint --- builtin/logical/pki/backend_test.go | 2 +- builtin/logical/pki/crl_test.go | 4 ++-- builtin/logical/pki/crl_util.go | 4 ++-- builtin/logical/pki/path_config_crl.go | 17 ++++++++++------- builtin/logical/pki/pki_backend/crl_config.go | 4 ++-- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 3f80de3cc23b..9292892144cd 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -6103,7 +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_size"], pki_backend.DefaultCrlConfig.MaxCRLSize) + 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 31aed01bc9b0..7f26f49a780f 100644 --- a/builtin/logical/pki/crl_test.go +++ b/builtin/logical/pki/crl_test.go @@ -384,7 +384,7 @@ func crlEnableDisableTestForBackend(t *testing.T, b *backend, s logical.Storage, // Set a limit, and test that it blocks building an over-large CRL CBWrite(b, s, "config/crl", map[string]interface{}{ - "max_crl_size": 6, + "max_crl_entries": 6, }) revoke(6, "revocation list size (7) exceeds configured maximum (6)") test(6) @@ -395,7 +395,7 @@ func crlEnableDisableTestForBackend(t *testing.T, b *backend, s logical.Storage, // Set unlimited, and try again CBWrite(b, s, "config/crl", map[string]interface{}{ - "max_crl_size": -1, + "max_crl_entries": -1, }) _, err = CBRead(b, s, "crl/rotate") require.NoError(t, err) diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index f72820d275cd..99ec95a79757 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -1769,8 +1769,8 @@ func buildAnyCRLsWithCerts( } // Enforce the max CRL size guard before building the actual CRL - if globalCRLConfig.MaxCRLSize > -1 { - limit := maxCRLSizeOrDefault(globalCRLConfig.MaxCRLSize) + 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 diff --git a/builtin/logical/pki/path_config_crl.go b/builtin/logical/pki/path_config_crl.go index d04bdc76db87..a9cdcf5ca1cd 100644 --- a/builtin/logical/pki/path_config_crl.go +++ b/builtin/logical/pki/path_config_crl.go @@ -73,10 +73,10 @@ 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_size": { + "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.MaxCRLSize, + Default: pki_backend.DefaultCrlConfig.MaxCRLEntries, }, } @@ -212,8 +212,11 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra config.UnifiedCRLOnExistingPaths = unifiedCrlOnExistingPathsRaw.(bool) } - if maxCRLSizeRaw, ok := d.GetOk("max_crl_size"); ok { - config.MaxCRLSize = maxCRLSizeRaw.(int) + 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 { @@ -298,9 +301,9 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra return resp, nil } -func maxCRLSizeOrDefault(size int) int { +func maxCRLEntriesOrDefault(size int) int { if size == 0 { - return pki_backend.DefaultCrlConfig.MaxCRLSize + return pki_backend.DefaultCrlConfig.MaxCRLEntries } return size } @@ -319,7 +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_size": maxCRLSizeOrDefault(config.MaxCRLSize), + "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 6bb6e3372e49..99ccf6f1208f 100644 --- a/builtin/logical/pki/pki_backend/crl_config.go +++ b/builtin/logical/pki/pki_backend/crl_config.go @@ -19,7 +19,7 @@ type CrlConfig struct { UseGlobalQueue bool `json:"cross_cluster_revocation"` UnifiedCRL bool `json:"unified_crl"` UnifiedCRLOnExistingPaths bool `json:"unified_crl_on_existing_paths"` - MaxCRLSize int `json:"max_crl_size"` + MaxCRLEntries int `json:"max_crl_entries"` } // Implicit default values for the config if it does not exist. @@ -36,5 +36,5 @@ var DefaultCrlConfig = CrlConfig{ UseGlobalQueue: false, UnifiedCRL: false, UnifiedCRLOnExistingPaths: false, - MaxCRLSize: 100000, + MaxCRLEntries: 100000, } From 10e0671145882da16d6983f78918d79c5671c9fe Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 9 Oct 2024 14:05:57 -0500 Subject: [PATCH 07/10] changelog --- changelog/28654.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/28654.txt diff --git a/changelog/28654.txt b/changelog/28654.txt new file mode 100644 index 000000000000..35f7c619e470 --- /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. +``` From b14405ecede061668df1c6416a4998ef343d24ab Mon Sep 17 00:00:00 2001 From: "Scott G. Miller" Date: Wed, 9 Oct 2024 14:22:18 -0500 Subject: [PATCH 08/10] rename field in docs --- website/content/api-docs/secret/pki/index.mdx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index ead288f24396..ef39d0f99709 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/website/content/api-docs/secret/pki/index.mdx @@ -3929,7 +3929,7 @@ $ curl \ "cross_cluster_revocation": true, "unified_crl": true, "unified_crl_on_existing_paths": true, - "max_crl_size": 100000 + "max_crl_entries": 100000 }, "auth": null } @@ -4038,7 +4038,7 @@ the CRL. without having to re-issue certificates or update scripts pulling a single CRL. -- `max_crl_size` `(int: 100000)` - +- `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. @@ -4058,7 +4058,7 @@ the CRL. "cross_cluster_revocation": true, "unified_crl": true, "unified_crl_on_existing_paths": true, - "max_crl_size": 100000 + "max_crl_entries": 100000 } ``` From 52eb7eb28ac98bca8d5689c870e92a39941cb757 Mon Sep 17 00:00:00 2001 From: Scott Miller Date: Wed, 9 Oct 2024 15:08:21 -0500 Subject: [PATCH 09/10] Update website/content/api-docs/secret/pki/index.mdx Co-authored-by: Steven Clark --- website/content/api-docs/secret/pki/index.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index ead288f24396..2cc8dda49870 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/website/content/api-docs/secret/pki/index.mdx @@ -3929,7 +3929,7 @@ $ curl \ "cross_cluster_revocation": true, "unified_crl": true, "unified_crl_on_existing_paths": true, - "max_crl_size": 100000 + "max_crl_entries": 100000 }, "auth": null } From bae28742542dc9ebc7ded61a5c4029f86bcabbc9 Mon Sep 17 00:00:00 2001 From: Scott Miller Date: Wed, 9 Oct 2024 15:08:40 -0500 Subject: [PATCH 10/10] Update website/content/api-docs/secret/pki/index.mdx Co-authored-by: Steven Clark --- website/content/api-docs/secret/pki/index.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index 2cc8dda49870..2afec3147345 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/website/content/api-docs/secret/pki/index.mdx @@ -4058,7 +4058,7 @@ the CRL. "cross_cluster_revocation": true, "unified_crl": true, "unified_crl_on_existing_paths": true, - "max_crl_size": 100000 + "max_crl_entries": 100000 } ```