feat: MU1 multi-user control plane (opt-in, SH_REQUIRE_AUTH=false by default)

[pdettori]

Implements the MU1 multi-user control plane from
docs/specs/2026-09-08-multi-user-control-plane-design.md
(ADR-0033). Before this, the deployment had one shared upstream API key and no notion of a user.

Opt-in and inert by default. deploy/knative/control-plane.yaml is deliberately not in the base
kustomization, and SH_REQUIRE_AUTH defaults to false — so existing single-user deployments see no
behaviour change. The 14 existing deploy/knative scripts that drive /turn and /runs without a token
keep working; a present-but-bad token is 401 in either mode. Flipping the default is a later milestone.

What it adds

• packages/control-plane (new) — the authenticated /v1 surface: Ed25519 session tokens
  (alg: EdDSA, hand-rolled on node:crypto), AES-256-GCM envelope encryption over per-user Kubernetes
  Secrets, a Redis session-ownership index with ordered cascade delete, GitHub OAuth device flow for
  identity, and a per-turn credential exchange on /internal/credentials.
• Data-plane integration in packages/knative-server — /turn resolves the caller's own credential
  from a session token, on both the buffered and SSE paths.
• harness/src/run-turn.ts — strictly additive: a tagged UpstreamCredential union and one new term
  at the front of the auth chain. P5's env fallbacks and its write-once ANTHROPIC_API_KEY seed are
  untouched; removing them is P5's to do.
• docs/api/openapi.yaml — the /v1 contract as a checked-in document, pinned by a drift test that
  compares it against ROUTES, CP_ERROR_CODES and CREDENTIAL_NAME_RE in both directions.
• deploy/knative/demo-multiuser.sh — a two-account live demo asserting 10 numbered claims, gated on
  MULTIUSER_LIVE_SMOKE, with a cluster-free test of the properties that make it honest.

Design decisions worth a reviewer's attention

• The token asymmetry is load-bearing. The control plane holds the private key; the data plane gets
  only public keys, as plain config — a compromised harness can verify but not mint. The keyset is a
  comma-separated list so rotation needs no flag day.
• Credentials are keyed on consumer tier (inference / sandbox-egress / control-plane), not on
  vendor. A multi-secret-field kind is refused for inference at write time, because the exchange
  delivers one Bearer token and basic would otherwise send a username upstream.
• AAD is subject|name, so relabelling a Secret's annotations cannot repoint one user's ciphertext at
  another's name.
• RBAC omits list. Secret names derive from the subject, so every access is a get by exact name;
  reaching another user's Secret requires already knowing their subject. list lives in a maintenance
  Role bound to nothing.
• 404, never 403, for another user's session — 403 is an existence oracle.
• Fail-closed everywhere. Redis down, kubectl unavailable, control plane unreachable, exchange 401 —
  every one refuses the turn rather than falling back to the deployment-wide key.

Known gap, tracked and guarded

#248 — the control plane currently runs in default, where the harness ServiceAccount holds
unrestricted pods/exec, so code execution in the harness pod can read the signing key from
/proc/1/environ. Spec §8.2 documents this honestly rather than leaving it implicit, and §8.1's
token-forgery row is qualified to the sandbox tier. It is not reachable from the sandbox tier where
model code runs, and it is inert while MU1 is opt-in with auth off.

A CI tripwire in packages/knative-server/test/control-plane-manifest.test.ts refuses the combination
SH_REQUIRE_AUTH=true and a default-namespace control plane, so the flag cannot be flipped without
#248 landing first.

Verification

pnpm -r test — 1377 passed / 17 skipped across 10 packages. pnpm -r typecheck, make lint (tree
clean after), make test-deploy, and pre-commit run --all-files across all 9 hooks: all green.

Built task-by-task with a per-task review gate, then a whole-branch review, one fix wave and a scoped
re-review. That final review returned CHANGES REQUESTED on three findings — all in deploy/ YAML, none
in the TypeScript — and the two blocking ones are fixed here:

• an additive NetworkPolicy: the base egress policy default-denied TCP 8080, so on any egress-enforcing
  cluster (including every OCP profile) the exchange hop was dropped and MU1 was non-functional;
• credential writes returned 503 during a Redis outage, contradicting spec §9.2's promise that they
  stay up — the audit is now best-effort on those two routes, and logged so the gap is discoverable.

Roughly twenty-five defects were found and fixed during development. The dominant class, by a wide margin,
was a test that cannot fail — so several commits here replace weak assertions with derived ones
(comparing against the code's own exported values) rather than adding new coverage on top.

[cwiklik]

First pass on 628feaa. 58 files, +9722/−46, 40 commits — reviewed the security-critical path in full (token mint/verify, envelope encryption, the exchange, the ownership choke point, the kubectl argv builders, RBAC) rather than by summary, and spot-checked the rest.

One must-fix, three suggestions. The must-fix is not in the crypto or the authz — those hold up. It is a keyset parse that runs per request outside a try block, which turns an operator typo into a 500 on every /turn including unauthenticated ones, and falsifies a property token.ts states in a comment.

What I verified rather than took on trust

The body makes a lot of specific claims. These check out against the tree:

Claim	Verified
RBAC omits list; maintenance Role bound to nothing	control-plane.yaml:80 is get,create,update,patch,delete; credentials-maintenance appears exactly once in the whole deploy/ tree — its own definition, no RoleBinding
Every Secret access is a get by exact name	secretNameFor is sh-cred-<sha256(subject)[0:16]>; K8sSecretStore.list() is a single get filtered in process, so the list verb genuinely is not needed
A compromised harness can verify but not mint	makeSigner refuses a non-Ed25519 and a public key; data plane only ever calls parseKeyset/verifyToken
The three secrets are separately mounted	sh-session-token-key, sh-credential-kek, sh-exchange-token — three distinct Secrets, and a test pins the exchange name/key parity across both tiers
/turn auth covers the SSE path	resolveTurnAuth is at server.ts:147, before the wantsStream branch at :153, and auth is threaded into handleTurnStream. This is the bypass #239's spec got wrong by parking :174; here it is right
Deleting a session actually revokes	exchange.ts:75-82 re-checks existence, owner match and tombstone on every turn, so local verification does not outlive a delete
404 never 403 for another user's session	assertOwner:85 collapses unknown and non-owner into one session_not_found; admin deliberately does not bypass it
The #248 tripwire is real	It is a conjunction read from both manifests with .toBe(false), plus a canary pinning false+default so changing either half alone still fails

Two things worth calling out as load-bearing and correct: verifyToken pins alg before the key lookup and parses claims only after cryptoVerify, so a forged exp cannot produce a token_expired oracle; and because the Secret name is a hash, nothing caller-controlled reaches spawn('kubectl', args) in leading-dash position — argv injection is closed by construction rather than by validation.

The unauthorized and KEK items below are both cases where the design was careful in one place and did not carry the same care to its neighbour. Neither is a bug today; both get materially more expensive after MU1 is switched on once.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript (control-plane, knative-server, harness), Kubernetes/YAML, Shell, Docs, Tests, Build
Agent/IDE config (.claude/.vscode): none — grepped both +++ b/ and rename to forms; 0 renames anywhere in the diff
Commits: 40, all signed off, all Assisted-By per house convention, conventional prefixes throughout
CI status: passing (12/12 on 628feaa — DCO, CodeQL, Trivy, shellcheck, hadolint, dependency-review, lint, proto, deploy-scripts all green)
Base: 45a218d; mergeable_state: blocked pending review

[cwiklik]

must-fix — turnAuthDeps() is outside the try that starts on the next-but-one line, and it can throw.

turnAuthDepsFromEnv calls parseKeyset (turn-auth.ts:64), which throws a plain Error — not a CpError — on three conditions: a malformed entry (token.ts:103), a non-Ed25519 key (:112), and a kid that does not match its key (:119). Because the throw lands outside the try at :146, writeAuthError never sees it. It propagates to the route's own handler at :637:

res.writeHead(500, JSON_HEADERS).end(JSON.stringify({ error: String(err) }));

So a malformed SH_SESSION_TOKEN_PUBLIC_KEYS produces, for every POST /turn and POST /v1/turn:

500 {"error":"Error: SH_SESSION_TOKEN_PUBLIC_KEYS entry '<kid>' is not a base64 Ed25519 SPKI ..."}

Three separate problems, in increasing order of how much they matter:

1. Internal error text to an arbitrary caller. Low severity on its own — the keyset holds public keys, and the design is explicit that those are not secrets — but it is the pattern errors.ts:89-92 deliberately refuses ("an arbitrary error's text can carry a Redis connection string or a presented token"). The control plane hardened this; the data plane's new auth path feeds the unhardened site.

2. It breaks the unauthenticated path too. The throw happens before anything looks at requireAuth or at whether a token was presented. A deployment adopting MU1 that fumbles the keyset does not get "MU1 is broken" — it gets /turn returning 500 for everyone, including the 14 existing no-token deploy scripts. The whole opt-in design exists so that turning MU1 on cannot disturb them, and this is the one path where a config error does.

3. token.ts:63-68 claims this cannot happen, and that claim is the rationale for the curve assertion:

   Asserting the curve here fails at PARSE time instead: parseKeyset runs at startup on both tiers, so a mislabelled key becomes a boot error naming the real problem.

   It runs at startup on one tier. main.ts:57 parses at control-plane boot, so the property holds there. On the data plane parseKeyset is reached only through turnAuthDepsFromEnv, which server.ts:98 calls per request — there is no startup call anywhere in packages/knative-server/src. And /healthz and /readyz do not touch the keyset, so the pod goes Ready and stays Ready while every turn 500s. That is precisely the "failure arrives per request on a healthy-looking deployment" outcome the comment says it prevents.

The easiest trigger is the one the keyset design exists to support: rotate a key, publish the new one alongside the old, forget to update the kid. parseKeyset:118-122 rejects that — correctly — and on this tier the rejection surfaces as a total /turn outage with no boot-time signal.

Fix, two halves, both small. They address different problems, so I would take both:

• Validate at startup. Call parseKeyset(process.env.SH_SESSION_TOKEN_PUBLIC_KEYS) once before server.listen(...) (:653) and let it throw. That is what makes token.ts:68's "on both tiers" true, and it converts an operator typo from a silent per-request 500 into a crashloop with the reason in the container log.
• Keep the per-request read, but make it typed. Move const deps = turnAuthDeps() inside the try at :146 and have turnAuthDepsFromEnv wrap the parseKeyset call so a keyset failure becomes new CpError('credential_unavailable', 'the token keyset is not usable') → 503 with no raw text. Defence in depth for the case where the env changes after boot, which is the whole reason the read is per-request.

If you would rather not add a boot check, the second half alone fixes the 500 and the disclosure; but then token.ts:63-68 needs its "on both tiers" claim corrected, because a future reader will otherwise rely on a boot error that does not exist.

[pdettori]

Fixed in 206b538, both halves.

I reproduced the 500 before changing anything — a test that posts to /turn with no token after setting SH_SESSION_TOKEN_PUBLIC_KEYS=garbage returned exactly 500 with the stringified parse error, on the unauthenticated path, as you described.

• Boot check. assertKeysetUsable(process.env) in startServer, before createServer — so nothing binds with an unusable keyset. It deliberately rethrows parseKeyset's own message, since that one is read by an operator in a container log and its whole value is naming the bad entry.
• Typed per-request read. keysFromEnv wraps the parse and throws CpError('credential_unavailable', 'the token keyset is not usable'); const deps = turnAuthDeps() moved inside the try at :146. Fixed message, so no entry text reaches a caller.

On your point 3: rather than just adding the call, I rewrote the token.ts comment to name both call sites — main.ts via configFromEnv, and turn-auth.ts's assertKeysetUsable from startServer — and to state that /healthz and /readyz never touch the keyset, so dropping either call returns it to a per-request failure on a Ready pod. The prose claim is what drifted; naming the sites makes the next drift visible. Same paragraph added to spec §5.2.

Also tightened a test this exposed: turn-auth.test.ts had a bare expect(...).toThrow() on a malformed keyset, which passed for either a plain Error or the typed one — i.e. it could not have caught this. It now asserts the code and the 503, that the message carries no entry text, and covers all three parseKeyset failure modes rather than one, including the kid-mismatch rotation footgun you called out as the easiest trigger.

[cwiklik]

suggestion — unauthorized is the one member of this set that is never about the caller, and passing it through reports a deployment misconfiguration as the user's fault.

Every other member is caller-attributable: credential_required/credential_ambiguous/credential_not_found are the subject's credential state, endpoint_unresolved is their credential's endpoint, session_not_found is their session, token_invalid/token_expired are their token. A caller can act on all seven.

unauthorized on /internal/credentials has exactly two sources, and neither involves the caller:

• checkExchangeAuth (exchange.ts:48-62) — the shared bearer, i.e. SH_EXCHANGE_TOKEN
• the defensive re-check at handlers.ts:389 — a routing bug

The user's token is checked separately inside exchangeCredential, and it fails as token_invalid / token_expired / session_not_found. So unauthorized reaching this tier means precisely one thing: the harness cannot authenticate to its own control plane.

What the user sees is statusFor('unauthorized') = 401 (errors.ts:47), which reads as "your token is bad." It is not, and no user action fixes it — they re-run the device flow, get a brand-new token, and it fails identically. Meanwhile the same class of fault reported one line earlier is 503: credential_unavailable covers "control plane unreachable" (:114), "returned a non-JSON body" (:122) and "returned an unknown credential mode" (:137). A control plane that is unreachable is a 503; a control plane that rejects the harness is a 401. Same fault, opposite blame.

Two practical consequences:

• The outage is invisible where you would look for it. 5xx alerting and retry/backoff see nothing; the dashboard shows a 401 spike that looks like users fumbling tokens. This bites hardest on first rollout and on exchange-token rotation — exactly when SH_EXCHANGE_TOKEN is most likely to be wrong on one side, which is why the manifest test pins the name/key parity across both tiers in the first place.
• If any client treats 401 as "token is dead, re-authenticate", a one-sided rotation makes every client discard a valid session token and stampede the device flow. I have not checked the clients here, so treat that as a possibility rather than a claim — but it is the reason I would not leave the status wrong.

Fix: delete 'unauthorized' from this set. It then falls through to throw new CpError('credential_unavailable', 'control plane returned ${res.status}') at :130 — a 503, with the status preserved in the message for whoever reads the log. One line, and it puts this failure in the same bucket as the three neighbours it belongs with.

Worth a line in the PASSTHROUGH doc comment too: the invariant that makes this set safe is "every code here is attributable to the caller," and stating it is what stops the next code being added on the weaker test of "the control plane can return it."

[pdettori]

Done in 206b538 — 'unauthorized' deleted from the set.

I checked the two sources before removing it. exchange.ts:54 and handlers.ts:390 are the only ones reachable via /internal/credentials; identity.ts throws unauthorized in five places but all are on the device-flow routes, which the data plane never calls. And exchangeCredential fails the user's own token as token_invalid / session_not_found well before any of that. So the reclassification is safe in the sense that matters: no caller-attributable failure gets swallowed by it.

It now falls through to credential_unavailable at :130 with the status in the message, and there is a test asserting the message still contains 401 — reclassifying should not cost the operator the diagnosis.

Took the doc-comment suggestion too, and made it the lead paragraph rather than a footnote: the invariant is that every member is caller-attributable, with the explicit note that "the control plane can return this code" is the weaker test that let unauthorized in. Pinned by expect(PASSTHROUGH.has('unauthorized')).toBe(false) so the decision is a test and not just prose.

On your unverified possibility — a client treating 401 as "token is dead, re-authenticate": I did not chase it either, and it does not change the fix, so I have left it out of the comment rather than assert something I have not checked.

[cwiklik]

suggestion — clearing the memoised state does not close the client it discards, so this leaks a Redis connection per failure — the same class of leak sharedRuntimeReporter was built to prevent.

The round-1 fix is right about the bug it targets: without clearing, ready stays a rejected promise forever and the reporter is dead for the life of the process. But client is block-scoped at :257 and is never referenced again, so after ready = undefined nothing holds it and nothing has called quit().

The post-connect case is the one that definitely leaks. Take the scenario the comment itself names — a Redis restart mid-rollout:

1. connect() resolves, index is set, calls succeed.
2. Redis restarts. putRuntime throws. The catch clears ready and index.
3. The client from step 1 is still connected (or reconnecting under node-redis's default reconnectStrategy) and is now unreachable — no reference, no quit().
4. The next call builds a fresh client. Steady state is one live client plus one orphan per clearing event.

Each flap adds an orphan, and each orphan keeps retrying on its own timer. Bounded in practice by pod lifetime — and this is display-only data on a best-effort path, which is why this is a suggestion and not a must-fix — but it is a slow leak in the one function whose sibling comment (:218-224) is entirely about not opening a connection per turn.

Hoist the client so the catch can close it:

let client: ReturnType<typeof createClient> | undefined;
let index: OwnershipIndex | undefined;
let ready: Promise<void> | undefined;
return async (sessionId, fields) => {
  try {
    if (!ready) {
      client = createClient({ url: redisUrl });
      const c = client;
      ready = c.connect().then(() => {
        index = new OwnershipIndex(c as unknown as CpRedisLike);
      });
    }
    await ready;
    await index?.putRuntime(sessionId, fields);
  } catch {
    // Close the client we are about to orphan, or a flapping Redis accumulates one connected,
    // retrying client per failure. quit() may itself reject; that must not mask the retry.
    void client?.quit().catch(() => undefined);
    client = undefined;
    ready = undefined;
    index = undefined;
  }
};

The const c = client capture matters: without it the .then closes over the mutable client, which the catch has already set to undefined by the time a slow connect() resolves, and index silently never gets built.

[pdettori]

Fixed in 206b538, with one deliberate divergence.

Not quit(). In node-redis 6 quit() is marked @deprecated use .close instead, and disconnect() likewise defers to destroy() (@redis/client/dist/lib/client/index.d.ts:424-439). Of the two live options, close() "waits for pending commands" — against a server that has just gone away, which is the case that lands here — and destroy() "rejects all commands immediately". For a client being abandoned on a failure path, immediate is what we want, so it is destroy().

That also means no promise to swallow, but destroy() throws on a client that never opened, so it is wrapped — and the state clearing happens before the close attempt, so a throw there cannot skip the retry your round-1 fix exists for.

I took the const c = client capture as written. It is not strictly reachable today — ready is assigned synchronously right after, so no other call can enter the if before the .then — but with client now mutable it is a one-line guard against exactly the silent failure you describe, and the comment says why rather than leaving a future reader to re-derive it.

Four tests, all red before the change:

• post-connect failure (your "definitely leaks" case): destroy called once, and the retry builds a fresh client
• connect() rejecting: destroy called
• five consecutive flaps: five clients, each destroyed exactly once — the accumulating-orphan steady state
• a destroy() that throws: the turn still resolves and the next call still retries

The existing mock only exposed connect/hSet, so it would have passed with any close call at all, including one that threw. It now records a destroy per client.

[cwiklik]

suggestion — the wire form is versioned but carries no key identity, so there is no KEK rotation path. Cheap to add now; not addable later without the old KEK.

The contrast with the token keyset is stark and, I think, unintentional. parseKeyset takes a list specifically so key rotation needs no flag day, and token.ts:81-83 spells out the procedure ("publish the new key alongside the old, roll the Service, then switch signing key"). Rotation is discussed eight times across the spec and token.ts. For the KEK: git grep -niE 'kek.*rotat|rotat.*kek|re-?seal' across docs/ and packages/ returns nothing. SH_CREDENTIAL_KEK is a single value, kekFromBase64 accepts exactly one, and the sealed form has a version but no key id.

Concretely, today:

• Changing SH_CREDENTIAL_KEK makes every stored credential undecryptable at once. There is no dual-key read window, so there is no rotation — only a cutover that breaks everything sealed before it.
• A rotation in progress is indistinguishable from an attack. open()'s single opaque message (:65-70) is the right call for a caller, but it means "sealed under the previous KEK" and "tampered ciphertext" produce the same failed to decrypt credential '<name>'. An operator mid-rotation cannot tell which they are looking at.
• Recovery is not an operator action. There is deliberately no read-back path anywhere in /v1 (handlers.ts:320-326), so nobody — including the control plane's own admin surface — can export and re-seal. Re-sealing requires the old KEK; without it, every user must re-enter every credential by hand. A leaked KEK therefore forces exactly the outcome the design otherwise avoids: full manual re-entry, at the worst possible moment.

The v1 prefix suggests at-rest evolution was already on your mind — it just stopped at format rather than key identity. Two options, either fine:

• Key id in the wire form: v1.<kekid>.<iv>.<tag>.<ct>, with kekid derived from the KEK the way keyIdFor derives one from a public key. open() then selects the right KEK and can say "sealed under an unknown key id" — a diagnosis, not a guess — without telling a caller anything about which bytes they got right.
• Accept a KEK list, newest-first for seal, try-each for open, mirroring parseKeyset. No format change, so no migration; rotation becomes "add the new KEK, re-seal lazily on next write, drop the old one."

The reason to do it in this PR rather than a follow-up: the moment MU1 seals its first production credential, adding a key id becomes a migration that itself needs the old KEK and a re-seal pass that the no-read-back rule forbids. Right now there is no ciphertext anywhere and it is a format decision. That asymmetry — free today, unbounded later — is the whole argument; if you would rather not expand this PR, a ## 9-style scope note recording the gap and the trigger would at least make it a decision rather than an omission.

[pdettori]

Taken, in this PR, as your second option — the KEK list. 206b538.

Your "free today, unbounded later" argument is the one that decided it: there is no ciphertext anywhere yet, so this is a configuration decision now and a migration-needing-the-old-KEK after the first production seal. Doing it later is the one option that is actually expensive.

keksFromBase64 replaces kekFromBase64 and parses a comma-separated ring, newest first; seal uses keks[0], open tries each. K8sSecretStore holds keks: Buffer[]. Rotation is: prepend, roll, let writes re-seal forward, drop the retired key.

Why not the key id in the wire form. I considered it and think the two options are not symmetric. A key id does not enable rotation by itself — open still needs a list to select from — so the list is the necessary half and the key id is the diagnostic addition on top. And it is a format change, which is precisely the cost your own argument says to avoid incurring later; incurring a smaller version of it now to improve an error message did not seem worth it. If the diagnosis becomes a real operational need it can go in as v2 alongside a v1 reader, which the version prefix already allows.

Two things I made sure the ring did not quietly weaken:

• It is not an oracle. One opaque failed to decrypt credential '<name>' after the whole ring fails, byte-identical to the single-wrong-key and tampered-ciphertext messages. Trying N keys must not turn one outcome into N distinguishable ones; there is a test asserting the exact message for a 3-key wrong ring and for a 1-key one.
• The AAD still binds against every key. It is re-bound per attempt, so a relabelled ciphertext is refused by the retired key too, not just the primary. That was the failure mode worth checking — a ring is a set of chances for the relabelling defence to be skipped.

Also: an all-empty value is refused at parse (an empty ring would have had seal reach for keks[0] and encrypt under undefined), seal([]) refuses explicitly rather than surfacing as a node crypto error naming neither the ring nor the credential, and a bad entry names its position — an operator adding a second KEK mid-rotation is exactly who needs to know which of the two is wrong.

Tests include a full cycle through the store, since that is what an operator performs rather than seal/open in isolation: write under the old key → read with [new, old] → next write re-seals forward → [new] alone reads it → [old] alone no longer does. Plus a credential under a dropped key throwing rather than reading as absent — a miss must not look like "no such credential", or a user sees it in list() and cannot use it with no signal to the operator.

Documented in spec §6.5 (including why the ring is in config and not on the wire) and in control-plane.yaml's header, with the three-step rotation procedure spelled out next to the kubectl create secret line.

[cwiklik]

Round 2 on 206b538. All four round-1 findings are fixed, and I checked each against the code rather than against the replies. The must-fix is closed on both halves.

Approving. One non-blocking suggestion below, and it is about the operational story around the new KEK ring rather than its cryptography — which holds up.

The four fixes, verified

Round-1 finding	Status	What I checked
must-fix server.ts keyset parse outside the try	fixed, both halves	assertKeysetUsable(process.env) at server.ts:657, before createServer. let deps: TurnAuthDeps at :150, assigned inside the try and definitely-assigned afterwards because the catch returns at :157. keysFromEnv yields a CpError, which makes writeAuthError:104's throw err rethrow — the thing that actually produced the 500 — unreachable for a keyset fault. token.ts now names both call sites instead of asserting the property in prose. turn-auth-route.test.ts:334 covers the SSE branch too, which is where I would have expected the fix to stop short.
unauthorized in PASSTHROUGH	fixed	Gone from the set (turn-auth.ts:129-137), so it falls through to credential_unavailable at :173 with the status kept in the message. The invariant is now the lead paragraph, and turn-auth.test.ts:365 pins has('unauthorized') === false — so re-admitting it fails a test rather than needing a reviewer to notice.
Redis client leaked per failure	fixed	State cleared at :318-321 before the destroy() attempt at :327-331, so a throwing close cannot skip the retry. That ordering was the thing worth verifying and it is right. const c = client capture at :306. Concurrent-failure case is safe too: the second catch sees client === undefined and no-ops rather than double-destroying.
KEK had no rotation path	fixed, as a ring	seal uses keks[0] (:70), open loops (:95). All four guards hold: the opaque throw is outside the loop at :113 and carries no key count, with a test anchoring /^failed to decrypt credential 'a'$/ across both a 3-key and a 1-key ring; setAAD is genuinely inside the loop at :98, with a test that relabels against [newK, oldK]; an all-empty value is refused at parse; seal([]) is refused at :71. k8s-secret-store.test.ts:160 walks the full cycle including step 4 — the retired key alone no longer opens it — which is what makes "re-seals forward" a demonstrated property rather than a claim.

On the two sub-suggestions you declined, both hold up. destroy() over quit() is better than what I proposed — I had not checked node-redis 6's deprecation or close()'s pending-command behaviour, and your reason for preferring it is the right one. Rejecting the wire-format key id also holds: the ring is what creates the dual-read window, and the format cost is real. The suggestion below is deliberately the version of that idea that needs no format change, because your argument against the format change is correct.

Author: pdettori (MEMBER — maintainer)
Areas reviewed (incremental 628feaa..206b538): TypeScript (control-plane, knative-server), Kubernetes/YAML, Docs, Tests
Agent/IDE config (.claude/.vscode): none — grepped both +++ b/ and rename to forms on the incremental diff; 0 renames
Commits: 41 (1 new: 206b538), signed off, Assisted-By per house convention, fix(mu1): prefix
CI status: passing — 12/12 green on 206b538 (DCO, CodeQL, Trivy, shellcheck, hadolint, dependency-review, lint, proto, deploy-scripts)
Incremental diff: 13 files, +614/−80

[cwiklik]

suggestion — step (c) is the one step an operator has to decide, and nothing in the system tells them when it is true.

The ring is the right shape and its guards are real — I verified all four. This is about the procedure's terminating condition, which is currently unobservable:

• open returns only the plaintext. The ring index that succeeded is discarded (envelope.ts:100-103).
• Nothing counts or logs a non-primary open. The whole control plane has four console.* calls; none is in envelope.ts or k8s-secret-store.ts.
• list() deliberately never touches the KEK (k8s-secret-store.ts:190), so the metadata path cannot answer it either.
• There is no read-back path in /v1, so nobody can sweep the store to check.
• The audit record carries subject, credential and decision — not the key that opened it.

So "once nothing is left under the old key" is a guess, and when the guess is wrong the outcome is the one the ring was added to prevent, narrowed to whichever subset of users had not re-PUT since the rotation began. Their credential is intact, undecryptable, and — correctly, per your own test's reasoning — throws rather than reading as absent. Recovery is still manual re-entry.

The part that makes it worse than it needs to be: the operator cannot tell whose. The only artifact is server.ts:128's

[control-plane] unhandled Error: failed to decrypt credential 'my-anthropic'

That is the credential name — user-chosen, and it collides freely across subjects. Nothing makes my-anthropic unique, and it is the obvious name for everyone to pick. The subject appears nowhere. So after dropping the key too early, the operator knows some users are broken and cannot enumerate them.

Fix, and specifically not the key id you rejected — this needs no wire-format change. Have open report which ring index succeeded (return it alongside the plaintext, or take an onKeyIndex?: (i: number) => void), and have K8sSecretStore.get log or count it when i > 0, keyed by subjectHash(subject) — already computed, and it discloses no login, which is the property k8s-secret-store.ts:28 already relies on. Two things follow:

• Step (c) becomes a positive signal: "no credential has opened under a non-primary key for N days." Today the only signal is the failure.
• When it does go wrong anyway, the log names the subject hash, which is enough to identify the user who needs to re-enter.

Your rejection of the wire-format key id and this are not the same question. A key id does not enable rotation — the ring does, and you are right that the format cost is not worth paying for key selection. But the key id was also the only thing that would have made completing a rotation observable, and dropping it left that hole open. An index report closes it for free, because it never leaves the process and changes no bytes at rest.

If you would rather not add it here, the honest alternative is to change this line and spec §6.5's "drop the retired key once nothing is left under it" to something an operator can actually perform — keep the retired key for a full credential-lifetime window, say, or require every user to re-PUT before step (c). As written, both documents instruct a check that cannot be carried out.

[pdettori]

Taken, as the index report rather than the doc-only alternative. 8d9d225.

Your framing is right that these are two separate questions and that the format cost was the only reason to refuse the first. An index that never leaves the process costs nothing at rest, so there was no argument left against it.

open returns { plaintext, keyIndex }, and only on success — a failure still yields the one opaque message with no index and no ring size, so the thing I checked hardest is that this does not walk back "a ring must not turn one outcome into N." K8sSecretStore.get logs every keyIndex > 0, keyed by subjectHash(subject).

Log rather than count, since there is no metrics surface to count into and inventing one for this would be the larger change. And deliberately not deduplicated, which was the one design decision in here worth arguing: a once-per-(subject, credential) cache is the obvious tidying, and it would break the signal. "No non-primary open for N days" is then satisfiable by a long-lived pod that logged once on day one while the credential is still stale — a false positive on exactly the condition the line exists to establish. So the steady state is silent and, during a rotation, the volume is the backlog. There is a test pinning the silence, because that is the half that makes it a signal.

On the failure path, the subject hash goes into the message rather than into a second log line: one artifact, no duplication with server.ts:128, and the original error is kept as cause. It reaches no caller — writeError reduces a non-CpError to a bare internal_error with no message at all, which is what makes enriching it safe. I did not reclassify the caller-visible status; a dropped KEK arguably reads better as credential_unavailable than as internal_error, but that is a different question from this one and you did not raise it.

Both documents now name the check instead of implying one. control-plane.yaml step (c) and spec §6.5 give the positive terminating condition, that dropping early is unrecoverable because /v1 has no read-back path, and that the hash in the log is the sh-cred-<hash> suffix — so "enumerate whom to tell" is a concrete instruction rather than a hope.

Four tests, and I checked each against a mutation of the code it covers rather than trusting that it was red once:

• the index report uses a three-key ring, because a boolean "was it the primary" passes a two-key test and still cannot say which retired key is in use (mutation: report keyIndex + 1)
• the log line names the subject hash, the credential and the ring position (mutation: log the raw subject — caught; the raw subject is a login, the hash is not)
• silence when the primary opens (mutation: drop the > 0 guard)
• the failure message, asserted exactly rather than by substring — which is also what pins the absence of the raw subject, instead of asserting that separately (mutation: drop the hash)

One assertion I had to tighten before it was worth anything: expect(line).toContain('1') for the ring position is satisfied by the subject hash on its own, since that is hex. It is toContain('ring index 1') now.

pnpm -r test — 1407 passed / 17 skipped; control-plane 306 → 310, which is the four above. pnpm -r typecheck, make lint (tree clean after), make test-deploy, all green.