server(adcp-security): RFC 9421 webhook verifier (Ed25519 + ECDSA-P256, content-digest, typed errors)

[EmmaLouise2018]

Summary

Reference-server implementation of the AdCP 3.0 webhook-signing profile (test vectors were spec-merged in #2445).

• Uses http-message-signatures lib; supports Ed25519 and ECDSA-P256-SHA256 (with P1363 -> DER conversion for P-256)
• Verifies Content-Digest header matches payload
• Surfaces typed error codes (stale/expired/algorithm-confusion/content-digest-mismatch/etc.)
• 10 vitest cases covering valid-sig, invalid-sig, stale, expired, algorithm-confusion, content-digest-mismatch, and more

Files

• server/src/adcp-security/webhook-verifier.ts (new, 485 lines)
• server/src/adcp-security/__fixtures__/webhook-signature-keys.ts (new, 30 lines)
• server/tests/unit/webhook-verifier.test.ts (new, 234 lines, 10/10 passing)
• package.json — adds http-message-signatures ^1.0.5

Context

Split out from closed #2474 so the server-side verifier can land independently of the other red-team-batch-3 workstreams.

Test plan

• npm run typecheck passes
• npx vitest run server/tests/unit/webhook-verifier.test.ts — 10/10 cases pass
• Pre-commit hook ran the full server unit test suite (587 tests) — all green

[EmmaLouise2018]

RFC 9421 Webhook Verifier Review

Solid MVP verifier with good error taxonomy and clean structure. Strong test scaffolding (real signing with node:crypto + fixture keys). A few concerns worth addressing before this gets wired into a handler — several are security-relevant.

Critical / security

1. extractLabelValue regex-free parser has subtle bugs (webhook-verifier.ts:697-743)

• indexOf(needle) finds the first occurrence of <label>=, but dictionary member values can legitimately contain sig1=... substrings (e.g. inside a quoted string nonce=\"...sig1=...\"). For the canonical AdCP profile this is unlikely, but it's a correctness landmine if inputs ever grow.
• The quoted-string handling increments i += 1 on backslash but continues the loop, which then does i += 1 again — consuming two chars per escape. That's actually correct for \\\", but worth a comment since it's not obvious.
• Stronger: use the raw header value parsed.raw together with the label name as a key search from the start of a top-level member. Preferred: reserialize the parsed structure rather than byte-slicing.

2. Signature base reconstruction risk (webhook-verifier.ts:678-695)

• You delegate to httpbis.createSignatureBase but then append @signature-params from a manually byte-sliced raw segment. Any whitespace-normalization difference between what the signer emitted and what the verifier extracts produces a mismatch that looks like a bad signature rather than a malformed header. Consider adding a negative test with whitespace variants (; created= vs ;created=) to pin down behavior.
• The test at L956-966 builds its signature base manually in parallel — so the test never actually exercises buildSignatureBase's httpbis round-trip. A test that signs via library A and verifies via library B would catch interop drift.

3. Timing-attack surface on digest compare (webhook-verifier.ts:527-529)

• Comment says "Constant-time compare would be ideal; content-digest is not a MAC so plain equality is acceptable." Agreed on the threat model, but the signature-base then trusts the Content-Digest header to bind body to sig. If content-digest is in components, it IS effectively part of the MAC. Using crypto.timingSafeEqual costs nothing and avoids debates — please just use it.

4. No tag / nonce / algorithm-confusion coverage in tests

• The summary claims "algorithm-confusion" is covered but I don't see a test where signer says alg=\"ed25519\" while JWK is P-256 (or vice versa). Please add: (a) JWK alg present and mismatched to Signature-Input alg, (b) JWK kty=EC but Signature-Input alg=ed25519, (c) ES256 signature with P1363 length tampering.
• No test verifies tag=\"adcp/webhook-signing/v1\" — you note it's out of scope, but the verifier doesn't even parse it. Recommend at minimum surfacing tag in VerifyResult so call sites can enforce.

Correctness

5. P1363 → DER conversion edge case (webhook-verifier.ts:646-672)

• if (sig.length % 2 !== 0) is correct, but there's no check that sig.length === 64 for P-256. A 32-byte or 66-byte signature would be silently "converted" and fail verification with webhook_signature_invalid instead of webhook_signature_header_malformed. Minor, but taxonomy matters.
• body[0] !== undefined — if after stripping leading zeros body is empty (r or s = 0, which is invalid anyway), you emit 02 00 which DER-decoders reject. Safer: reject empty body explicitly.

6. expires - created > 300 → webhook_signature_expired (webhook-verifier.ts:819-821)

• Semantically this is a malformed signature (spec violation), not an expired one. The error code conflates "clock drifted" with "attacker forged a 1-hour window." Consider a distinct code or route this to webhook_signature_header_malformed.

7. algSpec accepts both ed25519/EdDSA and ecdsa-p256-sha256/ES256 (webhook-verifier.ts:629-640)

• RFC 9421's IANA registry uses ed25519 and ecdsa-p256-sha256. Accepting JOSE names (EdDSA, ES256) is lenient — is that intentional for the AdCP profile? Spec alignment question: should the verifier reject non-9421 names?

Questions

• @signature-params re-emission uses the raw string. What happens if a buggy signer uses Created (capitalized param)? structured-headers lowercases on parse; your extractLabelValue does a case-sensitive indexOf. Is this tested?
• requireContentDigest=true is the default (good), but the handler-integration path isn't in this PR. Is there a lint / test ensuring call sites don't flip this to false in prod?
• Why Promise<VerifyResult> when every branch is synchronous? Async-by-default is fine, but leaves perf on the table for hot paths.

Nits

• toBase64 wrapper (L494) is a one-liner — inline or drop.
• BareItemValue = ... | unknown collapses to unknown; drop the union (L538).
• The changeset file is empty aside from --- / --- — fine for --empty but no description of what changed.

Overall: request changes on #3 (timing-safe compare) and #4 (alg-confusion test coverage). Everything else is discussion / polish. Nice work on the P1363→DER conversion and the typed error taxonomy.

[EmmaLouise2018]

Thanks for the thorough review. Addressed in bcf73b9.

Critical / security

1. extractLabelValue parser — rewrote to anchor on top-level dictionary members (comma-separated, depth/quote-aware), with whitespace-tolerant name=value split and case-insensitive label match. A nonce="...,sig1=..." no longer fools the scan. Returns null on miss instead of silently returning the whole header. The double-i+=1 on \\ was indeed correct but is now explicit.
2. Signature base reconstruction — kept the byte-slice approach (RFC 9421 §2.5 requires byte-fidelity for @signature-params) but the new extractor is whitespace-robust. Added a test that uses non-default param separators to pin the behavior. (Library-A-signs / library-B-verifies interop test deferred — happy to add as a follow-up if you want.)
3. Timing-safe digest compare — done. crypto.timingSafeEqual(asBuffer, expected) after the length check. Comment updated to note that content-digest IS in the signature base so this compare effectively participates in MAC verification.
4. Algorithm-confusion coverage —
   • Split algSpec into algSpecRfc9421 (Signature-Input only: ed25519, ecdsa-p256-sha256) and algSpecJwk (JWK alg only: EdDSA, ES256). JOSE names in Signature-Input are now rejected.
   • Added tests: (a) keyid → ES256 JWK with Signature-Input alg=ed25519, (b) keyid → Ed25519 JWK with Signature-Input alg=ecdsa-p256-sha256, (c) ES256 with truncated 32-byte P1363 signature → webhook_signature_header_malformed.
   • tag is now parsed and surfaced on VerifyResult (typed as optional tag?: string); enforcement remains a call-site responsibility.

Correctness

5. P1363 length / zero-component guards — p1363ToDer(sig, expectedLen) now requires the length to match exactly (64 for P-256), and refuses to encode an integer with empty contents (zero r or s). Length mismatch is caught upstream and routed to webhook_signature_header_malformed per your taxonomy point.
6. expires - created > 300 taxonomy — moved to webhook_signature_header_malformed. Comment notes this is a spec violation (forged long window) distinct from a clock-skew expiry.
7. JOSE alg leniency — removed. Only RFC 9421 IANA names are accepted in Signature-Input.

Questions

• Capitalized Created param — structured-headers lowercases on parse, and the verifier uses the parsed value for semantics; for @signature-params re-emission we use the raw bytes verbatim, so the verifier and a buggy capital-C signer would still hash the same string. Not separately tested but unaffected by the new extractor (case-insensitive on label name, parameter names left as-is).
• requireContentDigest=true enforcement at call sites — no lint yet, since there are no call sites; will plan to enforce when the handler integration lands.
• Promise<VerifyResult> — dropped. Function is now synchronous.

Nits

• toBase64 wrapper removed (timingSafeEqual doesn't need it).
• BareItemValue no longer has | unknown.
• Changeset now has a real description.

[bokelley]

Superseded by @adcp/client — the RFC 9421 webhook verifier, JWKS resolution, and conformance runner all shipped upstream in the last ~48h:

• feat: RFC 9421 request-signing profile (AdCP 3.0 optional) adcp-client#587 — RFC 9421 request-signing profile
• feat(signing)!: verifier API v3 + HTTPS JWKS / revocation stores adcp-client#601 — verifier API v3 + HTTPS JWKS / revocation stores
• feat(storyboard): outbound-webhook conformance runner (#2426) + 9421 webhook verifier adcp-client#631 — outbound-webhook conformance runner + 9421 webhook verifier
• feat(signing): brand.json → JWKS auto-resolver (follow-up to #631) adcp-client#634 — brand.json → JWKS auto-resolver

If the adcp site server ever needs webhook verification at runtime, it should depend on @adcp/client rather than carrying a parallel 485-LOC crypto implementation plus its own http-message-signatures dependency. Closing to avoid maintaining two verifiers.