Skip to content

fix(rbac): let the per-experiment cred reaper list secrets to reap orphans (backend#2692) - #875

Merged
aptracebloc merged 1 commit into
developfrom
fix/2692-db-creds-reaper-list-secrets
Aug 27, 2026
Merged

fix(rbac): let the per-experiment cred reaper list secrets to reap orphans (backend#2692)#875
aptracebloc merged 1 commit into
developfrom
fix/2692-db-creds-reaper-list-secrets

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What

perExperimentDbCreds granted jobs-manager delete on Secrets but not list. The per-experiment credential reaper enumerates (LISTs) orphaned cred Secrets before deleting them, so it 403s on the LIST and can never reap an orphan — orphaned credential material accumulates in etcd and the matching MySQL users are never revoked, the exact failure mode per-experiment creds exist to prevent. It backs off quietly (WARNING/exponential), so it went unnoticed on dev (since 2026-08-25) and staging; prod is unaffected only because the flag is off.

Fix

Add list to the existing flag-gated, secrets-only rule → verbs: ["list", "delete"], mirroring the perDatasetPvcs GC grant directly below it (same shape, done right).

  • Applied in both the ClusterRole and the namespaced Role branch — the reaper runs regardless of clusterScope, and the delete-only rule was duplicated in both. (Class check: these were the only two enumerate-then-delete grants missing list; the auto-upgrade-rbac.yaml delete grants are all resourceNames-scoped delete-by-name and deliberately avoid list.)
  • Stays flag-gated and byte-for-byte unchanged when off (proven — see tests).
  • list on secrets does widen exposure namespace-wide, and RBAC list cannot be resourceNames-scoped, so this is the minimal viable chart-level grant; the tradeoff is documented inline. The issue's narrower alternative (derive candidates from the Job list) is a client-runtime change, out of scope here.

Chart.yaml 1.9.75 → 1.9.76 (version + appVersion) for the version-bump gate.

Out of scope

The issue's secondary ask — the reaper backs off too quietly on a permanent 403 and should escalate — lives in client-runtime (jobs-manager), not this chart. Not addressed here; worth a separate client-runtime issue.

Tests / evidence

  • helm unittest ./client585 → 586 suites green (rbac suite: 20 tests). Asserts list+delete on secrets in both branches when on, and no secrets-only rule of either shape in both branches when off.
  • Mutation-proven: reverting the template to ["delete"] reddens exactly the two flag-on tests.
  • helm lint clean; chart-version-guard ✓ (content changed + version bumped).
  • Renders confirmed for all four perExperimentDbCreds × clusterScope combinations.
  • manifest.sha256 untouched — chart templates aren't hash-pinned.

Closes tracebloc/backend#2692

🤖 Generated with Claude Code


Note

Medium Risk
Expands jobs-manager RBAC to namespace-wide Secret list when perExperimentDbCreds is on, though it remains flag-gated and scoped to secrets only.

Overview
When perExperimentDbCreds is enabled, the Helm chart now grants jobs-manager list as well as delete on Secrets (flag-gated, secrets-only), in both the ClusterRole and namespaced Role paths. The prior delete-only rule was enough for named deletes at Job end, but the orphan credential reaper must enumerate Secrets first; without list that path 403s and orphaned cred material can linger in etcd.

Default installs stay unchanged when the flag is off; helm unittest coverage was extended to require list+delete when on and to reject both the old delete-only rule and the new pair when off. Chart version bumps to 1.9.76.

Reviewed by Cursor Bugbot for commit 826c516. Bugbot is set up for automated code reviews on this repo. Configure here.

…phans (backend#2692)
The perExperimentDbCreds RBAC grant gave jobs-manager `delete` on Secrets but
not `list`. The orphan reaper enumerates (LISTs) cred Secrets to find any whose
Job died before running cleanup, then deletes them — so it 403s on the LIST and
can never reap an orphan. Orphaned credential material accumulates in etcd and
the matching MySQL users are never revoked, the exact failure mode per-experiment
creds exist to prevent. The reaper backs off quietly (WARNING/exponential), so
it went unnoticed on dev (since 2026-08-25) and staging; prod is unaffected only
because the flag is off.
Add `list` to the existing flag-gated, secrets-only rule in both the ClusterRole
and namespaced Role branches (the reaper runs regardless of clusterScope), giving
`["list", "delete"]` — the same shape as the perDatasetPvcs GC grant directly
below. The grant stays flag-gated and byte-for-byte unchanged when the flag is
off. `list` on secrets does widen exposure namespace-wide; RBAC `list` cannot be
resourceNames-scoped, so this is the minimal viable chart-level grant and the
tradeoff is documented inline.
Tests: rbac_test.yaml now asserts list+delete in both branches when on and no
secrets-only rule of either shape in both branches when off; mutation-proven
(reverting to delete-only reddens exactly the two on-tests). Chart.yaml
1.9.75 -> 1.9.76 for the version-bump gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@aptraceblocaptracebloc self-assigned this Aug 27, 2026
@aptracebloc
aptracebloc requested review from saqlainsyed007 and removed request for saqlainsyed007August 27, 2026 12:17

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the security-critical part rather than the description: list lands on exactly one rule — apiGroups: [""], resources: ["secrets"], inside {{- if .Values.perExperimentDbCreds }} — in both the ClusterRole and the namespaced Role branch, with nothing outside that block touched. Secrets only, never configmaps, and no other verb or resource moves.

The disclosure is the part I'd want kept verbatim."list on secrets does widen exposure namespace-wide, and RBAC list cannot be resourceNames-scoped" is exactly right — there is no narrower chart-level grant available, so calling this the minimal viable chart-level grant is precise rather than a hedge. Naming the narrower alternative (derive candidates from the Job list) and placing it in client-runtime is the correct handling: it's a real option, it isn't available here, and pretending otherwise would be the easy thing to write.

The class check is the second negative one I've seen this week and it's the more useful kind: "these were the only two enumerate-then-delete grants missing list; the auto-upgrade-rbac.yaml delete grants are all resourceNames-scoped delete-by-name and deliberately avoid list." Establishing that the siblings are a different shape, on purpose, is what makes "only two" trustworthy.

And the reason this sat unnoticed is more important than the permission. The reaper backs off quietly on a permanent 403 — WARNING plus exponential — so a security mechanism was provably doing nothing on dev since 2026-08-25 and on staging, and the only reason prod is clean is that the flag is off. Orphaned credential material accumulating while the matching MySQL users are never revoked is the precise failure per-experiment creds exist to prevent, so the mechanism was inverted, not merely degraded.

You've correctly scoped the escalate-on-permanent-403 fix to client-runtime, and I'd argue it matters more than its "secondary" billing suggests: a permanent 403 retried forever is indistinguishable from a healthy idle reaper, so the same quiet backoff will hide the next permission gap exactly as well as it hid this one. Worth filing rather than remembering.

Mutation-proven (reverting to ["delete"] reddens exactly the two flag-on tests) and the off-path asserted absent in both branches, which is the right pair — the second half is what stops a default install silently gaining the grant.

Green, no threads. 👍

@aptracebloc
aptracebloc merged commit 897a899 into developAug 27, 2026
28 checks passed
@aptracebloc
aptracebloc deleted the fix/2692-db-creds-reaper-list-secrets branch August 27, 2026 12:25
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@aptracebloc@saadqbal@LukasWodka