fix(authz): emit WWW-Authenticate DPoP on all proof rejections (DSPX-3397)#3665
fix(authz): emit WWW-Authenticate DPoP on all proof rejections (DSPX-3397)#3665dmihalcik-virtru wants to merge 1 commit into
Conversation
… (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>
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 addresses an issue where DPoP proof rejections (such as tampered claims or replayed tokens) were returning bare 401 Unauthorized responses without the required DPoP challenge headers. By introducing a specific error type for proof failures and updating the authentication handlers to catch and process these errors, the service now correctly provides the necessary WWW-Authenticate headers. This change improves client-side error handling and resolves flakiness in negative DPoP tests. 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. A token arrives with a claim, But DPoP proof is not the same. With headers now set, No more flakiness yet, And errors are handled by name. Footnotes
|
📝 WalkthroughWalkthroughThe authentication flow now distinguishes retryable DPoP nonce failures from invalid DPoP proof failures. HTTP and Connect handlers emit different ChangesDPoP proof challenge handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces DPoPProofError to represent non-retryable DPoP proof rejections, updating both the HTTP mux handler and Connect interceptor to return the standard invalid_dpop_proof challenge. The review feedback suggests adding safety checks to the Error() and Unwrap() methods of DPoPProofError to prevent potential nil pointer dereference panics when the error is uninitialized or nil.
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.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
dmihalcik-virtru
changed the title
3 participants
Summary
Part of DSPX-3397 (split out from the SDK DPoP work). Server-side fix only.
The auth handlers previously attached a
WWW-Authenticateresponse header only when the failure was a*DPoPNonceError(use_dpop_nonce). Every other DPoP proof rejection — tamperedhtu(checked before nonce), replayedjti(checked last), malformed nonce, badath, wronghtm— returned a bare401with no challenge header. Per RFC 9449 §7.1 a resource server rejecting a DPoP-authenticated request should respond with aDPoPchallenge, and clients/tests can't distinguish a DPoP failure from an unrelated 401 without it.This surfaced as flaky xtest DPoP negative tests:
tampered_htufailed ~every run,replayed_jtiflaked depending on nonce-rotation timing, andtampered_noncefailed whenever a non-nonce check tripped first.Changes
DPoPProofErrormarker type (delegatingError(),Unwrap()).checkTokenwraps all non-noncevalidateDPoPerrors in it (nonce errors stay unwrapped).MuxHandlerandConnectAuthNInterceptornow emitWWW-Authenticate: DPoP error="invalid_dpop_proof"for proof failures (and a freshDPoP-Noncewhen nonce is required); theuse_dpop_noncepath is unchanged.Testing
go test ./service/internal/auth/...passes.golangci-lintadds no new issues.Summary by CodeRabbit
Bug Fixes
Tests