Repository navigation
fix(helm): separate bundled dependency credential keys - #782
Conversation
|
@coderabbitai full review Please verify the pinned dependency consumer/name/key/namespace gates and shared worker/gateway/MCP/upstream alias checks, including Prometheus certgen |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 1 files. (4 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: separating bundled Helm dependency credential keys. Linked Issues check Issue #767requires active, same-namespace bundled dependency credentials to enter the shared Secret-key inventory. The change adds Grafana SMTP/LDAP, KEDA, Prometheus admission, trust-manager, Loki g…Out of Scope Changes check The helper changes, render tests, CI routing, README updates, and values documentation support issue #767. The PR keeps arbitrary dependency environment, volume, provider, whole-Secret, external certi…Full details: Linked Issues check
-
Explanation
Issue
#767requires active, same-namespace bundled dependency credentials to enter the shared Secret-key inventory. The change adds Grafana SMTP/LDAP, KEDA, Prometheus admission, trust-manager, Loki gateway, and Loki MinIO references. The real-render tests verify consumers, names, keys, namespaces, enabled contexts, worker/gateway/extra-env/upstream/MCP aliases, generated credentials, inactive cases, cross-namespace cases, and different-key controls. The CI workflow runs the dedicated suite in the Helm lane.
Full details: Out of Scope Changes check
-
Explanation
The helper changes, render tests, CI routing, README updates, and values documentation support issue
#767. The PR keeps arbitrary dependency environment, volume, provider, whole-Secret, external certificate, and enterprise configuration outside the finite inventory. The supplied evidence assigns those cases to#780and the trust-manager namespace mismatch to#781. No dependency versions, authentication defaults, deployments, or live settings change.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 1 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Independent adversarial review of the original PR head, posted verbatim below. F1 is being addressed with a null-safe test inspector, a rendered enterprise-GEL exclusion control and generated-token cases. The final updated head will receive a separate exact-head verification before merge. Agent-generated adversarial security review Reviewed head: Whole-PR range: I did not author the change. I reviewed the whole diff and pinned dependency source, independently executed the 326 new test assertions against copied charts, and ran additional generated-token/enterprise probes and 25 mutations. Tests began on the stable worktree snapshot; afterward all five source SHA-256 values were checked against this exact immutable commit and matched. The hashes are recorded in Scope and source verificationThe extension stays within the existing
Detailed pinned TLS gates, names, namespaces, root Issuers, and proposed edge cases are retained in Abuse cases and independent tests
Total: 326 assertions/test cases passed. All were executed independently here; the author's larger existing-suite/lint results are separate evidence. Mutation checks also passed:
Security effects considered include accidental privilege expansion by credential alias, foreign credential presentation upstream, generated and operator-managed names, child naming/helper context, disabled consumers, cross-namespace false positives, alternate key names and certificate modes, CA Issuer consumers, and jobs reading keys without TLS serving. This is static deployment validation; replay, expiry, authentication middleware behavior, and tenant authorization are not changed by this diff. FindingF1. Nonblocking: the test consumer inspector rejects a valid nullable initContainers renderWhere: Reproduction: render The guard itself matches the enterprise exclusion and independently rendered correctly. The defect affects test robustness and prevents using the common consumer inspector for this relevant negative control; it does not change deployed manifests or create an authentication bypass. The current 326 cases remain green because they do not exercise that combination through the inspector. Recommended fix: normalize nullable container lists with One additional test-quality limit is informational: unrestricted Secret-volume fixtures prove that a Secret is mounted, not which filenames an application reads. Here, explicit serving-argument source checks and the positive/negative certificate-filename cases establish the intended pinned contract. Future dependency upgrades still require source review rather than treating the wildcard fixture alone as proof of per-key correctness. Sibling coverage and bounded exclusionsThe structured inventory covers the known #767 consumers: Grafana SMTP/LDAP, KEDA serving/CA, Prometheus serving/CA/patch/root signer, trust-manager serving, Loki NGINX basic auth, and optional MinIO root credentials. The scan command and full per-file counts are saved at The README explicitly excludes KEDA No new blocking security defect was found in the implementation. The only new finding is the bounded test-helper issue above. No live Kubernetes upgrade, Secret content lookup, external endpoint, hosted service, or release was exercised. A new immutable head after addressing F1 requires a fresh verification of that delta; this report applies exactly to Blocking findings: 0 |
|
Agent-generated adversarial security verification Verified head: Whole-PR range: Follow-up delta: I independently verified every reviewed source file against F1 dispositionRESOLVED. The consumer inspector now normalizes nullable The added absent-consumer checks correctly exclude three key-mode cases that still mount the same whole Secret for other known filenames. Those cases retain their explicit alternate-key controls. A whole-Secret mount does not establish which filenames an executable reads; the pinned-source analysis remains part of the contract. The test delta also adds generated worker-token and generated gateway-token aliases, including the gateway's mode-none preparation step. Each is rejected when a dependency consumer points at the generated Secret's Independent execution and mutation evidenceThe original review independently executed 326 suite assertions against For this follow-up,
All passed, bringing independently executed suite assertions to 359 across the two reviews. The author's single full-suite run also reports 359 passing tests; that is separate evidence. Five follow-up mutation runs passed:
These are 25 original plus five follow-up mutation executions, not 30 distinct mutation classes. The GEL predicate and SMTP entry properties intentionally overlap the original review; the follow-up proves the newly added repository tests cover those properties and the three generated representations. Every source mutation and test-module mutation was confined to uniquely named
Scope and retained limitsThe original pinned-source conclusions remain applicable: the shared list protects the documented Grafana SMTP/LDAP, KEDA serving/CA, Prometheus serving/self-monitoring/patch-Job/root-CA, trust-manager serving, Loki NGINX, and optional MinIO credential refs. It uses enabled child contexts, actual consumer namespaces, names, keys, and gates, and feeds all existing SIE token/upstream readers. The independent generated-token probes confirm the same inventory also applies before gateway enforcement. The explicit exclusions for scaler-defined KEDA gRPC/Vault file layouts and arbitrary dependency env/volume/provider/custom-image configuration remain acceptable bounded triage linked to #780. The inherited trust-manager Helm-certificate namespace mismatch remains documented and linked to #781. Neither is silently claimed fixed by this PR. Detailed source and sibling-sweep records remain This reviewer did not author source or change repository files, refs, PRs, services, or deconflict state. Credentials were not technically stripped from the environment; the read-only restriction rests on instructions and observed actions. No live Secret data was read or printed. Except for the explicitly authorized read-only retrieval of official pinned certgen source during the original review, verification remained local; live issue bodies, registry image binaries, Kubernetes installs/upgrades, enforcing CNIs, and hosted endpoints were not independently tested. Static alias rejection does not assert that arbitrary TLS material is a valid runtime bearer token. Blocking findings: 0 |
|
Current-head release candidate has one setup failure: CPU/default job failed in mise's cache-save action, before buildx/build/smoke/artifact steps ran. Cache upload reported a non-200 response followed by The same head's other tool setups and several artifact builds passed. This points to cache/runner infrastructure; it is not yet classified as transient until an unchanged-head retry passes. The job retry endpoint currently returns |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
✅ Action performedFull review finished. |
|
Merged at |
1 participant
An operator can currently point SIE client, worker or upstream credentials at a bundled dependency's credential key without a render error. Extend the existing shared Secret-key inventory to reject those aliases for active, same-namespace Grafana SMTP/LDAP, KEDA TLS/CA, Prometheus admission TLS/CA, trust-manager TLS, Loki gateway basic auth and Loki MinIO root credentials.
Use enabled child contexts and their actual naming/namespace helpers. Preserve literal Secret names, custom data keys, generated/default names, disabled-component behavior and cross-namespace separation. Generation flags do not stand in for consumer gates: KEDA still mounts external TLS, Prometheus's root Issuer can remain active with the operator off, and its certgen Jobs read
caeven without TLS/self-monitoring. Pinned certgen source.The new real-render suite verifies dependency consumers before asserting collisions, covers worker/gateway/extra-env/MCP/upstream roles and inactive/different-key controls, and runs in the prepared Helm CI lane. README documents the finite guarantee. Arbitrary env/volume/provider/whole-Secret file layouts are separately assessed in #780; the inherited trust-manager Helm certificate namespace mismatch is tracked in #781. No dependency versions, auth defaults, deployment or live settings change.
Closes #767.
Validation
mise run helm -- lint, CI Ruff format/check selections,git diff --check: passed.Sibling-sweep evidence
Bug class: a SIE bearer/upstream credential names the same active Secret/key as another credential owner. The internal helper was run against this isolated public tree with scope
cluster; public reproduction below matches its 81 instances in 18 files. These consist of existing first-party guards, the newly covered dependency refs and the explicitly triaged operator configuration in #780/#781. The recursively extracted archives are exactly theChart.lockdependencies frommise run helm -- dependencies.Full public-tree sibling sweep
Pinned archive dispositions: Grafana SMTP/LDAP plus existing admin refs, KEDA serving/CA Issuer, Prometheus serving/monitor/patch-Job/root Issuer, trust-manager serving, Loki NGINX gateway and MinIO root refs now enter the same inventory. Arbitrary extension/configuration refs, scaler-defined KEDA gRPC/Vault filenames, externally backed Issuers, Prometheus/Alertmanager Secret lists and enterprise/provider layouts remain explicit operator responsibilities (#780). The trust-manager namespace-generation mismatch is not hidden by the guard (#781). Existing first-party NATS/config/gateway/worker/pull/oauth/MCP/ingress credential guards remain unchanged and are exercised by the existing Helm suite. Dependency
.tgzcontents and Secret payloads are not committed.Summary by CodeRabbit