feat(sdk): comprehensive DPoP nonce handling and verification#939
Draft
dmihalcik-virtru wants to merge 53 commits into
Draft
feat(sdk): comprehensive DPoP nonce handling and verification#939dmihalcik-virtru wants to merge 53 commits into
dmihalcik-virtru wants to merge 53 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces DPoP-Nonce caching per RFC 9449 §8, implementing a DPoPNonceCache to store and reuse server-issued nonces by origin, and updating both the authentication interceptor and OIDC client to handle nonce-based retries on 401 challenges. It also adds a CLI command to check for DPoP support. The review feedback highlights several critical improvements: ensuring that 401 retries occur when a new or different nonce is received (rather than only when no nonce was cached), using optional chaining on error metadata to prevent runtime crashes, avoiding direct process.exit calls in the CLI handler, and adding defensive checks when extracting nonces from headers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+129
to
+131
| if (serverNonce && !cachedNonce) { | ||
| // Server sent a nonce and we didn't have one cached | ||
| // Cache it and retry once |
There was a problem hiding this comment.
This condition prevents retrying the request if a cachedNonce already exists. If the cached nonce has expired or been invalidated, the server will reject it with a 401 and return a new DPoP-Nonce. Because of !cachedNonce, the client will fail to retry with the new nonce and throw a 401 error. Changing the condition to serverNonce !== cachedNonce correctly allows retrying when the server provides a new/different nonce, while still preventing infinite loops if the same nonce is repeatedly returned.
Suggested change
| if (serverNonce && !cachedNonce) { | |
| // Server sent a nonce and we didn't have one cached | |
| // Cache it and retry once | |
| if (serverNonce && serverNonce !== cachedNonce) { | |
| // Server sent a new nonce (or we didn't have one cached) | |
| // Cache it and retry once |
Comment on lines
+126
to
+127
| const metadata = err.metadata as { get?: (key: string) => string | null }; | ||
| const serverNonce = metadata.get?.('dpop-nonce'); |
There was a problem hiding this comment.
If err.metadata is null or undefined, attempting to access metadata.get will throw a TypeError (e.g., Cannot read properties of undefined (reading 'get')), which will crash the error handler and mask the original authentication error. Using optional chaining on metadata prevents this potential runtime crash.
Suggested change
| const metadata = err.metadata as { get?: (key: string) => string | null }; | |
| const serverNonce = metadata.get?.('dpop-nonce'); | |
| const metadata = err.metadata as { get?: (key: string) => string | null } | undefined; | |
| const serverNonce = metadata?.get?.('dpop-nonce'); |
Comment on lines
+523
to
+531
| async (argv) => { | ||
| const feature = argv.feature as string; | ||
| if (feature === 'dpop') { | ||
| // DPoP is supported - exit 0 | ||
| process.exit(0); | ||
| } | ||
| // Unknown feature - exit 1 | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Calling process.exit directly inside a CLI command handler is discouraged. It prevents graceful cleanup/shutdown, makes unit testing the command extremely difficult (as it terminates the test runner), and is redundant here. Since choices: ['dpop'] is defined on the positional argument, yargs automatically validates the input and will fail with a non-zero exit code if any other feature is requested. If 'dpop' is passed, the handler can simply return successfully to exit with code 0 naturally.
async () => {
// If we reached here, yargs validation passed and the feature is supported.
}
Comment on lines
+210
to
+212
| if (this.config.dpopEnabled && response.status === 401) { | ||
| const responseNonce = DPoPNonceCache.extractNonce(response.headers); | ||
| if (responseNonce) { |
There was a problem hiding this comment.
If the server returns a 401 with the exact same nonce that we just sent, retrying the request will inevitably fail again with another 401. To avoid redundant and wasteful HTTP requests, we should verify that the returned responseNonce is different from the cachedNonce we used in the initial request before attempting a retry.
Suggested change
| if (this.config.dpopEnabled && response.status === 401) { | |
| const responseNonce = DPoPNonceCache.extractNonce(response.headers); | |
| if (responseNonce) { | |
| if (this.config.dpopEnabled && response.status === 401) { | |
| const responseNonce = DPoPNonceCache.extractNonce(response.headers); | |
| const cachedNonce = globalNonceCache.get(origin); | |
| if (responseNonce && responseNonce !== cachedNonce) { |
Comment on lines
+34
to
+37
| static extractNonce(headers: Headers): string | undefined { | ||
| // Headers.get() is case-insensitive per spec | ||
| return headers.get('dpop-nonce') || undefined; | ||
| } |
There was a problem hiding this comment.
To enforce defensive programming and prevent potential runtime crashes, we should guard against cases where headers is null, undefined, or does not implement the standard Headers interface (which can easily happen in test environments with mocked responses or custom fetch implementations). Checking if headers?.get is a function before calling it makes this utility much more robust.
static extractNonce(headers?: Headers): string | undefined {
return typeof headers?.get === 'function' ? headers.get('dpop-nonce') || undefined : undefined;
}
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
dmihalcik-virtru
force-pushed
the
DSPX-3397-web-sdk
branch
from
bce2ae8 to
cc190c6
Compare
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
dmihalcik-virtru
changed the title
dmihalcik-virtru
force-pushed
the
DSPX-3397-web-sdk
branch
from
bda4566 to
5e933b0
Compare
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
dmihalcik-virtru
force-pushed
the
DSPX-3397-web-sdk
branch
from
de2ca53 to
686bd9f
Compare
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
…PX-3397) Implements RFC 9449 DPoP-Nonce support across SDK and CLI: - Add DPoP-Nonce cache manager (dpop-nonce.ts) with per-origin nonce storage - Update authTokenDPoPInterceptor with automatic nonce retry on 401 challenges - Extend OIDC token endpoint and userinfo flows to handle nonce caching/refresh - Add 'supports dpop' CLI command for xtest integration testing detection - Refresh cached nonces from successful response headers per RFC 9449 §8 All DPoP proofs now include cached nonces when available and automatically retry with server-provided nonces on 401 use_dpop_nonce errors. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…3397) Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…DSPX-3397) Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…nsive handling (DSPX-3397) - interceptors.ts: use serverNonce !== cachedNonce to allow retry on nonce rotation, not just first nonce - interceptors.ts: optional-chain err.metadata to prevent TypeError crash on absent metadata - dpop-nonce.ts: guard extractNonce against null/non-standard headers (test-env robustness) - oidc.ts: hoist cachedNonce outside dpopEnabled block; skip retry when server returns same nonce - cli.ts: remove redundant process.exit(0) from supports command handler; yargs choices handles validation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…ogic (DSPX-3397) - dpop-nonce.ts: add clearAll() to DPoPNonceCache for test teardown - server.ts: add /protocol/openid-connect/token endpoint that issues a DPoP-Nonce challenge (fixed nonce 'dpop-test-nonce-abc') when the incoming DPoP proof has no nonce, accepts on retry with correct nonce - tests/web/auth/dpop-nonce.test.ts: WTR unit tests covering doPost() nonce retry (via mock fetch) and authTokenDPoPInterceptor nonce retry (via mock next), including no-retry-on-same-nonce regression cases - tests/mocha/dpop-nonce.spec.ts: Mocha integration tests hitting the real server — verifies transparent retry, nonce cache population, and pre-cached nonce path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…SPX-3397) The KAS rewrap request token was always signed with RS256, so an EC dpop key (e.g. --dpop ES256) made WebCrypto throw 'Unable to use this key to sign', surfacing as 'unable to unwrap key from kas'. Derive the JWS alg from the dpop private key's algorithm instead. Adds a regression test decrypting with EC dpop keys.
…3397) The legacy REST rewrap fallback 404s on Connect-only KAS and masked the real RPC error (e.g. a post-nonce-challenge 401) in tryPromisesUntilFirstSuccess. Now auth/validation errors (401/403/400) surface immediately without a legacy attempt, and for other errors the legacy fallback is still tried but the original RPC error is surfaced if it also fails. Add an integration test driving a full encrypt->decrypt roundtrip through a DPoP auth provider so the rewrap trips the mock KAS RS nonce gate and the interceptor must retry once (RFC 9449 §9). Update rewrap error-case tests to assert the real RPC error now surfaces instead of the masked 404.
…ry (DSPX-3397) Against a real require_nonce KAS, the rewrap 401 challenge carries DPoP-Nonce as a raw HTTP response header that connect-web does not surface on ConnectError. metadata, so the interceptor never saw the nonce and could not retry — the 401 propagated (xtest test_dpop_* failing with js as decrypt SDK). Wrap the Connect transport's fetch (platform.ts) to record DPoP-Nonce from the raw Response into the per-origin globalNonceCache via a new captureNonce helper (dpop-nonce.ts). Both DPoP interceptors now source the challenge nonce from the cache (populated by that wrapper) and fall back to error metadata, then re-mint a nonce-bearing proof and retry once (RFC 9449 §9).
…PX-3397) The KAS rewrap request token (SRT) is signed via reqSignature -> signJwt, but signJwt used cryptoService.sign's output directly, which is DER-encoded for ECDSA. JWS (RFC 7518 §3.4) requires raw IEEE P1363 (R||S), so a real KAS rejected the ES256-signed SRT with 'unable to verify request token' (a 401 that surfaced only after the DPoP nonce challenge was satisfied). The mock test server only decodeJwt's the SRT, so this was invisible locally. signJwt now converts ECDSA signatures DER->P1363 (mirroring src/auth/dpop.ts), and verifyJwt converts P1363->DER before cryptoService.verify so ES* assertions still round-trip. Export ieeeP1363ToDer for the verify path. Add a spec that verifies reqSignature/signJwt ES256/384/512 output against jose.jwtVerify (RFC-conformant), which the in-SDK round-trip and the mock server cannot catch. Also removes the temporary fetch-layer debug logging.
Prefer a type alias over an interface, matching the prevailing style in lib/src/auth (the only interface in the directory). No behavior change.
The DPoP-Nonce challenge/retry bookkeeping (RFC 9449 §8/§9) was duplicated across six call sites in two transport families. Extract three helpers in dpop-nonce.ts: - adoptChallengeNonce: fresh challenge nonce from a Response (raw-fetch family) - adoptChallengeNonceFromConnectError: same for a Connect Unauthenticated error - warmNonceFromResponse: keep the cache warm from any response nonce Each call site now delegates the cache bookkeeping and keeps only its transport-specific re-sign/resend inline. Still routed through globalNonceCache; no behavior change (DPoP mocha specs green).
Replace the process-wide global nonce singleton with a per-client cache so nonces no longer leak across independent SDK clients (RFC 9449 §8). - AccessToken owns a DPoPNonceCache; each OIDC provider exposes it via a nonceCache getter, so its interceptor and legacy-fetch retry read the same instance its withCreds writes. - AuthProvider and DPoPInterceptorOptions/PlatformClientOptions gain an optional nonceCache; authProviderInterceptor and fetchWithCredsAndNonceRetry resolve authProvider.nonceCache, authTokenDPoPInterceptor and PlatformClient fall back to defaultNonceCache (also exposed as the deprecated globalNonceCache alias for back-compat). - captureNonce and the transport fetch wrapper (makeNonceCapturingFetch) take the cache explicitly; access-rpc/access.ts thread the provider's cache so the transport's nonce capture and the interceptor retry stay on one instance (the KAS rewrap/RPC nonce-challenge guardrail). Tests migrated to observe the per-client cache. Full suite green: mocha 354, web-test-runner 260.
When the legacy REST rewrap fallback also fails, we still (correctly) rethrow the more meaningful RPC error — but the legacy failure was dropped entirely. Log it via console.info (matching the sibling 'v2 rewrap request error' line) so the fallback path is debuggable. Behavior is otherwise unchanged.
The CLI wraps the OIDC provider in a LoggedAuthProvider decorator that exposed withCreds/updateClientPublicKey but not nonceCache. After the per-client cache change, the auth interceptor and Connect transport resolved `authProvider.nonceCache ?? defaultNonceCache` (=> default, since the wrapper hid it) while withCreds — delegated to the wrapped AccessToken — minted proofs from the AccessToken's own cache. The two caches diverged, so the DPoP-Nonce challenge retry (RFC 9449 §9) never carried the server nonce and js decrypt failed at ListKeyAccessServers with 401 (xtest test_dpop_happy_path_roundtrip, test_dpop_server_issued_nonce_retry). Forward nonceCache so the wrapper, its interceptor, and withCreds share one instance.
Per-client-by-default proved fragile: an AuthProvider decorator that doesn't forward nonceCache (the CLI's LoggedAuthProvider) split the interceptor/transport cache from the one AccessToken.withCreds mints against, breaking the DPoP-Nonce retry. Make the shared defaultNonceCache the default for AccessToken so every DPoP path converges on one instance even through non-forwarding wrappers. Per-client isolation stays opt-in via the existing injection points (AccessToken constructor, PlatformClientOptions.nonceCache, DPoPInterceptorOptions.nonceCache). The CLI nonceCache forward (previous commit) is now redundant but kept as future-proofing. Provider/AccessToken-based specs restore afterEach teardown of the shared cache to stay isolated. Full suite green: mocha 354, web-test-runner all pass.
|
…recated alias (DSPX-3397) - dpop-nonce.ts: extract shared adoptIfFresh() + toOrigin(); route captureNonce and the two try/catch origin sites (interceptors, access-fetch) through toOrigin - remove the unused @deprecated globalNonceCache alias (not exported, no consumers) - dpop.ts: narrow DPoPJwtHeaderParameters.typ to the literal 'dpop+jwt' - oidc.ts: fix the truncated/contradictory signingKey doc comment - interceptors.ts: add X-VirtruPubKey->X-OpenTDF-PubKey rename TODO (mirrors oidc.ts) - tests: dpop-nonce.spec assert real outgoing headers (not just a config flag); dpop-headers.spec drop console noise + assert proof typ='dpop+jwt'
…tics (DSPX-3397) access.ts: replace tryPromisesUntilFirstSuccess with tryRpcThenLegacy, reused by fetchWrappedKey, fetchKeyAccessServers, and fetchKasPubKey. The two sibling fetches now short-circuit on a definitive RPC auth error and surface the RPC error (not the legacy 404) on double failure — the fix previously applied only to fetchWrappedKey, so a DPoP auth failure on a Connect-only platform no longer masquerades as a 404 for an unimplemented legacy endpoint. dpop-nonce.ts: add warnNonceRetryGiveUp() and emit one concise diagnostic at every nonce-retry give-up site (both interceptors, legacy fetch, oidc doPost/info), gated on a genuinely present DPoP-Nonce so ordinary 401s stay quiet. captureNonce now warns when a nonce arrives on a non-absolute URL (which silently disables the Connect-RPC retry) instead of dropping it. tests: add an interceptor 'gives up and rethrows the original error' test; tighten the access-fetch no-retry tests to assert the surfaced ServiceError identity.
…logging (DSPX-3397) cli/dpop-helpers.ts: reject RS384/RS512 (removed from VALID_DPOP_ALGS; they signed as RS256 anyway) instead of warning + silently downgrading. Error when an explicit --dpop=<alg> conflicts with a --dpopKey file's algorithm (EC curve exact, RSA family match); a bare --dpop still infers from the key. Threads algWasExplicit from resolveDPoPFromArgs. access.ts: add errBrief() and log a one-line summary (message + Connect code) instead of the whole error object in the RPC->legacy fallback path, so response headers/metadata (incl. DPoP nonces) aren't dumped to logs. docs: update the DPoP CLI design spec + plan to match (RS384/RS512 rejected; explicit alg/key conflict is an error rather than 'key type wins'). tests: cover RS384/RS512 rejection and the explicit alg/key conflict + match cases.
Bounds-check every index and slice while parsing the ECDSA DER signature so malformed input always throws a controlled ConfigurationError instead of an out-of-bounds RangeError/TypeError or a silently-truncated component: - reject signatures shorter than the 8-byte minimal SEQUENCE up front - parse each INTEGER through a bounds-checked reader (rejects truncated or over-long length fields rather than slicing a short/empty component) - strip all leading zero pad bytes and reject any component larger than the curve's fixed width (previously produced a negative result.set offset) Valid signatures are byte-for-byte unchanged (ES256/384/512 sign->verify round-trips still pass). Adds direct malformed-DER unit tests. derToIeeeP1363 is on both the DPoP proof and TDF3 JWT signing/verification paths (RFC 7518 3.4).
determineJWSAlgorithmFromKeyInfo now returns AsymmetricSigningAlgorithm (the subset CryptoService can actually sign with); it never produces the forward-looking PS256/EdDSA members of JWSAlgorithm. In jwt(), replace the unchecked 'header.alg as AsymmetricSigningAlgorithm' with an isAsymmetricSigningAlgorithm type guard that throws UnsupportedOperationError for an unsupported alg, so a bad value fails early and clearly instead of deep inside getSigningAlgorithmParams. The JWSAlgorithm union and its PS256/EdDSA roadmap docs are kept unchanged.
…3397) The dpopEnabled && !signingKey state was handled inconsistently at request time: withCreds and doPost threw (with two different messages) while info() silently downgraded to a Bearer token — a silent failure on a misconfigured DPoP client. Add a private requireSigningKey() helper and route all three request paths through it, so they fail with one clear ConfigurationError. info() no longer falls back to Bearer when DPoP is enabled but no key is bound. Validation stays at request time (not construction), so the legitimate deferred-binding flow — enable DPoP now, bind the key later via refreshTokenClaimsWithClientPubkeyIfNeeded (opentdf.ts ready) — keeps working; a new test covers it. Also removes a stray debug console.log on the Connect-RPC rewrap error path (log-hygiene, matching the earlier errBrief work).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements comprehensive DPoP (RFC 9449) support for the web-sdk as part of the Keycloak v26 upgrade and platform-wide DPoP feature.
Parent Jira: https://virtru.atlassian.net/browse/DSPX-3397
Test Scenario: xtest/scenarios/DSPX-3397.yaml
Changes
DPoP-Nonce Support (RFC 9449 §8)
lib/src/auth/dpop-nonce.ts- Per-origin nonce cache managerlib/src/auth/interceptors.ts- Auto-retry on 401 with DPoP-Nonce challengelib/src/auth/oidc.ts- Token endpoint and userinfo nonce handlingVerification & Testing
athclaim on resource callscnf.jktvia existing proof generationxtest Integration
cli/src/cli.ts- Addedsupports dpopcommand for feature detectionopentdf supports dpop→ exit 0 if supportedImplementation Details
Per RFC 9449 §8, the SDK now:
DPoP-Nonceresponse header:Works across:
Related PRs
opentdf/tests#DSPX-3397-kc26-dpop- Integration tests & otdf-local KC26 bumpopentdf/platform#DSPX-3397-platform-service- Platform service DPoP validationopentdf/platform#DSPX-3397-platform-go-sdk- Go SDK DPoP clientopentdf/java-sdk#DSPX-3397-java-sdk- Java SDK DPoP clientTesting
Local builds and lints pass. Integration tests will activate once the tests-cell KC26 bump lands and this PR's CI exposes
supports dpop.🤖 Generated with Claude Code