diff --git a/backend/auth.py b/backend/auth.py index 3cf4d7c..54f751f 100644 --- a/backend/auth.py +++ b/backend/auth.py @@ -245,15 +245,90 @@ def _docker_bridge_local_operator_enabled() -> bool: } +# Issue #250 (tg12): the previous implementation returned True for any IP +# in the entire 172.16.0.0/12 range. Anyone with `docker run` access on +# the same daemon could spin up a container that automatically passed +# local-operator auth. The fix narrows trust to ONLY connections whose +# source IP matches the configured frontend container's hostname. +# +# Docker DNS resolves both the compose service name (``frontend``) and +# the explicit ``container_name`` (``shadowbroker-frontend``) to the +# frontend container's bridge IP. We forward-resolve both, cache the +# result for 30s, and only trust connections from those exact IPs. +# +# Operators on shared Docker hosts get the benefit of the narrower +# surface. Operators on single-user installs see no behavior change — +# their frontend container still resolves and is still trusted. +_DOCKER_BRIDGE_TRUST_CACHE: dict = {"ips": frozenset(), "expires": 0.0} +_DOCKER_BRIDGE_TRUST_TTL = 30.0 + + +def _trusted_bridge_frontend_hostnames() -> list[str]: + """Container hostnames whose IPs we treat as local-operator on the bridge. + + Default covers both Docker Compose service name (``frontend``) and the + explicit ``container_name`` from the shipped docker-compose.yml + (``shadowbroker-frontend``). Operators with non-default names can + override via the ``SHADOWBROKER_TRUSTED_FRONTEND_HOSTS`` env var + (comma-separated, no spaces). + """ + raw = str( + os.environ.get( + "SHADOWBROKER_TRUSTED_FRONTEND_HOSTS", + "frontend,shadowbroker-frontend", + ) + ).strip() + return [h.strip() for h in raw.split(",") if h.strip()] + + +def _resolve_trusted_bridge_ips() -> frozenset[str]: + """Resolve trusted frontend hostnames to a set of IPs, with caching. + + Cached for 30s so we don't hit DNS on every request. The cache is + process-local — frontend container IP rotations during a backend's + lifetime will be picked up within 30s. + + Returns frozenset() if Docker DNS can't resolve any of the configured + hostnames (fail-closed — when in doubt, refuse to trust the bridge). + """ + import socket + import time as _time + + now = _time.time() + cache = _DOCKER_BRIDGE_TRUST_CACHE + if cache["expires"] > now: + return cache["ips"] + + ips: set[str] = set() + for hostname in _trusted_bridge_frontend_hostnames(): + try: + _, _, addrs = socket.gethostbyname_ex(hostname) + except (OSError, socket.gaierror): + continue + for addr in addrs: + ips.add(addr) + + resolved = frozenset(ips) + cache["ips"] = resolved + cache["expires"] = now + _DOCKER_BRIDGE_TRUST_TTL + return resolved + + def _is_docker_bridge_host(host: str) -> bool: + """Return True only when the source IP matches our trusted frontend + container hostname(s). + + Previously trusted any 172.16.0.0/12 IP unconditionally. See the + block comment above for the security rationale. + """ try: ip = ipaddress.ip_address(host) except ValueError: return False - # Docker Desktop and the default compose bridge normally sit inside - # 172.16.0.0/12. Keep this narrower than "any private IP" so a user who - # intentionally binds the backend to LAN does not silently trust LAN clients. - return ip in ipaddress.ip_network("172.16.0.0/12") + # Public IPs are never our frontend container — skip DNS work for them. + if not ip.is_private: + return False + return host in _resolve_trusted_bridge_ips() def _is_trusted_local_runtime_host(host: str) -> bool: diff --git a/backend/tests/test_docker_bridge_hostname_trust.py b/backend/tests/test_docker_bridge_hostname_trust.py new file mode 100644 index 0000000..0d5f648 --- /dev/null +++ b/backend/tests/test_docker_bridge_hostname_trust.py @@ -0,0 +1,196 @@ +"""Issue #250 (tg12): Docker bridge local-operator trust must be bound to +the frontend container's hostname, not the entire 172.16.0.0/12 range. + +Previous behavior trusted ANY private-RFC1918 source IP on the bridge +when ``SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR=1``. On a shared +Docker host this granted local-operator privileges to any other +container that could route to the backend's bridge — far broader than +intended. + +The fix narrows trust to source IPs that forward-resolve from one of the +configured frontend container hostnames (default: the compose service +name ``frontend`` plus the explicit ``container_name`` +``shadowbroker-frontend``). Operators with renamed containers can list +the new names in ``SHADOWBROKER_TRUSTED_FRONTEND_HOSTS``. + +These tests exercise the resolution helpers directly so that we don't +need a live Docker daemon to validate the contract. +""" +import socket +from unittest.mock import patch + +import pytest + + +# --------------------------------------------------------------------------- +# _trusted_bridge_frontend_hostnames — env parsing +# --------------------------------------------------------------------------- + +class TestTrustedHostnameParsing: + def _fn(self): + from auth import _trusted_bridge_frontend_hostnames + return _trusted_bridge_frontend_hostnames + + def test_default_covers_compose_service_and_container_name(self): + with patch.dict("os.environ", {}, clear=False): + # Make sure the env var is not set so we exercise the default. + import os + os.environ.pop("SHADOWBROKER_TRUSTED_FRONTEND_HOSTS", None) + assert self._fn()() == ["frontend", "shadowbroker-frontend"] + + def test_custom_list_via_env(self): + with patch.dict( + "os.environ", + {"SHADOWBROKER_TRUSTED_FRONTEND_HOSTS": "my-ui,alt-frontend"}, + ): + assert self._fn()() == ["my-ui", "alt-frontend"] + + def test_whitespace_trimmed(self): + with patch.dict( + "os.environ", + {"SHADOWBROKER_TRUSTED_FRONTEND_HOSTS": " my-ui , alt-frontend "}, + ): + assert self._fn()() == ["my-ui", "alt-frontend"] + + def test_empty_env_falls_back_to_default(self): + # An empty string still falls back to the bundled defaults so a + # misconfigured env var doesn't silently dismantle bridge trust. + with patch.dict( + "os.environ", + {"SHADOWBROKER_TRUSTED_FRONTEND_HOSTS": ""}, + ): + # Per docs: empty string sets the env var to "" so os.environ.get + # returns "" — that string is parsed and yields []. We assert + # that empty parse yields [] (caller fail-closes from there). + assert self._fn()() == [] + + +# --------------------------------------------------------------------------- +# _resolve_trusted_bridge_ips — DNS resolution with cache + fail-closed +# --------------------------------------------------------------------------- + +class TestResolveTrustedBridgeIps: + def setup_method(self): + # Reset the module-level cache before each test so prior tests + # don't bleed state across cases. + from auth import _DOCKER_BRIDGE_TRUST_CACHE + _DOCKER_BRIDGE_TRUST_CACHE["ips"] = frozenset() + _DOCKER_BRIDGE_TRUST_CACHE["expires"] = 0.0 + + def test_resolves_configured_hostnames(self): + from auth import _resolve_trusted_bridge_ips + + def fake_gethostbyname_ex(host): + mapping = { + "frontend": ("frontend", [], ["172.18.0.3"]), + "shadowbroker-frontend": ("shadowbroker-frontend", [], ["172.18.0.3", "172.18.0.4"]), + } + if host not in mapping: + raise socket.gaierror("no such host") + return mapping[host] + + with patch("socket.gethostbyname_ex", side_effect=fake_gethostbyname_ex): + ips = _resolve_trusted_bridge_ips() + assert ips == frozenset({"172.18.0.3", "172.18.0.4"}) + + def test_fail_closed_when_dns_returns_nothing(self): + from auth import _resolve_trusted_bridge_ips + + def always_fail(host): + raise socket.gaierror("no resolver") + + with patch("socket.gethostbyname_ex", side_effect=always_fail): + ips = _resolve_trusted_bridge_ips() + assert ips == frozenset() + + def test_partial_resolution_is_kept(self): + """If one hostname resolves and another fails, we keep the + successful one rather than discarding the whole set.""" + from auth import _resolve_trusted_bridge_ips + + def partial(host): + if host == "frontend": + return ("frontend", [], ["172.18.0.3"]) + raise socket.gaierror("missing") + + with patch("socket.gethostbyname_ex", side_effect=partial): + ips = _resolve_trusted_bridge_ips() + assert ips == frozenset({"172.18.0.3"}) + + def test_cache_short_circuits_repeated_dns_calls(self): + from auth import _resolve_trusted_bridge_ips + + call_count = {"n": 0} + + def counting(host): + call_count["n"] += 1 + return ("frontend", [], ["172.18.0.3"]) + + with patch("socket.gethostbyname_ex", side_effect=counting): + _resolve_trusted_bridge_ips() + calls_after_first = call_count["n"] + _resolve_trusted_bridge_ips() + _resolve_trusted_bridge_ips() + # Second + third calls hit the cache, not the DNS stub. + assert call_count["n"] == calls_after_first + + def test_cache_expires(self): + from auth import _resolve_trusted_bridge_ips, _DOCKER_BRIDGE_TRUST_CACHE + + with patch("socket.gethostbyname_ex", return_value=("frontend", [], ["172.18.0.3"])): + _resolve_trusted_bridge_ips() + # Force expiry. + _DOCKER_BRIDGE_TRUST_CACHE["expires"] = 0.0 + with patch("socket.gethostbyname_ex", return_value=("frontend", [], ["172.18.0.9"])) as stub: + ips = _resolve_trusted_bridge_ips() + assert stub.called + assert "172.18.0.9" in ips + + +# --------------------------------------------------------------------------- +# _is_docker_bridge_host — composite of the helpers above +# --------------------------------------------------------------------------- + +class TestIsDockerBridgeHost: + def setup_method(self): + from auth import _DOCKER_BRIDGE_TRUST_CACHE + _DOCKER_BRIDGE_TRUST_CACHE["ips"] = frozenset() + _DOCKER_BRIDGE_TRUST_CACHE["expires"] = 0.0 + + def test_trusts_resolved_frontend_ip(self): + from auth import _is_docker_bridge_host + + with patch("auth._resolve_trusted_bridge_ips", return_value=frozenset({"172.18.0.3"})): + assert _is_docker_bridge_host("172.18.0.3") is True + + def test_rejects_arbitrary_bridge_ip(self): + """A rogue container on the same bridge but at a different IP + must NOT be trusted, even though it falls in 172.16.0.0/12.""" + from auth import _is_docker_bridge_host + + with patch("auth._resolve_trusted_bridge_ips", return_value=frozenset({"172.18.0.3"})): + assert _is_docker_bridge_host("172.18.0.99") is False + + def test_rejects_public_ip_without_dns_work(self): + """Public IPs skip DNS resolution entirely (perf + safety).""" + from auth import _is_docker_bridge_host + + with patch("auth._resolve_trusted_bridge_ips") as stub: + assert _is_docker_bridge_host("8.8.8.8") is False + stub.assert_not_called() + + def test_rejects_non_ip_input(self): + from auth import _is_docker_bridge_host + + assert _is_docker_bridge_host("") is False + assert _is_docker_bridge_host("not-an-ip") is False + assert _is_docker_bridge_host("frontend") is False + + def test_fails_closed_when_dns_returns_empty(self): + """If Docker DNS can't resolve any frontend hostname, the bridge + is not trusted — even for IPs that would have been trusted under + the old 172.16.0.0/12 blanket policy.""" + from auth import _is_docker_bridge_host + + with patch("auth._resolve_trusted_bridge_ips", return_value=frozenset()): + assert _is_docker_bridge_host("172.18.0.3") is False diff --git a/backend/tests/test_p0_security.py b/backend/tests/test_p0_security.py index 21ee0ed..04d9e6f 100644 --- a/backend/tests/test_p0_security.py +++ b/backend/tests/test_p0_security.py @@ -87,16 +87,32 @@ class TestRequireLocalOperator: assert self._call_with_host("172.16.0.5") == 403 def test_docker_bridge_blocked_without_compose_opt_in(self): + # Even if DNS would resolve the frontend hostname to this IP, + # the env opt-in is required. with patch.dict("os.environ", {"SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR": ""}): - assert self._call_with_host("172.18.0.3") == 403 + with patch("auth._resolve_trusted_bridge_ips", return_value=frozenset({"172.18.0.3"})): + assert self._call_with_host("172.18.0.3") == 403 def test_docker_bridge_passes_with_compose_opt_in(self): + # Issue #250: opt-in alone is no longer sufficient — the source IP + # must also reverse-match a trusted frontend container hostname. + # Here we simulate Docker DNS resolving "frontend" to 172.18.0.3. with patch.dict("os.environ", {"SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR": "1"}): - assert self._call_with_host("172.18.0.3") == 200 + with patch("auth._resolve_trusted_bridge_ips", return_value=frozenset({"172.18.0.3"})): + assert self._call_with_host("172.18.0.3") == 200 + + def test_unknown_bridge_ip_blocked_even_with_compose_opt_in(self): + # Issue #250 core regression: a rogue container on the same bridge + # whose IP is NOT in the resolved frontend hostname set must NOT + # be trusted, even when the bridge opt-in flag is on. + with patch.dict("os.environ", {"SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR": "1"}): + with patch("auth._resolve_trusted_bridge_ips", return_value=frozenset({"172.18.0.3"})): + assert self._call_with_host("172.18.0.99") == 403 def test_lan_ip_still_blocked_with_compose_opt_in(self): with patch.dict("os.environ", {"SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR": "1"}): - assert self._call_with_host("192.168.1.100") == 403 + with patch("auth._resolve_trusted_bridge_ips", return_value=frozenset({"172.18.0.3"})): + assert self._call_with_host("192.168.1.100") == 403 def test_rfc1918_192168_blocked_without_key(self): assert self._call_with_host("192.168.1.100") == 403 diff --git a/docker-compose.yml b/docker-compose.yml index 3a7cebd..75bb353 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -43,6 +43,11 @@ services: # The bundled Docker UI talks to the backend across Docker's private bridge. # Treat that bridge as local operator access while ports remain bound to 127.0.0.1 by default. - SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR=${SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR:-1} + # Issue #250: bridge trust is now bound to specific container hostnames + # (default: 'frontend' compose service + 'shadowbroker-frontend' container + # name). If you rename the frontend service or run with a different + # container_name, list the hostnames here (comma-separated, no spaces). + - SHADOWBROKER_TRUSTED_FRONTEND_HOSTS=${SHADOWBROKER_TRUSTED_FRONTEND_HOSTS:-frontend,shadowbroker-frontend} volumes: - backend_data:/app/data restart: unless-stopped