Skip to content

feat: Implement allow_list for scopes for resource specific permissions #5769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jan 19, 2023
Merged

feat: Implement allow_list for scopes for resource specific permissions #5769

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
212ebce
feat: Implement allow_list for scopes for resource specific permissions
Emyrk Jan 18, 2023
655e8db
Fix rbac benchmark
Emyrk Jan 18, 2023
5f5653d
Update coderd/rbac/scopes.go
Emyrk Jan 18, 2023
4d66a03
Update coderd/rbac/scopes.go
Emyrk Jan 18, 2023
f924578
Include workspace agent token example in rbac readme
Emyrk Jan 18, 2023
b9fed91
WithID unit tests and model method support
Emyrk Jan 18, 2023
504c54b
Fix typo and fmt of readme
Emyrk Jan 18, 2023
257701f
Plumbing WithID for rbac objects (incomplete)
Emyrk Jan 18, 2023
a9cf8f4
Plumb through WithID on rbac objects (complete)
Emyrk Jan 18, 2023
fa44bfe
Fix TestAuthorizeAllEndpoints token routes
Emyrk Jan 19, 2023
94ad568
Fix TestAuthorizeAllEndpoints provisonerdaemon route
Emyrk Jan 19, 2023
043a1d5
Rename Scope -> ScopeName
Emyrk Jan 19, 2023
e0e6312
Update input.json with scope allow_list
Emyrk Jan 19, 2023
7492385
Allow empty string allow_list
Emyrk Jan 19, 2023
64a246d
Delete unused file.
Emyrk Jan 19, 2023
ca88a38
Rename Scope back to 'Scope'
Emyrk Jan 19, 2023
9e4d72f
Merge remote-tracking branch 'origin/main' into stevenmasley/scope_re…
Emyrk Jan 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions coderd/rbac/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ This can be represented by the following truth table, where Y represents _positi
- `level` is either `site`, `org`, or `user`.
- `object` is any valid resource type.
- `id` is any valid UUID v4.
- `id` is included in the permission syntax, however only scopes may use `id` to specify a specific object.
- `action` is `create`, `read`, `modify`, or `delete`.

## Example Permissions
Expand All @@ -72,6 +73,20 @@ Y indicates that the role provides positive permissions, N indicates the role pr
| | \_ | \_ | N | N |
| unauthenticated | \_ | \_ | \_ | N |

## Scopes

Scopes can restrict a given set of permissions. The format of a scope matches a role with the addition of a list of resource ids. For a authorization call to be successful, the subject's roles and the subject's scopes must both allow the action. This means the resulting permissions is the intersection of the subject's roles and the subject's scopes.

An example to give a readonly token is to grant a readonly scope across all resources `+site.*.*.read`. The intersection with the user's permissions will be the readonly set of their permissions.


### Resource IDs

There exists use cases that require specifying a specific resource. If resource IDs are allowed in the roles, then there is
an unbounded set of resource IDs that be added to an "allow_list", as the number of roles a user can have is unbounded. This also adds a level of complexity to the role evaluation logic that has large costs at scale.

The use case for specifying this type of permission in a role is limited, and does not justify the extra cost. To solve this for the remaining cases (eg. workspace agent tokens), we can apply an `allow_list` on a scope. For most cases, the `allow_list` will just be `["*"]` which means the scope is allowed to be applied to any resource. This adds negligible cost to the role evaluation logic and 0 cost to partial evaluations.

# Testing

You can test outside of golang by using the `opa` cli.
Expand Down
16 changes: 8 additions & 8 deletions coderd/rbac/authz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,10 @@ func NewAuthorizer(registry prometheus.Registerer) *RegoAuthorizer {
}

type authSubject struct {
ID string `json:"id"`
Roles []Role `json:"roles"`
Groups []string `json:"groups"`
Scope Role `json:"scope"`
ID string `json:"id"`
Roles []Role `json:"roles"`
Groups []string `json:"groups"`
Scope ScopeRole `json:"scope"`
}

// ByRoleName will expand all roleNames into roles before calling Authorize().
Expand All @@ -197,7 +197,7 @@ func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNa
return err
}

scopeRole, err := ScopeRole(scope)
scopeRole, err := ExpandScope(scope)
if err != nil {
return err
}
Expand All @@ -216,7 +216,7 @@ func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNa

// 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 {
func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles []Role, scope ScopeRole, groups []string, action Action, object Object) error {
input := map[string]interface{}{
"subject": authSubject{
ID: subjectID,
Expand Down Expand Up @@ -252,7 +252,7 @@ func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string,
return nil, err
}

scopeRole, err := ScopeRole(scope)
scopeRole, err := ExpandScope(scope)
if err != nil {
return nil, err
}
Expand All @@ -275,7 +275,7 @@ func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string,

// 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) {
func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, scope ScopeRole, 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)
Expand Down
26 changes: 13 additions & 13 deletions coderd/rbac/authz_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ type subject struct {
// For the unit test we want to pass in the roles directly, instead of just
// by name. This allows us to test custom roles that do not exist in the product,
// but test edge cases of the implementation.
Roles []Role `json:"roles"`
Groups []string `json:"groups"`
Scope Role `json:"scope"`
Roles []Role `json:"roles"`
Groups []string `json:"groups"`
Scope ScopeRole `json:"scope"`
}

type fakeObject struct {
Expand Down Expand Up @@ -200,7 +200,7 @@ func TestAuthorizeDomain(t *testing.T) {

user := subject{
UserID: "me",
Scope: must(ScopeRole(ScopeAll)),
Scope: must(ExpandScope(ScopeAll)),
Groups: []string{allUsersGroup},
Roles: []Role{
must(RoleByName(RoleMember())),
Expand Down Expand Up @@ -299,7 +299,7 @@ func TestAuthorizeDomain(t *testing.T) {

user = subject{
UserID: "me",
Scope: must(ScopeRole(ScopeAll)),
Scope: must(ExpandScope(ScopeAll)),
Roles: []Role{{
Name: "deny-all",
// List out deny permissions explicitly
Expand Down Expand Up @@ -340,7 +340,7 @@ func TestAuthorizeDomain(t *testing.T) {

user = subject{
UserID: "me",
Scope: must(ScopeRole(ScopeAll)),
Scope: must(ExpandScope(ScopeAll)),
Roles: []Role{
must(RoleByName(RoleOrgAdmin(defOrg))),
must(RoleByName(RoleMember())),
Expand Down Expand Up @@ -374,7 +374,7 @@ func TestAuthorizeDomain(t *testing.T) {

user = subject{
UserID: "me",
Scope: must(ScopeRole(ScopeAll)),
Scope: must(ExpandScope(ScopeAll)),
Roles: []Role{
must(RoleByName(RoleOwner())),
must(RoleByName(RoleMember())),
Expand Down Expand Up @@ -408,7 +408,7 @@ func TestAuthorizeDomain(t *testing.T) {

user = subject{
UserID: "me",
Scope: must(ScopeRole(ScopeApplicationConnect)),
Scope: must(ExpandScope(ScopeApplicationConnect)),
Roles: []Role{
must(RoleByName(RoleOrgMember(defOrg))),
must(RoleByName(RoleMember())),
Expand Down Expand Up @@ -507,7 +507,7 @@ func TestAuthorizeDomain(t *testing.T) {
// In practice this is a token scope on a regular subject
user = subject{
UserID: "me",
Scope: must(ScopeRole(ScopeAll)),
Scope: must(ExpandScope(ScopeAll)),
Roles: []Role{
{
Name: "ReadOnlyOrgAndUser",
Expand Down Expand Up @@ -600,7 +600,7 @@ func TestAuthorizeLevels(t *testing.T) {

user := subject{
UserID: "me",
Scope: must(ScopeRole(ScopeAll)),
Scope: must(ExpandScope(ScopeAll)),
Roles: []Role{
must(RoleByName(RoleOwner())),
{
Expand Down Expand Up @@ -661,7 +661,7 @@ func TestAuthorizeLevels(t *testing.T) {

user = subject{
UserID: "me",
Scope: must(ScopeRole(ScopeAll)),
Scope: must(ExpandScope(ScopeAll)),
Roles: []Role{
{
Name: "site-noise",
Expand Down Expand Up @@ -726,7 +726,7 @@ func TestAuthorizeScope(t *testing.T) {
user := subject{
UserID: "me",
Roles: []Role{must(RoleByName(RoleOwner()))},
Scope: must(ScopeRole(ScopeApplicationConnect)),
Scope: must(ExpandScope(ScopeApplicationConnect)),
}

testAuthorize(t, "Admin_ScopeApplicationConnect", user,
Expand Down Expand Up @@ -760,7 +760,7 @@ func TestAuthorizeScope(t *testing.T) {
must(RoleByName(RoleMember())),
must(RoleByName(RoleOrgMember(defOrg))),
},
Scope: must(ScopeRole(ScopeApplicationConnect)),
Scope: must(ExpandScope(ScopeApplicationConnect)),
}

testAuthorize(t, "User_ScopeApplicationConnect", user,
Expand Down
2 changes: 2 additions & 0 deletions coderd/rbac/input.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"action": "never-match-action",
"object": {
"id": "00000000-0000-0000-0000-000000000000",
"owner": "00000000-0000-0000-0000-000000000000",
"org_owner": "bf7b72bd-a2b1-4ef2-962c-1d698e0483f6",
"type": "workspace",
Expand All @@ -16,6 +17,7 @@
"scope": {
"name": "Scope_all",
"display_name": "All operations",
"allow_list": ["123", "*"],
"site": [
{
"negate": false,
Expand Down
17 changes: 17 additions & 0 deletions coderd/rbac/object.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ var (
// that represents the set of workspaces you are trying to get access too.
// Do not export this type, as it can be created from a resource type constant.
type Object struct {
// ID is the resource's uuid
ID string `json:"id"`
Owner string `json:"owner"`
// OrgID specifies which org the object is a part of.
OrgID string `json:"org_owner"`
Expand All @@ -184,9 +186,21 @@ func (z Object) All() Object {
}
}

func (z Object) WithID(id uuid.UUID) Object {
return Object{
ID: id.String(),
Owner: z.Owner,
OrgID: z.OrgID,
Type: z.Type,
ACLUserList: z.ACLUserList,
ACLGroupList: z.ACLGroupList,
}
}

// InOrg adds an org OwnerID to the resource
func (z Object) InOrg(orgID uuid.UUID) Object {
return Object{
ID: z.ID,
Owner: z.Owner,
OrgID: orgID.String(),
Type: z.Type,
Expand All @@ -198,6 +212,7 @@ func (z Object) InOrg(orgID uuid.UUID) Object {
// WithOwner adds an OwnerID to the resource
func (z Object) WithOwner(ownerID string) Object {
return Object{
ID: z.ID,
Owner: ownerID,
OrgID: z.OrgID,
Type: z.Type,
Expand All @@ -209,6 +224,7 @@ func (z Object) WithOwner(ownerID string) Object {
// WithACLUserList adds an ACL list to a given object
func (z Object) WithACLUserList(acl map[string][]Action) Object {
return Object{
ID: z.ID,
Owner: z.Owner,
OrgID: z.OrgID,
Type: z.Type,
Expand All @@ -219,6 +235,7 @@ func (z Object) WithACLUserList(acl map[string][]Action) Object {

func (z Object) WithGroupACL(groups map[string][]Action) Object {
return Object{
ID: z.ID,
Owner: z.Owner,
OrgID: z.OrgID,
Type: z.Type,
Expand Down
3 changes: 2 additions & 1 deletion coderd/rbac/partial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ EachQueryLoop:
return ForbiddenWithInternal(xerrors.Errorf("policy disallows request"), pa.input, nil)
}

func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, scope Role, groups []string, action Action, objectType string) (*PartialAuthorizer, error) {
func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, scope ScopeRole, groups []string, action Action, objectType string) (*PartialAuthorizer, error) {
input := map[string]interface{}{
"subject": authSubject{
ID: subjectID,
Expand All @@ -141,6 +141,7 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, s
rego.Query("data.authz.allow = true"),
rego.Module("policy.rego", policy),
rego.Unknowns([]string{
"input.object.id",
"input.object.owner",
"input.object.org_owner",
"input.object.acl_user_list",
Expand Down
20 changes: 20 additions & 0 deletions coderd/rbac/policy.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,23 @@ user_allow(roles) := num {
num := number(allow)
}

# Scope allow_list is a list of resource IDs explicitly allowed by the scope.
# If the list is '*', then all resources are allowed.
scope_allow_list {
"*" in input.subject.scope.allow_list
}

scope_allow_list {
# If the wildcard is listed in the allow_list, we do not care about the
# object.id. This line is included to prevent partial compilations from
# ever needing to include the object.id.
not "*" in input.subject.scope.allow_list
input.object.id != ""
input.object.id in input.subject.scope.allow_list
}



# The allow block is quite simple. Any set with `-1` cascades down in levels.
# Authorization looks for any `allow` statement that is true. Multiple can be true!
# Note that the absence of `allow` means "unauthorized".
Expand Down Expand Up @@ -179,15 +196,18 @@ role_allow {
}

scope_allow {
scope_allow_list
scope_site = 1
}

scope_allow {
scope_allow_list
not scope_site = -1
scope_org = 1
}

scope_allow {
scope_allow_list
not scope_site = -1
not scope_org = -1
# If we are not a member of an org, and the object has an org, then we are
Expand Down
47 changes: 30 additions & 17 deletions coderd/rbac/scopes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,39 +8,52 @@ import (

type Scope string

// TODO: @emyrk rename this struct
type ScopeRole struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be Scope and the string should be ScopeID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, I am thinking of changing this plumbing, I just wanted to implement the allow_list into the rego policy. I will come back to this once I get IDs onto rbac objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still going to rename?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed Scope to ScopeName. So I can rename ScopeRole -> Scope again 👍

Will do now.

Role
AllowIDList []string `json:"allow_list"`
}

const (
ScopeAll Scope = "all"
ScopeApplicationConnect Scope = "application_connect"
)

var builtinScopes map[Scope]Role = map[Scope]Role{
// TODO: Support passing in scopeID list for whitelisting allowed resources.
var builtinScopes map[Scope]ScopeRole = map[Scope]ScopeRole{
// ScopeAll is a special scope that allows access to all resources. During
// authorize checks it is usually not used directly and skips scope checks.
ScopeAll: {
Name: fmt.Sprintf("Scope_%s", ScopeAll),
DisplayName: "All operations",
Site: permissions(map[string][]Action{
ResourceWildcard.Type: {WildcardSymbol},
}),
Org: map[string][]Permission{},
User: []Permission{},
Role: Role{
Name: fmt.Sprintf("Scope_%s", ScopeAll),
DisplayName: "All operations",
Site: permissions(map[string][]Action{
ResourceWildcard.Type: {WildcardSymbol},
}),
Org: map[string][]Permission{},
User: []Permission{},
},
AllowIDList: []string{WildcardSymbol},
},

ScopeApplicationConnect: {
Name: fmt.Sprintf("Scope_%s", ScopeApplicationConnect),
DisplayName: "Ability to connect to applications",
Site: permissions(map[string][]Action{
ResourceWorkspaceApplicationConnect.Type: {ActionCreate},
}),
Org: map[string][]Permission{},
User: []Permission{},
Role: Role{
Name: fmt.Sprintf("Scope_%s", ScopeApplicationConnect),
DisplayName: "Ability to connect to applications",
Site: permissions(map[string][]Action{
ResourceWorkspaceApplicationConnect.Type: {ActionCreate},
}),
Org: map[string][]Permission{},
User: []Permission{},
},
AllowIDList: []string{WildcardSymbol},
},
}

func ScopeRole(scope Scope) (Role, error) {
func ExpandScope(scope Scope) (ScopeRole, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it makes sense to keep Scope as a string in the database once we are going to have things like an allowlist into it. Do we need to expand it to a JSON object?

We're not going to be able to keep a map of all workspace scopes in memory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to, but we are going to create the workspace agent authorization context in memory, not from the db.

Agent tokens right now do not have an RBAC subject related to it, it's not an APIKey. So when we implement agent tokens, we will use the workspace owner's roles and add the agent scope to it. Meaning the scope is not in the database.

But you are correct if we use this for more and more cases, it might require us to elevate this to the DB in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are agent tokens tied to a specific workspace at present (in the DB)?

Copy link
Member Author

@Emyrk Emyrk Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

coder/coderd/database/dump.sql

Lines 437 to 438 in b9fed91

resource_id uuid NOT NULL,
auth_token uuid NOT NULL,

The connection is through the workspace_resources table, so you have to follow some foreign keys, but it is connected to the workspace.

role, ok := builtinScopes[scope]
if !ok {
return Role{}, xerrors.Errorf("no scope named %q", scope)
return ScopeRole{}, xerrors.Errorf("no scope named %q", scope)
}
return role, nil
}