Improvements#74
Conversation
WalkthroughThis PR migrates the pf-status-relay-operator from cluster-wide RBAC to namespace-scoped RBAC, removes HostPID access from managed DaemonSet pods, and updates both bundle and stable ClusterServiceVersion manifests to consolidate RBAC rules in the deployment permissions section. ChangesRBAC Namespace Scoping and Pod Security Hardening
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (14 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: zeeke 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
There was a problem hiding this comment.
I think that commit cannot enter if we want to have #73 wdyt?
There was a problem hiding this comment.
I suppose you're referring to the ClusterRole -> Role change for the manager-role resource.
Whatever the installation method is, the operator will read pflacpmonitors objects from its own namespace, right?
And it will create/update/delete daemonsets in the namespace the operator is running.
So, this should be compatible with
There was a problem hiding this comment.
he operator will read pflacpmonitors objects from its own namespace, right?
Probably that is the right approach, but I was under the impression that moving to "AllNamespaces" would actually mean the opposite.
The pf-status-relay DaemonSet runs with privileged: true + hostPID: true + hostNetwork: true. hostPID is not needed by the agent: netlink and sysfs VF manipulation work without sharing the host PID namespace. Dropping it removes the agent's visibility into every host process (/proc/<pid>/environ and cmdline can leak secrets of other workloads, and any process can be ptraced or signalled) and removes the cheapest container-to-host breakout (nsenter -t 1 -a into a host root shell). Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
The controller SA held a cluster-wide ClusterRole with full CRUD on apps/daemonsets: a compromised operator pod (or stolen SA token) could create arbitrary DaemonSets in any namespace. Add namespace=system to the kubebuilder RBAC markers so controller-gen emits a namespaced Role instead of a ClusterRole, replace the ClusterRoleBinding with a RoleBinding, and regenerate the bundle so the CSV ships these rules under permissions instead of clusterPermissions. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
zeeke
force-pushed
the
fix/security-audit-high-findings
branch
from
328a0cc to
7dea6e9
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundle/manifests/pf-status-relay-operator.clusterserviceversion.yaml (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBundle directory contains generated files that should not be committed.
As per coding guidelines, the
bundle/directory is regenerated bymake generate && make manifestsand should not be committed. The changes here will be overwritten during bundle regeneration and may cause merge conflicts.Consider reverting this file and documenting the expected bundle output in the PR description instead.
As per coding guidelines: "The bundle regeneration modifies
bundle/directory—do not commit generated bundle files."🤖 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 `@bundle/manifests/pf-status-relay-operator.clusterserviceversion.yaml` at line 1, The commit includes generated bundle files (e.g., bundle/manifests/pf-status-relay-operator.clusterserviceversion.yaml) that should not be committed; revert the changes to any files under bundle/, remove them from the commit (or reset them to the state in main), and ensure bundle artifacts are excluded from future commits (follow repo convention: do not add bundle/ to commits or add a note to the PR instead). Update the PR description with the expected bundle output if needed and run make generate && make manifests locally to regenerate the bundle rather than committing generated files.Source: Coding guidelines
🤖 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 `@config/rbac/role_binding.yaml`:
- Around line 2-7: The RoleBinding metadata is missing an explicit namespace;
add "namespace: system" under the metadata of the RoleBinding (the resource with
name "manager-rolebinding") so it matches the Role's namespace and avoids
deployment inconsistencies when applied directly.
---
Outside diff comments:
In `@bundle/manifests/pf-status-relay-operator.clusterserviceversion.yaml`:
- Line 1: The commit includes generated bundle files (e.g.,
bundle/manifests/pf-status-relay-operator.clusterserviceversion.yaml) that
should not be committed; revert the changes to any files under bundle/, remove
them from the commit (or reset them to the state in main), and ensure bundle
artifacts are excluded from future commits (follow repo convention: do not add
bundle/ to commits or add a note to the PR instead). Update the PR description
with the expected bundle output if needed and run make generate && make
manifests locally to regenerate the bundle rather than committing generated
files.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: c11b7d21-4236-4433-947d-3233fd925b59
📒 Files selected for processing (5)
bundle/manifests/pf-status-relay-operator.clusterserviceversion.yamlconfig/rbac/role.yamlconfig/rbac/role_binding.yamlinternal/controller/pflacpmonitor_controller.gomanifests/stable/pf-status-relay-operator.clusterserviceversion.yaml
| kind: RoleBinding | ||
| metadata: | ||
| labels: | ||
| app.kubernetes.io/name: pf-status-relay-operator | ||
| app.kubernetes.io/managed-by: kustomize | ||
| name: manager-rolebinding |
There was a problem hiding this comment.
Add namespace: system to RoleBinding metadata for consistency.
The Role in role.yaml explicitly declares namespace: system in its metadata, but this RoleBinding does not. While kustomize's namespace transformer may add it during build, explicitly declaring the namespace ensures both resources are consistent and avoids potential deployment issues if applied directly.
Suggested fix
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
labels:
app.kubernetes.io/name: pf-status-relay-operator
app.kubernetes.io/managed-by: kustomize
name: manager-rolebinding
+ namespace: system🤖 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 `@config/rbac/role_binding.yaml` around lines 2 - 7, The RoleBinding metadata
is missing an explicit namespace; add "namespace: system" under the metadata of
the RoleBinding (the resource with name "manager-rolebinding") so it matches the
Role's namespace and avoids deployment inconsistencies when applied directly.
|
@zeeke: all tests passed! 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. |
Can you rebase? I see some config files like certmanager that were removed added back (probably the merge will be okay but I prefer to be sure and that the end status will not generate file difference) |
|
Two notes
|
|
/lgtm |
|
/verified later by @gkopels |
|
@zeeke: Only users can be targets for the 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. |
|
/verified later @gkopels |
openshift-ci-robot
added
verified-later
verified
|
@zeeke: This PR has been marked to be verified later by 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. |
PR openshift#74 converted all RBAC to namespace-scoped Role/RoleBinding. TLS compliance requires cluster-scoped access to tokenreviews, subjectaccessreviews, and apiservers.config.openshift.io. Add a dedicated ClusterRoleBinding so operator-sdk populates clusterPermissions in the CSV. Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Summary by CodeRabbit
Refactor
Chores