feat(sdk): add comprehensive DPoP (RFC 9449) support (DSPX-3397)#374
Open
dmihalcik-virtru wants to merge 43 commits into
Open
feat(sdk): add comprehensive DPoP (RFC 9449) support (DSPX-3397)#374dmihalcik-virtru wants to merge 43 commits into
dmihalcik-virtru wants to merge 43 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThe CLI adds feature reporting, verbose diagnostics, and explicit credential validation. The SDK adds configurable DPoP keys and algorithms, nonce-aware token handling, bearer fallback, interceptor retries, and expanded authentication-flow tests. ChangesCLI behavior and build setup
SDK DPoP authentication
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SDKBuilder
participant TokenSource
participant AuthInterceptor
participant ResourceServer
SDKBuilder->>TokenSource: configure DPoP key and algorithm
AuthInterceptor->>TokenSource: request authorization headers
TokenSource-->>AuthInterceptor: Authorization and optional DPoP
AuthInterceptor->>ResourceServer: send authenticated request
ResourceServer-->>AuthInterceptor: 401 use_dpop_nonce with nonce
AuthInterceptor->>TokenSource: cache nonce and refresh headers
AuthInterceptor->>ResourceServer: retry request once
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
June 11, 2026 17:55
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
June 12, 2026 14:41
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
June 26, 2026 21:36
95ce0d0 to
b8d21d1
Compare
Contributor
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
July 2, 2026 16:04
b8d21d1 to
28510cb
Compare
Contributor
Contributor
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>
Contributor
Contributor
Drops orphaned imports that SonarCloud (java:S1128) reported: - SDKBuilder: com.nimbusds.jose.jwk.Curve, com.nimbusds.jose.jwk.gen.ECKeyGenerator - TokenSource: com.nimbusds.jose.jwk.RSAKey
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
July 14, 2026 15:50
f975143 to
1b2ea46
Compare
Contributor
`tdf supports` with no argument now prints the supported feature names,
one per line. A new --json flag emits a JSON object mapping feature name
to a boolean (e.g. {"dpop":true}); with no argument it lists all
supported features as true. Exit codes are unchanged: 0 when a queried
feature is supported, 1 otherwise.
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
July 14, 2026 16:16
1b2ea46 to
6ae85b2
Compare
Contributor
dmihalcik-virtru
added a commit
to opentdf/tests
that referenced
this pull request
Jul 14, 2026
## Problem The java-sdk removed the user-facing `--dpop` / `--dpop-key` CLI flags in opentdf/java-sdk#374. DPoP is now always-on by default, and capability is exposed through the `supports` subcommand instead. The Java wrapper's `supports dpop` case still detects DPoP by grepping `help encrypt` for the `--dpop` flag: ```bash dpop) set -o pipefail java -jar "$SCRIPT_DIR"/cmdline.jar help encrypt | grep -iE -- '--dpop' exit $? ;; ``` With the flags gone this grep finds nothing, exits 1, and xtest skips java DPoP tests: ``` SKIPPED [2] tdfs.py:619: java@... sdk doesn't yet support [dpop] ``` The sibling `dpop_nonce_challenge` case already delegates to the `supports` subcommand and is unaffected. ## Fix Detect `dpop` the same way `dpop_nonce_challenge` already does — via the `supports` subcommand, whose exit code is the capability contract (0 = supported, 1 = not): ```bash dpop) java -jar "$SCRIPT_DIR"/cmdline.jar supports dpop exit $? ;; ``` `set -o pipefail` is dropped since there's no longer a pipe. ## Verification With a current java `cmdline.jar` installed: `./cli.sh supports dpop; echo $?` prints `0`, and `dpop_nonce_challenge` still returns `0`. Only `xtest/sdk/java/cli.sh` changes; other SDK wrappers are untouched. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved detection of DPoP support in the Java CLI feature probe. * Prevents incorrect results caused by relying on help output and flag matching. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds end-to-end DPoP (RFC 9449) support to the Java SDK (proof minting, nonce caching, and single-retry nonce challenges) alongside CLI changes to make DPoP always-on for authenticated usage and to provide stable feature detection via a supports subcommand.
Changes:
- Implement DPoP proof generation and Bearer fallback in
TokenSource, including per-origin nonce caching and token-endpoint retry onuse_dpop_nonce. - Add OkHttp-level DPoP nonce-challenge retry and Connect unary-path nonce caching in
AuthInterceptor, and disable Connect-GET for authenticated clients to preserve thehtmbinding. - Update CLI behavior: introduce
tdf supports(plain / JSON), add--verboseplumbing, adjust logging defaults, and expand test coverage.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/src/test/kotlin/io/opentdf/platform/sdk/AuthInterceptorConnectPathTest.kt | New Kotlin tests covering Connect unary ThreadLocal URL handoff + nonce caching behavior. |
| sdk/src/test/java/io/opentdf/platform/sdk/TokenSourceTest.java | New unit tests covering nonce caching, htu normalization, token endpoint retry, key/alg validation, and Bearer fallback. |
| sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.java | Adds tests for EC DPoP key behavior (separate RSA SRT signer) and missing platform_issuer failure when DPoP is explicitly requested. |
| sdk/src/test/java/io/opentdf/platform/sdk/DPoPRetryInterceptorTest.java | New tests for OkHttp 401 nonce-challenge retry, single-retry guarantee, concurrency, and Bearer downgrade behavior. |
| sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt | Adds Connect unary response nonce caching + OkHttp DPoP nonce retry interceptor and debug logging summaries. |
| sdk/src/main/java/io/opentdf/platform/sdk/TokenSource.java | Introduces DPoP/Bearer scheme snapshotting, JWK-based DPoP key support, nonce cache, token endpoint retry, and improved error shaping. |
| sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java | Adds DPoP key/algorithm configuration, resolves EC-vs-RSA DPoP/SRT key material, disables Connect-GET for authenticated requests, wires in retry interceptor. |
| sdk/src/main/java/io/opentdf/platform/sdk/DpopKeyValidation.java | New centralized validation for DPoP JWK + algorithm compatibility (RSA/EC) and EC curve→alg inference. |
| sdk/pom.xml | Ensures Kotlin test sources are included in test compilation. |
| cmdline/src/test/java/io/opentdf/platform/CommandTest.java | New CLI tests for supports behavior, JSON output, and ensuring supports does not require credentials. |
| cmdline/src/main/resources/log4j2.xml | Lowers default CLI logging verbosity (root from trace → info). |
| cmdline/src/main/java/io/opentdf/platform/TDF.java | Adds execution exception handler and verbose stack-trace behavior for the CLI entrypoint. |
| cmdline/src/main/java/io/opentdf/platform/Command.java | Adds supports subcommand, makes credentials options non-required (enforced in buildSDK()), adds --verbose, and refactors some parsing. |
| cmdline/pom.xml | Adds runtime BouncyCastle dependency and JUnit/AssertJ test dependencies. |
| AGENTS.md | Documents local JAVA_HOME setup guidance for the agent environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- SDKBuilder: extract DPoP/SRT key resolution out of buildServices into a resolveDpopMaterial() helper + DpopMaterial holder, cutting the method's cognitive complexity below the threshold (S3776). No behavior change. - Tests: drop a redundant 'public' modifier (S5786) and two unthrowable 'throws Exception' declarations (S1130), document the intentionally empty catch (S108), and hoist URL/URI construction out of five assertThatThrownBy lambdas so each has a single throwing call (S5778).
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
July 14, 2026 17:46
6ae85b2 to
f16063b
Compare
Contributor
dmihalcik-virtru
marked this pull request as ready for review
July 14, 2026 18:10
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cmdline/src/main/java/io/opentdf/platform/Command.java (1)
384-385: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
Files.readStringfor reading text files.Since the SDK targets Java 11, you can use
Files.readString(Path)instead ofnew String(Files.readAllBytes(Path)). The latter relies on the platform's default charset, whileFiles.readStringexplicitly defaults toUTF-8and is more idiomatic.♻️ Proposed refactor
- String fileJson = new String(Files.readAllBytes(Paths.get(assertionVerificationInput))); + String fileJson = Files.readString(Paths.get(assertionVerificationInput));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmdline/src/main/java/io/opentdf/platform/Command.java` around lines 384 - 385, Update the file-reading logic that supplies fileJson to gson.fromJson in Command so it uses Files.readString(Path) instead of constructing a String from Files.readAllBytes. Preserve UTF-8 text decoding and the existing Config.AssertionVerificationKeys deserialization behavior.sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java (1)
351-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate default-algorithm logic between
resolveDpopMaterial()andresolveDpopAlgorithm().The auto-generated-key branch reimplements a subset of
resolveDpopAlgorithm's default logic inline instead of delegating to it. Functionally equivalent today (generated key is always RSA), but the two defaulting paths can silently diverge if the default policy changes later.♻️ Proposed fix to reuse the single source of truth
private DpopMaterial resolveDpopMaterial() { if (this.dpopKey == null) { // Auto-generate a single RSA-2048 key used for both DPoP and SRT. RSAKey generated = generateSrtRsaKey("Error generating DPoP key"); - JWSAlgorithm alg = this.dpopAlg != null ? this.dpopAlg : JWSAlgorithm.RS256; - return new DpopMaterial(generated, alg, generated); + return new DpopMaterial(generated, resolveDpopAlgorithm(generated), generated); }Also applies to: 366-371
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java` around lines 351 - 357, Update resolveDpopMaterial() to obtain the algorithm by delegating to resolveDpopAlgorithm() instead of duplicating the dpopAlg-null fallback inline, including the corresponding logic in the alternate branch around the referenced lines. Preserve the generated RSA key handling while ensuring all DPoP algorithm defaulting uses resolveDpopAlgorithm() as the single source of truth.sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt (1)
31-32: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDebug-log arguments are evaluated on every call, not only when DEBUG is enabled.
SLF4J's
logger.debug("...{}", arg)defers message formatting, but Kotlin/Java still evaluates the arguments eagerly before the call.dpopSummary(...)parses the DPoP JWT (SignedJWT.parse) andauthScheme(...)runs on every stream/unary request, every unary/okhttp response, and every retry — regardless of whether DEBUG is actually enabled. In production (INFO/WARN), this is unconditional JWT-parsing overhead on the hot authenticated-request path for no benefit.⚡ Proposed fix (guard with isDebugEnabled)
- logger.debug("DPoP path=stream url={} method=POST authScheme={} {}", - request.url, authScheme(authHeaders.authHeader), dpopSummary(authHeaders.dpopHeader)) + if (logger.isDebugEnabled) { + logger.debug("DPoP path=stream url={} method=POST authScheme={} {}", + request.url, authScheme(authHeaders.authHeader), dpopSummary(authHeaders.dpopHeader)) + }Apply the same
if (logger.isDebugEnabled) { ... }guard at the other three call sites (unary request, okhttp path, okhttp retry).Also applies to: 60-63, 114-117, 133-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt` around lines 31 - 32, Guard the debug logging call sites in the interceptor—including the shown stream path and the unary request, okhttp response, and okhttp retry paths— with logger.isDebugEnabled before evaluating authScheme(...) and dpopSummary(...). Keep the existing log messages and behavior unchanged when DEBUG is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/TokenSource.java`:
- Around line 213-324: Reduce cognitive complexity in getToken() by extracting
request construction/sending into sendTokenRequest(String cachedNonce), the
use_dpop_nonce retry flow into retryWithNonce(String dpopNonce), and settled
success-response field assignment into applyTokenResponse(TokenResponse, URL
tokenEndpointUrl). Keep getToken() as a short orchestration method while
preserving nonce caching, retry behavior, error handling, and token field
updates.
- Around line 241-271: Update the nonce-retry handling in the token request flow
around TokenRequest and tokenResponse so that a DPoP-Nonce returned on the
retried response is cached before the final error is thrown. Preserve the
existing single retry behavior, and ensure the latest nonce from either response
is stored for the next getToken() call.
---
Nitpick comments:
In `@cmdline/src/main/java/io/opentdf/platform/Command.java`:
- Around line 384-385: Update the file-reading logic that supplies fileJson to
gson.fromJson in Command so it uses Files.readString(Path) instead of
constructing a String from Files.readAllBytes. Preserve UTF-8 text decoding and
the existing Config.AssertionVerificationKeys deserialization behavior.
In `@sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java`:
- Around line 351-357: Update resolveDpopMaterial() to obtain the algorithm by
delegating to resolveDpopAlgorithm() instead of duplicating the dpopAlg-null
fallback inline, including the corresponding logic in the alternate branch
around the referenced lines. Preserve the generated RSA key handling while
ensuring all DPoP algorithm defaulting uses resolveDpopAlgorithm() as the single
source of truth.
In `@sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt`:
- Around line 31-32: Guard the debug logging call sites in the
interceptor—including the shown stream path and the unary request, okhttp
response, and okhttp retry paths— with logger.isDebugEnabled before evaluating
authScheme(...) and dpopSummary(...). Keep the existing log messages and
behavior unchanged when DEBUG is enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 975d89e9-d720-42b9-87a1-bd004126e193
📒 Files selected for processing (15)
AGENTS.mdcmdline/pom.xmlcmdline/src/main/java/io/opentdf/platform/Command.javacmdline/src/main/java/io/opentdf/platform/TDF.javacmdline/src/main/resources/log4j2.xmlcmdline/src/test/java/io/opentdf/platform/CommandTest.javasdk/pom.xmlsdk/src/main/java/io/opentdf/platform/sdk/DpopKeyValidation.javasdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.javasdk/src/main/java/io/opentdf/platform/sdk/TokenSource.javasdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.ktsdk/src/test/java/io/opentdf/platform/sdk/DPoPRetryInterceptorTest.javasdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.javasdk/src/test/java/io/opentdf/platform/sdk/TokenSourceTest.javasdk/src/test/kotlin/io/opentdf/platform/sdk/AuthInterceptorConnectPathTest.kt
mkleene
previously approved these changes
Jul 16, 2026
mkleene
left a comment
mkleene
left a comment
Contributor
There was a problem hiding this comment.
Looks great! Really thorough tests and worked around some tricky issues.
- TokenSource.getToken: split into sendTokenRequest/retryWithNonce/ applyTokenResponse/nonceForRetry to clear the SonarCloud cognitive-complexity gate; cache the DPoP-Nonce rotated on a failed retry response so a double rotation self-heals on the next call (RFC 9449 §8.2). - SDKBuilder: replace the standalone dpopAlgorithm() setter with paired dpopKey(JWK) (infers algorithm) and dpopKey(JWK, JWSAlgorithm) overloads; resolveDpopMaterial delegates to resolveDpopAlgorithm. - AuthInterceptor: drop the ThreadLocal URL handoff for a per-call closure-local and create a fresh interceptor per connect call (per-request contract); guard debug logging with isDebugEnabled. - cmdline: use Files.readString for UTF-8 file reads. - Tests: cover failed-retry nonce caching, per-call URL isolation, and the two-arg dpopKey overload.
dmihalcik-virtru
force-pushed
the
DSPX-3397-java-sdk
branch
from
July 18, 2026 01:24
f16063b to
a47fc9f
Compare
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmdline/src/main/java/io/opentdf/platform/Command.java (1)
327-344: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject whitespace-only credential values.
isEmpty()still lets" "through forplatformEndpoint,clientId, andclientSecret, so invalid input reaches SDK construction instead of the usage error. UseisBlank()or trim before checking all three values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmdline/src/main/java/io/opentdf/platform/Command.java` around lines 327 - 344, Update the credential validation in Command so platformEndpoint, clientId, and clientSecret reject whitespace-only values by using isBlank() or equivalent trimming checks. Preserve the existing ParameterException messages and normal validation behavior for null and genuinely non-empty values.Source: Coding guidelines
sdk/src/main/java/io/opentdf/platform/sdk/TokenSource.java (1)
188-196: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the raw path when stripping query and fragment.
getPath()decodes escaped reserved characters, so/kas/a%2Fbbecomes/kas/a/bin the proof. The resource server then validates a differenthtuand rejects the DPoP proof.Proposed fix
private static URI htuOf(URI uri) { if (uri.getRawQuery() == null && uri.getRawFragment() == null) { return uri; } - try { - return new URI(uri.getScheme(), uri.getAuthority(), uri.getPath(), null, null); - } catch (URISyntaxException e) { - throw new SDKException("failed to normalize URI for DPoP htu claim: " + uri, e); - } + String rawUri = uri.toASCIIString(); + int query = rawUri.indexOf('?'); + int fragment = rawUri.indexOf('#'); + int end = rawUri.length(); + if (query >= 0) { + end = Math.min(end, query); + } + if (fragment >= 0) { + end = Math.min(end, fragment); + } + return URI.create(rawUri.substring(0, end)); }Add coverage for a URL such as
/kas/a%2Fb?x=1#fragment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/io/opentdf/platform/sdk/TokenSource.java` around lines 188 - 196, Update htuOf(URI) to preserve the URI’s raw path when removing query and fragment components; use getRawPath() rather than getPath() while constructing the normalized URI, retaining escaped sequences such as %2F. Add coverage for /kas/a%2Fb?x=1#fragment and verify the resulting htu excludes query and fragment but preserves the raw path.sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.java (1)
239-263: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that the supplied RSA key is actually reused.
Checking only
alg() == "RS256"also passes if the builder silently generates another RSA key. Sign test data and verify the signature usingrsaDpopKey.toRSAPublicKey()to pin the intended reuse behavior.Suggested assertion
assertThat(sdk.getSrtSigner()).isPresent(); assertThat(sdk.getSrtSigner().get().alg()).isEqualTo("RS256"); + + byte[] payload = new byte[]{1, 2, 3}; + byte[] signature = sdk.getSrtSigner().get().sign(payload); + java.security.Signature verifier = java.security.Signature.getInstance("SHA256withRSA"); + verifier.initVerify(rsaDpopKey.toRSAPublicKey()); + verifier.update(payload); + assertThat(verifier.verify(signature)).isTrue();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.java` around lines 239 - 263, Strengthen rsaDpopKeyReusesSameKeyForSrt by signing representative test data with sdk.getSrtSigner() and verifying that signature with rsaDpopKey.toRSAPublicKey(). Keep the existing RS256 algorithm assertion, and ensure the verification specifically proves the supplied RSA key, rather than a newly generated key, was reused.sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt (1)
27-31: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPreserve existing request headers.
Both
streamFunctionandunaryFunctioninitializerequestHeadersas an empty map, which silently drops any existing headers (such asconnect-protocol-version, tracing identifiers, or user-supplied metadata) present inrequest.headers.
sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt#L27-L31: initializerequestHeaderswithrequest.headers.toMutableMap()instreamFunctioninstead of an empty map.sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt#L58-L62: initializerequestHeaderswithrequest.headers.toMutableMap()inunaryFunctionas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt` around lines 27 - 31, The streamFunction and unaryFunction paths discard existing request headers by starting with empty maps. In sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt lines 27-31 and 58-62, initialize each requestHeaders map from request.headers.toMutableMap(), then preserve the existing Authorization and optional DPoP header assignments.
🧹 Nitpick comments (1)
sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt (1)
122-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant header fallback and duplicate nonce caching.
OkHttp and Connect-Kotlin natively handle header case-insensitivity (or lowercase them explicitly), making the explicit
"DPoP-Nonce"fallbacks redundant. Additionally, the retry flow caches the exact same nonce twice.
sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt#L122-L125: remove the?: response.header("DPoP-Nonce")fallback, and remove the redundantts.cacheNonce(url, dpopNonce)sincecacheNonceIfPresentat line 112 already cached it.sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt#L83-L84: remove the?: resp.headers["DPoP-Nonce"]?.firstOrNull()fallback inunaryFunction.sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt#L166-L166: remove the?: response.header("DPoP-Nonce")fallback incacheNonceIfPresent.sdk/src/test/kotlin/io/opentdf/platform/sdk/AuthInterceptorConnectPathTest.kt#L91-L91: update the manually constructed mock response in the test to use the lowercase"dpop-nonce"key so it correctly simulates the Connect-Kotlin parser and passes without the fallback.sdk/src/test/kotlin/io/opentdf/platform/sdk/AuthInterceptorConnectPathTest.kt#L126-L127: similarly update the mock responses in the concurrent isolation test to use"dpop-nonce".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt` around lines 122 - 125, Remove redundant case-variant DPoP nonce header fallbacks in AuthInterceptor.unaryFunction, the retry flow, and cacheNonceIfPresent, and remove the duplicate ts.cacheNonce call after cacheNonceIfPresent has already cached the nonce. In AuthInterceptorConnectPathTest, update the manually constructed responses in both affected tests to use the lowercase dpop-nonce header key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/TokenSource.java`:
- Around line 350-352: Update the token expiry assignment in TokenSource so a
refreshed token with lifetime 0 clears or resets tokenExpiryTime instead of
retaining the previous expired value; preserve the existing one-third-lifetime
calculation for tokens with a nonzero lifetime.
---
Outside diff comments:
In `@cmdline/src/main/java/io/opentdf/platform/Command.java`:
- Around line 327-344: Update the credential validation in Command so
platformEndpoint, clientId, and clientSecret reject whitespace-only values by
using isBlank() or equivalent trimming checks. Preserve the existing
ParameterException messages and normal validation behavior for null and
genuinely non-empty values.
In `@sdk/src/main/java/io/opentdf/platform/sdk/TokenSource.java`:
- Around line 188-196: Update htuOf(URI) to preserve the URI’s raw path when
removing query and fragment components; use getRawPath() rather than getPath()
while constructing the normalized URI, retaining escaped sequences such as %2F.
Add coverage for /kas/a%2Fb?x=1#fragment and verify the resulting htu excludes
query and fragment but preserves the raw path.
In `@sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt`:
- Around line 27-31: The streamFunction and unaryFunction paths discard existing
request headers by starting with empty maps. In
sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt lines 27-31 and
58-62, initialize each requestHeaders map from request.headers.toMutableMap(),
then preserve the existing Authorization and optional DPoP header assignments.
In `@sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.java`:
- Around line 239-263: Strengthen rsaDpopKeyReusesSameKeyForSrt by signing
representative test data with sdk.getSrtSigner() and verifying that signature
with rsaDpopKey.toRSAPublicKey(). Keep the existing RS256 algorithm assertion,
and ensure the verification specifically proves the supplied RSA key, rather
than a newly generated key, was reused.
---
Nitpick comments:
In `@sdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.kt`:
- Around line 122-125: Remove redundant case-variant DPoP nonce header fallbacks
in AuthInterceptor.unaryFunction, the retry flow, and cacheNonceIfPresent, and
remove the duplicate ts.cacheNonce call after cacheNonceIfPresent has already
cached the nonce. In AuthInterceptorConnectPathTest, update the manually
constructed responses in both affected tests to use the lowercase dpop-nonce
header key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3951f4b-9880-40f8-8ead-b5f2fd3a95a8
📒 Files selected for processing (9)
cmdline/pom.xmlcmdline/src/main/java/io/opentdf/platform/Command.javacmdline/src/test/java/io/opentdf/platform/CommandTest.javasdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.javasdk/src/main/java/io/opentdf/platform/sdk/TokenSource.javasdk/src/main/kotlin/io/opentdf/platform/sdk/AuthInterceptor.ktsdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.javasdk/src/test/java/io/opentdf/platform/sdk/TokenSourceTest.javasdk/src/test/kotlin/io/opentdf/platform/sdk/AuthInterceptorConnectPathTest.kt
💤 Files with no reviewable changes (1)
- cmdline/pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- cmdline/src/test/java/io/opentdf/platform/CommandTest.java
- sdk/src/test/java/io/opentdf/platform/sdk/TokenSourceTest.java
Comment on lines
+350
to
352
| if (token.getLifetime() != 0) { | ||
| this.tokenExpiryTime = Instant.now().plusSeconds(token.getLifetime() / 3); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset expiry when the refreshed token omits a lifetime.
If the previous token expired and the replacement has lifetime 0, the old, expired tokenExpiryTime remains. Every subsequent request then fetches another token.
Proposed fix
- if (token.getLifetime() != 0) {
- this.tokenExpiryTime = Instant.now().plusSeconds(token.getLifetime() / 3);
- }
+ this.tokenExpiryTime = token.getLifetime() == 0
+ ? null
+ : Instant.now().plusSeconds(token.getLifetime() / 3);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (token.getLifetime() != 0) { | |
| this.tokenExpiryTime = Instant.now().plusSeconds(token.getLifetime() / 3); | |
| } | |
| this.tokenExpiryTime = token.getLifetime() == 0 | |
| ? null | |
| : Instant.now().plusSeconds(token.getLifetime() / 3); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/main/java/io/opentdf/platform/sdk/TokenSource.java` around lines 350
- 352, Update the token expiry assignment in TokenSource so a refreshed token
with lifetime 0 clears or resets tokenExpiryTime instead of retaining the
previous expired value; preserve the existing one-third-lifetime calculation for
tokens with a nonzero lifetime.
Member
Author
|
@/tmp/r_nits.md |
Contributor
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
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)
Green e2e Test Run: https://github.com/opentdf/tests/actions/runs/29624686479
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 is always-on for any authenticated client, so the CLI needs no DPoP flags:
encrypt,decrypt, and metadata commands are sender-constrained without any opt-in.--dpop/--dpop-keyflags are exposed (an earlier revision added them; they were removed together withCliDpopOptionssince they served no purpose once DPoP is unconditional). This mirrors the go-sdk direction in feat(sdk): add DPoP client support with HTTP RoundTripper (DSPX-3397) platform#3581, which likewise dropped otdfctl's--dpopparameters.supportssubcommand (no help-text scraping required):tdf supports <feature>exits0if supported,1otherwise.tdf supportswith no argument lists all supported feature names, one per line.--jsonemits a JSON object mapping feature name → boolean, e.g.{"dpop":true}or, with no argument,{"dpop":true,"dpop_nonce_challenge":true}.Feature Detection
Testing
New coverage:
TokenSourceTest,DPoPRetryInterceptorTest,AuthInterceptorConnectPathTest(Kotlin), andCommandTest— covering nonce caching/origin isolation, both retry paths, EC/RSA key handling, and thesupportssubcommand (bare exit codes, no-arg listing, and--jsonoutput).Related PRs
cli.shto detectdpopviasupports dpopinstead of grepping the removed--dpopflag.Checklist
--dpop/--dpop-key+CliDpopOptions)supportsfeature detection: bare exit code, no-arg listing,--jsonSummary by CodeRabbit
supportsCLI subcommand (plain list or--jsonfeature support results with canonical casing).-v/--verbosefor more detailed error output.use_dpop_nonce(with correct Bearer downgrade).JAVA_HOMEvia Homebrew.