Skip to content

fix(authz): emit WWW-Authenticate DPoP on all proof rejections (DSPX-3397)#3665

Merged
dmihalcik-virtru merged 1 commit into
mainfrom
DSPX-3397-platform-service-part-2
Jun 26, 2026
Merged

fix(authz): emit WWW-Authenticate DPoP on all proof rejections (DSPX-3397)#3665
dmihalcik-virtru merged 1 commit into
mainfrom
DSPX-3397-platform-service-part-2

Conversation

@dmihalcik-virtru

@dmihalcik-virtru dmihalcik-virtru commented Jun 25, 2026

Copy link
Copy Markdown
Member

Summary

Part of DSPX-3397 (split out from the SDK DPoP work). Server-side fix only.

The auth handlers previously attached a WWW-Authenticate response header only when the failure was a *DPoPNonceError (use_dpop_nonce). Every other DPoP proof rejection — tampered htu (checked before nonce), replayed jti (checked last), malformed nonce, bad ath, wrong htm — returned a bare 401 with no challenge header. Per RFC 9449 §7.1 a resource server rejecting a DPoP-authenticated request should respond with a DPoP challenge, and clients/tests can't distinguish a DPoP failure from an unrelated 401 without it.

This surfaced as flaky xtest DPoP negative tests: tampered_htu failed ~every run, replayed_jti flaked depending on nonce-rotation timing, and tampered_nonce failed whenever a non-nonce check tripped first.

Changes

  • New DPoPProofError marker type (delegating Error(), Unwrap()).
  • checkToken wraps all non-nonce validateDPoP errors in it (nonce errors stay unwrapped).
  • MuxHandler and ConnectAuthNInterceptor now emit WWW-Authenticate: DPoP error="invalid_dpop_proof" for proof failures (and a fresh DPoP-Nonce when nonce is required); the use_dpop_nonce path is unchanged.
  • Unit tests for the error type and the challenge headers across both handlers (nonce on/off).

Testing

  • go test ./service/internal/auth/... passes.
  • golangci-lint adds no new issues.

Summary by CodeRabbit

  • Bug Fixes

    • Improved DPoP authentication error handling by distinguishing non-retryable invalid proof failures from retryable nonce issues.
    • Updated HTTP and Connect authentication responses to return the correct WWW-Authenticate challenge (invalid_dpop_proof vs nonce-based use_dpop_nonce), including DPoP-Nonce when required.
  • Tests

    • Added unit tests covering the new DPoP proof error behavior and its errors compatibility, plus end-to-end challenge header assertions for both HTTP and Connect flows.

@dmihalcik-virtru dmihalcik-virtru requested a review from a team as a code owner June 25, 2026 14:10
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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 DPoPProofError Type: Introduced a new marker error type, DPoPProofError, to distinguish non-retryable DPoP proof rejections from nonce-related errors.
  • Enhanced Error Handling: Updated checkToken to wrap non-nonce DPoP validation failures in DPoPProofError, ensuring they are correctly identified during error handling.
  • RFC 9449 Compliance: Modified MuxHandler and ConnectAuthNInterceptor to emit the WWW-Authenticate: DPoP error="invalid_dpop_proof" challenge for proof failures, improving RFC 9449 compliance.
  • Expanded Test Coverage: Added comprehensive unit tests to verify that both MuxHandler and ConnectAuthNInterceptor correctly issue the appropriate challenge headers for different DPoP error scenarios.
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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@dmihalcik-virtru, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 56 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 review availability.

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, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 68f159be-203c-4a18-8718-7d42767b1145

📥 Commits

Reviewing files that changed from the base of the PR and between 192fd99 and 48e86d6.

📒 Files selected for processing (3)
  • service/internal/auth/authn.go
  • service/internal/auth/authn_test.go
  • service/internal/auth/dpop_nonce_test.go
📝 Walkthrough

Walkthrough

The authentication flow now distinguishes retryable DPoP nonce failures from invalid DPoP proof failures. HTTP and Connect handlers emit different WWW-Authenticate challenges, and tests cover the new error classification and response behavior.

Changes

DPoP proof challenge handling

Layer / File(s) Summary
Error classification and wrapper
service/internal/auth/authn.go, service/internal/auth/dpop_nonce_test.go
Adds DPoPProofError, changes checkToken to wrap non-nonce DPoP validation failures, and updates unit coverage for wrapped-error behavior and nonce configuration.
HTTP and Connect challenges
service/internal/auth/authn.go, service/internal/auth/authn_test.go
MuxHandler and ConnectAuthNInterceptor distinguish DPoPNonceError from DPoPProofError, and tests force each error to check the emitted challenge and DPoP-Nonce handling.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • opentdf/platform#3582: Also changes DPoP authentication behavior in service/internal/auth/authn.go, including how nonce and proof failures are mapped to auth challenges.

Suggested reviewers

  • elizabethhealy

Poem

A bunny hopped through auth at dawn,
One nonce was asked, one proof went wrong.
The headers twitched, then clear and bright,
Chose nonce or proof just right. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: emitting DPoP WWW-Authenticate challenges for proof rejections.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch DSPX-3397-platform-service-part-2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread service/internal/auth/authn.go
@github-actions

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 204.365258ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 119.307357ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 448.092226ms
Throughput 223.17 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 46.818824337s
Average Latency 466.742727ms
Throughput 106.79 requests/second

@dmihalcik-virtru dmihalcik-virtru changed the title fix(service/auth): emit WWW-Authenticate DPoP on all proof rejections (DSPX-3397) fix(authz): emit WWW-Authenticate DPoP on all proof rejections (DSPX-3397) Jun 25, 2026
pflynn-virtru
pflynn-virtru previously approved these changes Jun 25, 2026
elizabethhealy
elizabethhealy previously approved these changes Jun 25, 2026
@dmihalcik-virtru dmihalcik-virtru force-pushed the DSPX-3397-platform-service-part-2 branch from 6d5ad4e to 192fd99 Compare June 26, 2026 19:25
@policy-bot-opentdf policy-bot-opentdf Bot dismissed stale reviews from pflynn-virtru and elizabethhealy June 26, 2026 19:25

Invalidated by push of 192fd99

… (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 <[email protected]>
@dmihalcik-virtru dmihalcik-virtru force-pushed the DSPX-3397-platform-service-part-2 branch from 192fd99 to 48e86d6 Compare June 26, 2026 19:29
@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 209.542598ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 113.083296ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 485.589242ms
Throughput 205.94 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 46.238824387s
Average Latency 460.669393ms
Throughput 108.13 requests/second

@github-actions

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 211.815787ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 113.468564ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 430.700771ms
Throughput 232.18 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 46.284656966s
Average Latency 460.656211ms
Throughput 108.03 requests/second

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • examples
  • otdfctl
  • sdk
  • service
  • lib/fixtures
  • tests-bdd

See the workflow run for details.

@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue Jun 26, 2026
Merged via the queue into main with commit d7caacd Jun 26, 2026
45 checks passed
@dmihalcik-virtru dmihalcik-virtru deleted the DSPX-3397-platform-service-part-2 branch June 26, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants