diff --git a/client/begin_authorization.go b/client/begin_authorization.go index c817b76..3377dcd 100644 --- a/client/begin_authorization.go +++ b/client/begin_authorization.go @@ -221,26 +221,28 @@ func (c *Client) pushAuthorizationRequestWithDPoPProof(ctx context.Context, para return nil, newError(ErrorInternal, "failed to build pushed authorization request", buildErr) } - body, status, header, err := c.postParRequestWithDPoP(ctx, dpopSigner, &parURL, form, "") + body, status, header, err := c.postParRequestWithDPoP(ctx, dpopSigner, &parURL, form, c.cachedDPoPNonce(ctx, asNonceScope)) if err != nil { return nil, newError(ErrorInternal, "pushed authorization request failed", err) } + nextNonce := header.Get("DPoP-Nonce") + c.cacheDPoPNonce(ctx, asNonceScope, nextNonce) if status == http.StatusCreated || status == http.StatusOK { return body, nil } - nonce := header.Get("DPoP-Nonce") - if nonce == "" || !isDPoPNonceError(body) { + if nextNonce == "" || !isDPoPNonceError(body) { return nil, parErrorFromResponse(body) } retryForm, buildErr := buildParForm() if buildErr != nil { return nil, newError(ErrorInternal, "failed to build pushed authorization request", buildErr) } - body, status, _, err = c.postParRequestWithDPoP(ctx, dpopSigner, &parURL, retryForm, nonce) + body, status, header, err = c.postParRequestWithDPoP(ctx, dpopSigner, &parURL, retryForm, nextNonce) if err != nil { return nil, newError(ErrorInternal, "pushed authorization request failed", err) } + c.cacheDPoPNonce(ctx, asNonceScope, header.Get("DPoP-Nonce")) if status != http.StatusCreated && status != http.StatusOK { return nil, parErrorFromResponse(body) } diff --git a/client/dependencies.go b/client/dependencies.go index f7f24b0..5c3fdfa 100644 --- a/client/dependencies.go +++ b/client/dependencies.go @@ -42,4 +42,12 @@ type Dependencies struct { // this stays an opt-in dependency rather than a mandatory one every // embedder has to wire up. Decryption keys.Decrypter + + // DPoPNonceCache lets this client proactively reuse a DPoP nonce a + // server already handed it, instead of always paying the challenge + // round trip RFC 9449 §8/§9 otherwise requires on every call — see + // its own doc comment. Nil (the zero value) disables the + // optimization entirely; pass NewInMemoryDPoPNonceCache() to enable + // it, the same deliberate opt-in Clock: SystemClock{} already is. + DPoPNonceCache DPoPNonceCache } diff --git a/client/exchange_code.go b/client/exchange_code.go index 2c53d91..b3f6a2b 100644 --- a/client/exchange_code.go +++ b/client/exchange_code.go @@ -216,26 +216,28 @@ func (c *Client) ExchangeCode(ctx context.Context, resp ValidatedAuthorizationRe // client assertion is exactly as single-use as a DPoP proof is (see // ExchangeCode's own comment on buildTokenForm). func (c *Client) sendTokenRequest(ctx context.Context, dpopSigner crypto.Signer, tokenURL *url.URL, buildTokenForm func() ([]byte, error), form []byte) ([]byte, *Error) { - body, status, header, err := c.postTokenRequestWithDPoP(ctx, dpopSigner, tokenURL, form, "") + body, status, header, err := c.postTokenRequestWithDPoP(ctx, dpopSigner, tokenURL, form, c.cachedDPoPNonce(ctx, asNonceScope)) if err != nil { return nil, newError(ErrorInternal, "token request failed", err) } + nextNonce := header.Get("DPoP-Nonce") + c.cacheDPoPNonce(ctx, asNonceScope, nextNonce) if status == http.StatusOK { return body, nil } - nonce := header.Get("DPoP-Nonce") - if nonce == "" || !isDPoPNonceError(body) { + if nextNonce == "" || !isDPoPNonceError(body) { return nil, parErrorFromResponse(body) } retryForm, buildErr := buildTokenForm() if buildErr != nil { return nil, newError(ErrorInternal, "failed to build client assertion", buildErr) } - body, status, _, err = c.postTokenRequestWithDPoP(ctx, dpopSigner, tokenURL, retryForm, nonce) + body, status, header, err = c.postTokenRequestWithDPoP(ctx, dpopSigner, tokenURL, retryForm, nextNonce) if err != nil { return nil, newError(ErrorInternal, "token request failed", err) } + c.cacheDPoPNonce(ctx, asNonceScope, header.Get("DPoP-Nonce")) if status != http.StatusOK { return nil, parErrorFromResponse(body) } diff --git a/client/flow_test.go b/client/flow_test.go index dc5ba11..33a5731 100644 --- a/client/flow_test.go +++ b/client/flow_test.go @@ -62,6 +62,13 @@ type fakeAS struct { challengeParDPoPNonce string parCallCount int + // nextDPoPNonce, if non-empty, makes both handlePAR and handleToken + // set a "DPoP-Nonce" response header on every successful response — + // the proactive-refresh behavior RFC 9449 §8 recommends and + // server's own NextDPoPNonce implements — so a test can check that + // client actually reuses it on its next, independent call. + nextDPoPNonce string + // tokenTypeOverride, if non-empty, replaces the token response's // token_type value (e.g. "dpop" lowercase, to test RFC 6749 §7.1's // "Values are case insensitive"). @@ -155,6 +162,9 @@ func (a *fakeAS) handlePAR(w http.ResponseWriter, r *http.Request) { a.lastNonce = r.PostForm.Get("nonce") } + if a.nextDPoPNonce != "" { + w.Header().Set("DPoP-Nonce", a.nextDPoPNonce) + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusCreated) json.NewEncoder(w).Encode(map[string]any{ @@ -294,6 +304,9 @@ func (a *fakeAS) handleToken(w http.ResponseWriter, r *http.Request) { resp["expires_in"] = 300 } + if a.nextDPoPNonce != "" { + w.Header().Set("DPoP-Nonce", a.nextDPoPNonce) + } w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(resp) } @@ -374,6 +387,45 @@ func newTestClientWithPARBinding(t *testing.T, binding client.PARDPoPBinding) (* return c, as, ts } +// newTestClientWithNonceCache is newTestClientWithPARBinding +// (PARDPoPBindingProof, so PAR presents a DPoP proof and is +// nonce-challengeable) plus a wired-in DPoPNonceCache, for tests +// checking that a cached nonce is proactively reused across +// independent calls. +func newTestClientWithNonceCache(t *testing.T) (*client.Client, *fakeAS, *httptest.Server) { + t.Helper() + as := newFakeAS(t, testIssuer, false) + ts := httptest.NewServer(as.handler()) + t.Cleanup(ts.Close) + + cfg := validConfig(t) + cfg.PARDPoPBinding = client.PARDPoPBindingProof + parURL, err := fapi.ParseEndpointURL(ts.URL+"/par", fapi.AllowLoopbackHTTP()) + if err != nil { + t.Fatalf("ParseEndpointURL(par): %v", err) + } + tokenURL, err := fapi.ParseEndpointURL(ts.URL+"/token", fapi.AllowLoopbackHTTP()) + if err != nil { + t.Fatalf("ParseEndpointURL(token): %v", err) + } + cfg.Endpoints.PushedAuthorizationRequest = parURL + cfg.Endpoints.Token = tokenURL + + deps := validDependencies(t) + deps.HTTP = ts.Client() + deps.DPoPNonceCache = client.NewInMemoryDPoPNonceCache() + deps.IssuerKeys = &fakeIssuerKeySource{keys: map[keys.IssuerVerificationPurpose]crypto.PublicKey{ + keys.JARMVerification: &as.jarmKey.PublicKey, + keys.IDTokenVerification: &as.idTokenKey.PublicKey, + }} + + c, err := client.New(cfg, deps) + if err != nil { + t.Fatalf("client.New: %v", err) + } + return c, as, ts +} + // newTestClientWithEncryptedIDToken is newTestClient (baseline profile // only) plus a configured Decryption dependency and a fakeAS that // encrypts every ID token it issues to that same key under alg — for @@ -796,6 +848,90 @@ func TestBeginAuthorizationCommitsDPoPKeyAtPARMatchingTokenEndpointProofDefault( // PARDPoPBindingProof means PAR now presents one for the first time, so // it needs the same retry BeginAuthorization already gets from // ExchangeCode at the token endpoint. +// RFC 9449 §8's own proactive-refresh recommendation only helps a +// client that actually tracks and reuses the nonce a response handed +// it. With a DPoPNonceCache configured, a second, independent +// authorization flow through the same Client must need no +// challenge/retry round trip at either PAR or the token endpoint, +// since both share one cached nonce (Dependencies.DPoPNonceCache, +// asNonceScope) seeded by the first flow's own successful responses. +func TestBeginAuthorizationAndExchangeCodeReuseCachedNonceOnSecondFlow(t *testing.T) { + c, as, _ := newTestClientWithNonceCache(t) + as.challengeParDPoPNonce = "server-nonce-1" + as.challengeDPoPNonce = "server-nonce-1" + as.nextDPoPNonce = "server-nonce-1" + ctx := context.Background() + + // First flow: PAR has no cached nonce yet, so it must be challenged + // once — but that challenge's own successful retry response caches + // a nonce into the *shared* "as" scope before ExchangeCode ever + // calls the token endpoint, so the token endpoint's very first + // attempt already succeeds. PAR priming the token call this way is + // exactly the point of sharing one scope between them. + session, err := c.BeginAuthorization(ctx, client.BeginAuthorizationRequest{Scope: []string{"openid", "accounts"}}) + if err != nil { + t.Fatalf("BeginAuthorization (first): %v", err) + } + if as.parCallCount != 2 { + t.Fatalf("first flow PAR call count = %d, want 2 (challenge + retry)", as.parCallCount) + } + rawQuery := as.callbackFor(t, session.Handle().String(), "auth-code-1", "") + if _, err := c.CompleteAuthorization(ctx, client.AuthorizationCallback{RawQuery: rawQuery}); err != nil { + t.Fatalf("CompleteAuthorization (first): %v", err) + } + if as.tokenCallCount != 1 { + t.Fatalf("first flow token call count = %d, want 1 (already primed by PAR's own cached nonce)", as.tokenCallCount) + } + + // Second, independent flow: both must succeed on the very first + // attempt, since the client proactively sends the nonce it cached + // from the first flow's own successful responses. + session, err = c.BeginAuthorization(ctx, client.BeginAuthorizationRequest{Scope: []string{"openid", "accounts"}}) + if err != nil { + t.Fatalf("BeginAuthorization (second): %v", err) + } + if as.parCallCount != 3 { + t.Fatalf("PAR call count after second BeginAuthorization = %d, want 3 (no retry needed)", as.parCallCount) + } + rawQuery = as.callbackFor(t, session.Handle().String(), "auth-code-2", "") + if _, err := c.CompleteAuthorization(ctx, client.AuthorizationCallback{RawQuery: rawQuery}); err != nil { + t.Fatalf("CompleteAuthorization (second): %v", err) + } + if as.tokenCallCount != 2 { + t.Fatalf("token call count after second exchange = %d, want 2 (no retry needed)", as.tokenCallCount) + } +} + +// A cached nonce that turns out to be stale must still recover via the +// existing challenge/retry logic — caching only ever removes a round +// trip, it never introduces a new failure mode. +func TestBeginAuthorizationRecoversFromStaleCachedNonce(t *testing.T) { + c, as, _ := newTestClientWithNonceCache(t) + ctx := context.Background() + + // Seed the cache with "first-nonce" via one real flow. + as.challengeParDPoPNonce = "first-nonce" + as.nextDPoPNonce = "first-nonce" + if _, err := c.BeginAuthorization(ctx, client.BeginAuthorizationRequest{Scope: []string{"openid"}}); err != nil { + t.Fatalf("BeginAuthorization (seed): %v", err) + } + if as.parCallCount != 2 { + t.Fatalf("PAR call count after seed flow = %d, want 2 (challenge + retry)", as.parCallCount) + } + + // The server has since rotated its nonce — the client's cached + // value ("first-nonce") is now stale, but the flow must still + // succeed via its own challenge/retry. + as.challengeParDPoPNonce = "rotated-nonce" + as.nextDPoPNonce = "rotated-nonce" + if _, err := c.BeginAuthorization(ctx, client.BeginAuthorizationRequest{Scope: []string{"openid"}}); err != nil { + t.Fatalf("BeginAuthorization (stale cache): %v", err) + } + if as.parCallCount != 4 { + t.Fatalf("PAR call count = %d, want 4 (2 from the seed flow + 2 more: the second flow still needed its own challenge/retry)", as.parCallCount) + } +} + func TestBeginAuthorizationRetriesOnPARDPoPNonceChallenge(t *testing.T) { c, as, _ := newTestClientWithPARBinding(t, client.PARDPoPBindingProof) as.challengeParDPoPNonce = "server-issued-par-nonce-1" diff --git a/client/noncecache.go b/client/noncecache.go new file mode 100644 index 0000000..a1751fb --- /dev/null +++ b/client/noncecache.go @@ -0,0 +1,97 @@ +package client + +import ( + "context" + "net/url" + "sync" +) + +// asNonceScope is the DPoPNonceCache key shared by the token endpoint +// and PAR — a Client is already bound to exactly one issuer, and +// server's own Dependencies.Nonces treats PAR and the token endpoint as +// one shared nonce space (RFC 9449 §8), so a nonce obtained from either +// is valid to present to either. +const asNonceScope = "as" + +// resourceNonceScope is a ResourceClient.Do call's DPoPNonceCache key: +// per resource-server origin, since RFC 9449 §9 nonces are scoped to +// the resource server that issued them and Do can be pointed at any +// resource URL, not just one fixed endpoint. +func resourceNonceScope(u *url.URL) string { + return u.Scheme + "://" + u.Host +} + +// DPoPNonceCache remembers the most recent DPoP nonce a server handed +// this client for a given scope, so the next call can present it +// proactively instead of always paying the initial challenge round +// trip RFC 9449 §8/§9 otherwise requires. It is a hint, not a ledger — +// losing it, or a stale/wrong value, only costs one extra +// challenge/retry round trip on the next call, recovered by the +// existing retry logic exactly as if this cache didn't exist at all. +// Dependencies.DPoPNonceCache being nil disables the optimization +// entirely — every call behaves exactly as it always has. +type DPoPNonceCache interface { + // Get returns the nonce last cached for scope, and whether one was + // found at all. + Get(ctx context.Context, scope string) (nonce string, ok bool) + + // Set replaces the cached nonce for scope. Called with whatever a + // response's own "DPoP-Nonce" header carried, regardless of that + // response's status — a server may send one on an unrelated error, + // or on success, purely to pre-seed the caller's next request. + Set(ctx context.Context, scope string, nonce string) +} + +// InMemoryDPoPNonceCache is a DPoPNonceCache backed by a plain map, safe +// for concurrent use. It is a ready-made implementation a caller can +// wire in explicitly — supplying it is as deliberate a choice as +// Clock: SystemClock{} is; New never installs one on its own. +type InMemoryDPoPNonceCache struct { + mu sync.Mutex + nonces map[string]string +} + +// NewInMemoryDPoPNonceCache builds an empty InMemoryDPoPNonceCache. +func NewInMemoryDPoPNonceCache() *InMemoryDPoPNonceCache { + return &InMemoryDPoPNonceCache{nonces: make(map[string]string)} +} + +// Get implements DPoPNonceCache. +func (c *InMemoryDPoPNonceCache) Get(_ context.Context, scope string) (string, bool) { + c.mu.Lock() + defer c.mu.Unlock() + nonce, ok := c.nonces[scope] + return nonce, ok +} + +// Set implements DPoPNonceCache. +func (c *InMemoryDPoPNonceCache) Set(_ context.Context, scope string, nonce string) { + c.mu.Lock() + defer c.mu.Unlock() + c.nonces[scope] = nonce +} + +// cachedDPoPNonce returns the nonce cached for scope, or "" if +// Dependencies.DPoPNonceCache is nil or has none — the same value every +// nonce-aware call site used unconditionally before this cache existed. +func (c *Client) cachedDPoPNonce(ctx context.Context, scope string) string { + if c.deps.DPoPNonceCache == nil { + return "" + } + nonce, ok := c.deps.DPoPNonceCache.Get(ctx, scope) + if !ok { + return "" + } + return nonce +} + +// cacheDPoPNonce records nonce for scope, if both a cache is configured +// and nonce is non-empty — a response with no "DPoP-Nonce" header +// leaves whatever was cached before untouched, rather than overwriting +// it with an empty value. +func (c *Client) cacheDPoPNonce(ctx context.Context, scope, nonce string) { + if c.deps.DPoPNonceCache == nil || nonce == "" { + return + } + c.deps.DPoPNonceCache.Set(ctx, scope, nonce) +} diff --git a/client/noncecache_test.go b/client/noncecache_test.go new file mode 100644 index 0000000..8e7ee4b --- /dev/null +++ b/client/noncecache_test.go @@ -0,0 +1,66 @@ +package client_test + +import ( + "context" + "sync" + "testing" + + "github.com/idfoundry/fapigo/client" +) + +func TestInMemoryDPoPNonceCacheRoundTrips(t *testing.T) { + c := client.NewInMemoryDPoPNonceCache() + ctx := context.Background() + + if _, ok := c.Get(ctx, "as"); ok { + t.Fatalf("Get(unset scope) ok = true, want false") + } + + c.Set(ctx, "as", "nonce-1") + got, ok := c.Get(ctx, "as") + if !ok || got != "nonce-1" { + t.Fatalf("Get(as) = (%q, %v), want (%q, true)", got, ok, "nonce-1") + } + + c.Set(ctx, "as", "nonce-2") + got, ok = c.Get(ctx, "as") + if !ok || got != "nonce-2" { + t.Fatalf("Get(as) after overwrite = (%q, %v), want (%q, true)", got, ok, "nonce-2") + } +} + +func TestInMemoryDPoPNonceCacheIsolatesScopes(t *testing.T) { + c := client.NewInMemoryDPoPNonceCache() + ctx := context.Background() + + c.Set(ctx, "as", "as-nonce") + c.Set(ctx, "https://rs.example.com", "rs-nonce") + + if got, ok := c.Get(ctx, "as"); !ok || got != "as-nonce" { + t.Fatalf("Get(as) = (%q, %v), want (%q, true)", got, ok, "as-nonce") + } + if got, ok := c.Get(ctx, "https://rs.example.com"); !ok || got != "rs-nonce" { + t.Fatalf("Get(rs) = (%q, %v), want (%q, true)", got, ok, "rs-nonce") + } + if _, ok := c.Get(ctx, "https://other.example.com"); ok { + t.Fatalf("Get(never-set scope) ok = true, want false") + } +} + +func TestInMemoryDPoPNonceCacheConcurrentUse(t *testing.T) { + c := client.NewInMemoryDPoPNonceCache() + ctx := context.Background() + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(2) + go func() { + defer wg.Done() + c.Set(ctx, "as", "nonce") + }() + go func() { + defer wg.Done() + c.Get(ctx, "as") + }() + } + wg.Wait() +} diff --git a/client/resource.go b/client/resource.go index 7615898..902e273 100644 --- a/client/resource.go +++ b/client/resource.go @@ -81,7 +81,8 @@ func (rc *ResourceClient) Do(ctx context.Context, req *http.Request) (*http.Resp ctx, cancel := context.WithTimeout(ctx, c.cfg.Limits.HTTPTimeout) defer cancel() - res, err := rc.send(ctx, dpopSigner, req, "") + scope := resourceNonceScope(req.URL) + res, err := rc.send(ctx, dpopSigner, req, c.cachedDPoPNonce(ctx, scope)) if err != nil { return nil, newError(ErrorInternal, "protected resource request failed", err) } @@ -97,6 +98,7 @@ func (rc *ResourceClient) Do(ctx context.Context, req *http.Request) (*http.Resp return nil, newError(ErrorInternal, "protected resource request failed", err) } } + c.cacheDPoPNonce(ctx, scope, res.Header.Get("DPoP-Nonce")) bounded, err := boundResponseBody(res, c.cfg.Limits.MaxHTTPResponseBytes) if err != nil { diff --git a/client/resource_test.go b/client/resource_test.go index 7f5568e..e1158cc 100644 --- a/client/resource_test.go +++ b/client/resource_test.go @@ -160,6 +160,77 @@ func TestProtectedResourceDoRetriesOnNonceChallenge(t *testing.T) { } } +// RFC 9449 §9's own proactive-refresh recommendation only helps a +// client that tracks and reuses the nonce a response handed it. With a +// DPoPNonceCache configured, a second, independent Do call to the same +// resource origin must need no challenge/retry round trip, since the +// first call's own successful response caches a nonce +// (resourceNonceScope, keyed by scheme+host). +func TestProtectedResourceDoReusesCachedNonceOnSecondCall(t *testing.T) { + const serverNonce = "server-nonce-1" + var mu sync.Mutex + var calls int + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + calls++ + call := calls + mu.Unlock() + claims := dpopProofClaims(t, r.Header.Get("DPoP")) + nonce, _ := claims["nonce"].(string) + if call == 1 && nonce != serverNonce { + w.Header().Set("WWW-Authenticate", `DPoP error="use_dpop_nonce", error_description="nonce required"`) + w.Header().Set("DPoP-Nonce", serverNonce) + w.WriteHeader(http.StatusUnauthorized) + return + } + w.Header().Set("DPoP-Nonce", serverNonce) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + })) + defer ts.Close() + + cfg := validConfig(t) + deps := validDependencies(t) + deps.HTTP = ts.Client() + deps.DPoPNonceCache = client.NewInMemoryDPoPNonceCache() + c, err := client.New(cfg, deps) + if err != nil { + t.Fatalf("client.New: %v", err) + } + + doOnce := func() { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, ts.URL+"/resource", nil) + if err != nil { + t.Fatalf("NewRequestWithContext: %v", err) + } + res, err := c.ProtectedResource(client.TokenSet{AccessToken: fapi.NewSecret("test-access-token")}).Do(context.Background(), req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200", res.StatusCode) + } + } + + doOnce() + mu.Lock() + callsAfterFirst := calls + mu.Unlock() + if callsAfterFirst != 2 { + t.Fatalf("calls after first Do = %d, want 2 (challenge + retry)", callsAfterFirst) + } + + doOnce() + mu.Lock() + callsAfterSecond := calls + mu.Unlock() + if callsAfterSecond != 3 { + t.Fatalf("calls after second Do = %d, want 3 (no retry needed)", callsAfterSecond) + } +} + // TestProtectedResourceDoDoesNotRetryOnBareNonceHeader confirms Do // requires the WWW-Authenticate challenge itself, not just a DPoP-Nonce // header — mirroring the token endpoint's own isDPoPNonceError