Skip to content

Commit 51de333

Browse files
bokelleyclaude
andauthored
fix(adagents): pin DNS to close rebinding TOCTOU on SSRF gate (#920)
The three publisher-controlled fetch paths in adagents.py ran the _dns_validate_host resolve-and-gate pre-check, then built a plain httpx.AsyncClient() that re-resolved the host at connect time — leaving the DNS-rebinding window the pre-check docstring admits. Thread build_async_ip_pinned_transport into the SDK-owned AsyncClient construction sites (ads.txt MANAGERDOMAIN fetch, adagents.json stream, AAO directory fetch) via a new _owned_pinned_client helper. The client now pins to the IP the SSRF gate validated, so pre-check and connect observe the same resolution. SSRFValidationError is mapped onto AdagentsValidationError. Injected-client branches keep _dns_validate_host as their only guard since the SDK does not own that transport. Closes #757 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 70abb5a commit 51de333

2 files changed

Lines changed: 164 additions & 3 deletions

File tree

src/adcp/adagents.py

Lines changed: 57 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,43 @@ async def _dns_validate_host(host: str, port: int) -> None:
269269
_check_safe_host(addr, "resolved address")
270270

271271

272+
def _owned_pinned_client(url: str, timeout: float) -> httpx.AsyncClient:
273+
"""Build an SDK-owned ``AsyncClient`` pinned to ``url``'s validated IP.
274+
275+
Resolves the host once via :func:`resolve_and_validate_host` and wires
276+
the resulting IP into an :class:`AsyncIpPinnedTransport`, so httpx
277+
connects to the address the SSRF gate approved instead of re-resolving
278+
at connect time. This is what closes the DNS-rebinding TOCTOU that the
279+
:func:`_dns_validate_host` pre-check alone leaves open: the pre-check
280+
and the connect now observe the *same* resolution.
281+
282+
``trust_env=False`` so an ``HTTPS_PROXY`` / ``HTTP_PROXY`` in the
283+
environment can't route the request through a proxy pool that ignores
284+
the pinned backend — that would reopen the same TOCTOU.
285+
286+
Only call this from branches where the SDK owns transport construction.
287+
When a caller injects their own client the SDK does not control the
288+
transport, so the pre-check remains the only available guard there.
289+
290+
Raises:
291+
AdagentsValidationError: If the host doesn't resolve or every
292+
resolved address is in a blocked/reserved range. Maps the
293+
transport layer's :class:`SSRFValidationError` onto the
294+
adagents error type so callers see one exception family.
295+
"""
296+
# Lazy import: keeps httpcore (a transport-only dependency) off the
297+
# adagents module-load path and avoids a load-time cycle, matching
298+
# adcp.signing.jwks.default_jwks_fetcher.
299+
from adcp.signing.ip_pinned_transport import build_async_ip_pinned_transport
300+
from adcp.signing.jwks import SSRFValidationError
301+
302+
try:
303+
transport = build_async_ip_pinned_transport(url)
304+
except SSRFValidationError as e:
305+
raise AdagentsValidationError(f"SSRF validation failed for {url!r}: {e}") from e
306+
return httpx.AsyncClient(transport=transport, timeout=timeout, trust_env=False)
307+
308+
272309
def _validate_publisher_domain(domain: str) -> str:
273310
"""Validate and sanitize publisher domain for security.
274311
@@ -716,7 +753,7 @@ async def _fetch_ads_txt_managerdomains(
716753
url, headers=headers, timeout=timeout, follow_redirects=False
717754
)
718755
else:
719-
async with httpx.AsyncClient() as new_client:
756+
async with _owned_pinned_client(url, timeout) as new_client:
720757
response = await new_client.get(
721758
url, headers=headers, timeout=timeout, follow_redirects=False
722759
)
@@ -727,6 +764,12 @@ async def _fetch_ads_txt_managerdomains(
727764
return _parse_managerdomains(response.text)
728765
except (httpx.TimeoutException, httpx.RequestError):
729766
return []
767+
except AdagentsValidationError:
768+
# The pinned-transport build re-resolves the host; if it now points
769+
# at a blocked address (DNS rebinding between the pre-check and the
770+
# connect), fail closed. This fallback is best-effort, so a blocked
771+
# resolution is "no MANAGERDOMAIN found", same as a network error.
772+
return []
730773

731774

732775
def _ensure_safe_manager_domain(manager_domain: str) -> str | None:
@@ -1056,13 +1099,20 @@ async def _fetch_adagents_url(
10561099
parsed.hostname or "", parsed.port or (443 if parsed.scheme == "https" else 80)
10571100
)
10581101

1102+
# When the SDK owns the client, pin it to the validated IP so httpx
1103+
# connects to the address the SSRF gate approved rather than re-resolving
1104+
# at connect time. A failed resolve/SSRF check surfaces from
1105+
# _owned_pinned_client as AdagentsValidationError — not an httpx error, so
1106+
# it propagates past the handlers below, which is the correct fail-closed
1107+
# outcome for the primary fetch path (unlike the best-effort ads.txt
1108+
# fallback, we do NOT swallow it).
10591109
try:
10601110
if client is not None:
10611111
body, status_code, response_headers = await _stream_capped(
10621112
client, url, headers, timeout, max_bytes
10631113
)
10641114
else:
1065-
async with httpx.AsyncClient() as new_client:
1115+
async with _owned_pinned_client(url, timeout) as new_client:
10661116
body, status_code, response_headers = await _stream_capped(
10671117
new_client, url, headers, timeout, max_bytes
10681118
)
@@ -2207,13 +2257,17 @@ async def fetch_agent_authorizations_from_directory(
22072257

22082258
headers = {"User-Agent": "AdCP-Client/1.0", "Accept": "application/json"}
22092259

2260+
# SDK-owned client is pinned to the validated IP (see _fetch_adagents_url).
2261+
# A failed resolve/SSRF check raises AdagentsValidationError, which
2262+
# propagates past the httpx handlers below — the correct fail-closed
2263+
# outcome (we do not convert it into an empty result).
22102264
try:
22112265
if client is not None:
22122266
body, status_code, _ = await _stream_capped(
22132267
client, request_url, headers, timeout, MAX_DIRECTORY_PAGE_BYTES
22142268
)
22152269
else:
2216-
async with httpx.AsyncClient() as new_client:
2270+
async with _owned_pinned_client(request_url, timeout) as new_client:
22172271
body, status_code, _ = await _stream_capped(
22182272
new_client, request_url, headers, timeout, MAX_DIRECTORY_PAGE_BYTES
22192273
)

tests/test_adagents.py

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -870,6 +870,113 @@ def _resolve_mixed(host, port, *args, **kwargs):
870870
with pytest.raises(AdagentsValidationError, match="private/reserved"):
871871
await fetch_adagents("split-horizon.realhost.org")
872872

873+
@pytest.mark.asyncio
874+
async def test_rebinding_after_precheck_connects_to_pinned_public_ip(self, monkeypatch):
875+
# DNS rebinding TOCTOU (issue #757): a hostile resolver returns a
876+
# PUBLIC IP while the SSRF gate runs, then flips to a private/loopback
877+
# IP at connect time. With the SDK-owned client pinned to the IP the
878+
# gate validated, httpx must connect to the pinned PUBLIC address and
879+
# never re-resolve into the private one.
880+
from adcp.adagents import fetch_adagents
881+
882+
public_ip = "93.184.216.34"
883+
private_ip = "127.0.0.1"
884+
885+
# First two getaddrinfo calls (the _dns_validate_host pre-check and the
886+
# pinned-transport build) see the public IP; any later call — which
887+
# would only happen if httpx re-resolved at connect — flips to private.
888+
call_count = {"n": 0}
889+
890+
def _rebinding_resolve(host, port, *args, **kwargs):
891+
call_count["n"] += 1
892+
ip = public_ip if call_count["n"] <= 2 else private_ip
893+
return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (ip, port))]
894+
895+
monkeypatch.setattr(socket, "getaddrinfo", _rebinding_resolve)
896+
897+
# Intercept the network backend's actual TCP connect (the layer the
898+
# pinned backend delegates to after rewriting host -> resolved IP) to
899+
# capture the destination IP, then short-circuit before TLS.
900+
import httpcore
901+
from httpcore._backends.anyio import AnyIOBackend
902+
903+
connected_hosts: list[str] = []
904+
905+
async def _capture_connect(self, host, port, *args, **kwargs):
906+
connected_hosts.append(host)
907+
raise httpcore.ConnectError("intercepted before real connect")
908+
909+
monkeypatch.setattr(AnyIOBackend, "connect_tcp", _capture_connect)
910+
911+
# The connect is short-circuited, so the fetch fails — but as a
912+
# validation error wrapping a network failure, not by reaching a
913+
# private address.
914+
with pytest.raises(AdagentsValidationError):
915+
await fetch_adagents("rebinding.realhost.org")
916+
917+
# The pin held: every connect targeted the validated public IP, and
918+
# the loopback address the resolver flipped to was never reached.
919+
assert connected_hosts, "expected the pinned transport to attempt a connect"
920+
assert all(h == public_ip for h in connected_hosts), connected_hosts
921+
assert private_ip not in connected_hosts
922+
923+
@pytest.mark.asyncio
924+
async def test_public_target_resolves_and_pins_then_serves_body(self, monkeypatch):
925+
# Acceptance criterion: a legitimately public target still works end to
926+
# end through the pinned, SDK-owned client — resolution succeeds, the
927+
# connection pins to the public IP, and the adagents.json body is
928+
# served and parsed.
929+
from adcp.adagents import fetch_adagents
930+
931+
public_ip = "93.184.216.34"
932+
933+
def _resolve_public(host, port, *args, **kwargs):
934+
return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (public_ip, port))]
935+
936+
monkeypatch.setattr(socket, "getaddrinfo", _resolve_public)
937+
938+
body = json.dumps(
939+
{
940+
"$schema": "/schemas/2.6.0/adagents.json",
941+
"authorized_agents": [
942+
{
943+
"url": "https://agent.example.com",
944+
"authorized_for": "Example inventory",
945+
"authorization_type": "property_ids",
946+
"property_ids": ["site1"],
947+
}
948+
],
949+
"last_updated": "2025-01-15T10:00:00Z",
950+
}
951+
).encode("utf-8")
952+
953+
# Spy on the SDK's owned-client builder: let it construct the REAL
954+
# pinned transport (so resolution + IP selection run for real), record
955+
# the IP it pinned, then serve the response body via a MockTransport so
956+
# the test stays offline.
957+
import adcp.adagents as adagents_mod
958+
from adcp.signing.ip_pinned_transport import build_async_ip_pinned_transport
959+
960+
pinned_ips: list[str] = []
961+
962+
def _spy_owned_pinned_client(url, timeout):
963+
transport = build_async_ip_pinned_transport(url)
964+
# AsyncIpPinnedTransport pins to a single resolved IP; surface it
965+
# so the test can assert the validated public IP was chosen.
966+
pinned_ips.append(transport._pool._network_backend._resolved_ip)
967+
968+
def _handler(request: httpx.Request) -> httpx.Response:
969+
return httpx.Response(200, content=body)
970+
971+
return httpx.AsyncClient(transport=httpx.MockTransport(_handler), timeout=timeout)
972+
973+
monkeypatch.setattr(adagents_mod, "_owned_pinned_client", _spy_owned_pinned_client)
974+
975+
result = await fetch_adagents("legit.realhost.org")
976+
977+
assert result["authorized_agents"][0]["url"] == "https://agent.example.com"
978+
assert pinned_ips == [public_ip], pinned_ips
979+
873980
@pytest.mark.asyncio
874981
async def test_redirect_uses_fresh_client(self):
875982
"""Redirect hops should not reuse the caller's client."""

0 commit comments

Comments
 (0)