feat(sdk): add comprehensive DPoP (RFC 9449) support (DSPX-3397)#374
Draft
dmihalcik-virtru wants to merge 36 commits into
Draft
feat(sdk): add comprehensive DPoP (RFC 9449) support (DSPX-3397)#374dmihalcik-virtru wants to merge 36 commits into
dmihalcik-virtru wants to merge 36 commits into
Conversation
Contributor
|
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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for custom DPoP keys in the SDKBuilder and implements DPoP nonce caching in TokenSource and AuthInterceptor to handle server-issued nonces. It also adds a new "supports" subcommand to the CLI to check for feature support (e.g., "dpop"). Regarding the feedback, the "supports" subcommand should avoid calling System.exit() directly, as this abruptly terminates the JVM and hinders testing; instead, it should implement Callable and return the exit code.
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.
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
5242c69 to
00414f1
Compare
Contributor
X-Test Failure Report |
Contributor
X-Test Results✅ java@pull-374-main |
dmihalcik-virtru
changed the title
dmihalcik-virtru
changed the title
dmihalcik-virtru
changed the title
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
f8baeab to
244f4ba
Compare
Contributor
Contributor
Contributor
X-Test Failure Report |
Contributor
Contributor
Contributor
X-Test Failure Report |
Contributor
Contributor
Contributor
Contributor
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
95ce0d0 to
b8d21d1
Compare
|
Contributor
- Add server-issued nonce caching infrastructure in TokenSource with per-origin storage - Add dpopKey() method to SDKBuilder for caller-supplied RSA keys (defaults to auto-generated ephemeral key) - Update AuthInterceptor to cache DPoP-Nonce from successful responses - Add 'supports dpop' CLI command for xtest feature detection - Extend TokenSource.getAuthHeaders() to accept optional nonce parameter for proof generation Implementation uses Nimbus OAuth2 SDK's DefaultDPoPProofFactory for RFC 9449 compliant DPoP proof generation with htm/htu/iat/jti claims (plus ath for resource endpoints). Current implementation uses RSA-2048/RS256 for DPoP keys. The SDK already had DPoP proof generation via Nimbus OAuth2 SDK; this PR adds nonce support infrastructure and makes the DPoP key configurable. Note: Full 401 retry logic with nonce challenges requires Connect RPC interceptor changes and is deferred to future work. Nonce caching infrastructure is in place for when retry logic is added. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
- AuthInterceptor.kt: fix resp.code→resp.status and resp.message.request() compilation errors; use ThreadLocal<URL> to thread request URL into responseFunction for nonce caching; change private→internal so SDKBuilder.java can access dpopRetryInterceptor(); add dpopRetryInterceptor() OkHttp interceptor that caches DPoP-Nonce and retries 401 once - TokenSource.java: wrap nonce String as new Nonce(nonce) to match DefaultDPoPProofFactory.createDPoPJWT signature; generalize RSAKey to JWK+JWSAlgorithm to support EC keys for ES256/ES384/ES512 - SDKBuilder.java: update to JWK+JWSAlgorithm, separate SRT key from DPoP key (EC DPoP key auto-generates RSA for SRT), wire dpopRetryInterceptor into OkHttpClient for KAS and all platform services; add dpopAlgorithm() builder method - Add TokenSourceTest and DPoPRetryInterceptorTest (6 new tests) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Add DPoP configuration flags to the tdf cmdline tool (shared by encrypt and decrypt via buildSDK()): - --dpop[=<alg>]: enable DPoP; optional algorithm (RS256, RS384, RS512, ES256, ES384, ES512); defaults to RS256; generates ephemeral key - --dpop-key <path>: use PEM-encoded private key from file; algorithm inferred from key type (EC or RSA); combinable with --dpop=<alg> Both flags work for encrypt and decrypt subcommands. Help text contains "dpop" so the grep probe matches: encrypt --help | grep -i dpop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…e.INHERIT Add scope = CommandLine.ScopeType.INHERIT to --dpop and --dpop-key so they appear in `help encrypt` and `help decrypt` (not just the parent `tdf` help), allowing the tests-repo cli.sh probe to pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Gemini review: System.exit() in Supports.run() abruptly terminates the JVM and prevents unit testing. Switch to Callable<Integer> so picocli handles the exit code via CommandLine.execute(). Also remove `required = true` from --client-id, --client-secret, and --platform-endpoint so `supports dpop` can run without auth credentials (it performs a local capability check, not a platform call). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
… imports Adds EC key, origin-isolated nonce, and empty-nonce guard tests to TokenSourceTest; removes three nl.altindag.ssl imports from SDKBuilder that were added without a corresponding pom.xml dependency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
- Add test cases for Command.Supports subcommand - Implement support for dpop_nonce_challenge feature - Add JUnit 5 and AssertJ test dependencies to cmdline module
- Validate WWW-Authenticate before retry/cache: only honor 401s carrying scheme=DPoP and error=use_dpop_nonce (RFC 9449 §8). A bare DPoP-Nonce header from any 401 no longer triggers a retry or poisons the cache. - Cache rotated nonces after every chain.proceed(), not just on 401s (RFC 9449 §8.1) — the next request now picks up rotations from 200s without an extra round-trip. - Clear the ThreadLocal<URL> defensively on entry and on exception so a failure in getAuthHeaders() cannot leak the URL into the next call on the same worker thread. Tests cover: single-retry guarantee, three negative WWW-Authenticate shapes (no challenge, Basic, DPoP error=invalid_token), 200-response nonce rotation cached for the next request, and 10-thread concurrent retry smoke test using a stateful Dispatcher (FIFO queues can't deliver alternating 401/200 reliably under concurrent load).
- Validate DPoP JWK/JWS-algorithm compatibility at TokenSource construction (new DpopKeyValidation helper, also used by SDKBuilder for EC curve→algorithm inference): RSA keys require RS*/PS*, EC keys must match curve to ES* (P-256↔ES256, P-384↔ES384, P-521↔ES512), other JWK types are rejected. Mismatches now fail fast at build time instead of at first proof. - Replace the broad catch(Exception) in getToken() with specific catches: SDKException passes through unwrapped (no double-wrapping of use_dpop_nonce errors), IOException/JOSEException/ParseException/ MalformedURLException each get a distinct, endpoint-named message. - Reject a 200 response with no access_token as a defensive guard against a null token being cached and producing 'DPoP null' headers. - Include the token endpoint URI in the use_dpop_nonce-missing-header warn log and promote the unknown-token-type log from trace to warn, since both indicate AS protocol violations. - Cover EC→RSA SRT key separation and the inverse RSA-reuse case with SDKBuilderTest cases. The EC path was previously untested even though it's the load-bearing motivation for the JWK generalization. Tests cover: nonce origin keying by port/scheme/default-port, parse errors attributed to the token endpoint, three JWK/algorithm mismatch cases, and EC↔RSA SRT signer behavior.
The --client-id/--client-secret/--platform-endpoint flags were demoted from required=true so that 'tdf supports <feature>' (used by xtest harnesses to probe SDK capabilities) could run without credentials. The side effect was that 'tdf encrypt' and 'tdf decrypt' silently passed picocli validation and failed deep inside SDKBuilder.build() with an opaque error. Keep the picocli annotations optional and instead enforce in buildSDK() with picocli's ParameterException, which produces the standard 'Missing required option: ...' error + usage and exits with code 2. 'tdf supports' is unaffected since it does not call buildSDK().
Picocli's default execution handler prints only ex.getMessage(), which is null for many failure modes (NPE, etc.). Exceptions thrown during CommandLine construction or by picocli itself bypass that handler entirely and would otherwise terminate the JVM silently. Wrap main() in a top-level try/catch that calls printStackTrace() — the first line is the exception's toString (class + message) so the failure category is always identifiable, and the stack trace gives diagnostic depth for bug reports. Normal exit codes from picocli's execute() are unaffected.
- AuthInterceptor: log exception at DEBUG level when DPoP retry chain.proceed() throws - Command: -v/--verbose now raises root log level to DEBUG (only if currently coarser) - log4j2.xml: default root level changed from trace to info
If Keycloak (or any AS) returns token_type=Bearer despite the SDK sending a DPoP proof, the prior behavior was to emit "Authorization: DPoP <bearer>" which misuses the scheme (RFC 9449 §7.1) and is rejected by any DPoP-enforcing resource server. TokenSource now remembers the scheme the AS declared (DPoP vs Bearer) and getAuthHeaders() emits a plain Bearer credential without a DPoP proof on downgrade. AuthInterceptor only sets the DPoP request header when a proof is present. A single WARN is logged on downgrade to flag the IdP misconfiguration.
RFC 9449 §4.2 requires the htu claim to be the request URI without
query and fragment, and Nimbus enforces it by throwing
IllegalArgumentException("The HTTP URI (htu) must not have a query").
When the OkHttp dpopRetryInterceptor handed Nimbus a URL whose query
string came from the caller (e.g. a KAS rewrap URL), proof creation
blew up inside the OkHttp Dispatcher thread and surfaced as
'error getting kas servers'.
Normalize every URI fed to DefaultDPoPProofFactory through a single
htuOf() helper that strips both query and fragment.
After 38ac01f the SDK correctly downgrades to the Bearer scheme when the token endpoint returns token_type=Bearer, so the shared sdkServicesSetup helper stopped sending a DPoP header and the three tests calling it (testCreatingSDKServicesPlainText, testPlatformPlainTextAndIDPWithSSL, testSDKServicesWithTruststore) failed at the 'expecting DPoP header' assertion. The test intent is the DPoP path, so make the mock token endpoint advertise DPoP.
- §8.1 covers nonce *syntax*; the rotation/provision mechanic is §8.2.
- The OkHttp dpopRetryInterceptor handles resource-server traffic (KAS,
platform-services Connect client); that is RFC 9449 §9, not §8.
- Delete the obsolete docs/superpowers/{plans,specs}/2026-06-16-* files:
they describe a ~25-line, 3-file plan that no longer matches what
shipped (2200+ lines, 14 files, new public API surface).
…tform_issuer Previously SDKBuilder.getAuthInterceptor would silently warn-and-return-null when the well-known configuration omitted platform_issuer, which also disabled the dpopRetryInterceptor (gated on authInterceptor != null). A caller who explicitly opted into DPoP via dpopKey()/dpopAlgorithm() would then watch every request silently downgrade to no-auth and 401 from a DPoP-enforcing resource server with no client-side breadcrumb. Now the explicit-DPoP case throws SDKException with a message naming both DPoP and platform_issuer. The pre-existing no-auth fallback (no DPoP key configured) still warn-and-returns-null. The two SRT-derivation tests that relied on the silent fallback path are updated to mock a real OIDC endpoint, since 'dpopKey set + no token endpoint' is no longer a supported configuration.
Previously Command.applyDPoPOptions caught Exception and wrapped everything as 'Failed to configure DPoP: <message>', collapsing file-not-found, malformed PEM, unsupported algorithm, and key generation failures into one opaque RuntimeException. A public-key-only PEM was accepted without complaint and only failed deep inside proof generation. A --dpop-key with --dpop=<alg> mismatch was deferred to TokenSource construction. Refactor: - New CliDpopOptions static helper owns parse + validate + private-key check. Returns DpopMaterial (jwk + alg) or Optional.empty(); throws IllegalArgumentException with user-actionable messages. - applyDPoPOptions delegates to the helper and wraps IAE as CommandLine.ParameterException so picocli exits with USAGE (2) instead of a generic stack trace. - Promote DpopKeyValidation to public so cmdline can reuse the validation rules instead of duplicating them. Latent bug fixed: bcpkix-jdk18on was only test-scoped via the sdk module, so the production CLI's JWK.parseFromPEMEncodedObjects call for --dpop-key would have thrown NoClassDefFoundError. Adding it as a cmdline runtime dependency. Tests: - New CliDpopOptionsTest (16 cases): all six supported algorithms, default RS256, unsupported algorithm, missing/malformed/public-only PEM, RSA vs EC PEM acceptance and alg inference, RSA-key + ES256 mismatch. - Two new end-to-end CommandTest cases covering the unsupported-algorithm and missing-key-file ParameterException paths through CommandLine.execute.
Six DEBUG-level log sites tagged 'DPoP path=<stream|unary|unary-response|okhttp|okhttp-retry|okhttp-retry-response>' to triage the CI failure where the platform DPoP validator rejected proofs with htm=GET but expected POST. Each entry records the URL, the HTTP method that flows into the proof, the Authorization scheme (Bearer/DPoP — never the token), and a parsed DPoP claim summary (htm, htu, jti, nonce). Both the connect-layer method (request.httpMethod.name) and the outgoing OkHttp method (chain.request().method) are logged so a divergence between the two is visible. Helpers: - authScheme(): redacts the Authorization header to just its scheme. - dpopSummary(): parses the DPoP proof JWT and emits the claims that matter, falling back to <unparseable> on error. DEBUG level only — zero cost in production; surfaces via --verbose on the CLI.
Connect-RPC's GET extension rewrites idempotent POST RPCs to GET on the wire (with the request payload moved into the query string). The Connect interceptor stamps the DPoP proof before that rewrite happens, so htm=POST ends up on a GET request and the server rejects: 'incorrect htm claim in DPoP JWT; received [POST], but should match [[GET]]' This is the same class of mismatch that commit 16f16c9 fixed for htu (Connect-GET appends ?base64=&connect=v1&...&message=... to the URL, drifting htu the same way). htm wasn't covered. Disable Connect-GET on the authenticated ProtocolClient where DPoP proofs are attached. Keep it enabled on the unauthenticated bootstrap client (well-known config fetch) since no proof is sent there. Cost: authenticated unary RPCs (ListKeyAccessServers, etc.) round-trip as POST instead of GET. Those calls were never going to be CDN-cacheable anyway because each carries a per-request DPoP proof.
A 401 with WWW-Authenticate: DPoP error=use_dpop_nonce but no DPoP-Nonce header is a server protocol violation (RFC 9449 §9). The okhttp retry interceptor previously skipped the retry block and returned the bare 401 with no diagnostic. Add a WARN so the cause is visible, mirroring the existing token-endpoint handling in TokenSource.getToken.
The nonce-challenge retry rebuilt the request from chain.request(), which
copies the original DPoP header. Authorization was set unconditionally but
DPoP only re-added when the refreshed token was still DPoP-bound, so a
Bearer downgrade left the original stale DPoP proof paired with a Bearer
Authorization header. Explicitly removeHeader("DPoP") before re-adding.
getAuthHeaders called synchronized getToken() for the token, then read the non-volatile tokenScheme field and built the header outside the lock. That allowed a stale scheme read (missing a completed Bearer downgrade) and a TOCTOU where the token value and scheme could come from different token generations. getToken() now returns an immutable TokenSnapshot captured under the monitor, and getAuthHeaders builds the header from it.
DpopKeyValidation.validate (used by both SDKBuilder.dpopKey and the CLI) did not check for private key material, so an SDK caller passing a public JWK got an opaque failure only at proof-signing time. Add an isPrivate() check so all entry points fail fast with a clear message.
The key/alg pairing was enforced only because parse() happened to call DpopKeyValidation.validate before constructing; the constructor accepted any pair. Move the validation into the constructor so every DpopMaterial is valid by construction, and keep the file-path-aware error message by catching around the construction in parse().
The Javadoc claimed the result always includes a DPoP proof, contradicting the Bearer downgrade path where dpopHeader is null. Reword to state the DPoP header is only present for DPoP-bound tokens.
No test verified the htm claim, so a regression re-enabling Connect-GET (which rewrites POST->GET and would drift the pre-minted proof's htm) would pass unnoticed. Pin htm=POST in a proof-parsing test.
The connect path (unaryFunction/responseFunction) threads the request URL through a ThreadLocal so nonces cache against the right origin — the code's own comment flags this as the fragile part — yet no unit test exercised it. Add a Kotlin test covering (1) requestFunction adding DPoP headers and responseFunction caching the nonce against the request origin, and (2) the catch-block clearing the ThreadLocal when requestFunction throws so a stray responseFunction cannot cache against a stale origin. Enable src/test/kotlin in the kotlin-maven-plugin test-compile execution so Kotlin tests are compiled (only src/test/java was registered before).
Replace the stringly-typed tokenScheme ("DPoP"/"Bearer" String constants)
with a two-value TokenScheme enum that carries its header prefix. This makes
the scheme states exhaustive and removes the .equals string comparison. Also
mark AuthHeaders static since it never used the enclosing instance.
…adoc - Document the retry-at-most-once guarantee on dpopRetryInterceptor. - Fix the htuOf comment so the rationale matches the code (query and fragment are both stripped; Nimbus additionally rejects a query). - Reduce the boilerplate Javadoc on the self-evident cacheNonce and getOrigin helpers to a single line each.
When a resource server responds to the §9 retry with a second use_dpop_nonce challenge (e.g. aggressive nonce rotation or clock skew), the interceptor previously returned the bare 401 with only a debug log, making the double-failure invisible. Log it at WARN so the failed handshake is visible, mirroring the token-endpoint handling in TokenSource.getToken(). The request is still retried at most once. Covered by DPoPRetryInterceptorTest via an in-memory log appender. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
b8d21d1 to
28510cb
Compare
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
1 participant
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
Adds comprehensive DPoP (RFC 9449) support to the Java SDK as part of the Keycloak v26 upgrade and DPoP implementation effort.
Related Jira: https://virtru.atlassian.net/browse/DSPX-3397
Test Scenario: xtest/scenarios/DSPX-3397.yaml (in tests repo)
Changes
Core DPoP proof generation
DefaultDPoPProofFactory, with claimsjti,htm,htu,iat(plusathfor resource endpoints).htuclaim strips query and fragment per RFC 9449 §4.2.DPoPvsBearer) modeled as an enum; token + scheme read atomically to avoid a refresh-time data race.Server-issued nonce handling (full retry — implemented)
use_dpop_nonce, retries the token request once with the AS-provided nonce.DPoP-Noncefrom aWWW-Authenticatechallenge; drops the stale proof header on the retry.Key configuration & validation
dpopKey(...)accepts a caller-supplied RSA or EC key; auto-generates an ephemeral key if none provided. An EC DPoP key triggers a separate RSA-2048 SRT key.htmclaim.Bearer fallback
Bearerscheme when the AS returns a non-DPoP-bound token, with a warning; stale DPoP proof is stripped so a Bearer request never carries a DPoP header.platform_issuer.CLI (cmdline)
--dpop/--dpop-keyflags on encrypt/decrypt (ScopeType.INHERIT), validated at parse time viaDpopMaterial.tdf supports dpopandtdf supports dpop_nonce_challenge(exit 0 if supported).Feature Detection
Testing
New coverage:
TokenSourceTest,DPoPRetryInterceptorTest,AuthInterceptorConnectPathTest(Kotlin),CliDpopOptionsTest,CommandTest— covering nonce caching/origin isolation, both retry paths, EC/RSA key handling, and CLI option parsing.Related PRs
Checklist
--dpop/--dpop-keyflags + feature detection