From 977322dc01c1b136fe79d8485ca933034150b11f Mon Sep 17 00:00:00 2001 From: "Constantine.mirin" Date: Wed, 29 Jul 2026 12:49:38 +0200 Subject: [PATCH] fix(signing): idna-normalize hosts in etld+1 binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tldextract is IDNA-agnostic: given a U-label it returns a U-label. Because host_from only case-folded, registrable_domain emitted 'straße.de' for the U-label spelling and 'xn--strae-oqa.de' for the A-label spelling of the same host, so same_registrable_domain compared two strings that can never be equal and returned False. That predicate is step 2a of the brand-authorization binding (brand_authz.py:255) and the redirect-containment gate (adagents.py:404). A brand whose brand.json and agent URL disagreed on spelling had its legitimate agent refused with reason="binding_failed"; the authorized_operators[] fallback compared the same mis-normalized values and missed too. Nothing was wrongly accepted, but a correct agent could not bind. Both branches of host_from now delegate to _idna_canonicalize.canonicalize_host — the same normalizer jwks, revocation_fetcher and key_origins already use. etld was the last signing module still on a bare .lower() after PR #777. That also fixes a second asymmetry the old docstring got wrong: the URL branch trimmed no trailing root dot while the bare-host branch trimmed all of them, so 'https://Example.COM./' and 'Example.COM.' normalized differently. registrable_domain maps idna.IDNAError onto None rather than letting it propagate or falling back to the raw string. Fail-closed is deliberate: - Propagating is not viable. IDNAError is a ValueError subclass, so it would be silently reclassified as brand_domain_invalid at one callsite and escape uncaught at five others mid-verification. - Failing open would loosen an existing check. adagents._idna_ascii_host is today the only gate rejecting an IDNA-invalid redirect target (_check_safe_host does not reject underscores). With a raw-string fallback, 'under_score.brand.com' would reduce to 'brand.com', match the origin, and the redirect would be accepted. - It widens a category the module already documents rather than inventing a contract: a string that is not an encodable hostname has no derivable eTLD+1, alongside IP literals and single-label hosts. The cost, stated plainly: hosts with an underscore label, a label over 63 bytes, or a leading/trailing-hyphen label now yield None instead of an eTLD+1. These are not valid hostnames per RFC 952/1123 and cannot appear in a fetchable https:// agent URL, but this is the one place an agent that binds today stops binding. Adopter-visible: registrable_domain and host_from are public exports and now return A-labels for IDN input; host_from additionally raises on IDNA-invalid input where it previously returned the string unchanged. Both sides of same_registrable_domain move together, so the predicate stays correct. adagents._idna_ascii_host is left in place — canonicalize_host is idempotent on the A-label output it produces, so it becomes a harmless double normalization. Removing that duplicate belongs in a follow-up that touches adagents. Fixes #988 --- src/adcp/signing/etld.py | 75 +++++++++++++---- tests/conformance/signing/test_etld.py | 112 +++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 18 deletions(-) create mode 100644 tests/conformance/signing/test_etld.py diff --git a/src/adcp/signing/etld.py b/src/adcp/signing/etld.py index 18228edc..5c457b2c 100644 --- a/src/adcp/signing/etld.py +++ b/src/adcp/signing/etld.py @@ -20,9 +20,19 @@ **Failure-closed convention.** Inputs whose eTLD+1 cannot be derived (raw IP addresses, single-label hosts like ``localhost``, hosts that are -themselves public suffixes) yield ``None`` from :func:`registrable_domain` -and ``False`` from :func:`same_registrable_domain`. Callers must treat -None / False as a binding failure, not a soft skip. +themselves public suffixes, hosts that are not IDNA-encodable — underscore +labels, labels over 63 bytes, leading/trailing-hyphen labels) yield ``None`` +from :func:`registrable_domain` and ``False`` from +:func:`same_registrable_domain`. Callers must treat None / False as a +binding failure, not a soft skip. + +**Hosts are compared in canonical A-label form.** ``tldextract`` is +IDNA-agnostic: hand it a U-label and it hands back a U-label. Comparing +unencoded hosts made ``straße.de`` and ``xn--strae-oqa.de`` — one host, +two spellings — never compare equal, refusing a legitimate agent whose +brand.json and agent URL disagreed on spelling. :func:`host_from` +therefore delegates to :func:`adcp.signing._idna_canonicalize.canonicalize_host`, +the same normalizer the JWKS, revocation, and key-origin checks use. """ from __future__ import annotations @@ -30,8 +40,11 @@ from functools import lru_cache from urllib.parse import urlsplit +import idna import tldextract +from ._idna_canonicalize import canonicalize_host + @lru_cache(maxsize=1) def _extractor() -> tldextract.TLDExtract: @@ -58,15 +71,27 @@ def _extractor() -> tldextract.TLDExtract: def host_from(value: str) -> str: - """Return the hostname portion of a URL, or pass a bare host through. - - Normalizes case and trims a single trailing dot (the FQDN root - separator) so ``Example.COM.`` and ``example.com`` compare equal. + """Return the canonical hostname of a URL, or of a bare host. + + Both branches delegate to + :func:`adcp.signing._idna_canonicalize.canonicalize_host`, which + strips a single trailing FQDN-root dot, ASCII-lowercases, + short-circuits IPv4/IPv6 literals (IDNA-2008 rejects purely-numeric + labels), and otherwise UTS-46 encodes with + ``idna.encode(uts46=True, transitional=False)``. So ``Example.COM.`` + and ``example.com`` compare equal, and so do ``straße.de`` and + ``xn--strae-oqa.de``. Routing *both* branches through it is what + makes the URL form and the bare-host form of one host normalize + identically — previously only the bare-host branch trimmed the root + dot, and it trimmed every trailing dot rather than one. Raises :class:`ValueError` on input that is a URL with no parseable - host (``"http://"``) or empty after normalization. URL inputs MUST - use a scheme — a bare ``//example.com`` is treated as a bare host, - which is by design: bare-host inputs to this helper come from + host (``"http://"``), empty after normalization, or not encodable as + a hostname. The last case surfaces as ``idna.IDNAError``, which is a + :class:`ValueError` subclass (via ``UnicodeError``), so the raise + contract is unchanged for callers catching ``ValueError``. URL inputs + MUST use a scheme — a bare ``//example.com`` is treated as a bare + host, which is by design: bare-host inputs to this helper come from ``brand_url`` fields whose schema already constrains them, so a bare-host input is never an attacker-controlled URL. """ @@ -75,11 +100,13 @@ def host_from(value: str) -> str: host = parts.hostname if not host: raise ValueError(f"URL has no host: {value!r}") - return host.lower() - stripped = value.strip().rstrip(".").lower() - if not stripped: - raise ValueError("host is empty") - return stripped + else: + host = value.strip() + # Preserve the pre-existing message for ``""`` / ``"."`` / ``".."``; + # without this guard ``idna`` would raise "Empty domain" instead. + if not host.strip("."): + raise ValueError("host is empty") + return canonicalize_host(host) def registrable_domain(host_or_url: str) -> str | None: @@ -92,14 +119,26 @@ def registrable_domain(host_or_url: str) -> str | None: * IP literals (v4 and v6) — IP addresses are not eTLD+1-bindable. * Single-label hosts (``localhost``, ``intranet``). * Hosts that are themselves a public suffix (``co.uk``). - - The returned domain is lowercased. + * Hosts that are not IDNA-encodable (``under_score.brand.com``, a + label over 63 bytes, ``-lead.brand.com``). Such a string is not a + hostname, so it has no eTLD+1 to derive. Failing open here would + let ``under_score.brand.com`` reduce to ``brand.com`` and satisfy + the binding on a name the encoder rejects. + + The returned domain is lowercased and in canonical A-label form: + ``straße.de`` returns ``"xn--strae-oqa.de"``. This is a change in + public return values for IDN inputs; both sides of + :func:`same_registrable_domain` move together, so the predicate + stays correct. Callers performing a binding check should treat ``None`` as a failure (the agent's host has no registrable domain to bind against), NOT as "no opinion". """ - host = host_from(host_or_url) + try: + host = host_from(host_or_url) + except (idna.IDNAError, UnicodeError): + return None result = _extractor()(host) if not result.domain or not result.suffix: return None diff --git a/tests/conformance/signing/test_etld.py b/tests/conformance/signing/test_etld.py new file mode 100644 index 00000000..08f9408c --- /dev/null +++ b/tests/conformance/signing/test_etld.py @@ -0,0 +1,112 @@ +"""IDNA conformance tests for :mod:`adcp.signing.etld`. + +Complements the behavioral suite in ``tests/test_etld.py`` (eTLD+1 +derivation, PSL private section, failure-closed IP/single-label cases) +with the host-canonicalization contract the brand-authorization binding +depends on. + +Behavior under test: + +* A host spelled as a U-label (``straße.de``) and the same host spelled + as an A-label (``xn--strae-oqa.de``) are ONE host and must bind. +* :func:`registrable_domain` returns canonical A-label form. +* The single-trailing-dot rule is symmetric across the URL branch and + the bare-host branch of :func:`host_from`. +* Failure-closed convention extends to hosts that are not IDNA-encodable + (underscore labels, labels over 63 bytes, leading-hyphen labels). +""" + +from __future__ import annotations + +import pytest + +from adcp.signing.etld import host_from, registrable_domain, same_registrable_domain + +# ----- host_from ----- + + +def test_host_from_trailing_dot_symmetric_across_url_and_bare_forms() -> None: + # The documented single-trailing-dot rule applies to BOTH branches. + # The URL branch previously trimmed nothing and the bare-host branch + # trimmed every trailing dot, so the two spellings of one host + # normalized differently. + assert host_from("https://Example.COM./") == "example.com" + assert host_from("Example.COM.") == "example.com" + assert host_from("https://x.example.com..") == host_from("x.example.com..") + + +def test_host_from_returns_a_label_for_idn() -> None: + # host_from is the single normalization point for the binding, so it + # owns UTS-46 / IDNA-2008 encoding, not just case folding. + assert host_from("https://shop.straße.de/") == "shop.xn--strae-oqa.de" + assert host_from("STRASSE.straße.DE") == "strasse.xn--strae-oqa.de" + assert host_from("xn--strae-oqa.de") == "xn--strae-oqa.de" + + +def test_host_from_ip_literals_pass_through() -> None: + # IDNA-2008 rejects purely-numeric labels; IP literals must survive + # normalization unchanged so the downstream eTLD+1 lookup can fail + # them closed for the right reason. + assert host_from("192.0.2.1") == "192.0.2.1" + assert host_from("https://[2001:0db8::0001]/") == "2001:db8::1" + + +def test_host_from_idna_invalid_host_raises_value_error() -> None: + # ``idna.IDNAError`` subclasses ``UnicodeError`` subclasses + # ``ValueError``, so the documented "raises ValueError" contract is + # unchanged and brand_authz's ``except ValueError`` around host_from + # still catches it. + with pytest.raises(ValueError): + host_from("under_score.brand.com") + + +def test_host_from_empty_and_dot_only_still_raise() -> None: + # The pre-existing emptiness contract survives the delegation. + for value in ("", ".", "..", " "): + with pytest.raises(ValueError): + host_from(value) + + +# ----- registrable_domain ----- + + +def test_registrable_domain_returns_a_label_for_idn() -> None: + assert registrable_domain("straße.de") == "xn--strae-oqa.de" + assert registrable_domain("ADS.Straße.DE") == "xn--strae-oqa.de" + assert registrable_domain("xn--strae-oqa.de") == "xn--strae-oqa.de" + + +def test_registrable_domain_idna_invalid_host_fails_closed() -> None: + # Not encodable as a hostname -> no derivable eTLD+1 -> None, the + # same failure-closed category as IP literals and single-label hosts. + # Failing open here would let ``under_score.brand.com`` reduce to + # ``brand.com`` and satisfy the binding on a string IDNA rejects. + assert registrable_domain("under_score.brand.com") is None + assert registrable_domain("a" * 64 + ".brand.com") is None + assert registrable_domain("-lead.brand.com") is None + assert same_registrable_domain("under_score.brand.com", "brand.com") is False + + +def test_registrable_domain_ip_literals_still_none() -> None: + # Guard for the IP short-circuit inside the canonicalizer: routing + # host_from through IDNA must not turn these into raises. + assert registrable_domain("192.0.2.1") is None + assert registrable_domain("https://[2001:db8::1]/") is None + + +# ----- same_registrable_domain ----- + + +def test_same_registrable_domain_idna_u_label_binds_to_a_label() -> None: + # A brand publishing brand.json under the U-label host and listing + # its agent under the A-label form (or the reverse) is one host and + # MUST bind. Uses a real delegated TLD (.de) — ``.example`` is RFC + # 2606 reserved, not in the PSL, and returns None for both spellings + # regardless, which would make this assertion vacuous. + assert same_registrable_domain("https://shop.straße.de/", "xn--strae-oqa.de") is True + assert same_registrable_domain("xn--strae-oqa.de", "https://shop.straße.de/") is True + + +def test_same_registrable_domain_idn_cross_domain_still_false() -> None: + # Canonicalizing both sides must not collapse distinct IDN domains. + assert same_registrable_domain("straße.de", "xn--bcher-kva.de") is False