-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fleet RBAC - InheritedFleetWorkspacePermissions validation #348
Fleet RBAC - InheritedFleetWorkspacePermissions validation #348
Conversation
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.
A couple of items:
- You need escalation checks for GlobalRoleBindings as well as GlobalRoles
- Please update the docs for the GlobalRole checks.
- The escalation logic is a bit flawed. The approach that we use needs to consider these permissions on their own since it gives permissions on all current workspaces and all future workspaces (besides local).
pkg/resources/rbac.authorization.k8s.io/v1/clusterrole/validator_test.go
Show resolved
Hide resolved
pkg/resources/rbac.authorization.k8s.io/v1/clusterrolebinding/validator_test.go
Show resolved
Hide resolved
cf97831
to
006c813
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.
Some minor issues and one overall larger issue regarding escalation checking.
pkg/resources/rbac.authorization.k8s.io/v1/clusterrole/validator.go
Outdated
Show resolved
Hide resolved
pkg/resources/rbac.authorization.k8s.io/v1/clusterrolebinding/validator.go
Outdated
Show resolved
Hide resolved
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.
A few concerns on the architecture/code structure, but the functionality/escalation checking looks much better.
pkg/auth/globalrole.go
Outdated
return nil | ||
} | ||
|
||
var rules []rbacv1.PolicyRule |
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.
Rather than allocating a new slice and copying the rules over, it might be good to just return what is in the role - like gr.InheritedFleetWorkspacePermissions.ResourceRules
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.
done
pkg/resolvers/grbClusterResolver.go
Outdated
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 core object here is called a GRBClusterRuleResolver
. I don't think that we can make this evaluate other types of rules (i.e. permissions on/in the workspace) without a name change.
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.
renamed to GRBRuleResolver
pkg/resources/management.cattle.io/v3/globalrolebinding/validator.go
Outdated
Show resolved
Hide resolved
pkg/resources/rbac.authorization.k8s.io/v1/clusterrolebinding/validator_test.go
Outdated
Show resolved
Hide resolved
36016dc
to
9f1fb01
Compare
57b1a9f
to
1d1092f
Compare
1d1092f
to
c7b77c9
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.
One NIT, but outside of that LGTM (After fixing merge conflict).
- Validate the user have enough permission to create/update the rules defined in InheritedFleetWorkspacePermissions.ResourceRules - Validate the user have enough permission to create/pdate the rules that are generated based on the InheritedFleetWorkspacePermissions.WorkspaceVerbs
Co-authored-by: Michael Bolot <michael.bolot@suse.com>
c7b77c9
to
70b603a
Compare
Issue: rancher/rancher#44901
Problem
Need to add validation for the new fields in
GlobalRole
InheritedFleetWorkspacePermissions
Solution
InheritedFleetWorkspacePermissions.ResourceRules
InheritedFleetWorkspacePermissions.WorkspaceVerbs
authz.management.cattle.io/gr-owner
andauthz.management.cattle.io/grb-owner
inClusterRole
andClusterRoleBinding
CheckList