feat(dev): seal against the REAL KEK locally, closing the last parity gap - #861
Merged
Conversation
… gap `KMS_MODE=local` was the default, so every local stack sealed provider secrets and ingest tokens under a hermetic throwaway KEK. The envelope-encryption path — the compliance-critical one — was therefore never exercised before a deploy. It ran in production and nowhere else. The ledger said this one "has no alternative today: nobody should hold production AWS credentials to develop". The premise was wrong, in exactly the way that page warns about: THE CREDENTIAL ALREADY EXISTED. IAM user `webhook-co-claude-code` is already least-privilege — kms:GenerateDataKey + kms:Decrypt, on one key, nothing else. AGENTS.md says to look for the real credential before inventing a substitute; it had been there the whole time, recorded, while the page argued it could not exist. Verified before changing anything: a DEK wrapped by the real KEK, unwrapped, and the two proven to be the same key by encrypting with one and decrypting with the other. Then the engine booted with AWS bound and no KMS error. What a machine holds is the ability to wrap and unwrap, not the key — the KEK never leaves AWS. It is still a production capability, so the credential PAIR is excluded from the vault for the same reason RESEND_API_KEY is: narrow is not the same as non-production, and a passphrase beside the ciphertext is not where that belongs. KMS_KEY_ARN and AWS_REGION do travel, because an ARN and a region name are identifiers and withholding them is friction without security. Both halves of the pair are excluded together, so nobody has to wonder which half was the secret one. KMS_MODE survives as the opt-out for a contributor who cannot hold AWS credentials, now `external` with BLANK as the correct value rather than `local` with "local" — the substitute is no longer what you get by default. The engine already refuses the flag when any AWS field is bound, so the two cannot be half-applied; four existing tests cover that fence, including partial config.⚠️ I could not trigger that fence live: the provider is constructed lazily on endpoint creation, so booting the engine never reaches it. The tests are the evidence, not my probe — saying so because "I ran it and saw nothing" is not the same as "it fired". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae
🔴 AI review — VERDICT: BLOCKBlocking issues found (advisory — review before merging; add the Full reviewReview (
|
Right, and the same gap the review caught on the last two PRs: the load-bearing change here is that four AWS fields became `parityRequired` with `relaxedBy: KMS_MODE=local`, and nothing pinned it. Drop the flag or break the opt-out and local dev silently returns to sealing under a throwaway KEK with the vault, example and kms-provider suites all still green. The runtime fence does not cover this. It refuses `KMS_MODE=local` when AWS is bound — a different failure, and one that only fires once a provider is constructed, which is lazy. Nothing was checking that the fields are demanded in the first place. Three tests, mirroring the dmarc Resend and play Turnstile pins: each field is required and relaxed by that one flag; a BLANK KMS_MODE demands all four; and only "local" relaxes them. That middle one matters most. Blank is the PARITY path here, which inverts the usual convention — the substitute is the flag's presence, not its absence — so "blank relaxes" is exactly the one-character change that would put everyone back on the throwaway KEK without a single test going red. Mutation-verified: dropping `parityRequired` fails, and making blank relax fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae
choraria
added a commit
that referenced
this pull request
Jul 31, 2026
… name (#862) I merged #861 while the review still said BLOCK, having assumed its verdict was the previous round's. It was not: it had re-run and raised a different, correct finding, and I did not read it before merging. Both fixes are here. 1) The shipped DEFAULT was not pinned, only the enforcement. My tests covered that the four AWS fields are `parityRequired` and that only `KMS_MODE=local` relaxes them. None of that notices the manifest going back to `{ scope: "local", value: "local" }` — `pnpm dev:secrets` would then WRITE `KMS_MODE=local` into every .dev.vars, preflight would relax all four, and the engine would use the hermetic KEK again with all three tests still green. The regression lives in what we SHIP, not in what we enforce, and I had only guarded the second. Now pinned on both: the spec's scope and value, and the rendered example line (`KMS_MODE=`), because the rendered artifact is what actually reaches a developer's machine.⚠️ My first mutation check "passed" and I nearly took that as proof the test was weak. It was the CHECK that was wrong: `s.index('name: "KMS_MODE",')` matched a `relaxedBy: { name: "KMS_MODE", ... }` reference seven lines earlier, so it mutated a different field's scope. Verifying the mutation actually mutated — printing the resulting spec — is what caught it. A mutation that does not mutate proves nothing in either direction. 2) The IAM principal name was in a public doc. `no-secrets` covers account identifiers, not only secrets, and this repo is public. The doc now says what the principal is scoped to without naming it; the name lives in the credential store. Claude-Session: https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
KMS_MODE=localwas the default, so every local stack sealed provider secrets and ingest tokens under a hermetic throwaway KEK. The envelope-encryption path — the compliance-critical one — was never exercised before a deploy.The ledger said this one "has no alternative today: nobody should hold production AWS credentials to develop". The premise was wrong in exactly the way that page warns about: the credential already existed. IAM user
webhook-co-claude-codeis already least-privilege —kms:GenerateDataKey+kms:Decrypt, one key, nothing else.Verified before changing anything
A DEK wrapped by the real KEK, unwrapped, and the two proven identical by encrypting with one and decrypting with the other. Then the engine booted with AWS bound, no KMS error.
Blast radius
What a machine holds is the ability to wrap and unwrap, not the key — the KEK never leaves AWS. It's still a production capability, so the credential pair is excluded from the vault for the same reason
RESEND_API_KEYis.KMS_KEY_ARNandAWS_REGIONdo travel — an ARN and a region are identifiers, and withholding them is friction without security. Both halves of the pair are excluded together so nobody wonders which half was secret.KMS_MODEsurvives as the opt-out, nowexternalwith blank as the correct value rather thanlocal/"local"— the substitute is no longer the default.I could not trigger the AWS/local fence live — the provider is constructed lazily on endpoint creation, so booting never reaches it. Four existing tests cover it (including partial config); those are the evidence, not my probe.
Test plan
pnpm lint— 993 pass, 0 fail, exit 0dev-preflightgreen; 0 secrets in the committed example🤖 Generated with Claude Code