feat(signing): sign/verify webhooks with the request-signing key; deprecate webhook-signing#2239
Conversation
Webhook verifier step 8 now accepts a key whose adcp_use is either 'webhook-signing' or 'request-signing', letting a signer reuse the request-signing key it already publishes instead of minting a dedicated webhook key. Any other purpose (response-signing, governance-signing, unknown) is still rejected with webhook_mode_mismatch. Safe because domain separation is carried by the RFC 9421 tag (adcp/webhook-signing/v1, part of the signed base) plus mandatory content-digest coverage, not by the key-purpose discriminator: a captured request signature can never be replayed against the webhook verifier because step 3 rejects its tag. signWebhook / signWebhookAsync accept the same set. Conformance vector 008 flipped from negative (request-signing rejected) to positive (request-signing reuse accepted); a new negative 008 covers a response-signing key. A dedicated webhook key remains RECOMMENDED but is no longer REQUIRED. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6ddc57c to
083df78
Compare
There was a problem hiding this comment.
The crypto is sound; the question is sequencing. This relaxes a normative conformance vector ahead of an unratified spec — and that's the author's own open question, not a defect to bounce, so I'm commenting rather than blocking or approving.
The security argument holds. security-reviewer: sound. Step 3 (webhook-verifier.ts:124) checks the signature tag — part of the signed base, covered by the step-10 verify — before keyid resolution (step 7) or the relaxed purpose check (step 8, :188). A captured request signature carries tag=adcp/request-signing/v1 and dies at step 3, never reaching step 8. The reverse direction is closed independently: verifier.ts step 8 still demands request-signing exactly, and a webhook signature's adcp/webhook-signing/v1 tag is rejected at the request verifier's own step 3. Relaxing step 8 grants no new attacker capability — the holder of the request-signing private key is the same trusted publisher who could already mint a webhook-signing key. tag is the cross-protocol fence, not adcp_use. Agreed.
code-reviewer: clean, three Low nits below. The vector reuse is legitimate — 008-request-signing-key-reuse reuses the byte-identical Ed25519 signature from the former negative vector because the base and key are unchanged and Ed25519 is deterministic. Right call, cheapest faithful representation.
So why not approve. ad-tech-protocol-expert: unsound — ship-behind-spec. The PR body frames this as a "Proposal / RFC for discussion" "paired with a spec PR in adcontextprotocol/adcp," with an open implicit-vs-explicit design question. Flipping normative negative vector 008 (request-signing rejected → accepted) bakes un-ratified spec opinion into verifier behavior. Per CLAUDE.md's spec → mock → SDK triage order, when the spec defines the contract the SDK conforms — it doesn't redefine the contract and rewrite the vector to match. compliance/cache/ is absent in this checkout, so I can't confirm the spec PR has merged.
And the relaxation is half-applied. The verifier and the vector moved; two SDK docs that call the same reuse "forbidden" did not:
docs/guides/SIGNING-GUIDE.md:43— "Reusing a key across purposes is forbidden by the spec."docs/guides/SIGNING-GUIDE.md:552— "Both halves of the key MUST carryadcp_use: \"webhook-signing\"" (citing adcp#2423).test/fixtures/webhook-signing-vectors/README.md:27— "adcp_useMUST be\"webhook-signing\"on the verifying JWK";:45still describes 008 aswebhook_mode_mismatch (step 8; adcp_use=request-signing).
Things I checked
- Step ordering: tag (3) → keyid (7) → purpose (8) → revocation (9) → replay precheck (12) → crypto (10) → digest (11) → commit (13). The relaxed
adcp_usecheck is downstream of the tag fence.webhook-verifier.ts:124,156,188. - Reverse-direction replay:
verifier.tsrequest path unchanged, still single-purposerequest-signingat step 8, tag-gated at step 3. No loop opened. signWebhookBypassingPurposeGateis test-local (webhook-verifier-error-codes.test.js), composed from publicprepareWebhookSignature+finalizeRequestSignatureprimitives — not a production bypass surface.- Changeset present and correctly typed:
.changeset/webhook-verify-accept-request-signing-key.md,minor— additive accept-widening, no exported symbol removed (assertWebhookProviderPurposeis new). Right shape. - Negative coverage preserved:
response-signing→webhook_mode_mismatch,undefined/no-verify →webhook_signature_key_purpose_invalid. Failure-mode split intact.
What flips this to approve
- The paired
adcontextprotocol/adcpspec PR is merged andcompliance/cachere-synced to reflect it. Link it here. - The implicit-vs-explicit question is resolved. If the spec lands explicit (JWK declares an array/combined
adcp_use), this implicit form is a second breaking change — worth settling before shipping either. SIGNING-GUIDE.md(:43, :552) and the vectorsREADME.md(:27, :45) are corrected in this PR, in lockstep with the vector flip.
Minor nits (non-blocking)
- Wrong error code in the new doc comment.
signer.tsWEBHOOK_SIGNING_PURPOSESblock claims non-webhook purposes are "refused with the samewebhook_signature_key_purpose_invalidcode the verifier emits." The verifier emitswebhook_mode_mismatchfor a known-but-wrong purpose likeresponse-signing(webhook-verifier.ts:188), notwebhook_signature_key_purpose_invalid. Drop the "same code the verifier emits" clause. WEBHOOK_SIGNING_PURPOSES: readonly string[]loses the compile-time exhaustiveness the siblingthrowIfPurposeMismatchswitch has via itsneverarm.['webhook-signing','request-signing'] as consttypedreadonly Rfc9421AdcpUse[]would triptscif the union widens.assertWebhookProviderPurpose'srequirePurposeoption is never passed by any caller — kept only for signature parity withassertProviderPurpose. Defensible, but unexercised.
Happy to approve fast once the spec PR is linked and merged and the two docs move with the vector. The engineering is right; the ordering against the spec is what I'm holding on.
There was a problem hiding this comment.
Relaxes a verifier acceptance rule the current published spec still marks MUST-reject, shipped as the default in a published minor ahead of any ratified spec change — hold it. A verifier loosening an acceptance rule is a security-policy downgrade, not an additive change; the SDK must not lead the spec on a MUST.
To be clear up front: the crypto is fine. The block is spec sequencing.
Things I checked
- Domain-separation argument holds. The
tagis bound into@signature-params(canonicalize.ts:127), checked at step 3 (webhook-verifier.ts:124) before JWKS resolution and before the step-8 purpose gate, and reconstructed verbatim for the verify (webhook-verifier.ts:238-245). A captured request signature carriestag=adcp/request-signing/v1and dies at step 3 — it never reaches the widened step 8.security-reviewer: SOUND, no reachable replay/confusion/substitution vector; request-side verifier symmetry untouched (verifier.ts:115,156). This is not a crypto hole. - Signer gate mirrors the existing pattern. New
assertWebhookProviderPurpose(signer.ts) preserves theassertProviderPurposesoft-binding semantics —adcpUse === undefined && !requirePurpose→ skip. Consistent, no regression. - Conformance vectors internally consistent. Flipped
008reuses the original valid Ed25519 signature; new negative008(response-signing) + newtest-response-purpose-2026key inkeys.jsonline up withexpected_outcome. - Changeset present,
minor. Right type for the shape of the change (additive acceptance). The question is whether the change should ship at all yet — see below.
Must fix (blocking)
Spec divergence. ad-tech-protocol-expert: unsound. The current published spec — docs/building/by-layer/L1/security.mdx, webhook verifier checklist step 8 — reads: "adcp_use equals `webhook-signing`. Reject on any mismatch, including absent adcp_use," and the key-publication section: "a request-signing key MUST NOT verify a webhook signature... Buyers verifying a webhook MUST reject any JWK whose adcp_use is not exactly webhook-signing." This PR ships a published @adcp/sdk minor whose default verifier stops enforcing that MUST-reject. Per CLAUDE.md triage (spec → mock → SDK; "Don't bake ... into SDK behavior — that compounds drift"), the SDK must not lead the spec on a security MUST. "Accepts a superset, rejects nothing" is the wrong frame for a verifier: every buyer that upgrades the minor silently stops enforcing a spec-mandated reject, with no opt-out.
The expert could not surface the paired spec PR, and found the spec moving the opposite direction (adcp#2502 reaffirms adcp_use prevents cross-purpose verification). The changeset asserts "Tracks the spec change in adcontextprotocol/adcp" — a changeset citing a spec change that, as far as I can find, is currently moving the other way.
What flips this to approve (either):
- Gate the relaxation behind an off-by-default verifier option so the published default still matches the spec MUST, while signers who want reuse can opt in; or
- Land after the spec PR merges, with the changeset linking the merged spec change.
This is the right venue order — the design argument (tag carries domain separation, not adcp_use) is legitimate, but it belongs in the spec PR first, not in the SDK's default behavior ahead of ratification. The "Open question" in the PR body (implicit superset vs. explicit multi-purpose adcp_use) is exactly the kind of thing the spec gets to decide before the SDK commits to one.
Fix before this lands (regardless of the gating decision)
- False JSDoc parity claim. The new block on
assertWebhookProviderPurpose(signer.ts) says disallowed purposes are "refused with the samewebhook_signature_key_purpose_invalidcode the verifier emits." Not true: the verifier emitswebhook_mode_mismatchfor a known-but-wrong purpose (webhook-verifier.ts:193-199) andwebhook_signature_key_purpose_invalidonly for missing / no-verify. The signer and verifier now disagree on the code for "wrong purpose," which breaks the parity invariant stated inassertKeyPurpose's own JSDoc. Fix the claim, or split the signer code to mirror the verifier. - Stale MUST-language docs.
webhook-emitter.ts:109,119-130andSIGNING-GUIDE.md:552still say the key MUST bewebhook-signingand MUST NOT be reused. Left untouched, they contradict the code on a security-sensitive feature. Update to RECOMMENDED. - End-to-end gap. If the registry
register()validator (SIGNING-GUIDE.md:552, adcp#2423) still rejectsrequest-signingkeys at publish time, the reuse path is unusable end-to-end — the verifier accepts a key the publisher can't register. Confirm and relax, or document the asymmetry.
Minor nits (non-blocking)
- Exhaustiveness coupling lost.
WEBHOOK_SIGNING_PURPOSES: readonly string[](signer.ts) drops thenever-switch exhaustiveness thatthrowIfPurposeMismatchhas — a future widening ofRfc9421AdcpUsewon't triptsc. Type itreadonly Rfc9421AdcpUse[].
Request changes — not on the crypto, which security-reviewer cleared, but on the sequencing: a published default that stops enforcing a spec MUST needs the spec to move first, or an opt-in flag so the default still complies.
…lures
Per reviewer feedback on the paired spec PR: stop overloading
webhook_mode_mismatch (reserved for the HMAC-vs-9421 auth-mode selector) for
key-purpose failures. Step 8 now emits webhook_signature_key_purpose_invalid
for absent adcp_use, a missing verify key_op, OR an adcp_use outside
{webhook-signing, request-signing}. Updates the negative 008 vector, keys.json
and fixture README, and the test assertion to match.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
The crypto is right. The spec ordering is the open question. Holding for an answer, not for code changes.
The design is sound: domain separation is carried by the signed RFC 9421 tag (adcp/webhook-signing/v1, checked at step 3, part of the @signature-params base — canonicalize.ts:44,126-127), not by the adcp_use discriminator at step 8. security-reviewer: LOW — a captured request signature dies at step 3 before it reaches the relaxed step 8, and the request verifier's step 8 (verifier.ts:156) is untouched, so the relaxation is one-directional and never weakens the higher-stakes request path. Both replay directions blocked at step 3 on signed bytes. No new capability for anyone who doesn't already hold the private key. That part I'm confident in.
What I can't sign off on from the diff: this flips a normative negative conformance vector (008-wrong-adcp-use — request-signing key MUST be rejected, webhook_mode_mismatch, per adcp#2467) into a positive one, and re-maps the wire error code on the response-signing path (webhook_mode_mismatch → webhook_signature_key_purpose_invalid). ad-tech-protocol-expert: unsound (ship-behind-spec) — per CLAUDE.md's spec → mock → SDK triage order, the SDK conforms to the published spec; it doesn't rewrite a normative vector to match an un-ratified spec PR. The SDK pins ADCP_VERSION = 3.1.0-rc.14. compliance/cache/ isn't in the checkout, so I can't confirm the paired adcontextprotocol/adcp PR has merged into that line. You flagged this yourself as a Proposal/RFC with an open question — right call.
Things I checked
- Step-8 boolean (
webhook-verifier.ts:161-179) is logically equivalent to the prior two-stage check except it now acceptsrequest-signing; undefinedadcp_use, missingverifykey_op, andresponse-signing/governance-signing/unknown all still reject withwebhook_signature_key_purpose_invalid. The?.guard onkey_opscorrectly fails a missing array. - Tag is cryptographically bound —
SIGNATURE_PARAMS_ORDERincludestag, serialized into@signature-params, appended to the signed base, and the verifier reusesparsedInput.signatureParamsValueverbatim. An attacker can't rewrite the tag without failing crypto verify at step 10. - Request verifier (
verifier.ts:156) NOT touched — still hard-requiresadcp_use === 'request-signing'. Correct asymmetry. webhook_mode_mismatchstays in theWebhookSignatureErrorCodeunion and thewebhook-assertions.ts:609switch arm — no enum value removed, exhaustivenevercheck still compiles. No internal caller breaks.- Positive vector
008-request-signing-key-reusereuses the original valid Ed25519 signature (OT_F-…) over an unchanged base — it was always crypto-valid, just rejected at step 8. Clean reuse. - Changeset present,
minor. Defensible: broadening accepted inputs is additive for legitimate signers, and request-signing keys that formerly producedwebhook_mode_mismatchnow verify, so no receiver observes the old code on valid traffic.
What flips this to approve
- Confirm the paired spec PR has merged into the
3.1.0-rcline the SDK pins (or the GA target), so the conformance-vector flip and error-code re-map track ratified spec — not get ahead of it. This is the gating item. Link it in the PR body. - Resolve the implicit-vs-explicit open question you raised. If maintainers want explicit opt-in (
adcp_useas an array / combined value), that changes the verifier shape and this PR.
Follow-ups (fix before merge once the spec gate clears — these are half-applied relaxation)
src/lib/signing/errors.ts:57-64— theWebhookSignatureErrorCodetaxonomy comment still describes the OLD split (webhook_mode_mismatch= "key scoped for a different mode, e.g. request-signing" / "mint a new key scoped for webhook-signing"). Now directly contradicts shipped behavior. This is the canonical type definition — update it.docs/guides/SIGNING-GUIDE.md:43("Reusing a key across purposes is forbidden by the spec"),:279(cross-protocol KMS reuse is "a cross-protocol oracle"),:552("Both halves of the key MUST carryadcp_use: \"webhook-signing\"", citing adcp#2423) — all still forbid what this PR allows. The relaxation is half-applied.test/fixtures/webhook-signing-vectors/README.md:27— "adcp_useMUST bewebhook-signingon the verifying JWK" left unedited, contradicts the rest of the file.
Minor nits (non-blocking)
- PR body prose disagrees with the code. The body says non-webhook purposes are "still rejected with
webhook_mode_mismatch" — the code and changeset correctly usewebhook_signature_key_purpose_invalid. The code is right; fix the prose so reviewers aren't chasing a phantom. - New negative vector's signature is never crypto-verified.
negative/008-wrong-adcp-use.json(response-signing key,lgEK6…) is rejected at step 8 before crypto verify, and the negative-vector test loop asserts only the error code — so a malformed signature there would pass silently. ConfirmlgEK6…validates againsttest-response-purpose-2026, or note it's unverified-by-design since step 8 short-circuits. (code-reviewerflag.)
Sound code, correct security model. The only thing between this and an approve is confirming the spec ratified ahead of the SDK, plus the doc cleanup so the repo stops contradicting itself. Link the spec PR and I'll take another pass.
…bhook-signing Reframe to the unified model: webhooks are signed with a request-signing key (no separate webhook key purpose). Verifier step 8 and the signer gates still accept request-signing and the deprecated webhook-signing (backward compat, removed in 4.0); the error message and all doc comments now lead with request-signing. Marks the AdcpUse 'webhook-signing' member deprecated and updates the webhook-emitter key-purpose guidance (reuse the request-signing provider; isolate via a second request-signing kid, not a distinct adcp_use). No behavior change — code already accepted both purposes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Approving on the strength of the domain-separation proof plus the additive verifier relaxation. The right call: the cryptographically-bound tag is the load-bearing control, the adcp_use key-purpose check was operational hygiene, and dropping the weaker control while keeping the signature-bound one breaks no conformant signer.
Things I checked
- The safety claim, at the byte level.
tagis one of the six@signature-paramsand lands on the final line of the signed base (canonicalize.ts:126-128), reconstructed and crypto-verified atwebhook-verifier.ts:238-245. An attacker cannot strip or swap it without invalidating the signature, and step 3 (webhook-verifier.ts:124) string-rejects a non-matching tag before any crypto runs.security-reviewer: SOUND, no High. Cross-protocol replay is closed in both directions — a captured request signature carriestag=adcp/request-signing/v1and dies at step 3; the request verifier (verifier.ts:156) is untouched and still hard-requiresrequest-signing. - Step-8 condition (
webhook-verifier.ts, diff L274-278): rejects on absentadcp_use, missingverifykey_op, oradcp_use ∉ {request-signing, webhook-signing}. No short-circuit hazard — clause 3 only evaluates whenadcp_useis defined. Matches the changeset, the tests, and both vectors. - Replay surface unchanged. Window (
webhook-verifier.ts:301-329),(keyid, @target-uri)dedup scope, per-keyid cap, and commit-after-all-checks ordering are all outside the diff.content-digeststays unconditional for webhooks. webhook_mode_mismatchis reserved, not orphaned. Still a union member inerrors.tsand still mapped atwebhook-assertions.ts:609.ad-tech-protocol-expertconfirmed the spec's normative text (core.generated.ts:9151) defines it as the HMAC-vs-9421 registered-mode selector — so this PR moves the SDK toward spec and reverts the adcp#2467 SDK-local meaning, rather than inventing one.- Changeset. Present,
minor. Package is at9.0.0-beta.29; the error-code reassignment folds under the unreleased major andminor-within-beta calculates correctly. No exported symbol removed, no enum value dropped. - Vector reclassification.
positive/008-request-signing-key-reusereuses the identical key, base, and signature bytes from old negative-008 — clean. New negative-008 uses a freshtest-response-purpose-2026key; step 8 runs before crypto, sofailedStep === 8holds regardless of the new signature bytes.
Follow-ups (non-blocking — file as issues)
- Gate the merge on the paired spec PR + a lockstep vector cut.
ad-tech-protocol-expert: SOUND-WITH-CAVEATS. The verifier acceptance relaxation is additive and safe to ship today. The conformance-vector repurposing is not — flipping the published008from negative→positive while other SDKs pin/compliance/{version}/test-vectors/webhook-signing/negative/008-wrong-adcp-use.jsonand asserterror_codebyte-for-byte means two conformant SDKs grade each other as failing until the CDN vector set is re-cut. Confirm theadcontextprotocol/adcpspec PR has landed and the versioned vector set ships in the same release before this merges. - Blast-radius posture. Reusing one
request-signingkey for both requests and webhooks widens single-key-compromise blast radius. It's opt-in and the alternative (a secondrequest-signingkey under a distinctkid) is documented, but steer first-time integrators toward the distinctkidin the emitter quickstart so secure-by-default isn't lost to the path of least resistance.
Minor nits (non-blocking)
- PR body contradicts the code. The description says other purposes are "still rejected with `webhook_mode_mismatch`". The code, changeset, and vectors all use `webhook_signature_key_purpose_invalid`. The implementation is internally consistent — fix the prose so the merge commit doesn't bake in the wrong code.
@adcp/sdk→@adcp/clientis a regression.test/fixtures/webhook-signing-vectors/README.md(diff L376, L379) now points readers at a package that doesn't exist. The npm name is@adcp/sdk— same as this PR's own changeset header. The repo is namedadcp-client; the package is not. Revert both.- Changeset understates the observable change. It frames
webhook_mode_mismatchas "unchanged/reserved" but doesn't state that aresponse-signing/unknown key now returnswebhook_signature_key_purpose_invalidwhere v5.2.0 returnedwebhook_mode_mismatch. One sentence naming the changed code helps consumers who branch on it. requirePurposeis a dead param onassertWebhookProviderPurpose(signer.ts:206-212) — no caller passestrue. Harmless; add a one-line note that it's intentionally unused or drop it.- Spec-URL drift. The README moves to
docs/building/by-layer/L1/securitybutdocs/guides/SIGNING-GUIDE.md:685still usesbuilding/implementation/security.ci:doc-linkswon't catch externaladcontextprotocol.orgURLs — make the two agree on whichever is canonical.
One note on the fixtures: a key whose \$comment says it exists "so the verifier MUST reject at step 8" is now the positive vector's accepted key. Minting a fresh test-request-purpose-2026 rather than re-flagging test-wrong-purpose-2026 would spare the next reader the double-take.
Approving on the code's merits. Sequence the merge behind the spec PR.
Mirror the spec review fix: replace hard-coded '4.0' removal language in the AdcpUse deprecation notes, verifier/signer/emitter comments, changeset, and vendored conformance README with a reference to the tracking issue (adcontextprotocol/adcp#5555), since the removal window is a WG decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
…re paths Align with the merged spec (adcontextprotocol/adcp#5552): webhooks are signed with the request-signing key, so the sell-side emission grader (assertJwksPurpose) and the tenant-registry webhook auto-wire (assertWebhookSigningUse) must accept adcp_use 'request-signing' as well as the deprecated 'webhook-signing'. Previously both hard-required 'webhook-signing' and would reject a conformant request-signing agent. Updates the auto-wire tests and the signWebhook doc comment to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Spec PR merged (adcontextprotocol/adcp#5552). The final merged decision matches this SDK PR's behavior exactly:
Maintainers added two enforcement surfaces on merge that this PR didn't originally cover — fixed in
Note: the generated types still carry |
|
Thanks for the spec-alignment summary, @BaiyuScope3. Noted: adcontextprotocol/adcp#5552 is merged, the PR's verifier behavior and conformance vectors match the final spec, and the two enforcement surfaces (emission grader + tenant-registry auto-wire) are already addressed in Generated by Claude Code |
|
Acknowledged — noted @bokelley's confirmation that this PR is spec-aligned and ready for maintainer merge review. No triage action outstanding. Generated by Claude Code |
There was a problem hiding this comment.
Holding this — not because the code is wrong, but because it ships ahead of the spec it's paired with. The engineering is clean; the sequencing is the review.
The crypto reasoning holds. security-reviewer verified the load-bearing claim against the actual code, not the prose: the tag lives in the signed @signature-params base (canonicalize.ts:126-128), and both verifiers pin their tag unconditionally before the key check and before crypto — webhook step 3 at webhook-verifier.ts:124, request step 3 at verifier.ts:115. The request verifier's key-purpose check stays strict (verifier.ts:156). So the relaxation is one-directional and shared key material does not enable cross-domain replay. Domain separation by tag, isolation by kid — right model. tag is the load-bearing control; adcp_use was belt-and-suspenders here.
Things I checked
- Step-8 boolean (
webhook-verifier.ts, new form): accept iffadcp_use ∈ {request-signing, webhook-signing}ANDkey_opsincludesverify.?.short-circuits safely on missingkey_ops.response-signing/governance-signing/unknown/absent all still rejected withwebhook_signature_key_purpose_invalid. Correct —code-reviewerandjavascript-protocol-expertboth confirmed the truth table. - Signer/verifier
adcpUse-undefined asymmetry (signer.tsassertWebhookProviderPurpose): the early-return on undefined provider purpose is not a downgrade — it's identical to the pre-existingassertProviderPurposecontract, andSigningProvider.adcpUsenever reaches the wire. The verifier readsadcp_useoff the published JWK, which the SDK can't see through the provider interface. Benign, pre-existing. - Revocation stays
kid-scoped on both paths (webhook-verifier.ts:200,verifier.ts:166) — no regression. Replay scoping, pre-crypto short-circuits, and commit-after-all-checks ordering untouched. - Changeset: present,
minor. Correct — pure widening (verifiers/signers accept a superset), new exportassertWebhookProviderPurpose, no symbol removed. - Conformance vectors: positive
008-request-signing-key-reusereuses the original valid Ed25519 signature bytes; new negative008-wrong-adcp-useuses a genuineresponse-signingkey. Mechanically consistent.
Why this is a hold, not an approve
ad-tech-protocol-expert: unsound as a mergeable change. Two cited reasons, both real:
-
Inverting a published conformance invariant ahead of a merged spec. These vectors aren't internal tests — the README states they're served at
https://adcontextprotocol.org/compliance/{version}/test-vectors/webhook-signing/for cross-implementation grading, and that "a verifier MUST accept both." Flipping008from MUST-reject to MUST-accept whileadcontextprotocol/adcp#5555is admittedly unmerged tells any SDK grading againstlatestthat arequest-signingwebhook key is conformant — before the spec says so. That's the exact failure mode CLAUDE.md's triage rule (spec → mock → SDK, "don't bake storyboard opinion into SDK behavior") exists to prevent. Your own PR body — "Proposal / RFC for discussion," "Open question for maintainers" (implicit superset vs. explicit multi-purposeadcp_use) — concedes the design isn't settled. The SDK shouldn't pick before the spec does. -
webhook_mode_mismatchis now stranded. After this diff it has zero producers insrc/lib/signing/, yet it remains a schema-generated wire code mapped atcore.generated.ts:609. The "reserved for the HMAC-vs-9421 auth-mode selector" justification is aspirational — the HMAC path (webhooks/index.ts) returnsmissing_secret/bad_signature, and the storyboard HMAC check emitslegacy_hmac_fallback.removed. Reserving a published error code by comment, with no code path, is taxonomy debt that the next reader inherits.
What flips this to approve
adcontextprotocol/adcp#5555merged (or confirmed-open with the implicit-superset form adopted), so the SDK follows the spec rather than leads it; or land the positive case as an additive009-series vector behind a version gate instead of inverting the published negative008.webhook_mode_mismatcheither wired to a real producer or formally deprecated in the schema — not reassigned by comment.
Follow-ups (non-blocking — fix in this PR if you respin)
- Stale emitter docs.
webhook-emitter.ts:109and:119-130still sayadcp_useMUST be"webhook-signing"and "mint a secondcryptoKeyVersion... rather than sharing the request-signing key" — the exact opposite of what this PR implements. It's the public emit surface; an adopter reading it gets the inverted instruction.javascript-protocol-expertflagged this. - Duplicated purpose list.
WEBHOOK_SIGNING_PURPOSES(signer.ts:248) andWEBHOOK_WIRE_PURPOSES(tenant-registry.ts:60) are identical['request-signing','webhook-signing']. When adcp#5555 removeswebhook-signing, both must move in lockstep — export one constant. - Webhook path skips
validateJwkParameterConsistency(alg/kty/crv binding) that the request verifier runs (verifier.ts:163). Pre-existing, Low, but this PR encourages one key flowing through both paths, so the alg-downgrade-resistance asymmetry is worth an issue.
Minor nits (non-blocking)
- Step-8 error message.
webhook-verifier.tsinterpolates onlyadcp_use="...", so a key that fails solely on a missingverifykey_op reports its validadcp_usewith no mention of the key_op — misleading for that path. Pre-existing message shape.
The implementation is the easy part and you nailed it. Land the spec, then this is a quick approve.
Why
A signer that already publishes an
adcp_use: "request-signing"key (for outbound RFC 9421 request signing) currently must also mint and publish a second, dedicatedadcp_use: "webhook-signing"key to sign webhooks — the webhook verifier rejects any other purpose at step 8 (webhook_mode_mismatch). Maintaining two KMS keys, two rotation procedures, and two JWKS entries is real operational overhead for signers who'd rather reuse one key.This PR lets a signer choose: reuse the existing request-signing key for webhooks, or keep a dedicated webhook-signing key. The verifier accepts both.
Why it's safe
Cross-protocol confusion is prevented by the RFC 9421
tag(adcp/webhook-signing/v1), which is part of the signed base and checked at step 3, plus the mandatorycontent-digestcoverage at step 6 — not by theadcp_usekey-purpose discriminator at step 8. A captured request signature carriestag=adcp/request-signing/v1and can never be replayed against the webhook verifier because step 3 rejects it. So step 8's purpose check is operational hygiene, not the load-bearing domain-separation control.What changed
webhook-verifier.ts(step 8): acceptadcp_use ∈ {webhook-signing, request-signing}. Any other purpose (response-signing,governance-signing, unknown) is still rejected withwebhook_mode_mismatch. The "missing purpose / noverifykey_op" failure (webhook_signature_key_purpose_invalid) is unchanged.signer.ts/signer-async.ts:signWebhook/signWebhookAsyncaccept the same{webhook-signing, request-signing}set via a dedicated webhook purpose gate (the generic single-purpose gates for request/response signing are untouched).008-wrong-adcp-use(request-signing key rejected) → positive008-request-signing-key-reuse(accepted, original valid Ed25519 signature reused). New negative008-wrong-adcp-usecovers aresponse-signingkey (still rejected at step 8). Newtest-response-purpose-2026key added tokeys.json.webhook-verifier-error-codes.test.jsandsigning-provider-purpose-gate.test.js.A dedicated webhook-signing key remains RECOMMENDED (blast-radius isolation, independent rotation) but is no longer REQUIRED.
Open question for maintainers
This implements the implicit form: a
request-signingkey is accepted for webhook verification. An explicit alternative is to let a JWK declare multiple purposes (adcp_useas an array, or a combined value) so reuse is opt-in rather than a request-signing superset. I went with implicit for minimal surface, but happy to switch if you prefer explicit.Verification
positive/008-request-signing-key-reuse,negative/008-wrong-adcp-use).tsc(viabuild:lib) compiles cleanly.joseESMrequire()and arevocation_stalere-map) — both reproduce on a clean checkout without this change, so they're environmental, not introduced here. CI on supported Node will be authoritative.🤖 Generated with Claude Code