Skip to content

fix(security): re-judge a remote hosts alias after a mid-parse publish - #20861

Closed
jeeshofone wants to merge 1 commit into
kirodotdev:mainfrom
jeeshofone:fix/20842-ssh-self-host-isolation
Closed

jeeshofone wants to merge 1 commit into
kirodotdev:mainfrom
jeeshofone:fix/20842-ssh-self-host-isolation

Conversation

@jeeshofone

@jeeshofone jeeshofone commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Problem / Motivation

Goal: On a cold worker, the first ssh to a dotless name the hosts file maps to a remote address (10.4.4.4 devbox, then ssh devbox uptime) is allowed even when the own-host enrichment worker publishes while that call is parsing the hosts file.

Follow-up to #20964. That PR made the in-call parse hand back its table uncached when only the own-address half of the hosts-cache key moves mid-parse, so a name the file does not hold (the case #20842 reported) or a local entry is answered from it. A remote entry in that table was judged against the older own set, so the own-state re-check before return still refuses it as pending. Measured with the real enrichment worker in fresh processes: with the parse slowed to 20 ms, main refuses ssh devbox uptime in 20 of 20 cold calls, and in 19 of 20 at 200 ms. This head allows it in 20 of 20 at 0, 20 and 200 ms. An alias for this host's own address is refused 20 of 20 on both. Reported as #21228.

Sensitive path: src/kiro_crew/security/argv_floor.py.

Why it matters

A user whose hosts file names a remote machine gets a false sandbox-escape refusal on the first ssh after the gateway starts, and the same command passes on the next call. The refusal comes from a security gate, so it reads as an attack rather than a race.

Not a goal

  • Changing fix(security): keep a hosts-file miss open when the own set grows mid-parse #20964's split of the key or its uncached-table answers for absent and local names.
  • Judging again a file that changes while it is parsed, or a failed re-stat. Both stay pending at once, as on main. A file still being written is not a reason to parse it again on the gate path, and this keeps one retry path.
  • Changing which names count as self, the deny-floor direction of a name that maps local on any line, or the enrichment worker.

What changed (motivation → approach → change)

Symptom: on a cold worker, the first ssh to a remote hosts-file alias is refused as sandbox-escape-ssh-self and passes on the next call.

Root cause: _hosts_file_verdict judges each table under the key it read before parsing. When the worker publishes mid-parse, that table is uncached and judged against the older own set. For a remote entry, the re-check (key[-2], key[-1]) != own_state then answers pending.

Change (_hosts_file_verdict only): main's body runs inside one loop of at most _HOSTS_KEY_MOVED_ATTEMPTS = 3 passes. The only retry is the case above, a remote entry whose table's own state no longer matches at return: every path is judged again on the current key. After the last pass the answer is pending and the warm is scheduled, as before. Every other exit is main's, unchanged: local, absent, unreadable, over-cap, unverifiable Windows, a file changed mid-parse, and a failed re-stat.

  • Why three passes: one enrichment pass moves the key at most twice (the netlink publish, then the DNS merge), so a third pass sees a settled key.
  • Why not serve the old table: a remote entry judged before publication can be an alias for the newly published own address. The retry judges it under the new own set and refuses it.
  • Why not wait for the worker: the gate never blocks on it. The retry costs at most three parses of one file per call, and only when the own state moves during that call.

State: no new state. Each pass reads the key, the cache and the own state afresh. The enrichment worker (publication and own set) and the warm thread (cache) are the other writers, as on main. The return check still compares the own state at return with the key each remote table was judged under.

Docstrings: _hosts_file_verdict states the bound (at most _HOSTS_KEY_MOVED_ATTEMPTS passes per call). _parse_and_cache_for_key says the gate judges a moved-key remote entry again.

Line budget: the security package stays at 29,411 lines, exactly main's budget, which is not raised.

Backwards compatibility

Compatible: every answer main gives is unchanged, except a remote hosts-file entry whose own state moves during the call. That one is judged again on the current key, instead of being refused as pending. A name that maps local on any line is still refused.

Removes nothing: no function, constant, key, field or record goes away.

Tests

