Skip to content

Commit 969f373

Browse files
bokelleyclaude
andauthored
fix(adagents): resolve-and-validate gate against DNS-based SSRF (#757, partial) (#760)
Closes the common case where a public-looking hostname's authoritative DNS points at a private address — e.g. `metadata.example.com` → `169.254.169.254`. `_check_safe_host` is purely string-level and would let this pass; the new `_dns_validate_host` resolves the hostname once via `socket.getaddrinfo` (in an executor) and gates every returned address through the same IP-block check. Plumbed in front of every outbound HTTP request in adagents discovery: - `_fetch_adagents_url` (publisher hop, authoritative_location hop, conditional-refresh hop) - `_fetch_ads_txt_managerdomains` (MANAGERDOMAIN fallback fetch — fails closed to "no MANAGERDOMAIN" rather than surfacing a validation error, matching the existing best-effort semantics of that path) IP literals short-circuit (already gated by `_check_safe_host`). RFC 2606 / 6761 reserved domains (`.example`, `.test`, `.invalid`, `.localhost`, `example.com`/`.net`/`.org`) also short-circuit so tests using these names don't need DNS mocks. Test infrastructure adds an autouse `_stub_getaddrinfo` fixture in test_adagents.py that returns a benign public IP (`8.8.8.8`) for every host by default. Tests that want to exercise the DNS gate (`test_resolved_private_ip_rejected_before_connect`, `test_resolved_dns_failure_surfaces_as_validation_error`, `test_resolved_mixed_public_and_private_is_rejected`) override via `monkeypatch.setattr(socket, "getaddrinfo", ...)`. Residual rebinding window (still open after this PR): a determined attacker controlling the authoritative DNS can return a public IP on this lookup and a private IP on httpx's connect lookup milliseconds later. Closing that window requires intercepting httpx's network backend to pin the connection IP — left tracked on #757 as the remaining work after this resolve-and-validate gate ships. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 01c6491 commit 969f373

2 files changed

Lines changed: 139 additions & 0 deletions

File tree

src/adcp/adagents.py

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,11 @@
88
for sales agents to verify they are authorized for specific properties.
99
"""
1010

11+
import asyncio
1112
import ipaddress
1213
import json
1314
import re
15+
import socket
1416
from dataclasses import dataclass, field
1517
from typing import Any, Literal
1618
from urllib.parse import urlparse
@@ -199,6 +201,61 @@ def _check_safe_host(hostname: str, context: str) -> None:
199201
raise AdagentsValidationError(f"{context} must not target a private/reserved address")
200202

201203

204+
# Reserved TLDs (RFC 6761) and second-level domains (RFC 2606) that never
205+
# resolve in the public DNS. Tests use these consistently; skipping the
206+
# resolve gate on them saves every test from having to mock
207+
# socket.getaddrinfo while still applying the gate to real-world hostnames.
208+
_NON_RESOLVING_SUFFIXES: tuple[str, ...] = (
209+
".example",
210+
".test",
211+
".invalid",
212+
".localhost",
213+
".example.com",
214+
".example.net",
215+
".example.org",
216+
)
217+
_NON_RESOLVING_HOSTS: frozenset[str] = frozenset({"example.com", "example.net", "example.org"})
218+
219+
220+
async def _dns_validate_host(host: str, port: int) -> None:
221+
"""Resolve a hostname once and gate every returned address.
222+
223+
Closes the common SSRF path where a public-looking hostname's DNS
224+
points at a private address (e.g. ``metadata.example.com`` →
225+
``169.254.169.254``). ``_check_safe_host`` only sees the string;
226+
this helper sees what the resolver returns.
227+
228+
Residual rebinding window: a determined attacker controlling the
229+
authoritative DNS can still return a public IP on this lookup and
230+
a private IP on httpx's connect lookup milliseconds later. Closing
231+
that window requires intercepting httpx's network backend to pin
232+
the connection IP — tracked separately.
233+
"""
234+
if not host:
235+
return
236+
try:
237+
ipaddress.ip_address(host)
238+
return # IP literal — already gated by _check_safe_host
239+
except ValueError:
240+
pass
241+
if host in _NON_RESOLVING_HOSTS or host.endswith(_NON_RESOLVING_SUFFIXES):
242+
return
243+
loop = asyncio.get_running_loop()
244+
try:
245+
# Run via executor (not loop.getaddrinfo) so tests can mock
246+
# socket.getaddrinfo at the C-level entry point — patching the
247+
# concrete event loop's bound method doesn't intercept reliably.
248+
infos = await loop.run_in_executor(
249+
None, lambda: socket.getaddrinfo(host, port, type=socket.SOCK_STREAM)
250+
)
251+
except socket.gaierror as e:
252+
raise AdagentsValidationError(f"DNS resolution failed for {host!r}: {e}") from e
253+
for info in infos:
254+
addr = info[4][0]
255+
if isinstance(addr, str):
256+
_check_safe_host(addr, "resolved address")
257+
258+
202259
def _validate_publisher_domain(domain: str) -> str:
203260
"""Validate and sanitize publisher domain for security.
204261
@@ -636,6 +693,10 @@ async def _fetch_ads_txt_managerdomains(
636693
"""
637694
url = f"https://{publisher_domain}/ads.txt"
638695
headers = {"User-Agent": user_agent, "Accept": "text/plain"}
696+
try:
697+
await _dns_validate_host(publisher_domain, 443)
698+
except AdagentsValidationError:
699+
return []
639700
try:
640701
if client is not None:
641702
response = await client.get(
@@ -970,6 +1031,11 @@ async def _fetch_adagents_url(
9701031
if cache_entry.last_modified:
9711032
headers["If-Modified-Since"] = cache_entry.last_modified
9721033

1034+
parsed = urlparse(url)
1035+
await _dns_validate_host(
1036+
parsed.hostname or "", parsed.port or (443 if parsed.scheme == "https" else 80)
1037+
)
1038+
9731039
try:
9741040
if client is not None:
9751041
body, status_code, response_headers = await _stream_capped(

tests/test_adagents.py

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
"""Tests for adagents.json validation functionality."""
44

55
import json
6+
import socket
67
import unittest.mock
78
from unittest.mock import AsyncMock, MagicMock
89

@@ -26,6 +27,27 @@
2627
)
2728

2829

30+
@pytest.fixture(autouse=True)
31+
def _stub_getaddrinfo(monkeypatch):
32+
"""Stub socket.getaddrinfo to a benign public IP for every test.
33+
34+
The DNS pre-check in `_dns_validate_host` calls
35+
`socket.getaddrinfo` (via an executor) for every outbound URL whose
36+
host isn't a reserved RFC 2606 / 6761 name. Without this stub, tests
37+
that use arbitrary public-looking hostnames (e.g.
38+
`cdn.other-domain.com`) either hit live DNS or fail in CI
39+
environments without resolution.
40+
41+
Tests that specifically want to exercise the DNS gate override this
42+
fixture or call `monkeypatch.setattr` on the same target.
43+
"""
44+
45+
def _fake_resolve(host, port, *args, **kwargs):
46+
return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", ("8.8.8.8", port))]
47+
48+
monkeypatch.setattr(socket, "getaddrinfo", _fake_resolve)
49+
50+
2951
def _make_stream_response(
3052
*,
3153
status_code: int,
@@ -756,6 +778,57 @@ async def test_rejects_dot_local_redirect(self):
756778
with pytest.raises(AdagentsValidationError, match="localhost"):
757779
await fetch_adagents("example.com", client=mock_client)
758780

781+
@pytest.mark.asyncio
782+
async def test_resolved_private_ip_rejected_before_connect(self, monkeypatch):
783+
# A public-looking hostname whose DNS points at a private address
784+
# must be rejected by the resolve-and-validate pre-check, not
785+
# silently connected to. Closes the string-level gap left by
786+
# _check_safe_host alone.
787+
from adcp.adagents import fetch_adagents
788+
789+
def _resolve_to_private(host, port, *args, **kwargs):
790+
return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", ("169.254.169.254", port))]
791+
792+
monkeypatch.setattr(socket, "getaddrinfo", _resolve_to_private)
793+
794+
# Use a hostname that isn't in the RFC 2606 / 6761 reserved list
795+
# so the resolve pre-check actually runs.
796+
with pytest.raises(AdagentsValidationError, match="private/reserved"):
797+
await fetch_adagents("metadata.realhost.org")
798+
799+
@pytest.mark.asyncio
800+
async def test_resolved_dns_failure_surfaces_as_validation_error(self, monkeypatch):
801+
# A DNS gaierror (NXDOMAIN, transient SERVFAIL) becomes a clear
802+
# AdagentsValidationError rather than bubbling raw socket errors
803+
# through the SDK boundary.
804+
from adcp.adagents import fetch_adagents
805+
806+
def _resolve_fails(host, port, *args, **kwargs):
807+
raise socket.gaierror(-2, "Name or service not known")
808+
809+
monkeypatch.setattr(socket, "getaddrinfo", _resolve_fails)
810+
811+
with pytest.raises(AdagentsValidationError, match="DNS resolution failed"):
812+
await fetch_adagents("nonexistent.realhost.org")
813+
814+
@pytest.mark.asyncio
815+
async def test_resolved_mixed_public_and_private_is_rejected(self, monkeypatch):
816+
# If a hostname resolves to a list of addresses where ANY one is
817+
# private, the SDK must reject — defending against split-horizon
818+
# DNS that returns both a public and a private address.
819+
from adcp.adagents import fetch_adagents
820+
821+
def _resolve_mixed(host, port, *args, **kwargs):
822+
return [
823+
(socket.AF_INET, socket.SOCK_STREAM, 6, "", ("8.8.8.8", port)),
824+
(socket.AF_INET, socket.SOCK_STREAM, 6, "", ("10.0.0.5", port)),
825+
]
826+
827+
monkeypatch.setattr(socket, "getaddrinfo", _resolve_mixed)
828+
829+
with pytest.raises(AdagentsValidationError, match="private/reserved"):
830+
await fetch_adagents("split-horizon.realhost.org")
831+
759832
@pytest.mark.asyncio
760833
async def test_redirect_uses_fresh_client(self):
761834
"""Redirect hops should not reuse the caller's client."""

0 commit comments

Comments
 (0)