From fd38356c2871c0a8653411d259933853e1d9ba8e Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Mon, 28 Aug 2023 16:47:54 -0400 Subject: [PATCH 1/7] Skip resolve role if no role-based quotas are enabled for the mount --- http/util.go | 27 ++++++++++++++------- vault/core.go | 16 ++++++++++--- vault/quotas/quotas.go | 27 +++++++++++++++++++++ vault/quotas/quotas_test.go | 48 +++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 11 deletions(-) diff --git a/http/util.go b/http/util.go index ef3e2e5199e8..0901616ee3ee 100644 --- a/http/util.go +++ b/http/util.go @@ -60,19 +60,30 @@ func rateLimitQuotaWrapping(handler http.Handler, core *vault.Core) http.Handler } r.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes)) - role := core.DetermineRoleFromLoginRequestFromBytes(mountPath, bodyBytes, r.Context()) - - // add an entry to the context to prevent recalculating request role unnecessarily - r = r.WithContext(context.WithValue(r.Context(), logical.CtxKeyRequestRole{}, role)) - - quotaResp, err := core.ApplyRateLimitQuota(r.Context(), "as.Request{ + quotaReq := "as.Request{ Type: quotas.TypeRateLimit, Path: path, MountPath: mountPath, - Role: role, NamespacePath: ns.Path, ClientAddress: parseRemoteIPAddress(r), - }) + } + requiresResolveRole, err := core.ResolveRoleForQuotas(r.Context(), quotaReq) + if err != nil { + core.Logger().Error("failed to lookup quotas", "path", path, "error", err) + respondError(w, http.StatusUnprocessableEntity, err) + return + } + + // If any role-based quotas are enabled for this namespace/mount, just + // do the role resolution once here. + if requiresResolveRole { + role := core.DetermineRoleFromLoginRequestFromBytes(r.Context(), mountPath, bodyBytes) + // add an entry to the context to prevent recalculating request role unnecessarily + r = r.WithContext(context.WithValue(r.Context(), logical.CtxKeyRequestRole{}, role)) + quotaReq.Role = role + } + + quotaResp, err := core.ApplyRateLimitQuota(r.Context(), quotaReq) if err != nil { core.Logger().Error("failed to apply quota", "path", path, "error", err) respondError(w, http.StatusUnprocessableEntity, err) diff --git a/vault/core.go b/vault/core.go index 862296a61c19..86b7a312bfe4 100644 --- a/vault/core.go +++ b/vault/core.go @@ -3939,7 +3939,7 @@ func (c *Core) LoadNodeID() (string, error) { // DetermineRoleFromLoginRequestFromBytes will determine the role that should be applied to a quota for a given // login request, accepting a byte payload -func (c *Core) DetermineRoleFromLoginRequestFromBytes(mountPoint string, payload []byte, ctx context.Context) string { +func (c *Core) DetermineRoleFromLoginRequestFromBytes(ctx context.Context, mountPoint string, payload []byte) string { data := make(map[string]interface{}) err := jsonutil.DecodeJSON(payload, &data) if err != nil { @@ -3947,12 +3947,12 @@ func (c *Core) DetermineRoleFromLoginRequestFromBytes(mountPoint string, payload return "" } - return c.DetermineRoleFromLoginRequest(mountPoint, data, ctx) + return c.DetermineRoleFromLoginRequest(ctx, mountPoint, data) } // DetermineRoleFromLoginRequest will determine the role that should be applied to a quota for a given // login request -func (c *Core) DetermineRoleFromLoginRequest(mountPoint string, data map[string]interface{}, ctx context.Context) string { +func (c *Core) DetermineRoleFromLoginRequest(ctx context.Context, mountPoint string, data map[string]interface{}) string { c.authLock.RLock() defer c.authLock.RUnlock() matchingBackend := c.router.MatchingBackend(ctx, mountPoint) @@ -3971,9 +3971,19 @@ func (c *Core) DetermineRoleFromLoginRequest(mountPoint string, data map[string] if err != nil || resp.Data["role"] == nil { return "" } + return resp.Data["role"].(string) } +// ResolveRoleForQuotas looks for any quotas requiring a role for early +// computation in the RateLimitQuotaWrapping handler. +func (c *Core) ResolveRoleForQuotas(ctx context.Context, req *quotas.Request) (bool, error) { + if c.quotaManager == nil { + return false, nil + } + return c.quotaManager.QueryResolveRoleQuotas(req) +} + // aliasNameFromLoginRequest will determine the aliasName from the login Request func (c *Core) aliasNameFromLoginRequest(ctx context.Context, req *logical.Request) (string, error) { c.authLock.RLock() diff --git a/vault/quotas/quotas.go b/vault/quotas/quotas.go index 369c0ad5c0ab..7a3866547613 100644 --- a/vault/quotas/quotas.go +++ b/vault/quotas/quotas.go @@ -617,6 +617,33 @@ func (m *Manager) queryQuota(txn *memdb.Txn, req *Request) (Quota, error) { return nil, nil } +// QueryResolveRoleQuotas checks if there's a quota for the request mount path +// which requires ResolveRoleOperation. +func (m *Manager) QueryResolveRoleQuotas(req *Request) (bool, error) { + m.dbAndCacheLock.RLock() + defer m.dbAndCacheLock.RUnlock() + + txn := m.db.Txn(false) + + // ns would have been made non-empty during insertion. Use non-empty + // value during query as well. + if req.NamespacePath == "" { + req.NamespacePath = "root" + } + + // Check for any role-based quotas on the request namespaces/mount path. + for _, qType := range quotaTypes() { + quota, err := txn.First(qType, indexNamespaceMount, req.NamespacePath, req.MountPath, false, true) + if err != nil { + return false, err + } + if quota != nil { + return true, nil + } + } + return false, nil +} + // DeleteQuota removes a quota rule from the db for a given name func (m *Manager) DeleteQuota(ctx context.Context, qType string, name string) error { m.quotaLock.Lock() diff --git a/vault/quotas/quotas_test.go b/vault/quotas/quotas_test.go index 296c273ea46e..8776b2450e53 100644 --- a/vault/quotas/quotas_test.go +++ b/vault/quotas/quotas_test.go @@ -135,3 +135,51 @@ func TestQuotas_Precedence(t *testing.T) { checkQuotaFunc(t, "testns/nested2/nested3/", "", "", "", rateLimitMultiNestedNsInheritableQuota) checkQuotaFunc(t, "testns/nested2/nested3/nested4/nested5", "", "", "", rateLimitMultiNestedNsInheritableQuota) } + +// TestQuotas_QueryRoleQuotas checks to see if quota creation on a mount +// requires a call to ResolveRoleOperation. +func TestQuotas_QueryResolveRole_RateLimitQuotas(t *testing.T) { + leaseWalkFunc := func(context.Context, func(request *Request) bool) error { + return nil + } + qm, err := NewManager(logging.NewVaultLogger(log.Trace), leaseWalkFunc, metricsutil.BlackholeSink()) + require.NoError(t, err) + + // RLQ + { + rlqReq := &Request{ + Type: TypeRateLimit, + Path: "", + MountPath: "mount1/", + NamespacePath: "", + ClientAddress: "127.0.0.1", + } + // Check that we have no quotas requiring role resolution on mount1/ + required, err := qm.QueryResolveRoleQuotas(rlqReq) + require.NoError(t, err) + require.False(t, required) + + // Create a non-role-based RLQ on mount1/ and make sure it doesn't require role resolution + rlq := NewRateLimitQuota("tq", rlqReq.NamespacePath, rlqReq.MountPath, rlqReq.Path, rlqReq.Role, false, 1*time.Minute, 10*time.Second, 10) + require.NoError(t, qm.SetQuota(context.Background(), TypeRateLimit.String(), rlq, false)) + + required, err = qm.QueryResolveRoleQuotas(rlqReq) + require.NoError(t, err) + require.False(t, required) + + // Create a role-based RLQ on mount1/ and make sure it requires role resolution + rlqReq.Role = "test" + rlq = NewRateLimitQuota("tq", rlqReq.NamespacePath, rlqReq.MountPath, rlqReq.Path, rlqReq.Role, false, 1*time.Minute, 10*time.Second, 10) + require.NoError(t, qm.SetQuota(context.Background(), TypeRateLimit.String(), rlq, false)) + + required, err = qm.QueryResolveRoleQuotas(rlqReq) + require.NoError(t, err) + require.True(t, required) + + // Check that we have no quotas requiring role resolution on mount2/ + rlqReq.MountPath = "mount2/" + required, err = qm.QueryResolveRoleQuotas(rlqReq) + require.NoError(t, err) + require.False(t, required) + } +} From 4b1c5cd65d79022229294d2474abc2c6c0b43552 Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Tue, 29 Aug 2023 15:51:22 -0400 Subject: [PATCH 2/7] Do role lookup for lease if its not set --- vault/request_handling.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/vault/request_handling.go b/vault/request_handling.go index fac4734d4f7f..192cdbccdff7 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -1485,7 +1485,8 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // Check for request role in context to role based quotas var role string - if reqRole := ctx.Value(logical.CtxKeyRequestRole{}); reqRole != nil { + reqRole := ctx.Value(logical.CtxKeyRequestRole{}) + if reqRole != nil { role = reqRole.(string) } @@ -1686,6 +1687,13 @@ 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 { + role = c.DetermineRoleFromLoginRequest(ctx, req.MountPoint, req.Data) + } + leaseGen, respTokenCreate, errCreateToken := c.LoginCreateToken(ctx, ns, req.Path, source, role, resp) leaseGenerated = leaseGen if errCreateToken != nil { From 105954ea98c6811cf4d5f808a757b15acaa4863b Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Tue, 29 Aug 2023 21:44:28 -0400 Subject: [PATCH 3/7] changelog --- changelog/22597.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/22597.txt diff --git a/changelog/22597.txt b/changelog/22597.txt new file mode 100644 index 000000000000..0c37e561be28 --- /dev/null +++ b/changelog/22597.txt @@ -0,0 +1,3 @@ +```release-note:bug +core/quotas: Only perform ResolveRoleOperation for role-based quotas and lease creation. +``` From d890ed41b7b4754a573525c7e2b7db2876b038f0 Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Wed, 30 Aug 2023 09:07:45 -0400 Subject: [PATCH 4/7] Review feedback --- vault/request_handling.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/request_handling.go b/vault/request_handling.go index 192cdbccdff7..049ec79a8b5e 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -1690,7 +1690,7 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // 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 { + if reqRole == nil && resp.Auth.TokenType != logical.TokenTypeBatch { role = c.DetermineRoleFromLoginRequest(ctx, req.MountPoint, req.Data) } From e59b252bc091ecb74f1a346aca156f144feb84df Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Wed, 30 Aug 2023 09:25:10 -0400 Subject: [PATCH 5/7] Use StatusInternalServerError instead of StatusUnprocessableEntity --- http/util.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http/util.go b/http/util.go index 0901616ee3ee..fadac4f4ad31 100644 --- a/http/util.go +++ b/http/util.go @@ -70,7 +70,7 @@ func rateLimitQuotaWrapping(handler http.Handler, core *vault.Core) http.Handler requiresResolveRole, err := core.ResolveRoleForQuotas(r.Context(), quotaReq) if err != nil { core.Logger().Error("failed to lookup quotas", "path", path, "error", err) - respondError(w, http.StatusUnprocessableEntity, err) + respondError(w, http.StatusInternalServerError, err) return } @@ -86,7 +86,7 @@ func rateLimitQuotaWrapping(handler http.Handler, core *vault.Core) http.Handler quotaResp, err := core.ApplyRateLimitQuota(r.Context(), quotaReq) if err != nil { core.Logger().Error("failed to apply quota", "path", path, "error", err) - respondError(w, http.StatusUnprocessableEntity, err) + respondError(w, http.StatusInternalServerError, err) return } From e4a58f6d9ede265a45a290feb334e24976498f4c Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Wed, 30 Aug 2023 09:31:45 -0400 Subject: [PATCH 6/7] Drop superfluous curly braces --- vault/quotas/quotas_test.go | 61 ++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/vault/quotas/quotas_test.go b/vault/quotas/quotas_test.go index 8776b2450e53..45afed533c5e 100644 --- a/vault/quotas/quotas_test.go +++ b/vault/quotas/quotas_test.go @@ -145,41 +145,38 @@ func TestQuotas_QueryResolveRole_RateLimitQuotas(t *testing.T) { qm, err := NewManager(logging.NewVaultLogger(log.Trace), leaseWalkFunc, metricsutil.BlackholeSink()) require.NoError(t, err) - // RLQ - { - rlqReq := &Request{ - Type: TypeRateLimit, - Path: "", - MountPath: "mount1/", - NamespacePath: "", - ClientAddress: "127.0.0.1", - } - // Check that we have no quotas requiring role resolution on mount1/ - required, err := qm.QueryResolveRoleQuotas(rlqReq) - require.NoError(t, err) - require.False(t, required) + rlqReq := &Request{ + Type: TypeRateLimit, + Path: "", + MountPath: "mount1/", + NamespacePath: "", + ClientAddress: "127.0.0.1", + } + // Check that we have no quotas requiring role resolution on mount1/ + required, err := qm.QueryResolveRoleQuotas(rlqReq) + require.NoError(t, err) + require.False(t, required) - // Create a non-role-based RLQ on mount1/ and make sure it doesn't require role resolution - rlq := NewRateLimitQuota("tq", rlqReq.NamespacePath, rlqReq.MountPath, rlqReq.Path, rlqReq.Role, false, 1*time.Minute, 10*time.Second, 10) - require.NoError(t, qm.SetQuota(context.Background(), TypeRateLimit.String(), rlq, false)) + // Create a non-role-based RLQ on mount1/ and make sure it doesn't require role resolution + rlq := NewRateLimitQuota("tq", rlqReq.NamespacePath, rlqReq.MountPath, rlqReq.Path, rlqReq.Role, false, 1*time.Minute, 10*time.Second, 10) + require.NoError(t, qm.SetQuota(context.Background(), TypeRateLimit.String(), rlq, false)) - required, err = qm.QueryResolveRoleQuotas(rlqReq) - require.NoError(t, err) - require.False(t, required) + required, err = qm.QueryResolveRoleQuotas(rlqReq) + require.NoError(t, err) + require.False(t, required) - // Create a role-based RLQ on mount1/ and make sure it requires role resolution - rlqReq.Role = "test" - rlq = NewRateLimitQuota("tq", rlqReq.NamespacePath, rlqReq.MountPath, rlqReq.Path, rlqReq.Role, false, 1*time.Minute, 10*time.Second, 10) - require.NoError(t, qm.SetQuota(context.Background(), TypeRateLimit.String(), rlq, false)) + // Create a role-based RLQ on mount1/ and make sure it requires role resolution + rlqReq.Role = "test" + rlq = NewRateLimitQuota("tq", rlqReq.NamespacePath, rlqReq.MountPath, rlqReq.Path, rlqReq.Role, false, 1*time.Minute, 10*time.Second, 10) + require.NoError(t, qm.SetQuota(context.Background(), TypeRateLimit.String(), rlq, false)) - required, err = qm.QueryResolveRoleQuotas(rlqReq) - require.NoError(t, err) - require.True(t, required) + required, err = qm.QueryResolveRoleQuotas(rlqReq) + require.NoError(t, err) + require.True(t, required) - // Check that we have no quotas requiring role resolution on mount2/ - rlqReq.MountPath = "mount2/" - required, err = qm.QueryResolveRoleQuotas(rlqReq) - require.NoError(t, err) - require.False(t, required) - } + // Check that we have no quotas requiring role resolution on mount2/ + rlqReq.MountPath = "mount2/" + required, err = qm.QueryResolveRoleQuotas(rlqReq) + require.NoError(t, err) + require.False(t, required) } From 19d470725cb422f874814404c931c9976351021b Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Wed, 30 Aug 2023 09:44:53 -0400 Subject: [PATCH 7/7] memdb comment for clarity --- vault/quotas/quotas.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vault/quotas/quotas.go b/vault/quotas/quotas.go index 7a3866547613..18e69346ad30 100644 --- a/vault/quotas/quotas.go +++ b/vault/quotas/quotas.go @@ -633,6 +633,10 @@ func (m *Manager) QueryResolveRoleQuotas(req *Request) (bool, error) { // Check for any role-based quotas on the request namespaces/mount path. for _, qType := range quotaTypes() { + // Use the namespace and mount as indexes and find all matches with a + // set Role field (see: 'true' as the last argument). We can't use + // indexNamespaceMountRole for this, because Role is a StringFieldIndex, + // which won't match on an empty string. quota, err := txn.First(qType, indexNamespaceMount, req.NamespacePath, req.MountPath, false, true) if err != nil { return false, err