OCPBUGS-15157: Replace wildcard permissions with explicit verbs and resources in MCC ClusterRole#6000
OCPBUGS-15157: Replace wildcard permissions with explicit verbs and resources in MCC ClusterRole#6000isabella-janssen wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughThis PR tightens machine-config-controller RBAC by replacing wildcard ClusterRole rules with explicit resource-scoped rules, adds a namespace-scoped Role and RoleBinding for configmaps, and wires those new manifests into the operator sync logic. ChangesRBAC Permission Refinement and Namespace-Scoped Access
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isabella-janssen 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
manifests/machineconfigcontroller/clusterrole.yaml (1)
12-14: 💤 Low valueCluster-wide secrets management flagged by static analysis.
Trivy flags this rule (KSV-0041) for granting cluster-wide secrets management permissions. While this may be necessary for the MCC to function (e.g., managing pull secrets, certificates), consider whether these permissions could be scoped to specific namespaces via a namespaced
Roleinstead of aClusterRoleto follow the principle of least privilege.If cluster-wide access is genuinely required, this is acceptable but should be documented.
🤖 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/machineconfigcontroller/clusterrole.yaml` around lines 12 - 14, The ClusterRole grants cluster-wide secret and configmap management; either narrow it to a namespaced Role or document justification: replace the ClusterRole that contains resources ["configmaps","secrets"] and verbs ["get","list","watch","create","update","delete"] with a Role scoped to the specific namespace(s) the controller operates in and update any RoleBinding(s) (instead of ClusterRoleBinding) to bind the controller ServiceAccount, or if cluster-wide access is truly required, keep the ClusterRole but add a clear comment/markdown entry documenting why cluster scope is necessary and reference the ClusterRole name so auditors can find the justification.
🤖 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/machineconfigcontroller/clusterrole.yaml`:
- Around line 9-11: The RBAC rule for apiGroups:
["machineconfiguration.openshift.io"] is missing the machineosconfigs and
machineosbuilds resources used by pkg/operator/sync.go (list at line ~1613) and
pkg/controller/build/reconciler.go (status updates); update the resources array
in clusterrole.yaml to include "machineosconfigs", "machineosconfigs/status",
"machineosbuilds", and "machineosbuilds/status" and ensure the verbs for those
entries include the same verbs as the other resources (e.g.,
"get","list","watch","create","update","patch") so list operations and status
updates are permitted.
---
Nitpick comments:
In `@manifests/machineconfigcontroller/clusterrole.yaml`:
- Around line 12-14: The ClusterRole grants cluster-wide secret and configmap
management; either narrow it to a namespaced Role or document justification:
replace the ClusterRole that contains resources ["configmaps","secrets"] and
verbs ["get","list","watch","create","update","delete"] with a Role scoped to
the specific namespace(s) the controller operates in and update any
RoleBinding(s) (instead of ClusterRoleBinding) to bind the controller
ServiceAccount, or if cluster-wide access is truly required, keep the
ClusterRole but add a clear comment/markdown entry documenting why cluster scope
is necessary and reference the ClusterRole name so auditors can find the
justification.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: adf54928-9c34-47b3-8b98-75fe20947d21
📒 Files selected for processing (1)
manifests/machineconfigcontroller/clusterrole.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
manifests/machineconfigcontroller/clusterrole.yaml (1)
15-17: 💤 Low valueCluster-wide secrets access is intentional but warrants documentation.
Static analysis flags this as a security concern (KSV-0041), but per the PR objectives, secrets remain cluster-scoped due to the cluster-wide informer requirement. The controller accesses secrets from multiple namespaces (MCO namespace,
openshift-config,openshift-config-managed).Consider adding a comment in the manifest or documentation explaining why cluster-wide secret access is necessary, to help future reviewers understand this architectural decision.
🤖 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/machineconfigcontroller/clusterrole.yaml` around lines 15 - 17, Add a clear justification comment/annotation to the ClusterRole that grants cluster-scoped access to "secrets" (apiGroups: [""], resources: ["secrets"], verbs: ["get","list","watch","create","update","delete"]) explaining that the controller requires a cluster-wide secrets informer to read secrets from the MCO namespace as well as openshift-config and openshift-config-managed; include the security reference (KSV-0041) and note that this is an intentional architectural choice to aid future reviewers and auditors.
🤖 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/machineconfigcontroller/clusterrole.yaml`:
- Around line 15-17: Add a clear justification comment/annotation to the
ClusterRole that grants cluster-scoped access to "secrets" (apiGroups: [""],
resources: ["secrets"], verbs:
["get","list","watch","create","update","delete"]) explaining that the
controller requires a cluster-wide secrets informer to read secrets from the MCO
namespace as well as openshift-config and openshift-config-managed; include the
security reference (KSV-0041) and note that this is an intentional architectural
choice to aid future reviewers and auditors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dfc0e9e6-2b6b-4d1b-87ce-b0b1e877f573
📒 Files selected for processing (4)
manifests/machineconfigcontroller/clusterrole.yamlmanifests/machineconfigcontroller/configmaps-role-target.yamlmanifests/machineconfigcontroller/configmaps-rolebinding-target.yamlpkg/operator/sync.go
✅ Files skipped from review due to trivial changes (1)
- manifests/machineconfigcontroller/configmaps-rolebinding-target.yaml
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-1of2 periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-2of2 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@isabella-janssen: trigger 8 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9a54aeb0-4948-11f1-97df-c2fe2d4c9954-0 |
isabella-janssen
force-pushed
the
ocpbugs-15157
branch
2 times, most recently
from
abb3205 to
3fdce3c
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-1of2 periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-2of2 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@isabella-janssen: trigger 8 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c04fe680-494c-11f1-9a60-486def3e988a-0 |
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/machineconfigcontroller/configmaps-rolebinding-target.yaml`:
- Around line 6-8: The RoleBinding's roleRef is missing the required apiGroup;
update the roleRef for the Role named "machine-config-controller-configmaps" by
adding apiGroup: rbac.authorization.k8s.io alongside kind: Role and name:
machine-config-controller-configmaps so the RoleBinding's roleRef explicitly
includes apiGroup for consistent behavior across Kubernetes versions.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 445d8f66-cfe6-48ec-bdb0-daf4ebadddf7
📒 Files selected for processing (4)
manifests/machineconfigcontroller/clusterrole.yamlmanifests/machineconfigcontroller/configmaps-role-target.yamlmanifests/machineconfigcontroller/configmaps-rolebinding-target.yamlpkg/operator/sync.go
isabella-janssen
force-pushed
the
ocpbugs-15157
branch
3 times, most recently
from
e87d3c2 to
e963398
Compare
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/machineconfigcontroller/clusterrole.yaml`:
- Around line 10-14: Restore the missing RBAC permissions by adding "delete"
back to the verbs array for the machineconfiguration.openshift.io rule (the rule
that currently has verbs: ["get","list","watch","create","update","patch"]) and
ensure the apiGroups: ["machineconfiguration.openshift.io"] entry is present for
that rule; also include "machineosbuilds/finalizers" in the finalizers resources
array (the list containing "controllerconfigs/finalizers",
"kubeletconfigs/finalizers", etc.) so MachineConfigNodes, MachineConfigs,
MachineOSBuilds Delete() calls and MachineOSBuilds.UpdateStatus() have the
required permissions.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2d3a6fd-b551-4542-956a-a6055fc2c9af
📒 Files selected for processing (4)
manifests/machineconfigcontroller/clusterrole.yamlmanifests/machineconfigcontroller/configmaps-role-target.yamlmanifests/machineconfigcontroller/configmaps-rolebinding-target.yamlpkg/operator/sync.go
…n MCC ClusterRole & scope configmap access to target namespace via Role instead of ClusterRole Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
isabella-janssen
force-pushed
the
ocpbugs-15157
branch
from
e963398 to
091180b
Compare
|
/test all |
|
/test e2e-gcp-op-ocl-part1 |
|
@isabella-janssen: 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. |
|
/retest-required |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-1of2 periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-2of2 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@isabella-janssen: trigger 8 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/12c767a0-4a3b-11f1-8d1f-40bcd12b6d31-0 |
isabella-janssen
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@isabella-janssen: This pull request references Jira Issue OCPBUGS-15157, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/retest |
1 similar comment
|
/retest |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-1of2 periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-2of2 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@isabella-janssen: trigger 8 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6bd7ac00-4d43-11f1-8e6e-00ce6a3901d2-0 |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.22-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@isabella-janssen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/69898960-4d5b-11f1-9ee0-f58eaf1aac78-0 |
|
/test e2e-gcp-op-part1 |
2 participants
Closes: OCPBUGS-15157
- What I did
This replaces the wildcard permissions in the MCC ClusterRole with explicit verbs and resources. Note that this PR was written with the assistance of Claude.
- How to verify it
All tests should continue passing.
- Description for the changelog
OCPBUGS-15157: Replace wildcard permissions with explicit verbs and resources in MCC ClusterRole
Summary by CodeRabbit