diff --git a/coderd/coderd.go b/coderd/coderd.go index e957314bc8..30e306d1fd 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -177,12 +177,12 @@ func New(options *Options) *API { if options.FilesRateLimit == 0 { options.FilesRateLimit = 12 } - if options.Authorizer == nil { - options.Authorizer = rbac.NewAuthorizer() - } if options.PrometheusRegistry == nil { options.PrometheusRegistry = prometheus.NewRegistry() } + if options.Authorizer == nil { + options.Authorizer = rbac.NewAuthorizer(options.PrometheusRegistry) + } if options.TailnetCoordinator == nil { options.TailnetCoordinator = tailnet.NewCoordinator() } diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index 62bb11037f..f4242aee3c 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -565,7 +565,7 @@ func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Obje // CompileToSQL returns a compiled version of the authorizer that will work for // in memory databases. This fake version will not work against a SQL database. -func (fakePreparedAuthorizer) CompileToSQL(_ regosql.ConvertConfig) (string, error) { +func (fakePreparedAuthorizer) CompileToSQL(_ context.Context, _ regosql.ConvertConfig) (string, error) { return "", xerrors.New("not implemented") } diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index 35e22f95bd..348555285a 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -33,7 +33,7 @@ type templateQuerier interface { } func (q *sqlQuerier) GetAuthorizedTemplates(ctx context.Context, arg GetTemplatesWithFilterParams, prepared rbac.PreparedAuthorized) ([]Template, error) { - authorizedFilter, err := prepared.CompileToSQL(regosql.ConvertConfig{ + authorizedFilter, err := prepared.CompileToSQL(ctx, regosql.ConvertConfig{ VariableConverter: regosql.TemplateConverter(), }) if err != nil { @@ -183,7 +183,7 @@ type workspaceQuerier interface { // This code is copied from `GetWorkspaces` and adds the authorized filter WHERE // clause. func (q *sqlQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg GetWorkspacesParams, prepared rbac.PreparedAuthorized) ([]GetWorkspacesRow, error) { - authorizedFilter, err := prepared.CompileToSQL(rbac.ConfigWithoutACL()) + authorizedFilter, err := prepared.CompileToSQL(ctx, rbac.ConfigWithoutACL()) if err != nil { return nil, xerrors.Errorf("compile authorized filter: %w", err) } @@ -249,7 +249,7 @@ type userQuerier interface { } func (q *sqlQuerier) GetAuthorizedUserCount(ctx context.Context, arg GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) { - authorizedFilter, err := prepared.CompileToSQL(rbac.ConfigWithoutACL()) + authorizedFilter, err := prepared.CompileToSQL(ctx, rbac.ConfigWithoutACL()) if err != nil { return -1, xerrors.Errorf("compile authorized filter: %w", err) } diff --git a/coderd/rbac/README.md b/coderd/rbac/README.md index 9a6f1fb40f..66a8dcb863 100644 --- a/coderd/rbac/README.md +++ b/coderd/rbac/README.md @@ -71,3 +71,17 @@ Y indicates that the role provides positive permissions, N indicates the role pr | user | \_ | \_ | Y | Y | | | \_ | \_ | N | N | | unauthenticated | \_ | \_ | \_ | N | + +# Testing + +You can test outside of golang by using the `opa` cli. + +**Evaluation** + +opa eval --format=pretty 'false' -d policy.rego -i input.json + +**Partial Evaluation** + +```bash +opa eval --partial --format=pretty 'data.authz.allow' -d policy.rego --unknowns input.object.owner --unknowns input.object.org_owner --unknowns input.object.acl_user_list --unknowns input.object.acl_group_list -i input.json +``` diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index eaac305c71..4d43089fb7 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -4,8 +4,11 @@ import ( "context" _ "embed" "sync" + "time" "github.com/open-policy-agent/opa/rego" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promauto" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" "golang.org/x/xerrors" @@ -21,20 +24,16 @@ type Authorizer interface { type PreparedAuthorized interface { Authorize(ctx context.Context, object Object) error - CompileToSQL(cfg regosql.ConvertConfig) (string, error) + CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) } // Filter takes in a list of objects, and will filter the list removing all // the elements the subject does not have permission for. All objects must be // of the same type. +// +// Ideally the 'CompileToSQL' is used instead for large sets. This cost scales +// linearly with the number of objects passed in. func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, scope Scope, groups []string, action Action, objects []O) ([]O, error) { - ctx, span := tracing.StartSpan(ctx, trace.WithAttributes( - attribute.String("subject_id", subjID), - attribute.StringSlice("subject_roles", subjRoles), - attribute.Int("num_objects", len(objects)), - )) - defer span.End() - if len(objects) == 0 { // Nothing to filter return objects, nil @@ -42,6 +41,20 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub objectType := objects[0].RBACObject().Type filtered := make([]O, 0) + // Start the span after the object type is detected. If we are filtering 0 + // objects, then the span is not interesting. It would just add excessive + // 0 time spans that provide no insight. + ctx, span := tracing.StartSpan(ctx, + rbacTraceAttributes(subjRoles, len(groups), scope, action, objectType, + // For filtering, we are only measuring the total time for the entire + // set of objects. This and the 'PrepareByRoleName' span time + // is all that is required to measure the performance of this + // function on a per-object basis. + attribute.Int("num_objects", len(objects)), + ), + ) + defer span.End() + // Running benchmarks on this function, it is **always** faster to call // auth.ByRoleName on <10 objects. This is because the overhead of // 'PrepareByRoleName'. Once we cross 10 objects, then it starts to become @@ -82,6 +95,9 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub // RegoAuthorizer will use a prepared rego query for performing authorize() type RegoAuthorizer struct { query rego.PreparedEvalQuery + + authorizeHist *prometheus.HistogramVec + prepareHist prometheus.Histogram } var _ Authorizer = (*RegoAuthorizer)(nil) @@ -95,7 +111,7 @@ var ( query rego.PreparedEvalQuery ) -func NewAuthorizer() *RegoAuthorizer { +func NewAuthorizer(registry prometheus.Registerer) *RegoAuthorizer { queryOnce.Do(func() { var err error query, err = rego.New( @@ -106,7 +122,51 @@ func NewAuthorizer() *RegoAuthorizer { panic(xerrors.Errorf("compile rego: %w", err)) } }) - return &RegoAuthorizer{query: query} + + // Register metrics to prometheus. + // These bucket values are based on the average time it takes to run authz + // being around 1ms. Anything under ~2ms is OK and does not need to be + // analyzed any further. + buckets := []float64{ + 0.0005, // 0.5ms + 0.001, // 1ms + 0.002, // 2ms + 0.003, + 0.005, + 0.01, // 10ms + 0.02, + 0.035, // 35ms + 0.05, + 0.075, + 0.1, // 100ms + 0.25, // 250ms + 0.75, // 750ms + 1, // 1s + } + + factory := promauto.With(registry) + authorizeHistogram := factory.NewHistogramVec(prometheus.HistogramOpts{ + Namespace: "coderd", + Subsystem: "authz", + Name: "authorize_duration_seconds", + Help: "Duration of the 'Authorize' call in seconds. Only counts calls that succeed.", + Buckets: buckets, + }, []string{"allowed"}) + + prepareHistogram := factory.NewHistogram(prometheus.HistogramOpts{ + Namespace: "coderd", + Subsystem: "authz", + Name: "prepare_authorize_duration_seconds", + Help: "Duration of the 'PrepareAuthorize' call in seconds.", + Buckets: buckets, + }) + + return &RegoAuthorizer{ + query: query, + + authorizeHist: authorizeHistogram, + prepareHist: prepareHistogram, + } } type authSubject struct { @@ -120,6 +180,18 @@ type authSubject struct { // This is the function intended to be used outside this package. // The role is fetched from the builtin map located in memory. func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, scope Scope, groups []string, action Action, object Object) error { + start := time.Now() + ctx, span := tracing.StartSpan(ctx, + trace.WithTimestamp(start), // Reuse the time.Now for metric and trace + rbacTraceAttributes(roleNames, len(groups), scope, action, object.Type, + // For authorizing a single object, this data is useful to know how + // complex our objects are getting. + attribute.Int("object_num_groups", len(object.ACLGroupList)), + attribute.Int("object_num_users", len(object.ACLUserList)), + ), + ) + defer span.End() + roles, err := RolesByNames(roleNames) if err != nil { return err @@ -131,19 +203,20 @@ func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNa } err = a.Authorize(ctx, subjectID, roles, scopeRole, groups, action, object) + span.AddEvent("authorized", trace.WithAttributes(attribute.Bool("authorized", err == nil))) + dur := time.Since(start) if err != nil { + a.authorizeHist.WithLabelValues("false").Observe(dur.Seconds()) return err } + a.authorizeHist.WithLabelValues("true").Observe(dur.Seconds()) return nil } // Authorize allows passing in custom Roles. // This is really helpful for unit testing, as we can create custom roles to exercise edge cases. func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles []Role, scope Role, groups []string, action Action, object Object) error { - ctx, span := tracing.StartSpan(ctx) - defer span.End() - input := map[string]interface{}{ "subject": authSubject{ ID: subjectID, @@ -166,22 +239,12 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [ return nil } -// Prepare will partially execute the rego policy leaving the object fields unknown (except for the type). -// This will vastly speed up performance if batch authorization on the same type of objects is needed. -func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, scope Role, groups []string, action Action, objectType string) (*PartialAuthorizer, error) { - ctx, span := tracing.StartSpan(ctx) - defer span.End() - - auth, err := newPartialAuthorizer(ctx, subjectID, roles, scope, groups, action, objectType) - if err != nil { - return nil, xerrors.Errorf("new partial authorizer: %w", err) - } - - return auth, nil -} - func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope Scope, groups []string, action Action, objectType string) (PreparedAuthorized, error) { - ctx, span := tracing.StartSpan(ctx) + start := time.Now() + ctx, span := tracing.StartSpan(ctx, + trace.WithTimestamp(start), + rbacTraceAttributes(roleNames, len(groups), scope, action, objectType), + ) defer span.End() roles, err := RolesByNames(roleNames) @@ -194,5 +257,29 @@ func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, return nil, err } - return a.Prepare(ctx, subjectID, roles, scopeRole, groups, action, objectType) + prepared, err := a.Prepare(ctx, subjectID, roles, scopeRole, groups, action, objectType) + if err != nil { + return nil, err + } + + // Add attributes of the Prepare results. This will help understand the + // complexity of the roles and how it affects the time taken. + span.SetAttributes( + attribute.Int("num_queries", len(prepared.preparedQueries)), + attribute.Bool("always_true", prepared.alwaysTrue), + ) + + a.prepareHist.Observe(time.Since(start).Seconds()) + return prepared, nil +} + +// Prepare will partially execute the rego policy leaving the object fields unknown (except for the type). +// This will vastly speed up performance if batch authorization on the same type of objects is needed. +func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, scope Role, groups []string, action Action, objectType string) (*PartialAuthorizer, error) { + auth, err := newPartialAuthorizer(ctx, subjectID, roles, scope, groups, action, objectType) + if err != nil { + return nil, xerrors.Errorf("new partial authorizer: %w", err) + } + + return auth, nil } diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index e6ce53b12a..5aaa71eca7 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/xerrors" @@ -41,7 +42,7 @@ func (w fakeObject) RBACObject() Object { func TestFilterError(t *testing.T) { t.Parallel() - auth := NewAuthorizer() + auth := NewAuthorizer(prometheus.NewRegistry()) _, err := Filter(context.Background(), auth, uuid.NewString(), []string{}, ScopeAll, []string{}, ActionRead, []Object{ResourceUser, ResourceWorkspace}) require.ErrorContains(t, err, "object types must be uniform") @@ -160,7 +161,7 @@ func TestFilter(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() - auth := NewAuthorizer() + auth := NewAuthorizer(prometheus.NewRegistry()) scope := ScopeAll if tc.Scope != "" { @@ -808,7 +809,7 @@ type authTestCase struct { func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTestCase) { t.Helper() - authorizer := NewAuthorizer() + authorizer := NewAuthorizer(prometheus.NewRegistry()) for _, cases := range sets { for i, c := range cases { c := c diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go new file mode 100644 index 0000000000..b010674387 --- /dev/null +++ b/coderd/rbac/authz_test.go @@ -0,0 +1,214 @@ +package rbac_test + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/rbac" +) + +type benchmarkCase struct { + Name string + Roles []string + Groups []string + UserID uuid.UUID + Scope rbac.Scope +} + +// benchmarkUserCases builds a set of users with different roles and groups. +// The user id used as the subject and the orgs used to build the roles are +// returned. +func benchmarkUserCases() (cases []benchmarkCase, users uuid.UUID, orgs []uuid.UUID) { + orgs = []uuid.UUID{ + uuid.MustParse("bf7b72bd-a2b1-4ef2-962c-1d698e0483f6"), + uuid.MustParse("e4660c6f-b9de-422d-9578-cd888983a795"), + uuid.MustParse("fb13d477-06f4-42d9-b957-f6b89bd63515"), + } + + user := uuid.MustParse("10d03e62-7703-4df5-a358-4f76577d4e2f") + // noiseGroups are just added to add noise to the authorize call. They + // never match an object's list of groups. + noiseGroups := []string{uuid.NewString(), uuid.NewString(), uuid.NewString()} + + benchCases := []benchmarkCase{ + { + Name: "NoRoles", + Roles: []string{}, + UserID: user, + Scope: rbac.ScopeAll, + }, + { + Name: "Admin", + // Give some extra roles that an admin might have + Roles: []string{rbac.RoleOrgMember(orgs[0]), "auditor", rbac.RoleOwner(), rbac.RoleMember()}, + UserID: user, + Scope: rbac.ScopeAll, + Groups: noiseGroups, + }, + { + Name: "OrgAdmin", + Roles: []string{rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgAdmin(orgs[0]), rbac.RoleMember()}, + UserID: user, + Scope: rbac.ScopeAll, + Groups: noiseGroups, + }, + { + Name: "OrgMember", + // Member of 2 orgs + Roles: []string{rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgMember(orgs[1]), rbac.RoleMember()}, + UserID: user, + Scope: rbac.ScopeAll, + Groups: noiseGroups, + }, + { + Name: "ManyRoles", + // Admin of many orgs + Roles: []string{ + 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(), + }, + UserID: user, + Scope: rbac.ScopeAll, + Groups: noiseGroups, + }, + { + Name: "AdminWithScope", + // Give some extra roles that an admin might have + Roles: []string{rbac.RoleOrgMember(orgs[0]), "auditor", rbac.RoleOwner(), rbac.RoleMember()}, + UserID: user, + Scope: rbac.ScopeApplicationConnect, + Groups: noiseGroups, + }, + } + return benchCases, users, orgs +} + +// BenchmarkRBACAuthorize benchmarks the rbac.Authorize method. +// +// go test -bench BenchmarkRBACAuthorize -benchmem -memprofile memprofile.out -cpuprofile profile.out +func BenchmarkRBACAuthorize(b *testing.B) { + benchCases, user, orgs := benchmarkUserCases() + users := append([]uuid.UUID{}, + user, + uuid.MustParse("4ca78b1d-f2d2-4168-9d76-cd93b51c6c1e"), + uuid.MustParse("0632b012-49e0-4d70-a5b3-f4398f1dcd52"), + uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"), + ) + authorizer := rbac.NewAuthorizer(prometheus.NewRegistry()) + // This benchmarks all the simple cases using just user permissions. Groups + // are added as noise, but do not do anything. + for _, c := range benchCases { + b.Run(c.Name, func(b *testing.B) { + objects := benchmarkSetup(orgs, users, b.N) + b.ResetTimer() + for i := 0; i < b.N; i++ { + allowed := authorizer.ByRoleName(context.Background(), c.UserID.String(), c.Roles, c.Scope, c.Groups, rbac.ActionRead, objects[b.N%len(objects)]) + var _ = allowed + } + }) + } +} + +// BenchmarkRBACAuthorizeGroups benchmarks the rbac.Authorize method and leverages +// groups for authorizing rather than the permissions/roles. +// +// go test -bench BenchmarkRBACAuthorizeGroups -benchmem -memprofile memprofile.out -cpuprofile profile.out +func BenchmarkRBACAuthorizeGroups(b *testing.B) { + benchCases, user, orgs := benchmarkUserCases() + users := append([]uuid.UUID{}, + user, + uuid.MustParse("4ca78b1d-f2d2-4168-9d76-cd93b51c6c1e"), + uuid.MustParse("0632b012-49e0-4d70-a5b3-f4398f1dcd52"), + uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"), + ) + authorizer := rbac.NewAuthorizer(prometheus.NewRegistry()) + + // Same benchmark cases, but this time groups will be used to match. + // Some '*' permissions will still match, but using a fake action reduces + // the chance. + neverMatchAction := rbac.Action("never-match-action") + for _, c := range benchCases { + b.Run(c.Name+"GroupACL", func(b *testing.B) { + userGroupAllow := uuid.NewString() + c.Groups = append(c.Groups, userGroupAllow) + c.Scope = rbac.ScopeAll + objects := benchmarkSetup(orgs, users, b.N, func(object rbac.Object) rbac.Object { + m := map[string][]rbac.Action{ + // Add the user's group + // Noise + uuid.NewString(): {rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete}, + uuid.NewString(): {rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate}, + uuid.NewString(): {rbac.ActionCreate, rbac.ActionRead}, + uuid.NewString(): {rbac.ActionCreate}, + uuid.NewString(): {rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete}, + uuid.NewString(): {rbac.ActionRead, rbac.ActionUpdate}, + } + for _, g := range c.Groups { + // Every group the user is in will be added, but it will not match the perms. This makes the + // authorizer look at many groups before finding the one that matches. + m[g] = []rbac.Action{rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete} + } + // This is the only group that will give permission. + m[userGroupAllow] = []rbac.Action{neverMatchAction} + return object.WithGroupACL(m) + }) + b.ResetTimer() + for i := 0; i < b.N; i++ { + allowed := authorizer.ByRoleName(context.Background(), c.UserID.String(), c.Roles, c.Scope, c.Groups, neverMatchAction, objects[b.N%len(objects)]) + var _ = allowed + } + }) + } +} + +// BenchmarkRBACFilter benchmarks the rbac.Filter method. +// +// go test -bench BenchmarkRBACFilter -benchmem -memprofile memprofile.out -cpuprofile profile.out +func BenchmarkRBACFilter(b *testing.B) { + benchCases, user, orgs := benchmarkUserCases() + users := append([]uuid.UUID{}, + user, + uuid.MustParse("4ca78b1d-f2d2-4168-9d76-cd93b51c6c1e"), + uuid.MustParse("0632b012-49e0-4d70-a5b3-f4398f1dcd52"), + uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"), + ) + + authorizer := rbac.NewAuthorizer(prometheus.NewRegistry()) + for _, c := range benchCases { + b.Run(c.Name, func(b *testing.B) { + objects := benchmarkSetup(orgs, users, b.N) + b.ResetTimer() + allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, c.Scope, c.Groups, rbac.ActionRead, objects) + require.NoError(b, err) + var _ = allowed + }) + } +} + +func benchmarkSetup(orgs []uuid.UUID, users []uuid.UUID, size int, opts ...func(object rbac.Object) rbac.Object) []rbac.Object { + // Create a "random" but deterministic set of objects. + aclList := map[string][]rbac.Action{ + uuid.NewString(): {rbac.ActionRead, rbac.ActionUpdate}, + uuid.NewString(): {rbac.ActionCreate}, + } + objectList := make([]rbac.Object, size) + for i := range objectList { + objectList[i] = rbac.ResourceWorkspace. + InOrg(orgs[i%len(orgs)]). + WithOwner(users[i%len(users)].String()). + WithACLUserList(aclList). + WithGroupACL(aclList) + + for _, opt := range opts { + objectList[i] = opt(objectList[i]) + } + } + + return objectList +} diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index fc959b26f6..85ef6f6507 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -6,113 +6,13 @@ import ( "testing" "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/rbac" ) -// BenchmarkRBACFilter benchmarks the rbac.Filter method. -// -// go test -bench BenchmarkRBACFilter -benchmem -memprofile memprofile.out -cpuprofile profile.out -func BenchmarkRBACFilter(b *testing.B) { - orgs := []uuid.UUID{ - uuid.MustParse("bf7b72bd-a2b1-4ef2-962c-1d698e0483f6"), - uuid.MustParse("e4660c6f-b9de-422d-9578-cd888983a795"), - uuid.MustParse("fb13d477-06f4-42d9-b957-f6b89bd63515"), - } - - users := []uuid.UUID{ - uuid.MustParse("10d03e62-7703-4df5-a358-4f76577d4e2f"), - uuid.MustParse("4ca78b1d-f2d2-4168-9d76-cd93b51c6c1e"), - uuid.MustParse("0632b012-49e0-4d70-a5b3-f4398f1dcd52"), - uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"), - } - - benchCases := []struct { - Name string - Roles []string - Groups []string - UserID uuid.UUID - Scope rbac.Scope - }{ - { - Name: "NoRoles", - Roles: []string{}, - UserID: users[0], - Scope: rbac.ScopeAll, - }, - { - Name: "Admin", - // Give some extra roles that an admin might have - Roles: []string{rbac.RoleOrgMember(orgs[0]), "auditor", rbac.RoleOwner(), rbac.RoleMember()}, - UserID: users[0], - Scope: rbac.ScopeAll, - }, - { - Name: "OrgAdmin", - Roles: []string{rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgAdmin(orgs[0]), rbac.RoleMember()}, - UserID: users[0], - Scope: rbac.ScopeAll, - }, - { - Name: "OrgMember", - // Member of 2 orgs - Roles: []string{rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgMember(orgs[1]), rbac.RoleMember()}, - UserID: users[0], - Scope: rbac.ScopeAll, - }, - { - Name: "ManyRoles", - // Admin of many orgs - Roles: []string{ - 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(), - }, - UserID: users[0], - Scope: rbac.ScopeAll, - }, - { - Name: "AdminWithScope", - // Give some extra roles that an admin might have - Roles: []string{rbac.RoleOrgMember(orgs[0]), "auditor", rbac.RoleOwner(), rbac.RoleMember()}, - UserID: users[0], - Scope: rbac.ScopeApplicationConnect, - }, - } - - authorizer := rbac.NewAuthorizer() - for _, c := range benchCases { - b.Run(c.Name, func(b *testing.B) { - objects := benchmarkSetup(orgs, users, b.N) - b.ResetTimer() - allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, c.Scope, c.Groups, rbac.ActionRead, objects) - require.NoError(b, err) - var _ = allowed - }) - } -} - -func benchmarkSetup(orgs []uuid.UUID, users []uuid.UUID, size int) []rbac.Object { - // Create a "random" but deterministic set of objects. - aclList := map[string][]rbac.Action{ - uuid.NewString(): {rbac.ActionRead, rbac.ActionUpdate}, - uuid.NewString(): {rbac.ActionCreate}, - } - objectList := make([]rbac.Object, size) - for i := range objectList { - objectList[i] = rbac.ResourceWorkspace. - InOrg(orgs[i%len(orgs)]). - WithOwner(users[i%len(users)].String()). - WithACLUserList(aclList). - WithGroupACL(aclList) - } - - return objectList -} - type authSubject struct { // Name is helpful for test assertions Name string @@ -124,7 +24,7 @@ type authSubject struct { func TestRolePermissions(t *testing.T) { t.Parallel() - auth := rbac.NewAuthorizer() + auth := rbac.NewAuthorizer(prometheus.NewRegistry()) // currentUser is anything that references "me", "mine", or "my". currentUser := uuid.New() diff --git a/coderd/rbac/input.json b/coderd/rbac/input.json new file mode 100644 index 0000000000..6797122a42 --- /dev/null +++ b/coderd/rbac/input.json @@ -0,0 +1,30 @@ +{ + "action": "never-match-action", + "object": { + "owner": "00000000-0000-0000-0000-000000000000", + "org_owner": "bf7b72bd-a2b1-4ef2-962c-1d698e0483f6", + "type": "workspace", + "acl_user_list": { + "f041847d-711b-40da-a89a-ede39f70dc7f": ["create"] + }, + "acl_group_list": {} + }, + "subject": { + "id": "10d03e62-7703-4df5-a358-4f76577d4e2f", + "roles": [], + "groups": ["b617a647-b5d0-4cbe-9e40-26f89710bf18"], + "scope": { + "name": "Scope_all", + "display_name": "All operations", + "site": [ + { + "negate": false, + "resource_type": "*", + "action": "*" + } + ], + "org": {}, + "user": [] + } + } +} diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go index 1bf3155033..b9673c809a 100644 --- a/coderd/rbac/partial.go +++ b/coderd/rbac/partial.go @@ -5,6 +5,8 @@ import ( "github.com/open-policy-agent/opa/ast" "github.com/open-policy-agent/opa/rego" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" "golang.org/x/xerrors" "github.com/coder/coder/coderd/rbac/regosql" @@ -28,7 +30,15 @@ type PartialAuthorizer struct { var _ PreparedAuthorized = (*PartialAuthorizer)(nil) -func (pa *PartialAuthorizer) CompileToSQL(cfg regosql.ConvertConfig) (string, error) { +func (pa *PartialAuthorizer) CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) { + _, span := tracing.StartSpan(ctx, trace.WithAttributes( + // Query count is a rough indicator of the complexity of the query + // that needs to be converted into SQL. + attribute.Int("query_count", len(pa.preparedQueries)), + attribute.Bool("always_true", pa.alwaysTrue), + )) + defer span.End() + filter, err := Compile(cfg, pa) if err != nil { return "", xerrors.Errorf("compile: %w", err) @@ -41,7 +51,8 @@ func (pa *PartialAuthorizer) Authorize(ctx context.Context, object Object) error return nil } - // No queries means always false + // If we have no queries, then no queries can return 'true'. + // So the result is always 'false'. if len(pa.preparedQueries) == 0 { return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), pa.input, nil) } @@ -111,9 +122,6 @@ EachQueryLoop: } func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, scope Role, groups []string, action Action, objectType string) (*PartialAuthorizer, error) { - ctx, span := tracing.StartSpan(ctx) - defer span.End() - input := map[string]interface{}{ "subject": authSubject{ ID: subjectID, diff --git a/coderd/rbac/trace.go b/coderd/rbac/trace.go new file mode 100644 index 0000000000..ee96fa7450 --- /dev/null +++ b/coderd/rbac/trace.go @@ -0,0 +1,20 @@ +package rbac + +import ( + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" +) + +// rbacTraceAttributes are the attributes that are added to all spans created by +// the rbac package. These attributes should help to debug slow spans. +func rbacTraceAttributes(roles []string, groupCount int, scope Scope, action Action, objectType string, extra ...attribute.KeyValue) trace.SpanStartOption { + return trace.WithAttributes( + append(extra, + attribute.StringSlice("subject_roles", roles), + attribute.Int("num_subject_roles", len(roles)), + attribute.Int("num_groups", groupCount), + attribute.String("scope", string(scope)), + attribute.String("action", string(action)), + attribute.String("object_type", objectType), + )...) +} diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 4c0f9eb2a0..2ec7548f54 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -13,6 +13,7 @@ import ( "github.com/cenkalti/backoff/v4" "github.com/go-chi/chi/v5" + "github.com/prometheus/client_golang/prometheus" "cdr.dev/slog" "github.com/coder/coder/coderd" @@ -42,8 +43,11 @@ func New(ctx context.Context, options *Options) (*API, error) { if options.Options == nil { options.Options = &coderd.Options{} } + if options.PrometheusRegistry == nil { + options.PrometheusRegistry = prometheus.NewRegistry() + } if options.Options.Authorizer == nil { - options.Options.Authorizer = rbac.NewAuthorizer() + options.Options.Authorizer = rbac.NewAuthorizer(options.PrometheusRegistry) } ctx, cancelFunc := context.WithCancel(ctx) api := &API{