- Notifications
You must be signed in to change notification settings - Fork927
chore: remove rbac psuedo resources, add custom verbs#13276
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Emyrk commentedMay 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. |
46dbb72 tob2f9524Comparee8977b6 to0089e1dComparecoderd/rbac/policy/policy.go Outdated
| ActionReadPersonal: actDef(fieldOwner, "read personal user data like password"), | ||
| ActionUpdatePersonal: actDef(fieldOwner, "update personal data"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This used to beResourceUserData, but is now just additional verbs
50aee62 to103ed1fCompareeeb6ac2 to116a38eCompare| codersdk/rbacresources_gen.go: scripts/rbacgen/main.go coderd/rbac/object.go | ||
| go run scripts/rbacgen/main.go codersdk > codersdk/rbacresources_gen.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Before this was manuallykept in sync. Or so we told ourselves
coderd/database/dbauthz/dbauthz.go Outdated
| rbac.ResourceFile.Type: {policy.ActionRead}, | ||
| rbac.ResourceSystem.Type: {rbac.WildcardSymbol}, | ||
| rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate}, | ||
| rbac.ResourceUser.Type: {policy.ActionRead}, | ||
| rbac.ResourceWorkspace.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionDelete}, | ||
| rbac.ResourceWorkspaceBuild.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionDelete}, | ||
| rbac.ResourceUserData.Type: {policy.ActionRead, policy.ActionUpdate}, | ||
| rbac.ResourceAPIKey.Type: {rbac.WildcardSymbol}, | ||
| rbac.ResourceFile.Type: {policy.ActionRead}, | ||
| rbac.ResourceSystem.Type: {rbac.WildcardSymbol}, | ||
| rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate}, | ||
| // Unsure why provisionerd needs update and read personal | ||
| rbac.ResourceUser.Type: {policy.ActionRead, policy.ActionReadPersonal, policy.ActionUpdatePersonal}, | ||
| rbac.ResourceWorkspace.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceBuild}, | ||
| rbac.ResourceApiKey.Type: {rbac.WildcardSymbol}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ResourceWorkspaceBuild ->ResourceWorkspace: ActionWorkspaceBuild
(unsure why it needs this, keeping status quo)ResourceUserData ->ResourceUser: ActionReadPersonal, ActionUpdatePersonal
coderd/database/dbauthz/dbauthz.go Outdated
| rbac.ResourceSystem.Type: {rbac.WildcardSymbol}, | ||
| rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate}, | ||
| rbac.ResourceWorkspace.Type: {policy.ActionRead, policy.ActionUpdate}, | ||
| rbac.ResourceWorkspaceBuild.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionDelete}, | ||
| rbac.ResourceUser.Type: {policy.ActionRead}, | ||
| rbac.ResourceSystem.Type: {rbac.WildcardSymbol}, | ||
| rbac.ResourceTemplate.Type: {policy.ActionRead, policy.ActionUpdate}, | ||
| rbac.ResourceWorkspace.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceBuild}, | ||
| rbac.ResourceUser.Type: {policy.ActionRead}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ResourceWorkspaceBuild ->ResourceWorkspace: ActionWorkspaceBuild
coderd/database/dbauthz/dbauthz.go Outdated
| rbac.ResourceWorkspaceBuild.Type: {policy.ActionUpdate}, | ||
| rbac.ResourceWorkspaceExecution.Type: {policy.ActionCreate}, | ||
| rbac.ResourceUser.Type: rbac.ResourceUser.AvailableActions(), | ||
| rbac.ResourceWorkspace.Type: {policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceBuild, policy.ActionSSH}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ResourceWorkspaceExecution ->ResourceWorkspace: ActionSSH
| @@ -106,7 +106,7 @@ You can test outside of golang by using the `opa` cli. | |||
| **Evaluation** | |||
| opa eval --format=pretty'false' -d policy.rego -i input.json | |||
| opa eval --format=pretty"data.authz.allow" -d policy.rego -i input.json | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was a typo I found in the readme.
| // RBACPermissions is indexed by the type | ||
| var RBACPermissions = map[string]PermissionDefinition{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The new source of truth for our policies.
coderd/rbac/roles_test.go Outdated
| // Only run these if the tests on top passed. Otherwise, the error output is too noisy. | ||
| if passed { | ||
| for rtype, v := range remainingPermissions { | ||
| // nolint:tparallel -- Making a subtest for easier diagnosing failures. | ||
| t.Run(fmt.Sprintf("%s-AllActions", rtype), func(t *testing.T) { | ||
| if len(v) > 0 { | ||
| assert.Equal(t, map[policy.Action]bool{}, v, "remaining permissions should be empty for type %q", rtype) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added this to force us to update this test if resources or actions are added. This is sort of our source of truth for our "policy".
Would be really awesome to come up with a custom syntax for making this more compressed...
574d565 to6020cc2Compare6020cc2 to7262bd8Compare1f5788f intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Supports#13226
What this does
Removes our pseudo rbac resources like
WorkspaceApplicationConnectin favor of additional verbs likessh. This is to make more intuitive permissions for building custom roles.Prior to this change, trying to create a custom role creator would not be intuitive or dynamic.
Implementation
The source of truth is now
policy.go.To prevent even more line changes by drastically changing the golang code, the previous resource list is autogenerated from this source of truth:
coder/coderd/rbac/object_gen.go
Lines 218 to 220 in2255162
The autogen is the easiest way to ensure consistency while still using the
rbac.Object.The codersdk authz constants are now also in sync via auto gen:https://github.com/coder/coder/blob/eeb6ac20988b77ae7bf577c79b304fa34a7848f5/codersdk/rbacresources_gen.go
Additional checks
The prior and current implementation do not force the caller to populate the object fields (owner, org, id, acls, etc) or use a correct action when calling the rego.
Now that we have a definitive policy location, we can add at least runtime checks for correctness. I have only added correctness checks for actions on unit tests.
coder/coderd/rbac/authz.go
Lines 243 to 247 ind60f65a
This will prevent asserting actions that are not available for a given object.