In TestHostsFileWarmUp (test/test_denied_commands_security.py):

  • test_a_cold_ssh_to_a_remote_alias_is_allowed_when_publication_lands_mid_parse (new, the regression): the worker's real publish step runs inside the first gate parse, with no clock. ssh farbox uptime is allowed end to end, after exactly two parses, with the new key cached and no warm.
  • test_the_second_judgement_uses_the_newly_published_own_set (new): an alias for the address published mid-parse is refused on the retry.
  • test_an_own_set_that_moves_before_a_remote_answer_is_judged_again (new).
  • test_a_key_still_moving_after_every_attempt_is_pending (new): exactly three parses, then pending, nothing cached, one warm.
  • test_a_file_edited_during_the_gate_parse_is_pending_at_once and test_a_failed_re_stat_after_the_parse_is_pending_at_once (new): one parse, pending, one warm.
  • test_own_set_merge_after_the_key_check_is_a_deny (changed pin): the merged set is judged in the same call, so the alias is cached local instead of a warm being scheduled.

On main 5 of these fail. Mutations, each turning tests red (count in brackets):

  • 1 pass (5);
  • 2 passes (1);
  • a re-stat OSError skipping the path (1);
  • a changed file retried (2);
  • exhaustion answered remote (2);
  • remote served without the own-state check (6);
  • the check comparing publication only (4).

Gates on this head: the 12 test files that name argv_floor, test_mcp_cron_security.py, the 17 ratchet files, test_agent_refactor_facade.py and test_flake_pattern_ratchet.py at -n 4 (4505 passed, 17 skipped). Also black (the repo gate), isort, flake8, mypy on src/kiro_crew/ (CI's command), COMMENT_HISTORY_BASE_REF=origin/main scripts/check_comment_history.py, and Semgrep 1.78.0 with the repo rules (0 findings on changed lines).

Manual verification

The race was reproduced in fresh processes with the real enrichment worker, only the parse slowed by a sleep. main refused the remote alias 20 of 20 at 20 ms and 19 of 20 at 200 ms, and this head allowed it 20 of 20 at 0, 20 and 200 ms. An alias for the host's own address was refused 20 of 20 on both. #20842's own case (ssh -o ConnectTimeout=10 backup@host uptime) is allowed 20 of 20 on both.

Related Issues

Fixes #21228
Follow-up to #20964, which covers the case #20842 reported
Known limit, not changed here: #21051 (on Linux a same-size rewrite within one coarse ctime tick can keep the hosts-cache key; the same on main).
#17666 moves this code to its own module. Whichever lands second re-homes the loop.

Pattern harvest

Rule candidate: review-prompt
Pattern: a check-then-use answer of "pending" returned on the first race, when re-reading the moved state and judging again a bounded number of times would answer correctly.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

Contributions are accepted under the project's Apache-2.0 license; see the Licensing section of CONTRIBUTING.md.

@jeeshofone
jeeshofone requested review from a team and KiroCrewCaptain as code owners October 15, 2026 15:07
@jeeshofone
jeeshofone requested a review from bolichen97 October 15, 2026 15:07
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

UX Review (Opus 5.5, fork) — ⏭️ skipped

Revision e1fb3049f4f5d51ffd69c2ad1a485e21c079ef3c touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Design Review (Opus 5.5, fork) — ✅ PASS

Design-level review of e1fb3049f4f5d51ffd69c2ad1a485e21c079ef3c via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Check that up to three hosts-file parses per gate call on the event loop stay within budget when the file is near _HOSTS_FILE_READ_CHUNK.

[DESIGN-REVIEWED] e1fb304

Description read

[DESCRIPTION-READ] 3ebf907506f370903c7d5bcccb29a410bdb667ab8bb2ba492219cec2370e5bf3

That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

First Principles Review (Opus 5.5, fork) — ✅ PASS

Premise-level review of e1fb3049f4f5d51ffd69c2ad1a485e21c079ef3c via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: PASS

Before merge, check the claim behind _HOSTS_KEY_MOVED_ATTEMPTS = 3: "one enrichment pass moves the key at most twice". If it is wrong, the call stays pending, as main does today.

What this change ships

Inventory (4 items) — 4 justified

Intent: the first ssh to a remote hosts-file alias on a cold worker should be allowed, not refused as ssh-to-self, when the own-host worker publishes mid-parse (#21228). This is a FIX.

  1. A remote hosts-file alias whose own-address state changes during the call is judged again on the current key, not refused as pending — justified
  2. Re-judging is capped at three passes (_HOSTS_KEY_MOVED_ATTEMPTS), then pending with the warm scheduled, as on main — justified
  3. An own-set merge after the key check is now judged local in the same call and cached, with no warm scheduled. The pinned deny still holds — justified
  4. Docstrings for _hosts_file_verdict and _parse_and_cache_for_key now describe the retry instead of the pending answer — justified

[FIRST-PRINCIPLES-REVIEWED] e1fb304

Description read

[DESCRIPTION-READ] 8a8038ea268819c48d7ed5748176885f28d188a37d36fd12393aac96ce892aca

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Opus 5.5 Review (fork) — ✅ no blocking findings

Reviewed e1fb3049f4f5d51ffd69c2ad1a485e21c079ef3c via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] e1fb304

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

GPT 6.1 Review (fork) — ✅ no blocking findings

Reviewed e1fb3049f4f5d51ffd69c2ad1a485e21c079ef3c via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e1fb304

@jeeshofone
jeeshofone force-pushed the fix/20842-ssh-self-host-isolation branch from d57cbef to 9dd409a Compare October 15, 2026 15:27
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Head 9dd409a (rebased on 5460e2f).

An independent security review of d57cbef approved it with no blocking or should-fix findings. The retry opens no false allow: a remote answer still needs every table parsed on its own key, a matching post-parse re-stat, and publication plus the own set unchanged at return. The root cause was confirmed with the real enrichment thread. On main a descheduled cold parse denied 30 of 30 times, and on the fix 30 of 30 were allowed. Nine mutations and seven attacks were tried; every one except NIT-1's mutant was killed or still denied.

Both nits are fixed on this head:

  • NIT-1 (_hosts_file_verdict): an OSError from the post-parse re-stat was mapped to "changed" and retried, so a hosts file that vanished could end as absent. It now answers pending at once, as on main, because it is a read failure, not a moved key. test_a_failed_re_stat_after_the_parse_is_pending_at_once pins it. The test fails with the retry mapping (3 parses instead of 1) and passes on this head.
  • NIT-2: the docstring states the bound: at most _HOSTS_KEY_MOVED_ATTEMPTS parses per call (about 55 ms on the loop at 64 KB, 3.3 s off it at 4 MB), and in practice only at cold start.

The security package stays at its line budget, and the budget test passes. Gates on this head: -k "HostsFile or hosts or ssh" 337 passed. TestHostsFileWarmUp, test_mcp_cron_security.py and the gate-liveness tests gave 717 passed and 3 failed. Those 3 are the fd-closed assertions in test_mcp_cron_security.py, which fail the same way on the parent commit on the test host and do not touch argv_floor. mypy, black gate, isort, flake8, comment-history and Semgrep 1.78.0 (0 findings on changed lines) are clean.

Rulings for this round: NIT-1 is done at src/kiro_crew/security/argv_floor.py:1706, pinned at test/test_denied_commands_security.py:10839. NIT-2 is done at argv_floor.py:1677-1681. The line budget holds: the package is unchanged in line count, with the docstring tightened to make room.

First-pass check: R1, R3-R8 OK. R2: argv_floor.py is a sensitive path, which this fix needs and the body discloses. No new audit id was added, and the secret-pattern grep prints nothing.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Security Scope Review (fork) — ✅ PASS

Which legitimate operations does e1fb3049f4f5d51ffd69c2ad1a485e21c079ef3c newly refuse? Model proposes, scripts/deny_diff.py decides — updated in place on each push. A script-confirmed regression blocks PR readiness.

adjudicated, zero confirmed regressions

Scope-Verdict: PASS

TIGHTENING: the hosts-file self-host check _hosts_file_verdict in the sensitive-command tier (sensitive-bash, ssh/scp/rsync-to-self). The one threat it refuses is a dotless hosts-file alias for this machine being served as "remote" from a table judged before the own-address set changed. This change loosens that check rather than tightening it. The old code refused immediately when the own state had moved: if any((key[-2], key[-1]) != own_state for key in remote): _schedule_hosts_file_warm(); return True. The new code checks again on the current key, for _attempt in range(_HOSTS_KEY_MOVED_ATTEMPTS): … if all((key[-2], key[-1]) == own_state for key in remote): return False if own_state[0] else None. It only refuses after _HOSTS_KEY_MOVED_ATTEMPTS = 3 attempts still find the state moving: _schedule_hosts_file_warm(); return True. Every refusal the old code made outside that retry path (unreadable file, file changed mid-parse, a large file on the event loop, any local entry) is unchanged.

ADJUDICATED: 10 candidates, 0 confirmed newly-refused

PLATFORM COVERAGE:

  • any: the dotless short-name ssh forms (ssh dev-dsk, user@buildbox with -o, -J bastion appbox, the scp-style git clone gitbox:). These all reach the changed dotless-host branch of _host_is_self. Two neighbouring controls are included: an IP literal and a dotted FQDN, which take adjacent branches and must not move.
  • posix: scp and rsync host:path remote operands, which are split before they reach the same check.
  • windows: ssh.exe by its executable name, and an scp copy using a .\ source path and a C:/ remote path. These cover the Windows client spelling and the Windows hosts file read through _hosts_file_paths.
  • Paths and parsing: the diff adds no new path, separator, interpreter or environment-variable assumption. It only changes the retry and pending logic inside an existing function.
  • Coverage gap: the new retry branch only runs when the enrichment worker publishes, or the own-name set grows, during a single check. A classification of a fixed string on an idle host normally never reaches that branch. These rows prove the ordinary dotless-host path is not newly refused. They do not exercise the race itself, which only the PR's own TestHostsFileWarmUp tests cover.

[SCOPE-REVIEWED] e1fb304

Security scope review -- adjudicated candidates

  • macos-latest (platform posix): 8 classified, 0 newly refused, 7 still allowed.
  • ubuntu-latest (platform posix): 8 classified, 0 newly refused, 7 still allowed.
  • windows-latest (platform windows): 8 classified, 0 newly refused, 7 still allowed.

No candidate flipped from allowed to refused.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 15, 2026
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Sensitive Change Review cannot pass on a fork PR: src/kiro_crew/security/argv_floor.py is a sensitive path, so a maintainer needs to take this over on a branch of this repository (keep the author with Co-authored-by: and write Supersedes #20861), or approve the head with the reasoning template. Filled-in values for 9dd409aaa, for the approval body:

Sensitive change review

  • What rule changed: sandbox-escape-ssh-self's hosts-file verdict (_hosts_file_verdict) re-judges the file on the new key when the own-host cache key moves during the call, up to _HOSTS_KEY_MOVED_ATTEMPTS = 3, then answers pending as before.
  • Before -> after: before, any key move during the parse answered pending, so a cold worker refused a non-self dotless ssh target (19/20 cold calls with a 20 ms parse); after, the check is re-run on fresh state and only answers remote when every table was judged against the own set in its own key and nothing moved at return.
  • Worst case if wrong: a self name judged remote, i.e. an ssh-to-self escape allowed. The independent security review found no such path (every attack still denies or answers on fresh state).
  • Evidence checked: real-thread reproduction of the race on main (30/30 denied) and on the fix (0/200); 4 new race tests plus the re-stat pin; mutation table in the review (attempts 3 -> 1 killed; post-parse OSError pinned to immediate pending).
  • No regression:
    • Tools still work: hosts-file, ssh and cron-security suites pass (-k "HostsFile or hosts or ssh" 337 passed); only host-only failures that also fail on bare main.
    • Backward compatible: read failures, files over the cap, unverifiable Windows files and a failed post-parse re-stat still answer pending at once, as on main.
    • Existing tests: unchanged except one pin (own_set_merge_after_the_key_check still denies; it now caches the alias as local in the same call).
  • How to undo: revert the single commit; behaviour returns to main's (fail-closed false denies on cold workers).

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Oct 15, 2026
@jeeshofone
jeeshofone force-pushed the fix/20842-ssh-self-host-isolation branch from 9dd409a to f71bbdf Compare October 16, 2026 02:00
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Rebased onto main after #20964 merged (it edits the same hosts-file parse path). New head: f71bbdfd7.

What #20964 already covers: a name the hosts file does not hold, when only the own-address half of the key moves mid-parse. That is the allow-case #20842 reported. With the real enrichment thread and a 0.2 s parse, current main allows it in 10 of 10 cold calls; the commit before #20964 refused it in 10 of 10.

What this PR still covers, rebuilt on #20964's shape (one loop around main's body; no second retry path, _parse_and_cache_for_key keeps #20964's split): a name the file maps to a remote address, judged under the older own set, and a file that changes mid-parse. Both still answer pending on main: the remote alias is refused in 9 of 10 cold calls at a 20 ms or 0.2 s parse, 7 of 10 at 1 s. On this head it is allowed in 30 of 30, and an alias for this host's own address is still refused in 10 of 10.

Security review NITs on d57cbef: NIT-1 (re-stat OSError answers pending at once, pinned by test_a_failed_re_stat_after_the_parse_is_pending_at_once, and the retried mutant goes red) and NIT-2 (the bound is in the _hosts_file_verdict docstring) are both in.

The four behaviour tests fail on current main. #20964's own test passes unchanged. 7 of 7 mutations turn a test red. The security package stays at its 29,411-line budget; the budget is unchanged.

First-pass check: R1-R8 OK (sensitive path security/argv_floor.py is inherent to the fix and declared in the body, same as #20964; no new audit id; one commit).

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Oct 16, 2026
@jeeshofone
jeeshofone force-pushed the fix/20842-ssh-self-host-isolation branch from f71bbdf to a39fb8a Compare October 16, 2026 03:00
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for the independent security review of f71bbdfd7 (verdict APPROVE, 0 MUST-FIX, 2 SHOULD-FIX, 2 NIT). New head: a39fb8a2fd, rebased on fe87f5ba99 (no change on the security files).

SHOULD-FIX 1, a file rewritten constantly triples the event-loop stall: fixed. The retry budget is now split by cause in _hosts_file_verdict. An own-address move keeps up to _HOSTS_KEY_MOVED_ATTEMPTS = 3 parses per call. A "changed" result (the file moved mid-parse) gets at most one re-parse on the event loop and then answers pending, fail closed, with the warm scheduled; off the loop it may use the full budget. New tests: test_a_file_rewritten_during_every_parse_is_parsed_twice_on_the_loop (exactly 2 parses, pending, nothing cached, one warm) and test_a_file_rewritten_during_every_parse_uses_the_full_budget_off_the_loop (3 parses). Removing the on-loop cap turns the first one red. The reviewer's probes, same host, before and after:

  • writer thread rewriting a 60 KB file in a loop, 200 calls: 3.00 -> 2.00 parses per call, 58.4 -> 38.8 ms per call, 200 pending in both;
  • 64 KB file changed during every parse, on the loop: 3.03x -> 2.01x one parse (64.3 -> 42.7 ms, one parse 21.2 ms);
  • 4 MB, off the loop: 3.00x / 3.02x one parse, unchanged by design.

SHOULD-FIX 2, absolute cost figures in the docstring: fixed. The _hosts_file_verdict docstring states the bound as "up to 3x one parse" with no absolute figures, and the _HOSTS_FILE_READ_CHUNK comment drops "one at the cap in a few" and says "one parse of at most one chunk".

NIT 1, test_the_second_judgement_uses_the_newly_published_own_set survives M5: fixed. It now asserts _hosts_file_verdict("ownalias") is True and the parse sequence [False, True] before the _denied_by checks. With the own-state re-check replaced by if True: it goes red.

NIT 2, the re-stat test stubbed the parse: fixed. test_a_failed_re_stat_after_the_parse_is_pending_at_once now runs a real parse and makes the real second os.stat of the hosts path raise PermissionError; it asserts pending, exactly 2 stats and one warm. Retrying a re-stat OSError turns it red.

OUT-OF-SCOPE, same-size rewrite within one coarse ctime tick: filed as #21051. It reproduces the same way on main, so it is linked from the body as a known limit and not changed here. The issue lists the three fix options from the review.

Mutations on a39fb8a2fd (red tests in TestHostsFileWarmUp): 1 attempt (8), 2 attempts (2), re-stat OSError retried (1), exhaustion answered remote (3), own-state re-check removed (7), on-loop file-change cap removed (1).

Gates: the five test_denied_commands_* files, test_security_gate_liveness.py, test_argv_floor_inline_and_brace_scope.py, the kill-chokepoint, spawn-audit and ratchet-scope ratchets and test_mcp_cron_security.py: 2561 passed, 1 skipped. The ssh cases of test_mcp_cron.py and test_mcp_cron_security.py: 20 of 20 runs, 45 passed each. The security package stays at 29,411 lines, its budget unchanged. mypy, the black gate, isort, flake8 and the comment-history check are clean; Semgrep with the repo rules has 0 findings on changed lines. One commit.

First-pass check: R1-R8 OK. R2: src/kiro_crew/security/argv_floor.py is a sensitive path, as stated in the body. AUTOSDE rules matched: no-blocking-call-on-event-loop (this round cuts the on-loop parses for a moving file from 3 to 2); an-invariant-line-needs-a-mutation-pin (each budget line is pinned by a mutation above); tests-are-deterministic (the new tests count parses and use no clock).

On a cold worker the first dotless ssh check starts the own-host
enrichment worker and parses the hosts file in the same call. When the
worker publishes while that parse runs, the table comes back uncached,
judged against the older own set. An absent name or a local entry is
answered from that table; a remote entry was refused as pending because
its key no longer matched the own state at return.

_hosts_file_verdict now judges every path again on the current key in
that one case, at most _HOSTS_KEY_MOVED_ATTEMPTS passes per call, and
answers pending after that. A remote answer is still served only when
publication and the own set at return match the key its table was judged
under, so an alias for a newly published own address is refused on the
retry. A file that changes mid-parse and a failed re-stat stay pending at
once.

Fixes kirodotdev#21228
Follow-up to kirodotdev#20964
@jeeshofone jeeshofone changed the title fix(security): re-judge the hosts file when the own-host key moves mid-check fix(security): re-judge a remote hosts alias after a mid-parse publish Oct 16, 2026
@jeeshofone
jeeshofone force-pushed the fix/20842-ssh-self-host-isolation branch from a39fb8a to e1fb304 Compare October 16, 2026 13:12
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 16, 2026
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Security Scope CONCERNS (on 9dd409aaa, the reviewer's own judgement, no confirmed regression): answered on e1fb3049f4.

On this head the retry is narrower than the one that review read. It runs in one case only: a remote entry whose table was judged under an own state that no longer matches at return. A file that changes mid-parse and a re-stat that raises OSError are pending at once, exactly as on main. The except OSError: result = "changed" line is main's own, and a "changed" result is never retried.

Exit by exit:

  • The first pass is main's body, unchanged, so wherever main answers False or None this head gives the same answer on that pass.
  • Every return True in the loop is main's, on the same condition.
  • The final return True after the last pass is main's answer for the same moved-own-state remote entry.
  • A later pass runs only for a call main answered pending. It gives the gate's answer on the current key, and a remote answer only when publication and the own set at return equal the key its table was judged under.

So no input that main allows is refused here. The one class main refuses and this head allows is a remote hosts-file alias whose own state settles within _HOSTS_KEY_MOVED_ATTEMPTS passes.

The classifier cannot move the key mid-call, so the branch is proven by tests, not candidates.

  • test_a_cold_ssh_to_a_remote_alias_is_allowed_when_publication_lands_mid_parse runs the worker's real publish inside the first gate parse.
  • test_the_second_judgement_uses_the_newly_published_own_set keeps an alias for the published own address refused.
  • test_a_key_still_moving_after_every_attempt_is_pending, test_a_file_edited_during_the_gate_parse_is_pending_at_once and test_a_failed_re_stat_after_the_parse_is_pending_at_once pin the pending exits.

Mutations that turn them red, with the red count in brackets:

  • remote served without the own-state check (6);
  • the check comparing publication only (4);
  • exhaustion answered remote (2);
  • a changed file retried (2);
  • a re-stat OSError skipping the path (1).

With the real enrichment worker in fresh processes, an alias for the host's own address is still refused 20 of 20.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Head e1fb3049f4, rebased on 12835a82ce and rescoped as a follow-up to #20964. It now fixes #21228, filed for the residual. #20842's reported case (an absent name) is the one #20964 covers, so this PR no longer claims it.

What changed from a39fb8a2fd. argv_floor.py is rebuilt from main's file, and the whitespace-ignoring diff is 14 lines in, 14 out. One retry path is left: a remote entry whose own state moved before return is judged again on the current key, at most _HOSTS_KEY_MOVED_ATTEMPTS = 3 passes per call. The re-parse of a file that changed mid-parse is removed, along with its event-loop cap and its two tests. Such a file is pending after one parse, as on main.

Status of the independent security reviews' items on this head:

  • d57cbeffd NIT-1, an OSError re-stat pending at once: done. Main's except OSError: result = "changed" is kept and is never retried. test_a_failed_re_stat_after_the_parse_is_pending_at_once pins it; it goes red if that path is skipped or if a changed result is retried.
  • d57cbeffd NIT-2, the docstring states the bound: done. _hosts_file_verdict says at most _HOSTS_KEY_MOVED_ATTEMPTS passes per call, so at most that many parses of one file.
  • f71bbdfd7 SHOULD-FIX 1, a constantly rewritten file triples the stall: resolved by removing the file-change retry. That file costs one parse per call, as on main, pinned by test_a_file_edited_during_the_gate_parse_is_pending_at_once.
  • f71bbdfd7 SHOULD-FIX 2, absolute cost figures: this diff states the bound in passes and adds no figures. The _HOSTS_FILE_READ_CHUNK comment is main's text and is not touched here.
  • f71bbdfd7 NIT 1: kept. test_the_second_judgement_uses_the_newly_published_own_set asserts the verdict and the parse sequence, and goes red when the own-state check is removed.
  • f71bbdfd7 NIT 2: the re-stat test runs a real parse. The failing re-stat is injected through the module's own _hosts_file_key seam rather than by patching os.stat, so the stdlib module shared by every thread in the worker is left alone.
  • Hosts-file cache key misses a same-size rewrite within one coarse ctime tick on Linux #21051 (same-size rewrite within one ctime tick) stays a known limit, linked from the body.

Tests and proof. On main, 5 of the hosts tests fail, the new regression among them. Mutations, each turning tests red (count in brackets):

  • 1 pass (5);
  • 2 passes (1);
  • a re-stat OSError skipping the path (1);
  • a changed file retried (2);
  • exhaustion answered remote (2);
  • remote served without the own-state check (6);
  • the check comparing publication only (4).

The cold-process repro with the real worker: main refuses ssh devbox uptime 20 of 20 at a 20 ms parse and 19 of 20 at 200 ms; this head allows it 20 of 20 at 0, 20 and 200 ms. An own-address alias is refused 20 of 20 on both, and #20842's case is allowed 20 of 20 on both.

Gates on e1fb3049f4:

  • the 12 test files that name argv_floor, plus test_mcp_cron_security.py, the 17 ratchet files, test_agent_refactor_facade.py and test_flake_pattern_ratchet.py at -n 4: 4505 passed, 17 skipped;
  • mypy src/kiro_crew/: no issues in 2596 files;
  • the repo black gate, isort and flake8;
  • the comment-history gate, passed;
  • Semgrep 1.78.0 with the repo rules: 0 findings on changed lines;
  • the description gate, passed (body 7,556 bytes);
  • one commit; the security package at 29,411 lines, its budget unchanged.

First-pass check: R1-R8 OK.

  • R1: every hunk serves the Goal.
  • R2: argv_floor.py is a sensitive path, as the body says.
  • R3: no new state; each pass re-reads the key, the cache and the own state.

AUTOSDE rules matched:

  • backend-security-controls: fail closed on every exit except the judged-again remote.
  • no-blocking-call-on-event-loop: at most _HOSTS_KEY_MOVED_ATTEMPTS chunk-bounded parses per call on the loop, only while the own state moves.
  • an-invariant-line-needs-a-mutation-pin: the mutation list above.
  • tests-are-deterministic: the new tests count parses and key reads and use no clock.
  • a-bound-bounds-every-field-it-retains: no retained structure is added.
  • recorded-decisions-are-not-regressions: nothing under docs/decisions/ covers the hosts file.

For the maintainer's Sensitive Change Review on this head:

  • What rule changed: sandbox-escape-ssh-self's hosts-file verdict (_hosts_file_verdict) judges every path again on the current key when a remote entry's table was judged under an own state that has since moved, at most 3 passes per call, then answers pending as before.
  • Before -> after: before, a cold worker refused a remote hosts alias as pending whenever the own-host worker published during the in-call parse. After, it is judged under the published own set and allowed when remote. Every other answer is unchanged.
  • Worst case if wrong: a self name judged remote. The retry serves remote only when publication and the own set at return equal its table's key, and the own-address alias is refused in the tests and in 20 of 20 cold runs.

Issue Gate: #21228 is waiting for triage; one request is posted on it. Independent review: left to the loop.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 16, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to ship. Head reviewed: e1fb304.

What I checked by reading the diff and _parse_and_cache_for_key at this head:

  • The only new answer is a remote verdict (False) on a later pass. It needs a table parsed for its own key, a post-parse re-stat that matches the file half of that key, and publication plus the own set at return equal to that key. That is the same guard main applies on its single pass.
  • Every other exit is main's, on the same condition: unreadable key, unverifiable Windows file, a large file on the event loop, a file that moved mid-parse, a re-stat OSError (mapped to "changed", never retried), and any local entry.
  • A later pass re-reads the key, so it misses the cache and parses under the current own set. An alias for a newly published own address is judged local on that pass. test_the_second_judgement_uses_the_newly_published_own_set pins it.
  • The loop is bounded at _HOSTS_KEY_MOVED_ATTEMPTS = 3 passes, and exhaustion is pending with the warm scheduled, as on main. On the event loop each pass is capped at one chunk, so the worst case is three chunk parses per call, and only while the own state keeps moving.
  • Main has not changed argv_floor.py since this PR's base, and GitHub reports the branch as mergeable.

What still blocks a merge (maintainer action, not author action):

  • Sensitive Change Review: security/argv_floor.py is a sensitive path, so a maintainer sign-off with the reasoning template is still needed on this head.
  • Backend Tests (3.12, 7) reports 23995 passed and 0 failed, then exits 1. Coverage Gate fails closed on that result. The log shows no failure in the hosts-file tests. A re-run of that shard is the next step.

I did not run the test suite locally on this head.

@bolichen97

Copy link
Copy Markdown
Contributor

Thank you @jeeshofone for this fix and for the careful measurements behind it.

This PR touches a sensitive path, and the Sensitive Change Review gate cannot pass a fork PR there, so a maintainer has to carry it. Your commit is carried unchanged in #21726, with you credited as co-author, and the real-process check reproduced your numbers on that head. Closing this one in favor of #21726; your branch is left as it is.

@bolichen97 bolichen97 closed this Oct 18, 2026
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Oct 18, 2026
bolichen97 added a commit that referenced this pull request Oct 18, 2026
On a cold worker the first dotless ssh check starts the own-host
enrichment worker and parses the hosts file in the same call. When the
worker publishes while that parse runs, the table comes back uncached,
judged against the older own set. An absent name or a local entry is
answered from that table; a remote entry was refused as pending because
its key no longer matched the own state at return.

_hosts_file_verdict now judges every path again on the current key in
that one case, at most _HOSTS_KEY_MOVED_ATTEMPTS passes per call, and
answers pending after that. A remote answer is still served only when
publication and the own set at return match the key its table was judged
under, so an alias for a newly published own address is refused on the
retry. A file that changes mid-parse and a failed re-stat stay pending at
once.

Fixes #21228
Follow-up to #20964

Supersedes #20861

Co-authored-by: Will Laws <will.laws@me.com>
bolichen97 added a commit that referenced this pull request Oct 18, 2026
#21726)

On a cold worker the first dotless ssh check starts the own-host
enrichment worker and parses the hosts file in the same call. When the
worker publishes while that parse runs, the table comes back uncached,
judged against the older own set. An absent name or a local entry is
answered from that table; a remote entry was refused as pending because
its key no longer matched the own state at return.

_hosts_file_verdict now judges every path again on the current key in
that one case, at most _HOSTS_KEY_MOVED_ATTEMPTS passes per call, and
answers pending after that. A remote answer is still served only when
publication and the own set at return match the key its table was judged
under, so an alias for a newly published own address is refused on the
retry. A file that changes mid-parse and a failed re-stat stay pending at
once.

Fixes #21228
Follow-up to #20964

Supersedes #20861

Co-authored-by: Will Laws <will.laws@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ssh-self floor refuses a remote hosts-file alias when the own-host worker publishes mid-parse

2 participants