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.

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

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Jan 18, 2023

What this does

This allows making scopes for specific resources to support workspace agent tokens. This PR is a prerequisite to pushing RBAC authz checks to the db layer. When authz checks are pushed down, workspace agent tokens will conform to the same RBAC as users. To prevent workspace agent tokens from having more access than required, we need to limit the token to a single workspace, and ideally do this in the authz.

Implementation

Add an allow_list to the scope section to specify particular resources. User scopes will almost always have an allow_list = ["*"].

I have not added the code to add resource ids to scopes yet. It is not yet in use and the plumbing can be pushed off. Currently users can define scopes when making api keys, tbd if we should give them access to this or only use it internally.

Performance

Originally our RBAC supported an id field in permissions. It was removed as it added complexity that would impact performance, especially at scale. If permissions can specify individual resource IDs, it could be heavily used compounding the number of permissions required to be checked for each authz. The resource ID check also makes partial evaluation increase in complexity and adds id = ANY(ARRAY ['id-1', 'id-2', ...])) to all SQL filters. Performance of RBAC authz checks is critical to user experience, so the extra complexity was dropped.

To support the edge cases where this is required, we can limit the additional complexity by adding it as a field on the scope. Only 1 scope can be applied, so this list is always going to be explicitly set once, rather than being a combination of multiple roles.

The additional cost is to be negligible for all current authz costs. The benchmarks show that using the all scope has no impact. The rego check is very quick:

scope_allow_list {
	"*" in input.subject.scope.allow_list
}

For partial evaluations, the rego check is designed to add 0 additional queries and 0 additional terms to existing queries if * is in the 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
}

If the allow_list is used, performance on authz call in memory will likely not be impacted (TODO confirm). SQL filters will have additional queries and terms in the existing queries (TODO eval sql impact).

Plumbing

Part of this plumbing is making sure ID is included in RBAC objects sent to authorize. This will be much easier to conform and find instances when the intermediate layer between the db is implemented.

Until then, I am manually tracking down all cases. If a case is missed, it will fail secure.

Future

If this implementation is too limited for the future, we investigate alternative more complex solutions. Migrating this to another implementation will not be challenging imo. This is an incredibly simple approach to solving this issue. Since the set of scopes is currently a finite hard coded set, it gives a lot of flexibility in handling any migration.

Tl'dr: We are not coding ourselves into a corner here.

Notes

  • ProvisionerDaemons are not currently org scoped resources. Should associate them to an org?

Emyrk added 2 commits January 18, 2023 10:09
Feature that adds an allow_list for scopes to specify particular resources.
This enables workspace agent tokens to use the same RBAC system as users.
Emyrk and others added 4 commits January 18, 2023 11:28
Co-authored-by: Cian Johnston <cian@coder.com>
Co-authored-by: Cian Johnston <cian@coder.com>
- Add ID to compileSQL matchers
},
}

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

Choose a reason for hiding this comment

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

Yes.

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.

@@ -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.

@Emyrk Emyrk marked this pull request as ready for review January 19, 2023 15:37
Emyrk added 2 commits January 19, 2023 10:01
- Add comments to Scope
- Remove extra lines in rego policy
@Emyrk Emyrk merged commit 08cce81 into main Jan 19, 2023
@Emyrk Emyrk deleted the stevenmasley/scope_resource_ids branch January 19, 2023 19:41
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants