Repository navigation
fix(security): re-judge a remote hosts alias after a mid-parse publish - #20861
jeeshofone wants to merge 1 commit into
Conversation
UX Review (Opus 5.5, fork) — ⏭️ skippedRevision |
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of 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 [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 |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Before merge, check the claim behind What this change shipsInventory (4 items) — 4 justifiedIntent: the first
[FIRST-PRINCIPLES-REVIEWED] e1fb304 Description read[DESCRIPTION-READ] 8a8038ea268819c48d7ed5748176885f28d188a37d36fd12393aac96ce892aca That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
d57cbef to
9dd409a
Compare
|
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 Both nits are fixed on this head:
The security package stays at its line budget, and the budget test passes. Gates on this head: Rulings for this round: NIT-1 is done at First-pass check: R1, R3-R8 OK. R2: |
Security Scope Review (fork) — ✅ PASSWhich legitimate operations does adjudicated, zero confirmed regressions Scope-Verdict: PASS TIGHTENING: the hosts-file self-host check ADJUDICATED: 10 candidates, 0 confirmed newly-refused PLATFORM COVERAGE:
[SCOPE-REVIEWED] e1fb304Security scope review -- adjudicated candidates
No candidate flipped from allowed to refused. |
|
Sensitive Change Review cannot pass on a fork PR: Sensitive change review
|
9dd409a to
f71bbdf
Compare
|
Rebased onto 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 What this PR still covers, rebuilt on #20964's shape (one loop around main's body; no second retry path, Security review NITs on d57cbef: NIT-1 (re-stat The four behaviour tests fail on current First-pass check: R1-R8 OK (sensitive path |
f71bbdf to
a39fb8a
Compare
|
Disposition for the independent security review of SHOULD-FIX 1, a file rewritten constantly triples the event-loop stall: fixed. The retry budget is now split by cause in
SHOULD-FIX 2, absolute cost figures in the docstring: fixed. The NIT 1, NIT 2, the re-stat test stubbed the parse: fixed. OUT-OF-SCOPE, same-size rewrite within one coarse ctime tick: filed as #21051. It reproduces the same way on Mutations on Gates: the five First-pass check: R1-R8 OK. R2: |
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
a39fb8a to
e1fb304
Compare
|
Security Scope CONCERNS (on 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 Exit by exit:
So no input that The classifier cannot move the key mid-call, so the branch is proven by tests, not candidates.
Mutations that turn them red, with the red count in brackets:
With the real enrichment worker in fresh processes, an alias for the host's own address is still refused 20 of 20. |
|
Head What changed from Status of the independent security reviews' items on this head:
Tests and proof. On
The cold-process repro with the real worker: Gates on
First-pass check: R1-R8 OK.
AUTOSDE rules matched:
For the maintainer's Sensitive Change Review on this head:
Issue Gate: #21228 is waiting for triage; one request is posted on it. Independent review: left to the loop. |
bolichen97
left a comment
There was a problem hiding this comment.
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_setpins it. - The loop is bounded at
_HOSTS_KEY_MOVED_ATTEMPTS = 3passes, 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.pysince 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.pyis 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 Gatefails 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.
|
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. |
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>
#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>
Problem / Motivation
Goal: On a cold worker, the first
sshto a dotless name the hosts file maps to a remote address (10.4.4.4 devbox, thenssh 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,
mainrefusesssh devbox uptimein 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
sshafter 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
main. A file still being written is not a reason to parse it again on the gate path, and this keeps one retry path.What changed (motivation → approach → change)
Symptom: on a cold worker, the first
sshto a remote hosts-file alias is refused assandbox-escape-ssh-selfand passes on the next call.Root cause:
_hosts_file_verdictjudges 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_statethen answers pending.Change (
_hosts_file_verdictonly): main's body runs inside one loop of at most_HOSTS_KEY_MOVED_ATTEMPTS = 3passes. 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.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_verdictstates the bound (at most_HOSTS_KEY_MOVED_ATTEMPTSpasses per call)._parse_and_cache_for_keysays 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
maingives 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 uptimeis 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_onceandtest_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
main5 of these fail. Mutations, each turning tests red (count in brackets):OSErrorskipping the path (1);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.pyandtest_flake_pattern_ratchet.pyat-n 4(4505 passed, 17 skipped). Also black (the repo gate), isort, flake8, mypy onsrc/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.
mainrefused 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
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
Contributions are accepted under the project's Apache-2.0 license; see the Licensing section of CONTRIBUTING.md.