From 9042812f82d47dbe6aa1a65d4e6d796281aa05e0 Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Fri, 1 Sep 2023 09:07:21 -0400 Subject: [PATCH] Backport of Add config value that gives users options to skip calculating role for each lease into release/1.13.x (#22729) * Add config value that gives users options to skip calculating role for each lease (#22651) * Add config value that gives users options to skip calculating role for each lease * add changelog * change name * add config for testing * Update changelog/22651.txt Co-authored-by: Violet Hynes * update tests, docs and reorder logic in conditional * fix comment * update comment * fix comment again * Update comments and change if order * change comment again * add other comment * fix tests * add documentation * edit docs * Update http/util.go Co-authored-by: Mike Palmiotto * Update vault/core.go * Update vault/core.go * update var name * udpate docs * Update vault/request_handling.go Co-authored-by: Mike Palmiotto * 1 more docs change --------- Co-authored-by: Violet Hynes Co-authored-by: Mike Palmiotto * remove wrong part of cherry-pick --------- Co-authored-by: Ellie Co-authored-by: Violet Hynes Co-authored-by: Mike Palmiotto --- changelog/22651.txt | 3 +++ command/server.go | 1 + command/server/config.go | 9 +++++++++ command/server/config_test_helpers.go | 1 + http/sys_config_state_test.go | 1 + http/util.go | 2 ++ vault/core.go | 7 +++++++ vault/request_handling.go | 14 ++++++++++---- vault/testing.go | 2 ++ website/content/docs/configuration/index.mdx | 4 ++++ 10 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 changelog/22651.txt diff --git a/changelog/22651.txt b/changelog/22651.txt new file mode 100644 index 0000000000..5ca2819837 --- /dev/null +++ b/changelog/22651.txt @@ -0,0 +1,3 @@ +```release-note:improvement +core/quotas: Add configuration to allow skipping of expensive role calculations +``` \ No newline at end of file diff --git a/command/server.go b/command/server.go index 7e5cafdf93..049f9d5795 100644 --- a/command/server.go +++ b/command/server.go @@ -2691,6 +2691,7 @@ func createCoreConfig(c *ServerCommand, config *server.Config, backend physical. LogicalBackends: c.LogicalBackends, Logger: c.logger, DetectDeadlocks: config.DetectDeadlocks, + ImpreciseLeaseRoleTracking: config.ImpreciseLeaseRoleTracking, DisableSentinelTrace: config.DisableSentinelTrace, DisableCache: config.DisableCache, DisableMlock: config.DisableMlock, diff --git a/command/server/config.go b/command/server/config.go index 055edb11a1..5f3bd04d27 100644 --- a/command/server/config.go +++ b/command/server/config.go @@ -107,6 +107,8 @@ type Config struct { DetectDeadlocks string `hcl:"detect_deadlocks"` + ImpreciseLeaseRoleTracking bool `hcl:"imprecise_lease_role_tracking"` + EnableResponseHeaderRaftNodeID bool `hcl:"-"` EnableResponseHeaderRaftNodeIDRaw interface{} `hcl:"enable_response_header_raft_node_id"` @@ -407,6 +409,11 @@ func (c *Config) Merge(c2 *Config) *Config { result.DetectDeadlocks = c2.DetectDeadlocks } + result.ImpreciseLeaseRoleTracking = c.ImpreciseLeaseRoleTracking + if c2.ImpreciseLeaseRoleTracking { + result.ImpreciseLeaseRoleTracking = c2.ImpreciseLeaseRoleTracking + } + result.EnableResponseHeaderRaftNodeID = c.EnableResponseHeaderRaftNodeID if c2.EnableResponseHeaderRaftNodeID { result.EnableResponseHeaderRaftNodeID = c2.EnableResponseHeaderRaftNodeID @@ -1131,6 +1138,8 @@ func (c *Config) Sanitized() map[string]interface{} { "experiments": c.Experiments, "detect_deadlocks": c.DetectDeadlocks, + + "imprecise_lease_role_tracking": c.ImpreciseLeaseRoleTracking, } for k, v := range sharedResult { result[k] = v diff --git a/command/server/config_test_helpers.go b/command/server/config_test_helpers.go index 6bc5057846..77b0c5ef92 100644 --- a/command/server/config_test_helpers.go +++ b/command/server/config_test_helpers.go @@ -839,6 +839,7 @@ func testConfig_Sanitized(t *testing.T) { "add_lease_metrics_namespace_labels": false, }, "administrative_namespace_path": "admin/", + "imprecise_lease_role_tracking": false, } addExpectedEntSanitizedConfig(expected, []string{"http"}) diff --git a/http/sys_config_state_test.go b/http/sys_config_state_test.go index 5a8fa6bfb8..405f0597d8 100644 --- a/http/sys_config_state_test.go +++ b/http/sys_config_state_test.go @@ -172,6 +172,7 @@ func TestSysConfigState_Sanitized(t *testing.T) { }, "storage": tc.expectedStorageOutput, "administrative_namespace_path": "", + "imprecise_lease_role_tracking": false, } if tc.expectedHAStorageOutput != nil { diff --git a/http/util.go b/http/util.go index fc16e366a3..b8430479c8 100644 --- a/http/util.go +++ b/http/util.go @@ -68,6 +68,8 @@ func rateLimitQuotaWrapping(handler http.Handler, core *vault.Core) http.Handler NamespacePath: ns.Path, ClientAddress: parseRemoteIPAddress(r), } + + // This checks if any role based quota is required (LCQ or RLQ). requiresResolveRole, err := core.ResolveRoleForQuotas(r.Context(), quotaReq) if err != nil { core.Logger().Error("failed to lookup quotas", "path", path, "error", err) diff --git a/vault/core.go b/vault/core.go index 69f5368fa7..092d78eb73 100644 --- a/vault/core.go +++ b/vault/core.go @@ -686,6 +686,9 @@ type Core struct { // contains absolute paths that we intend to forward (and template) when // we're on a secondary cluster. writeForwardedPaths *pathmanager.PathManager + + // If any role based quota (LCQ or RLQ) is enabled, don't track lease counts by role + impreciseLeaseRoleTracking bool } // c.stateLock needs to be held in read mode before calling this function. @@ -747,6 +750,9 @@ type CoreConfig struct { // Use the deadlocks library to detect deadlocks DetectDeadlocks string + // If any role based quota (LCQ or RLQ) is enabled, don't track lease counts by role + ImpreciseLeaseRoleTracking bool + // Disables the trace display for Sentinel checks DisableSentinelTrace bool @@ -1011,6 +1017,7 @@ func CreateCore(conf *CoreConfig) (*Core, error) { experiments: conf.Experiments, pendingRemovalMountsAllowed: conf.PendingRemovalMountsAllowed, expirationRevokeRetryBase: conf.ExpirationRevokeRetryBase, + impreciseLeaseRoleTracking: conf.ImpreciseLeaseRoleTracking, } c.standbyStopCh.Store(make(chan struct{})) diff --git a/vault/request_handling.go b/vault/request_handling.go index 40a60a8b70..7db9195f47 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -1670,10 +1670,16 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // Attach the display name, might be used by audit backends req.DisplayName = auth.DisplayName - // If this is not a role-based quota, we still need to associate the - // login role with this lease for later lease-count quotas to be - // accurate. - if reqRole == nil && resp.Auth.TokenType != logical.TokenTypeBatch { + requiresLease := resp.Auth.TokenType != logical.TokenTypeBatch + + // If role was not already determined by http.rateLimitQuotaWrapping + // and a lease will be generated, calculate a role for the leaseEntry. + // We can skip this step if there are no pre-existing role-based quotas + // for this mount and Vault is configured to skip lease role-based lease counting + // until after they're created. This effectively zeroes out the lease count + // for new role-based quotas upon creation, rather than counting old leases toward + // the total. + if reqRole == nil && requiresLease && !c.impreciseLeaseRoleTracking { role = c.DetermineRoleFromLoginRequest(ctx, req.MountPoint, req.Data) } diff --git a/vault/testing.go b/vault/testing.go index 7f37883229..f03d64843d 100644 --- a/vault/testing.go +++ b/vault/testing.go @@ -217,6 +217,7 @@ func TestCoreWithSealAndUINoCleanup(t testing.T, opts *CoreConfig) *Core { conf.Experiments = []string{experiments.VaultExperimentEventsAlpha1} conf.CensusAgent = opts.CensusAgent conf.AdministrativeNamespacePath = opts.AdministrativeNamespacePath + conf.ImpreciseLeaseRoleTracking = opts.ImpreciseLeaseRoleTracking if opts.Logger != nil { conf.Logger = opts.Logger @@ -1523,6 +1524,7 @@ func NewTestCluster(t testing.T, base *CoreConfig, opts *TestClusterOptions) *Te coreConfig.DisableAutopilot = base.DisableAutopilot coreConfig.AdministrativeNamespacePath = base.AdministrativeNamespacePath coreConfig.ServiceRegistration = base.ServiceRegistration + coreConfig.ImpreciseLeaseRoleTracking = base.ImpreciseLeaseRoleTracking if base.BuiltinRegistry != nil { coreConfig.BuiltinRegistry = base.BuiltinRegistry diff --git a/website/content/docs/configuration/index.mdx b/website/content/docs/configuration/index.mdx index 337a3dc018..1ae7f8d89e 100644 --- a/website/content/docs/configuration/index.mdx +++ b/website/content/docs/configuration/index.mdx @@ -221,6 +221,10 @@ a negative effect on performance due to the tracking of each lock attempt. the `VAULT_EXPERIMENTS` environment variable as a comma-separated list, or via the [`-experiment`](/vault/docs/commands/server#experiment) flag. +- `imprecise_lease_role_tracking` `(bool: "false")` - Skip lease counting by role if there are no role based quotas enabled. + When `imprecise_lease_role_tracking` is set to true and a new role-based quota is enabled, subsequent lease counts start from 0. + `imprecise_lease_role_tracking` affects role-based lease count quotas, but reduces latencies when not using role based quotas. + ### High availability parameters The following parameters are used on backends that support [high availability][high-availability].