OCPCLOUD-1645: Scope capi-controllers RBAC to least-privilege#585
OCPCLOUD-1645: Scope capi-controllers RBAC to least-privilege#585stefanonardo 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-1645 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughWildcard RBAC rules were removed and replaced with explicit ClusterRole and Role rules; RoleBindings were added to bind those Roles to the capi-controllers ServiceAccount in the target namespaces. ChangesRBAC Permissions Hardening
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
stefanonardo
force-pushed
the
OCPCLOUD-1645
branch
from
8ea8125 to
cfd7831
Compare
|
/jira refresh |
|
@stefanonardo: This pull request references OCPCLOUD-1645 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
manifests/0000_30_cluster-api_03_rbac_roles.yaml (1)
246-256: 💤 Low valueCluster-wide secrets access is intentional but worth documenting.
Trivy flags this rule because cluster-wide secrets management is a sensitive permission. However, this appears justified given:
- Secret Sync Controller needs cross-namespace access (MAPI ↔ CAPI namespaces)
- Kubeconfig Controller manages kubeconfig secrets
- The
deleteverb is intentionally omitted, demonstrating least-privilege- This is a significant improvement over the previous wildcards
Consider adding a comment in the manifest documenting why cluster-wide scope is required, which helps future audits.
🤖 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_03_rbac_roles.yaml` around lines 246 - 256, Add an inline comment above the ClusterRole rule that grants resources: ["secrets"] under apiGroups: [""] explaining why cluster-wide secrets access is required (e.g., Secret Sync Controller must sync secrets between MAPI and CAPI namespaces and the Kubeconfig Controller manages kubeconfig secrets), note that delete is intentionally omitted to preserve least-privilege, and reference the controllers (Secret Sync Controller, Kubeconfig Controller) so auditors can understand the justification during reviews.Source: 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_03_rbac_roles.yaml`:
- Around line 246-256: Add an inline comment above the ClusterRole rule that
grants resources: ["secrets"] under apiGroups: [""] explaining why cluster-wide
secrets access is required (e.g., Secret Sync Controller must sync secrets
between MAPI and CAPI namespaces and the Kubeconfig Controller manages
kubeconfig secrets), note that delete is intentionally omitted to preserve
least-privilege, and reference the controllers (Secret Sync Controller,
Kubeconfig Controller) so auditors can understand the justification during
reviews.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 76c3409f-8bf9-49a3-a6ff-eeb8fc36df3c
📒 Files selected for processing (1)
manifests/0000_30_cluster-api_03_rbac_roles.yaml
|
/test ? |
|
/test required |
|
/lgtm |
|
Scheduling tests matching the |
|
/lgtm cancel Haven't actually looked at this, yet. Was just checking if it was waiting on the pipeline controller. Wasn't sure it was, as normally you see the pipeline jobs listed but 'Waiting', but they were not present at all. |
stefanonardo
force-pushed
the
OCPCLOUD-1645
branch
from
cfd7831 to
44913ce
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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_03_rbac_roles.yaml`:
- Around line 254-264: The ClusterRole currently grants cluster-wide read/write
on Secrets (apiGroups: "", resources: secrets, verbs:
get/list/watch/create/update/patch) which is too permissive; remove secret
permissions from the ClusterRole and instead create namespace-scoped Role +
RoleBinding pairs that grant only the needed secret verbs in the exact
namespaces touched by the secret-sync/kubeconfig flows, bind those Roles to the
capi-controllers ServiceAccount used by openshift-capi-controllers, and where
the set of secrets is fixed use resourceNames to restrict to specific Secret
objects; update any references to the removed ClusterRole permissions
accordingly.
🪄 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: 954effa5-a923-4230-8052-f53063758493
📒 Files selected for processing (1)
manifests/0000_30_cluster-api_03_rbac_roles.yaml
stefanonardo
force-pushed
the
OCPCLOUD-1645
branch
from
44913ce to
947e14a
Compare
Replace wildcard RBAC rules with enumerated resources and verbs for the capi-controllers ServiceAccount. Validated with audit2rbac and e2e tests on an AWS cluster with MachineAPIMigration enabled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stefanonardo
force-pushed
the
OCPCLOUD-1645
branch
from
947e14a to
39788fd
Compare
|
@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. |
4 participants
Summary
apiGroups: ['*'], resources: ['*'], verbs: ['*']) in theopenshift-capi-controllersClusterRole andcapi-controllersRole with enumerated, auditable permissionsopenshift-cluster-api,openshift-machine-api, andkube-system— secrets access is no longer cluster-widePermission justification
Each rule was derived from code analysis and validated against audit2rbac output from an AWS cluster with
MachineAPIMigrationenabled.Confirmed by audit2rbac
config.openshift.ioinfrastructurescorecluster_controller.go:122,infracluster_controller.go:278,kubeconfig.go:85config.openshift.ioclusteroperatorsoperator_status.go:115,186,controller_status.go:295config.openshift.ioclusteroperators/statusoperator_status.go:186,controller_status.go:295config.openshift.iofeaturegates,clusterversionsfeaturegates.go:50-53apiextensions.k8s.iocustomresourcedefinitionsmachine-api-migration)""nodescluster-api-controller-manager)openshift-cluster-apicluster.x-k8s.ioclusterscorecluster_controller.go:122,152,machine_sync_controller.go:857openshift-cluster-apicluster.x-k8s.ioclusters/statuscorecluster_controller.go:216openshift-cluster-apicluster.x-k8s.iomachinesmachine_sync_controller.go:220,656,686,1060openshift-cluster-apicluster.x-k8s.iomachines/statusmachine_sync_controller.go:1365openshift-cluster-apicluster.x-k8s.iomachinesetsmachineset_sync_controller.go:200,838,866,1191openshift-cluster-apicluster.x-k8s.iomachinesets/statusmachineset_sync_controller.go:917openshift-cluster-apiinfrastructure.cluster.x-k8s.ioawsclustersaws.go:57,90openshift-cluster-apiinfrastructure.cluster.x-k8s.ioawsclusters/statusinfracluster_controller.go:162openshift-cluster-apiinfrastructure.cluster.x-k8s.ioawsmachinesmachine_sync_mapi2capi_infrastructure.go:108,136,213openshift-cluster-apiinfrastructure.cluster.x-k8s.ioawsmachines/statusmachine_sync_mapi2capi_infrastructure.go:194openshift-cluster-apiinfrastructure.cluster.x-k8s.ioawsmachinetemplatesmachineset_sync_controller.go:251,453,763,492openshift-cluster-api""secretssecret_sync_controller.go:73,90,161,kubeconfig.go:138,171,209openshift-cluster-api""eventsmachine_sync_controller.go:180,machineset_sync_controller.go:151openshift-cluster-apicoordination.k8s.ioleasesopenshift-machine-apimachine.openshift.iomachinesmachine_sync_controller.go:203,1391,1467,1039openshift-machine-apimachine.openshift.iomachines/statusmachine_sync_controller.go:1441,migratestatus.go:105openshift-machine-apimachine.openshift.iomachinesetsmachineset_sync_controller.go:190,943,1149openshift-machine-apimachine.openshift.iomachinesets/statusmachineset_sync_controller.go:996,migratestatus.go:105openshift-machine-apimachine.openshift.iocontrolplanemachinesetsinfracluster_controller.go:285-288,412openshift-machine-api""secretssecret_sync_controller.go:73(worker-user-data source)openshift-machine-api""eventsmachine_sync_controller.go:180Not observed by audit2rbac — justified by code evidence
config.openshift.ioclusteroperatorsoperator_status.go:119— GetOrCreateClusterOperator creates CO if missing (first install).openshift-cluster-apicluster.x-k8s.ioclusters/finalizers,machines/finalizers,machinesets/finalizersblockOwnerDeletion: trueon ownerReferences (machine_sync_controller.go:577,867,machineset_sync_controller.go:635). Kubernetes requiresupdateon the owner'sfinalizerssubresource. Not captured by audit2rbac (validated by GC admission plugin, not a separate API call).openshift-cluster-apicluster.x-k8s.ioclustersopenshift-cluster-api""secretssecret_sync_controller.go:152,azure.go:131— creates secrets if not found.openshift-cluster-api""eventscreatewas observed.openshift-cluster-apicoordination.k8s.ioleasescreateonly on first leader election.list/watch/deleteare standard controller-runtime patterns.openshift-cluster-apiinfrastructure.cluster.x-k8s.ioopenshift-cluster-apiinfrastructure.cluster.x-k8s.ioazureclusteridentitiesazure.go:142Get,:188Create.openshift-cluster-apiinfrastructure.cluster.x-k8s.ioawsmachinetemplatesopenshift-machine-apimachine.openshift.iomachinesetsmachineset_sync_controller.go— creates MAPI MachineSets during CAPI→MAPI sync. Only MAPI→CAPI direction was tested.kube-system""secretsvsphere.go:157— only exercised on vSphere clusters.Test plan
make lintpasses🤖 Generated with Claude Code
Summary by CodeRabbit