Skip to content

Let the handshake decide the session - #480

Closed
bar-capsule wants to merge 1 commit into
refimpl/adapters-04-fail-posturefrom
refimpl/adapters-05-session-gate
Closed

bar-capsule wants to merge 1 commit into
refimpl/adapters-04-fail-posturefrom
refimpl/adapters-05-session-gate

Conversation

@bar-capsule

@bar-capsule bar-capsule commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #479. Next: #481. The full stack is listed in #22.

What changed

  • The first event of a session handshakes, and the answer decides the session for its lifetime (§4.1). A verified ServerHello negotiates. A verified refusal, or an answer that fails its binding or signature checks, refuses the session, and every step is denied. No answer leaves the session with no negotiated Guardian: no further requests go out, and ACS_STARTUP_POSTURE settles each step.
  • The decision is stored per session with an HMAC under the session key over the decision, the session and the Guardian URL, written first-wins. A record that is modified, damaged, copied from another endpoint, left by an earlier adapter, or cannot be stored refuses the session. The .failed marker and both handshake TTL settings are gone.
  • Input that does not parse no longer crashes a hook, and an exception that escapes main() is audited as adapter_crashed and denied.

Ariel's findings this answers

Where this differs from Ariel's proposal

  • SessionRefused carries a cause label, such as session_refused_response, and the answer's fields, rather than the numeric code.
  • ACS_STARTUP_POSTURE left unset follows ACS_DEFAULT_DENY instead of defaulting to proceed, 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

  • Reference implementation or adapter

I tested this

At this slice's top (f15a190b), with claude removed from PATH and ACS_LIVE_CLAUDE and ACS_SPEC_DIR unset: 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

  • Commits are signed off with git commit -s (required by the DCO)
  • Prose follows STYLE.md
  • No secrets, tokens, or internal URLs in the diff

Security

A refused session denies every step whatever the posture, and a session with no negotiated Guardian sends nothing and follows the startup posture.

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>

@drk-ram drk-ram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_first in acs/client/_handshake.py replace the old "write a plain ServerHello JSON file, and separately touch an empty .failed marker on failure" scheme with a single first-wins record, published atomically via tempfile.mkstemp + os.link (link fails with FileExistsError if 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 .failed marker 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 fails bound = body.get("session_id") == session_id and body.get("guardian_url") == guardian_url and is treated as absent-but-then-caught by the legacy/invalid branch - I checked _read_record and confirmed an unbound or unauthentic record is never silently accepted, it always resolves to SessionRefused("session_record_invalid", ...), i.e. fails closed rather than falling through to a fresh handshake that an attacker could race.
  • _is_legacy explicitly detects the pre-this-PR cache format (bare ServerHello, no acs_session_record field) and refuses it with session_record_legacy rather 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 Handshake result (Negotiated / SessionRefused / NoAnswer) is a real improvement over the old dict | None - _classify_hello correctly distinguishes a verified session-refusal error code (is_session_refusal or is_guardian_refusal) as SessionRefused (denies every step of the session, no posture) from a merely-unreachable/malformed response as NoAnswer (defers to ACS_STARTUP_POSTURE/ACS_DEFAULT_DENY via Config.gate).
  • Config.startup_proceeds defaulting to not self.posture.deny_on_failure when ACS_STARTUP_POSTURE is 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_hook now wraps main() itself in try/except Exception and treats any escaping exception as adapter_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 after main() 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_string hand-rolled scanner (recovering hook_event_name from 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 by tests/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.py is genuinely good end-to-end coverage: it drives the installed console scripts as real subprocesses against a ProgrammableGuardian, 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.

@rocklambros

Copy link
Copy Markdown
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.

@rocklambros rocklambros closed this Nov 5, 2026
@github-project-automation github-project-automation Bot moved this from In progress to Done in ACS Project Tracker Nov 5, 2026
@bar-capsule
bar-capsule deleted the refimpl/adapters-05-session-gate branch November 8, 2026 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants