Repository navigation
Let the handshake decide the session - #480
Closed
bar-capsule wants to merge 1 commit into
Closed
bar-capsule wants to merge 1 commit into
bar-capsule wants to merge 1 commit into
Conversation
open_session handshakes on a session's first event. A ServerHello negotiates, a refusal denies every step, and no answer applies the startup posture (section 4.1). The decision is stored per session under an HMAC. Signed-off-by: Bar Kaduri <bar@capsule.security>
4 tasks done
9 of 10 tasks
bar-capsule
marked this pull request as ready for review
November 4, 2026 12:32
bar-capsule
requested review from
GangGreenTemperTatum,
Lrd0036,
afogel,
almogbhl,
artmaro,
fewdisc,
mamicidal,
rocklambros,
sclintonowasp and
stefanoamorelli
as code owners
November 4, 2026 12:32
4 tasks done
drk-ram
approved these changes
Nov 4, 2026
drk-ram
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at head f15a190b84bf2b8239bcc3973262ee6682a8712c.
This is the most consequential PR in the stack - it closes the finding that the handshake's result never actually gated the session - and I think the implementation is careful and correct.
The session-record mechanism
open_session/_store_firstinacs/client/_handshake.pyreplace the old "write a plain ServerHello JSON file, and separately touch an empty.failedmarker on failure" scheme with a single first-wins record, published atomically viatempfile.mkstemp+os.link(link fails withFileExistsErrorif a record is already there, so two racing hook processes for the same session can't both win). This directly fixes the bug cited in the PR description where touching an empty.failedmarker was sufting the fail posture - there's no longer a marker file whose mere presence, independent of content, changes behavior.- The record carries an HMAC computed over
derive_session_key(ikm, session_id)as the MAC key and JCS-canonicalized{session_id, guardian_url, decision, cause/fields}as the message (_mac/_record_key). That means a record copied from one session or one Guardian URL to another failsbound = body.get("session_id") == session_id and body.get("guardian_url") == guardian_urland is treated as absent-but-then-caught by the legacy/invalid branch - I checked_read_recordand confirmed an unbound or unauthentic record is never silently accepted, it always resolves toSessionRefused("session_record_invalid", ...), i.e. fails closed rather than falling through to a fresh handshake that an attacker could race. _is_legacyexplicitly detects the pre-this-PR cache format (bareServerHello, noacs_session_recordfield) and refuses it withsession_record_legacyrather than trusting it, which is the right call given the old format's signature didn't cover the Guardian URL at all.- The three-way
Handshakeresult (Negotiated/SessionRefused/NoAnswer) is a real improvement over the olddict | None-_classify_hellocorrectly distinguishes a verified session-refusal error code (is_session_refusaloris_guardian_refusal) asSessionRefused(denies every step of the session, no posture) from a merely-unreachable/malformed response asNoAnswer(defers toACS_STARTUP_POSTURE/ACS_DEFAULT_DENYviaConfig.gate). Config.startup_proceedsdefaulting tonot self.posture.deny_on_failurewhenACS_STARTUP_POSTUREis unset (rather than hard-coding "proceed") is called out as a deliberate deviation from the linked proposal in the PR body, and I agree with the reasoning: a deployment that already fails closed on later decision failures shouldn't quietly start open just because this one variable was never set.
Also notable
acs/integrations/_bootstrap.run_hooknow wrapsmain()itself intry/except Exceptionand treats any escaping exception asadapter_crashed-> fail closed, instead of letting the process exit with an ambiguous nonzero code the host might not interpret as a deny. Combined with the stdin-buffering change (sys.stdin = io.StringIO(raw), so the event name can be recovered for the deny response even aftermain()already consumed stdin and crashed), this closes a real gap where a crash mid-processing could previously look like "no decision" without a clean path to the deny shape.claude_code/hook.py's_top_level_stringhand-rolled scanner (recoveringhook_event_namefrom unparseable JSON by walking quoted tokens at depth 1) is more code than I'd reach for by default, but it's narrowly scoped, fails to""on any ambiguity, and is exercised bytests/common/test_session_gate.py's subprocess-level tests. Not a blocker, just flagging the complexity for anyone touching that function later.tests/common/test_session_gate.pyis genuinely good end-to-end coverage: it drives the installed console scripts as real subprocesses against aProgrammableGuardian, including a down-then-restored Guardian scenario asserting an unguarded session never contacts the restored Guardian for its lifetime, and a refused-session scenario asserting no step is ever sent across multiple hook-process invocations of the same session. That's exactly the right level of test for a cross-process, disk-persisted gating mechanism like this.
No blocking issues. Approving.
Collaborator
|
Closing this slice because PR #22 (the reference adapters for Claude Code, Cursor and NAT) merged into integration as 2630ec2, and that merge carries every slice in this stack. The top of the stack had the same files as PR #22's final commit, so nothing in this slice was left behind. Thanks, Bar, for cutting the adapters into pieces this size, because it made the review possible. |
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.
Stacked on #479. Next: #481. The full stack is listed in #22.
What changed
ACS_STARTUP_POSTUREsettles each step..failedmarker and both handshake TTL settings are gone.main()is audited asadapter_crashedand denied.Ariel's findings this answers
.failedmarker flipped the session's posture. Add adapters/ with Claude Code, Cursor, and NAT reference implementations #22 already verified the cached ServerHello's signature.Where this differs from Ariel's proposal
SessionRefusedcarries a cause label, such assession_refused_response, and the answer's fields, rather than the numeric code.ACS_STARTUP_POSTUREleft unset followsACS_DEFAULT_DENYinstead of defaulting toproceed, so a deployment that fails closed also starts closed.Which issue does this implement
Part of #134
Base branch
Stacked on #479, not on
integration. It lands through #22; see the stack in #22.Type of change
I tested this
At this slice's top (
f15a190b), withclauderemoved from PATH andACS_LIVE_CLAUDEandACS_SPEC_DIRunset: 377 passed and 3 expected skips with the NAT extra, 328 passed and 3 expected skips without it. The skips are the two opt-in live Claude Code tests and the manual Cursor procedure. CI passes on this PR, including the installed-wheel check on Python 3.11 and 3.12.Checklist
git commit -s(required by the DCO)Security
A refused session denies every step whatever the posture, and a session with no negotiated Guardian sends nothing and follows the startup posture.