fix(signing): expert-review fixes for key_origins consistency check

Folds three expert review passes into the stage 4 PR (ad-tech-protocol-expert,
code-reviewer, security-reviewer all run in parallel on the original
commit 62d2379).

**1. Spec-mandated structured detail fields (MUST-FIX, protocol expert)**

ADCP #3690 security.mdx step 7 mandates that
``request_signature_key_origin_mismatch`` carry
``{purpose, expected_origin, actual_origin}`` and
``_key_origin_missing`` carry ``{purpose, posture}`` as structured
fields, not just an opaque message string. Middleware adapters surface
them on the 401 / in a DLQ; without this, the SDK can't be spec-
conformant once Stage 5 wires the verifier.

- Extend ``SignatureVerificationError`` with optional
  ``detail: Mapping[str, str] | None``. ``str(exc)`` still renders the
  free-form message for unstructured logs; structured callers read
  ``exc.detail``.
- ``check_key_origin_consistency`` now passes the spec-mandated keys.
- Two new tests pin the detail shape for both code paths.

**2. Bare-host vs URL canonicalization asymmetry (HIGH, security reviewer)**

The previous ``_origin_host`` fallback path for bare-host inputs only
guarded against ``/`` and space — accepting ``user@host``,
``host:port``, ``host?query``, ``host#fragment`` verbatim. An attacker
with capability-write access could declare a bare-host-with-port
(``keys.brand.com:8443``) to force a mismatch against the operator's
brand.json origin and DoS the honest verification path.

Fix: factor a ``_extract_host`` helper that re-parses bare-host inputs
through ``urlsplit`` with a synthetic ``https://`` scheme prepended.
URL form and bare-host form now strip port / userinfo / query /
fragment symmetrically. Two new tests cover ``host:port`` and
``user@host`` declaration shapes.

**3. Trailing-dot FQDN asymmetry (HIGH, security reviewer)**

``host.example.`` and ``host.example`` are the same FQDN — the trailing
dot denotes the root zone. Previous code preserved the dot, so a
brand.json serving the dot form against a capability declaring the
no-dot form (or vice versa) byte-mismatched. An attacker controlling
capabilities could weaponize this to deny verification against the
real counterparty.

Fix: strip a single trailing dot before IDNA-encoding. Test covers
both directions.

**4. IDN U-label vs A-label equivalence test (NICE, all three)**

The docstring promised IDN canonicalization but no test exercised it.
Added a test using ``münchen.example`` ↔ ``xn--mnchen-3ya.example``
in both directions.

**5. Carve-out for publisher-pin source in function docstring (MEDIUM, security)**

Previously the publisher-pin skip was documented only in the module
docstring. A caller reading just the function doc would miss it. Added
a "Caller contract" paragraph at the top of the function docstring
flagging that callers MUST skip this call for publisher-pinned tuples.

**6. Symmetric fail-closed test on declared side (LOW, security)**

The existing ``test_consistency_raises_mismatch_on_invalid_jwks_uri``
covers the resolved side but not the declared side. Added a symmetric
test so a future refactor can't silently invert the fail direction on
one side.

**Deferred to follow-up issues** (not in scope for this PR):

- Plumbing the ``source`` discriminant ("brand.json walk" vs
  "publisher adagents.json pin") through ``BrandJsonJwksResolver`` so
  Stage 5 can enforce the carve-out automatically. Belongs in the
  Stage 5 verifier-integration PR.
- Migrating all four codebase callsites (jwks.py, ip_pinned_transport.py,
  revocation_fetcher.py, key_origins.py) from stdlib IDNA-2003 to the
  ``idna`` PyPI package's IDNA-2008 in one commit. Package-wide
  conformance pass, separate concern.

Tests: 20 in test_key_origins (up from 12). Full signing surface
(582 tests across tests/test_*.py + tests/conformance/signing/)
remains green. ruff + mypy clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: bokelley

src/adcp/signing/errors.py

@@ -7,20 +7,34 @@
 
 from __future__ import annotations
 
+from collections.abc import Mapping
+
 
 class SignatureVerificationError(Exception):
-    """Raised when a request signature fails any step of the verifier checklist."""
+    """Raised when a request signature fails any step of the verifier checklist.
+
+    ``detail`` carries the spec-mandated structured fields for codes that
+    require them — e.g. ``request_signature_key_origin_mismatch`` carries
+    ``{purpose, expected_origin, actual_origin}`` per ADCP #3690
+    security.mdx step 7, and ``request_signature_brand_json_url_missing``
+    carries ``{agent_url}`` per the same section's rejection-code table.
+    Middleware adapters surface these as structured fields on the 401
+    response or in a DLQ payload; ``str(exc)`` continues to render the
+    free-form message for unstructured logs.
+    """
 
     def __init__(
         self,
         code: str,
         *,
         step: int | str | None = None,
         message: str | None = None,
+        detail: Mapping[str, str] | None = None,
     ) -> None:
         super().__init__(message or code)
         self.code = code
         self.step = step
+        self.detail = dict(detail) if detail is not None else None
 
 
 REQUEST_SIGNATURE_REQUIRED = "request_signature_required"

src/adcp/signing/key_origins.py

@@ -74,6 +74,16 @@ def check_key_origin_consistency(
     """Verify that the resolved ``jwks_uri`` host matches the declared
     ``identity.key_origins.{purpose}``.
 
+    **Caller contract: skip this call for publisher-pinned JWKS sources.**
+    Per ADCP #3690 the consistency check is mandatory only when the JWKS
+    source for the (agent, purpose, role) tuple was the operator
+    brand.json. For tuples sourced from a publisher
+    ``adagents.json signing_keys`` pin, the JWKS origin is the
+    publisher's domain by design — invoking this check on a
+    publisher-pinned tuple would incorrectly reject a legitimate key.
+    Callers are responsible for that branching; the helper takes no
+    ``source`` parameter and will always raise on host disagreement.
+
     Parameters
     ----------
     jwks_uri:
@@ -91,7 +101,7 @@ def check_key_origin_consistency(
     posture:
         Optional context attached to ``key_origin_missing`` rejection
         for adopter diagnostics (e.g. ``"required"``, ``"supported"``).
-        Not consulted by the check itself.
+        Surfaced as ``detail['posture']`` and in the message.
     code_family:
         ``"request"`` (default) or ``"webhook"``. Picks the
         corresponding spec error code family.
@@ -100,19 +110,28 @@ def check_key_origin_consistency(
     ------
     SignatureVerificationError
         With ``code = *_key_origin_missing`` when ``purpose`` is absent
-        from ``key_origins``; ``code = *_key_origin_mismatch`` when the
-        purpose's declared origin differs from the resolved
-        ``jwks_uri`` host (after IDNA-A-label canonicalization).
+        from ``key_origins`` — ``detail`` carries ``{purpose, posture}``.
+
+        With ``code = *_key_origin_mismatch`` when the purpose's declared
+        origin differs from the resolved ``jwks_uri`` host (after IDNA
+        A-label canonicalization). ``detail`` carries
+        ``{purpose, expected_origin, actual_origin}`` per the spec's
+        rejection-code shape — middleware adapters surface these as
+        structured fields on the 401 / in a DLQ.
     """
     declared = (key_origins or {}).get(purpose)
     if declared is None:
+        missing_detail: dict[str, str] = {"purpose": purpose}
+        if posture:
+            missing_detail["posture"] = posture
         raise SignatureVerificationError(
             _MISSING_CODE[code_family],
             step=7,
             message=(
                 f"identity.key_origins.{purpose} declaration missing"
                 + (f" (posture={posture})" if posture else "")
             ),
+            detail=missing_detail,
         )
 
     actual_host = _origin_host(jwks_uri)
@@ -125,6 +144,15 @@ def check_key_origin_consistency(
                 f"identity.key_origins.{purpose} declares {declared_host!r} "
                 f"but resolved jwks_uri host is {actual_host!r}"
             ),
+            detail={
+                "purpose": purpose,
+                # Use the canonicalized values when available; fall back
+                # to the raw inputs for diagnostic accuracy when one
+                # side failed to canonicalize. Spec wording is
+                # ``expected_origin`` / ``actual_origin`` verbatim.
+                "expected_origin": declared_host if declared_host is not None else declared,
+                "actual_origin": actual_host if actual_host is not None else jwks_uri,
+            },
         )
 
 
@@ -142,24 +170,64 @@ def _origin_host(value: str) -> str | None:
     keeps the canonicalization story coherent. A future IDNA-2008
     migration would update all four callsites together.
 
