sec(secrets): resolve clientId/clientPassword from the existing Secret

[LukasWodka]

Part of tracebloc/backend#2571 — the structural half.

Deliberately notCloses: #2571 also carries the Docker Hub PAT rotation and the
credential rotations, which are human actions this PR cannot perform. Auto-closing the
ticket on merge would retire those with it. Per org-standards (.github#354), partial
work says Part of and keeps the ticket number out of the PR subject.

The problem

clientId and clientPassword were required in both the template and values.schema.json's top-level required, with minLength: 1. They therefore had to be supplied as values on every install and every upgrade — which means they were necessarily written into the Helm release's user-supplied values, in cleartext, in every retained revision (10 by default).

Measured on our own fleets: 3 fleets × 10 revisions, each a readable copy. Rotating the credential does not clear the older revisions, and anyone with get secret in the namespace can read all of them. Unlike the chart's other five credentials, there was no way to avoid it.

The fix

Three-tier resolution — the shape this file already uses for podTokenSigningSecret, credmgrPassword, tbMetaPassword, tbIngestPassword and bootstrapDbPassword:

1. explicit values, else
2. the CLIENT_ID / CLIENT_PASSWORD keys already in the live Secret, else
3. fail, naming both remedies.

Tier 3 is a hard failure rather than randAlphaNum — the backend issues these, so the chart must never invent one. An operator can now pre-create the Secret, or install once with values and drop them on the next upgrade, and the credential never enters release values.

The part worth reviewing closely: the schema was the real enforcement

Changing only the template was a no-op. JSON Schema validation runs before rendering, so an absent or empty value was rejected before lookup could fire — the template's own refusal was dead code and tier 2 was unreachable.

So this PR removes both from the schema's required and drops minLength: 1. Enforcement moves to the template, which is the only layer that can see the Secret. Placeholder rejection stays in the schema and is now applied to the resolved value, so a badly pre-created Secret is caught too.

I found this by mutation testing, not by reading. Worth a reviewer's eye on whether relaxing the schema is acceptable — my argument is that the template's tier 3 fails closed with a strictly better message and covers a path the schema cannot see.

Test coverage — including what is NOT covered

The unit suite cannot observe tier 2.lookup is inert under helm-unittest: deleting the entire tier-2 branch leaves all 30 unit tests green. Measured, not assumed. Shipping on unit tests alone would have meant the central mechanism of this PR was unverified.

So tier 2 is covered where it can be — a new path 5 in scripts/tests/e2e-auto-upgrade.sh, which already installs on real k3d. It upgrades with --reset-values (every user value discarded, so the credentials are genuinely absent) and asserts they survive in the Secret. If they do, the lookup is the only thing that could have supplied them.

Vacuous assertions fixed on the way

failedTemplate with errorPattern is silently ignored in helm-unittest 0.5.2. A pattern appearing nowhere in the output still passes. Proven by mutating two messages that such tests claim to assert — 49 tests stayed green. My own first draft used errorPattern and was completely hollow.

• Converted this file's two to errorMessage (honoured, exact) and re-ran the same mutation: it now reddens.
• Six more live in five other test files (auto_upgrade, image_refresh, network_policy, priority_class_pdb, rbac). Filed separately rather than dragged into a credentials PR.
• should reject empty credentials was passing on schema validation, not on anything in secrets.yaml. Its bare failedTemplate: {} could not tell the difference, and this change moved which layer refuses it.
• Placeholder tests stay bare deliberately: schema failures surface as a chart-load error no message assertion can match. Commented rather than dressed up.

Test plan

• helm unittest client — 579/579 pass, 34 suites
• helm lint clean
• Offline render without credentials fails with the tier-3 message at secrets.yaml:40
• Offline render with credentials produces 56 documents carrying CLIENT_ID/CLIENT_PASSWORD
• Mutation: break the tier-3 message → 2 tests redden
• Mutation: break mysqlRootPassword must be alphanumeric → reddens now, was green before the conversion
• Mutation: delete the tier-2 branch → 30 unit tests stay green (the gap path 5 exists to close)
• e2e-auto-upgrade.sh path 5 on a real k3d cluster — needs a CI run; GitHub runners were starved when this was written

Note on rollout

This does not remove the credentials already in our fleets' release values. That is an operational follow-up on backend#2571, and it must be coordinated: dev and prod currently share the same clientPassword, so it cannot be rotated per-fleet.

Refs: backend#2571 · backend#947 · backend#1528

🤖 Generated with Claude Code

---

Note

High Risk
Changes how platform authentication credentials are sourced and relaxes JSON Schema while shifting enforcement to templates; installer one-client detection behavior inverts for id-only-in-Secret releases.

Overview
clientId and clientPassword no longer have to stay in Helm values on every upgrade. The secrets template resolves them in three tiers—explicit values, then keys on the live release Secret via lookup, then a hard fail—matching other credentials in the chart but without a generated fallback because the backend issues these values.

Schema enforcement moved to the template.clientId / clientPassword were removed from top-level required and lost minLength: 1 so empty values can reach render and tier 2 can run; missing credentials are refused in secrets.yaml, and empty Secret keys count as absent so blank credentials cannot ship to pods.

Install and upgrade guards follow the same contract. Bash (_client_id_from_secret) and PowerShell (Get-ClientIdFromSecret) read CLIENT_ID from <release>-secrets when it is missing from helm get values; a client-chart release with no id in values or Secret is treated as unidentifiable and blocks another install instead of being ignored.

Tests and docs spell out limits. Helm unittest cannot exercise lookup (tier 2); e2e-auto-upgrade path 5 upgrades with --reset-values to prove Secret resolution. values.yaml documents Helm Secret adoption and that client-side GitOps renderers without cluster access must still keep these two in values.

^{Reviewed by Cursor Bugbot for commit 77f86fb. Bugbot is set up for automated code reviews on this repo. Configure here.}

[saqlainsyed007]

Direction is right and public-repo hygiene is clean — reading the credentials from an existing Secret so they never live in release values is exactly the backend#2571 goal. Requesting changes on one security-correctness gap, with two workflow issues to fix-or-scope. Details are in the three inline threads.

Blocker — empty stored key silently renders an empty credential (templates/secrets.yaml:38, and 46 for the password). The old code used required, which fails on an empty string. Tier 2 now takes the branch on hasKey $existingSecret.data "CLIENT_ID", and hasKey is true for a present-but-empty key — so a Secret carrying CLIENT_ID: "" resolves $clientId = "", the tier-3 fail (absent-key only) never fires, and the placeholder guard regexMatch "^<.*>$" doesn't match "". The chart then renders an empty credential and the client authenticates with nothing. Dropping minLength/required left no non-empty check on the resolved value. Please restore one (e.g. fail when the resolved $clientId/$clientPassword is empty), so the PR's own promise — "a badly pre-created Secret is caught" — actually holds.

Please address or explicitly scope in docs:

• Greenfield adoption (values.yaml:999): the documented preferred flow (pre-create the Secret out of band, empty values) fails a first helm install — Helm 3 won't adopt a kubectl-created Secret that lacks app.kubernetes.io/managed-by: Helm, aborting with "invalid ownership metadata". Tier 2 only works once Helm already owns the Secret (install-with-values-then-drop). Worth stating that ordering in the README, or the "preferred" label is misleading.

• GitOps / helm template (secrets.yaml:40): lookup is inert under helm template, --dry-run, and client-side ArgoCD/Flux rendering, so tier 2 can't fire there and operators must keep clientId/clientPassword in values — the cleartext-in-revisions problem this PR targets. If GitOps isn't a supported render path for this chart, a one-line caveat is enough; if it is, tier 2 alone doesn't cover it.

Two minor, non-blocking (not posted inline): tier 2 (the central mechanism) has no green test yet — its only coverage is e2e path 5 with the checkbox unchecked; per the repo's "evidence, not assertion" bar, land that run before merge. And path 5's helm upgrade --reset-values discards overrides set by earlier paths — harmless as the last path today, a latent trap if another is appended after it.

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 13ab07f. Configure here.}

