feat(platform): add comprehensive DPoP (RFC 9449) support (DSPX-3397)#3582
Open
dmihalcik-virtru wants to merge 13 commits into
Open
feat(platform): add comprehensive DPoP (RFC 9449) support (DSPX-3397)#3582dmihalcik-virtru wants to merge 13 commits into
dmihalcik-virtru wants to merge 13 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughKeycloak is upgraded to 26.2 with admin-fine-grained-authz enabled and a new ChangesDPoP nonce enforcement and Keycloak 26.2 upgrade
Sequence Diagram(s)sequenceDiagram
participant Client
participant MuxHandler
participant checkToken
participant validateDPoP
participant dpopNonceManager
Client->>MuxHandler: HTTP request (Authorization: DPoP <token>, DPoP: <proof>)
MuxHandler->>checkToken: extract scheme + token
checkToken->>validateDPoP: verify DPoP proof, htu via matchHTU
validateDPoP->>dpopNonceManager: ValidateNonce(nonce claim)
alt nonce missing or expired
dpopNonceManager-->>validateDPoP: DPoPNonceError
validateDPoP-->>checkToken: DPoPNonceError
checkToken-->>MuxHandler: DPoPNonceError
MuxHandler-->>Client: 401 + DPoP-Nonce: <fresh> + WWW-Authenticate: use_dpop_nonce
else nonce malformed (non-string)
dpopNonceManager-->>validateDPoP: DPoPNonceMalformedError
validateDPoP-->>MuxHandler: DPoPNonceMalformedError
MuxHandler-->>Client: 401 unauthenticated (no retry hint)
else valid nonce
dpopNonceManager-->>validateDPoP: ok
validateDPoP-->>MuxHandler: authenticated principal
MuxHandler-->>Client: 200 + DPoP-Nonce: <fresh>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 introduces full RFC 9449 DPoP support to the OpenTDF platform service. By integrating DPoP proof validation and server-issued nonce management, the changes significantly enhance the security of token-based authentication for both HTTP and gRPC interfaces. The implementation includes robust error handling for nonce challenges and ensures compatibility with existing authentication flows. 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. Tokens held with proof of key, RFC standards, plain to see. Nonces rotate, threats subside, Security with nowhere to hide. Footnotes
|
![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 server-issued DPoP nonce management per RFC 9449 §8, including configuration options, a nonce manager with rotation and validation, and interceptor integrations for both HTTP and Connect handlers. Feedback on these changes highlights two critical issues: a race condition in getCurrentNonce() that can cause concurrent double-rotation of nonces, and incorrect response header propagation in the Connect interceptor where metadata.AppendToOutgoingContext is used instead of setting headers on the response directly.
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
dmihalcik-virtru
force-pushed
the
DSPX-3397-platform-service
branch
from
2f36d01 to
d304bd7
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
jentfoo
reviewed
Jun 10, 2026
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-service
branch
from
f8d30ac to
77a7d4d
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-service
branch
from
77a7d4d to
d0155a6
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
Error Summary
TDF3 Benchmark Results:
Error Summary:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
![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
🤖 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 `@service/internal/auth/authn.go`:
- Around line 518-523: The code pattern where `errors.As` extracts a
`*connect.Error` into `connectErr`, modifies it via
`connectErr.Meta().Set("DPoP-Nonce"...)`, and then returns the original `err` is
subtle and could be clearer. Add a clarifying comment above the `if
errors.As(err, &connectErr)` block explaining that `errors.As` extracts a
pointer to the underlying `*connect.Error` within the error chain, so modifying
`connectErr.Meta()` mutates the same underlying object that is part of the
returned `err`, making the return of `err` correct despite the modifications
being made on `connectErr`.
In `@test/start-up-with-containers/action.yaml`:
- Around line 144-146: The three curl commands downloading init-temp-keys.sh,
docker-compose.yaml, and watch.sh are using a hardcoded feature tag
`feat-kc26-dpop` instead of using the `platform-ref` input parameter that is
already defined and validated in the action. Replace the hardcoded
`feat-kc26-dpop` tag in all three curl URLs with `${{ inputs.platform-ref }}` to
align with the action's existing parameter usage and prevent breakage when the
feature tag is deleted.
🪄 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: ca44809a-aa60-40fe-929d-0383d289be78
📒 Files selected for processing (7)
docker-compose.yamlservice/cmd/keycloak_data.yamlservice/internal/auth/authn.goservice/internal/auth/authn_test.goservice/internal/auth/config.goservice/internal/auth/dpop_nonce_test.gotest/start-up-with-containers/action.yaml
Comment on lines
+144
to
+146
| curl -sSfL https://raw.githubusercontent.com/opentdf/platform/refs/tags/feat-kc26-dpop/.github/scripts/init-temp-keys.sh > otdf-test-platform/.github/scripts/init-temp-keys.sh | ||
| curl -sSfL https://raw.githubusercontent.com/opentdf/platform/refs/tags/feat-kc26-dpop/docker-compose.yaml > otdf-test-platform/docker-compose.yaml | ||
| curl -sSfL https://raw.githubusercontent.com/opentdf/platform/refs/tags/feat-kc26-dpop/.github/scripts/watch.sh > otdf-test-platform/.github/scripts/watch.sh |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the feat-kc26-dpop tag exists and what it references
gh api repos/opentdf/platform/git/refs/tags/feat-kc26-dpop 2>/dev/null || echo "Tag feat-kc26-dpop not found or is a branch reference"
# Check if it's a branch instead
gh api repos/opentdf/platform/branches/feat-kc26-dpop 2>/dev/null && echo "feat-kc26-dpop is a branch"Repository: opentdf/platform
Length of output: 547
🏁 Script executed:
# Check the full context of action.yaml around lines 144-146
sed -n '140,150p' test/start-up-with-containers/action.yaml
# Check if there's a platform-ref input parameter that could be used
grep -n "platform-ref\|platform_ref" test/start-up-with-containers/action.yaml | head -20
# Check if these scripts exist on main branch
curl -sSfL https://raw.githubusercontent.com/opentdf/platform/main/.github/scripts/init-temp-keys.sh > /dev/null 2>&1 && echo "init-temp-keys.sh exists on main" || echo "init-temp-keys.sh NOT found on main"
curl -sSfL https://raw.githubusercontent.com/opentdf/platform/main/.github/scripts/watch.sh > /dev/null 2>&1 && echo "watch.sh exists on main" || echo "watch.sh NOT found on main"
curl -sSfL https://raw.githubusercontent.com/opentdf/platform/main/docker-compose.yaml > /dev/null 2>&1 && echo "docker-compose.yaml exists on main" || echo "docker-compose.yaml NOT found on main"Repository: opentdf/platform
Length of output: 1292
Use platform-ref input instead of hardcoded feature tag.
The downloads on lines 144-146 reference the hardcoded feat-kc26-dpop tag, but these files (init-temp-keys.sh, watch.sh, docker-compose.yaml) already exist on the main branch. Since the action already has a platform-ref input parameter (line 6) that is validated and used elsewhere (line 139), update these downloads to use ${{ inputs.platform-ref }} instead. This will align with the action's existing parameter usage and prevent breakage when the feature tag is eventually deleted.
🤖 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 `@test/start-up-with-containers/action.yaml` around lines 144 - 146, The three
curl commands downloading init-temp-keys.sh, docker-compose.yaml, and watch.sh
are using a hardcoded feature tag `feat-kc26-dpop` instead of using the
`platform-ref` input parameter that is already defined and validated in the
action. Replace the hardcoded `feat-kc26-dpop` tag in all three curl URLs with
`${{ inputs.platform-ref }}` to align with the action's existing parameter usage
and prevent breakage when the feature tag is deleted.
Implement RFC 9449 DPoP support for the OpenTDF platform service: - Accept both `Authorization: Bearer` and `Authorization: DPoP` schemes - Validate DPoP proofs per RFC 9449 §4.3 + §7.1: - typ=dpop+jwt header validation - Allowed algorithms: ES256, RS256, PS256 (and 384/512 variants) - JWK extraction and signature verification - htm/htu/ath claim validation - RFC 7638 JWK thumbprint matching cnf.jkt - Server-issued DPoP-Nonce challenges per RFC 9449 §8: - Configurable via `server.auth.dpop.require_nonce` (default: false) - 401 response with `DPoP-Nonce` header and `WWW-Authenticate: DPoP error="use_dpop_nonce"` - Nonce rotation window (current + previous) with configurable expiration - Nonce validation in both HTTP and gRPC paths - gRPC support: htm=POST, htu=full service path, DPoP-Nonce via response headers/trailers - Feature detection: register `supports_dpop` in wellknown service for xtest integration - Comprehensive unit tests for nonce management, proof validation, error handling Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…once action input, and KC26 bump (DSPX-3397) - Register dpop_supported_alg_values and dpop_nonce_required in the /.well-known/opentdf-configuration endpoint so SDK clients can discover server DPoP capabilities (RFC 9449 §5.1) - Add opentdf-dpop Keycloak client with dpop.bound.access.tokens=true attribute for DPoP-bound token testing alongside existing opentdf client - Add dpop-challenge-enabled input to start-up-with-containers action that patches server.auth.dpop.require_nonce: true when enabled - Update action curl downloads from pqc-enabled to feat-kc26-dpop tag - Bump Keycloak from 25.0 to 26.2 in docker-compose.yaml Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Two fixups for the DSPX-3397 platform-service work: - authn.go: structpb.NewStruct rejects []string when serializing the well-known configuration. Convert dpop_supported_alg_values to []any before registration. Without this, /.well-known/opentdf-configuration returns 500 with "proto: invalid type: []string". - docker-compose.yaml: KC26 dropped admin-fine-grained-authz from the default preview profile. The platform's `service provision keycloak` calls setManagementPermissionsEnabled which requires this feature. Enable it explicitly via KC_FEATURES so provisioning succeeds against the bumped Keycloak image. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…gation - getCurrentNonce: use double-checked locking to prevent concurrent double-rotation; inlines rotation under write lock instead of calling rotate() (which acquires its own lock and would deadlock) - ConnectUnaryServerInterceptor: replace metadata.AppendToOutgoingContext with a next-wrapper that sets DPoP-Nonce on res.Header() so the header is actually returned to the client Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
- Rename well-known key to dpop_signing_alg_values_supported (RFC 9449 §5.1) - Add DPoPNonceMalformedError type for non-string nonce claims; malformed proofs fall through to hard rejection rather than issuing a nonce challenge - Downgrade DPoPNonceError log entries from Warn to Debug since nonce challenges are normal protocol handshakes, not failures - Set DPoP-Nonce on Connect error responses so clients retain nonce after downstream handler errors - Add DPoPConfig.Validate() to reject zero NonceExpiration when RequireNonce is true; call it from validateAuthNConfig - Remove tautological tests that asserted only on local variables; replace with real checkToken coverage for missing/valid/wrong/malformed nonce cases Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…empty nonce validation Replace sync.RWMutex with atomic.Pointer[nonceState] for lock-free reads on the hot path (getCurrentNonce, validateNonce), keeping sync.Mutex only to serialize writes. Also guards validateNonce against empty-string nonce matching the initial zero-value previousNonce. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Per RFC 9449 §7.1, DPoP-bound access tokens (cnf.jkt claim present) MUST be presented under the "DPoP" Authorization scheme. The platform currently accepts either scheme as long as a valid DPoP proof is attached. This change emits a WARN log when a cnf-bound token arrives under "Bearer" scheme, surfacing non-compliant SDKs without breaking existing clients. A follow-up will promote this to a hard reject once all SDKs are compliant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ptor The ConnectRPC interceptor was using req.Spec().Procedure (path only) as the expected htu value, but RFC 9449 requires htu to be the full request URI (scheme + host + path). Clients correctly send the full URL, causing htu validation to always fail for gRPC/ConnectRPC endpoints. Fix both the ConnectRPC unary interceptor and ipcReauthCheck to construct the full URL from the Host header, accepting both http and https schemes since TLS state is not available in the interceptor context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In loose mode (default, strict_htu: false) a path-only htu claim is accepted when its path matches the path of any acceptable URI, easing SDK skew during rollout. If the origin is present it must still match exactly (scheme-flexible via the http+https pair already in dpopInfo.u). In strict mode (strict_htu: true) the origin must be present and match; path-only htu claims are rejected outright. Config path: server.auth.dpop.strict_htu (bool, default false) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…interceptor ConnectRPC supports idempotent unary RPCs over HTTP GET (proto option idempotency_level = NO_SIDE_EFFECTS). The Java connect-go client (Buf's 'Hasan'/Get-requests feature) uses this whenever the server advertises an idempotent method. DPoP requires the proof JWT's htm claim to equal the actual HTTP method, but the Connect interceptor hardcoded POST in receiverInfo.m, so any GET request with htm:'GET' was rejected as 'incorrect htm claim'. Use connect.AnyRequest.HTTPMethod() (which is populated server-side from the underlying *http.Request) to bring the Connect path to parity with the MuxHandler path, which already reads r.Method. IPCUnaryServerInterceptor is unchanged: IPC traffic goes through memhttp which always uses POST. Tests: - New positive checkToken case for htm:GET. - New interceptor test that captures receiverInfo via _testCheckTokenFunc and asserts the method is propagated (parameterized over GET and POST). - New invalid-DPoP table row exercising the wrong-method failure direction (GET-DPoP against POST-only receiver). Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-3397-platform-service
branch
from
24d7101 to
70cb173
Compare
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
🤖 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 `@service/internal/auth/authn_test.go`:
- Around line 786-804: The test case in the makeDPoPToken call and the
receiverInfo structure both use path-only htu values (such as
"/kas.AccessService/PublicKey"), which makes the test dependent on loose HTU
matching mode. To decouple the GET-method coverage from HTU validation behavior,
update the htu field in the makeDPoPToken call to use a full URL with scheme and
host (for example "http://localhost/kas.AccessService/PublicKey" or similar)
instead of just the path, ensuring the test can verify the htm=GET behavior
regardless of whether strict or loose HTU validation is enabled.
In `@service/internal/auth/dpop_nonce_test.go`:
- Around line 143-146: The DPoPConfig in the newAuthWithNonce() fixture does not
explicitly set the StrictHTU field, which leaves the HTU validation mode
implicit. Since the nonce tests use path-only htu values (as seen in test cases
at various lines), if the default HTU behavior changes, these tests could fail
for the wrong reason rather than exercising nonce validation. Add the StrictHTU
field to the DPoPConfig structure in newAuthWithNonce() and pin it to an
explicit value (likely false to match the path-only htu usage in the tests) to
ensure the tests remain deterministic and focused on nonce functionality.
🪄 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: 6b2d448e-a4c0-4260-989c-45e3b7d5ecec
📒 Files selected for processing (7)
docker-compose.yamlservice/cmd/keycloak_data.yamlservice/internal/auth/authn.goservice/internal/auth/authn_test.goservice/internal/auth/config.goservice/internal/auth/dpop_nonce_test.gotest/start-up-with-containers/action.yaml
Comment on lines
+786
to
+804
| dpopToken := makeDPoPToken(s.T(), dpopTestCase{ | ||
| key: dpopPublic, | ||
| actualSigningKey: dpopKey, | ||
| accessToken: signedTok, | ||
| alg: jwa.RS256, | ||
| typ: "dpop+jwt", | ||
| htm: http.MethodGet, | ||
| htu: "/kas.AccessService/PublicKey", | ||
| iat: time.Now(), | ||
| }) | ||
|
|
||
| _, _, err = s.auth.checkToken( | ||
| context.Background(), | ||
| []string{"DPoP " + string(signedTok)}, | ||
| receiverInfo{ | ||
| u: []string{"/kas.AccessService/PublicKey"}, | ||
| m: []string{http.MethodGet}, | ||
| }, | ||
| []string{dpopToken}, |
There was a problem hiding this comment.
Decouple GET-method coverage from loose HTU matching.
Line 793 and Line 801 use path-only htu, so this test also depends on loose HTU mode. If strict HTU is enabled later, it will fail before reaching the htm=GET behavior this test intends to verify.
Proposed adjustment
dpopToken := makeDPoPToken(s.T(), dpopTestCase{
key: dpopPublic,
actualSigningKey: dpopKey,
accessToken: signedTok,
alg: jwa.RS256,
typ: "dpop+jwt",
htm: http.MethodGet,
- htu: "/kas.AccessService/PublicKey",
+ htu: "https://localhost:8080/kas.AccessService/PublicKey",
iat: time.Now(),
})
@@
[]string{"DPoP " + string(signedTok)},
receiverInfo{
- u: []string{"/kas.AccessService/PublicKey"},
+ u: []string{
+ "http://localhost:8080/kas.AccessService/PublicKey",
+ "https://localhost:8080/kas.AccessService/PublicKey",
+ },
m: []string{http.MethodGet},
},
[]string{dpopToken},
)📝 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
| dpopToken := makeDPoPToken(s.T(), dpopTestCase{ | |
| key: dpopPublic, | |
| actualSigningKey: dpopKey, | |
| accessToken: signedTok, | |
| alg: jwa.RS256, | |
| typ: "dpop+jwt", | |
| htm: http.MethodGet, | |
| htu: "/kas.AccessService/PublicKey", | |
| iat: time.Now(), | |
| }) | |
| _, _, err = s.auth.checkToken( | |
| context.Background(), | |
| []string{"DPoP " + string(signedTok)}, | |
| receiverInfo{ | |
| u: []string{"/kas.AccessService/PublicKey"}, | |
| m: []string{http.MethodGet}, | |
| }, | |
| []string{dpopToken}, | |
| dpopToken := makeDPoPToken(s.T(), dpopTestCase{ | |
| key: dpopPublic, | |
| actualSigningKey: dpopKey, | |
| accessToken: signedTok, | |
| alg: jwa.RS256, | |
| typ: "dpop+jwt", | |
| htm: http.MethodGet, | |
| htu: "https://localhost:8080/kas.AccessService/PublicKey", | |
| iat: time.Now(), | |
| }) | |
| _, _, err = s.auth.checkToken( | |
| context.Background(), | |
| []string{"DPoP " + string(signedTok)}, | |
| receiverInfo{ | |
| u: []string{ | |
| "http://localhost:8080/kas.AccessService/PublicKey", | |
| "https://localhost:8080/kas.AccessService/PublicKey", | |
| }, | |
| m: []string{http.MethodGet}, | |
| }, | |
| []string{dpopToken}, |
🤖 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 `@service/internal/auth/authn_test.go` around lines 786 - 804, The test case in
the makeDPoPToken call and the receiverInfo structure both use path-only htu
values (such as "/kas.AccessService/PublicKey"), which makes the test dependent
on loose HTU matching mode. To decouple the GET-method coverage from HTU
validation behavior, update the htu field in the makeDPoPToken call to use a
full URL with scheme and host (for example
"http://localhost/kas.AccessService/PublicKey" or similar) instead of just the
path, ensuring the test can verify the htm=GET behavior regardless of whether
strict or loose HTU validation is enabled.
Comment on lines
+143
to
+146
| DPoP: DPoPConfig{ | ||
| RequireNonce: true, | ||
| NonceExpiration: 5 * time.Minute, | ||
| }, |
There was a problem hiding this comment.
Pin StrictHTU in the nonce fixture to keep nonce tests deterministic.
newAuthWithNonce() leaves HTU mode implicit while nonce tests use path-only htu (for example, Line 229, Line 251, Line 271, Line 295). If strict HTU behavior changes, these tests can fail before nonce validation is exercised.
Proposed adjustment
DPoP: DPoPConfig{
RequireNonce: true,
NonceExpiration: 5 * time.Minute,
+ StrictHTU: false,
},📝 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
| DPoP: DPoPConfig{ | |
| RequireNonce: true, | |
| NonceExpiration: 5 * time.Minute, | |
| }, | |
| DPoP: DPoPConfig{ | |
| RequireNonce: true, | |
| NonceExpiration: 5 * time.Minute, | |
| StrictHTU: false, | |
| }, |
🤖 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 `@service/internal/auth/dpop_nonce_test.go` around lines 143 - 146, The
DPoPConfig in the newAuthWithNonce() fixture does not explicitly set the
StrictHTU field, which leaves the HTU validation mode implicit. Since the nonce
tests use path-only htu values (as seen in test cases at various lines), if the
default HTU behavior changes, these tests could fail for the wrong reason rather
than exercising nonce validation. Add the StrictHTU field to the DPoPConfig
structure in newAuthWithNonce() and pin it to an explicit value (likely false to
match the path-only htu usage in the tests) to ensure the tests remain
deterministic and focused on nonce functionality.
Contributor
Log the htm claim at every site where DpopInfo.m is set on the server and where the htm claim is built on the client, plus the htm comparison in validateDPoP. Helps diagnose method mismatches between client and server (e.g. client hardcoding POST while server reads the transport method). 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
Map errors from canAccess into categorized 403/500 responses with a sanitized cause-class tag (forbidden: pdp-denied, internal: auth-service-unavailable, internal: context-cancelled) so clients and tests can distinguish routine denials from infrastructure failures without parsing logs. Previously every category from canAccess collapsed to code=Internal with the opaque message "could not perform access". Split logging at the two GetDecision call sites and the rewrap call site into Info (terse, no sensitive payload — category, connect code, batch size) plus Debug (full error, policies, resource attribute FQNs, fulfillable obligation FQNs). The Info lines previously dropped err entirely at ErrorContext, so debug logging in CI couldn't surface what the authorization service actually said. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
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
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.
Implements comprehensive DPoP (Demonstrating Proof-of-Possession) support per RFC 9449 for the OpenTDF platform service.
Summary
This PR adds full RFC 9449 DPoP support to the platform authentication middleware, enabling:
Authorization: BearerandAuthorization: DPoPtoken schemessupports_dpop)Since we have an existing, partial implementation, some fallback protection is in place:
Authorization: Bearer [jwt]headers.cnfclaim.htuclaims with missing origin, e.g./kas.AccessService/Rewrapwill matchhtu, setserver.auth.dpop.strict_htu: trueImplementation Details
DPoP Proof Validation (RFC 9449 §4.3 + §7.1)
The middleware validates all required DPoP proof claims:
dpop+jwtcnf.jktclaimDPoP-Nonce Support (RFC 9449 §8)
Server-issued nonces prevent replay attacks:
server.auth.dpop.require_nonce(default: false),server.auth.dpop.nonce_expiration(default: 5m)DPoP-Nonceheader andWWW-Authenticate: DPoP error="use_dpop_nonce"gRPC Support
DPoP works seamlessly with gRPC/Connect:
POSTfor gRPC calls/kas.AccessService/Rewrap)Feature Detection
Registers
supports_dpop: truein the wellknown service for integration with xtest feature gates (pfs.skip_if_unsupported("dpop")).Testing
Unit Tests (
dpop_nonce_test.go)Comprehensive test coverage:
Integration Tests
Existing DPoP tests in
test/integration/oauth/oauth_test.gocontinue to pass and validate end-to-end flows with real Keycloak.Related
xtest/scenarios/DSPX-3397.yamlAll PRs:
Summary by CodeRabbit
Release Notes
New Features
Configuration
Chores