fix(session-pid): bind the signed pid mapping to the process incarnation - #8467
Conversation
The session_pid_<pid>.txt mapping and its HMAC sidecar bound only the pid NUMBER, so a recycled pid kept verifying and answered for the new process with the previous owner's session key until the next restart's orphan sweep (issue #8343). Publication now records the process start token (platform_compat.get_process_start_id — the same incarnation identity session_pid.py records in its <gw>:<pid>:<start_token> sweep entries) as a second line of the .txt, the MAC covers the full published body, and both readers refuse on a proven token mismatch. Absent (legacy) or unreadable (Windows) tokens stay unknown, never a mismatch; the unsigned-publish degrade path is preserved. Fixes #8343
github-actions
Bot
added
the
readiness: checking
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All key claims verified. The First-Principles-Verdict: PASS A real misattribution defect (#8343) fixed at its cause — the mapping's identity contract — reusing the existing shared incarnation helper, with zero new public surface. What this change shipsIntent: stop a recycled OS pid from resolving to the previous owner's session key. FIX.
Lens-4/6 counts: incarnation identity already exists as [FIRST-PRINCIPLES-REVIEWED] 65e3de4 |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Binds the mapping to the process incarnation — the actual root cause of #8343 — with byte-compatible legacy MACs and fail-closed-only-on-proof asymmetry. Watch
[DESIGN-REVIEWED] 65e3de4 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Advisory disposition — Design Watch item (head 65e3de4)All four completed lanes PASS on this head (GPT 5.6 no findings, Opus 4.8 no findings, First Principles PASS, Design PASS). Design's PASS carried one Watch item; dispositioned here so nothing advisory is left unanswered: Watch: ADOPTED as a tracked follow-up → #8473. Verified before filing: the Windows gap is real (the docstring's Windows arm returns |
github-actions
Bot
added
readiness: passed
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Binds the pid mapping to the process incarnation the right way: the start token rides a second LINE (correct choice — session keys like dashboard:chat-7-… contain colons, so the sibling record's colon-field scheme could not disambiguate legacy from guarded), and the MAC now covers the whole .txt body so the token is signed while a legacy tokenless body produces a byte-identical message and keeps verifying without migration. The mismatch=refuse / absent-or-unreadable=unknown asymmetry is the load-bearing part and is right on both readers — refusing on the lenient read_session_pid_txt path too is necessary since peer_resolve falls back to it, which would otherwise silently restore the stale attribution. get_process_start_id swallows all exceptions and returns None, so the unsigned/Windows degrade paths cannot break publication, and the recycle check correctly runs AFTER the MAC so an unauthenticated token is never trusted.
github-actions
Bot
removed
the
readiness: passed
2 participants
Symptom
session_pid_<pid>.txtmaps a pid number to a session key, and its HMAC sidecar signs only that number plus the key. Nothing in the file or the signature identifies the process incarnation that owned the pid — so once the OS recycles the pid, the mapping still verifies and answers for the new process with the previous owner's session key. The orphan sweep runs only at gateway start/shutdown, so a mid-run recycle stays misattributed until the next restart.Scope, stated honestly: the mapping directory is same-uid, and the recycled process runs as the same user — this is a robustness/misattribution defect, not a privilege boundary. Fixing it prevents wrong-session attribution (state-mutating MCP tools, audit attribution), not an escalation.
Root cause
_compute_sigMAC'd exactly two fields ("<pid>:<session_key>"), and the.txtcarried only the key — the pid number was the sole process identity anywhere in the contract (src/kiro_crew/session_pid_sig.py).Fix
Bind the mapping to the process incarnation, following the in-tree precedent:
session_pid.py's<gw>:<pid>:<start_token>dual-parse recycle guard (its sweep records the start token at spawn, dual-parses legacy vs guarded entries, treats a mismatch as "this PID now belongs to a different process", and treats an unreadable token as unknown — never a mismatch).publish_session_pid): capture the process start token viaplatform_compat.get_process_start_idand append it as a second line of the.txt. A second line rather than a colon field — unlike the precedent's integer fields, the session key itself contains colons (dashboard:chat-7-…), so a colon split could not tell a legacy key from a key+token pair. Token source note:get_process_start_idis the single shared implementation (already consumed byapps/backend.py,instances/ssh_tunnel_manager.py,metrics/local_exporter.py);session_pid._pid_start_tokenis itself a one-line delegate to it, and importingsession_pidfromsession_pid_sigwould dragproviders.baseinto every sandboxed resolver process, so this module calls the sharedplatform_compatimplementation directly instead of the private delegate."<pid>:<body>"). A legacy body produces a byte-identical message to the old scheme, so every signed mapping written before this change still verifies — no migration, no tamper false-positive. Flipping only the token invalidates the MAC.verify_session_pidandread_session_pid_txt): dual-parse legacy vs guarded forms. A recorded token that is readable live and different is positive evidence of a recycled pid → refuse. An absent recorded token (legacy file) or an unreadable live token (Windows, exited process) is unknown → resolve exactly as today. That asymmetry is the correctness argument and is written as comments at the guard.peer_resolve.py:103-105), so a mismatch surfaced only from the strict path would be silently recovered by the fallback and the stale attribution kept..txtstill published, stale sidecar removed, strict resolvers fail closed (existing + new tests).docs/system-specs/modules/session.mdsidecar-contract section (MAC coverage + recycle-guard bullet).Out of scope, per the issue triage: making the orphan sweep periodic is a separate change with its own scheduling surface (see "Pattern harvest" below).
Verification
dabd83e91) — headlinetest_recycled_pid_refused_by_strict_resolverandtest_recycled_pid_refused_by_lenient_readerboth returned the old owner's key pre-fix. The 4 compatibility guards (legacy file, unknown-token, same-incarnation, sig-coverage) pass on both sides by design. Incarnation is simulated by controlling the token source, never by real pid recycling.test_signature_covers_the_token(1 failed / 46 passed). Verify-side-only variant also run → killed by the round-trip tests.test_legacy_tokenless_file_still_resolves(+2 existing lenient tests).test_recycled_pid_refused_by_lenient_readerwhile the strict headline stayed green (clean isolation).Noneso live host pids can't perturb exact-content assertions); 47/47 total.test_dashboard_peer_auth,test_identity_topology,test_mcp_core_audit_e,test_mcp_core_set_project,test_mcp_cron_caller_identity,test_resolve_session_key,test_sel): green excepttest_dashboard_peer_auth's 4 failures + 2 errors ("AF_UNIX path too long"), reproduced byte-identically on a pristineorigin/mainworktree — environmental.git worktreeatorigin/main, sorted failing-id sets diffed both directions — see the checked line below.flake8andmypyclean on touched files; black gate passes (both files baselined).✅ Zero-regression: branch
99 failed, 80092 passed, 2 errorsvsorigin/mainworktree99 failed, 80083 passed, 2 errors(pass-count delta = the 9 new tests); the sorted failing-id sets (101 ids each) are byte-identical both directions after normalizing one xdist log-interleave artifact (aSuperseded default in stored config:diagnostic glued onto a summary line — present once in each log, on different tests, both of which failed in both runs). This repo's failing baseline is environmental; set identity is the proof, not "0 failures".Pattern harvest
Rule candidate: a persisted pid-keyed record is only as valid as the pid number is stable — any pid map that outlives its process must record the incarnation (start token) and its signature must cover it, with mismatch=refuse / unknown=proceed asymmetry. Knowingly out-of-scope sibling: the orphan sweep still runs only at gateway start/shutdown, so a recycled mapping's files linger until then (the readers now refuse them, which removes the misattribution); attaching the sweep to a recurring task is a separate scheduling-surface change — sibling issue filed and linked in the comments.
Backend-only change (no UI surface); evidence is the red-first test set and mutation kills above — a still frame cannot show a resolver refusal.
Fixes #8343