diff --git a/backend/onyx/sandbox_proxy/addons/gate.py b/backend/onyx/sandbox_proxy/addons/gate.py index ec7265e2ce..ae1eb3d45a 100644 --- a/backend/onyx/sandbox_proxy/addons/gate.py +++ b/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) diff --git a/backend/onyx/sandbox_proxy/errors.py b/backend/onyx/sandbox_proxy/errors.py index 2c95b38162..a66a47f142 100644 --- a/backend/onyx/sandbox_proxy/errors.py +++ b/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.)." + ), } diff --git a/backend/tests/unit/sandbox_proxy/test_gate.py b/backend/tests/unit/sandbox_proxy/test_gate.py index ac169183bd..4a2640ed04 100644 --- a/backend/tests/unit/sandbox_proxy/test_gate.py +++ b/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") diff --git a/backend/tests/unit/sandbox_proxy/test_response_streaming.py b/backend/tests/unit/sandbox_proxy/test_response_streaming.py index f19df54e08..8d5926956b 100644 --- a/backend/tests/unit/sandbox_proxy/test_response_streaming.py +++ b/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]]: