feat(sdk): add DPoP client support with HTTP RoundTripper (DSPX-3397)#3581
Draft
dmihalcik-virtru wants to merge 12 commits into
Draft
feat(sdk): add DPoP client support with HTTP RoundTripper (DSPX-3397)#3581dmihalcik-virtru wants to merge 12 commits into
dmihalcik-virtru wants to merge 12 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE 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 |
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
Implements RFC 9449 DPoP (Demonstrating Proof-of-Possession) for the Go SDK: - Add DPoPTransport as an http.RoundTripper that wraps any transport - Generate ES256/RS256 proofs with jti, htm, htu, iat claims for all requests - Add ath claim (access token hash) for resource endpoint calls - Handle server-issued DPoP-Nonce challenges with automatic retry - Cache nonces per-origin and refresh from successful responses - Normalize URIs per RFC 9449 (lowercase scheme/host, strip default ports) - Integrate into SDK's HTTP client construction via NewDPoPHTTPClient - Add SupportedFeatures() function for xtest feature detection All requests through the SDK now include DPoP proofs when credentials are configured. Token endpoint requests omit the ath claim; resource requests include both Authorization: DPoP <token> header and the DPoP proof header. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…PX-3397) Exposes DPoP algorithm/key selection via CLI flags on `otdfctl encrypt` and `otdfctl decrypt`, supporting ES256 (default), ES384, ES512, RS256, RS384, and RS512. Bare `--dpop` defaults to ES256 per RFC 9449 §4.2. `--dpop-key <path>` loads a PEM private key (algorithm inferred from key type). Both flags can be combined to override the inferred algorithm. SDK changes: - Add sdk/dpop_key.go: generateDPoPKeyForAlg, loadDPoPKeyFromPEM, resolveDPoPKey helpers - Add WithDPoPAlgorithm, WithDPoPKeyPEM, WithDPoPJWK SDK options - Thread custom JWK through buildIDPTokenSource and DPoPTransport setup; falls back to auto-generated RSA when no custom key is configured - Add JWK-accepting token source constructors for all four source types otdfctl changes: - Register --dpop (NoOptDefVal="ES256") and --dpop-key flags on encrypt/decrypt; update man docs accordingly - handlers.WithExtraSDKOpts appends (not replaces) SDK options - common.NewHandler accepts variadic extraSDKOpts (backward compatible) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Fixes critical and high-priority issues identified in PR review: - Fix request body consumed on retry: reset body using GetBody() before retrying - Fix data races: use local base variable instead of modifying t.Base - Fix nonce cache initialization: unconditional lock instead of double-checked lock - Fix missing HTTP client for token source: pass client with base transport to preserve custom configs - Optimize token endpoint URL parsing: cache parsed URL to avoid parsing on every request - Normalize origin casing: lowercase origin in cache to ensure consistent hits on uppercase URLs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
- Fix errcheck: use comma-ok for type assertion in dpop_transport_test.go - Fix govet shadow: rename inner ok vars (isStr, athOK, jtiOK) to avoid shadowing outer ok declaration in TestDPoPTransport_AddsProofToRequests - Fix nestif in RoundTrip: extract 401 nonce-retry into retryWithNonce method - Fix nestif in sdk.go New: extract DPoP key selection into pickDPoPKey helper Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
ConnectRPC/gRPC clients set req.Body and ContentLength but not req.GetBody. After the initial round trip consumed the body, the DPoP-Nonce retry path would clone the original request, inherit the exhausted reader, and net/http would abort with "ContentLength=N with Body length 0". Buffer the body on the request clone and install GetBody so the retry path can replay it. Mutates only the clone, preserving the http.RoundTripper contract. Adds a regression test that exercises the retry path with a POST body and no GetBody — the scenario every otdfctl ConnectRPC call hits when the platform has server.auth.dpop.require_nonce enabled. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Covers the production path that broke every body-bearing otdfctl call when the platform enables the DPoP-Nonce challenge (RFC 9449 §8): Connect-go's payloadCloser must replay the request body on the nonce retry, otherwise net/http aborts with 'ContentLength=N with Body length 0'. The existing TestDPoPTransport_NonceRetryReplaysBodyWithoutGetBody exercises the raw http.Request path. This new test drives a real kasconnect unary client through DPoPTransport against a stub that returns 401 + DPoP-Nonce, then asserts the two captured request bodies are byte-for-byte equal — the production failure mode that the raw-http test cannot reach. Confirmed it fails (with the exact 'ContentLength=15 with Body length 0' error) when the body-replay branch in retryWithNonce is reverted. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-3397-platform-go-sdk
branch
from
eb823ac to
a78a68f
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
bufferRequestBody previously skipped buffering whenever req.GetBody was already set, which is always true for ConnectRPC unary calls. That left the DPoP-nonce retry (and net/http's own connection-reuse retry) relying on ConnectRPC's GetBody, which hands back a single shared *payloadCloser with a mutable read offset. When net/http reuses a keep-alive connection the server has since closed (the auth interceptor returns the 401 DPoP-Nonce challenge without draining the request body), its internal rewind-and-retry races on that shared offset and presents an empty body, surfacing intermittently under load as 'net/http: HTTP/1.x transport connection broken: http: ContentLength=N with Body length 0'. Buffer unary bodies (ContentLength >= 0) into an SDK-owned immutable []byte and install a GetBody factory that returns an independent bytes.Reader per call, removing the shared mutable state. Streaming / unknown-length requests (ContentLength < 0) are left untouched so io.Pipe bodies are not drained. Confirmed by a behavior-preserving probe: ConnectRPC's GetBody returned the full payload on every retry (2880/2880, including the failing one) while net/http read 0 bytes from the handed-off body on the failure; no Go data race. ~13.7k retries at 48-64x concurrency: zero drops (reliably 1-5 per 2880 before). Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-3397-platform-go-sdk
branch
from
53958c4 to
cece089
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
… (DSPX-3397) Server-side DPoP validation only attached a WWW-Authenticate header for DPoPNonceError (use_dpop_nonce). Every other proof rejection (tampered htu, replayed jti, malformed nonce, bad ath, wrong htm) returned a bare 401 with no challenge header, so RFC 9449 §7.1-compliant clients/tests could not tell a DPoP failure from an unrelated 401. This made the opentdf/tests xtest negative cases unreliable: tampered_htu failed always (htu is checked before nonce), replayed_jti was flaky (gated on whether the cached nonce had rotated, deciding whether the nonce or jti check fired first), and tampered_nonce failed whenever a non-nonce check tripped first. Add a DPoPProofError marker type (delegating Error, Unwrap), wrap all non-nonce validateDPoP errors with it in checkToken, and have both MuxHandler and ConnectAuthNInterceptor emit `WWW-Authenticate: DPoP error="invalid_dpop_proof"` (plus a fresh DPoP-Nonce when require_nonce is on). The use_dpop_nonce path is unchanged. Add unit tests covering the error type, the malformed-nonce wrap contract, and the challenge headers for both handlers across nonce-on/off. 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
…SPX-3397) Enforcement lived at the top of the auth block (server.auth.enforceDPoP) while every other DPoP knob is nested under server.auth.dpop. Consolidate it: add server.auth.dpop.enforce and deprecate the old top-level field. The old field keeps working during the migration window. Both are defaulted bools, so mapstructure cannot distinguish "explicitly false" from "unset"; enforcement therefore uses OR semantics via a new dpopEnforced() helper (DPoP.Enforce || EnforceDPoP), and config validation warns when the deprecated field is set. - config.go: add DPoPConfig.Enforce, deprecate AuthNConfig.EnforceDPoP, add dpopEnforced(); update validateAuthNConfig warnings. - authn.go: NewAuthenticator uses cfg.dpopEnforced(). - server.go: warning strings reference server.auth.dpop.enforce. - example configs + docs/Configuring.md: use the nested dpop.enforce form; keep testdata/all-no-config.yaml on the legacy key for back-compat coverage. - tests: migrate to DPoP.Enforce and add TestDPoPEnforcement_Migration. 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
The DPoP nonce challenge only applies to DPoP-bound requests; without enforcement a plain Bearer token bypasses DPoP validation and never sees a challenge. When dpop-challenge-enabled is set, also set server.auth.dpop.enforce alongside require_nonce in both start actions. The flag only ever turns enforcement on: start-additional-kas uses with(select(...)) so it never writes enforce: false (preserving any base value), and start-up-with-containers sets it inside the step already gated on the flag. 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 enforcement and the nonce-challenge flow are separate concerns. Replace the coupling (where dpop-challenge-enabled also set server.auth.dpop.enforce) with a dedicated dpop-enforce-required input (default false) that drives enforcement on its own. dpop-challenge-enabled again sets only require_nonce. The enforce knob only ever turns enforcement on: start-additional-kas uses with(select(...)) keyed on DPOP_ENFORCE_REQUIRED, and start-up-with-containers sets it in a new step gated on the flag, so enforce: false is never written. 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
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
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.
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.yaml