From 9e86429713158a1602cd1fd0206b3ca40afc0b8d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 09:41:14 -0500 Subject: [PATCH 01/10] test: Add benchmark for static rbac roles --- coderd/rbac/authz_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 1dd9b3b085196..df3dc5b0c942e 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -96,6 +96,18 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Groups: noiseGroups, }, }, + { + Name: "StaticRoles", + Actor: rbac.Subject{ + // Give some extra roles that an admin might have + Roles: rbac.RoleNames{ + "auditor", rbac.RoleMember(), rbac.RoleMember(), + rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()}, + ID: user.String(), + Scope: rbac.ScopeAll, + Groups: noiseGroups, + }, + }, } return benchCases, users, orgs } From a00b27be9f7f8d4b2d8e216098dd9728ca0e5d5b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 09:42:21 -0500 Subject: [PATCH 02/10] static roles should only be allocated once --- coderd/rbac/astvalue.go | 8 +++ coderd/rbac/roles.go | 138 ++++++++++++++++++++++------------------ 2 files changed, 85 insertions(+), 61 deletions(-) diff --git a/coderd/rbac/astvalue.go b/coderd/rbac/astvalue.go index 2feb20d636b0b..4e12489e90fcd 100644 --- a/coderd/rbac/astvalue.go +++ b/coderd/rbac/astvalue.go @@ -133,7 +133,15 @@ func (z Object) regoValue() ast.Value { ) } +func (role Role) withCachedRegoValue() Role { + role.cachedRegoValue = role.regoValue() + return role +} + func (role Role) regoValue() ast.Value { + if role.cachedRegoValue != nil { + return role.cachedRegoValue + } orgMap := ast.NewObject() for k, p := range role.Org { orgMap.Insert(ast.StringTerm(k), ast.NewTerm(regoSlice(p))) diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index dd65886c04ed2..64a858e81f00e 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -4,6 +4,8 @@ import ( "sort" "strings" + "github.com/open-policy-agent/opa/ast" + "github.com/google/uuid" "golang.org/x/xerrors" @@ -128,86 +130,96 @@ func ReloadBuiltinRoles(opts *RoleOptions) { ) } + ownerRole := Role{ + Name: owner, + DisplayName: "Owner", + Site: allPermsExcept(ownerAndAdminExceptions...), + Org: map[string][]Permission{}, + User: []Permission{}, + }.withCachedRegoValue() + + memberRole := Role{ + Name: member, + DisplayName: "", + Site: Permissions(map[string][]Action{ + // All users can read all other users and know they exist. + ResourceUser.Type: {ActionRead}, + ResourceRoleAssignment.Type: {ActionRead}, + // All users can see the provisioner daemons. + ResourceProvisionerDaemon.Type: {ActionRead}, + }), + Org: map[string][]Permission{}, + User: allPermsExcept(), + }.withCachedRegoValue() + + auditorRole := Role{ + Name: auditor, + DisplayName: "Auditor", + Site: Permissions(map[string][]Action{ + // Should be able to read all template details, even in orgs they + // are not in. + ResourceTemplate.Type: {ActionRead}, + ResourceAuditLog.Type: {ActionRead}, + }), + Org: map[string][]Permission{}, + User: []Permission{}, + }.withCachedRegoValue() + + templateAdminRole := Role{ + Name: templateAdmin, + DisplayName: "Template Admin", + Site: Permissions(map[string][]Action{ + ResourceTemplate.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, + // CRUD all files, even those they did not upload. + ResourceFile.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, + ResourceWorkspace.Type: {ActionRead}, + // CRUD to provisioner daemons for now. + ResourceProvisionerDaemon.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, + // Needs to read all organizations since + ResourceOrganization.Type: {ActionRead}, + }), + Org: map[string][]Permission{}, + User: []Permission{}, + }.withCachedRegoValue() + + userAdminRole := Role{ + Name: userAdmin, + DisplayName: "User Admin", + Site: Permissions(map[string][]Action{ + ResourceRoleAssignment.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, + ResourceUser.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, + // Full perms to manage org members + ResourceOrganizationMember.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, + ResourceGroup.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, + }), + Org: map[string][]Permission{}, + User: []Permission{}, + }.withCachedRegoValue() + builtInRoles = map[string]func(orgID string) Role{ // admin grants all actions to all resources. owner: func(_ string) Role { - return Role{ - Name: owner, - DisplayName: "Owner", - Site: allPermsExcept(ownerAndAdminExceptions...), - Org: map[string][]Permission{}, - User: []Permission{}, - } + return ownerRole }, // member grants all actions to all resources owned by the user member: func(_ string) Role { - return Role{ - Name: member, - DisplayName: "", - Site: Permissions(map[string][]Action{ - // All users can read all other users and know they exist. - ResourceUser.Type: {ActionRead}, - ResourceRoleAssignment.Type: {ActionRead}, - // All users can see the provisioner daemons. - ResourceProvisionerDaemon.Type: {ActionRead}, - }), - Org: map[string][]Permission{}, - User: allPermsExcept(), - } + return memberRole }, // auditor provides all permissions required to effectively read and understand // audit log events. // TODO: Finish the auditor as we add resources. auditor: func(_ string) Role { - return Role{ - Name: auditor, - DisplayName: "Auditor", - Site: Permissions(map[string][]Action{ - // Should be able to read all template details, even in orgs they - // are not in. - ResourceTemplate.Type: {ActionRead}, - ResourceAuditLog.Type: {ActionRead}, - }), - Org: map[string][]Permission{}, - User: []Permission{}, - } + return auditorRole }, templateAdmin: func(_ string) Role { - return Role{ - Name: templateAdmin, - DisplayName: "Template Admin", - Site: Permissions(map[string][]Action{ - ResourceTemplate.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, - // CRUD all files, even those they did not upload. - ResourceFile.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, - ResourceWorkspace.Type: {ActionRead}, - // CRUD to provisioner daemons for now. - ResourceProvisionerDaemon.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, - // Needs to read all organizations since - ResourceOrganization.Type: {ActionRead}, - }), - Org: map[string][]Permission{}, - User: []Permission{}, - } + return templateAdminRole }, userAdmin: func(_ string) Role { - return Role{ - Name: userAdmin, - DisplayName: "User Admin", - Site: Permissions(map[string][]Action{ - ResourceRoleAssignment.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, - ResourceUser.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, - // Full perms to manage org members - ResourceOrganizationMember.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, - ResourceGroup.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete}, - }), - Org: map[string][]Permission{}, - User: []Permission{}, - } + return userAdminRole }, // orgAdmin returns a role with all actions allows in a given @@ -333,6 +345,10 @@ type Role struct { // roles. Org map[string][]Permission `json:"org"` User []Permission `json:"user"` + + // cachedRegoValue can be used to cache the rego value for this role. + // This is helpful for static roles that never change. + cachedRegoValue ast.Value } type Roles []Role From 73275954086c4a417e49a870bfa3064c5dadad18 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 09:48:22 -0500 Subject: [PATCH 03/10] Import order --- coderd/rbac/authz_test.go | 2 +- coderd/rbac/roles.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index df3dc5b0c942e..64283a6bce44f 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -114,7 +114,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U // BenchmarkRBACAuthorize benchmarks the rbac.Authorize method. // -// go test -bench BenchmarkRBACAuthorize -benchmem -memprofile memprofile.out -cpuprofile profile.out +// go test -run=^$ -bench BenchmarkRBACAuthorize -benchmem -memprofile memprofile.out -cpuprofile profile.out func BenchmarkRBACAuthorize(b *testing.B) { benchCases, user, orgs := benchmarkUserCases() users := append([]uuid.UUID{}, diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 64a858e81f00e..0423eecb9acc5 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -4,9 +4,8 @@ import ( "sort" "strings" - "github.com/open-policy-agent/opa/ast" - "github.com/google/uuid" + "github.com/open-policy-agent/opa/ast" "golang.org/x/xerrors" ) From 9225793471fa2f5080024591f6017991b1a5048d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 10:08:32 -0500 Subject: [PATCH 04/10] Add comments --- coderd/rbac/astvalue.go | 9 +++++++-- coderd/rbac/roles.go | 4 ++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/coderd/rbac/astvalue.go b/coderd/rbac/astvalue.go index 4e12489e90fcd..fc9bceb0419f7 100644 --- a/coderd/rbac/astvalue.go +++ b/coderd/rbac/astvalue.go @@ -133,9 +133,14 @@ func (z Object) regoValue() ast.Value { ) } +// withCachedRegoValue returns a copy of the role with the cachedRegoValue. +// It does not mutate the underlying role. +// Avoid using this function if possible, it should only be used if the +// caller can guarantee the role is static and will never change. func (role Role) withCachedRegoValue() Role { - role.cachedRegoValue = role.regoValue() - return role + tmp := role + tmp.cachedRegoValue = role.regoValue() + return tmp } func (role Role) regoValue() ast.Value { diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 0423eecb9acc5..10e32d751ac26 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -129,6 +129,10 @@ func ReloadBuiltinRoles(opts *RoleOptions) { ) } + // Static roles that never change should be allocated in a closure. + // This is to ensure these data structures are only allocated once and not + // on every authorize call. 'withCachedRegoValue' can be used as well to + // preallocate the rego value that is used by the rego eval engine. ownerRole := Role{ Name: owner, DisplayName: "Owner", From 395be1c1b2e74e7ce5db0abe7e0b50db97e5b446 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 10:13:47 -0500 Subject: [PATCH 05/10] Add comment on newbenchmark --- coderd/rbac/authz_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 64283a6bce44f..5614b9b16a298 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -97,12 +97,14 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U }, }, { + // This test should only use static roles. AKA no org roles. Name: "StaticRoles", Actor: rbac.Subject{ // Give some extra roles that an admin might have Roles: rbac.RoleNames{ "auditor", rbac.RoleMember(), rbac.RoleMember(), - rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()}, + rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin(), + }, ID: user.String(), Scope: rbac.ScopeAll, Groups: noiseGroups, From e8df3511bc958ba41ec42e12f51bc43501c92e8b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 10:27:55 -0500 Subject: [PATCH 06/10] Test was missing owner role --- coderd/rbac/authz_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 5614b9b16a298..4179920b0c041 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -102,7 +102,7 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Actor: rbac.Subject{ // Give some extra roles that an admin might have Roles: rbac.RoleNames{ - "auditor", rbac.RoleMember(), rbac.RoleMember(), + "auditor", rbac.RoleOwner(), rbac.RoleMember(), rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin(), }, ID: user.String(), From bf32869bef8ad66f6a4d1ec73e9bc389750deec7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 10:53:32 -0500 Subject: [PATCH 07/10] Add comment on benchmark about caching --- coderd/rbac/authz_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 4179920b0c041..d98a85d232bbb 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -125,6 +125,9 @@ func BenchmarkRBACAuthorize(b *testing.B) { uuid.MustParse("0632b012-49e0-4d70-a5b3-f4398f1dcd52"), uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"), ) + + // There is no caching that occurs because a fresh context is used for each + // call. And the context needs 'WithCacheCtx' to work. authorizer := rbac.NewCachingAuthorizer(prometheus.NewRegistry()) // This benchmarks all the simple cases using just user permissions. Groups // are added as noise, but do not do anything. From 4c955ce19c7a83eaf448c55294b3b4201cd3e62d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 11:05:23 -0500 Subject: [PATCH 08/10] A unit test that modifies the ast value should not mess with the globals --- coderd/rbac/roles_internal_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/coderd/rbac/roles_internal_test.go b/coderd/rbac/roles_internal_test.go index 5cf6446a30547..2055cfaafe42c 100644 --- a/coderd/rbac/roles_internal_test.go +++ b/coderd/rbac/roles_internal_test.go @@ -68,8 +68,19 @@ func BenchmarkRBACValueAllocation(b *testing.B) { func TestRegoInputValue(t *testing.T) { t.Parallel() + // Expand all roles and make sure we have a good copy. + // This is because these tests modify the roles, and we don't want to + // modify the original roles. + roles, err := RoleNames{RoleOrgMember(uuid.New()), RoleOrgAdmin(uuid.New()), RoleMember()}.Expand() + require.NoError(t, err, "failed to expand roles") + for i := range roles { + // If all cached values are nil, then the role will not use + // the shared cached value. + roles[i].cachedRegoValue = nil + } + actor := Subject{ - Roles: RoleNames{RoleOrgMember(uuid.New()), RoleOrgAdmin(uuid.New()), RoleMember()}, + Roles: Roles(roles), ID: uuid.NewString(), Scope: ScopeAll, Groups: []string{uuid.NewString(), uuid.NewString(), uuid.NewString()}, From 2bc9c7e680e4d286ed7636a89eac2a34bff8b70b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 11:23:21 -0500 Subject: [PATCH 09/10] Cache subject AST values to avoid reallocating slices --- coderd/database/dbauthz/dbauthz.go | 8 +++++--- coderd/httpmw/apikey.go | 2 +- coderd/httpmw/workspaceagent.go | 2 +- coderd/rbac/astvalue.go | 4 ++++ coderd/rbac/authz.go | 14 ++++++++++++++ coderd/rbac/authz_test.go | 15 +++++++++++++++ 6 files changed, 40 insertions(+), 5 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 20785c72bed3e..155dbd79b2dcb 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -144,7 +144,8 @@ var ( }, }), Scope: rbac.ScopeAll, - } + }.WithCachedASTValue() + subjectAutostart = rbac.Subject{ ID: uuid.Nil.String(), Roles: rbac.Roles([]rbac.Role{ @@ -161,7 +162,8 @@ var ( }, }), Scope: rbac.ScopeAll, - } + }.WithCachedASTValue() + subjectSystemRestricted = rbac.Subject{ ID: uuid.Nil.String(), Roles: rbac.Roles([]rbac.Role{ @@ -188,7 +190,7 @@ var ( }, }), Scope: rbac.ScopeAll, - } + }.WithCachedASTValue() ) // AsProvisionerd returns a context with an actor that has permissions required diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index 444c5d9a92837..509ce4a82a24a 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -379,7 +379,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon Roles: rbac.RoleNames(roles.Roles), Groups: roles.Groups, Scope: rbac.ScopeName(key.Scope), - }, + }.WithCachedASTValue(), } return &key, &authz, true diff --git a/coderd/httpmw/workspaceagent.go b/coderd/httpmw/workspaceagent.go index d24f0e412a38e..3bfe946c05fa2 100644 --- a/coderd/httpmw/workspaceagent.go +++ b/coderd/httpmw/workspaceagent.go @@ -110,5 +110,5 @@ func getAgentSubject(ctx context.Context, db database.Store, agent database.Work Roles: rbac.RoleNames(roles.Roles), Groups: roles.Groups, Scope: rbac.WorkspaceAgentScope(workspace.ID, user.ID), - }, nil + }.WithCachedASTValue(), nil } diff --git a/coderd/rbac/astvalue.go b/coderd/rbac/astvalue.go index fc9bceb0419f7..954f20cfeea53 100644 --- a/coderd/rbac/astvalue.go +++ b/coderd/rbac/astvalue.go @@ -65,6 +65,10 @@ func regoPartialInputValue(subject Subject, action Action, objectType string) (a // regoValue returns the ast.Object representation of the subject. func (s Subject) regoValue() (ast.Value, error) { + if s.cachedASTValue != nil { + return s.cachedASTValue, nil + } + subjRoles, err := s.Roles.Expand() if err != nil { return nil, xerrors.Errorf("expand roles: %w", err) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index e8beeea1bd468..d868caed21e0c 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -49,6 +49,20 @@ type Subject struct { Roles ExpandableRoles Groups []string Scope ExpandableScope + + // cachedASTValue is the cached ast value for this subject. + cachedASTValue ast.Value +} + +// WithCachedASTValue can be called if the subject is static. This will compute +// the ast value once and cache it for future calls. +func (s Subject) WithCachedASTValue() Subject { + tmp := s + v, err := tmp.regoValue() + if err == nil { + tmp.cachedASTValue = v + } + return tmp } func (s Subject) Equal(b Subject) bool { diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index d98a85d232bbb..d9c63b6147133 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -86,6 +86,21 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Groups: noiseGroups, }, }, + { + Name: "ManyRolesCachedSubject", + Actor: rbac.Subject{ + // Admin of many orgs + Roles: rbac.RoleNames{ + rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgAdmin(orgs[0]), + rbac.RoleOrgMember(orgs[1]), rbac.RoleOrgAdmin(orgs[1]), + rbac.RoleOrgMember(orgs[2]), rbac.RoleOrgAdmin(orgs[2]), + rbac.RoleMember(), + }, + ID: user.String(), + Scope: rbac.ScopeAll, + Groups: noiseGroups, + }.WithCachedASTValue(), + }, { Name: "AdminWithScope", Actor: rbac.Subject{ From c90f7b7e7a8a57d7946be8aafb455bf691463da2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 May 2023 11:32:31 -0500 Subject: [PATCH 10/10] add extra benchmark --- coderd/rbac/authz_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index d9c63b6147133..8a1f860a716e3 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -125,6 +125,20 @@ func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.U Groups: noiseGroups, }, }, + { + // This test should only use static roles. AKA no org roles. + Name: "StaticRolesWithCache", + Actor: rbac.Subject{ + // Give some extra roles that an admin might have + Roles: rbac.RoleNames{ + "auditor", rbac.RoleOwner(), rbac.RoleMember(), + rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin(), + }, + ID: user.String(), + Scope: rbac.ScopeAll, + Groups: noiseGroups, + }.WithCachedASTValue(), + }, } return benchCases, users, orgs }