Repository navigation
api: keep earlier attach tokens valid when a session create is replayed - #809
Closed
NancyKwongPerplexity wants to merge 2 commits into
Closed
NancyKwongPerplexity wants to merge 2 commits into
NancyKwongPerplexity wants to merge 2 commits into
Conversation
A data-plane session stored one attach token, so replaying its create with the same idempotency key overwrote the token every earlier response returned. Concurrent same-key replays left all but one caller with a token that failed with a non-retryable 401, and renewing a port session's token by replay could 401 requests still in flight on the previous one. Replay also ignored the session's status and returned a token for a revoked session. Create and replay now each add a row to data_plane_session_tokens with its own five-minute expiry, in the statement that writes the session and under the existing idempotency lock. Replay deletes expired rows and keeps at most 16 unexpired tokens per session, evicting those that expire first but never the one it returns. The session row still names the latest token so validators that predate the table keep working. Route resolution uses the presented token's row and its own expiry for attach and for each port or protocol request, and falls back to the session row's token for tokens minted before the table. A row counts only while the session row still names a token, so a revoke by an older server still ends every token. Revoking deletes the session's rows. Replaying the create of a revoked or closed session returns the existing conflict code with the detail reason=session_revoked instead of a token. The replay-credential smoke scenario, parked until this contract was decided, runs in normal selection again.
Overlapping credential validity after a replay is not yet agreed. Mark the section and its coverage row as a proposed contract, link the reviews that deferred the decision, and list the alternatives: an explicit renew endpoint, or a single previous-token grace column.
Collaborator
Author
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
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.
Summary
Proposal, pending agreement on the replay-versus-renewal contract. The review of #79 and the review of #91 asked that command-create replay be defined separately from credential renewal, without committing to overlapping validity, and that the smallest implementation of that contract be chosen. This PR implements one option for discussion. Alternatives:
previous_token_hashgrace column, so only the token before the latest stays valid.The revoked-session guard alone is split out as #811.
With this change, replaying a data-plane session's create with the same idempotency key adds a token instead of replacing the session's only one. Tokens returned earlier stay valid until their own expiry. Replaying the create of a revoked or closed session returns
409 conflictinstead of a token that cannot be used.At the moment a session keeps one attach token, in
data_plane_sessions.token_hash. A replay (same method, path, body and key, within the key's 24 hours) mints a new token and overwrites that hash. Every token returned earlier stops working immediately:201and a token.What changes:
Issuing (API). Create and replay each insert a
data_plane_session_tokensrow (expires_at= now + 5 minutes) in the statement that writes the session, inside the existing idempotency lock. Replay then deletes the session's expired rows and keeps at most 16 unexpired tokens (MAX_LIVE_ATTACH_TOKENS), evicting those that expire first and never the one it returns. Both still writetoken_hash/token_expires_atas the latest token. Create and replay are still six statements each.Revoked or closed sessions. The replay's refresh only matches a session with
revocation_status = 'active'and a state other thanclosed,revokedorfailed. When nothing matches, the replay returns the existingconflictcode (409,retryable: false) with the detail{"key": "reason", "value": "session_revoked"}, and no new error code is added. The row lock taken by that update also orders a replay against a concurrent revoke.Revocation.
revokeSessiondeletes the session's token rows in the transaction that clears the column.Validation (internal API). Route resolution looks up the presented token's row and uses that token's own expiry, both for attach and for the per-request port/protocol ingress check. A token without a row falls back to the existing constant-time comparison against
token_hash/token_expires_at. A row counts only whiletoken_hashis set, so a revoke performed by a server that predates the table (it clears only the column) still ends every token.Docs.
docs/api/index.mdgains "Renew session tokens":expires_atdocs/native-api-hardening.mddescribes this as a proposed contract pending that agreement, links both reviews, and lists the alternatives above.Smoke.
native_contract.command_replay_credentialsasserts that the original credential still attaches after a replay. It isignoredonmainpending the contract decision. This proposal runs it in normal selection, which would commit the API to the overlap guarantee. Its assertions are unchanged.Validation
At
652743e(on #808). The head since adds only the documentation rewording in623c5ae:cargo fmt --all --check: pass.cargo clippy -p pplx-space-api -p pplx-space-internal-api-server -p pplx-space-public-api -p pplx-space-db-migrate --all-features --all-targets -- -D warnings: pass.cargo nextest run --config 'profile.test.package.pplx-space-api.opt-level=0' --all-features -p pplx-space-api -p pplx-space-internal-api-server -p pplx-space-public-api -p pplx-space-db-migratewith nextest's Postgres and Redis: 2385 passed. The one skipped test is the#[ignore]dbenchmark_single_node_occupancy.cargo archwith the base branch's baseline andARCHITECTURE.toml: 0 required findings. It reports one advisory:crates/libs/public/api/src/lib.rsgrew past its recorded size, partly from the two detail constants added here.python3 docs/book/test.pyafterscripts/build-guide-docs.sh: pass.space_smoke/tests: 568 passed, 1 skipped.New tests:
pplx-space-api:closed,revokedorfailedsession is refused.conflictwith thesession_revokeddetail.pplx-space-internal-api-server: route resolution accepts the latest and an earlier table token. It rejects an expired token, another session's token and an unknown token. It accepts a column-only token (the mixed-version fallback) and rejects every token once the column is cleared (an older server's revoke).Each new test was mutation-checked by breaking the code it covers, one mutation at a time. 14 of 15 mutations fail a new test. These include the token insert on create and on replay, the status guard, the revoke delete, the cap bound, expired-row pruning, protection of the returned token, the detail value, the table lookup and each of its three predicates, the column fallback, and using the row's own expiry. The surviving mutation feeds the ingress check the session row's expiry instead of the presented token's. Attach validation has already rejected an expired presented token, and the row always holds the latest expiry, so in practice the two behave the same and no test distinguishes them.
E2E on Kind: a local cluster per
docs/installation.md(install-fc-vendor.sh,bootstrap.sh). It was first built entirely frommain(da5bfd2), then redeployed from this branch, and the migrator applied #808. A disposable script drove the public API on127.0.0.1:8080/:8090:main201,state: revoked, with a token409 conflict,retryable: false,details: [{key: reason, value: session_revoked}]native_contract.command_replay_credentialsfails onmainfor the expected reason (original_status: 401,replay_status: 200) and passes here.Typed smoke on the branch cluster:
bash scripts/tests/smoke-kind-typed.sh nativeran 27 scenarios, all passed (SUITE: 27 passed, 0 warned, 0 failed, 0 inconclusive). These includenative.m1_generated_sdk_conformance,native_contract.token_scope,native_contract.command_replay_credentials,native_concurrency.command_idempotencyand the native recovery scenarios.Not validated: the 409 for a
closedorfailedsession through the API (no public path sets those states, so only the store test covers it); a rolling deploy with old and new pods serving together (covered by the fallback tests, not on Kind).Severity and risk
Severity: moderate. Concurrent or renewing callers receive tokens that are rejected with a non-retryable 401, and port/protocol traffic on a token that was just renewed fails. Sessions are not exposed across tenants or after revocation. On the revoked-replay path the current code hands out a token for a dead session.
Risk: the create, replay and revoke statements change. A bug there affects every native command, terminal, port and protocol session create. Token validation now reads the presented token's row first. A wrong rejection would surface as data-plane 401s, and a wrong acceptance is limited to tokens minted for the same session. Callers that replayed a revoked session's create and used the
201now get a409. To roll back, revert this PR. Tokens minted only into the table then stop working, while the latest token per session keeps working through the column.Rollout
Deploy #808 first. Old and new releases interoperate in either order. An old validator honours only the latest token (the current behavior). An old issuer's tokens validate through the column fallback. A revoke from an old server ends every token, because rows count only while the column is set. Revoking a session from a new server ends every token at once. A token resolved in the last second can still be served from the data plane's one-second route cache, as it can today.
Stack
data_plane_session_tokenstable (basemain).#674 (signed route handover) compares the presented token against the stored
token_hash. It needs the same lookup, or tokens from earlier replays will be rejected on that path.Not covered
data_plane_sessions.token_hash/token_expires_atand the column fallback, in a later migration once no deployed release reads or writes them. No issue is filed yet.AI assistance: Claude Code drafted the code, tests, and this description; the author reviewed them.