-    Returns ``None`` when the input is structurally invalid (no scheme
-    or no host); callers treat ``None`` as a binding failure.
+    **Bare-host and URL forms are normalized symmetrically.** A bare
+    host like ``"keys.brand.com"`` is processed through the same
+    ``urlsplit`` path as a full URL (with a synthetic scheme prepended)
+    so port, userinfo, query, and fragment all strip consistently.
+    Without that synthesis, a declarant supplying
+    ``"keys.brand.com:8443"`` as a bare host would canonicalize to
+    ``"keys.brand.com:8443"`` while the matching URL form would
+    canonicalize to ``"keys.brand.com"`` — a fail-closed asymmetry an
+    attacker who controls capabilities could exploit to deny
+    verification against the operator's brand.json origin.
+
+    **Trailing-dot equality.** ``host.example.`` and ``host.example``
+    are the same FQDN at the protocol layer (the dot denotes the root
+    zone). A counterparty serving the dot form while the capability
+    declares the no-dot form (or vice versa) must not mismatch. We
+    strip a single trailing dot before IDNA encoding.
+
+    Returns ``None`` when the input is structurally invalid (no
+    resolvable host, or it parses but contains characters that don't
+    survive IDNA); callers treat ``None`` as a binding failure.
     """
-    parts = urlsplit(value)
-    host = parts.hostname
+    host = _extract_host(value)
+    if host is None:
+        return None
+    host = host.rstrip(".").lower()
     if not host:
-        # Permit bare-host inputs like ``"keys.brand.com"`` —
-        # capabilities ``identity.key_origins`` values are not
-        # spec-constrained to be full URLs, only to identify an origin.
-        host = value.strip().lower()
-        if not host or "/" in host or " " in host:
-            return None
+        return None
     try:
         return host.encode("idna").decode("ascii").lower()
     except (UnicodeError, UnicodeEncodeError):
         return None
 
 
+def _extract_host(value: str) -> str | None:
+    """Pull the host portion out of ``value``, accepting both URL form
+    (``https://keys.brand.com/...``) and bare-host form
+    (``keys.brand.com``).
+
+    For URL inputs the host comes from ``urlsplit().hostname``. For
+    bare-host inputs we prepend a synthetic ``https://`` scheme and
+    re-parse so port / userinfo / query / fragment all strip the same
+    way they would for an explicit URL — closing the bare-host vs URL
+    asymmetry that the bare-host fallback used to have.
+    """
+    parts = urlsplit(value)
+    if parts.hostname:
+        return parts.hostname
+
+    # Schemeless input. Prepend ``https://`` and re-parse.
+    # Strip whitespace first so leading-space inputs don't produce
+    # ``https:// foo.com`` which then fails to parse a host.
+    stripped = value.strip()
+    if not stripped:
+        return None
+    parts = urlsplit(f"https://{stripped}")
+    return parts.hostname or None
+
+
 __all__ = [
     "CodeFamily",
     "check_key_origin_consistency",

tests/test_key_origins.py

@@ -162,3 +162,136 @@ def test_consistency_webhook_family_missing_uses_webhook_code() -> None:
             code_family="webhook",
         )
     assert exc_info.value.code == WEBHOOK_SIGNATURE_KEY_ORIGIN_MISSING
+
+
+# ----- spec-mandated structured detail fields -----
+
+
+def test_consistency_mismatch_carries_structured_detail() -> None:
+    # Spec #3690 step 7: ``request_signature_key_origin_mismatch`` MUST
+    # carry ``{purpose, expected_origin, actual_origin}`` as structured
+    # fields, not just an opaque message string. Middleware adapters
+    # surface them on the 401 / in a DLQ.
+    with pytest.raises(SignatureVerificationError) as exc_info:
+        check_key_origin_consistency(
+            jwks_uri="https://attacker.example.org/.well-known/jwks.json",
+            key_origins={"request_signing": "https://keys.brand.com"},
+            purpose="request_signing",
+        )
+    detail = exc_info.value.detail
+    assert detail is not None
+    assert detail["purpose"] == "request_signing"
+    assert detail["expected_origin"] == "keys.brand.com"
+    assert detail["actual_origin"] == "attacker.example.org"
+
+
+def test_consistency_missing_carries_structured_detail() -> None:
+    # Spec #3690 step 7: ``_key_origin_missing`` MUST carry
+    # ``{purpose, posture}``.
+    with pytest.raises(SignatureVerificationError) as exc_info:
+        check_key_origin_consistency(
+            jwks_uri="https://keys.brand.com/.well-known/jwks.json",
+            key_origins={},
+            purpose="request_signing",
+            posture="required",
+        )
+    detail = exc_info.value.detail
+    assert detail is not None
+    assert detail["purpose"] == "request_signing"
+    assert detail["posture"] == "required"
+
+
+def test_consistency_missing_detail_omits_posture_when_unsupplied() -> None:
+    # ``posture`` is optional; when the caller doesn't pass one, the
+    # detail dict carries only ``purpose``. Adapters reading
+    # ``detail.get("posture")`` see ``None`` rather than an empty string.
+    with pytest.raises(SignatureVerificationError) as exc_info:
+        check_key_origin_consistency(
+            jwks_uri="https://keys.brand.com/.well-known/jwks.json",
+            key_origins={},
+            purpose="request_signing",
+        )
+    detail = exc_info.value.detail
+    assert detail is not None
+    assert detail == {"purpose": "request_signing"}
+
+
+# ----- canonicalization edge cases (regressions for reviewer findings) -----
+
+
+def test_consistency_trailing_fqdn_dot_compares_equal_either_side() -> None:
+    # ``host.example.`` and ``host.example`` are the same FQDN at the
+    # protocol layer (the dot denotes the root zone). An attacker who
+    # controls capabilities could otherwise declare the dot form while
+    # the brand.json serves the no-dot form (or vice versa) and weaponize
+    # the check to deny verification against the legitimate counterparty.
+    # Both directions must compare equal.
+    check_key_origin_consistency(
+        jwks_uri="https://keys.brand.com./jwks.json",  # trailing dot
+        key_origins={"request_signing": "https://keys.brand.com"},
+        purpose="request_signing",
+    )
+    check_key_origin_consistency(
+        jwks_uri="https://keys.brand.com/jwks.json",
+        key_origins={"request_signing": "https://keys.brand.com."},  # trailing dot
+        purpose="request_signing",
+    )
+
+
+def test_consistency_bare_host_with_port_normalizes_symmetrically() -> None:
+    # Capability declaring ``keys.brand.com:8443`` (bare host with port)
+    # must normalize the same way the URL form would — stripping the
+    # port — so it compares equal to a resolved jwks_uri of
+    # ``https://keys.brand.com/...``. Without symmetric normalization,
+    # an attacker with capability-write access could supply a
+    # bare-host-with-port declaration to force a mismatch against the
+    # operator's brand.json origin.
+    check_key_origin_consistency(
+        jwks_uri="https://keys.brand.com/.well-known/jwks.json",
+        key_origins={"request_signing": "keys.brand.com:8443"},
+        purpose="request_signing",
+    )
+
+
+def test_consistency_bare_host_with_userinfo_rejects_or_normalizes() -> None:
+    # Declarations with userinfo (``user@host``) are spec-suspicious;
+    # the helper must NOT accidentally accept the host portion while
+    # ignoring the user. Symmetric urlsplit-based normalization strips
+    # userinfo the same way it does for URL inputs, so the comparison
+    # collapses to ``host == host``.
+    check_key_origin_consistency(
+        jwks_uri="https://keys.brand.com/.well-known/jwks.json",
+        key_origins={"request_signing": "user@keys.brand.com"},
+        purpose="request_signing",
+    )
+
+
+def test_consistency_idn_a_label_equals_u_label() -> None:
+    # IDN U-label (``münchen.example``) and A-label (Punycode
+    # ``xn--mnchen-3ya.example``) refer to the same host. Canonicalization
+    # to A-label via ``host.encode("idna")`` must make them compare equal
+    # regardless of which form each side uses.
+    check_key_origin_consistency(
+        jwks_uri="https://xn--mnchen-3ya.example/.well-known/jwks.json",
+        key_origins={"request_signing": "münchen.example"},
+        purpose="request_signing",
+    )
+    check_key_origin_consistency(
+        jwks_uri="https://münchen.example/.well-known/jwks.json",
+        key_origins={"request_signing": "xn--mnchen-3ya.example"},
+        purpose="request_signing",
+    )
+
+
+def test_consistency_unparseable_declared_origin_fails_closed() -> None:
+    # Symmetric to ``test_consistency_raises_mismatch_on_invalid_jwks_uri``
+    # but with the unparseable string on the *declared* side. A future
+    # refactor must not silently invert the fail direction — both sides
+    # must fail closed.
+    with pytest.raises(SignatureVerificationError) as exc_info:
+        check_key_origin_consistency(
+            jwks_uri="https://keys.brand.com/.well-known/jwks.json",
+            key_origins={"request_signing": "not a host"},
+            purpose="request_signing",
+        )
+    assert exc_info.value.code == REQUEST_SIGNATURE_KEY_ORIGIN_MISMATCH