docs(privacy): decide session-key custody, rotation, and erasure (ADR-0016) - #156
Conversation
…-0016) Closes #146. #98/#143 shipped SC-01's mechanism (per-tenant HKDF, AES-256-GCM, a merge-blocking plaintext canary) and deliberately deferred custody. This resolves the deferred half as a decision, not an implementation: a KMS-wrapped master with the existing per-tenant derivation left unchanged, rotation via a key_epoch field retired by a batch re-wrap job, erasure via a new per-tenant epoch registry rather than file deletion, and an explicit floor keeping dev/CI on the env-var + generateEphemeral() path with no KMS dependency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
documentation
Review — ADR-0016 (session-key custody), PR #156Verdict: blocking issues. Decisions 1, 2 and 4 are genuinely decided and well argued. Decision 3 — the erasure story, which is the headline claim of this PR and is now asserted as settled in two other docs — does not work as specified, and contradicts Decision 1 in three places. This needs a substantive revision, not a patch, so I have pushed no commits to the branch. What I checked
Checks I ran independently (all reproduce the PR body's claims)
No stale green claims — the numbers in the test plan are the numbers I got. Scope discipline: clean. Verified factual claims that do hold:
BlockingB1. Decision 3 does not erase anything. A non-secret counter cannot make a derived key underivable.Decision 3 states:
and immediately constrains the registry to:
Those two cannot both be true.
So after "retirement", anyone holding the master can recompute the retired tenant's key exactly as before. Nothing has been destroyed; a row in a table now says not to. That is a runtime policy check — precisely what issue #146's constraint and Decision 2's own sentence ("a retired epoch has to actually stop working, not 'stop working unless someone forgot to check'") rule out. The ADR correctly diagnoses this problem one paragraph earlier ("there is nothing at the tenant granularity to destroy") and then adopts a fix that does not address the diagnosis. The same field cannot be both the public selector Decision 2 needs and the secret whose destruction erases. Concretely, there are only two horns and the ADR should pick one out loud:
This is not a wording nit: B2. Decision 1 and Decision 3 contradict each other in three places.
B3.
|
Review on #156 established that Decision 3 did not work: it proposed a per-tenant registry holding "an epoch counter and a retirement marker", explicitly not secret material. But TenantKey is a deterministic function of (master, tenantId, salt, epoch), and every one of those survives retirement — the master by construction, the tenant id because the caller supplies it, the salt from the envelope header, and the epoch from the envelope header too, because Decision 2 needs it readable. Anyone holding the master could recompute a "retired" tenant's key exactly as before. It was a runtime policy check wearing a deletion's clothes, which is the failure keys.ts already refuses in the master case. Takes the per-tenant-secret horn rather than softening the claim: - Decision 3 now adds a per-tenant erasure secret — 32 random bytes minted at onboarding, wrapped under the same CMK, held only in the registry, and NOT derivable from (master, tenantId). It becomes a second secret input to HKDF beside the master, so destroying it makes that tenant's files underivable for real. Chosen over a per-tenant wrapped DEK because both cost the same per-tenant state but this shape needs master AND registry to decrypt, so neither compromise alone yields plaintext. Chosen over a KMS key per tenant because that needs the vendor decision this ADR deliberately does not make — and the registry row is left as the seam so that upgrade costs no re-encryption later. - Decision 1 no longer claims "HKDF derivation unchanged". Its rejection of the per-tenant DEK is kept for confidentiality (the reason holds) and explicitly reversed for erasure (that reason never covered it). - B3: key_epoch is pinned GLOBAL — one master generation per deployment. The per-tenant secret is a separate thing with a separate name, is never in the envelope, and is not an epoch. Two concerns, two names. - B4: the "revoke a specific wrapped value without affecting others" primitive was wrong and is corrected. Destruction granularity is the key or key version, never the ciphertext — cited to AWS and GCP docs with access dates, which is also what forces per-tenant secret material. - States exactly what this buys: erasure is cryptographic only to the extent the secret is destroyed everywhere including backups, it is forward-looking, and key_id stays master-scoped so erased files remain findable but unreadable. The backup question is now the load-bearing open question. Non-blocking notes: corrected the addCookies() re-verification (the SC-04 tripwire does call it outside src/session/), unquoted the ADR-0003 paraphrase, named the real ENVELOPE_VERSION cost (additionalData() binds the module constant into the AAD, so relaxing parseEnvelope alone is not enough), and used "re-encrypt" throughout for the batch job. Cross-links in session-state-encryption.md and docs/README.md no longer advertise the old mechanism. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Revised in
|
Resolves the docs/README.md Decisions-table conflict: #158 landed the ADR-0015 row on main where this branch adds ADR-0016. Both rows are kept, in numeric order. No other file conflicted.
Resolves the docs/README.md Decisions-table conflict again: #159 landed the ADR-0017 row on main where this branch adds ADR-0016. Both rows kept, in numeric order. No other file conflicted.
…ors diverge The primitives table had one cell I had not verified directly: that AWS retains prior key material for decrypt after rotation. Fetching rotate-keys confirms it, and turns up a difference that matters enough to record. GCP can destroy one key version and keep the current one. AWS's built-in rotation cannot — it 'retains all key material for a KMS key ... even if key rotation is disabled' and 'deletes key material only when you delete the KMS key'. So per-epoch destruction on AWS needs one KMS key per epoch, not automatic rotation of a single key. That constrains Decision 2, not Decision 3: the requirement is now stated as 'destroy a retired master generation without destroying the current one', with the note that if a vendor satisfies it by separate keys rather than key versions, key_epoch maps to a key and nothing else changes. Decision 1's shape list is updated to match.
Follow-up: two merges from
|
Re-review — ADR-0016 at
|
| Claim | Verified |
|---|---|
| "After a KMS key is deleted, you can no longer decrypt the data that was encrypted under that KMS key, which means that data becomes unrecoverable" | ✅ verbatim, deleting-keys intro |
| "If you delete a symmetric encryption KMS key, all remaining ciphertexts encrypted by that key are unrecoverable" | ✅ verbatim (see nit N4 on where it lives) |
| AWS waiting period 7–30 days, default 30 | ✅ "AWS KMS requires you to set a waiting period of 7 – 30 days. The default waiting period is 30 days." |
| "Destroying a key version means that the key material is permanently deleted" / "crypto-shredded" | ✅ verbatim, destroy-restore |
| GCP default 30-day window, restorable until it elapses, key-version granularity | ✅ verbatim |
| "Rotating keys creates new active key versions, but doesn't re-encrypt your data and doesn't disable or delete previous key versions" | ✅ verbatim, key-rotation |
| "AWS KMS retains all key material for a KMS key … even if key rotation is disabled" + "deletes key material only when you delete the KMS key" + decrypt "uses the same version of the key material that was used to encrypt it" | ✅ verbatim, rotate-keys |
| Manual rotation = a new KMS key | ✅ "You might decide to create a new KMS key and use it in place of the original KMS key. This has the same effect as rotating the key material … often thought of as manually rotating the key" |
| Blog: per-tenant CMK, "$1 monthly" | ✅ "A single AWS KMS key costs $1 monthly, increasing to a maximum of $3 per month with two or more key rotations." |
| Blog does not discuss erasure or crypto-shredding | ✅ confirmed — I looked for it specifically. The post covers isolation, cost and governance; the only offboarding sentence is about deleting aliases. The narrow-reading caveat is correct and I would have raised it if it were not there. |
The corrected primitive is right, and so is the self-caught AWS/GCP divergence — this is the finding I most expected to be asserted once and forgotten, and it is not. It appears in four places that agree: the primitives table gains a dedicated "destroy one prior generation while keeping the current one" row (AWS No / GCP Yes); the paragraph under the table draws the consequence; Decision 1's shape list is restated as "destroy a retired generation without destroying the current one … which AWS and GCP satisfy by different means (Decision 2)"; and Decision 2 closes by converting it into a vendor-neutral requirement plus the mapping fallback ("key_epoch maps to a key rather than to a key version, and nothing else in this decision changes"). That is the constraint actually doing work in the decision, not a footnote.
sources_verified: true is now earned. Inline URLs + access_date is the repo's convention (ADR-0001/0003/0007), the frontmatter matches every other ADR, and I found no remaining uncited external claim.
Non-blocking
N1. Rotation destroys the epoch's key material — and the registry rows are wrapped under it. Not stated, and it is the one omission with teeth.
Decision 3 stores erasure_secret "wrapped under the same CMK as the master." Decision 2 requires that a retired master generation's key material be destroyable, and records that on AWS that means "one KMS key per epoch." Compose the two: on the AWS mapping, the epoch's KMS key is the key the registry rows are wrapped under, so destroying a retired epoch after the grace window destroys every tenant's erasure secret — including tenants whose files were already re-encrypted under the new epoch, whose keys still need that secret. Global erasure as a side effect of a routine rotation.
The fix is one sentence of sequencing, not a design change: a rotation must re-wrap the registry under the new generation's key before the retired one is destroyed, and the batch job in Decision 2 owns that step alongside re-encrypting files. This is exactly the class of thing the ADR already chose to record for key_id ("a sequencing constraint the implementation cannot discover late") — the same paragraph is the natural home. Related and smaller: Decision 1's "one CMK per deployment environment" and Decision 2's "one KMS key per epoch" are in mild tension about what "the same CMK" in Decision 3 refers to once the AWS mapping is taken; naming the referent once would settle it.
N2. "What this actually buys, stated exactly" omits the in-process copy.
The list covers backups/replicas/WAL/snapshots, forward-looking capture, and findability — but not a live process that resolved the secret before erasure and still holds it (or a derived TenantKey) in memory. Decision 1 sets the precedent explicitly: the master is held "in memory for the process lifetime." The resolver-that-throws design is a per-resolution guarantee, so a cached value is precisely the "check a caller can forget" that Decision 3 is written to eliminate. Erasure is therefore effective against new resolutions, not necessarily against a running process, and that is a one-bullet addition to a list whose whole point is being exhaustive ("Specifically:").
N3. The "strictly stronger than a per-tenant DEK" argument is measured against the wrong baseline. The conclusion survives; the argument as written overstates.
"Someone who exfiltrates the entire registry still has nothing without the KMS-custodied master" is true — but it is equally true of a wrapped DEK, which is the shape actually being rejected. Wrapped DEKs in a stolen registry also yield nothing without KMS unwrap, so that clause is not a discriminator. The second clause ("someone who has the master still has nothing for a tenant whose row is gone") is an erasure property, and a wrapped-DEK registry has it identically — destroying the row destroys the key either way. So neither clause supports "strictly stronger" against the shape named.
The real, narrower difference is: an adversary who obtains one unwrapped per-tenant value gets that tenant's plaintext under the DEK shape and gets nothing without the master under this one. That is genuine defence in depth and a genuine segregation-of-duties gain (registry access alone is insufficient). It is also bounded: the master and the registry rows are wrapped under the same CMK and unwrapped by the same principal, and the master sits unwrapped in the memory of any process that reads session state — so against CMK-principal compromise the two shapes are equivalent.
The rejection still holds, and the argument that carries it is the one already in the paragraph and not being leaned on: this shape composes with the derivation that exists (HKDF, per-file salt, info, key_id, the envelope all keep working with one new secret input) where a DEK replaces it. I would lead with that and state the security delta at its true size. Flagging it because "strictly stronger, same storage cost" is a stronger claim than the evidence, in a document whose revision was about exactly that.
N4. Two citation-precision nits, neither of which changes a conclusion.
- The rotate-keys ellipsis elides an origin qualifier: the source reads "AWS KMS retains all key material for a KMS key with
AWS_KMSorigin, even if key rotation is disabled." The conclusion survives theEXTERNAL-origin case too — imported material can be deleted individually, but the same page says "The KMS key becomes unusable as soon as any of its materials expires or is deleted," so you still cannot drop one generation and keep the current one. Naming that makes the row stronger, not weaker. - "If you delete a symmetric encryption KMS key, all remaining ciphertexts encrypted by that key are unrecoverable" lives in the Deleting KMS keys with imported key material subsection. The claim is independently carried by the unconditional statement in the page intro, which the ADR also quotes, so nothing is wrong — just worth knowing which sentence is the general one.
Mergeability and the README
MERGEABLE; mergeStateStatus: BLOCKED is REVIEW_REQUIRED only — all 12 checks are SUCCESS (lint-typecheck-test-secrets, privacy-canary, CodeQL, testbed-smoke, guards, labels). The two main merges are clean: docs/README.md's Decisions table carries ADR-0001 through ADR-0017 with no gaps and no duplicates, ADR-0016 sits in numeric order between 0015 and 0017, and there are no conflict markers anywhere under docs/. git diff --name-only origin/main...HEAD is still exactly the three docs files, so nothing else regressed in the merges.
Verdict
Safe to merge. B1, B2, B3 and B4 are all genuinely resolved — B1 by a mechanism that survives the test that broke its predecessor, B4 by citations I checked one at a time against the live pages, including the negative claim about what the AWS blog does not say. The self-caught AWS/GCP divergence is correct and is wired through the decision rather than mentioned once. N1–N4 are follow-ups; N1 is the one I would take before the implementation issue is written, since a rotation that silently erases every tenant is a bad thing to discover from the implementation side.
I have not approved and have not merged, per the reviewer's remit. The PR description still described the superseded counter-based Decision 3 and asserted that derivation does not change — since the verdict is merge-safe and a squash commit inherits that text, I have rewritten the body to describe what the document actually says now.
1 participant
Closes #146. Follow-up to #98/#143 (
docs/privacy/session-state-encryption.md).A decision document, not a code change. #98 closed SC-01's mechanism half — per-tenant HKDF derivation, an AES-256-GCM envelope, a
TenantKeyan object literal cannot forge — and deliberately deferred custody: where the master lives in prod, what rotation means for a stored file, and whether "destroy the key" is a real erasure story. ADR-0016 decides all four questions #146 asks.Timed ahead of need on purpose. Re-verified rather than inherited from the old finding:
grepforpersistSessionState/restoreSessionStateacrosssrc/andexperiments/returns nothing outsidesrc/session/itself, and the onlyaddCookies()call outside that package istests/unit/session-material-tripwire.test.tsgiving the SC-04 tripwire a real cookie to fail on. No production path persists or restores session material. That matters because two of the four decisions change key derivation, and changing derivation after real files exist means re-encrypting every one of them — the same ordering argument #98 used for per-tenant derivation itself.The four decisions
keys.tsreads fromPARAGENT_SESSION_MASTER_KEYtoday; the app unwraps once at start and holds them for the process lifetime, exactly as it holds the env-var bytes now. Vendor deliberately unpicked — greppeddocs/pitch|decisions|prd|privacyand confirmed no AWS/GCP/Azure/Vault commitment exists anywhere in this repo, so inventing one here would be the speculation Session-key custody: the deferred half of SC-01 (KMS, rotation, tenant offboarding) #146 rules out. What is decided is the shape a vendor must satisfy. Rejected: a per-tenant wrapped DEK as the confidentiality mechanism (HKDF already gives per-tenant confidentiality with no stored state), a self-hosted HSM/Vault (swaps a managed burden for an operational one with no benefit this repo has stated a preference for), an age-style offline key (a live decrypting service needs the private half loaded, which is a KMS with extra steps).key_epochbesideversion, retired by a batch job.versionmeans "how this envelope is laid out";key_epochmeans "which generation of the master wrote this." One live generation per deployment, bumped once globally per rotation, nothing per-tenant. Batch re-encryption, not lazy-on-read: lazy completion depends on reads happening, and nothing reads these files today, so a rotation that waits for organic reads may never finish. And it is re-encryption, not re-wrapping — the tenant key is derived from the master, so every file is a full read-modify-write, which is the number whoever sizes the grace window is actually sizing. A file carrying an epoch custody no longer retains fails closed with a distinct error (provisionallySessionKeyRetiredError), never a fallback to the current epoch — the posture ADR-0013 took for a partial cache hit.TenantKeyis a deterministic function of(master, tenantId, salt), and a per-tenant counter added to that is a fourth input that survives its own "retirement" — master untouched, tenant id known, salt in the envelope, counter necessarily in the envelope too. Nothing would be destroyed; a row would say not to, which is a runtime check somebody in a hurry can skip. So: a registry row per tenant holding a 32-byte erasure secret minted at onboarding, wrapped under the CMK, not derivable from(master, tenantId), and never in the envelope — which is what lets it be secret. Derivation extracts frommaster ‖ erasure_secret, leaving the per-filesaltand theinfodomain separation exactly as they are. Destroy the secret and that tenant's files are underivable, master and every other row untouched. Preferred over a per-tenant wrapped DEK (same per-tenant storage cost, but a DEK is the tenant's key while this is half the input, and it composes with the existing derivation instead of replacing it) and over one KMS key per tenant (rigorous, but it forces the vendor decision this ADR declines and puts a recurring per-tenant cost in the onboarding path — explicitly not foreclosed: the registry row is the seam, and backing the secret with a per-tenant KMS key later changes only the resolver, re-encrypting nothing).generateEphemeral()and the env var stay the only path either needs; neither holds real tenant material, so neither needs a KMS credential, rotation, or the registry. The ephemeral path mints a per-process erasure secret behind the same resolver type as prod, so dev is not a second, laxer way to build aTenantKey.What it refuses to claim
The mechanism is real, so the limits are stated rather than implied. Erasure is cryptographic only to the extent the secret is destroyed everywhere it exists — backups, replicas, WAL, snapshots — which makes the registry a secret store with a destruction requirement its datastore must actually meet, and that is now the load-bearing open question, with the honest fallbacks named (per-tenant KMS key, or word the commitment as policy-enforced). Erasure is forward-looking, as with every crypto-shredding scheme including KMS key deletion. And
key_idstays master-scoped on purpose so a sweep job can still find an erased tenant's files: erased means undecryptable, not unfindable.Vendor primitives, cited rather than assumed
A primitives table with inline URLs and access dates, because an earlier draft built a decision on a primitive that does not exist. The correction: destruction granularity is the key or key version, never the individual ciphertext — so there is no primitive that erases one tenant out of shared key material, which is why Decision 3's requirement is an identity and not a preference. The two vendors also diverge on the primitive Decision 2 leans on: GCP can destroy one key version and keep the current one, while AWS "deletes key material only when you delete the KMS key," so per-epoch destruction on AWS means one KMS key per epoch (its manual rotation) rather than automatic rotation of one key. Stated as a vendor-neutral requirement — the deployment must be able to destroy a retired master generation without destroying the current one — with the mapping fallback spelled out. The AWS multi-tenant blog is cited narrowly and labelled as such: it argues per-tenant keys for isolation and cost and does not discuss erasure at all.
Consequences worth knowing before implementation
ENVELOPE_VERSIONbump is not "bump the constant."parseEnveloperefuses any version but its own, andadditionalData()binds the module constant into the GCM AAD — so a v2 build reading a v1 file computes AAD over2and fails the tag. Reading old files requires the AAD to use the parsed version, a deliberate change to what it commits to. Cost is real and currently zero-impact, which is the argument for landing it before a caller exists.key_idis an HMAC under the master, so a rotation changes it: the batch job must resolve the master bykey_epochbefore comparingkey_id.persist.ts'sfilePathis caller-supplied and there is no caller to own populating one.Cross-links
docs/privacy/session-state-encryption.md: the first three "What v1 defers" rows now read decided, not implemented and point at the ADR; the derivation section says plainly that ADR-0016 changes it and why now; the prod threat-model row and the tenancy-granularity open question follow.docs/README.md: the ADR-0016 Decisions row, and the privacy row's last-true date.Test plan
npm run lint:docs— clean (61 docs)npm run ci— exit 0 (secret-scan, validate:contracts, lint, lint:docs, typecheck, 432 unit / 31 files, 26 integration / 5 files)npm run test:canary— 52 pass / 8 files (unaffected; docs-only)docs/README.md,docs/privacy/session-state-encryption.md, and the new ADR — nothing insrc/, no fixtures, no session materialmainmerged twice to absorb feat(intent): resolve a natural-language goal to a task_key before requiring one #158's and feat(cache): add a pinned-version vocabulary rule to the pool allowlist #159'sdocs/README.mdrows; the Decisions table carries ADR-0001 through ADR-0017 in order🤖 Generated with Claude Code