From 7ad778541eea11496199f3820ca84a1e4736c492 Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Fri, 16 Feb 2024 17:50:18 -0500 Subject: [PATCH] Disable Request Limiter by default (#25442) This PR flips the logic for the Request Limiter, setting it to default disabled. We allow users to turn on the global Request Limiter, but leave the Listener configuration as a "disable per Listener". --- changelog/25095.txt | 2 +- .../command_testonly/server_testonly_test.go | 56 +++++++++---------- command/server.go | 8 ++- command/server/config_util_test.go | 2 +- internalshared/configutil/request_limiter.go | 5 +- vault/core.go | 7 +-- 6 files changed, 40 insertions(+), 40 deletions(-) diff --git a/changelog/25095.txt b/changelog/25095.txt index 5266439ef2..69251984a9 100644 --- a/changelog/25095.txt +++ b/changelog/25095.txt @@ -1,3 +1,3 @@ ```release-note:improvement -limits: Introduce a reloadable disable configuration for the Request Limiter. +limits: Introduce a reloadable opt-in configuration for the Request Limiter. ``` diff --git a/command/command_testonly/server_testonly_test.go b/command/command_testonly/server_testonly_test.go index 643aa9566a..68f8943440 100644 --- a/command/command_testonly/server_testonly_test.go +++ b/command/command_testonly/server_testonly_test.go @@ -85,31 +85,6 @@ func TestServer_ReloadRequestLimiter(t *testing.T) { configAfter string expectedResponse *vault.RequestLimiterResponse }{ - { - "enable after default", - baseHCL + requestLimiterEnableHCL, - enabledResponse, - }, - { - "enable after enable", - baseHCL + requestLimiterEnableHCL, - enabledResponse, - }, - { - "disable after enable", - baseHCL + requestLimiterDisableHCL, - disabledResponse, - }, - { - "default after disable", - baseHCL, - enabledResponse, - }, - { - "default after default", - baseHCL, - enabledResponse, - }, { "disable after default", baseHCL + requestLimiterDisableHCL, @@ -120,6 +95,31 @@ func TestServer_ReloadRequestLimiter(t *testing.T) { baseHCL + requestLimiterDisableHCL, disabledResponse, }, + { + "enable after disable", + baseHCL + requestLimiterEnableHCL, + enabledResponse, + }, + { + "default after enable", + baseHCL, + disabledResponse, + }, + { + "default after default", + baseHCL, + disabledResponse, + }, + { + "enable after default", + baseHCL + requestLimiterEnableHCL, + enabledResponse, + }, + { + "enable after enable", + baseHCL + requestLimiterEnableHCL, + enabledResponse, + }, } ui, srv := command.TestServerCommand(t) @@ -166,7 +166,7 @@ func TestServer_ReloadRequestLimiter(t *testing.T) { cli.SetToken(initResp.RootToken) output = ui.ErrorWriter.String() + ui.OutputWriter.String() - require.Contains(t, output, "Request Limiter: enabled") + require.Contains(t, output, "Request Limiter: disabled") verifyLimiters := func(t *testing.T, expectedResponse *vault.RequestLimiterResponse) { t.Helper() @@ -187,8 +187,8 @@ func TestServer_ReloadRequestLimiter(t *testing.T) { require.Equal(t, expectedResponse, limiters) } - // Start off with default enabled - verifyLimiters(t, enabledResponse) + // Start off with default disabled + verifyLimiters(t, disabledResponse) for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/command/server.go b/command/server.go index 614616557f..402a923f7e 100644 --- a/command/server.go +++ b/command/server.go @@ -1441,9 +1441,9 @@ func (c *ServerCommand) Run(args []string) int { info["administrative namespace"] = config.AdministrativeNamespacePath infoKeys = append(infoKeys, "request limiter") - info["request limiter"] = "enabled" - if config.RequestLimiter != nil && config.RequestLimiter.Disable { - info["request limiter"] = "disabled" + info["request limiter"] = "disabled" + if config.RequestLimiter != nil && !config.RequestLimiter.Disable { + info["request limiter"] = "enabled" } sort.Strings(infoKeys) @@ -3120,6 +3120,8 @@ func createCoreConfig(c *ServerCommand, config *server.Config, backend physical. if config.RequestLimiter != nil { coreConfig.DisableRequestLimiter = config.RequestLimiter.Disable + } else { + coreConfig.DisableRequestLimiter = true } if c.flagDev { diff --git a/command/server/config_util_test.go b/command/server/config_util_test.go index ace9bb1689..81c5212584 100644 --- a/command/server/config_util_test.go +++ b/command/server/config_util_test.go @@ -112,7 +112,7 @@ request_limiter { name: "invalid disable", inConfig: ` request_limiter { - disable = "whywouldyoudothis" + disable = "people make mistakes" }`, outErr: true, }, diff --git a/internalshared/configutil/request_limiter.go b/internalshared/configutil/request_limiter.go index 1ce028940b..1bc97bfada 100644 --- a/internalshared/configutil/request_limiter.go +++ b/internalshared/configutil/request_limiter.go @@ -28,7 +28,7 @@ func (r *RequestLimiter) GoString() string { } var DefaultRequestLimiter = &RequestLimiter{ - Disable: false, + Disable: true, } func parseRequestLimiter(result *SharedConfig, list *ast.ObjectList) error { @@ -45,14 +45,13 @@ func parseRequestLimiter(result *SharedConfig, list *ast.ObjectList) error { return multierror.Prefix(err, "request_limiter:") } + result.RequestLimiter.Disable = true if result.RequestLimiter.DisableRaw != nil { var err error if result.RequestLimiter.Disable, err = parseutil.ParseBool(result.RequestLimiter.DisableRaw); err != nil { return err } result.RequestLimiter.DisableRaw = nil - } else { - result.RequestLimiter.Disable = false } return nil diff --git a/vault/core.go b/vault/core.go index 20c5430473..6ddcb574fc 100644 --- a/vault/core.go +++ b/vault/core.go @@ -1317,9 +1317,8 @@ func NewCore(conf *CoreConfig) (*Core, error) { c.limiterRegistry = conf.LimiterRegistry c.limiterRegistryLock.Lock() - if conf.DisableRequestLimiter { - c.limiterRegistry.Disable() - } else { + c.limiterRegistry.Disable() + if !conf.DisableRequestLimiter { c.limiterRegistry.Enable() } c.limiterRegistryLock.Unlock() @@ -4112,7 +4111,7 @@ func (c *Core) ReloadRequestLimiter() { return } - disable := false + disable := true requestLimiterConfig := conf.(*server.Config).RequestLimiter if requestLimiterConfig != nil { disable = requestLimiterConfig.Disable