feat(kmsCDHHelper): ecloud-platform stack integration

[seanmcgary]

Summary

Converts cmd/kmsCDHHelper (the SEV-SNP peer-pod secret-unseal helper) from the on-chain encrypted_env model to the ecloud-platform stack model introduced in #120.

On the stack (stack_id) path, the KMS /secrets handler returns only the recovered app-private-key — no env; the platform owns secrets. This PR teaches the helper to:

1. Use stack_id as the single identity (replaces app_id) — the KMS signs H(stackID) and secrets are IBE-sealed to identity stackID by the ec secrets set CLI, so the recovered key S·H(stackID) decrypts them.
2. Recover the app-private-key via the KMS stack_id platform path (SecretsOptions.StackID).
3. Fetch each sealed secret from the platform's InternalSecretsService HTTP gateway (GET /internal/v1/stacks/{stack_id}/secrets, Authorization: Bearer <internal_api_key>) and IBE-decrypt each inside the TEE.
4. Source stack_id + platform_secrets_url + platform_internal_api_key from SNP-bound cc_init_data (never stdin).

The on-chain encrypted_env/public_env decode + merge path is removed.

What changed

• pkg/clients/kmsClient: SecretsOptions.StackID wired into the eigenx-snp request (was never set — the helper couldn't reach the platform path).
• cmd/kmsCDHHelper/platform_secrets.go (new): fetchStackSecrets — InternalSecretsService HTTP client (Bearer auth, url.PathEscaped path, capped/truncated bodies).
• cmd/kmsCDHHelper/main.go: stack_id identity, SNP-bound platform config + validateStackID (path-injection guard), assembleEnvFromSecrets (per-secret IBE decrypt, fail-closed), resolveEnv (testable fetch seam; sentinel path skips fetch + cache); decodeEncryptedEnv removed.
• cmd/kmsCDHHelper/env_cache.go: mergeEnv removed; cache keyed by stack_id.
• Docs: main.go header + docs/009_eigenxSnpAttestation.md synced to the stack model.

Security posture

• Fail-closed: missing config, unreachable/non-200 platform endpoint, or an undecryptable secret all hard-error — no path emits an empty/wrong value.
• Path-injection double-guard: stack_id is content-validated (^[A-Za-z0-9._-]+$, rejects ./..) AND url.PathEscaped into a single URL segment (url.JoinPath deliberately avoided — it path-cleans rather than escapes).
• No stdin override: platform URL + API key are json:"-", sourced only from SNP-bound cc_init_data (same SSRF rationale as the KMS coords).
• No leak: the __EIGENX_APP_PRIVATE_KEY__ sentinel returns the raw key and never fetches or caches; no secrets/keys/plaintext logged.

Testing

• Unit tests per component: SecretsOptions.StackID wiring; fetchStackSecrets (happy/non-200/malformed/path-escape/body-cap/empty); assembleEnvFromSecrets IBE round-trip + fail-closed; applyInitdataKMSConfig + validateStackID; resolveEnv sentinel/normal/error; cache keyed by stack_id.
• Gates: go build ./... clean, gofmt clean, make lint 0 issues on touched packages, ./scripts/goTest.sh ./cmd/kmsCDHHelper/... ./pkg/clients/kmsClient/... PASS.

Design: docs/superpowers/specs/2026-07-08-kmscdhhelper-stack-integration-design.md
Plan: docs/superpowers/plans/2026-07-08-kmscdhhelper-stack-integration.md

Still a draft pending CI + your review.

[claude]

Claude finished @seanmcgary's task in 3m 11s —— View job

---

Code Review — PR #122

This is a well-engineered PR. The architecture is clearly thought through, the security rationale is documented at every key decision point, and the three prior rounds of feedback have been addressed cleanly. Below are the remaining items I found — one medium security concern that's genuinely new, and several lower-priority observations.

---

[Medium, Security] platform_secrets_url allows http:// — Bearer token sent in plaintext

validateHTTPURL (main.go:558) accepts both http:// and https://. fetchStackSecrets sets Authorization: Bearer <apiKey> (platform_secrets.go:88). If an operator configures platform_secrets_url = "http://..." in initdata, the internal API key is transmitted in plaintext on the wire.

The SNP-bound initdata prevents an attacker from changing the URL, but it doesn't prevent an operator from accidentally configuring http://. Given the peer-pod threat model (the key grants access to all of a stack's sealed secrets), requiring HTTPS for the platform path or at minimum emitting a log warning when the scheme is http:// is worth considering:

// in applyInitdataKMSConfig, after validateHTTPURL for platform_secrets_url:
if strings.HasPrefix(cfg.PlatformSecretsURL, "http://") {
    log.Printf("kmsCDHHelper: WARNING: platform_secrets_url uses http:// — Bearer token sent in plaintext; use https:// in production")
}

Fix this →

---

[Low, Quality] loadCachedEnv silently drops JSON decode errors — no diagnostic log

env_cache.go:92–96: a corrupt or partially-written cache file returns (nil, false, nil) — treated as a cache miss — with no log output:

if err := dec.Decode(&env); err != nil {
    // A corrupt/partial cache file shouldn't wedge the pod forever: treat
    // it as a miss so the caller re-fetches and overwrites it.
    return nil, false, nil
}

This is correct behavior (re-attest rather than wedge), but a silent re-attestation makes the corruption invisible to operators monitoring the journal. A log.Printf("warning: corrupt env cache for stack %q, re-attesting: %v", stackID, err) before the return would surface this without changing the behavior.

Fix this →

---

[Low, Testing] TestEmitKey_HappyPathWritesValueToStdout mutates global os.Stdout

env_cache_test.go:29–42 swaps os.Stdout via direct assignment: os.Stdout = w. This is global mutable state — if the package ever adopts t.Parallel() (which the other cache tests don't use today, so it's fine now), this creates a data race. A comment marking the test as not parallel-safe would prevent a future breakage:

// Not parallel-safe: swaps os.Stdout (global state).
// If the package ever uses t.Parallel(), refactor emitKey to accept an io.Writer.

---

[Low, Quality] decodeInitdata heuristic comment understates the fallback cost

main.go:387–419: The bytes.HasPrefix(trimmed, []byte("H4sI")) fast-path means a raw TOML file starting with "H4sI" will attempt base64→gzip decode first, fail at gzip.NewReader or io.ReadAll, and then propagate that error rather than falling back to raw TOML. The current code only falls back to raw TOML if the prefix check fails — there's no fallback on a gzip decode error. This is correct behavior (a file that looks like gzip but isn't is probably malformed), but the comment says "fail back to treating the input as raw TOML" which is slightly misleading:

// Sniff by attempting base64+gzip first; on failure, fall back to raw TOML.

It would be clearer to say: "If the magic bytes are present but decoding fails, the error is returned — we do NOT fall back to raw TOML in that case, to avoid silently accepting a malformed blob."

---

[Nitpick] validateHTTPURL doesn't reject embedded userinfo

main.go:558–573: http://user:pass@host/path passes validation. The SNP-bound integrity protection means an attacker can't inject a URL with embedded credentials, but a misconfigured initdata with platform_secrets_url = "http://admin:secret@host" would silently carry the credential in requests. Worth a if u.User != nil { return fmt.Errorf(...) } check.

---

[Info] var extraData []byte in run() is intentional dead code

main.go:277: var extraData []byte is always nil. The comment "reserved for the CDH plugin to bind extra runtime context" is clear. This is fine — just noting it's intentional.

---

Overall assessment

