fix: prevent cross-namespace secret read via DmsAPIKeySecretRef#342
fix: prevent cross-namespace secret read via DmsAPIKeySecretRef#342anispate wants to merge 2 commits into
Conversation
DeadmansSnitchIntegration.Spec.DmsAPIKeySecretRef is a SecretReference with both Name and Namespace. The reconciler was passing the CR-supplied Namespace directly to utils.LoadSecretData, which runs a cluster-wide Get using the operator's elevated secret-read privilege. Any principal with create on DeadmansSnitchIntegration could point Namespace at any namespace and have the operator transmit that secret to api.deadmanssnitch.com (CWE-441 confused-deputy, CWE-639, CWE-200 — CVSS 7.7). Fix: always pass config.OperatorNamespace to LoadSecretData, ignoring whatever namespace the CR author specifies. All production DMSI CRs already set namespace: deadmanssnitch-operator so there is no behaviour change for legitimate use. Adds TestCrossNamespaceSecretRefRejected: secret only present in a foreign namespace, asserts reconcile returns NotFound, proving the cross-namespace read path is closed. Fixes: ROSAENG-61327 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR restricts the DeadmansSnitchIntegration controller to always read the DMS API key secret from the operator's own namespace instead of the namespace specified in the CR spec, addressing a confused-deputy risk, and adds a corresponding regression test. A new CLAUDE.md documentation file is also added. ChangesCross-namespace secret restriction
Repository documentation
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant OperatorNamespaceSecrets
participant ForeignNamespaceSecrets
Reconciler->>OperatorNamespaceSecrets: LoadSecretData(config.OperatorNamespace)
alt secret found in operator namespace
OperatorNamespaceSecrets-->>Reconciler: Return secret data
else secret only in foreign namespace
OperatorNamespaceSecrets-->>Reconciler: NotFound error
Reconciler-->>Reconciler: Fail reconciliation
end
Suggested reviewers: 🚥 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #342 +/- ##
==========================================
+ Coverage 43.28% 43.76% +0.47%
==========================================
Files 11 11
Lines 834 834
==========================================
+ Hits 361 365 +4
+ Misses 424 422 -2
+ Partials 49 47 -2
🚀 New features to boost your workflow:
|
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>
![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.go (1)
101-106: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCorrect fix for the confused-deputy vulnerability.
Forcing the lookup to always use
config.OperatorNamespaceinstead of the CR-supplieddmsi.Spec.DmsAPIKeySecretRef.Namespacecorrectly closes the cross-namespace secret read. LoadSecretData fetches a Secret by the provided namespace/name and returns the specified data key, so passing the fixed operator namespace guarantees a CR author can no longer point at an arbitrary namespace to exfiltrate secrets.One residual concern:
DmsAPIKeySecretRef.Namespaceremains a settable field on the CRD spec but is now silently ignored. A CR author who sets it to something other than the operator namespace will get a confusingNotFounderror with no indication that the field is disregarded. Consider updating the field's godoc comment (inapi/v1alpha1/deadmanssnitchintegration_types.go) to state thatNamespaceis ignored/must match the operator namespace, and/or logging a warning when a mismatch is detected, to aid future debugging.📝 Example: clarify ignored field + warn on mismatch
// Always read from the operator's own namespace regardless of what DmsAPIKeySecretRef.Namespace // says. Using the CR-supplied namespace would allow any principal with create on // DeadmansSnitchIntegration to exfiltrate arbitrary Secrets via the operator's // cluster-wide secret-read privilege (CWE-441 confused-deputy, ROSAENG-61327). + if dmsi.Spec.DmsAPIKeySecretRef.Namespace != "" && dmsi.Spec.DmsAPIKeySecretRef.Namespace != config.OperatorNamespace { + reqLogger.Info("DmsAPIKeySecretRef.Namespace is ignored; secret is always read from the operator namespace", + "specifiedNamespace", dmsi.Spec.DmsAPIKeySecretRef.Namespace, "operatorNamespace", config.OperatorNamespace) + } dmsAPIKey, err := utils.LoadSecretData(r.Client, dmsi.Spec.DmsAPIKeySecretRef.Name, config.OperatorNamespace, deadMansSnitchAPISecretKey)🤖 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.go` around lines 101 - 106, The secret lookup fix in the deadmanssnitchintegration controller should keep using config.OperatorNamespace in LoadSecretData, but the CR field DmsAPIKeySecretRef.Namespace is now silently ignored. Update the DmsAPIKeySecretRef godoc in deadmanssnitchintegration_types.go to clearly say Namespace is ignored/must match the operator namespace, and consider adding a warning in reconcile when dmsi.Spec.DmsAPIKeySecretRef.Namespace differs so users get a clear signal instead of a confusing NotFound error.
🤖 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.go`:
- Around line 101-106: The secret lookup fix in the deadmanssnitchintegration
controller should keep using config.OperatorNamespace in LoadSecretData, but the
CR field DmsAPIKeySecretRef.Namespace is now silently ignored. Update the
DmsAPIKeySecretRef godoc in deadmanssnitchintegration_types.go to clearly say
Namespace is ignored/must match the operator namespace, and consider adding a
warning in reconcile when dmsi.Spec.DmsAPIKeySecretRef.Namespace differs so
users get a clear signal instead of a confusing NotFound error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8ef58ebd-f306-4805-91a9-5c448527bc7e
📒 Files selected for processing (3)
CLAUDE.mdcontrollers/deadmanssnitchintegration/deadmanssnitchintegration_controller.gocontrollers/deadmanssnitchintegration/deadmanssnitchintegration_controller_test.go
|
@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. |
| @@ -1 +1,91 @@ | |||
| @AGENTS.md | |||
|
|
|||
There was a problem hiding this comment.
The flow this repo already uses is to put all context into AGENTS.md and then use this @AGENTS.md to inform Claude Code to consume AGENTS.md.
The information you've added here to CLAUDE.md is duplicating the information in AGENTS.md and will only cause toil to maintain and additional token use.
Suggestion is to remove this whole change to CLAUDE.md, its unnecessary.
|
The test and fix look correct. I verified the test fails without the fix accurately and succeeds with the fix. The change to |
3 participants
Summary
Fixes ROSAENG-61327 with a minimal, targeted change.
DmsAPIKeySecretRefis acorev1.SecretReference(Name + Namespace). The reconciler was passing the CR-suppliedNamespacedirectly toutils.LoadSecretData, which uses the operator's cluster-wide secret-read privilege. Any principal withcreateonDeadmansSnitchIntegrationcould setDmsAPIKeySecretRef.Namespaceto point at any namespace (e.g. hive credentials, cloud-provider creds) and have the operator transmit that secret toapi.deadmanssnitch.com— a confused-deputy attack (CWE-441, CWE-639, CWE-200, CVSS 7.7).Fix: one line in the controller — pass
config.OperatorNamespacetoLoadSecretDatainstead of the CR-supplied namespace. All production DMSI CRs already setnamespace: deadmanssnitch-operator, so there is no behaviour change for legitimate use.Changes
controllers/deadmanssnitchintegration/deadmanssnitchintegration_controller.go— useconfig.OperatorNamespace(1 line)controllers/deadmanssnitchintegration/deadmanssnitchintegration_controller_test.go— addTestCrossNamespaceSecretRefRejected: places the secret only in a foreign namespace, asserts reconcile returnsNotFound, proving the cross-namespace read path is closedTest plan
go test ./controllers/deadmanssnitchintegration/...— all existing tests pass, new security regression test passesdmsAPIKeySecretRef.namespace: deadmanssnitch-operator)Fixes: ROSAENG-61327
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation