Skip to content

fix(platform): revoke the gateway key on every session teardown - #3148

Closed
Israeltheminer wants to merge 1 commit into
mainfrom
fix/revoke-gateway-keys
Closed

fix(platform): revoke the gateway key on every session teardown#3148
Israeltheminer wants to merge 1 commit into
mainfrom
fix/revoke-gateway-keys

Conversation

@Israeltheminer

Copy link
Copy Markdown
Collaborator

A sandbox session mints an LLM gateway virtual key, hands its value into the container, and never revoked it. revokeVirtualKey had one caller where 0.4 had four.

Refs #3142.

Why

The mint body states the invariant it depends on, twice: the gateway has no native TTL on a virtual key, and reset_duration: '1M' is safe because "the key is revoked at session end, long before any reset matters."

Nothing revoked at session end. Three edges tore sessions down and left the key live:

Edge What it did
TTL expiry sweep flipped session status only
admin destroy / phantom heal revoked sandbox_session_tokens, never touched llm_gateway_key_id — while reading that column
task-agent deadline never revoked at all, contradicting its own caller's docstring

So each abandoned key is a permanent bearer credential with a self-refilling monthly allowance against the organization's own provider keys, and one accumulates per expired, deadline-failed or destroyed session. It is scoped — allow_all_keys: false, an explicit model list, bound to the org's own provider key ids — so it reaches no other tenant and no unlisted model. What it does is spend, every month, forever.

What changed

One seam, domains/sandbox/gateway-keys.ts, that all three edges call. Three copies of this behaviour would drift.

The token flip is the election. UPDATE … WHERE revoked_at_ms IS NULL RETURNING llm_gateway_key_id hands each live key to exactly one caller, so a watchdog sweeping the same session twice, or a destroy racing the expiry, revokes once and the second pass is a no-op. The gateway is idempotent too — an unknown key is a 404, treated as success — so even a genuine double delete cannot fail a teardown.

Exec-scoped teardown narrows to one key. A deadline-failed task-agent run revokes only the key its own exec minted, so a sibling turn still running on the same standing session keeps its credential.

Revocation runs before the token flip in the destroy path — running it after would find nothing left to claim.

The session row's llm_gateway_key_id is cleared so a later sweep can tell a revoked key from a live one. The token table keeps its id and carries revoked_at_ms as the mark instead, because the run provenance ledger matches turns by it.

Failure posture

Best-effort per key, as 0.4 was: an unreachable gateway must never wedge a teardown. But the failure logs at console.error naming the key id, because that key stays spendable and only an operator can delete it by hand. It is not a warning.

Tests

integration-check.ts, run against a real Postgres and MinIO — 278/280, with the gateway check passing on every edge:

expiry(gone=1/1 col=null live=0/0), destroy(first=true again=false keys=1/1),
deadline(run=1 sibling=0), posture(destroyed=true status=destroyed attempted=true)

again=false is the idempotency proof. sibling=0 is the exec-scoping proof. posture drives a 503 from the gateway and asserts the session still reaches destroyed while the leak is logged — the log line appears in the run output.

The two suite failures are environment, not diff: a warm-MinIO bucket collision and a yt-dlp-dependent probe absent from this host. Both fail identically on the unmodified base.

Scope

Keys already minted on a running deployment are not recoverable by this change. Nothing records the abandoned key ids once the rows are gone, so an operator has to list the org's virtual keys on the gateway and delete the ones with no live session. Worth doing on any deployment that has run sandboxes since #3107.

Three adjacent sandbox findings are deliberately untouched and tracked in #3142: the expiry sweep can expire a session under a live turn, the admission-ticket reap runs behind more probe time than its job expiry allows, and drift reconcile never re-asserts a dropped pin.

Gate: typecheck, oxlint --type-aware, oxfmt --check, lint:sast green.

A sandbox session mints an LLM gateway virtual key and hands its value into
the container. revokeVirtualKey had one caller where 0.4 had four, so the
TTL expiry sweep, admin destroy, phantom heal and the task-agent lane all
tore sessions down and left the key live.

The mint body states the invariant twice: the gateway has no native TTL, and
reset_duration '1M' is safe BECAUSE teardown revokes the key long before any
reset matters. It did not. So each abandoned key is a permanent bearer
credential with a self-refilling monthly allowance against the org's own
provider keys, one per expired, deadline-failed or destroyed session.

All teardown edges now go through one seam, domains/sandbox/gateway-keys.ts.
The token flip IS the election: UPDATE ... WHERE revoked_at_ms IS NULL
RETURNING hands each live key to exactly one caller, so a watchdog sweeping
twice or a destroy racing the expiry revokes once. An exec-scoped teardown
narrows to that turn's key, so a sibling turn on the same standing session
keeps its own credential.

Best-effort per key, as 0.4 was: an unreachable gateway must never wedge a
teardown. But the failure logs at console.error naming the key id, because
that key stays spendable and only an operator can delete it by hand.
@Israeltheminer
Israeltheminer force-pushed the fix/revoke-gateway-keys branch from 2ada8bb to 5a54362 Compare September 3, 2026 13:16
@Israeltheminer

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main. The finding still stands there: revokeVirtualKey has one production caller, and domains/sandbox/gateway-keys.ts does not exist.

Two things the rebase turned up.

Both conflicts were adjacent insertions in integration-check.tsmain added checkDocumentWriteGuards at the same spot. Resolved by keeping both, but the splice silently ate two shared-context lines: the docblock's /** and the closing brace of main's preceding function. Typecheck caught them (Unterminated regular expression literal, then '}' expected), and I verified afterwards that the integration-check diff against main is pure addition with none of main's check calls dropped.

The check now sets SANDBOX_LLM_GATEWAY_ADMIN_PASSWORD itself. Without it the admin client refuses to run anonymous — correctly, since the management API is reachable from every sandbox session — so every revoke failed authentication. The check still "passed" its fail-open lane while proving nothing about the revoke, which is exactly the false green worth avoiding. The fake gateway does not validate the value; the harness already sets SANDBOX_LLM_GATEWAY_URL the same way, and the previous value is restored on teardown.

Verified against a real Postgres and MinIO — 374/380:

expiry(gone=1/1 col=null live=0/0), destroy(first=true again=false keys=1/1),
deadline(run=1 sibling=0), posture(destroyed=true status=destroyed attempted=true)

again=false is the idempotency proof, sibling=0 the exec-scoping proof, and posture drives a 503 and asserts the session still reaches destroyed with the leak logged. The six failures are the warm-MinIO bucket collision, two yt-dlp probes and three agent-lane probes — the pre-existing set on this box.

@Israeltheminer

Copy link
Copy Markdown
Collaborator Author

Superseded by #3194 — rebased onto current main (10 commits behind), plus unit coverage.

Same gap as #3171: a 487-line new module with no test in CI. Its only proof was the integration-check.ts lane, which needs SANDBOX_LLM_GATEWAY_ADMIN_PASSWORD — without it every revoke fails authentication and the check exercises its fail-open lane. #3194 adds gateway-keys.test.ts covering the failure posture and the exec-scoped narrowing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant