feat(sdk): add DPoP client support with HTTP RoundTripper (DSPX-3397)#3581
Open
dmihalcik-virtru wants to merge 27 commits into
Open
feat(sdk): add DPoP client support with HTTP RoundTripper (DSPX-3397)#3581dmihalcik-virtru wants to merge 27 commits into
dmihalcik-virtru wants to merge 27 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 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:
|
![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
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 <dmihalcik@virtru.com>
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 <dmihalcik@virtru.com>
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
pflynn-virtru
previously 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
Replace the raw string parameter in WithDPoPAlgorithm with a named SigningAlgorithm type (RFC 7518 §3.1) and export the six supported constants (ES256, ES384, ES512, RS256, RS384, RS512) from the sdk package. This guides correct use via IDE autocomplete and prevents untyped string variables from being passed without an explicit cast. Runtime validation via generateDPoPKeyForAlg's default error case is unchanged. Remove the private dpopAlgES*/RS* string constants from dpop_key.go in favour of the exported type. Update the otdfctl CLI caller to cast the flag string. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
bufferRequestBody replaced io.ReadAll with a ContentLength-pre-sized bytes.Buffer read, collapsing io.ReadAll's grow-by-doubling into a single exact allocation. Behavior is unchanged (ReadFrom still reads to EOF); this only trims allocation churn on the DPoP replay-buffering path. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Member
Author
|
@pflynn-virtru I made a couple of small changes |
pflynn-virtru
previously approved these changes
Jul 9, 2026
Contributor
|
Should we have clients upgrade to DPoP at the SDK level if the platform requires it? Some kind of well-known or published state that our SDKs are smart enough to handle? |
jakedoublev
reviewed
Jul 9, 2026
jakedoublev
reviewed
Jul 9, 2026
| - name: with-assertions | ||
| description: > | ||
| EXPERIMENTAL: JSON string or path to a JSON file of assertions to bind metadata to the TDF. See examples for more information. WARNING: Providing keys in a JSON string is strongly discouraged. If including sensitive keys, instead provide a path to a JSON file containing that information. | ||
| - name: dpop |
Contributor
There was a problem hiding this comment.
If these indeed are added as CLI flags, should they be relevant globally to every command or just encrypt/decrypt?
Member
Author
There was a problem hiding this comment.
These may apply to all authenticated connectRPC calls. The client will request DPoP enabled tokens when it hits the oauth server's access token endpoint.
Contributor
There was a problem hiding this comment.
Is there a way we can get that state into the SDK without a flag, then? Or is the flag mostly for testing purposes? I ask because it doesn't seem very user-friendly to have to pass a --dpop flag when I would want my CLI to figure that out for me as its own concern, and I would also want it to be done correctly on all API calls instead of just encrypt/decrypt if they would fail otherwise. Does that make sense?
Adding a variadic option parameter to NewHandler changed its concrete function type, which could break downstream integrators that reference common.NewHandler as a value. Restore the original NewHandler signature and move the variadic SDK-option support into a new NewHandlerWith, which NewHandler now delegates to. Update the encrypt/decrypt commands to call NewHandlerWith. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
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
3 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.