From 425cec0943a2ecbac9d9b11aeb7c92e7d9d458f6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 Jan 2023 10:32:03 -0600 Subject: [PATCH 1/2] chore: Authz should support non-named roles Named roles are a construct for users to assign/interact with roles. For authzlayer implementation, we need to create "system" users. To enforce strict security, we are making specific roles with the exact required permissions for the system action. These new roles should not be available to the user. There is a clear code divide with this implementation that allows a RoleNames implemenation for users to user, and system users can create their own implementation --- coderd/coderdtest/authorize.go | 10 ++++----- coderd/httpmw/apikey.go | 3 ++- coderd/rbac/authz.go | 35 +++++++++++++++++++++--------- coderd/rbac/authz_internal_test.go | 4 ++-- coderd/rbac/authz_test.go | 2 +- coderd/rbac/builtin.go | 18 ++++++++++++++- coderd/rbac/builtin_test.go | 2 +- 7 files changed, 53 insertions(+), 21 deletions(-) diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index 462dcc644f56f..03ad70ed338e6 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -529,7 +529,7 @@ func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck type authCall struct { SubjectID string - Roles []string + Roles rbac.ExpandableRoles Groups []string Scope rbac.ScopeName Action rbac.Action @@ -545,11 +545,11 @@ var _ rbac.Authorizer = (*RecordingAuthorizer)(nil) // ByRoleNameSQL does not record the call. This matches the postgres behavior // of not calling Authorize() -func (r *RecordingAuthorizer) ByRoleNameSQL(_ context.Context, _ string, _ []string, _ rbac.ScopeName, _ []string, _ rbac.Action, _ rbac.Object) error { +func (r *RecordingAuthorizer) ByRoleNameSQL(_ context.Context, _ string, _ rbac.ExpandableRoles, _ rbac.ScopeName, _ []string, _ rbac.Action, _ rbac.Object) error { return r.AlwaysReturn } -func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, scope rbac.ScopeName, groups []string, action rbac.Action, object rbac.Object) error { +func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames rbac.ExpandableRoles, scope rbac.ScopeName, groups []string, action rbac.Action, object rbac.Object) error { r.Called = &authCall{ SubjectID: subjectID, Roles: roleNames, @@ -561,7 +561,7 @@ func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, ro return r.AlwaysReturn } -func (r *RecordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, scope rbac.ScopeName, groups []string, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) { +func (r *RecordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles rbac.ExpandableRoles, scope rbac.ScopeName, groups []string, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) { return &fakePreparedAuthorizer{ Original: r, SubjectID: subjectID, @@ -580,7 +580,7 @@ func (r *RecordingAuthorizer) reset() { type fakePreparedAuthorizer struct { Original *RecordingAuthorizer SubjectID string - Roles []string + Roles rbac.ExpandableRoles Scope rbac.ScopeName Action rbac.Action Groups []string diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index c72d0d40e606a..14250c3e59583 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -20,6 +20,7 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" ) @@ -53,7 +54,7 @@ type userAuthKey struct{} type Authorization struct { ID uuid.UUID Username string - Roles []string + Roles rbac.RoleNames Groups []string Scope database.APIKeyScope } diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index d3e43b4e181a3..b897306d2a1e5 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -17,9 +17,24 @@ import ( "github.com/coder/coder/coderd/tracing" ) +// ExpandableRoles is any type that can be expanded into a []Role. This is implemented +// as an interface so we can have RoleNames for user defined roles, and implement +// custom ExpandableRoles for system type users (eg autostart/autostop system role). +// We want a clear divide between the two types of roles so users have no codepath +// to interact or assign system roles. +// +// Note: We may also want to do the same thing with scopes to allow custom scope +// support unavailable to the user. Eg: Scope to a single resource. +type ExpandableRoles interface { + Expand() ([]Role, error) + // Names is fo logging and tracing purposes, we want to know the human + // names of the expanded roles. + Names() []string +} + type Authorizer interface { - ByRoleName(ctx context.Context, subjectID string, roleNames []string, scope ScopeName, groups []string, action Action, object Object) error - PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope ScopeName, groups []string, action Action, objectType string) (PreparedAuthorized, error) + ByRoleName(ctx context.Context, subjectID string, roleNames ExpandableRoles, scope ScopeName, groups []string, action Action, object Object) error + PrepareByRoleName(ctx context.Context, subjectID string, roleNames ExpandableRoles, scope ScopeName, groups []string, action Action, objectType string) (PreparedAuthorized, error) } type PreparedAuthorized interface { @@ -33,7 +48,7 @@ type PreparedAuthorized interface { // // 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 ScopeName, groups []string, action Action, objects []O) ([]O, error) { +func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles ExpandableRoles, scope ScopeName, groups []string, action Action, objects []O) ([]O, error) { if len(objects) == 0 { // Nothing to filter return objects, nil @@ -45,7 +60,7 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub // 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, + rbacTraceAttributes(subjRoles.Names(), 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 @@ -179,11 +194,11 @@ type authSubject struct { // ByRoleName will expand all roleNames into roles before calling Authorize(). // 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 ScopeName, groups []string, action Action, object Object) error { +func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames ExpandableRoles, scope ScopeName, 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, + rbacTraceAttributes(roleNames.Names(), 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)), @@ -192,7 +207,7 @@ func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNa ) defer span.End() - roles, err := RolesByNames(roleNames) + roles, err := roleNames.Expand() if err != nil { return err } @@ -239,15 +254,15 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [ return nil } -func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope ScopeName, groups []string, action Action, objectType string) (PreparedAuthorized, error) { +func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames ExpandableRoles, scope ScopeName, groups []string, action Action, objectType string) (PreparedAuthorized, error) { start := time.Now() ctx, span := tracing.StartSpan(ctx, trace.WithTimestamp(start), - rbacTraceAttributes(roleNames, len(groups), scope, action, objectType), + rbacTraceAttributes(roleNames.Names(), len(groups), scope, action, objectType), ) defer span.End() - roles, err := RolesByNames(roleNames) + roles, err := roleNames.Expand() if err != nil { return nil, err } diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go index 204d65551f47e..0afff5226ec38 100644 --- a/coderd/rbac/authz_internal_test.go +++ b/coderd/rbac/authz_internal_test.go @@ -44,7 +44,7 @@ func TestFilterError(t *testing.T) { t.Parallel() auth := NewAuthorizer(prometheus.NewRegistry()) - _, err := Filter(context.Background(), auth, uuid.NewString(), []string{}, ScopeAll, []string{}, ActionRead, []Object{ResourceUser, ResourceWorkspace}) + _, err := Filter(context.Background(), auth, uuid.NewString(), RoleNames{}, ScopeAll, []string{}, ActionRead, []Object{ResourceUser, ResourceWorkspace}) require.ErrorContains(t, err, "object types must be uniform") } @@ -75,7 +75,7 @@ func TestFilter(t *testing.T) { testCases := []struct { Name string SubjectID string - Roles []string + Roles RoleNames Action Action Scope ScopeName ObjectType string diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index eb36fdb665537..f8588fa9baae5 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -13,7 +13,7 @@ import ( type benchmarkCase struct { Name string - Roles []string + Roles rbac.RoleNames Groups []string UserID uuid.UUID Scope rbac.ScopeName diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 964eec3a7386f..c2737ef82e7e5 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -19,6 +19,18 @@ const ( orgMember string = "organization-member" ) +// RoleNames is a list of user assignable role names. The role names must be +// in the builtInRoles map. +type RoleNames []string + +func (names RoleNames) Expand() ([]Role, error) { + return rolesByNames(names) +} + +func (names RoleNames) Names() []string { + return names +} + // The functions below ONLY need to exist for roles that are "defaulted" in some way. // Any other roles (like auditor), can be listed and let the user select/assigned. // Once we have a database implementation, the "default" roles can be defined on the @@ -244,6 +256,10 @@ func CanAssignRole(roles []string, assignedRole string) bool { // RoleByName returns the permissions associated with a given role name. // This allows just the role names to be stored and expanded when required. +// +// This function is exported so that the Display name can be returned to the +// api. We should maybe make an exported function that returns just the +// human-readable content of the Role struct (name + display name). func RoleByName(name string) (Role, error) { roleName, orgID, err := roleSplit(name) if err != nil { @@ -266,7 +282,7 @@ func RoleByName(name string) (Role, error) { return role, nil } -func RolesByNames(roleNames []string) ([]Role, error) { +func rolesByNames(roleNames []string) ([]Role, error) { roles := make([]Role, 0, len(roleNames)) for _, n := range roleNames { r, err := RoleByName(n) diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index 6a15e77f5415d..cdbf652367ca1 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -17,7 +17,7 @@ type authSubject struct { // Name is helpful for test assertions Name string UserID string - Roles []string + Roles rbac.RoleNames Groups []string } From 17ea3c30c3c67c315e3ea16f46259959774e9396 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 Jan 2023 10:37:10 -0600 Subject: [PATCH 2/2] Update comments --- coderd/rbac/authz.go | 2 +- coderd/rbac/builtin.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index b897306d2a1e5..888176c212780 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -27,7 +27,7 @@ import ( // support unavailable to the user. Eg: Scope to a single resource. type ExpandableRoles interface { Expand() ([]Role, error) - // Names is fo logging and tracing purposes, we want to know the human + // Names is for logging and tracing purposes, we want to know the human // names of the expanded roles. Names() []string } diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index c2737ef82e7e5..76d7f373a16f9 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -20,7 +20,8 @@ const ( ) // RoleNames is a list of user assignable role names. The role names must be -// in the builtInRoles map. +// in the builtInRoles map. Any non-user assignable roles will generate an +// error on Expand. type RoleNames []string func (names RoleNames) Expand() ([]Role, error) {