fix(egress): name the real fault when a deny-all is not an allowlist miss
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 <noreply@anthropic.com>
This commit is contained in:
@@ -379,6 +379,7 @@ class EgressAddon:
|
|||||||
env,
|
env,
|
||||||
request_method=flow.request.method,
|
request_method=flow.request.method,
|
||||||
request_headers=req_headers,
|
request_headers=req_headers,
|
||||||
|
deny_reason=config.deny_reason,
|
||||||
)
|
)
|
||||||
|
|
||||||
if decision.action == "block":
|
if decision.action == "block":
|
||||||
|
|||||||
@@ -89,6 +89,14 @@ LOG_FULL = 2 # log block/warn events + full request and response bodies
|
|||||||
class Config:
|
class Config:
|
||||||
routes: tuple[Route, ...]
|
routes: tuple[Route, ...]
|
||||||
log: int = LOG_OFF
|
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)
|
@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":
|
def _config_from_policy(policy: "str | None") -> "Config":
|
||||||
"""Parse a resolved policy blob into a Config, fail-closed: None / empty /
|
"""Parse a resolved policy blob into a Config, fail-closed: None / empty /
|
||||||
unparseable all become a deny-all Config (no routes → every request
|
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:
|
if not policy:
|
||||||
return Config(routes=()) # unattributed or empty → deny-all
|
return Config(routes=(), deny_reason=DENY_UNATTRIBUTED)
|
||||||
try:
|
try:
|
||||||
return load_config(policy)
|
return load_config(policy)
|
||||||
except ValueError:
|
except ValueError:
|
||||||
return Config(routes=()) # unparseable policy → deny
|
return Config(routes=(), deny_reason=DENY_UNPARSEABLE)
|
||||||
|
|
||||||
|
|
||||||
def resolve_client_config(
|
def resolve_client_config(
|
||||||
@@ -428,7 +459,7 @@ def resolve_client_config(
|
|||||||
try:
|
try:
|
||||||
policy = resolver.resolve(client_ip, identity_token)
|
policy = resolver.resolve(client_ip, identity_token)
|
||||||
except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught
|
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)
|
return _config_from_policy(policy)
|
||||||
|
|
||||||
|
|
||||||
@@ -457,7 +488,7 @@ def resolve_client_context(
|
|||||||
client_ip, identity_token,
|
client_ip, identity_token,
|
||||||
)
|
)
|
||||||
except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught
|
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
|
return _config_from_policy(policy), (bottle_id or ""), tokens
|
||||||
|
|
||||||
|
|
||||||
@@ -572,12 +603,16 @@ def decide(
|
|||||||
*,
|
*,
|
||||||
request_method: str = "GET",
|
request_method: str = "GET",
|
||||||
request_headers: typing.Mapping[str, str] | None = None,
|
request_headers: typing.Mapping[str, str] | None = None,
|
||||||
|
deny_reason: str = "",
|
||||||
) -> Decision:
|
) -> 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)
|
route = match_route(routes, request_host)
|
||||||
if route is None:
|
if route is None:
|
||||||
return Decision(
|
return Decision(
|
||||||
action="block",
|
action="block",
|
||||||
reason=(
|
reason=deny_reason or (
|
||||||
f"egress: host {request_host!r} is not in the "
|
f"egress: host {request_host!r} is not in the "
|
||||||
f"bottle's egress.routes allowlist. Declare a "
|
f"bottle's egress.routes allowlist. Declare a "
|
||||||
f"route for it or remove the request."
|
f"route for it or remove the request."
|
||||||
@@ -852,6 +887,9 @@ __all__ = [
|
|||||||
"is_git_push_request",
|
"is_git_push_request",
|
||||||
"is_git_fetch_request",
|
"is_git_fetch_request",
|
||||||
"load_config",
|
"load_config",
|
||||||
|
"DENY_UNATTRIBUTED",
|
||||||
|
"DENY_UNPARSEABLE",
|
||||||
|
"DENY_RESOLVER_ERROR",
|
||||||
"resolve_client_config",
|
"resolve_client_config",
|
||||||
"resolve_client_context",
|
"resolve_client_context",
|
||||||
"PolicyResolverLike",
|
"PolicyResolverLike",
|
||||||
|
|||||||
@@ -4,7 +4,14 @@ from __future__ import annotations
|
|||||||
|
|
||||||
import unittest
|
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
|
from bot_bottle.policy_resolver import PolicyResolveError
|
||||||
|
|
||||||
|
|
||||||
@@ -108,3 +115,51 @@ class TestResolveClientContext(unittest.TestCase):
|
|||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.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)
|
||||||
|
|||||||
Reference in New Issue
Block a user