From 69361114d1d16052223abbfb78cf81d9c96924bf Mon Sep 17 00:00:00 2001 From: didericis Date: Mon, 20 Jul 2026 22:29:53 -0400 Subject: [PATCH] fix(egress): name the real fault when a deny-all is not an allowlist miss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unattributed bottle, an unreachable orchestrator, and an unparseable policy all become a deny-all Config, and a deny-all is indistinguishable from "policy loaded, host not allowed" at the decision point — both are just "no matching route". So every one of them reported `host X is not in the bottle's egress.routes allowlist`, which reads as a config problem and sends the operator hunting for a route that was never missing. Diagnosing a bricked registration cost hours for exactly this reason. Carry the structural reason on Config and prefer it in decide(). A genuine allowlist miss — a policy that loaded and simply lacks the host — keeps the original wording, so the message now tells the two cases apart. Co-Authored-By: Claude Opus 4.8 --- bot_bottle/egress_addon.py | 1 + bot_bottle/egress_addon_core.py | 50 ++++++++++++++++++++--- tests/unit/test_egress_multitenant.py | 57 ++++++++++++++++++++++++++- 3 files changed, 101 insertions(+), 7 deletions(-) diff --git a/bot_bottle/egress_addon.py b/bot_bottle/egress_addon.py index b550dc6..30e370d 100644 --- a/bot_bottle/egress_addon.py +++ b/bot_bottle/egress_addon.py @@ -379,6 +379,7 @@ class EgressAddon: env, request_method=flow.request.method, request_headers=req_headers, + deny_reason=config.deny_reason, ) if decision.action == "block": diff --git a/bot_bottle/egress_addon_core.py b/bot_bottle/egress_addon_core.py index d3e2d3a..354876f 100644 --- a/bot_bottle/egress_addon_core.py +++ b/bot_bottle/egress_addon_core.py @@ -89,6 +89,14 @@ LOG_FULL = 2 # log block/warn events + full request and response bodies class Config: routes: tuple[Route, ...] log: int = LOG_OFF + # Why this Config is a deny-all, when it is one for a reason *other* than + # the bottle's own policy genuinely not listing the host. A deny-all is + # indistinguishable from "policy loaded, host not allowed" at the decision + # point — both are simply "no matching route" — so without this the + # operator sees `host X is not in the allowlist` and goes hunting for a + # missing route that was never the problem. Empty for a normally-parsed + # policy; `decide` prefers it over the allowlist wording when set. + deny_reason: str = "" @dataclass(frozen=True) @@ -405,16 +413,39 @@ class PolicyResolverLike(typing.Protocol): ... +# Deny-all explanations. Each names the *actual* failure so an operator isn't +# sent looking for a missing egress route when the bottle never had a policy +# to begin with — the failure mode that made a bricked registration read like +# a misconfigured allowlist. +DENY_UNATTRIBUTED = ( + "egress: this bottle is not registered with the orchestrator, so it has " + "no egress policy at all and every host is denied. The bottle's registry " + "row is missing or ambiguous — it was torn down, or another bottle claimed " + "its source IP. Relaunch the bottle; this is not an allowlist problem." +) +DENY_UNPARSEABLE = ( + "egress: this bottle's egress policy could not be parsed, so it is being " + "treated as deny-all. Fix the bottle's egress.routes; every host is denied " + "until it loads." +) +DENY_RESOLVER_ERROR = ( + "egress: the orchestrator could not be reached to resolve this bottle's " + "egress policy, so every host is denied (fail-closed). Check that the " + "control plane is up; this is not an allowlist problem." +) + + def _config_from_policy(policy: "str | None") -> "Config": """Parse a resolved policy blob into a Config, fail-closed: None / empty / unparseable all become a deny-all Config (no routes → every request - blocked).""" + blocked). Each deny-all carries the reason it is one, so the block message + names the real fault instead of blaming the allowlist.""" if not policy: - return Config(routes=()) # unattributed or empty → deny-all + return Config(routes=(), deny_reason=DENY_UNATTRIBUTED) try: return load_config(policy) except ValueError: - return Config(routes=()) # unparseable policy → deny + return Config(routes=(), deny_reason=DENY_UNPARSEABLE) def resolve_client_config( @@ -428,7 +459,7 @@ def resolve_client_config( try: policy = resolver.resolve(client_ip, identity_token) except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught - return Config(routes=()) # orchestrator unreachable/errored → deny + return Config(routes=(), deny_reason=DENY_RESOLVER_ERROR) return _config_from_policy(policy) @@ -457,7 +488,7 @@ def resolve_client_context( client_ip, identity_token, ) except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught - return Config(routes=()), "", {} # orchestrator unreachable/errored → deny + return Config(routes=(), deny_reason=DENY_RESOLVER_ERROR), "", {} return _config_from_policy(policy), (bottle_id or ""), tokens @@ -572,12 +603,16 @@ def decide( *, request_method: str = "GET", request_headers: typing.Mapping[str, str] | None = None, + deny_reason: str = "", ) -> Decision: + """`deny_reason` is `Config.deny_reason`: when the deny-all came from a + missing/unparseable policy rather than the bottle's own allowlist, report + that instead of implying a route is merely absent.""" route = match_route(routes, request_host) if route is None: return Decision( action="block", - reason=( + reason=deny_reason or ( f"egress: host {request_host!r} is not in the " f"bottle's egress.routes allowlist. Declare a " f"route for it or remove the request." @@ -852,6 +887,9 @@ __all__ = [ "is_git_push_request", "is_git_fetch_request", "load_config", + "DENY_UNATTRIBUTED", + "DENY_UNPARSEABLE", + "DENY_RESOLVER_ERROR", "resolve_client_config", "resolve_client_context", "PolicyResolverLike", diff --git a/tests/unit/test_egress_multitenant.py b/tests/unit/test_egress_multitenant.py index e35ec95..01219d7 100644 --- a/tests/unit/test_egress_multitenant.py +++ b/tests/unit/test_egress_multitenant.py @@ -4,7 +4,14 @@ from __future__ import annotations import unittest -from bot_bottle.egress_addon_core import resolve_client_config, resolve_client_context +from bot_bottle.egress_addon_core import ( + DENY_RESOLVER_ERROR, + DENY_UNATTRIBUTED, + DENY_UNPARSEABLE, + decide, + resolve_client_config, + resolve_client_context, +) from bot_bottle.policy_resolver import PolicyResolveError @@ -108,3 +115,51 @@ class TestResolveClientContext(unittest.TestCase): if __name__ == "__main__": unittest.main() + + +class TestDenyReasonNamesTheRealFault(unittest.TestCase): + """A deny-all must not masquerade as a missing allowlist entry. + + Regression: an unregistered bottle resolves no policy, so *every* host is + denied — but the block message said `host X is not in the allowlist`, + which reads as a config problem and sends the operator hunting for a route + that was never missing. The structural reason wins over that wording. + """ + + def _reason(self, resolver: object, host: str = "chatgpt.com") -> str: + cfg = resolve_client_config(resolver, "10.243.0.1") # type: ignore[arg-type] + return decide(cfg.routes, host, "/v1/x", {}, deny_reason=cfg.deny_reason).reason + + def test_unattributed_says_unregistered_not_allowlist(self) -> None: + reason = self._reason(_FakeResolver(result=None)) + self.assertEqual(DENY_UNATTRIBUTED, reason) + # The misleading claim is the one that must be gone: the host was + # never "not in the allowlist" — there was no allowlist at all. + self.assertNotIn("is not in the bottle's egress.routes allowlist", reason) + self.assertIn("not registered", reason) + + def test_resolver_error_says_orchestrator_unreachable(self) -> None: + self.assertEqual(DENY_RESOLVER_ERROR, self._reason(_FakeResolver(raises=True))) + + def test_unparseable_policy_says_so(self) -> None: + self.assertEqual( + DENY_UNPARSEABLE, self._reason(_FakeResolver(result="routes: notalist\n"))) + + def test_a_real_allowlist_miss_keeps_the_allowlist_wording(self) -> None: + """The message only changes for structural deny-alls — a loaded policy + that genuinely lacks the host still points at the allowlist.""" + reason = self._reason(_FakeResolver(result='routes:\n - host: "api.example.com"\n')) + self.assertIn("is not in the bottle's egress.routes allowlist", reason) + self.assertIn("chatgpt.com", reason) + + def test_allowed_host_is_still_forwarded(self) -> None: + cfg = resolve_client_config( + _FakeResolver(result='routes:\n - host: "api.example.com"\n'), "10.243.0.1") + decision = decide( + cfg.routes, "api.example.com", "/v1/x", {}, deny_reason=cfg.deny_reason) + self.assertEqual("forward", decision.action) + + def test_a_parsed_policy_carries_no_deny_reason(self) -> None: + cfg = resolve_client_config( + _FakeResolver(result='routes:\n - host: "api.example.com"\n'), "10.243.0.1") + self.assertEqual("", cfg.deny_reason)