The architecture is sound and the security posture is strong. The SNP-bound initdata identity model, the fail-closed design throughout, the path-injection double-guard, and the appPrivateKeyKey sentinel cache exclusion are all implemented correctly. The test suite is comprehensive and covers the right edge cases. The medium item (http:// Bearer token) is the only one I'd want resolved before merge. The rest are quality/diagnostic improvements.

[seanmcgary]

Round 1 — addressed in a59dd4f

Thanks for the review. Dispositions:

[Medium] platform_secrets_url query/fragment preserved — Fixed. fetchStackSecrets now clears u.RawQuery and u.Fragment after building the fixed /internal/v1/stacks/{id}/secrets path, so stray params on the configured base URL can't leak into the request.

[Low] Uncapped body in requestSecretsFromKMS — Fixed. Wrapped the non-200 io.ReadAll in io.LimitReader(resp.Body, 64<<10), matching the caps already in GetMasterPublicKey / CollectPartialSignatures. Good catch that this PR expands client use inside the constrained peer-pod.

[Low] emitKey happy path untested — Fixed. Added TestEmitKey_HappyPathWritesValueToStdout (redirects os.Stdout via os.Pipe, asserts exact value, no trailing newline).

[Low] resolveEnv sentinel + Verified=false untested — Fixed. Added TestResolveEnv_SentinelUnverifiedErrors — drives the unverified root-key result through resolveEnv (not emitAppPrivateKey directly), asserts the "not verified" error propagates and the fetch is still never called.

[Nitpick] maxInitDataFileSize function-scoped — Fixed. Moved to the package-level const block alongside aaMaxBodyBytes et al., with the rationale comment.

[Nitpick] stackSecret(s) conversion safety comment — Fixed. Added a note on stackSecret explaining the field layout must stay identical to internalSecret for the conversion to hold.

[Nitpick] Re-parse/re-validation of base URL undocumented — Fixed. Added a comment noting the re-parse is intentional defense-in-depth so fetchStackSecrets is safe to call independently.

[Info] Escape test IDs are pre-rejected by validateStackID — Already documented. The test already carries a comment noting config-time validation is the primary guard and this covers the request-boundary escape as defense-in-depth. No change.

[Nitpick] docs/superpowers/ design + plan files in repo — Pushed back. These follow the convention established in #120 (which committed its spec + plan under docs/superpowers/). The plan also carries a Pipeline State block used to resume the ship pipeline, so it's intentionally version-controlled alongside the code. Happy to revisit repo-wide in a separate cleanup if the team prefers archiving them elsewhere.

[seanmcgary]

Round 2 — addressed in 9592327

[Low] Cache-path collision between valid stack IDs — Fixed. Dropped the ".." → "_" substitution from cachePath's replacer — it mapped distinct valid IDs ("ver..2", "ver_2") onto the same cache file. Traversal was never a real risk there: validateStackID rejects the exact ./.. segments, and stripping / + os.PathSeparator means a .. substring can't act as a traversal component. Added TestCachePath_DistinctValidIDsDoNotCollide to lock the regression.

[Low, pre-existing] Uncapped body in collectPartialSignaturesForDecrypt — Fixed. Wrapped the non-200 read in io.LimitReader(resp.Body, 64<<10), so all four operator-response reads in the file are now consistently capped.

[Nitpick] Step-5 EncryptedEnv check is a Byzantine DoS on the stack path — Fixed. The cross-response EncryptedEnv equality check is now skipped when opts.StackID != "". On the stack path env is fetched out-of-band and this field is ignored downstream, so gating recovery on it let a single Byzantine operator (esp. as responses[0]) fail the whole recovery over a value nobody consumes. On-chain behavior is unchanged.

[Nitpick] Silent override of stdin stack_id — Fixed. stack_id is now included in the stdinOverridden audit condition, so a stdin stack_id differing from the SNP-bound initdata is logged to stderr/journal like the other coord overrides. Message generalized to "ignoring stdin config overrides".

All gates green on the touched packages (build, gofmt, lint 0 issues, cmd/kmsCDHHelper + pkg/clients/kmsClient tests pass).

[seanmcgary]

Round 3 — addressed in 3778706 (final round, N=3 cap)

[Low] Cache lookup races applyInitdataKMSConfig (stdin stack_id forces re-attestation) — Fixed. Restructured run() so initdata is read + applyInitdataKMSConfig runs (setting the SNP-bound req.StackID) before the cache lookup. The cache is now always keyed by the trusted stack_id, so a stdin/initdata mismatch can no longer force a fresh attestation per sealed var. Added a comment documenting the ordering constraint.

[Nitpick] RSA keypair generated before initdata check — Fixed by the same restructure: generateRSAKeypair() now runs only on a cache miss, after the initdata read, so a cache hit avoids the entropy draw entirely and a missing/oversized initdata short-circuits first.

[Low] EncryptedEnv/PublicEnv populated from responses[0] on the stack path — Fixed. Added a comment on the SecretsResult construction noting these are always empty on the platform (stack_id) path (secrets are fetched out-of-band) and retained only for on-chain caller compatibility.

[Low] Query/fragment stripping untested — Fixed. Added TestFetchStackSecrets_StripsBaseURLQueryAndFragment — passes a baseURL with ?debug=true#frag and asserts the server sees no query string and the fixed /internal/v1/stacks/{id}/secrets path.

[Nitpick] emitKey lives in env_cache.go — Deferred. Purely cosmetic file placement; moving it adds churn without behavior change. Noted for a future tidy-up if the team wants it.

All gates green on the touched packages (build, gofmt, lint 0 issues, cmd/kmsCDHHelper + pkg/clients/kmsClient tests pass). This is the final feedback round (N=3 cap); the remaining item is the deferred cosmetic file move.