diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index 9c54fcd5b3..548a8be05b 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -53,6 +53,7 @@ require_initialized, ) from openhands.agent_server.llm_router import llm_router +from openhands.agent_server.local_secret_resolver import local_secret_resolution from openhands.agent_server.mcp_router import mcp_router from openhands.agent_server.middleware import CORSDispatcher from openhands.agent_server.openai.router import ( @@ -154,6 +155,7 @@ def _cleanup_stale_tmux_sessions() -> None: @asynccontextmanager async def api_lifespan(api: FastAPI) -> AsyncIterator[None]: tmux_tmpdir, tmux_tmpdir_was_defaulted = _ensure_server_tmux_tmpdir() + secret_resolution: local_secret_resolution | None = None try: # Clean up stale tmux sessions from previous server runs _cleanup_stale_tmux_sessions() @@ -165,6 +167,11 @@ async def api_lifespan(api: FastAPI) -> AsyncIterator[None]: ) or create_conversation_registry(config) api.state.conversation_registry = conversation_registry + # Answer our own LookupSecret URLs in-process; a loopback fetch made + # from the event loop cannot be served by the loop blocked on it. + secret_resolution = local_secret_resolution(config) + secret_resolution.__enter__() + # Deferred pods boot with telemetry disabled and are rebuilt by # InitService, so they emit `server_started` there instead. api.state.telemetry_sink = await build_telemetry_sink(config) @@ -292,6 +299,8 @@ async def stop_tool_preload_service(): finally: # Outer finally so a startup failure cannot leak the drain task, and # after `async with service` so terminal events are still accepted. + if secret_resolution is not None: + secret_resolution.__exit__(None, None, None) emit_server_stopped() await shutdown_telemetry_sink() diff --git a/openhands-agent-server/openhands/agent_server/local_secret_resolver.py b/openhands-agent-server/openhands/agent_server/local_secret_resolver.py new file mode 100644 index 0000000000..11523c0a5c --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/local_secret_resolver.py @@ -0,0 +1,87 @@ +"""Serve this server's own ``LookupSecret`` URLs without a loopback request. + +``LookupSecret`` resolves lazily over HTTP so raw values never transit an SDK +client. When the client and the server are the same process, that round trip +buys nothing and costs correctness: a resolution on the event loop blocks the +very loop that would answer it, so it stalls until the client times out. +Resolving those URLs against the local store keeps the laziness and drops the +round trip. +""" + +import os +from urllib.parse import urlsplit + +from openhands.agent_server.config import Config +from openhands.agent_server.persistence import get_secrets_store +from openhands.sdk.logger import get_logger +from openhands.sdk.secret import ( + register_local_secret_resolver, + unregister_local_secret_resolver, +) + + +logger = get_logger(__name__) + +_INTERNAL_SERVER_URL_ENV = "OH_INTERNAL_SERVER_URL" +_DEFAULT_INTERNAL_SERVER_URL = "http://127.0.0.1:8000" +_SECRET_PATH_PREFIX = "/api/settings/secrets/" +_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "localhost", "::1"}) +_DEFAULT_PORTS = {"http": 80, "https": 443} + + +def _effective_port(scheme: str, port: int | None) -> int | None: + return port if port is not None else _DEFAULT_PORTS.get(scheme) + + +def _secret_name_if_local(url: str) -> str | None: + """Return the secret name when ``url`` is one this process serves itself.""" + parsed = urlsplit(url) + server = urlsplit(os.getenv(_INTERNAL_SERVER_URL_ENV, _DEFAULT_INTERNAL_SERVER_URL)) + if parsed.scheme != server.scheme: + return None + if _effective_port(parsed.scheme, parsed.port) != _effective_port( + server.scheme, server.port + ): + return None + if parsed.hostname != server.hostname and not { + parsed.hostname, + server.hostname, + }.issubset(_LOOPBACK_HOSTS): + return None + if not parsed.path.startswith(_SECRET_PATH_PREFIX): + return None + name = parsed.path[len(_SECRET_PATH_PREFIX) :] + # Only a bare name; anything deeper is a different route. + if not name or "/" in name: + return None + return name + + +def build_local_secret_resolver(config: Config): + """Build a resolver that answers this server's own secret URLs.""" + + def resolve(url: str) -> str | None: + name = _secret_name_if_local(url) + if name is None: + return None + value = get_secrets_store(config).get_secret(name) + if value is None: + # Fall through to HTTP so the caller still sees the server's 404. + return None + logger.debug("Resolved secret '%s' in-process", name) + return value + + return resolve + + +class local_secret_resolution: + """Context manager registering the in-process resolver for a server.""" + + def __init__(self, config: Config) -> None: + self._resolver = build_local_secret_resolver(config) + + def __enter__(self) -> None: + register_local_secret_resolver(self._resolver) + + def __exit__(self, *exc_info) -> None: + unregister_local_secret_resolver(self._resolver) diff --git a/openhands-sdk/openhands/sdk/secret/__init__.py b/openhands-sdk/openhands/sdk/secret/__init__.py index 7907d3662c..34c99d9cb8 100644 --- a/openhands-sdk/openhands/sdk/secret/__init__.py +++ b/openhands-sdk/openhands/sdk/secret/__init__.py @@ -4,16 +4,22 @@ """ from openhands.sdk.secret.secrets import ( + LocalSecretResolver, LookupSecret, SecretSource, SecretValue, StaticSecret, + register_local_secret_resolver, + unregister_local_secret_resolver, ) __all__ = [ + "LocalSecretResolver", "SecretSource", "StaticSecret", "LookupSecret", "SecretValue", + "register_local_secret_resolver", + "unregister_local_secret_resolver", ] diff --git a/openhands-sdk/openhands/sdk/secret/secrets.py b/openhands-sdk/openhands/sdk/secret/secrets.py index 78be9c314d..b2cccc56cf 100644 --- a/openhands-sdk/openhands/sdk/secret/secrets.py +++ b/openhands-sdk/openhands/sdk/secret/secrets.py @@ -2,6 +2,7 @@ import os from abc import ABC, abstractmethod +from collections.abc import Callable from urllib.parse import urljoin, urlsplit import httpx @@ -32,6 +33,50 @@ def _resolve_lookup_secret_url(url: str) -> str: return urljoin(f"{base_url.rstrip('/')}/", url) +LocalSecretResolver = Callable[[str], str | None] + +_local_secret_resolvers: list[LocalSecretResolver] = [] + + +def register_local_secret_resolver(resolver: LocalSecretResolver) -> None: + """Register an in-process resolver for ``LookupSecret`` URLs. + + A process that both serves and consumes ``LookupSecret`` URLs — an + agent-server running an in-process conversation, say — cannot fetch them + over HTTP from its own event loop: the request can only be answered by the + loop that is blocked waiting for it, so it stalls until the client times + out. Registering a resolver lets such a process answer its own URLs + directly and skip the round trip. + + The resolver receives the fully-resolved URL and returns the secret value, + or ``None`` when it does not serve that URL, in which case resolution falls + through to the next resolver and finally to HTTP. + """ + if resolver not in _local_secret_resolvers: + _local_secret_resolvers.append(resolver) + + +def unregister_local_secret_resolver(resolver: LocalSecretResolver) -> None: + """Remove a resolver registered with ``register_local_secret_resolver``.""" + if resolver in _local_secret_resolvers: + _local_secret_resolvers.remove(resolver) + + +def _resolve_secret_locally(url: str) -> str | None: + """Return a locally-served value for ``url``, or ``None`` if there is none.""" + for resolver in list(_local_secret_resolvers): + try: + value = resolver(url) + except Exception: + logger.warning( + "Local secret resolver raised; falling back to HTTP", exc_info=True + ) + continue + if value is not None: + return value + return None + + class SecretSource(DiscriminatedUnionMixin, ABC): """Source for a named secret which may be obtained dynamically""" @@ -77,6 +122,9 @@ def _normalize_url(cls, url: str) -> str: return _resolve_lookup_secret_url(url) def get_value(self) -> str: + local_value = _resolve_secret_locally(self.url) + if local_value is not None: + return local_value response = httpx.get(self.url, headers=self.headers, timeout=30.0) response.raise_for_status() return response.text diff --git a/tests/agent_server/test_local_secret_resolver.py b/tests/agent_server/test_local_secret_resolver.py new file mode 100644 index 0000000000..ddd7f65772 --- /dev/null +++ b/tests/agent_server/test_local_secret_resolver.py @@ -0,0 +1,85 @@ +"""Tests for in-process resolution of this server's own LookupSecret URLs.""" + +from collections.abc import Iterator +from contextlib import contextmanager +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from threading import Thread + +from openhands.agent_server.config import Config +from openhands.agent_server.local_secret_resolver import ( + _secret_name_if_local, + build_local_secret_resolver, +) +from openhands.agent_server.persistence import get_secrets_store +from openhands.sdk.secret import ( + LookupSecret, + register_local_secret_resolver, + unregister_local_secret_resolver, +) + + +class _ForeignSecretHandler(BaseHTTPRequestHandler): + def do_GET(self) -> None: + self.send_response(200) + self.end_headers() + self.wfile.write(b"foreign-value") + + def log_message(self, format: str, *args: object) -> None: + pass + + +@contextmanager +def _foreign_secret_server() -> Iterator[str]: + server = ThreadingHTTPServer(("127.0.0.1", 0), _ForeignSecretHandler) + thread = Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + host = server.server_address[0] + port = server.server_address[1] + yield f"http://{host}:{port}" + finally: + server.shutdown() + thread.join() + server.server_close() + + +def test_matches_loopback_secret_url(monkeypatch): + monkeypatch.setenv("OH_INTERNAL_SERVER_URL", "http://127.0.0.1:8000") + assert ( + _secret_name_if_local("http://127.0.0.1:8000/api/settings/secrets/TOKEN") + == "TOKEN" + ) + assert ( + _secret_name_if_local("http://localhost:8000/api/settings/secrets/TOKEN") + == "TOKEN" + ) + + +def test_ignores_remote_host(monkeypatch): + monkeypatch.setenv("OH_INTERNAL_SERVER_URL", "http://127.0.0.1:8000") + assert ( + _secret_name_if_local("https://remote.example/api/settings/secrets/T") is None + ) + + +def test_ignores_other_routes_and_malformed_names(monkeypatch): + monkeypatch.setenv("OH_INTERNAL_SERVER_URL", "http://127.0.0.1:8000") + assert _secret_name_if_local("http://127.0.0.1:8000/api/settings") is None + assert _secret_name_if_local("http://127.0.0.1:8000/api/settings/secrets/") is None + assert ( + _secret_name_if_local("http://127.0.0.1:8000/api/settings/secrets/a/b") is None + ) + + +def test_foreign_loopback_port_uses_its_own_server(monkeypatch): + monkeypatch.setenv("OH_INTERNAL_SERVER_URL", "http://127.0.0.1:18000") + config = Config() + get_secrets_store(config).set_secret("TOKEN", "local-value") + resolver = build_local_secret_resolver(config) + register_local_secret_resolver(resolver) + try: + with _foreign_secret_server() as base_url: + secret = LookupSecret(url=f"{base_url}/api/settings/secrets/TOKEN") + assert secret.get_value() == "foreign-value" + finally: + unregister_local_secret_resolver(resolver) diff --git a/tests/sdk/conversation/test_secret_source.py b/tests/sdk/conversation/test_secret_source.py index d838d8a247..a823988f37 100644 --- a/tests/sdk/conversation/test_secret_source.py +++ b/tests/sdk/conversation/test_secret_source.py @@ -302,3 +302,78 @@ def test_lookup_secret_get_value_resolves_relative_url(monkeypatch): headers={}, timeout=30.0, ) + + +@pytest.fixture +def local_resolvers_cleared(): + """Keep the process-wide resolver registry isolated per test.""" + from openhands.sdk.secret import secrets as secrets_module + + original = list(secrets_module._local_secret_resolvers) + secrets_module._local_secret_resolvers.clear() + yield secrets_module._local_secret_resolvers + secrets_module._local_secret_resolvers[:] = original + + +def test_local_resolver_short_circuits_http(local_resolvers_cleared): + """A matching resolver answers without any HTTP request. + + This is the deadlock guard: resolving over loopback from the event loop + blocks the loop that would serve the request. + """ + from openhands.sdk.secret import register_local_secret_resolver + + secret = LookupSecret(url="http://127.0.0.1:8000/api/settings/secrets/TOKEN") + register_local_secret_resolver( + lambda url: "resolved-locally" if url.endswith("/TOKEN") else None + ) + + with patch("httpx.get", side_effect=AssertionError("HTTP must not be used")): + assert secret.get_value() == "resolved-locally" + + +def test_local_resolver_declining_falls_back_to_http(local_resolvers_cleared): + """A resolver returning None leaves the URL to HTTP.""" + from openhands.sdk.secret import register_local_secret_resolver + + secret = LookupSecret(url="https://remote.example/api/settings/secrets/TOKEN") + register_local_secret_resolver(lambda url: None) + + response = Mock(text="from-http") + response.raise_for_status = Mock() + with patch("httpx.get", return_value=response) as http_get: + assert secret.get_value() == "from-http" + http_get.assert_called_once() + + +def test_local_resolver_raising_falls_back_to_http(local_resolvers_cleared): + """A broken resolver must not take secret resolution down with it.""" + from openhands.sdk.secret import register_local_secret_resolver + + def boom(url: str) -> str | None: + raise RuntimeError("resolver is broken") + + secret = LookupSecret(url="https://remote.example/api/settings/secrets/TOKEN") + register_local_secret_resolver(boom) + + response = Mock(text="from-http") + response.raise_for_status = Mock() + with patch("httpx.get", return_value=response): + assert secret.get_value() == "from-http" + + +def test_unregister_local_secret_resolver(local_resolvers_cleared): + from openhands.sdk.secret import ( + register_local_secret_resolver, + unregister_local_secret_resolver, + ) + + def resolver(url: str) -> str | None: + return "resolved-locally" + + register_local_secret_resolver(resolver) + register_local_secret_resolver(resolver) + assert len(local_resolvers_cleared) == 1 + + unregister_local_secret_resolver(resolver) + assert local_resolvers_cleared == []