feat(sep-1932): DPoP discovery signals (stacked on #528) - #529
nbarbettini wants to merge 6 commits into
Conversation
…ocol#369) Follow-up on the DPoP client PR (shared foundation). Adds the server-role conformance for SEP-1932 / RFC 9449: the framework acts as a DPoP client against the MCP server under test and emits the sep-1932-server-* checks across the RFC 9449 §4.3 validation surface — proof validation, the ±5-minute iat window, asymmetric-only algorithms, the 401 + WWW-Authenticate challenge, token audience validation under DPoP, and the optional server-provided nonce. - src/scenarios/server/auth/dpop.ts (+ test, spec-references): the scenario. - examples/servers/typescript/sep-1932-{compliant,broken}-server.ts: passing and failing fixtures proving every check passes and fails. - Registered in the pending + all-client scenario lists. Depends only on the shared DPoP foundation (dpopProof/dpopToken); independent of the authorization-server PR. No DPoP-capable MCP SDK exists yet, so correctness rests on RFC vectors + an independent verifier + the fixtures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Refresh the held DPoP nonce from every response's DPoP-Nonce header (RFC 9449 §8.2 newest-wins) instead of capturing it once, so a server that rotates or single-uses its nonce no longer turns the negative probes into false "not testable" failures (or vacuous passes). (modelcontextprotocol#1) - Widen the stale iat probe to -303 s (matching the future side) so the ±1 s quantization of the whole-second Date header can't pull it onto the ±300 s boundary and be false-accepted. (modelcontextprotocol#2) - Update the isNonceChallenge comment and the untestable message: probes now carry the held nonce, so a use_dpop_nonce challenge is about the nonce lifetime (rotated/stale/single-use), not a missing nonce. (modelcontextprotocol#3) - Fixture: parse integer env vars NaN-safely (intEnv helper) so a malformed DPOP_CLOCK_OFFSET_SECONDS / DPOP_IAT_SKEW_SECONDS falls back to its default instead of silently disabling the iat window. (modelcontextprotocol#4) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Soften the heldNonce comment: newest-wins refresh is correct for ROTATING servers; a strict single-use server that re-arms only via challenges can still push alternate negatives to untestable (correctly reported, not mis-scored) — the previous "rotate or single-use" over-claimed. (R5) - Fix the clock-skew test comment: the stale probe is now -303s (~273s old), not -301s/~271s. (R5) Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A crash or 404 must not count as correctly refusing a DPoP-bound token presented as Bearer. Co-authored-by: Cursor <cursoragent@cursor.com>
A challenge with no error code was passing. The code is a SHOULD, so a wrong or missing value is a warning and the existing MUST checks stay as they are. Co-authored-by: Cursor <cursoragent@cursor.com>
A client that has never met the server has to learn the requirement from the 401 challenge and from protected resource metadata. A missing challenge or metadata field is a failure, and the advertisement has to match what the server enforces. Co-authored-by: Cursor <cursoragent@cursor.com>
f20bf07 to
5d213b9
Compare
|
@nbarbettini I am reviewing the PR and also revisiting the SEP1932 text. I think the following is overly strict (or at least as I think the tests are implemented):
Here are my thoughts process. I think if the MCP server supports DPoP it MUST advertise that support in protected resource metadata (PRM). However, there will be a lot of MCP servers out there that does not support it and won't advertise that they don't support it, they just won't have that field set. According to RFC9728, absence of a field means "not supported". I don''t think we want to register absence of the field as a failure, but rather as being equivalent to being set to "false". I feel like this is a more reasonable approach and avoids a lot of servers having to advertise "false" when they can achieve the same by just not advertising support. |
| DISCOVERY_REFERENCES | ||
| ); | ||
| } | ||
| if (!algsOk || !requiredPresent) { |
There was a problem hiding this comment.
Grading change needed here: an absent dpop_bound_access_tokens_required is a valid state, not a violation (see proposed update to SEP-1932 modelcontextprotocol/ext-auth#33).
RFC 9728 §2 defines the field as OPTIONAL with "If omitted, the default value is false" — so silence is a defined answer ("DPoP-bound tokens not required, bearer OK"), not a missing one. SEP-1932 now states this explicitly in the new "MCP Server DPoP Advertisement" section added in modelcontextprotocol/ext-auth#33: "When the field is absent, the default behavior defined in RFC 9728 applies and DPoP-bound access tokens are not required." Requiring the field's presence would also fail every metadata generator that correctly follows the RFC.
As written, requiredPresent folds absence into this FAILURE gate, so a fully conformant bearer-accepting server — correct dpop_signing_alg_values_supported, flag validly omitted — is failed with "metadata is missing dpop_bound_access_tokens_required" (reproduced against a live fixture). The consistency check downstream already handles the absent case correctly via its own arms, so nothing else depends on presence here.
Suggested reshape of this gate:
- absent → SUCCESS (keep recording
dpop_bound_access_tokens_required: nullin details, which you already do); - present but not a boolean → keep FAILURE, but as "malformed", not "missing";
- and while splitting the message:
!algsOkalso fires for a present-but-empty (or non-array)dpop_signing_alg_values_supported, which this message reports as "missing" — worth separate wording ("empty"/"malformed") so operators aren't sent looking for a field that's already there.
Generated with the aid of Claude (Fable 5).
| algs: algs ?? null, | ||
| resourceMetadata: resourceMetadataParam(res.wwwAuthenticate) ?? null | ||
| }; | ||
| if (!challenged) { |
There was a problem hiding this comment.
Grading change: "401 without a DPoP challenge" needs two tiers, keyed off the metadata this scenario already fetches.
SEP-1932's new advertisement section (modelcontextprotocol/ext-auth#33) grades the challenge differently depending on the server's declared posture:
-
A server that merely supports DPoP —
dpop_signing_alg_values_supportedpublished,dpop_bound_access_tokens_requiredabsent orfalse(bearer tokens accepted per the RFC 9728 default) — is under a SHOULD: "an MCP server supporting this extension SHOULD include aDPoPchallenge…". For it, a textbook RFC 6750 Bearer-only challenge is conformant (and discovery still works —resourceMetadataParam()in this function readsresource_metadatafrom the Bearer challenge, since RFC 9728 §5.1's parameter is scheme-independent). Missing DPoP scheme → WARNING. -
A server that requires DPoP —
dpop_bound_access_tokens_required: true— is under a MUST: "An MCP server that setsdpop_bound_access_tokens_requiredtotrue… MUST include aDPoPchallenge in everyWWW-Authenticateresponse it sends." For it, a Bearer-only challenge invites authentication it will always reject. Missing DPoP scheme → FAILURE.
As written, this arm hard-FAILs both postures, which over-grades the first (a conformant server fails) and under-specifies the second (the FAILURE is right but for a reason the message doesn't state). Since the PRM document is fetched later in run() anyway, the simplest shape is probably: record the challenge observation here, grade it after discovery — WARNING when the metadata is silent/false/unreachable (the MUST tier can't be established), FAILURE with a "required=true but no DPoP challenge" message when the metadata says true. The catch-path untestableCheck for this check (~line 887) should likewise default to WARNING severity per the #248 keyword convention, since the unconditional obligation is only a SHOULD.
Note the SEP also expressly permits other challenges alongside the DPoP one (e.g. the RFC 9449 §7.2 Bearer error carrier) — so the predicate should stay "DPoP challenge present among the challenges", never "DPoP only".
Generated with the aid of Claude (Fable 5).
| CONSISTENCY_NAME, | ||
| CONSISTENCY_DESC, | ||
| 'FAILURE', | ||
| 'server requires DPoP but does not advertise it', |
There was a problem hiding this comment.
This arm can assert something the run never observed. saysDpopRequired is initialized to false (line 859) and only becomes true when a metadata document was successfully fetched and read — so "we read the metadata and it doesn't advertise DPoP-required" and "we never managed to read any metadata" arrive here looking identical. In the second case this check still returns a definitive FAILURE saying the server "does not advertise it", and the details carry dpop_bound_access_tokens_required: false — a value no document ever supplied.
Reproduced against a live fixture whose PRM endpoint 404s while the backend requires DPoP: the run reports PrmDpop = "Not testable: … unreachable" and PrmConsistency = FAILURE "server requires DPoP but does not advertise it" — two red marks from one root cause, with the second claiming knowledge of a document that was never retrieved. An operator reading that report would hunt for a metadata field problem when their actual issue is that the metadata endpoint is down.
Fix (the document-was-read case is correct and unchanged): track three states instead of two — required / not required / no document, e.g. boolean | null — and when no document was read, report this check via untestableCheck ("cannot evaluate advertisement consistency: protected resource metadata was unreachable") instead of this arm.
Generated with the aid of Claude (Fable 5).
| ]; | ||
| for (const candidate of candidates) { | ||
| const doc = await fetchJsonDocument(candidate.url); | ||
| if (doc) return { doc, source: candidate.source, url: candidate.url }; |
There was a problem hiding this comment.
Missing gate: before grading the DPoP fields, check that the document is one a conformant client would be allowed to use.
RFC 9728 §3.3 requires every metadata consumer to verify the document's resource field identifies the resource it asked about, and on mismatch "the data contained in the response MUST NOT be used" (§7.3 explains the impersonation attack this prevents). A server whose metadata fails that gate is serving metadata that's unusable to conformant clients — however correct the DPoP fields inside it are.
This function skips the gate: reproduced with a fixture serving otherwise-valid metadata with resource: "https://attacker.example/mcp" — the document is consumed as-is, PrmDpop grades SUCCESS, and the consistency check trusts its dpop_bound_access_tokens_required. So the check currently asserts "the right fields exist in some JSON at the right URL" rather than what we actually want: "a conformant client following this server's discovery would obtain usable metadata and find correct DPoP advertisement in it."
Fix is small: at both successful-fetch returns (line 494 and here), compare doc.resource to this function's resourceUrl (per §3.3 that's the right comparand on both discovery paths), and on mismatch return a { reason } naming the two values instead of the document. The existing reason-handling then does the rest: PrmDpop reports the mismatch, and the consistency check has no disqualified value to trust.
Generated with the aid of Claude (Fable 5).
| res: Response | ||
| ): ConformanceCheck { | ||
| const accepted = isAccepted(res.statusCode); | ||
| const rejected = res.statusCode === 401; |
There was a problem hiding this comment.
The SEP's MUST says "reject", not "401" — so a server that refuses the unbound Bearer token with 403 or 400 satisfies the requirement but currently fails the says-required arm.
Suggested split, since this shared predicate serves two arms with different jobs: for the says-required arm (grading the MUST from modelcontextprotocol/ext-auth#33), count any non-2xx as rejection; for the silent arm (inferring unadvertised DPoP enforcement), keep 401-only and add an isNonceChallenge guard like rejectionCheck has — a use_dpop_nonce 401 says nothing about the Bearer token, and an accusation needs stronger evidence than a MUST-compliance pass.
Generated with the aid of Claude (Fable 5).
| } catch { | ||
| return undefined; | ||
| } | ||
| if (res.statusCode < 200 || res.statusCode >= 300) return undefined; |
There was a problem hiding this comment.
A healthy server whose metadata URL answers with an ordinary redirect (e.g. the trailing-slash 301/308 that nginx/Express add by default) is reported "metadata unreachable" and fails the discovery checks — undici's request doesn't follow 3xx, so the redirect lands in this non-2xx branch as undefined, even though any real client resolves it in one hop. Fix: allow a redirect hop or two (maxRedirections).
Two smaller gaps in the same function, both against RFC 9728 §3.2 ("A successful response MUST use the 200 OK HTTP status code and return a JSON object using the application/json content type"): the status gate accepts any 2xx where the spec requires exactly 200, and the Content-Type header is never checked, so a response labeled text/html whose body happens to parse as JSON would be accepted and graded. Both are one-line guards, worth adding while this function is being touched for the redirect fix.
Generated with the aid of Claude (Fable 5).
| ): ConformanceCheck { | ||
| const accepted = isAccepted(res.statusCode); | ||
| const rejected = res.statusCode === 401; | ||
| const details = { |
There was a problem hiding this comment.
Nit: details carry only the status code — adding the response's WWW-Authenticate value (or its parsed error code) would make this check's verdicts auditable from the report, same as the other rejection checks.
Generated with the aid of Claude (Fable 5).
| # Quoted from RFC 9449 §7.1. Missing `algs` is WARNING (SHOULD). | ||
| # No DPoP scheme is a failure: SEP-1932 requires the server to advertise DPoP. | ||
| - check: sep-1932-server-advertises-dpop | ||
| text: 'An `algs` parameter SHOULD be included to signal to the client the JWS algorithms that are acceptable for the DPoP proof JWT. The value of the parameter is a space-delimited list of JWS `alg` (Algorithm) header values ([RFC7515], Section 4.1.1).' | ||
| # Quoted from RFC 9728 §2, where both fields are OPTIONAL. SEP-1932 requires | ||
| # them, so a missing field is a failure. A listed `none` or `HS*` value is | ||
| # a failure per RFC 9449 §11.6, matching sep-1932-as-no-none-alg. | ||
| - check: sep-1932-server-prm-dpop | ||
| text: '`dpop_signing_alg_values_supported`: OPTIONAL. JSON array containing a list of the JWS `alg` values (from the "JSON Web Signature and Encryption Algorithms" registry [IANA.JOSE]) supported by the resource server for validating Demonstrating Proof of Possession (DPoP) proof JWTs [RFC9449]. / `dpop_bound_access_tokens_required`: OPTIONAL. Boolean value specifying whether the protected resource always requires the use of DPoP-bound access tokens [RFC9449]. If omitted, the default value is false.' | ||
| # The boolean's meaning is the RFC 9728 §2 sentence above. The SEP text does not contain it yet. | ||
| - check: sep-1932-server-prm-consistency | ||
| text: '`dpop_bound_access_tokens_required`: OPTIONAL. Boolean value specifying whether the protected resource always requires the use of DPoP-bound access tokens [RFC9449]. If omitted, the default value is false.' |
There was a problem hiding this comment.
These three rows quote RFC 9449/9728 text and attribute it to SEP-1932 — including sentences whose own keywords (SHOULD, OPTIONAL) contradict the FAILURE grading the checks apply — because the SEP sentences didn't exist when this was written. They exist now: SEP-1932's "MCP Server DPoP Advertisement" section (modelcontextprotocol/ext-auth#33) has exact sentences for all three checks, so per this file's verbatim-from-SEP convention the rows should quote those (and the comment below saying "The SEP text does not contain it yet" is no longer true):
| # Quoted from RFC 9449 §7.1. Missing `algs` is WARNING (SHOULD). | |
| # No DPoP scheme is a failure: SEP-1932 requires the server to advertise DPoP. | |
| - check: sep-1932-server-advertises-dpop | |
| text: 'An `algs` parameter SHOULD be included to signal to the client the JWS algorithms that are acceptable for the DPoP proof JWT. The value of the parameter is a space-delimited list of JWS `alg` (Algorithm) header values ([RFC7515], Section 4.1.1).' | |
| # Quoted from RFC 9728 §2, where both fields are OPTIONAL. SEP-1932 requires | |
| # them, so a missing field is a failure. A listed `none` or `HS*` value is | |
| # a failure per RFC 9449 §11.6, matching sep-1932-as-no-none-alg. | |
| - check: sep-1932-server-prm-dpop | |
| text: '`dpop_signing_alg_values_supported`: OPTIONAL. JSON array containing a list of the JWS `alg` values (from the "JSON Web Signature and Encryption Algorithms" registry [IANA.JOSE]) supported by the resource server for validating Demonstrating Proof of Possession (DPoP) proof JWTs [RFC9449]. / `dpop_bound_access_tokens_required`: OPTIONAL. Boolean value specifying whether the protected resource always requires the use of DPoP-bound access tokens [RFC9449]. If omitted, the default value is false.' | |
| # The boolean's meaning is the RFC 9728 §2 sentence above. The SEP text does not contain it yet. | |
| - check: sep-1932-server-prm-consistency | |
| text: '`dpop_bound_access_tokens_required`: OPTIONAL. Boolean value specifying whether the protected resource always requires the use of DPoP-bound access tokens [RFC9449]. If omitted, the default value is false.' | |
| # Quoted from the SEP's "MCP Server DPoP Advertisement" section. Challenge | |
| # advertisement is two-tier: SHOULD for any server supporting the extension, | |
| # MUST when dpop_bound_access_tokens_required is true. | |
| - check: sep-1932-server-advertises-dpop | |
| text: 'When responding to an unauthenticated or failed request with HTTP 401, an MCP server supporting this extension SHOULD include a `DPoP` challenge in the `WWW-Authenticate` header, and that challenge SHOULD include the `algs` parameter signaling the JWS algorithms acceptable for DPoP proof JWTs, as described in RFC 9449 Section 7.1. / An MCP server that sets `dpop_bound_access_tokens_required` to `true` in its Protected Resource Metadata MUST include a `DPoP` challenge in every `WWW-Authenticate` response it sends.' | |
| - check: sep-1932-server-prm-dpop | |
| text: 'MCP servers supporting this extension MUST include the `dpop_signing_alg_values_supported` field in their Protected Resource Metadata (RFC 9728 Section 2). This field MUST contain a non-empty JSON array of registered JWS algorithm values (from the IANA JSON Web Signature and Encryption Algorithms registry) that the server accepts for DPoP proof JWTs. Only asymmetric digital signature algorithms are permitted. The `none` algorithm and symmetric (MAC) algorithms MUST NOT be included.' | |
| # Absence of the boolean is valid (RFC 9728 default) — see the grading | |
| # comments on dpop.ts. | |
| - check: sep-1932-server-prm-consistency | |
| text: 'An MCP server that requires DPoP-bound access tokens (that is, one that does not accept bearer tokens) MUST set the `dpop_bound_access_tokens_required` field to `true` in its Protected Resource Metadata. Conversely, an MCP server that sets `dpop_bound_access_tokens_required` to `true` MUST reject requests that do not present a DPoP-bound access token with a valid DPoP proof. When the field is absent, the default behavior defined in RFC 9728 applies and DPoP-bound access tokens are not required.' |
One dependency to be aware of: these quotes match the current text of ext-auth#33; if its wording shifts in auth-wg review, this file should be re-synced to the merged version.
Generated with the aid of Claude (Fable 5).
|
Big-picture: the test fixtures (the fake servers the suite runs against) have no way to trigger the situations described in the other review comments — so the grading fixes those comments ask for can't be tested, and could silently regress later. This comment collects the fixture modes and test cases needed to close that gap. Fixture modes (sep-1932-compliant-server.ts):
Test arms with no coverage today, worth pinning while in here:
Each new mode should flip exactly its targeted check and leave the rest green, per the suite's one-defect convention. Generated with the aid of Claude (Fable 5). |
PieterKas
left a comment
There was a problem hiding this comment.
Thorough review done (adversarial pass with the aid of Claude, Fable 5 — findings verified empirically against live fixtures). The architecture is good: discovery probes are cleanly separated, the consistency probe reuses the positive token minus cnf so rejections are attributable, and the suites run green.
Requesting changes for two themes, detailed in 9 comments:
- Grading drift against the proposed SEP text: the "MCP Server DPoP Advertisement" section proposed in modelcontextprotocol/ext-auth#33 grades these behaviors differently than this PR assumes — absent
dpop_bound_access_tokens_requiredis valid (line 591), the challenge is two-tier SHOULD/MUST (line 524), "reject" isn't "401" (line 622), and the yaml rows should quote the SEP sentences (yaml comment, with suggestion). - Verdicts without evidence: the consistency check asserts metadata contents when no document was read (line 666), and documents are consumed without the RFC 9728 §3.3 resource check (line 507); plus fetcher robustness (line 456) and a details nit (line 623).
The Conversation-tab comment collects the fixture modes and tests needed to pin all of the above. Sequencing: ext-auth#33 needs to land first so the yaml quotes track its final wording; happy to re-review quickly after.
I completely agree - I was too eager here and didn't mean to imply that all servers MUST advertise their DPoP stance. I meant it in the same way you described above: "if the MCP server supports DPoP it MUST advertise that support in protected resource metadata (PRM)" I will take a pass at updating this PR with your feedback, thanks! |
Stacked on #528 -> #527 -> #395 (
PieterKas/conformance:dpop-server, headdd9fd77). This branch contains those commits plus5d213b9. Rebase ontomainonce #528, #527, and #395 merge.The new commit (
5d213b9) is the only one to review.What each check asserts
#395 checks that a malformed proof is rejected. It does not check how a client that has never met the server learns that DPoP is required, or that the advertisement matches what the server enforces.
This assumes SEP-1932 will require the server to advertise DPoP - which @PieterKas proposed (and I agree with!). With that assumption, a missing DPoP challenge or a missing Protected Resource Metadata field is a failure. Missing
algsstays a WARNING because RFC 9449 §7.1 says SHOULD.sep-1932-server-advertises-dpopRFC 9449 §7.1, graded SHOULD for
algsfrom "Analgsparameter SHOULD be included to signal to the client the JWS algorithms that are acceptable for the DPoP proof JWT. The value of the parameter is a space-delimited list of JWSalg(Algorithm) header values ([RFC7515], Section 4.1.1)."An unauthenticated POST, no
Authorizationand noDPoPheader.algs: SUCCESSalgsmissing: WARNINGThe
resource_metadataparameter is captured from the DPoP challenge, or from a sibling challenge when the DPoP one does not carry it (RFC 9728 §5.1).sep-1932-server-prm-dpopRFC 9728 §2 marks both fields OPTIONAL. Assuming SEP-1932 requires them, absence is a failure.
Fetched from
resource_metadatawhen the challenge has it, otherwise the path-based well-known URL, otherwise the root one. Same order as the client metadata-discovery scenarios. Aresource_metadataURL that does not return a document is not replaced by a different well-known document.dpop_signing_alg_values_supportedpresent, non-empty, and asymmetric only: the field half is satisfied.noneorHS*is FAILURE, same rule assep-1932-as-no-none-alg(RFC 9449 §11.6). Absent or empty: FAILUREdpop_bound_access_tokens_requiredpresent as a boolean: the field half is satisfied. Absent: FAILUREsep-1932-server-prm-consistencyAn unbound token (
omitCnf), presented asBearerwith no DPoP header. Gated on the positive proof.Testing
none, and missing-algsfixturesMade with Cursor