Minimize RBAC roles for controllers

[killianmuldoon]

Following on from ##6510 (comment)

In Cluster API today we create RBAC manifests which are generated using kubebuilder tools. e.g.

cluster-api/internal/controllers/clusterclass/clusterclass_controller.go

Lines 42 to 44 in 626ab4d

	// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io;bootstrap.cluster.x-k8s.io;controlplane.cluster.x-k8s.io,resources=*,verbs=get;list;watch;update;patch
	// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusterclasses,verbs=get;list;watch;update;patch
	// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch

Many of our controllers seem to generate overly broad RBAC for themselves. e.g. many have the create verb when it is not used.
Removing these roles should have no impact on functitonality, minimize the capabilities of our controllers from a security perspective, and make our RBAC manifests more descriptive about what our controllers are actually doing.

We should audit the following controllers to minimize their RBAC roles:

• MachineHealthCheck
• Machinepool
• KubeadmConfig
• ClusterResourceSet
• Cluster
• Machine
• MachineDeployment
• MachineSet
• Topology/Cluster
• ClusterClass

[sbueringer]

We should be very careful. Not all verbs can be easily inferred from just reading the controller code:

• we probably have to go transitively through a few levels of util funcs
• controller-runtime internally creates (per default) informers when we call client.Get(), client.List(), ... (this requires get/list/watch verbs)

[sbueringer]

One simplification I did in the past is to categorize verbs into groups:

• read: get list watch
• create
• delete
• change: update patch

Essentially once we need one of them we just add the entire group. This simplifies it a bit. For example, once we use list is it really worth figuring out if we have a get or watch call somewhere? It comes down to pretty much the same result. Similar for update/patch.

If we are overly strict we risk missing a call somewhere, and if that call is only rarely used in edge cases we might not catch that case in e2e tests.

I think we have to balance minimal roles vs the risk of breaking something. Especially given that even with minimal roles per reconciler the whole controller will still have a lot of rights.

[killianmuldoon]

That seems like a sensible approach. My impression is that we probably have create in a few places it shouldn't be. I think we have update in a lot of places where it's not used, but we're using patch so it doesn't matter.

I'm also thinking this could be something pushed to the next API rev.

[sbueringer]

I think it's independent of our API version bump as it just affects the permissions of our controller and not the APIs/CRDs.

[killianmuldoon]

I didn't mean to say it's dependent on a rev, more that we could do it as part of a future overall API revision as it's not urgent now IMO

[fabriziopandini]

We can use also tools like https://github.com/liggitt/audit2rbac to check what is actually used

[sbueringer]

We can use also tools like https://github.com/liggitt/audit2rbac to check what is actually used

Good idea, but I think we should only use it as additional info not as a reference, otherwise this means we have to hit all edge cases to be safe.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[fabriziopandini]

/lifecycle frozen
/triage accepted
this is still a nice issue to be addressed

/help

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/lifecycle frozen
/triage accepted
this is still a nice issue to be addressed

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[fabriziopandini]

/kind cleanup
/priority important-longterm

[fabriziopandini]

It is still worth to audit the code to figure out if there is something suspicious in our permissions
/help
/triage accepted

[chrischdi]

/assign