From a61afcdb500889dad102e2dd9fa34d13b5f902ce Mon Sep 17 00:00:00 2001 From: Bolin Chen Date: Sun, 18 Oct 2026 00:51:16 +0000 Subject: [PATCH] fix(security): re-judge a remote hosts alias after a mid-parse publish 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 --- src/kiro_crew/security/argv_floor.py | 100 ++++++++++----------- test/test_denied_commands_security.py | 123 +++++++++++++++++++++++++- 2 files changed, 172 insertions(+), 51 deletions(-) diff --git a/src/kiro_crew/security/argv_floor.py b/src/kiro_crew/security/argv_floor.py index be46549f58f..905b21cf63d 100644 --- a/src/kiro_crew/security/argv_floor.py +++ b/src/kiro_crew/security/argv_floor.py @@ -1425,6 +1425,7 @@ def _hosts_file_paths() -> "tuple[str, ...]": # parse in the same call on a miss: a typical file in well under a # millisecond, one at the cap in a few. _HOSTS_FILE_READ_CHUNK = 64 * 1024 +_HOSTS_KEY_MOVED_ATTEMPTS = 3 # passes per call; one enrichment pass moves the key at most twice # path -> (key, {name -> maps-to-local}), key = (mtime, ctime, size, content @@ -1559,7 +1560,7 @@ def _parse_and_cache_for_key( vet is what starts the worker), the table is returned but NOT cached: it was judged against the older own set, so a remote entry could be an alias for the newly learned address, and the gate's own-state re-check - holds exactly those entries pending. The file itself is the one the + judges exactly those entries again. The file itself is the one the key names, so the table's NAME SET is exact: a local entry stays local under a grown own set, and a name absent from the file is absent under any own set. Answering those two from the uncached table is what keeps @@ -1692,60 +1693,59 @@ def _hosts_file_verdict(host: str) -> "bool | None": hostname or an IP address. An in-call parse whose FILE changed while it ran is also pending; one whose own-address state moved (the enrichment worker publishing mid-parse) still answers a local or an - absent name from the table it built, and holds a remote entry pending - through the own-state check below. A remote answer is given - only if publication and the own set still match the table's key at the - moment of return. A name on several lines is local if ANY of them maps - local (deny-floor direction). + absent name from the table it built. A remote answer is given only if + publication and the own set still match its table's key at return; + otherwise every path is judged again on the current key, at most + ``_HOSTS_KEY_MOVED_ATTEMPTS`` passes per call (so at most that many parses + of one file), then the answer is pending. A name on several lines is + local if ANY of them maps local (deny-floor direction). """ - tables: "list[tuple[_HostsKey, dict[str, bool]]]" = [] - for path in _hosts_file_paths(): - try: - key = _hosts_file_key(path) - except _HostsFileUnreadable: - _schedule_hosts_file_warm() - return True - except OSError: - continue - if _unverifiable_hosts_file(key): - return True - cached = _HOSTS_FILE_CACHE.get(path) - if cached is None or cached[0] != key: - on_loop = _event_loop_running() - if key[2] > _HOSTS_FILE_READ_CHUNK and on_loop: - # Every call on an uncached key re-schedules the warm, so a - # retry does not hang on one thread that failed to start. + for _attempt in range(_HOSTS_KEY_MOVED_ATTEMPTS): + tables: "list[tuple[_HostsKey, dict[str, bool]]]" = [] + for path in _hosts_file_paths(): + try: + key = _hosts_file_key(path) + except _HostsFileUnreadable: _schedule_hosts_file_warm() return True - limit = _HOSTS_FILE_READ_CHUNK if on_loop else None - try: - result = _parse_and_cache_for_key(path, key, limit=limit) except OSError: - result = "changed" - if isinstance(result, str): - _schedule_hosts_file_warm() + continue + if _unverifiable_hosts_file(key): return True - cached = (key, result) - tables.append(cached) - remote = [] - for key, table in tables: - verdict = table.get(host) - if verdict is True: - return True - if verdict is False: - remote.append(key) - if not remote: - return None - # The worker may have published or grown the own set since the key was - # read; a not-local entry judged by the older state is not served. - own_state = (_NETLINK_ADDRS_PUBLISHED, _own_host_names()) - if any((key[-2], key[-1]) != own_state for key in remote): - _schedule_hosts_file_warm() - return True - # Not-local counts only when judged after publication; while the - # own-address set is incomplete it defers to the async verdict layer. - # (Every remote key's publication bit equals own_state[0] here.) - return False if own_state[0] else None + cached = _HOSTS_FILE_CACHE.get(path) + if cached is None or cached[0] != key: + on_loop = _event_loop_running() + if key[2] > _HOSTS_FILE_READ_CHUNK and on_loop: + # Each uncached call re-schedules: a failed warm start cannot hang a later call. + _schedule_hosts_file_warm() + return True + limit = _HOSTS_FILE_READ_CHUNK if on_loop else None + try: + result = _parse_and_cache_for_key(path, key, limit=limit) + except OSError: + result = "changed" + if isinstance(result, str): + _schedule_hosts_file_warm() + return True + cached = (key, result) + tables.append(cached) + remote = [] + for key, table in tables: + verdict = table.get(host) + if verdict is True: + return True + if verdict is False: + remote.append(key) + if not remote: + return None + # A not-local entry judged under an own state that has since moved is judged again. + own_state = (_NETLINK_ADDRS_PUBLISHED, _own_host_names()) + if all((key[-2], key[-1]) == own_state for key in remote): + # Not-local counts only when judged after publication; while the + # own-address set is incomplete it defers to the async verdict layer. + return False if own_state[0] else None + _schedule_hosts_file_warm() + return True def _resolved_host_verdict(host: str, *, fail_closed: bool = True) -> bool: diff --git a/test/test_denied_commands_security.py b/test/test_denied_commands_security.py index e5c7ae6570b..92b09f63566 100644 --- a/test/test_denied_commands_security.py +++ b/test/test_denied_commands_security.py @@ -10794,6 +10794,123 @@ def _parse_then_grow(*args, **kwargs): assert _denied_by("ssh -o ConnectTimeout=10 backup@host uptime") is None assert _denied_by("ssh loopalias uptime") == self._RULE + def _publish_during_first_gate_parse(self, monkeypatch, addrs): + """The enrichment worker publishes *addrs* while the gate parses the file.""" + monkeypatch.setattr(_argv_floor, "_NETLINK_ADDRS_PUBLISHED", False) + real_parse = _argv_floor._parse_hosts_file + parses: "list[bool]" = [] + + def _parse(*args, **kwargs): + parses.append(_argv_floor._NETLINK_ADDRS_PUBLISHED) + if len(parses) == 1: + _argv_floor._publish_netlink_addresses(addrs) + return real_parse(*args, **kwargs) + + monkeypatch.setattr(_argv_floor, "_parse_hosts_file", _parse) + return parses + + def test_a_cold_ssh_to_a_remote_alias_is_allowed_when_publication_lands_mid_parse( + self, monkeypatch + ): + # The first ssh on a cold worker: the enrichment worker's netlink + # publish (its real publish step) lands inside the gate's parse, as it + # does whenever that parse is slow. The remote alias is judged again + # on the new key and allowed, not refused as ssh-to-self. + path = self._hosts(monkeypatch, "10.4.4.4 farbox\n") + parses = self._publish_during_first_gate_parse(monkeypatch, {"203.0.113.66"}) + assert _denied_by("ssh farbox uptime") is None + assert parses == [False, True] + assert _argv_floor._HOSTS_FILE_CACHE[path][0][-2] is True + assert "kirocrew-hosts-warm" not in self.started + + def test_the_second_judgement_uses_the_newly_published_own_set(self, monkeypatch): + # The address published mid-parse is this host's, and a line aliases + # a name to it: the retry judges that alias local, never remote. + self._hosts(monkeypatch, "10.4.4.4 farbox\n203.0.113.66 ownalias\n") + parses = self._publish_during_first_gate_parse(monkeypatch, {"203.0.113.66"}) + # The first table, judged before publication, maps ownalias remote: + # only the own-state re-check stops that table from being served. + assert _argv_floor._hosts_file_verdict("ownalias") is True + assert parses == [False, True] + assert _denied_by("ssh ownalias uptime") == self._RULE + assert _denied_by("ssh farbox uptime") is None + + def test_a_file_edited_during_the_gate_parse_is_pending_at_once(self, monkeypatch): + # Only a moved OWN state is judged again: a FILE that changes + # mid-parse is refused as pending after one parse, with the warm + # scheduled. + path = self._hosts(monkeypatch, "10.4.4.4 farbox\n") + real_parse = _argv_floor._parse_hosts_file + parses: "list[int]" = [] + + def _parse(*args, **kwargs): + parses.append(1) + table = real_parse(*args, **kwargs) + with open(path, "a", encoding="utf-8") as fh: + fh.write("10.5.5.5 newbox\n") + return table + + monkeypatch.setattr(_argv_floor, "_parse_hosts_file", _parse) + assert _argv_floor._hosts_file_verdict("farbox") is True + assert len(parses) == 1 + assert _argv_floor._HOSTS_FILE_CACHE == {} + assert self.started.count("kirocrew-hosts-warm") == 1 + + def test_an_own_set_that_moves_before_a_remote_answer_is_judged_again(self, monkeypatch): + # The own set grows between the table's key and the remote answer's + # re-check: the gate judges again on the grown set and serves it. + self._hosts(monkeypatch, "10.4.4.4 farbox\n") + reads: "list[int]" = [] + + def _own(): + reads.append(1) + grown = len(reads) > 2 # the key read and its re-check see the old set + return frozenset({"127.0.0.1"} | ({"198.51.100.9"} if grown else set())) + + monkeypatch.setattr(_argv_floor, "_own_host_names", _own) + assert _argv_floor._hosts_file_verdict("farbox") is False + assert "kirocrew-hosts-warm" not in self.started + + def test_a_key_still_moving_after_every_attempt_is_pending(self, monkeypatch): + # The bound: an own set that moves on every read is judged a fixed + # number of times, then a remote entry is refused as pending. + self._hosts(monkeypatch, "10.4.4.4 farbox\n") + reads = iter(range(10_000)) + monkeypatch.setattr( + _argv_floor, "_own_host_names", lambda: frozenset({f"10.200.0.{next(reads) % 250}"}) + ) + real_parse = _argv_floor._parse_hosts_file + parses: "list[int]" = [] + + def _parse(*args, **kwargs): + parses.append(1) + return real_parse(*args, **kwargs) + + monkeypatch.setattr(_argv_floor, "_parse_hosts_file", _parse) + assert _argv_floor._hosts_file_verdict("farbox") is True + assert len(parses) == _argv_floor._HOSTS_KEY_MOVED_ATTEMPTS == 3 + assert _argv_floor._HOSTS_FILE_CACHE == {} + assert self.started.count("kirocrew-hosts-warm") == 1 + + def test_a_failed_re_stat_after_the_parse_is_pending_at_once(self, monkeypatch): + # The key cannot be re-read after a real parse: a read failure, not a + # moved own state, so the gate refuses at once, unretried. + self._hosts(monkeypatch, "10.4.4.4 farbox\n") + real_key = _argv_floor._hosts_file_key + calls: "list[int]" = [] + + def _key(path): + calls.append(1) + if len(calls) == 2: # the re-stat after the parse + raise PermissionError(path) + return real_key(path) + + monkeypatch.setattr(_argv_floor, "_hosts_file_key", _key) + assert _argv_floor._hosts_file_verdict("farbox") is True + assert len(calls) == 2 + assert _argv_floor._HOSTS_FILE_CACHE == {} + assert self.started.count("kirocrew-hosts-warm") == 1 + def test_an_own_address_learned_later_re_marks_a_cached_alias(self, monkeypatch): self._hosts(monkeypatch, "198.51.100.44 lateownalias\n") _argv_floor._warm_hosts_file_cache() @@ -10936,7 +11053,11 @@ def _merge(): self._fire_after_lookup(monkeypatch, _merge) assert _argv_floor._host_is_self("ownalias") is True - assert "kirocrew-hosts-warm" in self.started + # The gate judged the file again on the merged set, in the same call: + # the alias is cached local, so no pending warm was needed. + assert dict(_argv_floor._HOSTS_FILE_CACHE)[str(self.tmp_path / "hosts")][1] == { + "ownalias": True + } def test_publication_after_the_key_check_is_a_deny(self, monkeypatch): # A table judged before publication must not be read as authoritative