diff --git a/examples/v3_reference_seller/src/tenant_router.py b/examples/v3_reference_seller/src/tenant_router.py index 84e9eafc..96e9334a 100644 --- a/examples/v3_reference_seller/src/tenant_router.py +++ b/examples/v3_reference_seller/src/tenant_router.py @@ -24,6 +24,9 @@ from adcp.server import Tenant +# Not yet re-exported from ``adcp.server``; import from the submodule. +from adcp.server.tenant_router import normalize_host_key + if TYPE_CHECKING: from sqlalchemy.ext.asyncio import async_sessionmaker @@ -49,13 +52,15 @@ def __init__( self._cache_lock = asyncio.Lock() async def resolve(self, host: str) -> Tenant | None: - # The middleware passes the raw Host header. RFC 7230 makes it - # case-insensitive and lets the client include ``:port``; the - # Protocol docstring is explicit that implementations strip the - # port suffix as needed. Normalize before the cache lookup AND - # the DB query so ``acme.localhost:3001`` resolves the same - # row as the seeded ``acme.localhost``. - host = host.strip().lower().split(":", 1)[0] + # The middleware passes the raw Host header. Use the SDK's shared + # normalizer rather than hand-rolling a lower-case + port split: + # a naive ``split(":", 1)`` mangles bracketed IPv6 authorities, + # and rolling your own guarantees it drifts from the key the rest + # of the SDK uses. Normalize before the cache lookup AND the DB + # query so ``acme.localhost:3001`` resolves the same row as the + # seeded ``acme.localhost``. Note the ``tenants.host`` column must + # be seeded in this same form (IPv6 de-bracketed, e.g. ``::1``). + host = normalize_host_key(host) # Bounded FIFO cache — when full, the oldest insertion is # evicted regardless of access frequency. Fine for stable # tenant sets under ``cache_size``; adopters with churn or diff --git a/src/adcp/server/tenant_registry.py b/src/adcp/server/tenant_registry.py index 4ac783e9..3a8caa44 100644 --- a/src/adcp/server/tenant_registry.py +++ b/src/adcp/server/tenant_registry.py @@ -35,7 +35,8 @@ from collections.abc import Awaitable, Callable from dataclasses import dataclass from typing import TYPE_CHECKING, Any, Literal -from urllib.parse import urlparse + +from adcp.server.tenant_router import normalize_host_key if TYPE_CHECKING: from adcp.decisioning.accounts import AccountStore @@ -226,10 +227,18 @@ def _get_lock(self, tenant_id: str) -> asyncio.Lock: @staticmethod def _normalize_host(raw: str) -> str: - """Lower-case and strip any port suffix from a host or URL. + """Reduce a host or URL to its tenant-lookup key. Accepts both full URLs (``https://acme.example.com``) and raw Host-header values (``acme.example.com``, ``acme.example.com:443``). + Delegates to :func:`~adcp.server.tenant_router.normalize_host_key` + so that a tenant registered by ``agent_url`` is reachable by the + ``Host`` header the subdomain router resolves — the two used to + key the same address differently. + + Beyond lower-casing and port stripping this discards any + ``user:pw@`` userinfo, removes IPv6 brackets (``[::1]:8443`` is + keyed as ``::1``), and folds a trailing FQDN-root dot. Note: port stripping is correct for ``Host`` headers where the port matches the scheme default. Some load-balancers forward @@ -237,13 +246,7 @@ def _normalize_host(raw: str) -> str: using that header should strip the port themselves before passing the value to :meth:`resolve_by_host` or :meth:`resolve`. """ - if "://" in raw: - host = urlparse(raw).netloc or raw - else: - host = raw - if ":" in host: - host = host.rsplit(":", 1)[0] - return host.lower() + return normalize_host_key(raw) async def _run_validator(self, tenant_id: str) -> bool: """Invoke the configured validator; return True when valid.""" diff --git a/src/adcp/server/tenant_router.py b/src/adcp/server/tenant_router.py index a01d8c78..e3359442 100644 --- a/src/adcp/server/tenant_router.py +++ b/src/adcp/server/tenant_router.py @@ -90,11 +90,13 @@ def build_context(meta): import contextvars import inspect +import ipaddress import time from collections import OrderedDict from collections.abc import Awaitable, Callable from dataclasses import dataclass, field from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable +from urllib.parse import urlsplit if TYPE_CHECKING: from collections.abc import Mapping @@ -141,9 +143,11 @@ class SubdomainTenantRouter(Protocol): async def resolve(self, host: str) -> Tenant | None: """Return the :class:`Tenant` for ``host`` or ``None`` to 404. - ``host`` is the raw ``Host`` header value (lower-cased by - the middleware before this call). Implementations strip any - ``:port`` suffix as needed; the middleware doesn't. + ``host`` is the raw ``Host`` header value; the middleware does + not normalize it. The bundled implementations run it through + :func:`normalize_host_key`, and custom implementations should + do the same rather than hand-rolling a port strip — see that + function for the cases a naive split gets wrong. """ ... @@ -151,16 +155,20 @@ async def resolve(self, host: str) -> Tenant | None: class InMemorySubdomainTenantRouter: """Reference :class:`SubdomainTenantRouter` for dev / test. - Backed by a static ``host → Tenant`` dict. Lookup is exact - match on the lower-cased host (with the port suffix stripped). - Production adopters swap to a SQL-backed impl that hits their - tenant table. + Backed by a static ``host → Tenant`` dict. Lookup is an exact match + on the :func:`normalize_host_key` form of the host. Production + adopters swap to a SQL-backed impl that hits their tenant table. + + Note that IPv6 keys are stored de-bracketed and compressed, so + ``{"[::1]": ...}`` is registered under ``::1``. """ def __init__(self, tenants: Mapping[str, Tenant]) -> None: - # Normalize keys to lower-cased + port-stripped at construction - # so resolve() can be a single dict lookup. Adopters who pass - # mixed case (``Acme.Example.com``) get the obvious behavior. + # Normalize keys at construction so resolve() is a single dict + # lookup. Adopters who pass mixed case (``Acme.Example.com``) or + # a bracketed IPv6 literal get the obvious behavior. The helper + # is idempotent, so normalizing keys here and hosts in resolve() + # cannot disagree. self._tenants: dict[str, Tenant] = { _normalize_host(host): tenant for host, tenant in tenants.items() } @@ -171,9 +179,9 @@ async def resolve(self, host: str) -> Tenant | None: # Type alias for adopter-supplied lookup callables. Either sync (returns # Tenant | None) or async (returns Awaitable[Tenant | None]) is accepted — -# CallableSubdomainTenantRouter awaits at call time. Receives the -# already-normalized (lower-cased + port-stripped) host so adopters don't -# reimplement the parser. +# CallableSubdomainTenantRouter awaits at call time. Receives the host +# already run through normalize_host_key() so adopters don't reimplement +# the parser. TenantResolver = Callable[[str], "Tenant | None | Awaitable[Tenant | None]"] @@ -182,9 +190,11 @@ class CallableSubdomainTenantRouter: The adopter passes a single callable mapping a normalized host to a :class:`Tenant` (or ``None`` for 404). The framework owns host - normalization (lower-case + port-strip), so adopters write only the - lookup itself — typically a single SQL query against their tenant - table. + normalization (see :func:`normalize_host_key`), so adopters write + only the lookup itself — typically a single SQL query against their + tenant table. Adopter lookup tables must be keyed in that same form: + notably, IPv6 hosts arrive de-bracketed and compressed (``::1``, not + ``[::1]``). The callable may be sync or async; the router awaits at call time. @@ -256,9 +266,10 @@ def __init__( """Construct the router. :param resolver: Callable taking a normalized host string and - returning ``Tenant | None`` (sync or async). Receives - already-normalized hosts — lower-cased with any - ``:port`` suffix stripped. + returning ``Tenant | None`` (sync or async). Receives hosts + already run through :func:`normalize_host_key` — lower-cased + and IDNA-folded, with userinfo, the ``:port`` suffix, IPv6 + brackets and any trailing root dot removed. :param cache_size: Maximum number of cached lookups. ``0`` disables caching entirely (the adopter callable is awaited on every request). Must be ``>= 0``. @@ -442,18 +453,103 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: # ----- helpers ----------------------------------------------------------- -def _normalize_host(host: str) -> str: - """Lower-case and strip ``:port`` suffix. - - The ``Host`` header is case-insensitive per RFC 7230, but a - case-sensitive dict lookup would miss legitimate variations. - Also strips the port suffix so ``acme.example.com:443`` resolves - the same as ``acme.example.com``. +def normalize_host_key(value: str) -> str: + """Return the canonical tenant-lookup key for a host or URL. + + This is the single normalizer shared by every host-keyed lookup in + the SDK (:class:`InMemorySubdomainTenantRouter`, + :class:`CallableSubdomainTenantRouter`, + :class:`~adcp.server.tenant_registry.TenantRegistry`, and the + reference-seller example). Keeping one implementation is what makes + a registration key and a request-time ``Host`` header agree. + + Accepts full URLs (``https://acme.example.com:8443/agent``) and raw + ``Host`` header values (``acme.example.com``, ``[::1]:8080``), and: + + * discards any ``user:pw@`` userinfo, + * strips the ``:port`` suffix, + * removes IPv6 brackets and compresses the address + (``[2001:DB8::0:1]:443`` → ``2001:db8::1``), + * folds a single trailing FQDN-root dot, + * lower-cases and applies IDNA-2008 folding, so a tenant registered + under either the U-label or the A-label is reachable by both. + + **Never raises.** The ``Host`` header is attacker-controlled and is + normalized before any tenant exists to reject the request, so a + raise here would turn a 404 into a 500. Input this function cannot + parse yields a best-effort key that simply fails to match, and the + caller 404s as it would for any unknown host. """ - normalized = host.strip().lower() - if ":" in normalized: - normalized = normalized.split(":", 1)[0] - return normalized + raw = value.strip() + + # Bare/bracketed IP-literal short-circuit. Without it, urlsplit reads + # an unbracketed "2001:db8::1" as host:port and yields '2001', which + # would also make this function non-idempotent over its own output — + # load-bearing because InMemorySubdomainTenantRouter normalizes + # registration keys and then normalizes the lookup host again. + candidate = raw[1:-1] if raw.startswith("[") and raw.endswith("]") else raw + try: + return str(ipaddress.ip_address(candidate)) + except ValueError: + pass + + try: + parts = urlsplit(raw if "://" in raw else "//" + raw) + # .hostname de-brackets IPv6, drops userinfo and port, lower-cases. + host = parts.hostname + except ValueError: + host = None + if not host: + host = raw.lower() # unparseable authority -> best-effort key + + # Re-run the IP-literal test on the EXTRACTED host, not just the raw input. + # The short-circuit above only sees `[2001:DB8::0:1]`; once a port is + # attached, `urlsplit` is what strips the brackets, and the address landed + # here uncompressed. That made the function non-idempotent over its own + # output -- `[2001:DB8::0:1]:443` keyed to `2001:db8::0:1` while the bare + # form keyed to `2001:db8::1` -- so a tenant registered under one was + # unreachable from the other. Idempotency is load-bearing here: + # InMemorySubdomainTenantRouter normalizes registration keys and then + # normalizes the lookup host again. + try: + return str(ipaddress.ip_address(host)) + except ValueError: + pass + + if host.endswith("."): + host = host[:-1] # single FQDN-root dot, matching canonicalize_host + + if host.isascii(): + # ASCII fast path, and it is not a micro-optimization. For all-ASCII + # input `canonicalize_host` either returns exactly this value or + # raises -- and every raise is caught below and falls back to exactly + # this value. So the answer is identical, while the slow path is + # skipped for the hosts every real deployment actually uses. + # + # What that buys: `canonicalize_host` lives in `adcp.signing`, whose + # package import pulls 30 modules (~0.2s locally, more on a cold CI + # runner). Reaching it at module level slowed EVERY `import + # adcp.server`; reaching it here on the ASCII path moved that cost + # into tenant-router construction, which is enough to blow the + # storyboard runner's 30s readiness budget on the one example that + # builds a router. Now it is only paid for a genuinely non-ASCII host. + return host + + # Deferred: only a non-ASCII host needs UTS-46, and only then is the + # `adcp.signing` import worth its cost. + from adcp.signing._idna_canonicalize import canonicalize_host + + try: + return canonicalize_host(host) + except (UnicodeError, ValueError): + # idna.IDNAError subclasses UnicodeError, so this covers every + # documented raise (underscore labels, over-long labels, ''). + return host + + +def _normalize_host(host: str) -> str: + """Deprecated alias for :func:`normalize_host_key`.""" + return normalize_host_key(host) def _extract_host_header(scope: Scope) -> str | None: diff --git a/tests/test_subdomain_tenant_router.py b/tests/test_subdomain_tenant_router.py index f2439f28..19bd9be0 100644 --- a/tests/test_subdomain_tenant_router.py +++ b/tests/test_subdomain_tenant_router.py @@ -35,6 +35,7 @@ Tenant, current_tenant, ) +from adcp.server.tenant_router import normalize_host_key # noqa: E402 # ----- handler that surfaces the resolved tenant ------------------------ @@ -110,6 +111,50 @@ def test_in_memory_router_strips_port_suffix() -> None: assert result.id == "acme" +def test_in_memory_router_resolves_ipv6_literal_host() -> None: + """A bracketed IPv6 Host header resolves to its own tenant only. + + The old first-colon split collapsed any host whose first colon + follows ``[`` to the key ``'['``, so an unrelated IPv6 literal + matched the loopback tenant instead of 404ing. + """ + router = InMemorySubdomainTenantRouter( + tenants={"[::1]": Tenant(id="loopback", display_name="Loopback")} + ) + result = asyncio.run(router.resolve("[::1]:8080")) + assert result is not None + assert result.id == "loopback" + + # Different address, no tenant registered for it -> must 404. + assert asyncio.run(router.resolve("[::2]")) is None + assert asyncio.run(router.resolve("[2001:db8::1]")) is None + + +def test_in_memory_router_distinct_ipv6_tenants_do_not_collide() -> None: + """Two IPv6 tenants must occupy two registration keys, not one. + + Registration keys are normalized at construction, so a normalizer + that truncates at the first colon merges every IPv6 tenant into a + single dict slot — last write wins and one tenant's host resolves + to the other tenant. + """ + router = InMemorySubdomainTenantRouter( + tenants={ + "[::1]": Tenant(id="loopback", display_name="Loopback"), + "[::2]": Tenant(id="other", display_name="Other"), + } + ) + assert len(router._tenants) == 2 + + loopback = asyncio.run(router.resolve("[::1]")) + assert loopback is not None + assert loopback.id == "loopback" + + other = asyncio.run(router.resolve("[::2]")) + assert other is not None + assert other.id == "other" + + def test_in_memory_router_satisfies_protocol() -> None: router = InMemorySubdomainTenantRouter(tenants={}) assert isinstance(router, SubdomainTenantRouter) @@ -496,6 +541,39 @@ async def noop_send(_message): assert sentinel == ["websocket", "lifespan"] +# ----- normalize_host_key --------------------------------------------- + + +def test_normalize_host_key_never_raises_on_hostile_input() -> None: + """The lookup-key helper must fail soft on any Host header value. + + The Host header is attacker-controlled and reaches this helper + before any tenant is resolved. A raise here would turn today's + 404 into a 500, so every hostile shape must return a string. + Deliberately no try/except: a raise fails the test with the + exception itself. + """ + hostile = [ + "under_score.example.com", + "a" * 100 + ".example.com", + "", + " ", + "[::1", + "]::1[", + "acme.example.com:abc", + "%00.example.com", + "http://", + "@", + ] + for value in hostile: + assert isinstance(normalize_host_key(value), str) + + +def test_normalize_host_key_folds_trailing_root_dot() -> None: + """``acme.example.com.`` and ``acme.example.com`` are one tenant.""" + assert normalize_host_key("acme.example.com.") == "acme.example.com" + + # ----- helpers -------------------------------------------------------- @@ -504,3 +582,43 @@ async def _async_pass_through(scope, receive, send) -> None: isolation. Never reached when the middleware short-circuits.""" await send({"type": "http.response.start", "status": 200, "headers": []}) await send({"type": "http.response.body", "body": b""}) + + +@pytest.mark.parametrize( + ("with_port", "bare"), + [ + ("[2001:DB8::0:1]:443", "2001:db8::0:1"), + ("[::1]:8080", "::1"), + ("[2001:db8:0:0:0:0:0:1]:9000", "2001:db8::1"), + ], + ids=["uncompressed-upper", "loopback", "fully-expanded"], +) +def test_non_canonical_ipv6_with_port_keys_the_same_as_the_bare_form( + with_port: str, bare: str +) -> None: + """A port must not change which tenant an IPv6 host resolves to. + + The bare-literal short-circuit only sees the raw input. Once a port is + attached, ``urlsplit`` is what removes the brackets, and the address + arrived downstream uncompressed -- so ``[2001:DB8::0:1]:443`` keyed to + ``2001:db8::0:1`` while the bare form keyed to ``2001:db8::1``. A tenant + registered under one was unreachable from the other, and the function was + not idempotent over its own output. + + Idempotency is load-bearing rather than tidy: ``InMemorySubdomainTenantRouter`` + normalizes registration keys at construction and normalizes the Host again + at lookup, so a non-idempotent key silently fails to match itself. + """ + key = normalize_host_key(with_port) + assert key == normalize_host_key(bare) + assert normalize_host_key(key) == key, "normalize_host_key must be idempotent" + + +def test_ipv6_tenant_is_reachable_with_and_without_a_port() -> None: + """The end-to-end consequence: same tenant, either Host spelling.""" + router = InMemorySubdomainTenantRouter( + tenants={"[2001:DB8::0:1]": Tenant(id="v6", display_name="IPv6 Tenant")} + ) + for host in ("[2001:DB8::0:1]", "[2001:db8::1]:443", "2001:db8::0:1"): + resolved = asyncio.run(router.resolve(host)) + assert resolved is not None and resolved.id == "v6", f"unreachable via {host!r}" diff --git a/tests/test_tenant_registry.py b/tests/test_tenant_registry.py index cf5194e0..401ca8a3 100644 --- a/tests/test_tenant_registry.py +++ b/tests/test_tenant_registry.py @@ -512,6 +512,47 @@ async def test_resolve_returns_none_for_unknown_host() -> None: assert await registry.resolve("unknown.example.com") is None +@pytest.mark.asyncio +async def test_register_with_userinfo_in_agent_url_keys_on_hostname_only() -> None: + """An ``agent_url`` carrying userinfo keys on the hostname. + + ``urlparse(...).netloc`` retains ``user:pw@``, so splitting it on + the last colon produced the key ``'user'`` — the tenant was + registered under a garbage key and its real host 404'd. + """ + registry = TenantRegistry() + await registry.register( + "t1", + agent_url="https://user:pw@acme.example.com:8443/agent", + platform=_mock_platform(), + ) + + result = registry.resolve_by_host("acme.example.com") + assert result is not None + assert result.tenant_id == "t1" + assert registry.resolve_by_host("user") is None + + +@pytest.mark.asyncio +async def test_register_with_ipv6_agent_url_is_reachable_by_host_header() -> None: + """A bracketed IPv6 ``agent_url`` is reachable via its Host header. + + Registration went through ``urlparse``/``rsplit`` and lookup went + through the Host-header path, so the two sides keyed the same + address differently and never met. + """ + registry = TenantRegistry() + await registry.register( + "t6", + agent_url="https://[2001:db8::1]:8443/agent", + platform=_mock_platform(), + ) + + result = registry.resolve_by_host("[2001:db8::1]") + assert result is not None + assert result.tenant_id == "t6" + + @pytest.mark.asyncio async def test_register_lazy_await_first_validation_builds_immediately() -> None: platform = _mock_platform()