Repository navigation
Fail closed by default - #495
bar-capsule wants to merge 4 commits into
Conversation
A session whose handshake gets no answer, and a hook whose adapter cannot load, refuse their steps unless ACS_STARTUP_POSTURE=proceed. The example Guardian declares on_decision_failure: deny unless ACS_ON_DECISION_FAILURE=proceed, acs-nat-wire writes default_deny: true, and --all-fail-open in acs-claude-code-wire and acs-cursor-wire writes ACS_DEFAULT_DENY=0 and ACS_STARTUP_POSTURE=proceed. In a negotiated session, a step with no usable decision follows the Guardian's declared posture, or is denied under ACS_DEFAULT_DENY=1. Signed-off-by: Bar Kaduri <bar@capsule.security>
It denies the step unless the hook sets ACS_STARTUP_POSTURE=proceed. Signed-off-by: Bar Kaduri <bar@capsule.security>
When several PreToolUse hooks deny, Claude reads only the reason of the one that finishes last, so another hook's deny hid the ACS reason. The deny now also carries it in additionalContext, which reaches Claude either way. Signed-off-by: Bar Kaduri <bar@capsule.security>
drk-ram
left a comment
There was a problem hiding this comment.
Reviewed at head 243b2682e20c1894aa4e0b4493540bd5256518b2. Approving this slice.
Scope. This is the adapters' half of the #32 default ruling (the leads' 3 November decision, restated in Bar's comment on #32). All 31 changed files are under adapters/. No schema or spec text moves here: §4.1, §6.4, §17.1, the on_decision_failure default in handshake.json, and error.signature in response-envelope.json are untouched, and the description says so. Those belong to the spec PR Bar said he would write for #32 (item 4 of his answer), so I reviewed this for what it does: whether the adapters and the example Guardian fail closed by default, whether the docs say the same thing, and whether it keeps the invariants that ruling needs.
What I checked
Config.startup_proceedsnow returns true only for a trimmed, case-insensitiveproceed. Unset, empty,refuseand a typo such asprocedeall refuse. The table intest_config.pycovers each._bootstrap._settle, the path for a hook whose adapter cannot load, now denies whenfail_closed,posture.deny_on_failureornot startup_proceeds. I droverun_hookdirectly with an unloadable adapter module and an event on stdin. At this head: no setting denies;ACS_DEFAULT_DENY=0andACS_DEFAULT_DENY=1deny;ACS_STARTUP_POSTURE=proceedproceeds, with or withoutACS_DEFAULT_DENY=0;proceedwithACS_DEFAULT_DENY=1denies;ACS_STARTUP_POSTURE=refusedenies;ACS_DISABLED=1still bypasses. At the base (fcc58fe) no setting,ACS_DEFAULT_DENY=0and an explicitACS_STARTUP_POSTURE=refuseall proceeded. The last one was a real hole: the hook ignored a posture the operator had set to refuse. This slice closes it.- The example Guardian reads
ACS_ON_DECISION_FAILUREonce at import and only a trimmedproceedopens it."",deny,openand0all staydeny, and the new test checks that in a subprocess so the import-time read is real. The ServerHello it builds carries the value. - Wiring:
--all-fail-openwrites bothACS_DEFAULT_DENY=0andACS_STARTUP_POSTURE=proceedon every Claude Code and Cursor hook, gates included, and the default wiring writes neither (AllFailOpenOpensEveryHookasserts both directions).acs-nat-wirewritesdefault_deny: trueby default andfalseunder--all-fail-open; the two flags are now a mutually exclusive group and--default-denystays accepted, so existing scripts do not break. - Opting out takes both sides: with the example Guardian declaring
deny,--all-fail-openstill denies a step with no usable decision, and the help text and the Claude Code and Cursor READMEs say so. - The
PreToolUseadditionalContextis added only ondeny, and is built from the samereasonstring aspermissionDecisionReason, so it exposes nothing the Guardian's reason did not already carry. The test pins the equality for both a Guardian deny and a decision failure. - Every posture table, setting row and walkthrough I found in the adapter READMEs,
mapping.mdfiles,settings.json.exampleand thewire.pyhelp text now states the new default. I found no stale "fail-open is the §6.4 default" claim left in the changed docs.
Validation at this head (Windows host, uv, Python 3.11.9, claude not on PATH, ACS_LIVE_CLAUDE and ACS_SPEC_DIR unset)
- Repository root,
uv run pytest -q: 1185 passed, 1 skipped, 10 failed, 20 errors. All 30 are intests/test_closing_choice.py,test_fetch_roadmap.py,test_overrides.py,test_pr_intake_workflow.pyandtest_publish_schemas.py, files this PR does not touch. The same five files give the same 10 failed and 20 errors onintegrationatbba1b84, so I treat them as host effects. uv run --no-dev mkdocs build --strict: exit 0.- Adapters suite with the
natextra: this host cannot run it as shipped, becauseexample_guardian.pyimportsfcntlandacs.testing.harnessusesselect.selecton a pipe, and the.venv\Scriptsconsole scripts have an.exesuffix the security tests do not expect. With two stand-ins kept outside the tree (a no-opfcntland aselectfallback), twelve of the thirteen test files this PR touches (I left the conformance module out) give 229 passed, 30 failed and 338 subtests passed at this head, against 217 passed and 35 failed at the base. Every failure at this head also fails at the base, so the slice adds none. Selecting the PR's own new or changed tests by name (posture,AllFailOpen, unanswered-handshake, declared-proceed, additional-context, startup-posture andcannot_loadcases) gave 15 passed and 4 failed. The 4 are the twocannot_loadtests, which need the POSIX console script, and two parallel-startup subtests; all four fail the same way at the base. I replaced thecannot_loadpair with the directrun_hookprobe above. I did not reproduce the 648/510 counts in the description, and CI is the right witness for those:package (3.11),package (3.12),testsandtests-without-natare green on this PR.
Non-blocking observations
- In both Guardian-down walkthroughs the stdout line sits outside its code fence.
adapters/docs/claude_code/README.md(the{"hookSpecificOutput": ...}line right after the closing fence, about line 223) andadapters/docs/cursor/README.md(the{"permission": "deny", ...}line, about line 234) both close the fence after the twoACS_AUDITlines and leave the JSON as loose paragraph text. Move the closing fence below the JSON line. ACS_STARTUP_POSTURE=proceedandACS_DEFAULT_DENY=1combine differently on the two failure paths. For a session whose handshake got no answer,proceedwins (test_startup_posturepins{"proceed", "DEFAULT_DENY": "1"}as true). For a hook whose adapter cannot load,ACS_DEFAULT_DENY=1wins (my probe above). It is probably intended, since the second path has no session to apply a posture to, but a reader of theACS_DEFAULT_DENYrow, which says "fail-closed whatever the Guardian declares", would not guess it. One sentence in the settings table would settle it.- The description is right that a ServerHello with no
on_decision_failurestill negotiatesproceeduntil the §6.4 andhandshake.jsonPR lands. Until then a third-party Guardian that omits the field leaves the non-gate hooks with no local floor. That is the intended staging, but I would link the spec PR from the adapter README's degraded-Guardian paragraph once it exists, so the fallback is visibly temporary.
The behaviour is correct, the opt-out is explicit on both sides, and the tests pin both directions of every change. Approving.
…e NAT middleware by _type An unset ACS_DEFAULT_DENY, and NAT's default_deny, now mean deny. Only ACS_DEFAULT_DENY=0 (default_deny: false) opts into proceed, and a ServerHello that declares deny still overrides it, so the Guardian can only tighten the posture. A Cursor preToolUse MODIFY answers updated_input with no permission key, so Cursor keeps its sandbox and approval for the rewritten call. acs-nat-wire finds the ACS middleware by _type: acs_guardian under any name and lists that name, so an existing guard is never added twice. --write stops when acs_guardian names a middleware of another type. Signed-off-by: Bar Kaduri <bar@capsule.security>
|
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. |
Stacked on #489. Last slice of the stack. The full stack is listed in #22.
What changed
The leads decided on 3 November that the default is fail-closed, configurable either way.
ACS_STARTUP_POSTURE=proceed(§4.1). Before, an unset startup posture followedACS_DEFAULT_DENYand proceeded. A hook whose adapter cannot load follows the same rule.on_decision_failure: deny;ACS_ON_DECISION_FAILURE=proceedmakes it declareproceed.ACS_DEFAULT_DENY=1denies it whatever the Guardian declares. The wired gate hooks keepACS_DEFAULT_DENY=1in their command.acs-nat-wirewritesdefault_deny: true, and--all-fail-openwritesfalse.--all-fail-openinacs-claude-code-wireandacs-cursor-wirewritesACS_DEFAULT_DENY=0 ACS_STARTUP_POSTURE=proceedon every hook; before, it wrote no posture.PreToolUsedeny also carries its reason inadditionalContext. When severalPreToolUsehooks deny, Claude reads only thepermissionDecisionReasonof the one that finishes last, so a project's own hook could hide the ACS reason; live, with a second hook that denied 2 s after ACS, Claude received only the project's reason before the change and both after it.§6.4 and
handshake.jsonstill giveproceedas the default ofon_decision_failure, so a ServerHello without the field still negotiatesproceed; that changes with the §6.4 PR for #32.Which issue does this implement
Part of #134
I tested this
At this slice's top (
243b2682), withclauderemoved from PATH andACS_LIVE_CLAUDEandACS_SPEC_DIRunset: 648 passed and 3 expected skips with the NAT extra, 510 passed and 3 expected skips without it. The skips are the two opt-in live Claude Code tests and the manual Cursor procedure. The installed-wheel check passes on Python 3.11 and 3.12,lint-importskeeps both contracts, the repository suite passes 1205 with 1 skip, andmkdocs build --strictpasses. CI passes on this PR.Live run of the wheel built from this slice's code (sha256
12ccf4d71445b7617e03e9eede82a62f4391dddb22d566c2eadd3aab204cef0c) in Claude Code 2.1.318 and NAT 1.7.0, every request through the recording proxy and the checker: with the Guardian down when the session starts, every hook refused its step (session_not_started_fail_closed) and the command did not run; with the Guardian silent on tool results, Claude's reply left out the file's contents when the Guardian declareddeny(decision_failure_fail_closed) and included them when it declaredproceed(fail_open_bypass). Across the full harness, 15 recordings, the checker found 0 problems.Base branch
Stacked on #489, not on
integration. It lands through #22; see the stack in #22.Type of change
Checklist
git commit -s(required by the DCO)Security
A session with no negotiated Guardian, and a hook whose adapter cannot load, deny their steps by default, and the example Guardian declares
on_decision_failure: deny.