[saqlainsyed007]

Re-reviewed at 13ab07f. The blocker is resolved — thanks for the thorough fixes.

Blocker (empty stored key → empty credential): verified fixed. Tier 2 now gates on the decoded value's truthiness, not hasKey: a pre-created Secret carrying CLIENT_ID: "" decodes to $secretClientId = "", the {{- else if $secretClientId -}} branch is falsy, and resolution falls through to the tier-3 fail. An empty key correctly reads as ABSENT instead of silently rendering a blank credential into the pod. Same for clientPassword. The placeholder guard running on the resolved value is a good addition. All three of my threads (greenfield adoption ordering, the empty-key blocker, the GitOps/helm template caveat) are addressed and resolved — I'm satisfied on the code.

Holding my approval only on CI — and it's this PR's own test. The Pester suite is red (ubuntu + windows) with exactly one failure:

[-] values without a clientId key do not trip the guard — it aborts with InvalidOperationException: A 'break' or 'continue' statement with a label that does not match any enclosing loop, which per Pester #2669 kills the block with no result rather than asserting. That's the test covering the very guard this PR changes, so it needs to go green (the mislabeled break/continue looks like the immediate cause). Unit tests / bats / Bugbot are still pending too.

Get CI green and I'll approve — the code side is no longer blocked on me.

[LukasWodka]

bugbot run

[LukasWodka]

@saqlainsyed007 — all three of your threads are addressed and resolved, plus two things CI found after your re-review. Summary of what moved since 13ab07f:

Your threads

1. Pre-created Secret / Helm ownership — took your first option and reordered: values.yaml leads with install-once-then-drop (no adoption step at all), and the pre-create path now carries the label/annotate commands for Helm's three ownership fields, with the note that <release>/<ns> must match the install exactly. Both tier-3 fail messages point there instead of at the bare create secret.
2. Present-but-empty key — took your suggestion with one shape change: the decode happens once into $secretClientId / $secretClientPassword and the tier-2 branch tests that for truthiness, rather than | b64dec inside the condition (which decodes twice). Same semantics for the empty case.
3. GitOps / client-side renderers — documented in 13ab07f, since no code change answers it. Your asymmetry framing is what I wrote down: the other five degrade to randAlphaNum on an inert lookup; these two can't, so tier 3 is a hard fail and the render stops. Both values.yaml and secrets.yaml now say outright that a GitOps install must keep both in values and that the cleartext-in-revisions problem is not solved for it.

Two found after that, both real

4. Pester was red on ubuntu and windows, and it was a genuine regression — e0981f4 inverted detect_installed_client's contract and left the PowerShell test asserting the old one ("values without a clientId key do not trip the guard"). That test was pinning the exact fail-open finding Develop #2 was about. Reproduced locally on pwsh 7.5.2 first, then rewrote it to the new contract, named the specific refusal (Should -Invoke Err -ParameterFilter { \$m -match 'unidentifiable existing client' } — every other fail-closed path here also throws, so a bare Should -Throw would pass on the wrong refusal), and mutation-proved it: restoring the pre-fix continue reddens it, restoring the fix greens it. Also dropped a trailing bare continue that PowerShell reported escaping as an unmatched loop label.
5. Bugbot Medium: the Secret read had no --request-timeout — and this PR is what makes that matter, since dropping clientId from values puts the call on the common path of two headless code paths. --request-timeout=5s on both twins (the repo's existing figure). A timeout exits non-zero into the existing "could not read" branch, which the caller turns into an unidentifiable client — so it fails closed and does not reopen the fail-open.

Verified: full Pester 784 total / 770 passed / 0 failed / 14 skipped (was 1 failed); helm-unittest 579/579; make check green; bash -n + shellcheck -S warning -x clean.

The PR was also retitled (ticket number out of the subject) and the body now says Part of tracebloc/backend#2571 rather than Closes — #2571 also carries the PAT and credential rotations, which are human actions this PR can't perform, so auto-closing it on merge would retire those with it.

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 77f86fb. Configure here.}

[LukasWodka]

@saqlainsyed007 CI is green at 77f86fb — the condition you held on is met.

Pester (ubuntu-latest) and Pester (windows-latest) both pass; the mislabeled break/continue in values without a clientId key do not trip the guard was the cause, as you read it. Full rollup at this head: 45 SUCCESS, 4 skipped, 0 failing. Bugbot's latest review (77f86fb) found no new issues, and all review threads are resolved.

Re-requesting your review so the change-request can clear.

[saqlainsyed007]

Approving at 77f86fb. Everything I raised is resolved and CI is green.

