-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add Default RBAC for Non-Admins #1025
Add Default RBAC for Non-Admins #1025
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.
Nice. Do we need to mention this somewhere in our documentation?
/approve +1 on @SaschaSchwarze0 's note about identifying in the docs ... feels like a new could also get into some detail about the rbac defined for the controllers, though I would not block merge if you did not want to take that on @adambkaplan |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabemontero The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
@adambkaplan looks good. Any reasons you could think against enabling the get
verb for the cluster-scope strategies?
My two thoughts:
- (abstraction): Users shouldn't need to take a look at them. Would like to keep them as much abstracted as possible. I think
list
is good enough. - (security): Being able to see cluster-scope strategies might expose too much. Not sure if this applies in general, probably yes wherever there is a hard definition between users and strategy admins.
wdyt?
I think get and list should be fine. Security-wise, a strategy does not expose anything in addition to what users can see anyway in the generated TaskRun or Pod. The only thing you could hide is the existance. Not sure if there is a use case for this. Scenario-wise having access to the strategies is important for client applications. If you implement a UI that allows to create a Build, then it will want to prefil a strategy drop-down with the available strategies. Once a strategy is selected, a UI will also want to look at the strategy's parameter definitions to render a form for them. |
@SaschaSchwarze0 right good points. |
Tekton allows "view" users to get any TaskRun in their namespace, so I think this proves @SaschaSchwarze0's point that there is minimal security risk granting "get" permission to https://github.com/tektoncd/pipeline/blob/main/config/clusterrole-aggregate-view.yaml
I imagine UIs for k8s create their own rolebindings and service accounts, though they may inherit these user-facing "view" or "edit" roles. This is a reasonable user story IMO. |
f4c5024
to
5754773
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.
Thanks for adding the details. Suggesting to add a link to the Kubernetes documentation about aggregated roles.
docs/configuration.md
Outdated
|
||
- `shpwright-build-aggregate-view`: this role grants read access (get, list, watch) to most Shipwright Build objects. | ||
This includes `BuildStrategy`, `ClusterBuildStrategy`, `Build`, and `BuildRun` objects. | ||
This role is aggregated to the Kubernetes "view" role. |
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 role is aggregated to the Kubernetes "view" role. | |
This role is [aggregated to the Kubernetes "view" role](https://kubernetes.io/docs/reference/access-authn-authz/rbac/#aggregated-clusterroles). |
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.
Thanks for the suggestion - I think the "Default user roles" section of that article may be a better fit:
https://kubernetes.io/docs/reference/access-authn-authz/rbac/#default-roles-and-role-bindings
- Add view aggregate role for inspecting Shipwright Build objects - Add edit aggregate role for managing Builds, BuildRuns, and BuildStrategies. - Ensure aggregated user roles are installed in e2e testing - Document the default cluster roles in the "configuration" markdown doc, with relevant links to upstream k8s docs.
5754773
to
af24c9b
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.
/lgtm
Changes
Fixes #1023
/kind feature
Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes