fix(server): check the DPoP nonce before consuming the request object

.github/mutation/mutants.json

@@ -193,15 +193,15 @@
   },
   {
     "id": "srv-ciba-dpop-nonce",
-    "file": "server/backchannel_authentication.go",
+    "file": "server/par.go",
     "old": "if s.deps.Nonces != nil {\n\t\tif challenge := s.checkDPoPNonce(",
     "new": "if s.deps.Nonces != nil && false {\n\t\tif challenge := s.checkDPoPNonce(",
     "module": "root",
     "packages": [
       "./server"
     ],
-    "run": "^TestBackchannelAuthenticationChallengesDPoPNonce$",
-    "why": "the backchannel authentication endpoint enforces DPoP nonces"
+    "run": "^(TestBackchannelAuthenticationChallengesDPoPNonce|TestPushAuthorizationRequestChallengesMissingNonceWhenDPoPPresented)$",
+    "why": "the backchannel authentication and PAR endpoints enforce DPoP nonces"
   },
   {
     "id": "srv-access-token-size-cap",

ARCHITECTURE.md

@@ -1180,7 +1180,7 @@ whenever `Config.MTLSEndpoints` is configured, the suite's own
 (confirmed by disassembly); (2) a DPoP proof's `htu` claim was checked
 against this server's plain endpoint URL only, in both
 `verifyTokenRequestDPoP` (`server/token.go`) and
-`reconcileParDPoPBinding` (`server/par.go`) — but a client
+`verifyOptionalDPoPProof` (`server/par.go`) — but a client
 authenticating via certificate must call the mTLS-alias URL to present
 it at all, so its DPoP proof legitimately names that alias instead;
 confirmed live via the token endpoint's `dpop: htu does not match

conformance/server/oidf-config/README.md

@@ -324,7 +324,7 @@ run:
   at all without that listener existing.
 - **Fixed: a DPoP proof presented at the mTLS-alias URL was rejected
   outright.** Both `verifyTokenRequestDPoP` (`server/token.go`) and
-  `reconcileParDPoPBinding` (`server/par.go`) checked a proof's `htu`
+  `verifyOptionalDPoPProof` (`server/par.go`) checked a proof's `htu`
   against this server's *plain* endpoint URL only. That's fine for
   every scenario tried before this profile — `sender_constrain=mtls`
   never uses DPoP at all, and every DPoP-sender-constrained client

interop/README.md

@@ -92,16 +92,4 @@ the one at fault.
 
 ## Known disagreements
 
-- **DPoP nonce retry of a pushed signed request object (FAPIgo).** With
-  DPoP nonces on, `server.PushAuthorizationRequest` verifies the request
-  object, recording its `jti` as used, before it checks the DPoP proof's
-  nonce. The request is then refused with `use_dpop_nonce`, and
-  openid-client retries it with the nonce, as RFC 9449 §8 expects of a
-  client, resending the same signed request object; FAPIgo refuses the
-  retry as a replay (`invalid_request_object`). A request the server
-  refused shouldn't use up its request object: RFC 9449 §8 makes the
-  retry part of the normal flow, and nothing in RFC 9101 or FAPI 2.0
-  Message Signing asks a client to re-sign it. Until FAPIgo checks the
-  nonce first (or records the `jti` only once the request succeeds), the
-  Message Signing instance runs with `-dpop-nonce-challenge=false`; leg 1
-  covers nonce handling.
+None known.

interop/run-openid-client.sh

@@ -79,13 +79,8 @@ echo "interop: starting conformance-as"
 "${work_dir}/conformance-as" -config "${work_dir}/as-baseline.json" \
   -mtls -ciba -client-credentials-grant >"${work_dir}/as-baseline.log" 2>&1 &
 server_pids+=("$!")
-# -dpop-nonce-challenge=false: with nonces on, the PAR request that gets
-# the use_dpop_nonce challenge has already recorded its request object's
-# jti, so openid-client's retry of the same request (RFC 9449 §8) is
-# refused as a replay. A FAPIgo issue: see interop/README.md, "Known
-# disagreements". Leg 1 covers nonce handling on the baseline instance.
 "${work_dir}/conformance-as" -config "${work_dir}/as-message-signing.json" \
-  -dpop-nonce-challenge=false >"${work_dir}/as-message-signing.log" 2>&1 &
+  >"${work_dir}/as-message-signing.log" 2>&1 &
 server_pids+=("$!")
 
 for port in "${INTEROP_AS_PORT}" "${INTEROP_AS_MTLS_PORT}" "${INTEROP_AS_MS_PORT}"; do

server/backchannel_authentication.go

@@ -16,7 +16,6 @@ import (
 
 	fapi "github.com/idfoundry/fapigo"
 	"github.com/idfoundry/fapigo/extension"
-	"github.com/idfoundry/fapigo/internal/dpop"
 	"github.com/idfoundry/fapigo/internal/requestobject"
 	"github.com/idfoundry/fapigo/keys"
 	"github.com/idfoundry/fapigo/storage"
@@ -170,6 +169,14 @@ func (s *Server) BeginBackchannelAuthentication(ctx context.Context, req BeginBa
 	if authErr != nil {
 		return s.backchannelBeginFail(ctx, "", authErr), nil
 	}
+	// Before the signed authentication request, which records its jti:
+	// see verifyOptionalDPoPProof. A client authenticating with its
+	// certificate calls the mTLS alias (RFC 8705 §5), and its proof names
+	// that URL, as at PAR and the token endpoint.
+	dpopThumbprint, dpopErr := s.verifyOptionalDPoPProof(ctx, dpopProof, s.cfg.Endpoints.BackchannelAuthentication, s.cfg.MTLSEndpoints.BackchannelAuthentication)
+	if dpopErr != nil {
+		return s.backchannelBeginFail(ctx, client.ID(), dpopErr), nil
+	}
 
 	verified, resolveErr := s.resolveBackchannelAuthenticationParameters(ctx, params, client)
 	if resolveErr != nil {
@@ -186,9 +193,9 @@ func (s *Server) BeginBackchannelAuthentication(ctx context.Context, req BeginBa
 		return s.backchannelBeginFail(ctx, client.ID(), subjectErr), nil
 	}
 
-	dpopJKT, dpopErr := s.reconcileBackchannelDPoPBinding(ctx, dpopProof, validated.params)
-	if dpopErr != nil {
-		return s.backchannelBeginFail(ctx, client.ID(), dpopErr), nil
+	dpopJKT, bindErr := reconcileBackchannelDPoPBinding(dpopThumbprint, validated.params)
+	if bindErr != nil {
+		return s.backchannelBeginFail(ctx, client.ID(), bindErr), nil
 	}
 
 	interaction, err := s.backchannelInteraction(client, validated.params, validated.resources)
@@ -623,38 +630,20 @@ func hasVisibleContent(msg string) bool {
 // counterpart: a DPoP proof at the backchannel authentication endpoint
 // is optional, exactly like PAR — see Open Question 1 in this feature's
 // design plan for why this mirrors PAR rather than the token endpoint's
-// always-required proof. It returns the key thumbprint the request is
-// bound to, which the token exchange then requires its DPoP proof's key
-// to match: the proof's key, or an explicit dpop_jkt parameter (RFC 9449
-// §10, applied here as at PAR) when there's no proof. A dpop_jkt that
-// doesn't match a proof sent with it is refused.
-func (s *Server) reconcileBackchannelDPoPBinding(ctx context.Context, proof string, params map[string]json.RawMessage) (string, *Error) {
+// always-required proof. thumbprint is the key thumbprint of the proof
+// verifyOptionalDPoPProof already verified, or "" when there was none.
+// It returns the key thumbprint the request is bound to, which the token
+// exchange then requires its DPoP proof's key to match: the proof's key,
+// or an explicit dpop_jkt parameter (RFC 9449 §10, applied here as at
+// PAR) when there's no proof. A dpop_jkt that doesn't match a proof sent
+// with it is refused.
+func reconcileBackchannelDPoPBinding(thumbprint string, params map[string]json.RawMessage) (string, *Error) {
 	// A dpop_jkt that isn't a non-empty string was refused with the
 	// request's other parameters (backchannelParameterTypes).
 	declared, _ := jsonString(params, "dpop_jkt")
-	if proof == "" {
+	if thumbprint == "" {
 		return declared, nil
 	}
-	// A client authenticating with its certificate calls the mTLS alias
-	// (RFC 8705 §5), and its proof names that URL, as at PAR and the
-	// token endpoint.
-	verified, err := verifyDPoPAtEitherEndpoint(ctx, dpop.VerifyRequest{
-		Proof:        proof,
-		Method:       "POST",
-		Now:          s.deps.Clock.Now(),
-		MaxProofAge:  s.cfg.Limits.MaxDPoPProofAge,
-		MaxClockSkew: s.cfg.Limits.MaxClockSkew,
-		Replay:       s.dpopReplayChecker(),
-	}, s.cfg.Endpoints.BackchannelAuthentication, s.cfg.MTLSEndpoints.BackchannelAuthentication)
-	if err != nil {
-		return "", newError(ErrorInvalidDPoPProof, 400, "DPoP proof verification failed", err)
-	}
-	if s.deps.Nonces != nil {
-		if challenge := s.checkDPoPNonce(ctx, verified.Nonce, s.deps.Clock.Now()); challenge != nil {
-			return "", challenge
-		}
-	}
-	thumbprint := verified.Thumbprint.String()
 	if declared != "" && declared != thumbprint {
 		return "", newError(ErrorInvalidRequest, 400, "dpop_jkt does not match the DPoP proof presented to the backchannel authentication endpoint", nil)
 	}

server/backchannel_dpop_jkt_test.go

@@ -190,3 +190,56 @@ func TestBackchannelAuthenticationChallengesDPoPNonce(t *testing.T) {
 		t.Fatalf("proof with the issued nonce: action = %T, want BackchannelInteractionRequired", action)
 	}
 }
+
+// TestBackchannelAuthenticationNonceChallengeKeepsRequestUsable: a
+// use_dpop_nonce challenge must not consume the signed authentication
+// request's jti. The client retries the same signed request with a fresh
+// proof carrying the nonce, and that retry succeeds; only a second
+// successful use is a replay.
+func TestBackchannelAuthenticationNonceChallengeKeepsRequestUsable(t *testing.T) {
+	endpoint, err := url.Parse(testBackchannelAuthenticationEndpoint)
+	if err != nil {
+		t.Fatalf("url.Parse: %v", err)
+	}
+	h := newHarnessWithBackchannelOptions(t, memstore.NewBackchannelAuthenticationStore(),
+		func(cfg *server.Config) { cfg.Limits.DPoPNonceLifetime = time.Minute },
+		func(deps *server.Dependencies) { deps.Nonces = memstore.NewNonceStore() },
+	)
+	dpopKey := generateKey(t)
+	requestObj := h.backchannelRequestObject(t, standardBackchannelParams(t))
+	begin := func(nonce string) server.BackchannelAuthenticationAction {
+		t.Helper()
+		proof, err := dpop.CreateProof(dpop.ProofRequest{Signer: dpopKey, Algorithm: fapi.ES256, Method: "POST", URL: endpoint, Now: h.now, Nonce: nonce})
+		if err != nil {
+			t.Fatalf("CreateProof: %v", err)
+		}
+		action, err := h.server.BeginBackchannelAuthentication(context.Background(), server.BeginBackchannelAuthenticationRequest{
+			HTTP:       server.FormRequest{Parameters: backchannelFormParams(h.clientAssertion(t), requestObj)},
+			DPoPProofs: []string{proof},
+		})
+		if err != nil {
+			t.Fatalf("BeginBackchannelAuthentication: %v", err)
+		}
+		return action
+	}
+
+	localErr, ok := begin("").(server.BackchannelAuthenticationLocalError)
+	if !ok || localErr.Error.Code() != server.ErrorUseDPoPNonce {
+		t.Fatalf("proof without a nonce: want a use_dpop_nonce BackchannelAuthenticationLocalError")
+	}
+	issued := localErr.Error.Nonce()
+
+	if action := begin(issued); !isBackchannelInteractionRequired(action) {
+		t.Fatalf("retry of the same signed request with the issued nonce: action = %#v, want BackchannelInteractionRequired", action)
+	}
+
+	localErr, ok = begin(issued).(server.BackchannelAuthenticationLocalError)
+	if !ok || localErr.Error.Code() != server.ErrorInvalidRequest {
+		t.Fatalf("signed request used a second time: want an invalid_request BackchannelAuthenticationLocalError")
+	}
+}
+
+func isBackchannelInteractionRequired(action server.BackchannelAuthenticationAction) bool {
+	_, ok := action.(server.BackchannelInteractionRequired)
+	return ok
+}

server/nonce_test.go

@@ -56,19 +56,24 @@ func TestNewRejectsInvalidNonceConfig(t *testing.T) {
 // newHarnessWithNonces mirrors newHarnessWithIdentityClaims's shape
 // (identity_claims_test.go), but wires a memstore.NonceStore and
 // Limits.DPoPNonceLifetime so token/PAR nonce-challenge tests don't have
-// to duplicate every other piece of a valid harness.
-func newHarnessWithNonces(t *testing.T) (harness, *memstore.NonceStore) {
+// to duplicate every other piece of a valid harness. mutateClient, if
+// given, adjusts the registered client's configuration.
+func newHarnessWithNonces(t *testing.T, mutateClient ...func(*storage.RegisteredClientConfig)) (harness, *memstore.NonceStore) {
 	t.Helper()
 	now := time.Now()
 	key := generateKey(t)
 	serverKey := generateKey(t)
 
-	client, err := storage.NewRegisteredClient(storage.RegisteredClientConfig{
+	clientCfg := storage.RegisteredClientConfig{
 		ID:                       testClientID,
 		RedirectURIs:             []fapi.RegisteredRedirectURI{testRedirectURI},
 		ClientAssertionAlgorithm: fapi.ES256,
 		AllowedScopes:            []string{"openid", "accounts", "offline_access"},
-	})
+	}
+	for _, mutate := range mutateClient {
+		mutate(&clientCfg)
+	}
+	client, err := storage.NewRegisteredClient(clientCfg)
 	if err != nil {
 		t.Fatalf("NewRegisteredClient: %v", err)
 	}
@@ -519,6 +524,53 @@ func TestPushAuthorizationRequestAcceptsValidNonceAndHandsOutNext(t *testing.T)
 	}
 }
 
+// TestPushAuthorizationRequestNonceChallengeKeepsRequestObjectUsable:
+// a use_dpop_nonce challenge must not consume the request object's jti.
+// The client retries the same signed request object with a fresh proof
+// carrying the nonce (RFC 9449 §8 asks it to re-send, not re-sign), and
+// that retry succeeds; only a second successful use is a replay.
+func TestPushAuthorizationRequestNonceChallengeKeepsRequestObjectUsable(t *testing.T) {
+	h, _ := newHarnessWithNonces(t, func(cfg *storage.RegisteredClientConfig) { cfg.RequestObjectAlgorithm = fapi.ES256 })
+	dpopKey := generateKey(t)
+	parURL, err := url.Parse(testPAREndpoint)
+	if err != nil {
+		t.Fatalf("url.Parse: %v", err)
+	}
+	requestObj := h.requestObject(t, standardAuthParams(t))
+	push := func(nonce string) error {
+		t.Helper()
+		proof, err := dpop.CreateProof(dpop.ProofRequest{
+			Signer: dpopKey, Algorithm: fapi.ES256, Method: "POST", URL: parURL, Now: h.now, Nonce: nonce,
+		})
+		if err != nil {
+			t.Fatalf("dpop.CreateProof: %v", err)
+		}
+		_, err = h.server.PushAuthorizationRequest(context.Background(), server.PushAuthorizationRequest{
+			HTTP: server.FormRequest{Parameters: []server.FormParameter{
+				formParam("client_assertion", h.clientAssertion(t)),
+				formParam("client_assertion_type", clientassertion.AssertionType),
+				formParam("request", requestObj),
+			}},
+			DPoPProofs: []string{proof},
+		})
+		return err
+	}
+
+	err = push("")
+	if code := serverErrorCode(t, err); code != server.ErrorUseDPoPNonce {
+		t.Fatalf("proof without a nonce: error code = %q, want %q", code, server.ErrorUseDPoPNonce)
+	}
+	issued := err.(*server.Error).Nonce()
+
+	if err := push(issued); err != nil {
+		t.Fatalf("retry of the same request object with the issued nonce: %v, want success", err)
+	}
+
+	if code := serverErrorCode(t, push(issued)); code != server.ErrorInvalidRequestObject {
+		t.Fatalf("request object used a second time: error code = %q, want %q", code, server.ErrorInvalidRequestObject)
+	}
+}
+
 // TestNonceIssuedAtPARIsValidAtTokenEndpoint confirms one shared nonce
 // store covers everything this server verifies (Dependencies.Nonces's
 // own doc comment) — a nonce issued from a PAR challenge must also be