Skip to content
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: Add view and edit aggregated cluster roles #3566

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Feb 7, 2023

This PR adds two cluster roles which grant the Kubernetes view, edit and admin roles access to Flux custom resources.

Fix: #2409
Fix: #3572
Ref: fluxcd/kustomize-controller#762

@stefanprodan stefanprodan added area/UX area/bootstrap Bootstrap related issues and pull requests area/install Install and uninstall related issues and pull requests area/security Security related issues and pull requests labels Feb 7, 2023
@stefanprodan stefanprodan requested a review from hiddeco February 7, 2023 12:03
@stefanprodan stefanprodan mentioned this pull request Feb 7, 2023
21 tasks
name: flux-edit
labels:
rbac.authorization.k8s.io/aggregate-to-edit: "true"
rbac.authorization.k8s.io/aggregate-to-admin: "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still necessary? Back in August 2018 I learned from @deads2k that "view" aggregation is supposed to be included by "edit," and "edit" is supposed to be include by "admin" automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

cert-manager and other controllers have these set. I guess it doesn't hurt to make this explicit so people reading the Flux RBAC understand what's going on.

Copy link
Contributor

@seh seh left a comment

Choose a reason for hiding this comment

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

Is the idea with the "edit" and "admin" mutative rights that a user who has those roles granted in a given namespace could edit the target objects that Flux would reconcile, annotating or labeling them to disable Flux's coverage?

I was thinking at first that an administrator might want to retain full control over the Flux-related objects in a given namespace, in order to ensure that they continue projecting some required set of objects. However, I realized that other users with the "edit" or "admin" roles granted could still fight back against Flux, since it honors the "kustomize.toolkit.fluxcd.io/reconcile" annotation and label.

@stefanprodan
Copy link
Member Author

@seh the admin/edit are for Flux impersonation, instead of creating a role binding to cluster-admin, the tenant service account could bind to admin and still be able to reconcile HelmRelease, Alerts, etc. The actual tenants shouldn't have edit rights to the cluster, all changes should be made in Git and Flux should apply them. I would grand tenants the view role only.

@seh
Copy link
Contributor

seh commented Feb 7, 2023

The actual tenants shouldn't have edit rights to the cluster, all changes should be made in Git and Flux should apply them. I would gran[t] tenants the view role only.

I understand that intention. It so happens that in my organization we have a variety of tenants, some of which are engineers maintaining the applications that run within a given namespace. We have to allow them fairly liberal privileges to inspect and manipulate things, even if they like to use Flux as a convenient way to deploy similar changes to many Kubernetes clusters easily. In other words, in this model, Flux serves as assistive automation, as opposed to enforcement of policy.

@stefanprodan
Copy link
Member Author

stefanprodan commented Feb 7, 2023

We have to allow them fairly liberal privileges to inspect and manipulate things, even if they like to use Flux as a convenient way to deploy similar changes to many Kubernetes clusters easily. In other words, in this model, Flux serves as assistive automation, as opposed to enforcement of policy.

Does this RBAC change prevents you from doing that?

@seh
Copy link
Contributor

seh commented Feb 7, 2023

Does this RBAC change prevents you from doing that?

It makes it more likely that we'd accidentally grant broader privileges than we had intended, but if we really need to prevent that, we can delete these ClusterRoles with a kustomize patch when reconciling Flux's manifests, or patch them to remove the "aggregate-to-..." annotations.

Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
@seh
Copy link
Contributor

seh commented Feb 20, 2023

Thank you for this part of the documentation, @stefanprodan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bootstrap Bootstrap related issues and pull requests area/install Install and uninstall related issues and pull requests area/security Security related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

listing kustomizations requires cluster admin privileges Add more scopes to the default k8s ClusterRoles
3 participants