Repository navigation
feat(sep-1932): Client check for dpop_jkt - #523
nbarbettini wants to merge 2 commits into
Conversation
…pop_jkt SEP-1932 adopts RFC 9449 §10, so a client binds its authorization code to its DPoP key and the test authorization server rejects a mismatched thumbprint. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
commit: |
|
Thanks @nbarbettini for proposing this. I think it's a good idea to have dpop_jkt validation in the conformance suite for the cases where clients implement it. I know I originally agreed it makes sense to make this mandatory, but on reflection I think that may not be the right call, given the other features already included in MCP that provide a high degree of mitigation against authorization code theft/replay. MCP already mandates PKCE (https://modelcontextprotocol.io/specification/draft/basic/authorization/security-considerations), and RFC 9449 §10 itself notes that PKCE is the recommended countermeasure for authorization code injection, with dpop_jkt providing "similar protections" (https://datatracker.ietf.org/doc/html/rfc9449#section-10). The additional protection dpop_jkt gives over PKCE only applies when a fresh DPoP key is used for every authorization request, and I don't expect that to be the norm. In practice a key will live for the duration of a session or client instance, and perhaps longer, depending on what key storage a client has available. I also took a look at FAPI 2.0, which allows DPoP as a sender-constraining mechanism (alongside MTLS). FAPI never mentions dpop_jkt. It mandates PKCE (along with PAR), which mitigates the authorization code theft/injection risks (https://openid.net/specs/fapi-security-profile-2_0-final.html#name-authorization-endpoint-flow). Consequently I don't think we should mandate binding the authorization code to the DPoP key, but keep the checks for clients that do send dpop_jkt (where a mismatched key is a hard failure, since the AS MUST reject it per §10 of RFC 9449), and emit nothing when a client doesn't use the parameter. @bc-pi I would love to get your perspective on this as well. |
Adds a SHOULD for MCP clients to bind the authorization code to their DPoP key per RFC 9449 Section 10. RFC 9449 makes this OPTIONAL; this profile recommends it as defense in depth alongside MCP’s mandatory PKCE. Surfaced by the conformance work in modelcontextprotocol/conformance#523; kept separate from modelcontextprotocol#33 so the two can be discussed and merged independently.
|
Update on my earlier comment: after further discussion (including offline input from Brian Campbell, who made the point that optionality itself carries implementation complexity), SEP-1932 will recommend (SHOULD) binding the authorization code to the DPoP key, with RFC 9449 Section 10 remaining definitive for the mechanism. The proposed text is in modelcontextprotocol/ext-auth#35, kept separate from the other SEP changes so it can be discussed on its own. That lands exactly on this PR's existing approach, so please disregard my earlier suggestion to emit nothing on omission. The remaining asks are in the review comments. |
| asNonceHonored: boolean; | ||
| /** | ||
| * `dpop_jkt` from the authorization request, if any (RFC 9449 §10). | ||
| * Written only on an authorization_code exchange after a valid proof, |
There was a problem hiding this comment.
This comment doesn't accurately describe the implementation: dpopJktSent is written before proof validation (line 541, confirmed by a live probe showing it populated while validProof was false), not "after a valid proof", and the sink doesn't have recordTokenRequestProof's sticky semantics (see the comment at line 655). One concrete consequence of the early write: dpopJktSent and dpopJktMatched can be set by two different exchanges, so the check's emitted details can pair values that never occurred together. Whichever way the sink is reworked, it would be good to update this comment to accurately reflect the final behavior — these fields are the likely foundation for refresh-token observation in #524, so an accurate contract here pays off immediately.
Generated with the aid of Claude (Fable 5).
| if (grantType === 'authorization_code' && dpopTokenRequestObs) { | ||
| dpopTokenRequestObs.dpopJktMatched = | ||
| storedDpopJkt !== undefined && storedDpopJkt === result.jkt; | ||
| } |
There was a problem hiding this comment.
A client the AS caught violating the key binding can end the run with SUCCESS.
dpopJktSent/dpopJktMatched are last-write-wins: a client that declares key A and proves with key B gets the 400 and obs.dpopJktMatched=false — then restarts the flow with matching keys and the sink overwrites to true, so sep-1932-client-dpop-jkt reports SUCCESS for a caught mismatch (reproduced empirically; it works even re-using the same authorization code, since codes are never consumed). The adjacent recordTokenRequestProof is explicitly sticky-failure to prevent exactly this class. Note the sticky rule needs to key on observed mismatch rather than on false — the current expression also yields false for a jkt-less exchange, and that case shouldn't poison a later correct flow:
| if (grantType === 'authorization_code' && dpopTokenRequestObs) { | |
| dpopTokenRequestObs.dpopJktMatched = | |
| storedDpopJkt !== undefined && storedDpopJkt === result.jkt; | |
| } | |
| if (grantType === 'authorization_code' && dpopTokenRequestObs) { | |
| // Sticky on mismatch (like recordTokenRequestProof): once a | |
| // mismatch is observed, a later clean exchange cannot erase it. | |
| // A jkt-less exchange leaves the observation unchanged. | |
| if (storedDpopJkt !== undefined && storedDpopJkt !== result.jkt) { | |
| dpopTokenRequestObs.dpopJktMatched = false; | |
| } else if ( | |
| storedDpopJkt !== undefined && | |
| dpopTokenRequestObs.dpopJktMatched !== false | |
| ) { | |
| dpopTokenRequestObs.dpopJktMatched = true; | |
| } | |
| } |
Generated with the aid of Claude (Fable 5).
| // No proof ⇒ DPoP is not being exercised here; fall through to a Bearer | ||
| // token. (Requiring a proof is a per-client `dpop_bound_access_tokens` | ||
| // registration policy — RFC 9449 §5.2 — not a global AS behaviour.) The | ||
| // client-side check still records that the client failed to ask for a | ||
| // bound token. | ||
| if (proof) { |
There was a problem hiding this comment.
A jkt-bound authorization code exchanged with no DPoP proof at all is issued a Bearer token (verified empirically) — which means the binding only constrains honest clients: anyone the key check would catch can simply opt out of presenting a key.
As an attack chain: a stolen jkt-bound code, redeemed proof-less, yields not just a token but an unbound Bearer token — a downgrade on top of the bypass. And it lands exactly in the scenario dpop_jkt exists for: an attacker strong enough to defeat PKCE (code + verifier stolen) is supposed to be stopped by the one thing they can't exfiltrate, the DPoP private key — this fall-through lets them decline to present one instead.
The general no-proof fall-through in this comment is right (requiring proofs is per-client RFC 9449 §5.2 policy), but §10 creates the one exception: a code bound via dpop_jkt can only be redeemed with a proof of the matching key, and absence can never match. Real §10-implementing ASs reject this; the fixture should too, with a sticky observation:
| // RFC 9449 §10: a code bound via dpop_jkt can only be redeemed with a | |
| // proof of the matching key — absence of a proof can never match. | |
| if (!proof && authorizationCodeState?.dpopJkt !== undefined) { | |
| if (grantType === 'authorization_code' && dpopTokenRequestObs) { | |
| dpopTokenRequestObs.dpopJktMatched = false; | |
| } | |
| res.status(400).json({ | |
| error: 'invalid_grant', | |
| error_description: | |
| 'authorization code is bound to a DPoP key (dpop_jkt) but no DPoP proof was presented' | |
| }); | |
| return; | |
| } | |
| // No proof ⇒ DPoP is not being exercised here; fall through to a Bearer | |
| // token. (Requiring a proof is a per-client `dpop_bound_access_tokens` | |
| // registration policy — RFC 9449 §5.2 — not a global AS behaviour.) The | |
| // client-side check still records that the client failed to ask for a | |
| // bound token. | |
| if (proof) { |
(invalid_grant to match the existing mismatch rejection's error-code choice; a jkt-bound code + proof-less exchange test would pin it — the no-token-proof fixture client with sendDpopJkt enabled is exactly that case.)
Generated with the aid of Claude (Fable 5).
| const authorizationCode = req.body.code as string | undefined; | ||
| const authorizationCodeState = authorizationCode | ||
| ? authorizationCodes.get(authorizationCode) | ||
| : undefined; |
There was a problem hiding this comment.
The token endpoint mints tokens for authorization codes this AS never issued — verified empirically with code="garbage-code" plus a valid proof, which returned 200 with a DPoP-bound access token.
The mechanism: the lookup here silently yields undefined for an unknown (or absent) code, and every code-dependent verification — was this code issued? does the PKCE verifier match? does the proof key match the stored dpop_jkt? — simply doesn't run, because there's nothing to compare against. The DPoP proof itself validates fine (proof validation is independent of the code), and the issued token gets bound to whatever key the proof carried — the stored dpop_jkt promise never enters the picture, since the code it's stored under was never presented.
This makes it the sibling of the no-proof hole above: the mismatch check compares a stored value against a presented one, and each hole removes one operand — no proof removes the presented side, a bogus code removes the stored side. Both let a wrong-key client evade the FAILURE. A bogus-code exchange also overwrites dpopJktSent to undefined, so a client that legitimately sent and matched earlier can end the run reported as "did not send dpop_jkt".
Fix: for grant_type=authorization_code, reject an unknown or missing code with 400 invalid_grant before any observation writes (the token-request check can still record the attempt; the dpopJkt* sinks must not be touched by an exchange that never mapped to an issued code). The lenient lookup predates this PR — a real AS rejects unknown codes as table stakes — but this PR is what gives the leniency teeth, as the first feature storing per-code state the exchange is supposed to verify.
Generated with the aid of Claude (Fable 5).
| } | ||
| if ( | ||
| grantType === 'authorization_code' && | ||
| storedDpopJkt !== undefined && |
There was a problem hiding this comment.
One client bug produces two check failures here, and the second one blames the client for something the test AS caused.
In the auth/dpop-nonce posture, a client with a wrong dpop_jkt gets this mismatch rejection before the AS ever issues its nonce challenge. No challenge → no retry → sep-1932-client-as-nonce fails with "Client never presented a valid proof that was answered with a use_dpop_nonce challenge" — but the client presented a valid proof; the AS rejected the exchange for the key mismatch and never challenged. The report then points at a nonce bug that doesn't exist.
Fix: order the checks possession → nonce → binding. Proof validation is self-contained (the proof verifies against its own embedded key), so it always runs first and the valid-proof observation is recorded as today; the use_dpop_nonce challenge comes next; and the dpop_jkt comparison moves after the nonce step, so it only runs against a nonce-current exchange. A wrong-jkt client then completes the nonce dance (nonce checks pass) and fails only the jkt check — one defect, one failure. A test running the wrong-jkt fixture against auth/dpop-nonce, asserting only sep-1932-client-dpop-jkt flips, would pin it (today that fixture only runs against auth/dpop).
Generated with the aid of Claude (Fable 5).
| let storedCodeChallenge: string | undefined; | ||
| // Authorization-request state is bound to the code it produced. Keeping this | ||
| // per-code prevents overlapping flows from overwriting PKCE or DPoP binding. | ||
| const authorizationCodes = new Map<string, StoredAuthorizationCode>(); |
There was a problem hiding this comment.
Authorization codes in this Map live forever and can be exchanged any number of times — a slow leak, and the enabler for the retry-masking issue flagged at line 654 (a mismatch verdict can be retried into SUCCESS using the same authorization code).
One subtlety before anyone reaches for single-use authorization codes: reuse is currently expected in the test suite for nonces. The §8 nonce flow presents the same authorization code twice (challenge, then retry) — so burning the code on first presentation would break the auth/dpop-nonce posture. The fix that threads the needle is evicting only on successful issuance:
present authorization code ──► 400 use_dpop_nonce (code survives — not a success)
present authorization code ──► 200 + token (evict now — flow complete)
present authorization code ──► 400 invalid_grant (replay after success: blocked)
i.e. a single authorizationCodes.delete(code) on the 2xx path: the nonce retry is untouched (the challenge is a rejection, so the authorization code survives it), the replay-after-success path closes, and the Map stops growing unboundedly. Worth also noting in this comment that real ASs differ here — some burn the authorization code on the nonce challenge itself (the behavior the AS scenario in #525 has to tolerate) — so the fixture's choice is deliberate: lenient enough for the nonce retry, strict enough to block replay.
Generated with the aid of Claude (Fable 5).
| * SEP-1932 requires the client to bind the authorization code to its DPoP key | ||
| * with `dpop_jkt` (RFC 9449 §10). Omission is a SHOULD miss. |
There was a problem hiding this comment.
Good news first: per the conversation thread, the severity below is staying exactly as you built it — SEP-1932 is adopting a SHOULD for authorization code binding (spec text in modelcontextprotocol/ext-auth#35), SHOULD maps to WARNING under the suite's convention, so DPOP_JKT_NOT_SENT_STATUS is correct as-is.
What needs fixing is one word of provenance: "requires" overstates the keyword, and the comment contradicts itself — the next sentence already calls omission "a SHOULD miss." Since the keyword was genuinely contested in the thread, the paper trail should show this check grades a SHOULD, not a MUST.
| * SEP-1932 requires the client to bind the authorization code to its DPoP key | |
| * with `dpop_jkt` (RFC 9449 §10). Omission is a SHOULD miss. | |
| * SEP-1932 recommends (SHOULD) that the client bind the authorization code to | |
| * its DPoP key with `dpop_jkt` — OPTIONAL in RFC 9449 §10, tightened to SHOULD | |
| * by the MCP profile. Omission is a SHOULD miss. |
Generated with the aid of Claude (Fable 5).
| # Proposed SEP-1932 client guidance; RFC 9449 §10 currently makes this optional. | ||
| - check: sep-1932-client-dpop-jkt | ||
| text: 'Clients using the authorization code grant SHOULD bind the authorization code to their DPoP key using the dpop_jkt authorization request parameter.' | ||
| url: https://github.com/modelcontextprotocol/modelcontextprotocol/pull/1932 |
There was a problem hiding this comment.
The quoted sentence doesn't exist in any spec — the yaml convention is that text: quotes the governing spec verbatim, and this row quotes a sentence that appears in neither RFC 9449 nor SEP-1932 (your # Proposed comment rightly flags exactly this). The url: also points at the SEP discussion PR rather than at spec text.
Real spec text now exists: modelcontextprotocol/ext-auth#35 adds an "Authorization Code Binding" section with the SHOULD. Once it lands, this row can quote it verbatim and drop both the provisional comment and the url override (the file's spec_url already points at the branch #35 merges into):
| # Proposed SEP-1932 client guidance; RFC 9449 §10 currently makes this optional. | |
| - check: sep-1932-client-dpop-jkt | |
| text: 'Clients using the authorization code grant SHOULD bind the authorization code to their DPoP key using the dpop_jkt authorization request parameter.' | |
| url: https://github.com/modelcontextprotocol/modelcontextprotocol/pull/1932 | |
| - check: sep-1932-client-dpop-jkt | |
| text: 'MCP clients using the authorization code grant SHOULD bind the authorization code to their DPoP key, as described in RFC 9449 Section 10' |
Sequencing note: this suggestion is committable only after #35 merges — until then the row would quote text that isn't in the spec yet, which is the same problem in a different hat.
Generated with the aid of Claude (Fable 5).
| expectedSuccessSlugs: ['sep-1932-client-token-request-proof'] | ||
| expectedSuccessSlugs: [ | ||
| 'sep-1932-client-token-request-proof', | ||
| 'sep-1932-client-dpop-jkt' |
There was a problem hiding this comment.
This reconciliation (adding the always-emitted dpop_jkt check to this test's expectations, and the rs-nonce test's below) was done in two of the three places that need it. The miss is the pre-existing "client never requests a sender-constrained token" test at line 433, outside this diff: with no valid token-endpoint proof, the new check takes its not-testable branch and emits WARNING, which that test's expectedFailureSlugs doesn't list. It still passes only because the helper's "verify that only the expected checks failed" assertion is one-directional (expect.arrayContaining checks expected ⊆ actual), so the extra WARNING slips through unasserted. The other two negative tests are fine — their clients make valid proofed token requests, so the check emits SUCCESS there.
Can't attach a committable suggestion outside the diff, so here's the edit for lines 434–442:
// No token-endpoint proof → the AS issues an unbound Bearer token, so both
// the token-request check and (as a consequence of the unbound token) the
// resource binding check fail. The dpop_jkt check degrades to its
// not-testable WARNING: without a valid token-endpoint proof there is no
// key to compare against.
const runner = new InlineClientRunner(dpopNoTokenProofClient);
await runClientAgainstScenario(runner, 'auth/dpop', {
expectedFailureSlugs: [
'sep-1932-client-token-request-proof',
'sep-1932-client-fresh-proof',
'sep-1932-client-dpop-jkt'
]
});Verified by running this file against the PR head with that edit applied: 70/70 pass, and a probe confirms the check lands as WARNING with the "Not testable" message in this run (and SUCCESS in the two negative tests above).
(The one-directional helper assertion itself predates this PR — mentioned only as the reason this wasn't caught, not as something to fix here.)
Generated with the aid of Claude (Fable 5).
| response_type: 'code', | ||
| client_id: 'test', | ||
| redirect_uri: REDIRECT, | ||
| code_challenge: 'x', |
There was a problem hiding this comment.
These tests run the AS through a deliberately broken PKCE exchange. 'x'/'x' is not a valid S256 pair ('x' is also well under RFC 7636 §4.1's 43-char verifier minimum), so every run of this file records a pkce-verifier-matches-challenge FAILURE into the checks array as a side effect (verified with a probe). The tests still pass because this test AS observes PKCE rather than enforcing it — but that makes the fixture silently dependent on non-enforcement, and it means the §10 behavior under test is being exercised in a run that's simultaneously failing an unrelated conformance check.
A real pair costs a few lines (both helpers pick it up; can't put this in one suggestion block since it touches the imports, a new constant, and line 64):
import { createHash } from 'node:crypto';
// A real S256 pair (RFC 7636 §4.1: verifier 43-128 chars), so these tests
// don't depend on the test AS tolerating an invalid PKCE exchange.
const CODE_VERIFIER = 'dpop-jkt-tests-code-verifier-0123456789-0123456789a';
const CODE_CHALLENGE = createHash('sha256')
.update(CODE_VERIFIER)
.digest('base64url');…then code_challenge: CODE_CHALLENGE here and code_verifier: CODE_VERIFIER at line 64. Verified against the PR head: 2/2 pass with this change.
Generated with the aid of Claude (Fable 5).
| let untestable = false; | ||
| if (!this.tokenReqObs.recorded || !this.tokenReqObs.validProof) { | ||
| status = 'WARNING'; | ||
| untestable = true; | ||
| errorMessage = notTestable( | ||
| 'the client did not present a valid token-endpoint DPoP proof, so dpop_jkt key agreement could not be evaluated' | ||
| ); | ||
| } else if (sent !== undefined && matched) { | ||
| status = 'SUCCESS'; | ||
| } else if (sent !== undefined) { | ||
| status = 'FAILURE'; | ||
| errorMessage = | ||
| 'Client sent dpop_jkt but it does not match the DPoP proof key used at the token endpoint'; | ||
| } else { | ||
| status = DPOP_JKT_NOT_SENT_STATUS; | ||
| errorMessage = | ||
| 'Client did not send dpop_jkt on the authorization request to bind the authorization code to its DPoP key'; | ||
| } | ||
| return this.build('sep-1932-client-dpop-jkt', status, { | ||
| errorMessage, | ||
| details: { | ||
| dpopJktSent: sent, | ||
| dpopJktMatched: matched, | ||
| ...(untestable ? { untestable: true } : {}) |
There was a problem hiding this comment.
This untestable emission drops the reason key that every other untestable emitter includes. The #248 convention shape is details: { untestable: true, reason } — that's what the canonical untestableCheck() helper emits, and the one other hand-rolled emitter (tools.ts's list-order check) follows it too. Here the reason text exists only inside errorMessage, so anything that consumes the structured details (as the existing emitters allow) gets the flag without the why. Hand-rolling the emission is right — the helper can't carry the dpopJktSent/dpopJktMatched details — it's just the one missing key:
| let untestable = false; | |
| if (!this.tokenReqObs.recorded || !this.tokenReqObs.validProof) { | |
| status = 'WARNING'; | |
| untestable = true; | |
| errorMessage = notTestable( | |
| 'the client did not present a valid token-endpoint DPoP proof, so dpop_jkt key agreement could not be evaluated' | |
| ); | |
| } else if (sent !== undefined && matched) { | |
| status = 'SUCCESS'; | |
| } else if (sent !== undefined) { | |
| status = 'FAILURE'; | |
| errorMessage = | |
| 'Client sent dpop_jkt but it does not match the DPoP proof key used at the token endpoint'; | |
| } else { | |
| status = DPOP_JKT_NOT_SENT_STATUS; | |
| errorMessage = | |
| 'Client did not send dpop_jkt on the authorization request to bind the authorization code to its DPoP key'; | |
| } | |
| return this.build('sep-1932-client-dpop-jkt', status, { | |
| errorMessage, | |
| details: { | |
| dpopJktSent: sent, | |
| dpopJktMatched: matched, | |
| ...(untestable ? { untestable: true } : {}) | |
| let untestableReason: string | undefined; | |
| if (!this.tokenReqObs.recorded || !this.tokenReqObs.validProof) { | |
| status = 'WARNING'; | |
| untestableReason = | |
| 'the client did not present a valid token-endpoint DPoP proof, so dpop_jkt key agreement could not be evaluated'; | |
| errorMessage = notTestable(untestableReason); | |
| } else if (sent !== undefined && matched) { | |
| status = 'SUCCESS'; | |
| } else if (sent !== undefined) { | |
| status = 'FAILURE'; | |
| errorMessage = | |
| 'Client sent dpop_jkt but it does not match the DPoP proof key used at the token endpoint'; | |
| } else { | |
| status = DPOP_JKT_NOT_SENT_STATUS; | |
| errorMessage = | |
| 'Client did not send dpop_jkt on the authorization request to bind the authorization code to its DPoP key'; | |
| } | |
| return this.build('sep-1932-client-dpop-jkt', status, { | |
| errorMessage, | |
| details: { | |
| dpopJktSent: sent, | |
| dpopJktMatched: matched, | |
| ...(untestableReason | |
| ? { untestable: true, reason: untestableReason } | |
| : {}) |
Verified against the PR head with this change applied: tsc --noEmit clean, prettier clean, index.test.ts 70/70.
Generated with the aid of Claude (Fable 5).
PieterKas
left a comment
PieterKas
left a comment
There was a problem hiding this comment.
Thanks Nick — this is a solid addition, and the three-branch coverage of the new check (SUCCESS/WARNING/FAILURE with dedicated broken clients) is exactly the pattern we want. Requesting changes for the correctness set below; the grading items are sequenced on the spec PR and the nits are non-blocking.
Status question first: per the conversation thread, SEP-1932 is adopting authorization code binding as a SHOULD (spec text in modelcontextprotocol/ext-auth#35), which means this PR's shape is right — WARNING on omission, FAILURE on mismatch. No re-grading needed.
The inline comments group as follows:
Correctness (blocking — these hold regardless of the SHOULD/MUST outcome, all on the shared test AS):
- Sticky-sink masking: dpopJktSent/dpopJktMatched are last-write-wins, so a caught mismatch can be overwritten and the run ends SUCCESS (createAuthServer.ts 654-657, with suggested fix).
- No-proof fall-through: a jkt-bound code exchanged with no DPoP proof at all gets a Bearer token — RFC 9449 §10 requires rejection (623-628, verified empirically, suggested code).
- Unknown codes mint tokens: the token endpoint doesn't reject codes it never issued (522, verified empirically).
- Mismatch rejection precedes the nonce challenge, so one client bug yields two check failures with a misleading second diagnosis (660).
- Authorization codes live forever and are infinitely reusable — the enabler for 2 and 3 (293, with lifecycle sketch).
Grading/provenance (non-blocking, both with committable suggestions):
6. Doc comment says "requires" where the SEP says SHOULD (dpop.ts 94-95).
7. The yaml row quotes a sentence that isn't in any spec yet; re-quote verbatim once ext-auth#35 lands (sep-1932.yaml 10-13).
Nits (non-blocking, all with tested fixes):
8. The pre-existing no-token-proof negative test doesn't assert the new check's untestable WARNING (index.test.ts, comment at 462).
9. The §10 unit tests run on an invalid PKCE pair ('x'/'x'), recording a spurious PKCE FAILURE observation every run (createAuthServer.test.ts 35).
10. The untestable emission omits the details.reason key that the #248 convention shape includes (dpop.ts 272-295, committable).
Methodology note: every claimed behavior above was verified empirically against the PR head (abc3cb2), and every suggestion block was executed before posting — tsc --noEmit clean, prettier clean, index.test.ts 70/70 and createAuthServer.test.ts 2/2 with the changes applied.
Generated with the aid of Claude (Fable 5).
This branch has not been deployed
2 participants
Discussed here: https://discord.com/channels/1358869848138059966/1552316422951149639
Motivation and Context
There is a short window between authorization and token exchange where a stolen authorization code could be redeemed using an attacker-controlled DPoP key. The
dpop_jktauthorization parameter closes that gap by declaring which key may redeem the code.This change adds a client check for
dpop_jktassuming SEP-1932 ends up with a SHOULD fordpop_jkt.The test client sends
dpop_jkt, and the test authorization server stores it with the specific authorization code. During token exchange, the server compares it with the key in the DPoP proof. Omittingdpop_jktproduces a WARNING; sending a mismatched thumbprint produces a FAILURE. Authorization state is tracked per code internally so overlapping flows cannot overwrite each other.How Has This Been Tested?
dpop_jktBreaking Changes
No API or configuration changes.
Clients participating in the DPoP extension may receive a new WARNING if they omit
dpop_jkt, assuming SEP-1932 ends up with a SHOULD.Types of changes
Checklist