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