mirror of
https://github.com/BigBodyCobain/Shadowbroker.git
synced 2026-05-26 17:17:51 +02:00
Tightens the bridge-trust check so a connection on the Docker bridge is only granted local-operator status when its source IP matches a configured frontend container hostname (default: `frontend` + the shipped `container_name` `shadowbroker-frontend`). Previously, when `SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR=1` was set, ANY IP in the 172.16.0.0/12 range was granted local-operator privileges — on a shared Docker host that included any unrelated container on the same bridge. Operators with renamed services can list new hostnames via the new `SHADOWBROKER_TRUSTED_FRONTEND_HOSTS` env var (comma-separated). DNS resolution is cached for 30s; if Docker DNS can't resolve any of the configured names we fail closed and refuse the bridge entirely. Single-user installs see no behavior change — the default-named frontend container still resolves and is still trusted. Credit: tg12 (external security audit)
This commit is contained in:
Shadowbroker
+79
-4
@@ -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:
|
||||
|
||||
@@ -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
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user