fix(handler): enforce DPoP sender-constraint on forward-auth verify - #272
fix(handler): enforce DPoP sender-constraint on forward-auth verify#272SashaMIT wants to merge 1 commit into
Conversation
GET /oauth2/token/verify introspected tokens and returned 200 without requiring a DPoP proof when cnf.jkt was present. Forward-auth is the resource server for every proxied upstream, so a leaked bound token replayed as bare Bearer passed nginx/Caddy/Traefik gates. When introspection surfaces cnf.jkt and a DPoP verifier is configured, require a matching DPoP proof (ValidateBoundToToken) before emitting identity headers. Unbound tokens are unchanged.
saucam
left a comment
There was a problem hiding this comment.
Thanks for this — spotting that forward-auth surfaces cnf without enforcing it is a genuinely good catch, and the write-up made the review easy to follow.
The intent here is right, and it's a real gap: a leaked DPoP-bound token replayed as a bare Bearer should not pass the proxy gate. I'd like to land something in this shape. Two problems block this particular implementation, though, and I think the second one needs a design decision before code.
1. As written, this rejects every DPoP-bound token behind forward-auth
Validate compares the proof strictly against the request it is handed (pkg/dpop/verifier.go:140-147):
if !methodEqual(proof.HTM, r.Method) { ... ErrHTMMismatch }
wantHTU := v.urlNormalize(r.URL)
if !constantTimeStringEq(proof.HTU, wantHTU) { ... ErrHTUMismatch }On the forward-auth path, the request the handler sees is the proxy's subrequest, not the client's request:
| client → proxy | proxy → zeroid (what the handler sees) | |
|---|---|---|
| method | POST |
GET (nginx auth_request, Caddy forward_auth, Traefik forwardAuth all issue GET) |
| URL | https://api.example.com/v1/orders |
https://zeroid.internal/oauth2/token/verify |
The client's proof carries htm/htu for the upstream request, so the comparison can never match. EffectiveRequestURL builds htu from r.URL.Path (internal/middleware/request_url.go:40), which on this path is /oauth2/token/verify, and r.Method is passed through directly.
The original method/URI do arrive — in X-Forwarded-Method / X-Forwarded-Uri / X-Original-URI — but nothing in zeroid currently reads them (I grepped the tree).
Reproduction. Modelling the real proxy shape — correct key, valid proof for its upstream:
// client's proof is bound to the UPSTREAM request
proof := buildBoundDPoPProof(t, key, http.MethodPost, "https://api.example.com/v1/orders", accessToken)
// the subrequest the proxy actually makes
req := httptest.NewRequest(http.MethodGet, "http://zeroid.internal/oauth2/token/verify", nil)
req.Header.Set("DPoP", proof)
req.Header.Set("X-Forwarded-Method", http.MethodPost)
req.Header.Set("X-Forwarded-Uri", "/v1/orders")
req.Header.Set("X-Forwarded-Host", "api.example.com")
rejected := rejectBoundTokenWithoutDPoP(rr, req, accessToken, claims, verifier, "")rejected=true status=401 body={"error":"invalid_token","error_description":"invalid DPoP proof"}
The current tests pass because they mint the proof for the verify endpoint itself:
proof := buildBoundDPoPProof(t, dpopKey, http.MethodGet, "http://rs.test/oauth2/token/verify", accessToken)A real client never does that — it doesn't know the endpoint exists. So the suite is exercising the helper's mechanics rather than the deployment, which is how this slipped through. That reproduction above is the case worth adding whichever way the fix goes.
It fails closed, so it isn't a vulnerability — but it does invert the goal: instead of enforcing the binding, it makes bound tokens unusable through the proxy.
If you take this on: reconstructing htm/htu from X-Forwarded-Method / X-Forwarded-Uri must be gated behind cfg.Server.TrustForwardedHeaders, the same way RequestURLMiddleware already gates proto/host. Ungated, a client could forge the htu it wants validated against, which defeats the binding entirely — worse than the status quo.
2. The jti replay store — needs a decision, not just code
Validate consumes the proof's jti (pkg/dpop/verifier.go:172):
if err := v.store.Insert(ctx, proof.JTI, expiresAt); err != nil { ... }and in production that store is Postgres-backed (server.go:361, postgres.NewDPoPReplayStore(db)). On a forward-auth gate that means:
- A Postgres write on every proxied request — this endpoint sits in front of all upstream traffic, so it's a different performance profile from the token endpoint.
- It consumes the
jtithe upstream resource server needs. If that server also validates the proof — the normal RFC 9449 posture, and rather the point of DPoP — its validation then hits replay and fails. So turning this on could break the enforcement it's meant to add.
pkg/dpop has no validate-without-consume option today (options.go covers clock skew, max age, now, logger, URL normalizer, max jti length). So this needs either a new option there, or an explicit decision that forward-auth is the sole enforcement point for a given deployment. Worth settling before more code — happy to discuss which way we want to go.
What's working well
Genuinely good and worth keeping in the next revision:
athis bound (AccessToken: accessToken). Commonly omitted; correct here.- Fail-open only where intended — unbound tokens and a nil verifier are untouched, matching the token endpoint's optional posture.
cnfJKTFromClaimshandling bothmap[string]anyandmap[string]string. Exactly the right defensiveness — a claim-shape quirk silently skipping enforcement is a bug class we hit recently in #257, so this instinct is appreciated.ValidateBoundToTokenerroring on an emptyexpectedJKTis a good fail-closed guard.WWW-Authenticatecarrying the PRM is correct per RFC 9728.
Minor
cnfJKTFromClaimsis slightly asymmetric: themap[string]anybranch falls through to""whenjktis present but not a string, while themap[string]stringbranch returns directly. Harmless, but worth making consistent.- The
htu == ""fallback inrejectBoundTokenWithoutDPoPis unreachable in production —RequestURLMiddlewarealways installs on this router (server.go:420). It exists only so unit tests can call the helper directly, which is the same seam that let the issue above through.
Suggested next step
Happy to keep this open while we settle (2) — that one is on us, not you. Once we've picked a direction on the replay store, (1) is a contained change: read the forwarded method/URI behind TrustForwardedHeaders, and add the real-proxy-shape test.
On your question in the description — I'd keep this separate from #269 rather than folding them. #269's middleware sees the actual client request, so it doesn't have the htm/htu problem, and it can land independently.
Thanks again for digging into this area; it's a valuable direction and I'd like to get it merged once these are sorted.
|
Split blocker (2) out into #274 so it doesn't sit on your plate — the That leaves one thing here that's actually yours: the No rush on it; if you'd rather wait until #274 lands so you only revise once, that's completely reasonable — say the word and I'll ping you here when it's decided. |
|
Thanks @saucam — clear review. I'll wait on #274 for the |
Problem
GET /oauth2/token/verify(nginxauth_request/ Caddyforward_auth/ TraefikforwardAuth) introspected the Bearer JWT and returned200with identity headers without ever readingcnf.jktor requiring aDPoPproof.ZeroID already mints
cnf.jktwhenever a DPoP proof accompanies the token request (credential.go), and introspection deliberately surfacescnfso resource servers can enforce binding. Forward-auth is that resource server for every proxied upstream. Without enforcement, a leaked DPoP-bound token replayed as a bareBearerheader passed the proxy gate — the stolen-token case RFC 9449 §6.1 exists to defeat.This is the same defect class as #269 (
AgentAuthMiddleware), on the reverse-proxy verify path that many deployments use instead of in-process agent auth.Fix
After a successful introspection (
active: true), when the claims carrycnf.jktand aDPoPVerifieris configured:DPoPproof headerValidateBoundToTokenagainst the access token and expected JKT (htufromEffectiveRequestURL)401 invalid_tokenon missing/invalid proofUnbound tokens are unchanged. A nil verifier leaves behavior unchanged (same optional posture as token-endpoint DPoP).
Test plan
DPoPDPoP→ 401 (red on pre-fix stub, green after fix)GOEXPERIMENT=jsonv2 go test ./internal/handler/ -run 'TestCnfJKTFromClaims|TestRejectBoundTokenWithoutDPoP' -count=1Notes
Made with Cursor