-
Notifications
You must be signed in to change notification settings - Fork 12
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
RBAC v2 role bindings #249
Conversation
fabc7dc
to
77c5da0
Compare
Signed-off-by: Bailin He <[email protected]>
Signed-off-by: Bailin He <[email protected]>
313f671
to
8e0dc0d
Compare
Signed-off-by: Bailin He <[email protected]>
8e0dc0d
to
c7b2205
Compare
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.
Looks directionally correct so far. A couple of things stick out:
- For mutations, we need to ensure the ZedToken for affected resources gets populated immediately
- We need some kind of locking mechanism similar to
LockRoleForUpdate
. For mutations on bindings, we will want to lock on both the role and the binding so we don't wind up with situations like trying to create a binding on a role that's being deleted
internal/api/rolebindings.go
Outdated
// permissions on role binding actions, similar to roles v1, are granted on the resources | ||
if err := r.checkActionWithResponse(ctx, subjectResource, string(iapl.RoleBindingActionCreate), resource); err != nil { | ||
return err | ||
} |
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 will be good to call out somewhere. From my understanding, this means that if I have the ability to perform a role binding on a resource, I can bind any available role.
One thing we'll want to add next is ensuring that you can only bind roles that have a subset of the actions you yourself can do on that resource. Roughly speaking, I can see that working as something like a bulk check to see that the user performing the binding can perform all of the actions in the role on the resource.
Edit: "add next" = add in a separate PR.
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 worth some discussions, my thought is that we should allow "IAM admins" to assign any permissions to users under the same OU and its children.
One can argue that "IAM admin" would essentially be a "super user" or owner and I would agree, "IAM admin" is indeed a role that need to be treated very cautiously, but the same goes with "owner" or "super user". And that's where groups come in to provide the flexibility, a group manager will be able to grant certain permissions to users without being an IAM admin.
Signed-off-by: Bailin He <[email protected]>
3a26665
to
8537662
Compare
30227c1
to
c065fc0
Compare
Co-authored-by: John Schaeffer <[email protected]> Signed-off-by: Bailin He <[email protected]>
c065fc0
to
98f22bc
Compare
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.
More thoughts. Looks better, not sure we need all the complexity around batch deleting role bindings when deleting a role (since I'm not sure we want to support that behavior anyway).
e0a7c96
to
cf92079
Compare
Co-authored-by: John Schaeffer <[email protected]> Signed-off-by: Bailin He <[email protected]>
cf92079
to
b72d193
Compare
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.
Looks good! A couple of minor comments before I do some manual testing of this.
// buildBatchInClauseWithIDs is a helper function that builds an IN clause for | ||
// a batch query with the provided prefixed IDs. | ||
func (e *engine) buildBatchInClauseWithIDs(ids []gidx.PrefixedID) (clause string, args []any) { | ||
args = make([]any, len(ids)) | ||
|
||
for i, id := range ids { | ||
fmtStr := "$%d" | ||
|
||
if i > 0 { | ||
fmtStr = ", $%d" | ||
} | ||
|
||
clause += fmt.Sprintf(fmtStr, i+1) | ||
args[i] = id.String() | ||
} | ||
|
||
return clause, args | ||
} |
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.
Do we still need this?
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.
it also used in rolev2
where we need to grab a bunch of roles, might as well keep it
Co-authored-by: John Schaeffer <[email protected]> Signed-off-by: Bailin He <[email protected]>
ee5599f
to
d85ed39
Compare
Signed-off-by: Bailin He <[email protected]>
e58dd42
to
c89ceb5
Compare
Signed-off-by: Bailin He <[email protected]>
c89ceb5
to
77784a8
Compare
Signed-off-by: Bailin He <[email protected]>
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.
Looks good enough for more thorough hands-on testing.
continue work on #245
The API should be updated to use them in place of direct role assignments. The scope of this task is to make the following API changes:
Roles
Create appropriate action relationships from the roles to subject:* type relationships rather than relationships from other resources to the roles
Assignments
Create a binding when creating an assignment rather than creating a direct membership to the role
List both bindings and direct memberships when listing assignments (and show an ID for bindings)
Delete both bindings and direct memberships when deleting assignments
Check permissions based on binding_{create,get,update,delete} actions rather than role-specific actions
See this OpenAPI specs for full list of APIs