OCPCLOUD-3451: Scope capi-operator and capi-installer RBAC to least-privilege#607
OCPCLOUD-3451: Scope capi-operator and capi-installer RBAC to least-privilege#607stefanonardo wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@stefanonardo: This pull request references OCPCLOUD-3451 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRBAC rules are expanded into explicit manifest entries for ChangesRBAC hardening and wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test e2e-aws-capi-techpreview |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/rbac.md (1)
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse full, unambiguous manifest filenames in the reference table.
The table references shortened manifest names (e.g.,
operator_03_clusterrole.yaml,operator_03_capi-installer-clusterrole.yaml) that don't match the actual filenames in the repository. Context snippets show the full names are0000_30_cluster-api-operator_03_clusterrole.yamland0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml. Developers searching the repo for the abbreviated names will not find them by exact match, reducing the documentation's usefulness for locating files. Use the full manifest names for clarity and searchability.✏️ Suggested filename updates in the table
- | `operator_03_clusterrole.yaml` | ClusterRole `openshift-capi-operator` | cluster-wide | `capi-operator` | config.openshift.io resources, ClusterOperator management, ClusterAPI/ConfigMap/Deployment read | - | `operator_03_clusterrole.yaml` | Role `capi-operator` | `openshift-cluster-api-operator` | `capi-operator` | Leader election leases, Deployment write (capi-installer), pod self-read, events | - | `operator_03_capi-installer-clusterrole.yaml` | ClusterRole `openshift-capi-installer` | cluster-wide | `capi-installer` | CRDs, admission resources, cluster RBAC, config.openshift.io, ClusterAPI/ClusterOperator status, tracking cache informers | - | `operator_03_capi-installer-clusterrole.yaml` | Role `capi-installer` | `openshift-cluster-api-operator` | `capi-installer` | Leader election leases, pod self-read, ConfigMap read, events | - | `operator_03_capi-installer-clusterrole.yaml` | Role `capi-installer` | `openshift-cluster-api` | `capi-installer` | boxcutter-managed resources (ServiceAccounts, Services, Deployments, Roles, RoleBindings), VAP paramRef list | - | `operator_03_capi-installer-clusterrole.yaml` | Role `capi-installer` | `openshift-machine-api` | `capi-installer` | VAP paramRef list (machines, machinesets) | + | `0000_30_cluster-api-operator_03_clusterrole.yaml` | ClusterRole `openshift-capi-operator` | cluster-wide | `capi-operator` | config.openshift.io resources, ClusterOperator management, ClusterAPI/ConfigMap/Deployment read | + | `0000_30_cluster-api-operator_03_clusterrole.yaml` | Role `capi-operator` | `openshift-cluster-api-operator` | `capi-operator` | Leader election leases, Deployment write (capi-installer), pod self-read, events | + | `0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml` | ClusterRole `openshift-capi-installer` | cluster-wide | `capi-installer` | CRDs, admission resources, cluster RBAC, config.openshift.io, ClusterAPI/ClusterOperator status, tracking cache informers | + | `0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml` | Role `capi-installer` | `openshift-cluster-api-operator` | `capi-installer` | Leader election leases, pod self-read, ConfigMap read, events | + | `0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml` | Role `capi-installer` | `openshift-cluster-api` | `capi-installer` | boxcutter-managed resources (ServiceAccounts, Services, Deployments, Roles, RoleBindings), VAP paramRef list | + | `0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml` | Role `capi-installer` | `openshift-machine-api` | `capi-installer` | VAP paramRef list (machines, machinesets) |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rbac.md` around lines 11 - 16, Replace the shortened manifest filenames throughout the RBAC reference table with their complete, actual repository filenames. Update all instances of `operator_03_clusterrole.yaml` to `0000_30_cluster-api-operator_03_clusterrole.yaml` and all instances of `operator_03_capi-installer-clusterrole.yaml` to `0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml` in the file column of the table to ensure developers can locate the files through exact repository searches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml`:
- Around line 210-237: In the capi-installer ClusterRole and Role definitions,
locate the two rule blocks for machines and machinesets resources. The first
block is under the cluster.x-k8s.io API group and the second is under the
machine.openshift.io API group. In both rule blocks, change the verbs field from
list to get, since ValidatingAdmissionPolicy paramRef authorization requires the
get verb to retrieve specific parameter resources by name rather than listing
them.
In `@manifests/0000_30_cluster-api-operator_03_clusterrole.yaml`:
- Around line 93-101: The capi-operator Role is missing the `create` verb for
the `deployments` resource in the apps apiGroup. The
InstallerDeploymentReconciler uses server-side apply which requires the `create`
verb to establish field ownership when creating resources that don't exist. Add
`create` to the verbs list for the deployments resource in the apps apiGroup
section to align with the permissions available in the capi-installer Role in
the openshift-cluster-api namespace.
---
Nitpick comments:
In `@docs/rbac.md`:
- Around line 11-16: Replace the shortened manifest filenames throughout the
RBAC reference table with their complete, actual repository filenames. Update
all instances of `operator_03_clusterrole.yaml` to
`0000_30_cluster-api-operator_03_clusterrole.yaml` and all instances of
`operator_03_capi-installer-clusterrole.yaml` to
`0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml` in the file
column of the table to ensure developers can locate the files through exact
repository searches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d9ad8c26-2b98-49a0-ad7a-9f2b88369977
📒 Files selected for processing (5)
docs/rbac.mdmanifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yamlmanifests/0000_30_cluster-api-operator_03_clusterrole.yamlmanifests/0000_30_cluster-api-operator_04_capi-installer-clusterrolebinding.yamlmanifests/0000_30_cluster-api-operator_04_clusterrolebinding.yaml
stefanonardo
force-pushed
the
OCPCLOUD-3451
branch
from
85634f4 to
18f93c0
Compare
|
/test e2e-aws-capi-techpreview |
stefanonardo
force-pushed
the
OCPCLOUD-3451
branch
from
18f93c0 to
1842e55
Compare
|
/test e2e-aws-capi-techpreview |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml`:
- Around line 12-19: Replace the overly permissive wildcard RBAC rule
(apiGroups: ['*'], resources: ['*'], verbs: ['get']) in the capi-installer
ClusterRole with an enumerated list of only the specific resource types that
boxcutter's manifest tracking actually requires. Remove the wildcard entirely
and instead explicitly list the minimal set of resources needed, as the
ValidatingAdmissionPolicyBinding authorization requirement is already satisfied
by existing namespace-scoped Roles that grant narrowly-scoped permissions on the
Machine paramKind from machine.openshift.io.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e7c249ac-8be7-4767-99bc-298e8da066b1
📒 Files selected for processing (5)
docs/rbac.mdmanifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yamlmanifests/0000_30_cluster-api-operator_03_clusterrole.yamlmanifests/0000_30_cluster-api-operator_04_capi-installer-clusterrolebinding.yamlmanifests/0000_30_cluster-api-operator_04_clusterrolebinding.yaml
✅ Files skipped from review due to trivial changes (1)
- docs/rbac.md
🚧 Files skipped from review as they are similar to previous changes (3)
- manifests/0000_30_cluster-api-operator_03_clusterrole.yaml
- manifests/0000_30_cluster-api-operator_04_capi-installer-clusterrolebinding.yaml
- manifests/0000_30_cluster-api-operator_04_clusterrolebinding.yaml
|
/test e2e-aws-capi-techpreview |
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - configmaps | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
Any idea what this is for? Suspect it only needs 1 CM in the -operator namespace. Would be good to pin this down a bit.
There was a problem hiding this comment.
yeah, only capi-installer-images is used. I moved configamaps into namespace Roles.
| - apiGroups: | ||
| - config.openshift.io | ||
| resources: | ||
| - clusteroperators |
There was a problem hiding this comment.
Can we restrict create/update/patch to just its own CO, by name?
There was a problem hiding this comment.
done. create can't be restricted by name because the object does not exist yet and list/watch do not support resourceNames
| - apiGroups: | ||
| - apps | ||
| resources: | ||
| - deployments | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
Does it need this? I see it has permissions to create its specific deployment in -operator below.
There was a problem hiding this comment.
same as configmaps
|
/approve But please address comments before committing. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
stefanonardo
force-pushed
the
OCPCLOUD-3451
branch
from
1842e55 to
c7a1ea4
Compare
|
/test e2e-aws-capi-techpreview |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
manifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml (1)
91-105: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope
bind/escalateto role resources only.
bindandescalateare special RBAC permissions forroles/clusterroles; binding objects only need their normal CRUD verbs. Splitting these rules keeps the required installer behavior while avoiding unnecessary special verbs onrolebindings/clusterrolebindings. (kubernetes.io)As per path instructions, “RBAC: least privilege; no cluster-admin for workloads.”
Suggested RBAC split
- apiGroups: - rbac.authorization.k8s.io resources: - clusterroles - - clusterrolebindings verbs: - get - list - watch - create - update - patch - delete - escalate - bind +- apiGroups: + - rbac.authorization.k8s.io + resources: + - clusterrolebindings + verbs: + - get + - list + - watch + - create + - update + - patch + - delete- apiGroups: - rbac.authorization.k8s.io resources: - roles - - rolebindings verbs: - create - get - update - patch - delete - escalate - bind +- apiGroups: + - rbac.authorization.k8s.io + resources: + - rolebindings + verbs: + - create + - get + - update + - patch + - deleteAlso applies to: 210-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@manifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml` around lines 91 - 105, The RBAC rule in the installer ClusterRole is granting bind and escalate alongside normal CRUD verbs for both roles and binding resources, which is too broad. Update the ClusterRole manifest so the special verbs are scoped only to the roles/clusterroles rules, and keep rolebindings/clusterrolebindings limited to standard get/list/watch/create/update/patch/delete access. Use the existing RBAC rule block in the installer ClusterRole as the place to split these permissions.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@manifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yaml`:
- Around line 91-105: The RBAC rule in the installer ClusterRole is granting
bind and escalate alongside normal CRUD verbs for both roles and binding
resources, which is too broad. Update the ClusterRole manifest so the special
verbs are scoped only to the roles/clusterroles rules, and keep
rolebindings/clusterrolebindings limited to standard
get/list/watch/create/update/patch/delete access. Use the existing RBAC rule
block in the installer ClusterRole as the place to split these permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2f198517-9f40-4f56-8e7b-f034099c22e7
📒 Files selected for processing (5)
docs/rbac.mdmanifests/0000_30_cluster-api-operator_03_capi-installer-clusterrole.yamlmanifests/0000_30_cluster-api-operator_03_clusterrole.yamlmanifests/0000_30_cluster-api-operator_04_capi-installer-clusterrolebinding.yamlmanifests/0000_30_cluster-api-operator_04_clusterrolebinding.yaml
✅ Files skipped from review due to trivial changes (1)
- docs/rbac.md
🚧 Files skipped from review as they are similar to previous changes (1)
- manifests/0000_30_cluster-api-operator_04_capi-installer-clusterrolebinding.yaml
stefanonardo
force-pushed
the
OCPCLOUD-3451
branch
from
c7a1ea4 to
809c78f
Compare
|
/lgtm |
|
Scheduling tests matching the |
Replace the shared wildcard ClusterRole with separate, enumerated RBAC for each ServiceAccount. Validated with audit2rbac and e2e tests on an AWS cluster. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stefanonardo
force-pushed
the
OCPCLOUD-3451
branch
from
809c78f to
55b154a
Compare
|
/test e2e-aws-capi-techpreview |
|
@stefanonardo: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
/test e2e-aws-capi-techpreview |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/pipeline required |
|
Scheduling tests matching the |
4 participants
Summary
openshift-capi-operator(apiGroups: ['*'], resources: ['*'], verbs: ['*']) with separate, enumerated RBAC for each ServiceAccountcapi-operatorgets a narrow ClusterRole (config resources, ClusterOperator management restricted tocluster-apiby name) + namespace Roles inopenshift-cluster-api-operator(leases, Deployment write, configmaps, pod self-read, events) andopenshift-cluster-api(read-only cache informers for deployments and configmaps)capi-installergets a ClusterRole withget+list */*(needed for VAP binding creation when policies don't exist yet — the API server falls back to checking bothgetandliston all resources), enumerated write permissions (CRDs, admission resources, cluster RBAC withescalate/bind), and namespace Roles inopenshift-cluster-api(boxcutter-managed resources, VAP paramRef),openshift-cluster-api-operator(leases, pod/configmap read), andopenshift-machine-api(VAP paramRef list)Permission justification
Each rule was derived from code analysis and validated against audit2rbac output from an AWS cluster.
capi-operator
config.openshift.ioinfrastructuresmain.go:104util.GetInfra()config.openshift.ioclusteroperatorsclusteroperator_controller.go:73,operator_status.go:119config.openshift.ioclusteroperatorsoperator_status.go:186(resourceNames: cluster-api)config.openshift.ioclusteroperators/statusoperator_status.go:186(resourceNames: cluster-api)config.openshift.ioclusterversions,featuregatesmain.go:114util.GetFeatureGates()operator.openshift.ioclusterapisinstallerdeployment/controller.go:167appsdeploymentscontroller.go:148SSA,:163For(),:209delete""configmapscontroller.go:107,166""podsmain.go:158pod self-read""eventsoperator_status.go:98coordination.k8s.ioleasesappsdeployments""configmapscapi-installer
**getandliston all resources. Also covers boxcutter tracking cache readsconfig.openshift.ioinfrastructuresrevision_controller.go:307Watches()config.openshift.ioclusteroperatorsconfig.openshift.ioclusteroperators/statusrelated_objects.go:113,controller_status.go:295operator.openshift.ioclusterapisinstaller_controller.go:100,revision_controller.go:303operator.openshift.ioclusterapis/statusrevision_controller.go:234,installer_controller.go:316apiextensions.k8s.iocustomresourcedefinitionsadmissionregistration.k8s.iomutating/validatingwebhookconfigurations,validatingadmissionpolicies/bindingsrbac.authorization.k8s.ioclusterrolesrbac.authorization.k8s.ioclusterrolebindingsroles,rolebindings,serviceaccounts,services,deploymentsserviceaccounts,services,deploymentsrbac.authorization.k8s.iorolesrbac.authorization.k8s.iorolebindingscluster.x-k8s.iomachines,machinesetsselector: {})machine.openshift.iomachines,machinesetsselector: {})coordination.k8s.ioleases""pods""configmapscapi-installer-imagesConfigMapNot observed by audit2rbac — justified by code or CI evidence
config.openshift.ioclusteroperatorsoperator_status.go:119— first install onlyrbac.authorization.k8s.ioclusterrolescapi-manager-role) that grant permissions to CAPI provider SAs. Invisible to audit2rbac**does not have "list" permission for all groups, versions and resourcesappsdeploymentscontroller.go:148SSA create on first install,:209unsupported platform cleanup🤖 Generated with Claude Code