• The blocker (empty stored key → empty credential) stays fixed: tier 2 gates on the decoded value's truthiness (else if $secretClientId → tier-3 fail), and templates/secrets.yaml is unchanged since I verified it at 13ab07f.
• CI is green. The Pester failure I flagged — values without a clientId key do not trip the guard, aborting on the unmatched-loop-label bug (Pester #2669) — is fixed by dropping the no-op trailing continue in Get-InstalledClientInfo, keeping the two real ones above.
• The delta since my review is sound and I'd own it. Adding --request-timeout=5s to the kubectl get secret CLIENT_ID read on both peers (install-k8s.ps1 and install-client-helm.sh) is the right call now that this read is on the common path once clientId leaves release values — an unbounded read against a wedged API would hang a headless install, and the timeout falls into the same fail-closed return ""/return 0 (unidentified client, never absent). manifest.sha256 re-pinned to match.

All three of my original threads (greenfield adoption ordering, the empty-key blocker, the GitOps caveat) are addressed and resolved. Nice work on the credential-hygiene hardening — LGTM.

[saadqbal]

Answering the question you asked directly: yes, relaxing the schema is acceptable, and it's a net increase in enforcement rather than a trade. I checked the two things that decide it.

Nothing the schema used to catch is now uncaught. minLength: 1 refused an empty value; the template refuses an empty resolved credential by truthiness — if .Values.clientId / else if $secretClientId / else fail, where "" is falsy in both tiers, so an empty CLIENT_ID in a pre-created Secret reads as ABSENT and lands on the hard fail. Keying tier 2 off the decoded value rather than hasKey is what makes that true, and it's the difference between this and a version that ships blank credentials into the pods.

And the placeholder guards now run on $clientId/$clientPassword rather than .Values.*, so a Secret pre-created with <CLIENT_ID> is refused — a path the schema could never see. Strictly more coverage, one layer lower.

The rest I'd have raised is already in the file, which is why this reviewed quickly:

"The unit suite cannot observe tier 2." Deleting the entire tier-2 branch leaves all 30 unit tests green because lookup is inert under helm-unittest — measured, not assumed, and remedied by putting the coverage on real k3d with --reset-values. That's the check-that-can-observe-the-property problem, found and fixed by the author on their own PR. It's the single most common defect I've seen this week and normally it's the reviewer who has to find it.

The GitOps limit, stated rather than left to be discovered.lookup needs a live cluster, so ArgoCD's default renderer and Flux post-render hit tier 3 — and unlike the other five credentials there's no randAlphaNum to degrade to, so it stops rather than degrading. Saying outright that "a GitOps-rendered install must keep clientId/clientPassword in its values, and for it the cleartext-in-revisions problem this change fixes is NOT fixed" is the honest scoping, and naming the external-secrets mechanism that would close it while declaring it out of scope is the right place to stop.

Part of rather than Closes, because #2571 also carries rotations a PR can't perform — correct, and matching .github#354 which landed yesterday.

One thing I could not confirm:mergeStateStatus came back UNKNOWN/UNKNOWN on three separate reads several minutes apart, so I have no confirmation this is conflict-free — checks are green and there are no open threads, but treat the merge state as unverified rather than clean and re-check before merging. My approval is about the code. 👍

[LukasWodka]

Correction to my own commit message on 77f86fb. It says the full bats file "hangs on this Mac" and offers a stub-fidelity check in place of running it. The first half is wrong and I would rather correct it here than leave it standing in the history.

bats does not hang. It is slow — about 25 minutes on this machine — and completes:

$ bats scripts/tests/install-client-helm.bats # on 77f86fb, clean tree
BATS_EXIT=0
ok: 235
not ok: 0

I inherited the claim from an earlier session's commit message on this branch (e0981f4, which reported it hanging at DEFAULT (TB_STORAGE_MODE unset)) and repeated it after my own first run appeared to stall. It had not stalled; I had not waited long enough. Verifying an inherited claim before restating it is the thing I skipped.

This also retires a second inherited claim.370251f's message reported one pre-existing failure, fresh arm64 auto -> values carry the 8.4 mysqlClient block, "which aborts the run at 174 of 232". There is no such failure on this head — 235 of 235 pass, and the tests in that neighbourhood (174–179, the mysql-engine and arm64 cases) are all green.

What this changes about the evidence for this PR: nothing is weakened, and the four Secret-fallback tests are now covered behaviourally rather than by my hand-rolled stand-in.

ok 68 detect_installed_client: no clientId in values -> reads it from the release Secret (backend#2571)
ok 69 detect_installed_client: no clientId in values AND no readable Secret -> UNKNOWN, never 'no client'
ok 70 detect_installed_client: an EMPTY CLIENT_ID in the Secret is not an id -> UNKNOWN
ok 71 install_client_helm: a client with its id only in the Secret still blocks a DIFFERENT client

Test 68 is the one that mattered for the --request-timeout=5s addition: its kubectl stub keys on argument position ([ "$5" = "liverel-secrets" ]), so a flag inserted mid-vector would have broken it silently. It passes, which is the real confirmation the flag is appended rather than inserted — my earlier stub replica only predicted that.

Full evidence for this head, all of it now actually run: bats 235/235, Pester 784 total / 770 passed / 0 failed / 14 skipped, helm-unittest 579/579, make check green, and every CI check on 77f86fb passing.