fix(sandbox-proxy): block sandbox egress relay to internal destinations (#12242)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

backend/onyx/sandbox_proxy/addons/gate.py

@@ -7,16 +7,20 @@ Fail-open: `RequestEvaluator` exceptions and non-matching action types.
 import asyncio
 import base64
 import binascii
+import ipaddress
 import operator
+import socket
 import threading
 from collections.abc import Callable
 from dataclasses import dataclass
 from typing import Protocol
+from urllib.parse import urlparse
 from uuid import UUID
 
 from cachetools import cachedmethod
 from cachetools import TTLCache
 from mitmproxy import http
+from mitmproxy.proxy import server_hooks
 from sqlalchemy.orm import Session
 
 from onyx.cache.interface import CACHE_TRANSIENT_ERRORS
@@ -54,6 +58,7 @@ from onyx.sandbox_proxy.logging_utils import full_log_id
 from onyx.sandbox_proxy.logging_utils import sandbox_log_label
 from onyx.sandbox_proxy.logging_utils import short_log_id
 from onyx.sandbox_proxy.request_evaluator import RequestEvaluator
+from onyx.server.features.build.configs import SANDBOX_API_SERVER_URL
 from onyx.server.features.build.db import action_approval
 from onyx.utils.logger import setup_logger
 
@@ -63,6 +68,92 @@ logger = setup_logger()
 PARSER_MAX_BODY_BYTES = 1_048_576
 
 
+# --- internal-destination egress lockdown: closes the proxy-relay path ---
+# A sandbox can only egress via the proxy, so the proxy is the single layer that can
+# stop it relaying (CONNECT-tunneling) to internal services (databases, caches, search,
+# metadata endpoints) — that destination is invisible at the sandbox's own egress (it sees
+# "TCP to proxy:8080"). Deny any forwarded destination that is, or resolves to, an
+# internal address; allow the one legitimate internal exception (the api-server) plus
+# the public internet. Keying off "not globally routable" — not a hostname allow-list —
+# catches internal services we never enumerated. The proxy's OWN cred-resolution DB
+# client connects directly (not through the mitmproxy listener), so it is unaffected here.
+
+
+# The single allowed internal destination: the api-server the sandbox calls via the
+# proxy (PAT-injected). Matched by host AND port so it works even when it's an
+# in-cluster name resolving to an internal IP, while still denying every other port
+# on that host (e.g. a co-located Redis/Postgres reachable at the same hostname).
+def _parse_api_server() -> tuple[str | None, int | None]:
+    if not SANDBOX_API_SERVER_URL:
+        return None, None
+    parsed = urlparse(SANDBOX_API_SERVER_URL)
+    host = (parsed.hostname or "").lower() or None
+    port = parsed.port or (443 if parsed.scheme == "https" else 80)
+    return host, port
+
+
+_API_SERVER_HOST, _API_SERVER_PORT = _parse_api_server()
+
+
+def _is_api_server(host: str, port: int) -> bool:
+    return (
+        _API_SERVER_HOST is not None
+        and host == _API_SERVER_HOST
+        and port == _API_SERVER_PORT
+    )
+
+
+def _ip_is_internal(ip_str: str) -> bool:
+    """True if ``ip_str`` is not a globally-routable public address.
+
+    `is_global` covers far more than RFC1918: CGNAT (100.64.0.0/10 — EKS pod IPs
+    under custom networking), loopback, link-local (incl. cloud metadata / IMDS),
+    IPv6 ULA (fc00::/7) + link-local (fe80::/10) + loopback (::1), and reserved
+    ranges. IPv4-mapped IPv6 (``::ffff:10.0.0.1``) is judged by its embedded IPv4
+    so it can't be used to smuggle an internal v4 address past the check.
+    """
+    try:
+        ip = ipaddress.ip_address(ip_str)
+    except ValueError:
+        return False
+    mapped = getattr(ip, "ipv4_mapped", None)
+    if mapped is not None:
+        ip = mapped
+    return not ip.is_global
+
+
+def destination_is_blocked(host: str, port: int) -> bool:
+    """True if the sandbox must not be relayed to ``host:port``.
+
+    Denied: anything that is, or resolves to, an internal address. Allowed: the
+    api-server (host + port) and any public address. Fail closed: a resolution
+    failure denies (with a warning) — a transient resolver error must not become
+    an opening to an internal service. If a name resolves to a mix of public and
+    internal addresses, deny — an attacker could otherwise steer the connection
+    to the internal one.
+    """
+    host = (host or "").strip().lower()
+    if not host:
+        return False
+    if _is_api_server(host, port):
+        return False
+    try:
+        ipaddress.ip_address(host)  # literal-IP destination: check directly, no DNS
+        return _ip_is_internal(host)
+    except ValueError:
+        pass
+    try:
+        infos = socket.getaddrinfo(host, port, proto=socket.IPPROTO_TCP)
+    except OSError as exc:
+        logger.warning(
+            "egress_destination_resolution_failed host=%s error=%s", host, exc
+        )
+        return True
+    # getaddrinfo types sockaddr[0] as `str | int`; the address element is always
+    # a str at runtime (AF_INET/AF_INET6), so coerce to satisfy the type checker.
+    return any(_ip_is_internal(str(info[4][0])) for info in infos)
+
+
 class _IdentityResolver(Protocol):
     """Subset of `IdentityResolver` the gate uses."""
 
@@ -165,7 +256,7 @@ class GateAddon:
     # mitmproxy hooks
     # ------------------------------------------------------------------
 
-    def http_connect(self, flow: http.HTTPFlow) -> None:
+    async def http_connect(self, flow: http.HTTPFlow) -> None:
         """Capture the per-session tag from the CONNECT's Proxy-Authorization.
 
         For MITM'd HTTPS the header rides on the CONNECT, not the decrypted
@@ -173,6 +264,22 @@ class GateAddon:
         connection id (one tunnel per subprocess = one session); evicted in
         `client_disconnected`. Best-effort.
         """
+        # Refuse to open a tunnel to an internal address, with a clear 403 the
+        # agent can act on. Best-effort early deny on the CONNECT host; the decrypted
+        # inner request is re-checked in `request`, and `server_connect` is the
+        # authoritative rebinding-proof enforcement (resolve-once + IP pin). The check
+        # can do a blocking DNS lookup, so run it off the event loop.
+        if await asyncio.get_running_loop().run_in_executor(
+            None, destination_is_blocked, flow.request.host, flow.request.port
+        ):
+            logger.info(
+                "egress_denied_internal_destination phase=connect host=%s port=%s",
+                flow.request.host,
+                flow.request.port,
+            )
+            flow.response = http_403(SandboxProxyError.DESTINATION_BLOCKED)
+            return
+
         conn_id = getattr(flow.client_conn, "id", None)
         auth_header = flow.request.headers.get("Proxy-Authorization")
         tag = _parse_proxy_auth_username(auth_header)
@@ -197,6 +304,41 @@ class GateAddon:
         if conn_id is not None:
             self._conn_session_tags.pop(conn_id, None)
 
+    async def server_connect(self, data: server_hooks.ServerConnectionHookData) -> None:
+        """Deny internal destinations at connection-setup time (backstop).
+
+        Last hook before mitmproxy opens the upstream. Re-checking here — closer to
+        the actual connect than the earlier `http_connect`/`request` denies — shrinks
+        the DNS-rebinding window where a host that vetted as public re-resolves to an
+        internal address. A deny is a TCP-level kill (`server.error`); the structured
+        `destination_blocked` 403 is delivered by the `http_connect`/`request` checks
+        for every normal request.
+
+        We deliberately do NOT pin `server.address` to the resolved IP. Under the
+        default `eager` connection strategy mitmproxy completes the upstream TLS
+        handshake before the client's ClientHello is available; connecting by bare IP
+        makes the upstream cert check fail and mitmproxy silently falls back to a raw
+        passthrough tunnel — which skips credential injection (the LLM provider key is
+        never swapped in, so the sandbox's placeholder leaks and the call 401s).
+        Leaving the hostname in place keeps interception (and key injection) working.
+        The cost is a residual rebind window between this resolution and mitmproxy's
+        own: accepted as the safe trade-off versus breaking credential injection.
+        """
+        server = data.server
+        if server.error or not server.address:
+            return
+        host, port = server.address[0], server.address[1]
+        blocked = await asyncio.get_running_loop().run_in_executor(
+            None, destination_is_blocked, host, port
+        )
+        if blocked:
+            logger.info(
+                "egress_denied_internal_destination phase=server_connect host=%s port=%s",
+                host,
+                port,
+            )
+            server.error = "destination_blocked: internal address"
+
     def responseheaders(self, flow: http.HTTPFlow) -> None:
         """
         Streams the response body to the sandbox instead of buffering it whole.
@@ -214,6 +356,22 @@ class GateAddon:
             self._inflight_tasks.add(task)
             task.add_done_callback(self._inflight_tasks.discard)
 
+        # Deny forwards to internal addresses with a clear 403 — both plain-HTTP
+        # (GET http://internal/...) and the decrypted inner request of a MITM'd
+        # HTTPS tunnel, so the structured error reaches the agent regardless of
+        # scheme. `server_connect` is the authoritative rebinding-proof backstop +
+        # pin. Resolution can block, so run it off the event loop.
+        if await asyncio.get_running_loop().run_in_executor(
+            None, destination_is_blocked, flow.request.host, flow.request.port
+        ):
+            logger.info(
+                "egress_denied_internal_destination phase=request host=%s port=%s",
+                flow.request.host,
+                flow.request.port,
+            )
+            flow.response = http_403(SandboxProxyError.DESTINATION_BLOCKED)
+            return
+
         gate_target = await self._resolve_and_match(flow)
         # Strip the in-band session tag so it never reaches the origin
         flow.request.headers.pop("Proxy-Authorization", None)

backend/onyx/sandbox_proxy/errors.py

@@ -27,6 +27,7 @@ class SandboxProxyError(str, Enum):
     INTERNAL_ERROR = "internal_error"
     POLICY_DENIED = "policy_denied"
     CREDENTIAL_ERROR = "credential_error"
+    DESTINATION_BLOCKED = "destination_blocked"
 
     @property
     def message(self) -> str:
@@ -92,6 +93,13 @@ _SANDBOX_ERROR_MESSAGES: dict[SandboxProxyError, str] = {
         "connect or reconnect this integration in their Craft settings, then "
         "retry."
     ),
+    SandboxProxyError.DESTINATION_BLOCKED: (
+        "This request targets an internal network address that sandboxes are not "
+        "allowed to reach, so it was blocked before any connection was made. Only "
+        "the public internet and the Onyx API server are reachable from here. "
+        "This is a fixed security boundary, not a transient error — do not retry "
+        "against internal hosts (databases, caches, metadata endpoints, etc.)."
+    ),
 }
 
 

backend/tests/unit/sandbox_proxy/test_gate.py

@@ -25,7 +25,9 @@ from uuid import UUID
 from uuid import uuid4
 
 import pytest
+from mitmproxy import connection
 from mitmproxy import http
+from mitmproxy.proxy import server_hooks
 from redis.exceptions import RedisError
 
 from onyx.db.enums import ApprovalDecidedVia
@@ -1002,24 +1004,150 @@ def test_parse_proxy_auth_username(header: str | None, expected: str | None) ->
     assert gate._parse_proxy_auth_username(header) == expected
 
 
-def test_http_connect_caches_tag_and_client_disconnected_evicts() -> None:
+@pytest.mark.asyncio
+async def test_http_connect_caches_tag_and_client_disconnected_evicts(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setattr(gate, "destination_is_blocked", lambda _host, _port: False)
     addon = _build(resolver=StubResolver(), matcher=_StubMatcher())
     flow = make_flow(conn_id="conn-xyz", proxy_auth=_basic_auth(_TAG_UUID))
 
-    addon.http_connect(flow)
+    await addon.http_connect(flow)
     assert addon._conn_session_tags == {"conn-xyz": _TAG_UUID}
 
     addon.client_disconnected(flow.client_conn)
     assert addon._conn_session_tags == {}
 
 
-def test_http_connect_ignores_missing_or_garbled_header() -> None:
+@pytest.mark.asyncio
+async def test_http_connect_ignores_missing_or_garbled_header(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setattr(gate, "destination_is_blocked", lambda _host, _port: False)
     addon = _build(resolver=StubResolver(), matcher=_StubMatcher())
-    addon.http_connect(make_flow(conn_id="c1"))  # no Proxy-Authorization
-    addon.http_connect(make_flow(conn_id="c2", proxy_auth="Bearer nope"))
+    await addon.http_connect(make_flow(conn_id="c1"))  # no Proxy-Authorization
+    await addon.http_connect(make_flow(conn_id="c2", proxy_auth="Bearer nope"))
     assert addon._conn_session_tags == {}
 
 
+# ---------------------------------------------------------------------------
+# destination_is_blocked — internal-egress guard
+# ---------------------------------------------------------------------------
+
+
+def _addrinfo(*ips: str) -> list[Any]:
+    return [(2, 1, 6, "", (ip, 0)) for ip in ips]
+
+
+def test_destination_is_blocked_literal_ips() -> None:
+    assert gate.destination_is_blocked("10.0.0.1", 443) is True
+    assert gate.destination_is_blocked("169.254.169.254", 80) is True  # IMDS
+    assert gate.destination_is_blocked("::ffff:10.0.0.1", 443) is True  # mapped v4
+    assert gate.destination_is_blocked("8.8.8.8", 443) is False
+    assert gate.destination_is_blocked("", 443) is False
+
+
+def test_destination_is_blocked_resolves_to_internal(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setattr(
+        gate.socket, "getaddrinfo", lambda *_a, **_k: _addrinfo("10.1.2.3")
+    )
+    assert gate.destination_is_blocked("intra.svc.cluster.local", 443) is True
+
+
+def test_destination_is_blocked_resolves_to_public(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    monkeypatch.setattr(
+        gate.socket, "getaddrinfo", lambda *_a, **_k: _addrinfo("93.184.216.34")
+    )
+    assert gate.destination_is_blocked("example.com", 443) is False
+
+
+def test_destination_is_blocked_fails_closed_on_resolution_error(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A resolver failure must deny (fail closed), not allow relay."""
+
+    def _boom(*_args: Any, **_kwargs: Any) -> Any:
+        raise OSError("temporary DNS failure")
+
+    monkeypatch.setattr(gate.socket, "getaddrinfo", _boom)
+    assert gate.destination_is_blocked("flaky-host.example", 443) is True
+
+
+def test_api_server_exception_is_port_scoped(monkeypatch: pytest.MonkeyPatch) -> None:
+    """The api-server bypass must match host AND port; other ports on the same
+    internal host (Redis/Postgres) stay blocked."""
+    monkeypatch.setattr(gate, "_API_SERVER_HOST", "api.internal")
+    monkeypatch.setattr(gate, "_API_SERVER_PORT", 443)
+    # api host resolves to an internal IP, like a real in-cluster service name.
+    monkeypatch.setattr(
+        gate.socket, "getaddrinfo", lambda *_a, **_k: _addrinfo("10.5.5.5")
+    )
+    assert gate.destination_is_blocked("api.internal", 443) is False  # allowed
+    assert gate.destination_is_blocked("api.internal", 6379) is True  # Redis: blocked
+    assert gate.destination_is_blocked("api.internal", 5432) is True  # PG: blocked
+
+
+def test_destination_is_blocked_blocks_if_any_resolved_ip_internal(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """If a name resolves to a mix of public and internal IPs, deny — an attacker
+    can otherwise steer mitmproxy to the internal one."""
+    monkeypatch.setattr(
+        gate.socket,
+        "getaddrinfo",
+        lambda *_a, **_k: _addrinfo("93.184.216.34", "10.0.0.9"),
+    )
+    assert gate.destination_is_blocked("rebind.example", 443) is True
+
+
+def _server_hook_data(host: str, port: int) -> server_hooks.ServerConnectionHookData:
+    server = connection.Server(address=(host, port))
+    server.sni = host  # mitmproxy sets sni to the hostname before server_connect
+    return server_hooks.ServerConnectionHookData(
+        server=server, client=connection.Client(peername=("c", 0), sockname=("s", 0))
+    )
+
+
+@pytest.mark.asyncio
+async def test_server_connect_allows_public_without_pinning(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Public host: allowed through untouched. We must NOT pin the IP or alter
+    sni — pinning breaks eager-mode TLS interception and credential injection."""
+    monkeypatch.setattr(
+        gate.socket, "getaddrinfo", lambda *_a, **_k: _addrinfo("93.184.216.34")
+    )
+    addon = _build(resolver=StubResolver(), matcher=_StubMatcher())
+    data = _server_hook_data("example.com", 443)
+
+    await addon.server_connect(data)
+
+    assert data.server.error is None
+    assert data.server.address == ("example.com", 443)  # hostname left intact
+    assert data.server.sni == "example.com"
+
+
+@pytest.mark.asyncio
+async def test_server_connect_blocks_rebind_to_internal(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Backstop: a name that resolves to internal at connect-time is killed."""
+    monkeypatch.setattr(
+        gate.socket, "getaddrinfo", lambda *_a, **_k: _addrinfo("10.1.2.3")
+    )
+    addon = _build(resolver=StubResolver(), matcher=_StubMatcher())
+    data = _server_hook_data("rebind.example", 443)
+
+    await addon.server_connect(data)
+
+    assert data.server.error is not None
+    assert data.server.address == ("rebind.example", 443)
+
+
 # ---------------------------------------------------------------------------
 # _resolve_and_match — exact in-band session resolution
 # ---------------------------------------------------------------------------
@@ -1047,9 +1175,12 @@ async def test_resolve_and_match_exact_tag_on_http_request() -> None:
 
 
 @pytest.mark.asyncio
-async def test_resolve_and_match_exact_tag_on_https_connect() -> None:
+async def test_resolve_and_match_exact_tag_on_https_connect(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
     """HTTPS: the tag rode on the CONNECT (captured via http_connect)
     and is read back off the connection, not the MITM'd request."""
+    monkeypatch.setattr(gate, "destination_is_blocked", lambda _host, _port: False)
     user_id = uuid4()
     tagged_id = UUID(_TAG_UUID)
     sandbox = make_resolved_sandbox(user_id=user_id)
@@ -1057,7 +1188,7 @@ async def test_resolve_and_match_exact_tag_on_https_connect() -> None:
     addon = _build(resolver=resolver, matcher=_StubMatcher(result=_MATCH))
 
     connect_flow = make_flow(conn_id="conn-1", proxy_auth=_basic_auth(_TAG_UUID))
-    addon.http_connect(connect_flow)
+    await addon.http_connect(connect_flow)
     # Decrypted request has no Proxy-Authorization of its own.
     request_flow = make_flow(conn_id="conn-1")
 

backend/tests/unit/sandbox_proxy/test_response_streaming.py

@@ -32,6 +32,7 @@ from mitmproxy import http as mitm_http
 from mitmproxy.options import Options
 from mitmproxy.tools.dump import DumpMaster
 
+from onyx.sandbox_proxy.addons import gate
 from onyx.sandbox_proxy.addons.gate import _IdentityResolver
 from onyx.sandbox_proxy.addons.gate import GateAddon
 from onyx.sandbox_proxy.credential_injection import CredentialInjectionDispatcher
@@ -160,6 +161,14 @@ def upstream() -> Iterator[int]:
         server.server_close()
 
 
+@pytest.fixture(autouse=True)
+def _allow_loopback_egress(monkeypatch: pytest.MonkeyPatch) -> None:
+    """These tests route through 127.0.0.1 (loopback = internal), which the egress
+    guard correctly blocks in production. They exercise response streaming, not the
+    egress boundary, so bypass the guard here."""
+    monkeypatch.setattr(gate, "destination_is_blocked", lambda _host, _port: False)
+
+
 def _start_proxy(
     *, stream_responses: bool, confdir: Path, attempts: int = 5
 ) -> tuple[int, Callable[[], None]]: