OLS-3886 Tighten ClusterRole RBAC to prevent privilege escalation - #1954
Open
xrajesh wants to merge 1 commit into
Open
OLS-3886 Tighten ClusterRole RBAC to prevent privilege escalation#1954xrajesh wants to merge 1 commit into
xrajesh wants to merge 1 commit into
Conversation
|
[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 |
📝 WalkthroughWalkthroughThe controller RBAC markers now restrict Secret and NetworkPolicy access by namespace. Named ClusterRole, ClusterRoleBinding, and RoleBinding permissions use separate create rules. New tests validate the generated RBAC YAML. ChangesRBAC permission scoping
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to This PR narrows the operator’s permissions to reduce privilege escalation, but the current tests can still pass if unauthorized resource names or extra verbs are granted. That creates a concrete risk that a future RBAC regression could restore broader access without failing CI; the PR is not merge-ready until the assertions are tightened or the risk is explicitly accepted. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@internal/controller/olsconfig_controller.go`:
- Line 126: The RBAC rules for managed ClusterRole, ClusterRoleBinding, and the
named Secret must support the operations performed by SetupWithManager(),
including unfiltered Owns() cache list/watch and controller Create calls. Remove
resourceNames restrictions or replace the watches/API access with matching
name-filtered behavior, and ensure the rules grant the required scopes and verbs
while retaining the broad Secret permissions needed by the cache.
- Line 126: Replace the cluster-scoped RBAC markers near the pull-secret rule
and alerts-adapter RoleBinding rule in the OLSConfig controller with
namespace-scoped Roles and RoleBindings (or the repository’s equivalent
installation mechanism): grant pull-secret access only in openshift-config and
alerts-adapter access only in openshift-monitoring. Do not rely on resourceNames
to scope namespaces, and provision create permissions through the
namespace-scoped resources rather than ClusterRole rules.
- Around line 132-134: Update the clusterrolebindings RBAC marker near the
OLSConfig controller to include lightspeed-agentic-alerts-adapter-proposals in
resourceNames, then regenerate the RBAC manifests so manager-role includes the
permission. Leave the namespaced roles and rolebindings rule unchanged.
- Around line 131-134: The RBAC markers around the controller’s ClusterRole and
ClusterRoleBinding rules incorrectly use resourceNames with create, preventing
top-level creation. Replace this authorization design with a supported bootstrap
or pre-provisioning approach, then update the generated role.yaml and OLM CSV
permissions consistently so managed resource creation succeeds without
unrestricted masking access.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1bb34ed7-a61e-4f12-b05a-650c26188879
⛔ Files ignored due to path filters (1)
config/rbac/role.yamlis excluded by!config/rbac/role.yaml
📒 Files selected for processing (1)
internal/controller/olsconfig_controller.go
Comment on lines
+132
to
+134
| // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles,resourceNames=lightspeed-app-server-sar-role;lightspeed-agentic-alerts-adapter-agenticruns;lightspeed-agentic-alerts-adapter-proposals,verbs=get;list;create;update;delete;watch | ||
| // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings,resourceNames=lightspeed-app-server-sar-role-binding;lightspeed-agentic-alerts-adapter-agenticruns,verbs=get;list;create;update;delete;watch | ||
| // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,resourceNames=lightspeed-agentic-alerts-adapter-alertmanager,verbs=get;list;create;update;delete;watch |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'removeLegacy|AlertsAdapter(LegacyProposalsClusterRoleName|ConfigRoleBindingName|ConfigRoleName)|resources=clusterrolebindings|resources=rolebindings|resources=roles' \
internal/controllerRepository: openshift/lightspeed-operator
Length of output: 12953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- project specification entry point ---'
if [ -f .ai/spec/README.md ]; then
sed -n '1,180p' .ai/spec/README.md
fi
printf '%s\n' '--- controller RBAC markers ---'
sed -n '118,148p' internal/controller/olsconfig_controller.go
printf '%s\n' '--- alerts-adapter cleanup and reconciliation ---'
sed -n '1,70p' internal/controller/alertsadapter/reconciler.go
sed -n '300,355p' internal/controller/alertsadapter/reconciler.go
sed -n '455,510p' internal/controller/alertsadapter/reconciler.go
printf '%s\n' '--- generated RBAC and bundle references ---'
rg -n -C 3 \
'lightspeed-agentic-alerts-adapter-(proposals|agenticruns|config|alertmanager)|name: (lightspeed-agentic-alerts-adapter|alerts-adapter-config)' \
config bundle deploy manifests 2>/dev/null || true
printf '%s\n' '--- RBAC generation and relevant watches ---'
rg -n -C 3 \
'SetupWithManager|Owns\(|ClusterRole|ClusterRoleBinding|RoleBinding|removeLegacyProposalsClusterRBAC|AlertsAdapter' \
internal/controller config bundle Makefile 2>/dev/null | head -n 500Repository: openshift/lightspeed-operator
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- generated manager Role rules ---'
sed -n '145,215p' config/rbac/role.yaml
printf '%s\n' '--- CSV permissions for RBAC resources ---'
rg -n -C 12 \
'clusterroles|clusterrolebindings|rolebindings|lightspeed-agentic-alerts-adapter-(proposals|agenticruns|alertmanager)' \
bundle/manifests/lightspeed-operator.clusterserviceversion.yaml \
bundle/manifests/*role*.yaml 2>/dev/null || true
printf '%s\n' '--- complete alerts-adapter deletion paths ---'
rg -n -C 8 \
'func (RemoveAlertsAdapter|delete|removeLegacy)|r\.(Get|Delete)\(.*(Role|Binding|ServiceAccount|Deployment|NetworkPolicy)' \
internal/controller/alertsadapter
printf '%s\n' '--- relevant constants ---'
rg -n -C 2 \
'AlertsAdapter.*(Role|Binding|ServiceAccount|Deployment|NetworkPolicy).*Name' \
internal/controller/utils/constants.go
printf '%s\n' '--- marker-to-cleanup consistency probe ---'
python3 - <<'PY'
from pathlib import Path
import re
controller = Path("internal/controller/olsconfig_controller.go").read_text()
reconciler = Path("internal/controller/alertsadapter/reconciler.go").read_text()
generated = Path("config/rbac/role.yaml").read_text()
markers = {}
for resource, names, verbs in re.findall(
r'resources=([^,]+),resourceNames=([^,]+),verbs=([^\n]+)',
controller,
):
markers[resource] = {
"names": set(names.split(";")),
"verbs": set(verbs.split(";")),
}
cleanup = sorted(set(re.findall(
r'utils\.(AlertsAdapter[A-Za-z0-9]+Name)',
reconciler,
)))
print("cleanup constants:", cleanup)
for constant in cleanup:
value = re.search(
rf'{re.escape(constant)}\s*=\s*"([^"]+)"',
Path("internal/controller/utils/constants.go").read_text(),
)
print(constant, "=>", value.group(1) if value else "UNRESOLVED")
for resource in ("clusterroles", "clusterrolebindings", "rolebindings"):
print(resource, markers.get(resource))
PYRepository: openshift/lightspeed-operator
Length of output: 37493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- generated namespace-scoped rules ---'
rg -n -C 8 \
'namespace: openshift-lightspeed|resources:|roles|rolebindings' \
config/rbac/role.yaml | head -n 180
printf '%s\n' '--- bundle manifest inventory ---'
find bundle/manifests -maxdepth 1 -type f -printf '%f\n' | sort | rg -i \
'role|clusterrole|operator|csv|serviceaccount'
printf '%s\n' '--- bundle RBAC objects and CSV permissions ---'
for f in bundle/manifests/*; do
if rg -q 'kind: (Role|ClusterRole|RoleBinding|ClusterRoleBinding)|clusterrolebindings|clusterroles|rolebindings|roles' "$f"; then
printf '\n### %s\n' "$f"
rg -n -C 5 \
'kind: (Role|ClusterRole|RoleBinding|ClusterRoleBinding)|namespace: openshift-lightspeed|clusterrolebindings|clusterroles|rolebindings|roles|resourceNames' \
"$f" | head -n 220
fi
done
printf '%s\n' '--- legacy cleanup test coverage ---'
rg -n -C 6 \
'LegacyProposals|proposals ClusterRoleBinding|lightspeed-agentic-alerts-adapter-proposals' \
internal/controller/alertsadapter/*_test.goRepository: openshift/lightspeed-operator
Length of output: 10100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- generated RBAC object and remaining rules ---'
sed -n '1,12p' config/rbac/role.yaml
sed -n '180,270p' config/rbac/role.yaml
printf '%s\n' '--- source marker and generated-rule comparison ---'
python3 - <<'PY'
from pathlib import Path
import re
source = Path("internal/controller/olsconfig_controller.go").read_text()
generated = Path("config/rbac/role.yaml").read_text()
csv = Path("bundle/manifests/lightspeed-operator.clusterserviceversion.yaml").read_text()
for resource in ("clusterroles", "clusterrolebindings", "rolebindings"):
marker = re.search(
rf'resources={resource},resourceNames=([^,]+),verbs=([^\n]+)',
source,
)
print(f"{resource} marker:", marker.groups() if marker else None)
for name in (
"lightspeed-agentic-alerts-adapter-proposals",
"lightspeed-agentic-alerts-adapter-agenticruns",
"lightspeed-agentic-alerts-adapter-config",
"lightspeed-agentic-alerts-adapter-alertmanager",
):
print(
name,
"generated=", name in generated,
"csv=", name in csv,
)
print("generated has a roles/rolebindings rule:",
bool(re.search(r"resources:\n(?: - [^\n]+\n)* - roles\b", generated)))
PY
printf '%s\n' '--- exact legacy cleanup test references ---'
rg -n -C 5 \
'removeLegacyProposalsClusterRBAC|AlertsAdapterLegacyProposalsClusterRoleName|proposals ClusterRoleBinding' \
internal/controller/alertsadapterRepository: openshift/lightspeed-operator
Length of output: 6601
Allow cleanup of the legacy proposals ClusterRoleBinding.
removeLegacyProposalsClusterRBAC accesses lightspeed-agentic-alerts-adapter-proposals as a ClusterRoleBinding, but the generated manager-role omits this name. Add it to the clusterrolebindings resourceNames list and regenerate the RBAC manifests. Keep the namespaced roles;rolebindings rule for legacy config resources. Without this permission, cleanup returns Forbidden and alerts-adapter reconciliation fails.
🤖 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 `@internal/controller/olsconfig_controller.go` around lines 132 - 134, Update
the clusterrolebindings RBAC marker near the OLSConfig controller to include
lightspeed-agentic-alerts-adapter-proposals in resourceNames, then regenerate
the RBAC manifests so manager-role includes the permission. Leave the namespaced
roles and rolebindings rule unchanged.
xrajesh
force-pushed
the
OLS-3886/tighten-clusterrole-rbac
branch
from
August 13, 2026 03:38
c8ef065 to
d194f9a
Compare
- Move secrets and networkpolicies from ClusterRole to namespace-scoped Role with namespace=system placeholder (kustomize/OLM replaces at deploy time). - Split RBAC resource rules: create without resourceNames (Kubernetes silently denies create when resourceNames is set), get/update/delete with resourceNames pinned to operator-managed resources. - Add static RBAC validation tests that parse role.yaml and enforce these invariants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
xrajesh
force-pushed
the
OLS-3886/tighten-clusterrole-rbac
branch
from
August 13, 2026 04:05
d194f9a to
368684f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/rbac/role_yaml_test.go`:
- Around line 39-48: Strengthen TestClusterRoleNoUnrestrictedSecrets in
internal/rbac/role_yaml_test.go:39-48 to require exactly one Secret resource
name, pull-secret, and fail any other Secret name or unrestricted rule. In
internal/rbac/role_yaml_test.go:88-120, add the managed RoleBinding name to the
allowlist and validate every rolebindings resource name against it, rejecting
unauthorized names.
- Around line 130-150: The TestRoleContainsSecretsAndNetworkPolicies test only
checks resource presence; update it to collect and compare each resource’s exact
verb set against the expected namespaced Role contract, requiring
deletecollection for secrets and excluding it for networkpolicies. Ensure
missing, extra, or incorrect verbs fail the test.
- Around line 14-150: Convert the manifest tests in loadRoleYAML and the
TestClusterRole*/TestRole* functions from testing.T assertions to Ginkgo/Gomega,
adding the package suite setup and structuring each test as Ginkgo specs with
appropriate matchers. Keep the existing YAML loading and RBAC validation
coverage unchanged, omit shared OLSConfig fixtures, and verify the suite through
make test.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5ca53683-3d58-438a-9c71-d32bbbc08303
⛔ Files ignored due to path filters (1)
config/rbac/role.yamlis excluded by!config/rbac/role.yaml
📒 Files selected for processing (2)
internal/controller/olsconfig_controller.gointernal/rbac/role_yaml_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/olsconfig_controller.go
Comment on lines
+14
to
+150
| func loadRoleYAML(t *testing.T) (rbacv1.ClusterRole, rbacv1.Role) { | ||
| t.Helper() | ||
| _, thisFile, _, _ := runtime.Caller(0) | ||
| roleFile := filepath.Join(filepath.Dir(thisFile), "..", "..", "config", "rbac", "role.yaml") | ||
|
|
||
| data, err := os.ReadFile(roleFile) | ||
| if err != nil { | ||
| t.Fatalf("reading role.yaml: %v", err) | ||
| } | ||
|
|
||
| decoder := yaml.NewYAMLOrJSONDecoder(bytes.NewReader(data), 4096) | ||
|
|
||
| var clusterRole rbacv1.ClusterRole | ||
| if err := decoder.Decode(&clusterRole); err != nil { | ||
| t.Fatalf("decoding ClusterRole: %v", err) | ||
| } | ||
|
|
||
| var role rbacv1.Role | ||
| if err := decoder.Decode(&role); err != nil { | ||
| t.Fatalf("decoding Role: %v", err) | ||
| } | ||
|
|
||
| return clusterRole, role | ||
| } | ||
|
|
||
| func TestClusterRoleNoUnrestrictedSecrets(t *testing.T) { | ||
| clusterRole, _ := loadRoleYAML(t) | ||
| for _, rule := range clusterRole.Rules { | ||
| for _, res := range rule.Resources { | ||
| if res == "secrets" && len(rule.ResourceNames) == 0 { | ||
| t.Error("ClusterRole grants secrets access without resourceNames restriction") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestClusterRoleNoClusterWideNetworkPolicies(t *testing.T) { | ||
| clusterRole, _ := loadRoleYAML(t) | ||
| for _, rule := range clusterRole.Rules { | ||
| for _, res := range rule.Resources { | ||
| if res == "networkpolicies" { | ||
| t.Error("ClusterRole grants networkpolicies access — should be in namespaced Role") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestClusterRoleNoCreateWithResourceNames(t *testing.T) { | ||
| rbacResources := map[string]bool{ | ||
| "clusterroles": true, | ||
| "clusterrolebindings": true, | ||
| "rolebindings": true, | ||
| "roles": true, | ||
| } | ||
| clusterRole, _ := loadRoleYAML(t) | ||
| for _, rule := range clusterRole.Rules { | ||
| hasRBACResource := false | ||
| for _, res := range rule.Resources { | ||
| if rbacResources[res] { | ||
| hasRBACResource = true | ||
| break | ||
| } | ||
| } | ||
| if !hasRBACResource || len(rule.ResourceNames) == 0 { | ||
| continue | ||
| } | ||
| for _, verb := range rule.Verbs { | ||
| if verb == "create" { | ||
| t.Errorf("create verb combined with resourceNames %v is silently denied by Kubernetes RBAC", rule.ResourceNames) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestClusterRoleRBACResourceNamesAllowed(t *testing.T) { | ||
| allowedClusterRoles := map[string]bool{ | ||
| "lightspeed-app-server-sar-role": true, | ||
| "lightspeed-agentic-alerts-adapter-agenticruns": true, | ||
| "lightspeed-agentic-alerts-adapter-proposals": true, | ||
| } | ||
| allowedCRBs := map[string]bool{ | ||
| "lightspeed-app-server-sar-role-binding": true, | ||
| "lightspeed-agentic-alerts-adapter-agenticruns": true, | ||
| } | ||
|
|
||
| clusterRole, _ := loadRoleYAML(t) | ||
| for _, rule := range clusterRole.Rules { | ||
| if len(rule.ResourceNames) == 0 { | ||
| continue | ||
| } | ||
| for _, res := range rule.Resources { | ||
| var allowed map[string]bool | ||
| switch res { | ||
| case "clusterroles": | ||
| allowed = allowedClusterRoles | ||
| case "clusterrolebindings": | ||
| allowed = allowedCRBs | ||
| default: | ||
| continue | ||
| } | ||
| for _, name := range rule.ResourceNames { | ||
| if !allowed[name] { | ||
| t.Errorf("ClusterRole grants access to unexpected %s: %s", res, name) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestRoleUsesSystemNamespace(t *testing.T) { | ||
| _, role := loadRoleYAML(t) | ||
| if role.Namespace != "system" { | ||
| t.Errorf("Role namespace = %q, want 'system' placeholder", role.Namespace) | ||
| } | ||
| } | ||
|
|
||
| func TestRoleContainsSecretsAndNetworkPolicies(t *testing.T) { | ||
| _, role := loadRoleYAML(t) | ||
| hasSecrets := false | ||
| hasNP := false | ||
| for _, rule := range role.Rules { | ||
| for _, res := range rule.Resources { | ||
| if res == "secrets" { | ||
| hasSecrets = true | ||
| } | ||
| if res == "networkpolicies" { | ||
| hasNP = true | ||
| } | ||
| } | ||
| } | ||
| if !hasSecrets { | ||
| t.Error("namespaced Role must include secrets access") | ||
| } | ||
| if !hasNP { | ||
| t.Error("namespaced Role must include networkpolicies access") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'github\.com/onsi/(ginkgo|gomega)' go.mod
fd -a 'suite_test.go' . -x sed -n '1,180p' {}Repository: openshift/lightspeed-operator
Length of output: 41005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- role_yaml_test.go ---'
cat -n internal/rbac/role_yaml_test.go
printf '%s\n' '--- rbac test files ---'
git ls-files 'internal/rbac/*_test.go' | while read -r f; do
printf '\n[%s]\n' "$f"
sed -n '1,80p' "$f"
done
printf '%s\n' '--- test targets ---'
rg -n -A12 -B3 '^(test|test-e2e):|ginkgo|go test' Makefile makefile* 2>/dev/null || true
printf '%s\n' '--- repository test conventions ---'
rg -n 'func Test[A-Za-z0-9_]+\(t \*testing\.T\)|RegisterFailHandler|RunSpecs' --glob '*_test.go' internal/rbac internal/controller | head -120Repository: openshift/lightspeed-operator
Length of output: 14919
Convert internal/rbac/role_yaml_test.go to Ginkgo/Gomega.
Add the package suite and replace testing.T assertions with Gomega matchers. Run the tests through make test. Shared OLSConfig fixtures are not needed for manifest-only tests.
🤖 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 `@internal/rbac/role_yaml_test.go` around lines 14 - 150, Convert the manifest
tests in loadRoleYAML and the TestClusterRole*/TestRole* functions from
testing.T assertions to Ginkgo/Gomega, adding the package suite setup and
structuring each test as Ginkgo specs with appropriate matchers. Keep the
existing YAML loading and RBAC validation coverage unchanged, omit shared
OLSConfig fixtures, and verify the suite through make test.
Source: Coding guidelines
Comment on lines
+39
to
+48
| func TestClusterRoleNoUnrestrictedSecrets(t *testing.T) { | ||
| clusterRole, _ := loadRoleYAML(t) | ||
| for _, rule := range clusterRole.Rules { | ||
| for _, res := range rule.Resources { | ||
| if res == "secrets" && len(rule.ResourceNames) == 0 { | ||
| t.Error("ClusterRole grants secrets access without resourceNames restriction") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce complete RBAC resource-name allowlists.
The tests accept unauthorized named resources. A ClusterRole can grant a Secret other than pull-secret, or a named rolebindings permission outside the managed RoleBinding, without failing.
internal/rbac/role_yaml_test.go#L39-L48: require a Secret rule for exactlypull-secretand reject every other Secret resource name.internal/rbac/role_yaml_test.go#L88-L120: add the managed RoleBinding name to an allowlist and validaterolebindingsresource names.
📍 Affects 1 file
internal/rbac/role_yaml_test.go#L39-L48(this comment)internal/rbac/role_yaml_test.go#L88-L120
🤖 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 `@internal/rbac/role_yaml_test.go` around lines 39 - 48, Strengthen
TestClusterRoleNoUnrestrictedSecrets in internal/rbac/role_yaml_test.go:39-48 to
require exactly one Secret resource name, pull-secret, and fail any other Secret
name or unrestricted rule. In internal/rbac/role_yaml_test.go:88-120, add the
managed RoleBinding name to the allowlist and validate every rolebindings
resource name against it, rejecting unauthorized names.
Comment on lines
+130
to
+150
| func TestRoleContainsSecretsAndNetworkPolicies(t *testing.T) { | ||
| _, role := loadRoleYAML(t) | ||
| hasSecrets := false | ||
| hasNP := false | ||
| for _, rule := range role.Rules { | ||
| for _, res := range rule.Resources { | ||
| if res == "secrets" { | ||
| hasSecrets = true | ||
| } | ||
| if res == "networkpolicies" { | ||
| hasNP = true | ||
| } | ||
| } | ||
| } | ||
| if !hasSecrets { | ||
| t.Error("namespaced Role must include secrets access") | ||
| } | ||
| if !hasNP { | ||
| t.Error("namespaced Role must include networkpolicies access") | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Assert the exact namespaced Role verb sets.
The test passes when the Role grants only get, or grants extra verbs. Both cases violate the expected RBAC contract. Assert exact verbs for secrets and networkpolicies, including deletecollection only for secrets.
🤖 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 `@internal/rbac/role_yaml_test.go` around lines 130 - 150, The
TestRoleContainsSecretsAndNetworkPolicies test only checks resource presence;
update it to collect and compare each resource’s exact verb set against the
expected namespaced Role contract, requiring deletecollection for secrets and
excluding it for networkpolicies. Ensure missing, extra, or incorrect verbs fail
the test.
|
@xrajesh: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
create/delete/deletecollection/get/list/patch/update/watch) from ClusterRole to namespace-scoped Role (openshift-lightspeed). Onlypull-secretread access (pinned byresourceNames) remains cluster-scoped for telemetry.clusterroles,clusterrolebindings, androlebindingswrite access to specificresourceNamesthe operator manages (lightspeed-app-server-sar-role,lightspeed-agentic-alerts-adapter-*), eliminating the ability to bindcluster-adminor create arbitrary roles.openshift-lightspeedonly.Motivation
A compromised operator ServiceAccount token could previously:
cluster-adminfor full cluster takeoverTest plan
make manifestsregeneratesconfig/rbac/role.yamlcorrectlymake test— all controller tests passpull-secretfromopenshift-configopenshift-lightspeedFixes: https://redhat.atlassian.net/browse/OLS-3886
🤖 Generated with Claude Code
Summary by CodeRabbit
Security
openshift-lightspeednamespace.Tests