diff --git a/src/adcp/decisioning/context.py b/src/adcp/decisioning/context.py index eaadcc82..71e5a2e8 100644 --- a/src/adcp/decisioning/context.py +++ b/src/adcp/decisioning/context.py @@ -210,6 +210,12 @@ def from_verified_signer( headers=dict(request.headers), body=request.get_data(), options=VerifyOptions(...), + # Pass the as-received header list when your framework has one + # (ASGI: ``request.headers.raw``). ``dict(request.headers)`` + # resolves a repeated header name to a single value, so the + # step-1 rejection of a proxy-inserted second line cannot fire + # without it. + raw_headers=getattr(request.headers, "raw", None), ) ctx.metadata["adcp.auth_info"] = AuthInfo.from_verified_signer( signer, max_verified_age_s=300.0, diff --git a/src/adcp/signing/_header_precheck.py b/src/adcp/signing/_header_precheck.py new file mode 100644 index 00000000..687c5d70 --- /dev/null +++ b/src/adcp/signing/_header_precheck.py @@ -0,0 +1,186 @@ +"""RFC 9421 checklist step 1: strict structured-field rejection. + +The verifier's later stages are permissive by design -- they resolve ambiguity +so that well-formed-but-unusual input still verifies. That is wrong at step 1. +Input that is *ambiguous* must be refused before anything downstream picks an +interpretation, because "pick one" is exactly what an attacker inserting a +second header line is counting on. + +The rules are a predicate table rather than an if-chain: the same predicate can +be reused at another call site that raises a different code at a different step. +`host_has_raw_non_ascii` is shared with `canonical` for precisely that reason -- +canonicalization rejects a malformed authority as `request_target_uri_malformed` +at step 6, while a U-label arriving on the wire is `request_signature_header_malformed` +at step 1. Same rule, two codes, two steps. + +## What "raw headers" buys, and where it does not + +The single-value rules below are only as strong as the header view they run +over. Every dict view of headers resolves a repeated name to ONE value -- first +or last depending on the framework -- so a proxy-inserted second line vanishes +before any check runs. Passing `raw_headers` preserves the repetition and lets +rule `repeated-line` see it. + +This works on ASGI/Starlette, where `request.headers.raw` is the wire order. +It does NOT work on WSGI/Flask, and that is not fixable here: PEP 3333 folds +repeated `HTTP_*` headers into a comma-joined string and writes `CONTENT_TYPE` +and `CONTENT_LENGTH` to bare environ keys with last-wins and no join. The +repetition is destroyed one layer above this SDK. A Flask `raw_headers` list +therefore carries the same information as the mapping, and the `repeated-line` +rule is inert there -- the comma-joined rules below still apply. +""" + +from __future__ import annotations + +from collections.abc import Mapping, Sequence + +from adcp.signing.canonical import ( + TargetUriMalformedError, + _split_or_reject, + host_has_raw_non_ascii, + split_structured_field, +) + +#: Headers that must arrive exactly once. Each is either a singleton by RFC +#: (`Content-Type`, RFC 9110 §8.3; `Host`, §7.2) or is a signed component whose +#: value the signature is computed over -- where a second line would split the +#: signed view from the parsed view even when RFC 9110 §5.3 would permit +#: combining. Rejecting a legally-split `Signature-Input` or `Content-Digest` is +#: a deliberate profile choice: the ambiguity it removes is worth more than the +#: flexibility it costs, and no known producer splits them. +_SINGLE_LINE_SIGNED_HEADERS = frozenset( + {"signature", "signature-input", "content-type", "content-digest", "host"} +) + +#: Headers whose value must be a single entry, i.e. carry no top-level comma. +#: `Content-Type` is a singleton by RFC 9110 §8.3, so a comma means two values +#: were folded together regardless of whether the signature covers it. +_SINGLE_VALUE_HEADERS = frozenset({"content-type"}) + + +def strict_header_precheck( + *, + headers: Mapping[str, str], + raw_headers: Sequence[tuple[bytes, bytes]] | None, + url: str, +) -> str | None: + """Why this request's headers are malformed at step 1, or `None`. + + Returns a reason string rather than raising so the caller owns the error + type and step number -- the same table is reusable at a call site that + raises a different code. + """ + pairs = _header_pairs(headers=headers, raw_headers=raw_headers) + + if raw_headers is not None: + seen: set[str] = set() + for name, _ in pairs: + if name in _SINGLE_LINE_SIGNED_HEADERS and name in seen: + return ( + f"{name!r} arrived on more than one header line; a repeated " + "signed field is ambiguous and MUST be rejected rather than folded" + ) + seen.add(name) + + for name, value in pairs: + if name in _SINGLE_VALUE_HEADERS and len(split_structured_field(value, ",")) > 1: + return ( + f"{name!r} carries more than one value; it is a singleton field " + "and a comma-joined value means two were folded together" + ) + if name == "signature-input": + reason = _duplicate_dictionary_key_reason(value, "Signature-Input") + if reason is not None: + return reason + if name == "content-digest": + reason = _duplicate_dictionary_key_reason(value, "Content-Digest") + if reason is not None: + return reason + + return _non_ascii_authority_reason(pairs=pairs, url=url) + + +def _header_pairs( + *, + headers: Mapping[str, str], + raw_headers: Sequence[tuple[bytes, bytes]] | None, +) -> list[tuple[str, str]]: + """Lower-cased (name, value) pairs, preferring the raw list when supplied. + + The raw list is the only view that preserves a repeated header name; the + mapping has already resolved it. Undecodable bytes are not an error to + diagnose here -- they cannot match any rule, so they are skipped and left to + the framework. + """ + if raw_headers is None: + return [(k.lower(), v) for k, v in headers.items()] + pairs: list[tuple[str, str]] = [] + for raw_name, raw_value in raw_headers: + try: + pairs.append((raw_name.decode("latin-1").lower(), raw_value.decode("latin-1"))) + except (UnicodeDecodeError, AttributeError): + continue + return pairs + + +def _duplicate_dictionary_key_reason(value: str, header_name: str) -> str | None: + """RFC 8941 §3.2: a duplicate dictionary key MUST be rejected, not resolved. + + "Retaining only the last value" is the other option the RFC allows, and it + is the one a parser reaches for by default -- `d[key] = v` in a loop. That + is the smuggling vector: a proxy appends a second entry with the same label + and a weaker covered-component set, the parser keeps the last, and the + signature verifies over fewer components than the producer signed. + """ + keys: set[str] = set() + for entry in split_structured_field(value, ","): + entry = entry.strip() + if not entry: + continue + key = entry.split("=", 1)[0].strip() + if not key: + continue + if key in keys: + return ( + f"{header_name} carries duplicate dictionary key {key!r}; RFC 8941 §3.2 " + "requires rejecting rather than resolving, since resolving lets a " + "repeated key downgrade what was signed" + ) + keys.add(key) + return None + + +def _non_ascii_authority_reason(*, pairs: Sequence[tuple[str, str]], url: str) -> str | None: + """A host carrying raw non-ASCII bytes MUST be rejected, never re-normalized. + + Checked against BOTH the `Host` header and the URL's authority, because + neither alone is sufficient: ASGI frameworks drop a non-ASCII Host when + building `request.url` (Starlette's `URL` falls back to `scope["server"]` + when the Host header fails its host regex), so the U-label survives only on + the header -- while the conformance vectors carry no Host header at all and + express the case through the URL. + + Re-normalizing instead of rejecting would pick one of several legitimate + UTS-46 outcomes and risk disagreeing with whoever signed. Converting is the + producer's job. + """ + for name, value in pairs: + if name == "host" and host_has_raw_non_ascii(value): + return ( + "the Host header carries raw non-ASCII bytes; the producer must send an " + "A-label, and a comparer that re-normalized could disagree with the signer" + ) + try: + netloc = _split_or_reject(url).netloc + except TargetUriMalformedError: + # A malformed authority is canonicalization's rejection to make, with + # its own code at its own step. Not this gate's business -- but it must + # not escape as an uncaught ValueError either, which is why it is caught + # here and handed on rather than left to propagate from a bare urlsplit. + return None + if host_has_raw_non_ascii(netloc): + return ( + "the request URL's authority carries raw non-ASCII bytes; the producer must " + "send an A-label, and a comparer that re-normalized could disagree with the signer" + ) + return None diff --git a/src/adcp/signing/middleware.py b/src/adcp/signing/middleware.py index a0a8efec..52a7ed9b 100644 --- a/src/adcp/signing/middleware.py +++ b/src/adcp/signing/middleware.py @@ -22,6 +22,29 @@ def unauthorized_response_headers(exc: SignatureVerificationError) -> dict[str, return {"WWW-Authenticate": f'Signature error="{exc.code}"'} +def _wsgi_raw_headers(request: Any) -> list[tuple[bytes, bytes]] | None: + """Best-effort raw header list from a WSGI request. + + Deliberately weaker than the ASGI equivalent, and it cannot be otherwise: + PEP 3333 folds repeated `HTTP_*` headers into one comma-joined environ value + and writes `CONTENT_TYPE` / `CONTENT_LENGTH` to bare environ keys with + last-wins and no join. A second `Content-Type` line is therefore already + gone before any WSGI app runs. This list carries the same information as + `dict(request.headers)`; it exists so the comma-joined rules run over the + same shape on both frameworks, not because it recovers the repetition. + + Returns `None` rather than raising if the headers cannot be encoded — a + header outside latin-1 is not something to turn a 401 into a 500 over. + """ + try: + return [ + (name.encode("latin-1"), value.encode("latin-1")) + for name, value in request.headers.items() + ] + except (AttributeError, UnicodeEncodeError): + return None + + def verify_flask_request(request: Any, *, options: VerifyOptions) -> VerifiedSigner: """Verify a Flask `request` object against the AdCP profile.""" return verify_request_signature( @@ -30,6 +53,7 @@ def verify_flask_request(request: Any, *, options: VerifyOptions) -> VerifiedSig headers=dict(request.headers), body=request.get_data(), options=options, + raw_headers=_wsgi_raw_headers(request), ) @@ -64,6 +88,14 @@ async def verify_starlette_request(request: Any, *, options: VerifyOptions) -> V headers=dict(request.headers), body=body, options=options, + # The as-received list, wire order intact. This is the arm where the + # step-1 repeated-line rule actually bites: `dict(request.headers)` + # resolves a repeated name to one value, so a proxy-inserted second + # `Content-Type` is invisible without it. It also carries a raw + # non-ASCII Host, which `str(request.url)` drops -- Starlette's `URL` + # falls back to `scope["server"]` when the Host header fails its host + # regex, so the U-label survives only here. + raw_headers=getattr(request.headers, "raw", None), ) diff --git a/src/adcp/signing/verifier.py b/src/adcp/signing/verifier.py index 0256cef7..2acc6b5f 100644 --- a/src/adcp/signing/verifier.py +++ b/src/adcp/signing/verifier.py @@ -9,11 +9,12 @@ from __future__ import annotations import warnings -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from dataclasses import dataclass, field from datetime import datetime, timezone from typing import Any, Literal +from adcp.signing._header_precheck import strict_header_precheck from adcp.signing.canonical import ( TargetUriMalformedError, _lookup, @@ -175,10 +176,20 @@ def verify_request_signature( headers: Mapping[str, str], body: bytes, options: VerifyOptions, + raw_headers: Sequence[tuple[bytes, bytes]] | None = None, ) -> VerifiedSigner: """Run the AdCP request-signing verifier checklist against a request. Raises SignatureVerificationError with the spec error code on failure. + + `raw_headers` is the as-received header list, wire order preserved, e.g. + Starlette's `request.headers.raw`. Supply it when you can: `headers` is a + mapping, and every mapping has already resolved a repeated header name to a + single value, so a proxy-inserted second line is invisible to the step-1 + checks without it. Omitting it does not weaken any other stage -- the + comma-joined form of each malformed shape is still rejected -- but the + repeated-line rule cannot fire. See `_header_precheck` for why WSGI cannot + supply a meaningful raw list at all. """ sig_input_raw = _lookup(headers, "signature-input") sig_raw = _lookup(headers, "signature") @@ -191,6 +202,19 @@ def verify_request_signature( ) assert sig_input_raw is not None and sig_raw is not None + # Step 1, and it must precede the parse below rather than wrap it: the + # parser resolves ambiguity (RFC 8941 lets a duplicate dictionary key be + # dropped rather than rejected), and once resolved the ambiguity is + # invisible. Runs after the presence pre-check so a request with no + # signature at all still reports `request_signature_required`. + precheck_reason = strict_header_precheck(headers=headers, raw_headers=raw_headers, url=url) + if precheck_reason is not None: + raise SignatureVerificationError( + REQUEST_SIGNATURE_HEADER_MALFORMED, + step=1, + message=precheck_reason, + ) + try: labels = parse_signature_input_header(sig_input_raw) except (ValueError, KeyError) as exc: diff --git a/tests/conformance/signing/test_header_precheck.py b/tests/conformance/signing/test_header_precheck.py new file mode 100644 index 00000000..bff8d49d --- /dev/null +++ b/tests/conformance/signing/test_header_precheck.py @@ -0,0 +1,174 @@ +"""Step-1 strict rejection, graded in the form the vectors cannot express. + +`negative/021`, `022`, `023` and `026` each ship their malformed shape as a +single comma-joined header value, because a JSON vector has nowhere to put two +lines with the same name. But the threat their own `$comment`s describe is a +proxy inserting a **second header line** — and every mapping view of headers +resolves that to one value before any check runs, so a gate written over the +mapping passes all four vectors while missing the attack entirely. + +These tests drive the two-line form. Without them the conformance suite is +green and the threat is open. +""" + +from __future__ import annotations + +import pytest + +from adcp.signing import ( + InMemoryReplayStore, + SignatureVerificationError, + VerifierCapability, + VerifyOptions, + verify_request_signature, +) +from adcp.signing.errors import REQUEST_SIGNATURE_HEADER_MALFORMED + +_URL = "https://seller.example.com/adcp/create_media_buy" +_SIG_INPUT = ( + 'sig1=("@method" "@target-uri" "@authority" "content-type");created=1776520800;' + 'expires=1776521100;nonce="KXYnfEfJ0PBRZXQyVXfVQA";keyid="test-ed25519-2026";' + 'alg="ed25519";tag="adcp/request-signing/v1"' +) +_SIG = "sig1=:" + "A" * 86 + ":" + + +def _options() -> VerifyOptions: + return VerifyOptions( + now=1776520800.0, + capability=VerifierCapability(), + operation="create_media_buy", + jwks_resolver=lambda keyid: None, + replay_store=InMemoryReplayStore(), + ) + + +def _verify(raw: list[tuple[bytes, bytes]] | None, headers: dict[str, str]) -> None: + verify_request_signature( + method="POST", + url=_URL, + headers=headers, + body=b'{"plan_id":"plan_001"}', + options=_options(), + raw_headers=raw, + ) + + +def test_second_content_type_line_is_rejected_at_step_1() -> None: + """The attack the vectors describe but cannot express. + + A proxy appends a second `Content-Type`. `dict(headers)` keeps exactly one + of them — first or last depending on the framework — so the signed view and + the parsed view can disagree while every conformance vector still passes. + """ + raw = [ + (b"content-type", b"application/json"), + (b"content-type", b"text/plain"), + (b"signature-input", _SIG_INPUT.encode()), + (b"signature", _SIG.encode()), + ] + # The mapping a framework would hand us has already lost the second line. + collapsed = { + "Content-Type": "application/json", + "Signature-Input": _SIG_INPUT, + "Signature": _SIG, + } + + with pytest.raises(SignatureVerificationError) as exc_info: + _verify(raw, collapsed) + assert exc_info.value.code == REQUEST_SIGNATURE_HEADER_MALFORMED + assert exc_info.value.step == 1 + + +def test_second_signature_input_line_is_rejected_at_step_1() -> None: + """Two `Signature-Input` lines are the covered-component smuggling vector. + + Same shape as `negative/021`'s duplicate dictionary key, arriving by the + other route: a second line whose component list is shorter than the one the + producer signed. + """ + weaker = 'sig1=("@method" "@target-uri");created=1776520800;expires=1776521100;nonce="AAAAAAAAAAAAAAAAAAAAAA";keyid="test-ed25519-2026";alg="ed25519";tag="adcp/request-signing/v1"' + raw = [ + (b"content-type", b"application/json"), + (b"signature-input", _SIG_INPUT.encode()), + (b"signature-input", weaker.encode()), + (b"signature", _SIG.encode()), + ] + collapsed = { + "Content-Type": "application/json", + "Signature-Input": _SIG_INPUT, + "Signature": _SIG, + } + + with pytest.raises(SignatureVerificationError) as exc_info: + _verify(raw, collapsed) + assert exc_info.value.code == REQUEST_SIGNATURE_HEADER_MALFORMED + assert exc_info.value.step == 1 + + +def test_repeated_line_is_invisible_without_raw_headers() -> None: + """The documented limit of the mapping-only path, pinned so it stays honest. + + This is not a bug being enshrined -- it is the reason `raw_headers` exists. + If this ever starts raising, the mapping grew the ability to carry a + repeated name and the docstring on `verify_request_signature` is stale. + """ + collapsed = { + "Content-Type": "application/json", + "Signature-Input": _SIG_INPUT, + "Signature": _SIG, + } + with pytest.raises(SignatureVerificationError) as exc_info: + _verify(None, collapsed) + # Fails later, on the unknown key -- NOT at step 1 as malformed. + assert exc_info.value.code != REQUEST_SIGNATURE_HEADER_MALFORMED + + +def test_non_ascii_host_header_is_rejected_even_when_the_url_is_clean() -> None: + """The real-traffic shape of `negative/026`, which no vector can carry. + + ASGI frameworks drop a non-ASCII Host when building `request.url` + (Starlette's `URL` falls back to `scope["server"]` when the Host header + fails its host regex), so in production the U-label survives only on the + header. A gate that checked the URL alone would pass vector 026 -- which + ships no Host header -- and miss every real request. + """ + raw = [ + (b"host", "bücher.example.com".encode()), + (b"content-type", b"application/json"), + (b"signature-input", _SIG_INPUT.encode()), + (b"signature", _SIG.encode()), + ] + collapsed = { + "Host": "bücher.example.com", + "Content-Type": "application/json", + "Signature-Input": _SIG_INPUT, + "Signature": _SIG, + } + with pytest.raises(SignatureVerificationError) as exc_info: + _verify(raw, collapsed) + assert exc_info.value.code == REQUEST_SIGNATURE_HEADER_MALFORMED + assert exc_info.value.step == 1 + + +def test_multi_algorithm_content_digest_is_not_rejected() -> None: + """False-positive guard: distinct algorithms are legal, duplicates are not. + + RFC 9530 permits a `Content-Digest` carrying several algorithms. Only a + repeated *key* is the defect `negative/023` describes. A gate that rejected + every comma would refuse traffic the spec requires accepting -- worse than + the bug it closes. + """ + digest = ( + "sha-256=:X48E9qOokqqrvdts8nOJRJN3OWDUoyWxBf7kbu9DBPE=:, " + "sha-512=:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=:" + ) + headers = { + "Content-Type": "application/json", + "Content-Digest": digest, + "Signature-Input": _SIG_INPUT, + "Signature": _SIG, + } + with pytest.raises(SignatureVerificationError) as exc_info: + _verify(None, headers) + assert exc_info.value.code != REQUEST_SIGNATURE_HEADER_MALFORMED diff --git a/tests/conformance/signing/test_verifier_vectors.py b/tests/conformance/signing/test_verifier_vectors.py index 409b448f..99983815 100644 --- a/tests/conformance/signing/test_verifier_vectors.py +++ b/tests/conformance/signing/test_verifier_vectors.py @@ -35,28 +35,7 @@ # Marked strict: when the verifier is fixed the vector XPASSes and this module # goes red, which is the signal to delete the entry rather than leave a stale # exemption behind. -KNOWN_VERIFIER_GAPS: dict[str, str] = { - "021-duplicate-signature-input-label.json": ( - "#976: duplicate Signature-Input dictionary key is silently resolved instead of " - "rejected (RFC 8941 §3.2 / covered-component smuggling)" - ), - "022-multi-valued-content-type.json": ( - "#976: multi-valued Content-Type on a covered non-list field is not rejected at parse" - ), - "023-multi-valued-content-digest.json": ( - "#976: multi-valued Content-Digest (duplicate RFC 9530 algorithm) is not rejected at parse" - ), - # The signer half of #977 has landed: @target-uri canonicalization now - # converts a U-label to its A-label form. This vector is the verifier half - # -- a U-label arriving on the wire must be REJECTED, not converted -- and - # it expects request_signature_header_malformed at step 1, which is #976's - # precheck. Same predicate (canonical.host_has_raw_non_ascii), different - # code at a different step; it retires with #976, not with the conversion. - "026-non-ascii-host.json": ( - "#977 (verifier half): raw IDN U-label on the wire is not rejected at step 1; " - "lands with #976's strict header precheck" - ), -} +KNOWN_VERIFIER_GAPS: dict[str, str] = {} def _operation_from_url(url: str) -> str: