feat(sdk): add DPoP client support with HTTP RoundTripper (DSPX-3397)#3581
Open
dmihalcik-virtru wants to merge 24 commits into
Open
feat(sdk): add DPoP client support with HTTP RoundTripper (DSPX-3397)#3581dmihalcik-virtru wants to merge 24 commits into
dmihalcik-virtru wants to merge 24 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DPoP configuration, transport, and SDK/client wiring for token-bound requests, plus CLI flag plumbing and host-origin normalization in service auth. ChangesDPoP Support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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 |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements RFC 9449 DPoP (Demonstrating Proof-of-Possession) client support for the OpenTDF Go SDK. By introducing a custom HTTP RoundTripper, the SDK can now generate and attach DPoP proofs to HTTP requests, handle server-side nonce challenges, and perform URI normalization. This work is a key component of the broader Keycloak v26 upgrade, ensuring secure, proof-of-possession-based authentication for HTTP-based interactions within the platform. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The proof is shown in token light, With DPoP we do it right. No replay here, the nonce is set, A secure path for the internet. Footnotes
|
github-actions
Bot
added
comp:sdk
![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 RFC 9449 DPoP (Demonstrating Proof-of-Possession) support to the SDK by adding a new DPoPTransport and integrating it into the client setup. The code review identified several critical and high-severity issues in the transport implementation, including a potential bug where request bodies are consumed and not reset on retry, concurrency data races on shared fields like t.Base and t.nonceCache, and the bypass of custom transport configurations when retrieving access tokens. Additionally, optimizations were suggested to cache parsed token endpoint URLs and normalize URL origins to lowercase to prevent cache misses.
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.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
DSPX-3397-platform-go-sdk
branch
from
441af7b to
61316ef
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
DSPX-3397-platform-go-sdk
branch
from
ebc3e40 to
37ed377
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
DSPX-3397-platform-go-sdk
branch
from
37ed377 to
b9dd8d8
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
DSPX-3397-platform-go-sdk
branch
from
5c4f57a to
4cec258
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Replace the two overlapping DPoP key selectors (resolveDPoPKey and pickDPoPKey) with a single pure resolveDPoPKey that follows one documented priority: dpopJWK -> dpopKeyPEM -> dpopAlgorithm -> dpopKey(RSA) -> nil. resolveDPoPKey no longer mutates c.dpopJWK. The previous read-with-hidden-write cache only existed to share the ephemeral (dpopAlgorithm) key between the token source and the DPoP transport, coupling them by call order. Instead the key is now resolved once in buildIDPTokenSource and returned so SDK.New gives the transport the same key the token source binds to. buildIDPTokenSource now builds every flow via buildIDPTokenSourceFromJWK (auto-generating the default RSA key and converting it with getDPoPJWK), and a custom access token source combined with any DPoP key option now consistently drives the transport. Signed-off-by: Dave Mihalcik <[email protected]>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
normalizeURI stripped default ports with strings.HasSuffix on the raw host, which is brittle around IPv6 literals (e.g. [fe80::443]). Use u.Hostname()/u.Port() compared against the scheme default instead, via a shared normalizedHostPort helper reused by getOrigin so the nonce-cache key and htu claim normalize consistently. isTokenEndpointRequest now compares normalized URIs so an explicit :443 matches a portless endpoint. Signed-off-by: Dave Mihalcik <[email protected]>
Parse the request Host with url.Hostname()/Port() instead of trimming a ":443"/":80" suffix, so IPv6 literals and hosts that merely end in the default port are handled correctly. Lowercase the host to match the SDK's htu normalization, since the DPoP htu match is an exact string comparison. Signed-off-by: Dave Mihalcik <[email protected]>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
sdk/sdk.go (1)
205-228:⚠️ Potential issue | 🟠 Major
cfg.coreConnstill bypasses the wrapped client and auth interceptors.The client/interceptor pipeline built above never reaches the
cfg.coreConnbranch, so resource calls still go out throughcfg.coreConn.Clientwithout the DPoP transport, and custom token sources without DPoP also miss theAuthorizationinterceptor. BuildplatformConnfrom the supplied endpoint/options but swap in the wrapped client, usingcfg.coreConn.Clientas the DPoP base when present.Suggested fix
- httpClient := cfg.httpClient + baseHTTPClient := cfg.httpClient + if cfg.coreConn != nil && cfg.coreConn.Client != nil { + baseHTTPClient = cfg.coreConn.Client + } + httpClient := baseHTTPClient dpopHandledByTransport := false if accessTokenSource != nil && dpopKey != nil { - httpClient = auth.NewDPoPHTTPClient(cfg.httpClient, dpopKey, accessTokenSource, cfg.tokenEndpoint) + httpClient = auth.NewDPoPHTTPClient(baseHTTPClient, dpopKey, accessTokenSource, cfg.tokenEndpoint) dpopHandledByTransport = true } @@ if cfg.coreConn != nil { - platformConn = cfg.coreConn + platformConn = &ConnectRPCConnection{ + Endpoint: cfg.coreConn.Endpoint, + Client: httpClient, + Options: append(cfg.coreConn.Options, connect.WithInterceptors(uci...)), + } } else {🤖 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/sdk.go` around lines 205 - 228, The `cfg.coreConn` branch in `sdk.go` is bypassing the wrapped `httpClient` and the auth interceptor setup created in this block. Update the `platformConn` construction so it always uses the same client/auth pipeline: when `cfg.coreConn` is present, keep its endpoint/options but replace its client with the wrapped client, and use `cfg.coreConn.Client` as the DPoP base when applicable. Ensure the `ConnectRPCConnection` path and the `cfg.coreConn` path both flow through the same `auth.NewDPoPHTTPClient` / `NewTokenAddingInterceptorWithClient` behavior.
🤖 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/auth/dpop_transport.go`:
- Around line 278-284: The URI normalization in normalizeURI currently uses
u.Path, which can decode escaped bytes and change the DPoP htu value. Update
normalizeURI in dpop_transport.go to preserve the original escaped path by using
u.EscapedPath() when building the normalized URI, while keeping the existing
scheme/host normalization and query/fragment stripping. Also add a test case
covering an escaped reserved path such as /a%2Fb to verify the normalized output
keeps the escape intact.
In `@sdk/sdk.go`:
- Around line 302-311: The credential setup logic in the SDK currently returns
the uncredentialed-client fast path before checking `certExchange` and
`tokenExchange`, which causes those exchange-based configurations to be ignored
and masks the dual-config error. Update the credential resolution flow in the
relevant SDK auth setup method so the `certExchange`/`tokenExchange` validation
and token-source creation run before the `clientCredentials == nil &&
oauthAccessTokenSource == nil` return. Keep the DPoP warning and uncredentialed
fallback only after exchange configurations have been handled.
---
Duplicate comments:
In `@sdk/sdk.go`:
- Around line 205-228: The `cfg.coreConn` branch in `sdk.go` is bypassing the
wrapped `httpClient` and the auth interceptor setup created in this block.
Update the `platformConn` construction so it always uses the same client/auth
pipeline: when `cfg.coreConn` is present, keep its endpoint/options but replace
its client with the wrapped client, and use `cfg.coreConn.Client` as the DPoP
base when applicable. Ensure the `ConnectRPCConnection` path and the
`cfg.coreConn` path both flow through the same `auth.NewDPoPHTTPClient` /
`NewTokenAddingInterceptorWithClient` behavior.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21b6a8bc-fc01-414c-aa6f-aa3b92fb9086
📒 Files selected for processing (8)
AGENTS.mdsdk/auth/dpop_transport.gosdk/auth/dpop_transport_test.gosdk/dpop_key.gosdk/dpop_key_test.gosdk/sdk.goservice/internal/auth/authn.goservice/internal/auth/authn_test.go
Wrap the caller-supplied oauth2.TokenSource in oauth2.ReuseTokenSource so a valid token is reused across calls instead of re-fetched. AccessToken is on the request hot path (once per gRPC/Connect call, plus once more to compute the DPoP ath claim), so an uncached source risked an IdP round-trip on every request. Wrapping an already-caching source is harmless. Signed-off-by: Dave Mihalcik <[email protected]>
Contributor
- transport: fall through on successful nonce retry so a rotated DPoP-Nonce is cached instead of dropped by the early return - transport: normalize htu with EscapedPath to preserve percent-encoded bytes - dpop_key: reject public-only keys, ES*/RS* family mismatches, and EC curve/alg mismatches at resolution time via validateDPoPKey - sdk: surface conflicting exchange config before the uncredentialed fast-path - sdk/otdfctl: apply DPoP options during credential validation via NewDPoPValidationHTTPClient so pre-flight token requests carry a proof Signed-off-by: Dave Mihalcik <[email protected]>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: Dave Mihalcik <[email protected]>
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/sdk.go (1)
346-354: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the intended ES256/P-256 default DPoP key.
This fallback still generates an RSA/RS256 key, while the PR objective says the SDK should default to an ephemeral P-256 ES256 key when none is supplied. That can bind tokens with a different algorithm than callers expect.
🤖 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/sdk.go` around lines 346 - 354, The DPoP fallback in sdk.go is still generating an RSA key pair instead of the intended default ES256/P-256 key. Update the dpopKey nil branch in the SDK initialization path to create an ephemeral P-256 key for use with getDPoPJWK and store it on c.dpopKey, rather than calling ocrypto.NewRSAKeyPair. Keep the existing error handling pattern, but make sure the default algorithm and key type align with the DPoP expectations in this constructor logic.
🤖 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/auth/dpop_transport.go`:
- Around line 95-100: The retry path in retryWithNonce from dpop_transport.go
still attempts to resend requests even when the body cannot be replayed, which
can consume a one-shot body and fail incorrectly. Update the nonce retry
handling around the retryWithNonce call to detect non-replayable requests (for
example when GetBody is nil after bufferRequestBody) and cache the nonce without
retrying. In that case, return the original 401 response instead of calling the
resend path for non-replayable bodies.
In `@sdk/dpop_key_test.go`:
- Around line 204-267: Add a regression test for the dpopKeyPEM + dpopAlgorithm
override path in TestValidateDPoPKey. The current cases only cover invalid
dpopJWK inputs, so add a mismatch scenario where selectDPoPKey resolves an RSA
PEM key but dpopAlgorithm forces ES256 (or equivalent) and assert it fails with
the expected algorithm/key-type error. Use the existing test helpers and config
fields in resolveDPoPKey/selectDPoPKey to keep the coverage on the override
path.
---
Outside diff comments:
In `@sdk/sdk.go`:
- Around line 346-354: The DPoP fallback in sdk.go is still generating an RSA
key pair instead of the intended default ES256/P-256 key. Update the dpopKey nil
branch in the SDK initialization path to create an ephemeral P-256 key for use
with getDPoPJWK and store it on c.dpopKey, rather than calling
ocrypto.NewRSAKeyPair. Keep the existing error handling pattern, but make sure
the default algorithm and key type align with the DPoP expectations in this
constructor logic.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13ea97c6-ec83-4a89-8d11-e9872f97f52f
📒 Files selected for processing (10)
otdfctl/cmd/common/common.gootdfctl/pkg/auth/auth.gosdk/auth/dpop_transport.gosdk/auth/dpop_transport_test.gosdk/dpop_key.gosdk/dpop_key_test.gosdk/dpop_validation_client_test.gosdk/idp_oauth_access_token_source.gosdk/idp_oauth_access_token_source_test.gosdk/sdk.go
Contributor
The condition logic is intentionally written to be easily understood by readers: don't append the port if it matches the default for its scheme. De Morgan transformation would make this less readable. Signed-off-by: Dave Mihalcik <[email protected]>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
- dpop_transport: don't retry nonce challenge when body is non-replayable (streaming/unknown-length body consumed on first attempt); cache the nonce and return the original 401 instead of resending an empty body - sdk: default to an ephemeral ES256/P-256 DPoP key instead of RSA when none is configured, matching documented behavior - dpop_key_test: add regression case for RSA PEM overridden to ES256 Signed-off-by: Dave Mihalcik <[email protected]>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
pflynn-virtru
approved these changes
Jul 1, 2026
pflynn-virtru
left a comment
pflynn-virtru
left a comment
Member
There was a problem hiding this comment.
no security concerns found
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 RFC 9449 DPoP (Demonstrating Proof-of-Possession) client support for the OpenTDF Go SDK.
This PR is part of the larger Keycloak v26 upgrade and comprehensive DPoP support feature tracked in DSPX-3397.
e2e Test
Changes
DPoP RoundTripper Implementation
sdk/auth/dpop_transport.go: NewDPoPTransportthat implementshttp.RoundTripperjti,htm,htu,iat(always);ath(resource calls only);nonce(when challenged)Server-Issued Nonce Support
DPoP-Noncechallenges per RFC 9449 §8401withDPoP-Nonceheader: cache nonce, regenerate proof, retry once2xxresponsesSDK Integration
sdk/sdk.go: Wrap HTTP client with DPoP transport during SDK constructiongetDPoPJWK()helper to convertocrypto.RsaKeyPairtojwk.KeyNewDPoPHTTPClient()factory for wrapping clients with DPoP supportFeature Detection
sdk/version.go: AddSupportedFeatures()function returning["dpop", "connectrpc"]supports dpopprobeTesting
sdk/auth/dpop_transport_test.go: Comprehensive unit testsath) verificationRelated Work
This PR implements the Go SDK cell of the DPoP feature. Related PRs:
xtest/scenarios/DSPX-3397.yaml)Testing
All tests pass:
Linting clean:
Notes
oauth.goalready handles DPoP for token endpoint requeststoken_adding_interceptor.goalready handles DPoP for gRPC/Connecthttp.ClientJira: DSPX-3397
Test Scenario:
xtest/scenarios/DSPX-3397.yamlSummary by CodeRabbit
--dpopand--dpop-keyflags to encrypt and decrypt commands.