fix: prevent cross-namespace secret read via DmsAPIKeySecretRef - #341
fix: prevent cross-namespace secret read via DmsAPIKeySecretRef#341anispate wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (7)
WalkthroughThe reconciler now loads the Dead Mans Snitch API key secret from the operator namespace instead of the namespace in ChangesOperator namespace secret lookup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anispate 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.
🧹 Nitpick comments (1)
controllers/deadmanssnitchintegration/deadmanssnitchintegration_controller_test.go (1)
826-829: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the not-found failure mode
assert.Erroris too broad here; adderrors.IsNotFound(reconcileErr)so the test only passes when the API key secret lookup fails inconfig.OperatorNamespace.🤖 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 `@controllers/deadmanssnitchintegration/deadmanssnitchintegration_controller_test.go` around lines 826 - 829, The test around the deadmanssnitchintegration reconcile path is too broad because it only checks for any error. Update the assertion in the test that exercises the DmsAPIKeySecretRef namespace case to specifically verify the secret lookup fails with a not-found error using errors.IsNotFound(reconcileErr), alongside the existing error check, so it only passes when the secret is missing from config.OperatorNamespace.
🤖 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
`@controllers/deadmanssnitchintegration/deadmanssnitchintegration_controller_test.go`:
- Around line 826-829: The test around the deadmanssnitchintegration reconcile
path is too broad because it only checks for any error. Update the assertion in
the test that exercises the DmsAPIKeySecretRef namespace case to specifically
verify the secret lookup fails with a not-found error using
errors.IsNotFound(reconcileErr), alongside the existing error check, so it only
passes when the secret is missing from config.OperatorNamespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 34c46264-f9ae-4882-a15e-4b67650808fc
📒 Files selected for processing (2)
controllers/deadmanssnitchintegration/deadmanssnitchintegration_controller.gocontrollers/deadmanssnitchintegration/deadmanssnitchintegration_controller_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #341 +/- ##
==========================================
+ Coverage 43.28% 43.89% +0.61%
==========================================
Files 11 11
Lines 834 836 +2
==========================================
+ Hits 361 367 +6
+ Misses 424 422 -2
+ Partials 49 47 -2
🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
the
needs-rebase
The DmsAPIKeySecretRef field accepts a corev1.SecretReference with both Name and Namespace. The reconciler was passing the CR-supplied Namespace directly to utils.LoadSecretData, which executes a cluster-wide Get using the operator's broad secret-read privilege. A principal with create rights on DeadmansSnitchIntegration could point the Namespace at any namespace (e.g. hive credentials) and have the operator transmit that secret's value to api.deadmanssnitch.com — a confused-deputy / CWE-441 attack. Fix: always read the DMS API key from config.OperatorNamespace, ignoring whatever namespace the CR author specifies. Adds a regression test that places the API-key secret only in a foreign namespace and asserts that reconcile returns an error, proving the cross-namespace path is closed. Fixes: ROSAENG-61327 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
assert.Error is too broad — the reconcile could fail for unrelated reasons and the test would still pass. Use errors.IsNotFound to confirm specifically that the secret lookup in config.OperatorNamespace failed because the secret isn't there, not for any other reason. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
api/v1alpha1/deadmanssnitchintegration_types.go: Document that DmsAPIKeySecretRef.Namespace is intentionally ignored to prevent confused-deputy attacks; makes the security contract explicit at the API level. pkg/utils/secrets.go: Add LoadOperatorSecretData — a namespace-free wrapper around LoadSecretData that always reads from config.OperatorNamespace. Removes the ability for callers to accidentally supply an arbitrary namespace when reading operator credentials with the operator's elevated cluster-wide secret-read privilege. controllers/deadmanssnitchintegration/deadmanssnitchintegration_controller.go: Switch DMS API key lookup from LoadSecretData (caller-supplied namespace) to LoadOperatorSecretData (namespace fixed to config.OperatorNamespace), eliminating the confused-deputy attack surface at the call site. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
pkg/utils/secrets.go: Add ctx context.Context to LoadSecretData and LoadOperatorSecretData so context propagates correctly through the call chain (fixes contextcheck lint violation). controllers/.../deadmanssnitchintegration_controller.go: Pass reconciler ctx to LoadOperatorSecretData. deploy/crds/..., deploy_pko/...: Commit CRD YAML regenerated by make generate — updates dmsAPIKeySecretRef description to reflect that Namespace is intentionally ignored. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
acc957e to
a8d15e8
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
The agentic-sdlc-check pipeline (ROSA-730) validates that CLAUDE.md contains build/test commands and architecture sections directly in the file. The previous content was just '@AGENTS.md' (a Claude Code include directive not readable by shell scripts). Inline the content from AGENTS.md so both Claude Code and SDLC tooling can find the required sections. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@anispate: 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. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
2 participants
Summary
Fixes the confused-deputy vulnerability reported in ROSAENG-61327.
`DeadmansSnitchIntegration.Spec.DmsAPIKeySecretRef` is a `corev1.SecretReference` (Name + Namespace). The reconciler was passing the CR-supplied `Namespace` directly to `utils.LoadSecretData`, which runs a cluster-wide `Get` using the operator's broad secret-read privilege. A principal with `create` on `DeadmansSnitchIntegration` could set `DmsAPIKeySecretRef.Namespace` to any namespace (e.g. hive credentials, cloud-provider creds) and have the operator transmit that secret's value to `api.deadmanssnitch.com` (CWE-441, CWE-639, CWE-200 — CVSS 7.7).
Changes
Ticket remediation checklist
Test plan
Fixes: ROSAENG-61327
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation