From 45189bcfba23f75142a55920d1ce042184c9d368 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 13:52:30 +0800 Subject: [PATCH 1/8] fix(server)!: limit the RSA keys of attestation x5c certificates X5CAttesterChain parsed the certificates of an attestation's x5c and path-built over them before anything in the request was authenticated. crypto/x509 bounds neither an RSA modulus nor its exponent, so a certificate with a 31,744-bit modulus and exponent 2^31-1 cost about 60 ms of CPU per unauthenticated token request. Every x5c certificate's key is now held to the limits v0.63.0 set for any other RSA key a caller supplies, at most 4096 bits and exponent 65537, through the new internal jose.ValidateCallerCertificateKey, before isSelfSigned or chain verification uses it. Configured anchors are not limited. BREAKING CHANGE: X5CAttesterChain refuses with invalid_client an attestation whose x5c carries an RSA key over 4096 bits or with an exponent other than 65537, on the signing certificate or an intermediate. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 28 +++++++++ internal/jose/callerkey.go | 13 ++++ server/attester_trust.go | 13 ++++ server/attester_trust_test.go | 115 ++++++++++++++++++++++++++++++++++ 4 files changed, 169 insertions(+) diff --git a/UPGRADING.md b/UPGRADING.md index 19469820..a25e84d6 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -95,6 +95,34 @@ example letters and digits: ```go req.BindingMessage = "W4SCT" + +### `X5CAttesterChain` limits the RSA keys of `x5c` certificates (server) + +**Affects:** a server whose `Dependencies.AttesterTrust` is +`server.X5CAttesterChain`, when a wallet's attestation `x5c` carries a +certificate — the signing certificate or an intermediate — whose +public key is RSA with a modulus over 4096 bits or a public exponent +other than 65537. Anchors you configure (`TrustAnchors`, `Anchors`) +aren't limited. + +**Why:** the `x5c` certificates are the caller's, and `crypto/x509` +bounds neither an RSA modulus nor its exponent. Path building spent an +RSA operation on whatever key an unauthenticated request carried, so a +31,744-bit modulus with exponent 2^31-1 cost the server about 60 ms of +CPU per request. v0.63.0 already limited every other RSA key a caller +supplies (a DPoP `jwk`, an attestation's `cnf.jwk`, a client's JWK Set) +to 4096 bits and exponent 65537; `x5c` now follows the same rule, +checked before any signature. + +**What fails:** nothing at startup. Such an attestation is refused +with `invalid_client`. There is no option to keep the old behaviour. + +**What to change:** have the attester's CA issue its signing and +intermediate certificates for 2048-, 3072- or 4096-bit RSA keys with +exponent 65537, or for EC keys: + +```sh +openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:4096 -out attester-ca.pem ``` ## v0.68.0 diff --git a/internal/jose/callerkey.go b/internal/jose/callerkey.go index aa37b89a..51ece51f 100644 --- a/internal/jose/callerkey.go +++ b/internal/jose/callerkey.go @@ -57,3 +57,16 @@ func checkCallerKeyMaterial(pub crypto.PublicKey) error { } return nil } + +// ValidateCallerCertificateKey checks pub, the public key of a +// certificate a caller supplied — one in an attestation's "x5c" chain, +// say — against the key-material rules ParseJWK applies to a caller's +// key, whatever algorithm the certificate is used with: an RSA modulus +// of at most 4096 bits (maxCallerRSAModulusBits) and exponent 65537. +// Run it before the certificate's key verifies anything, path building +// included: crypto/x509 accepts any modulus size and exponents up to +// 2^31-1, so an unchecked certificate lets the caller choose how much +// CPU each signature check costs. +func ValidateCallerCertificateKey(pub crypto.PublicKey) error { + return checkCallerKeyMaterial(pub) +} diff --git a/server/attester_trust.go b/server/attester_trust.go index 9f71873c..01af743e 100644 --- a/server/attester_trust.go +++ b/server/attester_trust.go @@ -17,6 +17,7 @@ import ( fapi "github.com/idfoundry/fapigo" "github.com/idfoundry/fapigo/internal/clientattestation" "github.com/idfoundry/fapigo/internal/depcheck" + "github.com/idfoundry/fapigo/internal/jose" "github.com/idfoundry/fapigo/keys" "github.com/idfoundry/fapigo/storage" ) @@ -130,6 +131,12 @@ func (r RegisteredAttesterKeys) validate() error { // fallback to "kid", which would let an attestation skip the chain // check just by omitting it — and "kid" plays no part. // +// Every "x5c" certificate holding an RSA key must hold one of at most +// 4096 bits with exponent 65537, the limits on any RSA key a caller +// supplies; this is checked before any signature, so a chain can't +// choose how much work verifying it costs. Anchors TrustAnchors or +// Anchors return are configured, not sent, and aren't limited. +// // The signing certificate must: // // - chain to one of the anchors through the other "x5c" certificates, @@ -552,6 +559,12 @@ func verifyAttesterChain(ders [][]byte, roots *x509.CertPool, now time.Time) (*x if err != nil { return nil, nil, fmt.Errorf("parse x5c certificate %d: %w", i, err) } + // Before any of these keys checks a signature (isSelfSigned, + // path building): the caller chose them, and crypto/x509 bounds + // neither an RSA modulus nor its exponent. + if err := jose.ValidateCallerCertificateKey(cert.PublicKey); err != nil { + return nil, nil, fmt.Errorf("x5c certificate %d: %w", i, err) + } certs[i] = cert } leaf := certs[0] diff --git a/server/attester_trust_test.go b/server/attester_trust_test.go index 769dec2b..78ef8837 100644 --- a/server/attester_trust_test.go +++ b/server/attester_trust_test.go @@ -581,3 +581,118 @@ func TestRegisteredAttesterKeysNoKeyForAlgorithm(t *testing.T) { t.Fatalf("error code = %q, want %q (err %v)", code, server.ErrorInvalidClient, err) } } + +// TestX5CAttesterChainRefusesUnboundedRSAKeys covers an x5c whose +// certificates carry RSA keys the caller chose to be costly: a modulus +// over 4096 bits or an exponent other than 65537, on the signing +// certificate or on an "intermediate" it names as its issuer. Nothing +// chains to the anchor, and the moduli were never generated, so a +// refusal from chain verification would be x509's unknown authority +// after one costly RSA operation per candidate; the key limits must +// refuse each first, without any signature check. +func TestX5CAttesterChainRefusesUnboundedRSAKeys(t *testing.T) { + root := newTestCert(t, "trust list CA", certOptions{isCA: true}) + trust := server.X5CAttesterChain{Revocation: server.NoClientCertificateRevocationCheck{}, TrustAnchors: server.StaticAttesterTrustAnchors{Roots: poolOf(root)}, IssuerBinding: server.AttesterIssuerInCertificate} + // An RSA signer, so the certificates' signature algorithm is one the + // costly RSA key would be used for. + signer, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa.GenerateKey: %v", err) + } + for name, tc := range map[string]struct { + bits int + exponent int + onLeaf bool + want error + }{ + "31744-bit intermediate, exponent 2^31-1": {31744, 1<<31 - 1, false, jose.ErrRSAKeyTooLarge}, + "4104-bit intermediate": {4104, 65537, false, jose.ErrRSAKeyTooLarge}, + "intermediate with exponent 2^31-1": {2048, 1<<31 - 1, false, jose.ErrUnsupportedRSAExponent}, + "intermediate with exponent 3": {2048, 3, false, jose.ErrUnsupportedRSAExponent}, + "8192-bit signing certificate": {8192, 65537, true, jose.ErrRSAKeyTooLarge}, + "signing certificate with exponent 3": {2048, 3, true, jose.ErrUnsupportedRSAExponent}, + } { + t.Run(name, func(t *testing.T) { + h := newHarnessWithAttesterTrust(t, nil, trust) + x5c, err := json.Marshal(costlyRSAX5C(t, signer, tc.bits, tc.exponent, tc.onLeaf)) // []byte marshals as standard base64 + if err != nil { + t.Fatalf("marshal x5c: %v", err) + } + instanceKey := generateKey(t) + err = requestWithAttestation(t, h, createX5CAttestation(t, generateKey(t), x5c, "", &instanceKey.PublicKey, h.now), instanceKey) + if code := serverErrorCode(t, err); code != server.ErrorInvalidClient { + t.Fatalf("error code = %q, want %q (err %v)", code, server.ErrorInvalidClient, err) + } + if !errors.Is(err, tc.want) { + t.Fatalf("err = %v, want it to wrap %v (refused by the key limits, before any signature check)", err, tc.want) + } + }) + } +} + +// costlyRSAX5C builds an attacker's x5c around an RSA public key with a +// random (never generated) modulus of the given bits and exponent, +// signed by signer: +// either as the signing certificate's own key, or as the key of an +// "intermediate" the signing certificate names as its issuer, whose +// signature is then junk as long as that modulus. +func costlyRSAX5C(t *testing.T, signer *rsa.PrivateKey, bits, exponent int, onLeaf bool) [][]byte { + t.Helper() + n, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), uint(bits))) + if err != nil { + t.Fatalf("rand.Int: %v", err) + } + n.SetBit(n, bits-1, 1) + n.SetBit(n, 0, 1) + costly := &rsa.PublicKey{N: n, E: exponent} + now := time.Now() + issuer := pkix.Name{CommonName: "attester CA"} + leafTemplate := &x509.Certificate{ + SerialNumber: big.NewInt(1), Subject: pkix.Name{CommonName: "attester"}, + NotBefore: now.Add(-time.Hour), NotAfter: now.Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + } + if onLeaf { + der, err := x509.CreateCertificate(rand.Reader, leafTemplate, &x509.Certificate{Subject: issuer}, costly, signer) + if err != nil { + t.Fatalf("create leaf: %v", err) + } + return [][]byte{der} + } + leafDER, err := x509.CreateCertificate(rand.Reader, leafTemplate, &x509.Certificate{Subject: issuer}, signer.Public(), signer) + if err != nil { + t.Fatalf("create leaf: %v", err) + } + intermediateDER, err := x509.CreateCertificate(rand.Reader, &x509.Certificate{ + SerialNumber: big.NewInt(2), Subject: issuer, + NotBefore: now.Add(-time.Hour), NotAfter: now.Add(time.Hour), + IsCA: true, BasicConstraintsValid: true, KeyUsage: x509.KeyUsageCertSign, + }, &x509.Certificate{Subject: pkix.Name{CommonName: "root"}}, costly, signer) + if err != nil { + t.Fatalf("create intermediate: %v", err) + } + return [][]byte{withJunkSignature(t, leafDER, bits), intermediateDER} +} + +// withJunkSignature replaces der's signature with bits/8 random bytes, +// as long as an RSA signature by a key of that modulus would be. +func withJunkSignature(t *testing.T, der []byte, bits int) []byte { + t.Helper() + var cert struct { + TBS asn1.RawValue + SigAlg pkix.AlgorithmIdentifier + Sig asn1.BitString + } + if _, err := asn1.Unmarshal(der, &cert); err != nil { + t.Fatalf("unmarshal certificate: %v", err) + } + sig := make([]byte, bits/8) + _, _ = rand.Read(sig) + sig[0] = 0x01 + cert.Sig = asn1.BitString{Bytes: sig, BitLength: bits} + out, err := asn1.Marshal(cert) + if err != nil { + t.Fatalf("marshal certificate: %v", err) + } + return out +} From f223e2b96a903c0462ef0e281ba4f6d470aaa7b3 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 13:55:51 +0800 Subject: [PATCH 2/8] fix(server): accept an x5c intermediate that is also a configured anchor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HAIP 1.0 §4.4.1 has the wallet leave the trust anchor out of x5c; it asks nothing of the verifier. X5CAttesterChain refused any x5c that carried a certificate for the anchor a verified chain ended at, so a conformant x5c [leaf, intermediate] verified against anchors {root} but was refused once the server also listed the intermediate as an anchor, which a wallet has no way to know. An attestation is now refused only when every verified chain ends at an anchor x5c carries itself (compared by subject and public key, as before); one chain ending at an anchor the wallet didn't send is enough. A self-signed certificate below the leaf is still refused. The native wallet guide no longer says HAIP requires the server to refuse, and the v0.66.0 and v0.68.0 upgrade notes say what changed. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 12 ++++++ docs/guides/native-wallet.md | 17 ++++++--- server/attester_chain_fuzz_test.go | 47 +++++++++++++----------- server/attester_trust.go | 59 ++++++++++++++++-------------- server/attester_trust_test.go | 19 ++++++---- 5 files changed, 93 insertions(+), 61 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index a25e84d6..0e961345 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -199,6 +199,12 @@ anchor, including cross-signed copies: x5c := [][]byte{leaf.Raw, intermediate.Raw} ``` +*Relaxed in v0.69.0:* a configured anchor in `x5c` that isn't +self-signed is accepted when a verified chain also ends at an anchor +`x5c` doesn't carry, as `[leaf, intermediate]` does with both the root +and the intermediate configured as anchors. It's refused only when +every verified chain ends at an anchor the wallet sent. + ## v0.66.0 ### `X5CAttesterChain` refuses other roles' signing certificates (server) @@ -302,6 +308,12 @@ anchor: x5c := [][]byte{leaf.Raw, intermediate.Raw} ``` +*Relaxed in v0.69.0:* a configured anchor in `x5c` that isn't +self-signed is accepted when a verified chain also ends at an anchor +`x5c` doesn't carry, as `[leaf, intermediate]` does with both the root +and the intermediate configured as anchors. It's refused only when +every verified chain ends at an anchor the wallet sent. + ## v0.65.0 ### `Limits.MaxAccessTokenClaimsBytes` is required (server) diff --git a/docs/guides/native-wallet.md b/docs/guides/native-wallet.md index d914b199..147d8463 100644 --- a/docs/guides/native-wallet.md +++ b/docs/guides/native-wallet.md @@ -41,12 +41,17 @@ needs `Config.AttestationBasedClientAuthentication` and an `AttesterTrust`: `server.RegisteredAttesterKeys` with the attesters' own keys, by issuer (a `keys.AttesterKeySource` such as `keys.StaticAttesterKeys` — never the clients' keys), or -`server.X5CAttesterChain` to verify the attestation's `x5c` chain. That -`x5c` carries the signing certificate and its intermediates only, as -HAIP 1.0 requires: an attestation whose `x5c` also includes a trust -anchor (any self-signed certificate, or a certificate with a -configured anchor's subject and public key, such as a cross-signed -copy of it) is `invalid_client`. With +`server.X5CAttesterChain` to verify the attestation's `x5c` chain. +HAIP 1.0 §4.4.1 has the wallet send the signing certificate and its +intermediates only, leaving out the trust anchor. The server refuses +an `x5c` that includes any self-signed certificate, or whose every +verified chain ends at an anchor `x5c` carries itself (a certificate +with a configured anchor's subject and public key, such as a +cross-signed copy of it), with `invalid_client`. An intermediate the +server also lists as an anchor is accepted when the chain verifies on +to another anchor, since the wallet can't know which of its CAs the +server lists. Each RSA key in `x5c` must be at most 4096 bits, with +exponent 65537. With a trust list shared by several attesters, use its `AttesterIssuerBoundToAnchor` binding, so each anchor vouches only for the attesters it's bound to. Its `LeafPolicy`, by default diff --git a/server/attester_chain_fuzz_test.go b/server/attester_chain_fuzz_test.go index fafef382..668cbdf1 100644 --- a/server/attester_chain_fuzz_test.go +++ b/server/attester_chain_fuzz_test.go @@ -105,7 +105,7 @@ func attesterChainRefusedOracle(chain []*x509.Certificate) bool { } // FuzzAttesterLeafPolicy checks DefaultAttesterLeafPolicy and -// checkX5CExcludesAnchors on chains of certificates whose extended key +// checkX5CReachesUnsentAnchor on chains of certificates whose extended key // usages, and shapes, come from the fuzzer. Beyond never panicking on // an empty or odd chain: // @@ -113,9 +113,9 @@ func attesterChainRefusedOracle(chain []*x509.Certificate) bool { // documented rule (attesterChainRefusedOracle) refuses; // - approvedAttesterChains keeps exactly the chains the policy // accepts, in order, and fails closed when it keeps none; -// - checkX5CExcludesAnchors refuses exactly when an x5c certificate -// below the leaf has the subject and public key of the last -// certificate of some chain. +// - checkX5CReachesUnsentAnchor refuses exactly when every chain's +// last certificate has the subject and public key of an x5c +// certificate below the leaf. func FuzzAttesterLeafPolicy(f *testing.F) { f.Add([]byte{}) f.Add([]byte{1, 0, 0, 0}) @@ -179,16 +179,16 @@ func checkX5CAnchorRule(t *testing.T, extra []*x509.Certificate, chains [][]*x50 // A certificate is identified, for path building, by its subject // and public key together. type identity struct{ subject, key string } - anchors := map[identity]bool{} - for _, c := range nonEmpty { - a := c[len(c)-1] - anchors[identity{string(a.RawSubject), string(a.RawSubjectPublicKeyInfo)}] = true + sent := map[identity]bool{} + for _, e := range extra { + sent[identity{string(e.RawSubject), string(e.RawSubjectPublicKeyInfo)}] = true } - want := slices.ContainsFunc(extra, func(e *x509.Certificate) bool { - return anchors[identity{string(e.RawSubject), string(e.RawSubjectPublicKeyInfo)}] + want := !slices.ContainsFunc(nonEmpty, func(c []*x509.Certificate) bool { + a := c[len(c)-1] + return !sent[identity{string(a.RawSubject), string(a.RawSubjectPublicKeyInfo)}] }) - if err := checkX5CExcludesAnchors(extra, nonEmpty); (err != nil) != want { - t.Fatalf("checkX5CExcludesAnchors = %v, want refusal %v", err, want) + if err := checkX5CReachesUnsentAnchor(extra, nonEmpty); (err != nil) != want { + t.Fatalf("checkX5CReachesUnsentAnchor = %v, want refusal %v", err, want) } } @@ -288,7 +288,8 @@ func newAttesterPool(f *testing.F) attesterPool { // - no self-signed certificate, and nothing that isn't a certificate, // anywhere in x5c; // - chains that each start at that certificate and end at one of the -// configured anchors, none of which x5c carries below the leaf. +// configured anchors, at least one of which x5c doesn't carry +// below the leaf. func FuzzVerifyAttesterChain(f *testing.F) { pool := newAttesterPool(f) f.Add([]byte{poolLeaf, poolIntermediate}, uint8(1< Date: Mon, 2 Nov 2026 13:58:15 +0800 Subject: [PATCH 3/8] fix(server): say an attestation can't be combined with another client auth method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A request carrying attestation headers together with a client_assertion or client_assertion_type stays refused with invalid_request (RFC 6749 §2.3), so an assertion naming another client is never ignored in favour of the attestation. The attestation-based client authentication draft-12 §7.6 also allows an attestation as an additional signal beside another method; that use is not supported, and the docs now say so: Config.AttestationBasedClientAuthentication, authenticateClient and the native wallet guide. The error_description now says the attestation can't be combined with another client authentication method, rather than only that the request uses more than one. Co-Authored-By: Claude Opus 5.5 --- docs/guides/native-wallet.md | 7 ++++++- server/attested_client.go | 2 +- server/client_auth_attestation_test.go | 18 ++++++++++++++++++ server/config.go | 8 ++++++++ server/par.go | 5 +++-- 5 files changed, 36 insertions(+), 4 deletions(-) diff --git a/docs/guides/native-wallet.md b/docs/guides/native-wallet.md index 147d8463..9df5ece6 100644 --- a/docs/guides/native-wallet.md +++ b/docs/guides/native-wallet.md @@ -36,7 +36,12 @@ A native client may use a private-use scheme in reverse-domain form, written with a single slash (`com.example.wallet:/callback`), loopback `http` to `127.0.0.1` or `[::1]` (never `localhost`, RFC 8252 §8.3), matched on any port, or a claimed `https` URI, in production as well; -`NewRegisteredClient` refuses any other form. The server also +`NewRegisteredClient` refuses any other form. The attestation is the +client's authentication method, not an extra signal beside another +one: the server doesn't support the attestation-based client +authentication draft's use alongside `private_key_jwt` (draft-12 +§7.6), and refuses a request carrying both an attestation and a +`client_assertion` with `invalid_request`. The server also needs `Config.AttestationBasedClientAuthentication` and an `AttesterTrust`: `server.RegisteredAttesterKeys` with the attesters' own keys, by issuer (a `keys.AttesterKeySource` such as diff --git a/server/attested_client.go b/server/attested_client.go index 0ce61de6..11b4e030 100644 --- a/server/attested_client.go +++ b/server/attested_client.go @@ -108,7 +108,7 @@ func (s *Server) AuthenticateAttestedClient(ctx context.Context, req AttestedCli if req.malformed { err = newError(ErrorInvalidRequest, 400, "the request's parameters are duplicated, too many, or too large", nil) } else if req.otherClientAuth { - err = newError(ErrorInvalidRequest, 400, "the request uses more than one client authentication method", nil) + err = newError(ErrorInvalidRequest, 400, "a client attestation cannot be combined with another client authentication method", nil) } else if err == nil && attestation == "" { err = newError(ErrorInvalidClient, 401, "client authentication is required", nil) } diff --git a/server/client_auth_attestation_test.go b/server/client_auth_attestation_test.go index 3f1221fc..0669e4bb 100644 --- a/server/client_auth_attestation_test.go +++ b/server/client_auth_attestation_test.go @@ -7,6 +7,7 @@ import ( "crypto/ed25519" "crypto/rand" "encoding/json" + "errors" "testing" "time" @@ -899,10 +900,26 @@ func TestAttestationWithClientAssertionRefused(t *testing.T) { if code := serverErrorCode(t, err); code != server.ErrorInvalidRequest { t.Fatalf("RequestClientCredentialsToken code %q, want invalid_request", code) } + checkAttestationCombinedDescription(t, err) }) } } +// checkAttestationCombinedDescription checks err's error_description +// says why: an attestation as an additional signal beside another +// client authentication method (draft-12 §7.6) isn't supported. +func checkAttestationCombinedDescription(t *testing.T, err error) { + t.Helper() + var srvErr *server.Error + if !errors.As(err, &srvErr) { + t.Fatalf("error %v is not a *server.Error", err) + } + const want = "a client attestation cannot be combined with another client authentication method" + if got := srvErr.PublicDescription(); got != want { + t.Fatalf("error_description = %q, want %q", got, want) + } +} + // TestAuthenticateAttestedClientWithClientAssertionRefused is // TestAttestationWithClientAssertionRefused for AuthenticateAttestedClient, // given a request from TokenEndpointRequest.AttestedClientAuthentication. @@ -921,4 +938,5 @@ func TestAuthenticateAttestedClientWithClientAssertionRefused(t *testing.T) { if code := serverErrorCode(t, err); code != server.ErrorInvalidRequest { t.Fatalf("AuthenticateAttestedClient code %q, want invalid_request", code) } + checkAttestationCombinedDescription(t, err) } diff --git a/server/config.go b/server/config.go index f92c0fee..b6824a40 100644 --- a/server/config.go +++ b/server/config.go @@ -589,6 +589,14 @@ type Config struct { // only private_key_jwt and mTLS. It's here for HAIP and OpenID4VCI // wallets; don't register a client that must be FAPI 2.0 compliant // for it. + // + // The attestation is supported only as the client authentication + // method, not as the additional security signal beside another + // method that draft-12 §7.6 allows: a request carrying attestation + // headers together with a client_assertion or client_assertion_type + // is refused with invalid_request (RFC 6749 §2.3: one method per + // request), so an assertion naming another client can't be ignored + // in favour of the attestation. AttestationBasedClientAuthentication bool // OAuthOnly, if set, makes this server a pure OAuth 2.0 + FAPI 2.0 diff --git a/server/par.go b/server/par.go index 922cccad..c345e4df 100644 --- a/server/par.go +++ b/server/par.go @@ -348,7 +348,8 @@ func (s *Server) validateAuthorizationRequest(ctx context.Context, params map[st // method in each request", so a request carrying both attestation // headers and a client_assertion/client_assertion_type is refused with // invalid_request rather than authenticated by whichever one is checked -// first. A peer certificate alone is not a second method: an mTLS +// first. Draft-12 §7.6's use of an attestation as an additional signal +// beside another method is not supported. A peer certificate alone is not a second method: an mTLS // connection presents one on every request, including a // private_key_jwt or attestation client's in an mTLS-sender-constrained // deployment, and it authenticates the client only when neither of the @@ -359,7 +360,7 @@ func (s *Server) authenticateClient(ctx context.Context, params map[string]strin switch { case carriesAttestation && carriesAssertion: return storage.RegisteredClient{}, clientassertion.VerifiedAssertion{}, - newError(ErrorInvalidRequest, 400, "the request uses more than one client authentication method", nil) + newError(ErrorInvalidRequest, 400, "a client attestation cannot be combined with another client authentication method", nil) case carriesAttestation: return s.authenticateClientViaAttestation(ctx, params["client_id"], attestation, attestationPoP) case params["client_assertion"] != "": From 0faec6ae9f0b0f9519053538b89c2ad93460a16e Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:05:12 +0800 Subject: [PATCH 4/8] fix(server): treat an empty resource as omitted and ignore resource when unsupported MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6749 §3.1 and §3.2: "Parameters sent without a value MUST be treated as if they were omitted from the request". An empty "resource" parameter at PAR or the token endpoint was refused with invalid_target; it is now dropped, alone or beside non-empty ones, as every other empty parameter already is. TokenEndpointRequest.Resources drops it too. With resource indicators disabled (neither Config.Resources nor a ResourcePolicy), one "resource" was ignored but a repeated one was refused with invalid_request as a duplicate parameter. RFC 8707 §2 lets a client repeat it, and an unrecognized parameter is ignored (RFC 6749 §3.1, §3.2), so it is now ignored however many times it appears. Co-Authored-By: Claude Opus 5.5 --- server/extension_grants.go | 3 +- server/resource_indicators.go | 21 +++++++---- server/resource_indicators_fuzz_test.go | 11 ++++-- server/resource_indicators_test.go | 50 +++++++++++++++++++------ 4 files changed, 60 insertions(+), 25 deletions(-) diff --git a/server/extension_grants.go b/server/extension_grants.go index 2c760c3c..27908738 100644 --- a/server/extension_grants.go +++ b/server/extension_grants.go @@ -28,7 +28,8 @@ func (t TokenEndpointRequest) Parameters() (map[string]string, error) { } // Resources returns the request's RFC 8707 "resource" parameters, in -// order: every one the client sent, which is what a check of the +// order: every one the client sent with a value (an empty one is +// omitted, RFC 6749 §3.2), which is what a check of the // resources an embedder's grant may be used for has to look at. Pass // them as IssueAccessTokenRequest.Resources and // IssueRefreshTokenRequest.Resources, which refuse any the client or diff --git a/server/resource_indicators.go b/server/resource_indicators.go index 421ed44d..c3e21e02 100644 --- a/server/resource_indicators.go +++ b/server/resource_indicators.go @@ -77,17 +77,22 @@ func (s *Server) resourceIndicatorsEnabled() bool { // takeResourceParameters splits a form request's "resource" parameters // (RFC 8707 §2: repeatable) from the rest, which then go through the // usual duplicate-refusing parsing. With resource indicators disabled -// it returns params unchanged, so "resource" stays an ordinary, -// ignored parameter. +// it drops them, however many there are: "resource" is then a +// parameter this server doesn't recognize, which RFC 6749 §3.1 and +// §3.2 have it ignore, and a client following RFC 8707 may repeat it. func (s *Server) takeResourceParameters(params []FormParameter) ([]FormParameter, []string) { + rest, resources := splitResourceParameters(params) if !s.resourceIndicatorsEnabled() { - return params, nil + return rest, nil } - return splitResourceParameters(params) + return rest, resources } // splitResourceParameters separates params' "resource" parameters from -// the rest. Too many parameters are returned unsplit, for +// the rest, dropping any without a value: RFC 6749 §3.1 and §3.2, +// "Parameters sent without a value MUST be treated as if they were +// omitted from the request", so "resource=" is no resource, even beside +// others. Too many parameters are returned unsplit, for // formParametersToMap to refuse, so the resources don't escape its // bound. func splitResourceParameters(params []FormParameter) ([]FormParameter, []string) { @@ -97,11 +102,11 @@ func splitResourceParameters(params []FormParameter) ([]FormParameter, []string) rest := make([]FormParameter, 0, len(params)) var resources []string for _, p := range params { - if p.Name == resourceParameter { + if p.Name != resourceParameter { + rest = append(rest, p) + } else if p.Value != "" { resources = append(resources, p.Value) - continue } - rest = append(rest, p) } return rest, resources } diff --git a/server/resource_indicators_fuzz_test.go b/server/resource_indicators_fuzz_test.go index d9b54bbf..cd5f6e39 100644 --- a/server/resource_indicators_fuzz_test.go +++ b/server/resource_indicators_fuzz_test.go @@ -85,8 +85,9 @@ func checkAcceptedResources(t *testing.T, raw []byte, accepted []string) { // FuzzSplitResourceParameters runs an arbitrary form body through // FormRequestFromHTTP and takeResourceParameters, with resource // indicators enabled, as every endpoint taking "resource" does. RFC 8707 -// lets "resource" repeat, and nothing else: every resource value must -// come out, in order and unchanged, while a repeat of any other +// lets "resource" repeat, and nothing else: every resource value but +// an empty one (RFC 6749 §3.2: omitted) must come out, in order and +// unchanged, while a repeat of any other // parameter is still refused by formParametersToMap, which also still // bounds the request. func FuzzSplitResourceParameters(f *testing.F) { @@ -115,8 +116,8 @@ func FuzzSplitResourceParameters(f *testing.F) { // checkResourceSplit fails t unless takeResourceParameters splits params // as RFC 8707 allows: over maxFormParameters, not at all (and -// formParametersToMap refuses them); otherwise every resource value out, -// in order, and the rest unchanged, with a repeat of any of those still +// formParametersToMap refuses them); otherwise every non-empty resource +// value out, in order, and the rest unchanged, with a repeat of any of those still // refused by formParametersToMap. func checkResourceSplit(t *testing.T, s *Server, params []FormParameter) { t.Helper() @@ -132,6 +133,8 @@ func checkResourceSplit(t *testing.T, s *Server, params []FormParameter) { } wantRest, wantResources := splitByName(params, resourceParameter) + // An empty value is no resource at all (RFC 6749 §3.1, §3.2). + wantResources = slices.DeleteFunc(wantResources, func(v string) bool { return v == "" }) if !slices.Equal(resources, wantResources) { t.Fatalf("resources = %q, want %q", resources, wantResources) } diff --git a/server/resource_indicators_test.go b/server/resource_indicators_test.go index 227116d4..f0cc992f 100644 --- a/server/resource_indicators_test.go +++ b/server/resource_indicators_test.go @@ -216,9 +216,34 @@ func TestResourceIndicatorsIgnoredWhenNothingConfigured(t *testing.T) { } wantAudience(t, result, testIssuer) - // "resource" stays an ordinary parameter: not repeatable. - _, err = pushWithResources(t, h, resourceA, resourceB) - wantErrorCode(t, err, server.ErrorInvalidRequest) + // Ignored however many times it's sent (RFC 6749 §3.1: an + // unrecognized parameter is ignored; RFC 8707 §2: it's repeatable). + code = grantForResources(t, h, resourceA, resourceB) + result, err = exchangeWithResources(t, h, code, resourceA, resourceB, "") + if err != nil { + t.Fatalf("ExchangeAuthorizationCode with repeated resources: %v", err) + } + wantAudience(t, result, testIssuer) +} + +// TestResourceIndicatorsEmptyTreatedAsOmitted covers RFC 6749 §3.1 and +// §3.2, "Parameters sent without a value MUST be treated as if they +// were omitted from the request": an empty "resource" at PAR or the +// token endpoint, alone or beside others, names no resource. +func TestResourceIndicatorsEmptyTreatedAsOmitted(t *testing.T) { + h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA, resourceB}, nil) + + result, err := exchangeWithResources(t, h, grantForResources(t, h, ""), "") + if err != nil { + t.Fatalf("empty resource at PAR and the token endpoint: %v", err) + } + wantAudience(t, result, testIssuer) + + result, err = exchangeWithResources(t, h, grantForResources(t, h, "", resourceA, ""), resourceA, "") + if err != nil { + t.Fatalf("empty resources beside one: %v", err) + } + wantAudience(t, result, resourceA) } func TestResourceIndicatorsPARValidation(t *testing.T) { @@ -226,15 +251,16 @@ func TestResourceIndicatorsPARValidation(t *testing.T) { resources []string want server.ErrorCode }{ - "one listed": {resources: []string{resourceA}}, - "two listed": {resources: []string{resourceA, resourceB}}, - "repeated": {resources: []string{resourceA, resourceA}}, - "unlisted": {resources: []string{resourceA, resourceC}, want: server.ErrorInvalidTarget}, - "relative": {resources: []string{"/api"}, want: server.ErrorInvalidTarget}, - "fragment": {resources: []string{resourceA + "#x"}, want: server.ErrorInvalidTarget}, - "empty": {resources: []string{""}, want: server.ErrorInvalidTarget}, - "too many": {resources: manyResources(17), want: server.ErrorInvalidTarget}, - "listed with prefix": {resources: []string{resourceA + "/"}, want: server.ErrorInvalidTarget}, + "one listed": {resources: []string{resourceA}}, + "two listed": {resources: []string{resourceA, resourceB}}, + "repeated": {resources: []string{resourceA, resourceA}}, + "unlisted": {resources: []string{resourceA, resourceC}, want: server.ErrorInvalidTarget}, + "relative": {resources: []string{"/api"}, want: server.ErrorInvalidTarget}, + "fragment": {resources: []string{resourceA + "#x"}, want: server.ErrorInvalidTarget}, + "empty": {resources: []string{""}}, + "empty beside listed": {resources: []string{"", resourceA, ""}}, + "too many": {resources: manyResources(17), want: server.ErrorInvalidTarget}, + "listed with prefix": {resources: []string{resourceA + "/"}, want: server.ErrorInvalidTarget}, } { t.Run(name, func(t *testing.T) { h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA, resourceB}, nil) From 6adb4ed0027fe515639864cf5c1255d1cca295b5 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:09:18 +0800 Subject: [PATCH 5/8] fix(server)!: refuse an edited TokenBinding thumbprint or attestation expiry IssueAccessToken documented that it accepts only a TokenBinding VerifyTokenRequestBinding returned, but checked only the client it was verified for: an embedder that changed the exported Thumbprint got an access token whose cnf named a key that never proved possession. In the same way, an AttestedClient whose exported AttestationExpiresAt was moved on passed the check that refuses a Client kept from an earlier request. TokenBinding now keeps an unexported copy of the thumbprint it verified, and AttestedClient of the expiry it verified. IssueAccessToken refuses a binding whose Thumbprint differs, and IssueAccessToken and IssueRefreshToken refuse a Client whose AttestationExpiresAt differs and judge the attestation's expiry by the verified copy. The exported fields are unchanged. BREAKING CHANGE: IssueAccessToken refuses with server_error a TokenBinding whose Thumbprint was changed after VerifyTokenRequestBinding returned it, and IssueAccessToken and IssueRefreshToken refuse an AttestedClient whose AttestationExpiresAt was changed after AuthenticateAttestedClient returned it. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 38 ++++++++++++++++++++++++++++ server/attested_client.go | 13 +++++++--- server/embedder_access_token.go | 5 ++-- server/embedder_access_token_test.go | 12 +++++++++ server/embedder_refresh.go | 4 +-- server/extension_grants.go | 13 ++++++---- 6 files changed, 72 insertions(+), 13 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 0e961345..dd346275 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -125,6 +125,44 @@ exponent 65537, or for EC keys: openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:4096 -out attester-ca.pem ``` +### `IssueAccessToken` and `IssueRefreshToken` refuse an edited binding or attestation expiry (server) + +**Affects:** an embedder serving its own grant +(`Config.AdditionalGrantTypes`) that changes, after the server returned +them, the `Thumbprint` of a `server.TokenBinding` from +`VerifyTokenRequestBinding` before passing it to `IssueAccessToken`, or +the `AttestationExpiresAt` of a `server.AttestedClient` from +`AuthenticateAttestedClient` before passing it to `IssueAccessToken` or +`IssueRefreshToken`. + +**Why:** both documented that only what the server verified is +accepted, but the check covered only the client the value was verified +for. An edited `Thumbprint` bound the access token to a key that never +proved possession, and an extended `AttestationExpiresAt` let a +`Client` kept from an earlier request pass the attestation expiry +check. Each now keeps its own copy of what was verified. + +**What fails:** nothing at startup. Such a call is refused with a +`server_error` `*server.Error`. + +**What to change:** pass both exactly as the server returned them, for +the current request: + +```go +attested, err := srv.AuthenticateAttestedClient(ctx, req.AttestedClientAuthentication()) +if err != nil { + return err +} +binding, err := srv.VerifyTokenRequestBinding(ctx, attested.Client, req) +if err != nil { + return err +} +result, err := srv.IssueAccessToken(ctx, server.IssueAccessTokenRequest{ + GrantType: grantType, Client: attested, Binding: binding, + Subject: subject, Scope: scope, +}) +``` + ## v0.68.0 ### `DefaultAttesterLeafPolicy` refuses more signing roles (server) diff --git a/server/attested_client.go b/server/attested_client.go index 11b4e030..9c63a4aa 100644 --- a/server/attested_client.go +++ b/server/attested_client.go @@ -48,7 +48,9 @@ type AttestedClient struct { Client storage.RegisteredClient // AttestationExpiresAt is when the verified Client Attestation - // expires. + // expires. IssueRefreshToken and IssueAccessToken refuse a Client + // whose AttestationExpiresAt was changed since: they judge the + // attestation's expiry by their own copy of it. AttestationExpiresAt time.Time // instanceKey is the RFC 7638 thumbprint of the Client Instance Key @@ -57,8 +59,11 @@ type AttestedClient struct { // this server authenticated carries one. authenticatedID is the // client it was verified for: IssueRefreshToken refuses a Client // changed since, so the key can't be attached to another client. - instanceKey string - authenticatedID fapi.ClientID + // verifiedExpiresAt is AttestationExpiresAt as verified, which they + // compare it with and judge expiry by. + instanceKey string + authenticatedID fapi.ClientID + verifiedExpiresAt time.Time } // AuthenticateAttestedClient authenticates a client with a Client @@ -122,5 +127,5 @@ func (s *Server) AuthenticateAttestedClient(ctx context.Context, req AttestedCli return AttestedClient{}, err } s.audit(ctx, AuditEventAuthenticateAttestedClient, client.ID(), AuditOutcomeSuccess, "") - return AttestedClient{Client: client, AttestationExpiresAt: verified.ExpiresAt, instanceKey: verified.InstanceKey, authenticatedID: client.ID()}, nil + return AttestedClient{Client: client, AttestationExpiresAt: verified.ExpiresAt, instanceKey: verified.InstanceKey, authenticatedID: client.ID(), verifiedExpiresAt: verified.ExpiresAt}, nil } diff --git a/server/embedder_access_token.go b/server/embedder_access_token.go index ef9b626b..0a564b78 100644 --- a/server/embedder_access_token.go +++ b/server/embedder_access_token.go @@ -27,7 +27,8 @@ type IssueAccessTokenRequest struct { // Binding is the token request's sender-constraining credential, // exactly as VerifyTokenRequestBinding returned it for Client: the // access token is bound to it (its "cnf" claim), so one built by - // hand, or verified for another client, is refused. + // hand, verified for another client, or whose Thumbprint was + // changed since, is refused. Binding TokenBinding // Subject is the access token's sub. @@ -82,7 +83,7 @@ type IssueAccessTokenRequest struct { func (s *Server) IssueAccessToken(ctx context.Context, req IssueAccessTokenRequest) (TokenResult, error) { client := req.Client.Client grant, err := s.embedderGrant(ctx, embedderGrantRequest(req)) - if err == nil && req.Binding.verifiedFor != client.ID() { + if err == nil && (req.Binding.verifiedFor != client.ID() || req.Binding.Thumbprint != req.Binding.verifiedThumbprint) { err = newError(ErrorServerError, 500, "Binding must be as VerifyTokenRequestBinding returned it for Client", nil) } if err != nil { diff --git a/server/embedder_access_token_test.go b/server/embedder_access_token_test.go index f2527873..76da164d 100644 --- a/server/embedder_access_token_test.go +++ b/server/embedder_access_token_test.go @@ -294,6 +294,12 @@ func TestIssueAccessTokenRefusals(t *testing.T) { r.Binding = server.TokenBinding{Thumbprint: r.Binding.Thumbprint, SenderConstrain: r.Binding.SenderConstrain} }, server.ErrorServerError}, "binding verified for another client": {func(r *server.IssueAccessTokenRequest) { r.Binding = otherClientBinding }, server.ErrorServerError}, + "binding thumbprint edited": {func(r *server.IssueAccessTokenRequest) { + r.Binding.Thumbprint = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" // a key nobody proved + }, server.ErrorServerError}, + "attestation expiry edited": {func(r *server.IssueAccessTokenRequest) { + r.Client.AttestationExpiresAt = r.Client.AttestationExpiresAt.Add(time.Hour) + }, server.ErrorServerError}, "grant type not configured": {func(r *server.IssueAccessTokenRequest) { r.GrantType = "authorization_code" }, server.ErrorServerError}, "client not authenticated": {func(r *server.IssueAccessTokenRequest) { r.Client = server.AttestedClient{Client: r.Client.Client} }, server.ErrorServerError}, "client swapped after authentication": {func(r *server.IssueAccessTokenRequest) { r.Client.Client = otherClient }, server.ErrorServerError}, @@ -397,6 +403,12 @@ func TestEmbedderGrantRefusesExpiredAttestation(t *testing.T) { if _, err := past.IssueRefreshToken(context.Background(), server.IssueRefreshTokenRequest(req)); serverErrorCode(t, err) != server.ErrorServerError { t.Errorf("IssueRefreshToken past the attestation's exp = %v, want server_error", err) } + // Moving AttestationExpiresAt on doesn't make a kept Client current. + extended := req + extended.Client.AttestationExpiresAt = extended.Client.AttestationExpiresAt.Add(time.Hour) + if _, err := past.IssueRefreshToken(context.Background(), server.IssueRefreshTokenRequest(extended)); serverErrorCode(t, err) != server.ErrorServerError { + t.Errorf("IssueRefreshToken with AttestationExpiresAt edited = %v, want server_error", err) + } } // TestEmbedderGrantRefusesRevokedGrantID: a GrantID already revoked diff --git a/server/embedder_refresh.go b/server/embedder_refresh.go index 07a26036..8807bd54 100644 --- a/server/embedder_refresh.go +++ b/server/embedder_refresh.go @@ -128,9 +128,9 @@ func (s *Server) embedderGrant(ctx context.Context, req embedderGrantRequest) (g switch { case !slices.Contains(s.cfg.AdditionalGrantTypes, req.GrantType): return grantRecord{}, newError(ErrorServerError, 500, "GrantType is not one of Config.AdditionalGrantTypes", nil) - case req.Client.instanceKey == "" || req.Client.authenticatedID != client.ID(): + case req.Client.instanceKey == "" || req.Client.authenticatedID != client.ID() || !req.Client.AttestationExpiresAt.Equal(req.Client.verifiedExpiresAt): return grantRecord{}, newError(ErrorServerError, 500, "Client must be as AuthenticateAttestedClient returned it", nil) - case s.deps.Clock.Now().After(req.Client.AttestationExpiresAt.Add(s.cfg.Limits.MaxClockSkew)): + case s.deps.Clock.Now().After(req.Client.verifiedExpiresAt.Add(s.cfg.Limits.MaxClockSkew)): // AuthenticateAttestedClient accepted the attestation up to // MaxClockSkew past its exp; past that, Client was kept from an // earlier request rather than authenticated for this one. diff --git a/server/extension_grants.go b/server/extension_grants.go index 27908738..5e21c2a1 100644 --- a/server/extension_grants.go +++ b/server/extension_grants.go @@ -76,7 +76,8 @@ func (t TokenEndpointRequest) AttestedClientAuthentication() AttestedClientAuthe // returned, for the same client: the access token is bound to its // Thumbprint, so a hand-built one, or one verified for another client, // would bind it to a key that never proved possession at the token -// endpoint. IssueRefreshToken records Thumbprint for reference only, so +// endpoint. It keeps its own copy of the thumbprint it verified, and +// refuses a binding whose Thumbprint was changed since. IssueRefreshToken records Thumbprint for reference only, so // it accepts any binding of the client's registered kind. type TokenBinding struct { // Thumbprint is the credential's thumbprint, as @@ -95,9 +96,11 @@ type TokenBinding struct { NextDPoPNonce string // verifiedFor is the client VerifyTokenRequestBinding verified the - // credential for. Unexported, so only a binding it returned carries - // one: see IssueAccessToken. - verifiedFor fapi.ClientID + // credential for, and verifiedThumbprint the Thumbprint it verified. + // Unexported, so only a binding it returned carries them, and an + // edit to Thumbprint shows: see IssueAccessToken. + verifiedFor fapi.ClientID + verifiedThumbprint string } // VerifyTokenRequestBinding verifies req's sender-constraining @@ -145,7 +148,7 @@ func (s *Server) verifyExtensionGrantBinding(ctx context.Context, client storage if err != nil { return TokenBinding{}, err } - return TokenBinding{Thumbprint: thumbprint, SenderConstrain: client.SenderConstrain(), NextDPoPNonce: nextNonce, verifiedFor: client.ID()}, nil + return TokenBinding{Thumbprint: thumbprint, SenderConstrain: client.SenderConstrain(), NextDPoPNonce: nextNonce, verifiedFor: client.ID(), verifiedThumbprint: thumbprint}, nil } // DPoPProofBinding is what VerifyTokenRequestDPoP verified: the DPoP key From 18131e7439afb6350ead681bf46deba34c9907ff Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:09:49 +0800 Subject: [PATCH 6/8] docs(server): say unregistered parameters are ignored, not rejected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Config.Extensions said a parameter without a registered Definition is rejected, and Config.RAR described the same default-reject stance. The server ignores and strips such a parameter, as RFC 6749 §3.1 requires and extension.Registry.Parse documents. Only "authorization_details" is refused without Config.RAR, with invalid_authorization_details. Co-Authored-By: Claude Opus 5.5 --- server/config.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/server/config.go b/server/config.go index b6824a40..dfd56cc5 100644 --- a/server/config.go +++ b/server/config.go @@ -452,11 +452,13 @@ type Config struct { // server accepts, beyond the standard OAuth/OIDC/PKCE parameters it // already understands (response_type, client_id, redirect_uri, // scope, state, nonce, code_challenge, code_challenge_method). A - // parameter with no registered Definition is rejected — there is no - // permissive fallback — so nil is equivalent to an empty Registry - // (no custom parameters accepted at all), not to "extensions - // disabled, accept anything." See ARCHITECTURE.md design rules - // 10-11 and extension.Registry.Parse. + // parameter with no registered Definition is ignored and stripped + // from the request (RFC 6749 §3.1: an authorization server "MUST + // ignore unrecognized request parameters"), so it never reaches + // storage, an InteractionRequest or a token; only a registered one + // is parsed, validated and kept. nil is equivalent to an empty + // Registry: no custom parameter is kept. See ARCHITECTURE.md design + // rules 10-11 and extension.Registry.Parse. // // A Definition whose AllowedSources permits SourcePlainParameter // must use a string-shaped T: this server's plain-parameter path @@ -471,10 +473,10 @@ type Config struct { // on PAR, CIBA backchannel authentication requests, and // client_credentials token requests (RFC 9396 §6). Unlike // Extensions, nil is not equivalent to an empty registry accepting - // nothing extra — it means "authorization_details" itself is - // rejected outright as an unregistered parameter, the same - // default-reject stance Extensions takes for any parameter without a - // matching Definition. A registered RARRegistry's own bounds (total + // nothing extra: it means a request carrying "authorization_details" + // at all is refused with invalid_authorization_details (RFC 9396 + // §5), rather than the parameter being ignored as an unregistered + // Extensions parameter is. A registered RARRegistry's own bounds (total // size, nesting depth, per-type object count and size) apply // identically to both flows. Each client requests only the types its // registration lists (storage.RegisteredClientConfig. From f97904718acf88a866921ae1ae1b2a24f8ddbadc Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:11:57 +0800 Subject: [PATCH 7/8] fix(server): compare requested claim values' numbers exactly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OIDC Core §5.5.1 releases a claim requested with "value" or "values" only when it equals one of them. internal/claimvalues decoded both sides into float64, so integers above 2^53 that differ compared equal, and a claim was released for a value the client didn't request. Numbers are now compared by the value their text denotes, reduced to sign, significant digits and a big.Int decimal exponent, so "100", "1e2" and "100.0" stay equal while 9007199254740993 and 9007199254740992 differ, without expanding a large exponent. Co-Authored-By: Claude Opus 5.5 --- internal/claimvalues/claimvalues.go | 95 ++++++++++++++++++++++-- internal/claimvalues/claimvalues_test.go | 46 ++++++++++++ 2 files changed, 134 insertions(+), 7 deletions(-) diff --git a/internal/claimvalues/claimvalues.go b/internal/claimvalues/claimvalues.go index f0a5f319..cac3adfd 100644 --- a/internal/claimvalues/claimvalues.go +++ b/internal/claimvalues/claimvalues.go @@ -12,7 +12,11 @@ package claimvalues import ( "bytes" "encoding/json" - "reflect" + "io" + "maps" + "math/big" + "slices" + "strings" ) // Acceptable is, for a claim requested with "value", "values" or both, @@ -53,14 +57,91 @@ func FromRequest(request json.RawMessage) ([]json.RawMessage, bool) { } // Equal reports whether a and b are the same JSON value: equal once -// decoded, so "1" equals "1.0" and member order doesn't matter. Text -// that doesn't decode equals nothing. +// decoded, so "1" equals "1.0" and member order doesn't matter. Numbers +// compare exactly, by the value their text denotes, not as float64, +// which would make integers above 2^53 that differ equal. Text that +// doesn't decode equals nothing. func Equal(a, b json.RawMessage) bool { - var av, bv any - if json.Unmarshal(a, &av) != nil || json.Unmarshal(b, &bv) != nil { - return false + av, aok := decode(a) + bv, bok := decode(b) + return aok && bok && equalValues(av, bv) +} + +// decode decodes raw as one JSON value, keeping numbers as their text. +func decode(raw json.RawMessage) (any, bool) { + d := json.NewDecoder(bytes.NewReader(raw)) + d.UseNumber() + var v any + if d.Decode(&v) != nil { + return nil, false + } + // Like json.Unmarshal, refuse anything after the value. + if _, err := d.Token(); err != io.EOF { + return nil, false + } + return v, true +} + +// equalValues compares two values decode returned. +func equalValues(a, b any) bool { + switch a := a.(type) { + case json.Number: + b, ok := b.(json.Number) + return ok && equalNumbers(a, b) + case []any: + b, ok := b.([]any) + return ok && slices.EqualFunc(a, b, equalValues) + case map[string]any: + b, ok := b.(map[string]any) + return ok && maps.EqualFunc(a, b, equalValues) + default: + // string, bool or nil. + return a == b + } +} + +// equalNumbers reports whether two JSON number texts denote the same +// value, exactly: each is reduced to a sign, its significant digits and +// a decimal exponent, so "100", "1e2" and "100.0" are equal. The +// exponent is a big.Int, so neither overflow nor a huge exponent's +// expansion (as big.Rat or big.Float would compute) is possible. +func equalNumbers(a, b json.Number) bool { + an, aok := canonicalNumber(string(a)) + bn, bok := canonicalNumber(string(b)) + return aok && bok && an.negative == bn.negative && an.digits == bn.digits && an.exponent.Cmp(bn.exponent) == 0 +} + +// number is a JSON number as ±digits × 10^exponent, with digits +// free of leading and trailing zeros; zero has no digits, no sign and +// exponent 0. +type number struct { + negative bool + digits string + exponent *big.Int +} + +// canonicalNumber reduces text, a JSON number (RFC 8259 §6) as +// encoding/json validated it, to a number. +func canonicalNumber(text string) (number, bool) { + negative := strings.HasPrefix(text, "-") + text = strings.TrimPrefix(text, "-") + mantissa, exp, hasExp := strings.Cut(strings.ToLower(text), "e") + exponent := new(big.Int) + if hasExp { + if _, ok := exponent.SetString(strings.TrimPrefix(exp, "+"), 10); !ok { + return number{}, false + } + } + integer, fraction, _ := strings.Cut(mantissa, ".") + digits := integer + fraction + exponent.Sub(exponent, big.NewInt(int64(len(fraction)))) + trimmed := strings.TrimRight(digits, "0") + exponent.Add(exponent, big.NewInt(int64(len(digits)-len(trimmed)))) + digits = strings.TrimLeft(trimmed, "0") + if digits == "" { + return number{exponent: new(big.Int)}, true } - return reflect.DeepEqual(av, bv) + return number{negative: negative, digits: digits, exponent: exponent}, true } // Allows reports whether name may be released with value under a. diff --git a/internal/claimvalues/claimvalues_test.go b/internal/claimvalues/claimvalues_test.go index b60ca8f3..f2660265 100644 --- a/internal/claimvalues/claimvalues_test.go +++ b/internal/claimvalues/claimvalues_test.go @@ -38,3 +38,49 @@ func TestFromRequest(t *testing.T) { } } } + +// TestEqualNumbersExactly covers OIDC Core §5.5.1's equality comparison +// for numbers: by the value the text denotes, exactly, so integers +// above 2^53 that float64 would round together stay distinct, while +// different spellings of one value are equal. +func TestEqualNumbersExactly(t *testing.T) { + for _, tc := range []struct { + a, b string + want bool + }{ + {`9007199254740993`, `9007199254740992`, false}, + {`9007199254740993`, `9007199254740993`, true}, + {`[9007199254740993]`, `[9007199254740992]`, false}, + {`{"n":9007199254740993}`, `{"n":9007199254740992}`, false}, + {`0.1000000000000000000001`, `0.1`, false}, + {`1`, `1.0`, true}, + {`100`, `1e2`, true}, + {`100`, `1E+2`, true}, + {`0.01`, `1e-2`, true}, + {`1.50`, `15e-1`, true}, + {`-1`, `1`, false}, + {`0`, `-0.0`, true}, + {`0`, `0e5`, true}, + {`1e400`, `1e400`, true}, + {`1e400`, `1e401`, false}, + {`1e99999999999999999999`, `10e99999999999999999998`, true}, + {`1`, `"1"`, false}, + {`"A"`, `"A"`, true}, + {`{"x":[1,{"y":2}]}`, `{"x":[1.0,{"y":2e0}]}`, true}, + {`[1,2]`, `[1,2,3]`, false}, + {`null`, `null`, true}, + {`1 2`, `1`, false}, + {`1x`, `1`, false}, + } { + if got := Equal(json.RawMessage(tc.a), json.RawMessage(tc.b)); got != tc.want { + t.Errorf("Equal(%s, %s) = %v, want %v", tc.a, tc.b, got, tc.want) + } + if got := Equal(json.RawMessage(tc.b), json.RawMessage(tc.a)); got != tc.want { + t.Errorf("Equal(%s, %s) = %v, want %v", tc.b, tc.a, got, tc.want) + } + } + a := Acceptable{"account_no": {json.RawMessage(`9007199254740993`)}} + if a.Allows("account_no", json.RawMessage(`9007199254740992`)) { + t.Error("Allows released 9007199254740992 for a request of value 9007199254740993") + } +} From a1248f3bc4862e40d26986c9c756b996805ad5dc Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:49:41 +0800 Subject: [PATCH 8/8] docs(upgrading): close the binding_message snippet in v0.69.0 The binding_message section's code block shared its closing fence with the next section's when both were added under v0.69.0, and lost it. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/UPGRADING.md b/UPGRADING.md index dd346275..ebabbccb 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -95,6 +95,7 @@ example letters and digits: ```go req.BindingMessage = "W4SCT" +``` ### `X5CAttesterChain` limits the RSA keys of `x5c` certificates (server)