From 6b5a36c682192497e8c5fd4f9ab79ab98b336827 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Tue, 20 Oct 2026 08:04:43 +0800 Subject: [PATCH 1/2] docs: say when to begin an authorization, what NoRevocation costs, and where nonces and trust mark status belong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - client.BeginAuthorization: call it only from a request the user made on purpose and that's protected against CSRF, not a plain GET any page can trigger. - resource.NoRevocation: a token the authorization server revoked stays usable until it expires, which falls short of what a FAPI deployment expects of a resource server. - resource.Dependencies.Nonces: never the authorization server's own nonce store (RFC 9449 §8 and §9 keep them apart); serverresource.NewVerifier refuses that, a Verifier built directly can't tell. - federation.CheckTrustMarkStatus: call it after VerifyTrustMark, never instead of it. - README: Message Signing covers JAR and JARM; signed introspection responses aren't implemented. - A test comment still cited draft-07 of attestation-based client authentication; it's draft-12 §10.3. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- client/begin_authorization.go | 8 ++++++++ federation/trustmark.go | 4 +++- resource/dependencies.go | 7 +++++++ resource/revocation.go | 7 +++++++ server/refresh_instance_binding_test.go | 2 +- 6 files changed, 27 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 367a4ae8..f5fbf39d 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ implemented. `server` only ever accepts `response_type=code`, and `ClientAuthMethod` is a closed enum of `private_key_jwt`, the mTLS variants and OAuth 2.0 attestation-based client authentication. -- FAPI 2.0 Security Profile Final + Message Signing Final +- FAPI 2.0 Security Profile Final + Message Signing Final (signed requests with JAR, signed responses with JARM; signed introspection responses aren't implemented) - PAR (RFC 9126) · DPoP (RFC 9449) · mTLS client auth & cert-bound tokens (RFC 8705), with CRL revocation checking - private_key_jwt client authentication - OAuth 2.0 Attestation-Based Client Authentication, including HAIP 1.0 x5c attester certificate chains, optionally binding each trust anchor to the attesters it may vouch for diff --git a/client/begin_authorization.go b/client/begin_authorization.go index d4bd5c1a..60a114b9 100644 --- a/client/begin_authorization.go +++ b/client/begin_authorization.go @@ -223,6 +223,14 @@ const errPushedAuthorizationRequestFailed = "pushed authorization request failed // authenticates to and calls the // pushed-authorization-request endpoint (RFC 9126), and persists // correlation state for the eventual callback. +// +// Call it only from a request the user made on purpose and that is +// protected against cross-site request forgery: a POST guarded by a +// SameSite cookie or a CSRF token, not a plain GET any page can trigger. +// An endpoint that starts an authorization on any cross-site request +// lets another site begin flows in the user's browser, and a user +// already signed in at the authorization server (who may have consented +// before) can then end up granting access without meaning to. func (c *Client) BeginAuthorization(ctx context.Context, req BeginAuthorizationRequest) (AuthorizationSession, error) { redirectURI, reqErr := c.checkBeginRequest(req) if reqErr != nil { diff --git a/federation/trustmark.go b/federation/trustmark.go index 6d7d94d4..584e1a9d 100644 --- a/federation/trustmark.go +++ b/federation/trustmark.go @@ -235,7 +235,9 @@ func (r *Resolver) verifyTrustMarkDelegationAgainstJWKS(d intfed.TrustMarkDelega // CheckTrustMarkStatus queries trustMarkToken's own issuer for its // current status (OpenID Federation 1.0 §8.4) and returns the verified -// response claims. §8.4: "The query MUST be sent to the Trust Mark +// response claims. It says only whether the issuer still considers the +// mark active, not that the mark is valid: call it after VerifyTrustMark +// has accepted the mark, never instead of it. §8.4: "The query MUST be sent to the Trust Mark // Issuer" — trustMarkToken's own unverified "iss" claim names it, and // that issuer is resolved as a fresh Trust Chain (the same way // VerifyTrustMark already establishes trust in a Trust Mark's issuer) diff --git a/resource/dependencies.go b/resource/dependencies.go index 38dba04e..db5517c6 100644 --- a/resource/dependencies.go +++ b/resource/dependencies.go @@ -42,6 +42,13 @@ type Dependencies struct { // Random, and Config.Limits.DPoPNonceLifetime) to have Verify // challenge a request whose proof carries no valid nonce, and // proactively reissue a fresh one on every successful call. + // + // Never pass an authorization server's own Dependencies.Nonces: a + // nonce store is keyed by the nonce alone, so a nonce one role issued + // could be consumed by the other, while RFC 9449 keeps the + // authorization server's nonces (§8) apart from a resource server's + // (§9). serverresource.NewVerifier refuses that for its + // Options.Nonces; a Verifier built here can't tell. Nonces storage.NonceStore // Random is the source of randomness for DPoP nonce generation. diff --git a/resource/revocation.go b/resource/revocation.go index 97c506e4..8e4b7064 100644 --- a/resource/revocation.go +++ b/resource/revocation.go @@ -31,6 +31,13 @@ type RevocationChecker interface { // that has decided not to check access-token revocation — see // server.NoRevocation's doc comment for the same "declining must be // visible, not silently defaulted" rationale. +// +// With it, a token the authorization server revoked (a reused +// authorization code, server.Server.RevokeGrant, a revoked refresh +// token's grant) stays usable here until it expires. That falls short of +// what a FAPI deployment expects of a resource server, so use it only +// where access tokens live too briefly for that to matter, or where +// revocation is checked some other way. type NoRevocation struct{} // IsRevoked implements RevocationChecker by never reporting a token as diff --git a/server/refresh_instance_binding_test.go b/server/refresh_instance_binding_test.go index 84a00193..6342ccbc 100644 --- a/server/refresh_instance_binding_test.go +++ b/server/refresh_instance_binding_test.go @@ -125,7 +125,7 @@ func (i *attestedInstance) refresh(refreshToken string) (server.TokenResult, err } // TestRefreshTokenBoundToClientInstanceKey covers -// draft-ietf-oauth-attestation-based-client-auth-07 §10.3: a refresh +// draft-ietf-oauth-attestation-based-client-auth-12 §10.3: a refresh // token issued to a client authenticated by Client Attestation is // redeemed only with an attestation for the same Client Instance Key. // Every installation of a wallet app shares the client_id and a valid From 71105e49a8103119af27642f3d316479f93f9096 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Tue, 20 Oct 2026 08:18:01 +0800 Subject: [PATCH 2/2] docs: correct spec citations the 2026-10-18 review found wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each of these was checked against the spec text: - RFC 9101 has no §5.3; the request object's client_id rule is §6.3. - request_uri at PAR is refused by RFC 9126 §2.1, and inside a request object by RFC 9101 §4, not RFC 9101 §5. - invalid_request_uri is defined in RFC 9101 §7, not RFC 9126 §2.3. - Resource-server DPoP nonces are RFC 9449 §9, not §8. - "At most one DPoP header" is RFC 9449 §4.3 check 1, not §7.1; the quoted "MUST NOT send this header field multiple times" isn't in the RFC. - at_hash: OIDC Core §3.1.3.8 lets a code-flow client validate it as §3.2.2.9 describes; it isn't REQUIRED for the code flow (§3.1.3.6). - azp: the current OIDC Core §3.1.3.7 has it at steps 4-5, not 9-10. - CIBA: one-time auth_req_id redemption is §10.1.1, poll mode §10.1, not §10.3 (Push Callback). - A256CBC-HS512 is RFC 7518 §5.2.5, and ECDH-ES+A256KW §4.6, not §5.2.3 and §4.7. - OpenID Federation: the Trust Mark kid rule is §7, the delegation validation steps §7.2.2 (the trust_mark_type match is step 8), and the delegation JWT's own rules §7.2.1. - RFC 8705 chain validation and revocation are §2.1, not §2.1.1; RFC 7592's registration access token is defined in §1.2; the "MUST NOT reply ... for a different user" rule is OIDC Core §3.1.2.2. - RFC 9449 §10.1 doesn't recommend the PAR DPoP proof over dpop_jkt; it says the proof "provides a stronger binding". Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 4 ++-- UPGRADING.md | 6 +++--- client/begin_authorization.go | 4 ++-- client/config.go | 15 ++++++++------- docs/guides/mtls.md | 2 +- encryption_algorithm.go | 2 +- federation/trustmark_issue.go | 2 +- internal/dpop/header.go | 4 ++-- internal/federation/errors.go | 2 +- internal/federation/trustmark.go | 18 +++++++++--------- internal/jwe/doc.go | 2 +- internal/jwe/keywrap.go | 2 +- internal/requestobject/create.go | 2 +- internal/requestobject/errors.go | 4 +++- internal/token/errors.go | 10 ++++++---- internal/token/idtoken.go | 9 +++++---- internal/token/validate.go | 30 ++++++++++++++++-------------- resource/dpop_nonce_header.go | 2 +- resource/errors.go | 4 ++-- resource/verify.go | 2 +- server/dpop_headers.go | 2 +- server/errors.go | 2 +- server/par.go | 8 ++++---- server/token.go | 4 ++-- storage/backchannel.go | 4 ++-- storage/client_repository.go | 4 ++-- 26 files changed, 79 insertions(+), 71 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 7a2b9986..49de5d1d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -368,7 +368,7 @@ The same reasoning applies to the DPoP header specifically: request carried, not a single already-resolved string — a caller passes `http.Header.Values("DPoP")` directly, never `Header.Get`, which silently returns only the first of several values instead of letting -the request be rejected. RFC 9449 §7.1 requires a request carry at +the request be rejected. RFC 9449 §4.3 (check 1) requires a request carry at most one DPoP header; `internal/dpop.ResolveHeaderValues` (used by both `server` and `resource`) enforces that itself, rejecting more than one value regardless of whether any individual value would otherwise have @@ -536,7 +536,7 @@ specific DCR plan) has no DCR module at all. If it is added: - `RegisterClient`/`ReadClient`/`UpdateClient`/`DeleteClient` follow the same typed-request → typed-result/typed-error shape every other method here does — no generic `HandleRequest` for this either. -- Registration access tokens (RFC 7592 §1.1) are a new storage +- Registration access tokens (RFC 7592 §1.2) are a new storage primitive, not a repurposed `GrantStore` refresh token: they authenticate the management endpoint itself, not a resource, and their rotate-on-read/update semantics (RFC 7592 §5, a MAY, never a diff --git a/UPGRADING.md b/UPGRADING.md index 48f31430..acbbcb68 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -707,9 +707,9 @@ parameter requesting a specific `sub` value, such as `{"id_token":{"sub":{"value":"248289761001"}}}`, to the pushed authorization request or the CIBA backchannel authentication endpoint. -**Why:** OIDC Core §5.5.1 says that when a client requests a specific -`sub`, the server MUST NOT reply with an ID token or access token for a -different user, and `id_token_hint` (§3.1.2.1, CIBA Core §7.1) names +**Why:** OIDC Core §3.1.2.2 says the server MUST NOT reply with an ID +token or access token for a different user than a requested `sub` +names (§5.5.1), and `id_token_hint` (§3.1.2.1, CIBA Core §7.1) names the user the same way. The pushed authorization request dropped `id_token_hint` as an unregistered parameter, CIBA passed it to the application unverified, and a `sub` value was never read, so a request diff --git a/client/begin_authorization.go b/client/begin_authorization.go index 60a114b9..c63e8956 100644 --- a/client/begin_authorization.go +++ b/client/begin_authorization.go @@ -411,8 +411,8 @@ func (c *Client) pushAuthorizationRequestWithJKT(ctx context.Context, params map // pushAuthorizationRequestWithDPoPProof implements PARDPoPBindingProof // (the default): binds the authorization code to this client's DPoP key -// by presenting an actual proof at PAR — RFC 9449 §10.1's recommended -// mechanism — instead of the plain dpop_jkt parameter, retrying once on +// by presenting an actual proof at PAR — the stronger binding of RFC +// 9449 §10.1's two mechanisms — instead of the plain dpop_jkt parameter, retrying once on // a use_dpop_nonce or attestation challenge (see challengeRetry). This mirrors sendTokenRequest's identical // mechanic for the token endpoint: an authorization server that // nonce-challenges DPoP proofs it verifies can now challenge this one diff --git a/client/config.go b/client/config.go index 5782b320..f875f6f3 100644 --- a/client/config.go +++ b/client/config.go @@ -33,10 +33,10 @@ const ( // both PAR and DPoP must accept either). Unlike Profile/Algorithms/ // Limits, whose zero value is deliberately invalid because there's no // universally-preferable choice, this type's zero value is a real, -// meaningful default: RFC 9449 §10.1 itself recommends -// PARDPoPBindingProof over PARDPoPBindingJKT, so a Config that never -// mentions this field gets the recommended behavior for free rather -// than a validation error. +// meaningful default: of the two mechanisms RFC 9449 §10.1 has a server +// support, the DPoP header "provides a stronger binding", so a Config +// that never mentions this field gets PARDPoPBindingProof rather than a +// validation error. type PARDPoPBinding uint8 const ( @@ -44,7 +44,8 @@ const ( // proof — not just its key's thumbprint — as this pushed // authorization request's own "DPoP" header, binding the eventual // authorization code to whichever key demonstrated possession here. - // RFC 9449 §10.1 recommends this over PARDPoPBindingJKT: it reuses + // RFC 9449 §10.1 notes it "provides a stronger binding" than + // PARDPoPBindingJKT's dpop_jkt, and it reuses // the same proof-building this client already does at the token and // resource endpoints (no separate thumbprint computation or // parameter), and unlike a bare dpop_jkt claim, it's actual proof of @@ -457,8 +458,8 @@ type Config struct { // PARDPoPBinding selects how BeginAuthorization commits this // client's DPoP key at PAR time — see PARDPoPBinding's own doc - // comment. Defaults to PARDPoPBindingProof, RFC 9449 §10.1's own - // recommended mechanism. Only meaningful when SenderConstrain is + // comment. Defaults to PARDPoPBindingProof, the stronger binding of + // RFC 9449 §10.1's two. Only meaningful when SenderConstrain is // SenderConstrainDPoP; ignored entirely under SenderConstrainMTLS, // which has no PAR-time pre-commitment concept of its own (RFC 8705 // binding is derived purely from whichever certificate authenticates diff --git a/docs/guides/mtls.md b/docs/guides/mtls.md index c322e9ed..57105256 100644 --- a/docs/guides/mtls.md +++ b/docs/guides/mtls.md @@ -57,7 +57,7 @@ srv, err := server.New(server.Config{ // Issuer, Endpoints, Profile, Algorithms, Limits, Subjects ... MTLSEndpoints: server.MTLSEndpoints{Token: mtlsTokenURL}, }, server.Dependencies{ - // RFC 8705 §2.1.1: verify the chain yourself, and check revocation. + // RFC 8705 §2.1: verify the chain yourself, and check revocation. ClientCertificateTrust: server.TrustedClientCAs{ Roots: rootCAs, Intermediates: issuingCAs, // revocation-checked, unlike Roots diff --git a/encryption_algorithm.go b/encryption_algorithm.go index 0d1794cc..fa847e65 100644 --- a/encryption_algorithm.go +++ b/encryption_algorithm.go @@ -84,7 +84,7 @@ const ( // the CBC-HMAC family requires. A256GCM - // A256CBCHS512 is AES_256_CBC_HMAC_SHA_512 (RFC 7518 §5.2.3): a + // A256CBCHS512 is AES_256_CBC_HMAC_SHA_512 (RFC 7518 §5.2.5): a // 64-octet CEK split into a 32-octet HMAC-SHA-512 key and a // 32-octet AES-256 key, PKCS #7-padded CBC encryption under a // 128-bit IV, and an authentication tag computed as HMAC-SHA-512 diff --git a/federation/trustmark_issue.go b/federation/trustmark_issue.go index f51aee4a..d68fe544 100644 --- a/federation/trustmark_issue.go +++ b/federation/trustmark_issue.go @@ -166,7 +166,7 @@ type DelegationParams struct { TrustMarkType string // Lifetime bounds how long the delegation is valid for. Zero omits - // the "exp" claim entirely — OpenID Federation 1.0 §7.2: "If not + // the "exp" claim entirely — OpenID Federation 1.0 §7.2.1: "If not // present, it means that the delegation does not expire." Lifetime time.Duration } diff --git a/internal/dpop/header.go b/internal/dpop/header.go index 19a99a98..b52846ed 100644 --- a/internal/dpop/header.go +++ b/internal/dpop/header.go @@ -2,8 +2,8 @@ package dpop // ResolveHeaderValues reduces values — every "DPoP" HTTP header value a // request carried, in receipt order — to the single proof value this -// package's own verification expects, enforcing RFC 9449 §7.1 itself -// ("the client... MUST NOT send this header field multiple times") +// package's own verification expects, enforcing RFC 9449 §4.3 check 1 +// itself ("There is not more than one DPoP HTTP request header field") // rather than leaving that check to whichever HTTP adapter a caller // happens to use. net/http's own Header.Get silently returns only the // first of several values instead of rejecting the request, so a diff --git a/internal/federation/errors.go b/internal/federation/errors.go index c5998946..30b6898b 100644 --- a/internal/federation/errors.go +++ b/internal/federation/errors.go @@ -82,7 +82,7 @@ var ( // ErrTrustMarkDelegationWrongType indicates a Trust Mark Delegation // JWT's "typ" header was not "trust-mark-delegation+jwt" (OpenID - // Federation 1.0 §7.2's own "Trust Mark delegation JWTs without a + // Federation 1.0 §7.2.1's "Trust Mark delegation JWTs without a // typ header parameter or with a different typ value MUST be // rejected"). ErrTrustMarkDelegationWrongType = errors.New("federation: trust mark delegation header typ is not trust-mark-delegation+jwt") diff --git a/internal/federation/trustmark.go b/internal/federation/trustmark.go index 60f83e55..4911aa1d 100644 --- a/internal/federation/trustmark.go +++ b/internal/federation/trustmark.go @@ -230,7 +230,7 @@ type CreateTrustMarkParams struct { Algorithm fapi.SignatureAlgorithm // KeyID is recorded in the Trust Mark's "kid" header. Required — - // OpenID Federation 1.0 §7.1: "Trust Mark JWTs MUST include the kid + // OpenID Federation 1.0 §7: "Trust Mark JWTs MUST include the kid // header parameter," unlike an Entity Statement's own KeyID // (CreateParams' own field, optional there). KeyID string @@ -268,7 +268,7 @@ func CreateTrustMark(p CreateTrustMarkParams) (string, error) { Issuer: p.Issuer, Subject: p.Subject, TrustMarkType: p.TrustMarkType, Now: p.Now, Lifetime: p.Lifetime, Delegation: p.Delegation, typ: trustMarkJWTType, - kidRequirement: `OpenID Federation 1.0 §7.1: "Trust Mark JWTs MUST include the kid header parameter"`, + kidRequirement: `OpenID Federation 1.0 §7: "Trust Mark JWTs MUST include the kid header parameter"`, }) } @@ -333,7 +333,7 @@ func createTrustMarkLikeJWT(p trustMarkLikeParams) (string, error) { } // trustMarkDelegationJWTType is the JWS "typ" header value every Trust -// Mark Delegation JWT MUST carry (OpenID Federation 1.0 §7.2: "MUST be +// Mark Delegation JWT MUST carry (OpenID Federation 1.0 §7.2.1: "MUST be // explicitly typed, by setting the typ header parameter to // trust-mark-delegation+jwt... Trust Mark delegation JWTs without a typ // header parameter or with a different typ value MUST be rejected"). @@ -349,7 +349,7 @@ type TrustMarkDelegationClaims struct { TrustMarkType string IssuedAt time.Time - // ExpiresAt is zero if the "exp" claim was absent — per §7.2, "If + // ExpiresAt is zero if the "exp" claim was absent — per §7.2.1, "If // not present, it means that the delegation does not expire." ExpiresAt time.Time } @@ -412,19 +412,19 @@ type TrustMarkDelegationVerifyPolicy struct { // ExpectedIssuer is the Trust Mark type's real owner, as named by // the Trust Anchor's own "trust_mark_owners" claim — the // delegation's iss claim must equal it exactly (OpenID Federation - // 1.0 §7.2's own validation step 5: "The Entity Identifier of the + // 1.0 §7.2.2's validation step 5: "The Entity Identifier of the // Trust Mark Owner MUST match the value of iss in the delegation"). ExpectedIssuer string // ExpectedSubject is the Trust Mark Issuer being delegated to — the - // delegation's sub claim must equal it exactly (§7.2's own step 4: + // delegation's sub claim must equal it exactly (§7.2.2 step 4: // "The Entity Identifier of the Trust Mark Issuer MUST match the // value of sub in the delegation"). ExpectedSubject string // ExpectedTrustMarkType is the Trust Mark's own trust_mark_type — // the delegation's trust_mark_type claim must equal it exactly - // (§7.2's own step 7). + // (§7.2.2 step 8). ExpectedTrustMarkType string // Algorithm is the algorithm the Trust Mark Owner is registered or @@ -531,7 +531,7 @@ type CreateTrustMarkDelegationParams struct { Algorithm fapi.SignatureAlgorithm // KeyID is recorded in the delegation's "kid" header. Required — - // OpenID Federation 1.0 §7.2: "Trust Mark delegation JWTs MUST + // OpenID Federation 1.0 §7.2.1: "Trust Mark delegation JWTs MUST // include the kid header parameter." KeyID string @@ -564,6 +564,6 @@ func CreateTrustMarkDelegation(p CreateTrustMarkDelegationParams) (string, error Issuer: p.Issuer, Subject: p.Subject, TrustMarkType: p.TrustMarkType, Now: p.Now, Lifetime: p.Lifetime, typ: trustMarkDelegationJWTType, - kidRequirement: `OpenID Federation 1.0 §7.2: "Trust Mark delegation JWTs MUST include the kid header parameter"`, + kidRequirement: `OpenID Federation 1.0 §7.2.1: "Trust Mark delegation JWTs MUST include the kid header parameter"`, }) } diff --git a/internal/jwe/doc.go b/internal/jwe/doc.go index 80a833c8..399c9b48 100644 --- a/internal/jwe/doc.go +++ b/internal/jwe/doc.go @@ -5,7 +5,7 @@ // content-encryption algorithm fapi.ContentEncryptionAlgorithm // supports: A256GCM (a single AEAD primitive) or A256CBC-HS512 // (encrypt-then-MAC — AES-256-CBC plus a separate HMAC-SHA-512 tag, -// RFC 7518 §5.2.3). +// RFC 7518 §5.2.5). // // Like internal/jose, this package is intentionally low-level and // generic: it knows nothing about ID tokens, nested JWTs, or any other diff --git a/internal/jwe/keywrap.go b/internal/jwe/keywrap.go index 954a638c..f938860d 100644 --- a/internal/jwe/keywrap.go +++ b/internal/jwe/keywrap.go @@ -14,7 +14,7 @@ var aesKeyWrapIV = [8]byte{0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6} // aesKeyWrap wraps plaintext (which must be a multiple of 8 bytes, at // least 16) under kek using AES Key Wrap (RFC 3394), returning // ciphertext 8 bytes longer than plaintext. This exists because JOSE's -// ECDH-ES+A256KW key management (RFC 7518 §4.7) wraps the CEK this way, +// ECDH-ES+A256KW key management (RFC 7518 §4.6) wraps the CEK this way, // and Go's standard library has no AES Key Wrap implementation — only // the underlying AES block cipher this function builds on // (crypto/aes.NewCipher). diff --git a/internal/requestobject/create.go b/internal/requestobject/create.go index d8ed93ce..09d24517 100644 --- a/internal/requestobject/create.go +++ b/internal/requestobject/create.go @@ -30,7 +30,7 @@ type CreateParams struct { // registered JWKS without trial and error. KeyID string - // ClientID is the "iss" claim (RFC 9101 §5.3). If Parameters + // ClientID is the "iss" claim. If Parameters // contains a "client_id" entry, it must equal ClientID exactly. ClientID string diff --git a/internal/requestobject/errors.go b/internal/requestobject/errors.go index 631a76f2..90d5aaa6 100644 --- a/internal/requestobject/errors.go +++ b/internal/requestobject/errors.go @@ -15,7 +15,9 @@ var ( ErrMalformedClaims = errors.New("requestobject: malformed claims") // ErrClientIDIssuerMismatch indicates the payload's "client_id" - // parameter (RFC 9101 §5.3) did not equal its "iss" claim. + // parameter did not equal its "iss" claim, which names the client + // that signed it (RFC 9101 §6.3 has the request's and the request + // object's client_id be identical). ErrClientIDIssuerMismatch = errors.New("requestobject: client_id parameter does not match iss") // ErrIssuerMismatch indicates the object's iss claim did not equal diff --git a/internal/token/errors.go b/internal/token/errors.go index 952ef7b1..9e112419 100644 --- a/internal/token/errors.go +++ b/internal/token/errors.go @@ -36,13 +36,15 @@ var ( ErrNonceMismatch = errors.New("token: nonce does not match expected value") // ErrMissingAuthorizedParty indicates an ID token's aud claim named - // more than one audience but carried no azp claim (OIDC Core - // §3.1.3.7 step 9) — needed to disambiguate which of those - // audiences the token was actually authorized for. + // more than one audience but carried no azp claim, needed to + // disambiguate which of those audiences the token was actually + // authorized for (see OIDC Core §2 on azp, and §3.1.3.7 steps 4-5). + // The current text of Core no longer has the client require it; + // this package still does. ErrMissingAuthorizedParty = errors.New("token: aud has multiple audiences but azp is missing") // ErrAuthorizedPartyMismatch indicates an ID token's azp claim did - // not equal the caller's own client ID (OIDC Core §3.1.3.7 step 10). + // not equal the caller's own client ID (OIDC Core §3.1.3.7 step 5). ErrAuthorizedPartyMismatch = errors.New("token: azp does not match expected audience") // ErrIssuedAtTooOld indicates an ID token's iat claim is further in diff --git a/internal/token/idtoken.go b/internal/token/idtoken.go index b9db2cd4..6edc0e22 100644 --- a/internal/token/idtoken.go +++ b/internal/token/idtoken.go @@ -14,10 +14,11 @@ import ( // just the one-element case) — unlike internal/clientassertion's own // Audience, which stays a single string because a client assertion is // always addressed to exactly one token endpoint, never several -// audiences at once. §3.1.3.7 step 9 additionally says a client "SHOULD -// verify that an azp Claim is present" when there's more than one -// audience, and step 10 that it "SHOULD verify" azp equals the client's -// own ID when present — IDTokenValidatePolicy.Validate enforces both. +// audiences at once. IDTokenValidatePolicy.Validate requires an azp +// claim when there's more than one audience, which earlier editions of +// OIDC Core §3.1.3.7 had a client check and the current one leaves to +// it, and checks that azp, when present, equals the client's own ID +// (§3.1.3.7 step 5: the client "SHOULD validate the azp value"). // // Everything beyond the claims this struct names is left in Parameters // as raw JSON. diff --git a/internal/token/validate.go b/internal/token/validate.go index 04547df6..b7740e6f 100644 --- a/internal/token/validate.go +++ b/internal/token/validate.go @@ -412,13 +412,14 @@ func (t IDToken) Validate(pub crypto.PublicKey, policy IDTokenValidatePolicy) (V return ValidatedIDToken{}, ErrNonceMismatch } } - // OIDC Core §3.1.3.6: at_hash, when present, binds this ID token to - // one specific access token — "the Client MUST validate the access - // token by calculating its hash... and comparing it to the value of - // the at_hash Claim." A token with no at_hash at all is not itself - // rejected here: OIDC Core marks it REQUIRED only "except when the - // ID Token is issued from the Authorization Endpoint" — this - // package has no way to know from the claims alone whether an + // OIDC Core §3.1.3.8: at_hash, when present, binds this ID token to + // one specific access token, which the client "MAY use ... to + // validate", as §3.2.2.9 describes ("The value of at_hash in the ID + // Token MUST match the value produced in the previous step"). A token + // with no at_hash at all is not itself rejected here: for the code + // flow §3.1.3.6 makes it OPTIONAL, REQUIRED only when an access token + // is issued from the Authorization Endpoint (§3.2.2.10, §3.3.2.11) — + // this package has no way to know from the claims alone whether an // issuer that omitted it did so because it doesn't implement this // check, and rejecting every such token would make this a stricter // contract than what was actually reported missing (see this @@ -485,13 +486,14 @@ func checkIDTokenAudience(c IDTokenClaims, policy IDTokenValidatePolicy) error { return nil } -// checkATHash enforces OIDC Core §3.1.3.6: at_hash, when present, binds -// this ID token to one specific access token — "the Client MUST -// validate the access token by calculating its hash... and comparing -// it to the value of the at_hash Claim." A token with no at_hash at -// all is not itself rejected here: OIDC Core marks it REQUIRED only -// "except when the ID Token is issued from the Authorization Endpoint" -// — this package has no way to know from the claims alone whether an +// checkATHash checks at_hash, which, when present, binds this ID token +// to one specific access token (OIDC Core §3.1.3.8, validated as +// §3.2.2.9 describes: "The value of at_hash in the ID Token MUST match +// the value produced in the previous step"). A token with no at_hash at +// all is not itself rejected here: for the code flow §3.1.3.6 makes it +// OPTIONAL, REQUIRED only when an access token is issued from the +// Authorization Endpoint (§3.2.2.10, §3.3.2.11) — this package has no +// way to know from the claims alone whether an // issuer that omitted it did so because it doesn't implement this // check, and rejecting every such token would make this a stricter // contract than what was actually reported missing (see diff --git a/resource/dpop_nonce_header.go b/resource/dpop_nonce_header.go index 707609b2..4a1b0011 100644 --- a/resource/dpop_nonce_header.go +++ b/resource/dpop_nonce_header.go @@ -4,7 +4,7 @@ import "net/http" // SetDPoPNonce sets h's DPoP-Nonce header to a.NextDPoPNonce, when Verify // issued one, so the client's next request already carries a valid -// nonce (RFC 9449 §8). Call it on every successful response: +// nonce (RFC 9449 §9). Call it on every successful response: // // authz, err := verifier.Verify(ctx, resource.VerifyRequestFromHTTP(r, target)) // // ... diff --git a/resource/errors.go b/resource/errors.go index db9ca7a2..a1939e6e 100644 --- a/resource/errors.go +++ b/resource/errors.go @@ -21,7 +21,7 @@ const ( // ErrorUseDPoPNonce indicates the presented DPoP proof was otherwise // valid but carried no nonce, or one this verifier didn't just issue - // (unknown, already consumed, or expired) — RFC 9449 §8's own error + // (unknown, already consumed, or expired) — RFC 9449 §7.1 and §9's error // value, distinct from ErrorInvalidToken (the token itself is fine; // the caller just needs to retry with the nonce this error's own // Nonce method returns). Only ever returned when @@ -156,7 +156,7 @@ func (e *Error) Unwrap() error { return e.cause } // WriteJSON writes e as a complete RFC 6750 §3.1 challenge response to // w: a WWW-Authenticate header, the DPoP-Nonce header when Nonce is -// non-empty (RFC 9449 §8 — see Nonce's own doc comment for when that +// non-empty (RFC 9449 §9 — see Nonce's own doc comment for when that // is), the "application/json" Content-Type, e's own HTTPStatus, and a // {"error": ..., "error_description": ...} body built from Code and // PublicDescription — never Unwrap's cause. Every *Error this diff --git a/resource/verify.go b/resource/verify.go index f279d999..69a024af 100644 --- a/resource/verify.go +++ b/resource/verify.go @@ -148,7 +148,7 @@ type AuthorizationContext struct { // NextDPoPNonce is a freshly issued DPoP nonce the caller should set // as this response's own DPoP-Nonce header, so its next call already // carries a valid one instead of needing its own challenge/retry - // round trip (RFC 9449 §8's own proactive-refresh recommendation): + // round trip (RFC 9449 §9): // SetDPoPNonce does that. // Always "" when Dependencies.Nonces is nil (nonce-challenge support // disabled); otherwise always populated on a successful Verify. diff --git a/server/dpop_headers.go b/server/dpop_headers.go index eb71bf17..7f3ddfa4 100644 --- a/server/dpop_headers.go +++ b/server/dpop_headers.go @@ -4,7 +4,7 @@ import "net/http" // DPoPProofsFromHTTP returns every "DPoP" header value r carried, in // receipt order — net/http's own r.Header.Values("DPoP"), which -// preserves duplicates deliberately: RFC 9449 §7.1 requires rejecting +// preserves duplicates deliberately: RFC 9449 §4.3 check 1 requires rejecting // a request that carries more than one, a check every DPoPProofs field // in this package (AuthorizationCodeExchangeRequest and its siblings) // only gets right if the caller extracted headers this way. The more diff --git a/server/errors.go b/server/errors.go index 8c9d17a0..fe21623b 100644 --- a/server/errors.go +++ b/server/errors.go @@ -40,7 +40,7 @@ const ( // token RevokeToken can't revoke: an access token. ErrorUnsupportedTokenType ErrorCode = "unsupported_token_type" - // ErrorInvalidRequestURI is RFC 9126 §2.3's dedicated error code for + // ErrorInvalidRequestURI is RFC 9101 §7's error code for // a request_uri the authorization endpoint cannot use — unknown, // already consumed by a completed interaction, expired, or pushed // by a different client than the one now presenting it — as diff --git a/server/par.go b/server/par.go index 294e1516..ad1569d6 100644 --- a/server/par.go +++ b/server/par.go @@ -539,7 +539,7 @@ func (s *Server) acceptableClientAssertionAudiences(client storage.RegisteredCli // request object's own "resource" member with one, where every other // form parameter is ignored too. func (s *Server) resolveAuthorizationParameters(ctx context.Context, params map[string]string, formResources []string, client storage.RegisteredClient) (map[string]json.RawMessage, map[string]json.RawMessage, []string, *Error) { - // RFC 9101 §5 / PAR-2.1: "request_uri" is never a meaningful *input* + // RFC 9126 §2.1 / PAR-2.1: "request_uri" is never a meaningful *input* // here, whether or not "request" is also present — PAR generates its // own request_uri as this call's *result*, and a client is never // meant to redeem one request_uri by presenting another. Checked @@ -605,9 +605,9 @@ func (s *Server) resolveAuthorizationParameters(ctx context.Context, params map[ if err != nil { return nil, nil, nil, newError(ErrorInvalidRequestObject, 400, "request object verification failed", err) } - // Same RFC 9101 §5 / PAR-2.1 prohibition as the sibling-parameter - // check above, but for "request_uri" claimed *inside* the signed - // request object instead of alongside it. This can no longer rely on + // RFC 9101 §4 ("request and request_uri parameters MUST NOT be + // included in Request Objects"): the counterpart, inside the signed + // request object, of the sibling-parameter check above. This can no longer rely on // checkExtensions'/Registry.Parse's own unregistered-parameter // handling to catch it: that now ignores (and strips) an unrecognized // name rather than rejecting the request over it (RFC 6749 §3.1), so diff --git a/server/token.go b/server/token.go index ec61a5ba..e66e23a3 100644 --- a/server/token.go +++ b/server/token.go @@ -32,7 +32,7 @@ type AuthorizationCodeExchangeRequest struct { // receipt order — pass net/http's own Header.Values("DPoP") // directly, not Header.Get, which silently discards every value but // the first instead of preserving them for this package to reject - // (RFC 9449 §7.1: a request must never carry more than one). Empty + // (RFC 9449 §4.3 check 1: a request must never carry more than one). Empty // is a valid "no proof presented" when the authenticated client's // storage.RegisteredClient.SenderConstrain() isn't SenderConstrainDPoP // (the default); a required proof is otherwise exactly one value. @@ -455,7 +455,7 @@ func (s *Server) nextDPoPNonce(ctx context.Context, client storage.RegisteredCli // down to the single proof value the rest of this package's DPoP // verification expects, via internal/dpop.ResolveHeaderValues; see that // function's own doc comment for why more than one is always rejected -// (RFC 9449 §7.1) regardless of whether any individual value would +// (RFC 9449 §4.3 check 1) regardless of whether any individual value would // otherwise have verified. Every public method whose request type // carries a DPoPProofs field calls this first, before doing anything // else with the request — the same "check malformed structural things diff --git a/storage/backchannel.go b/storage/backchannel.go index a53c30b3..19cd5fb7 100644 --- a/storage/backchannel.go +++ b/storage/backchannel.go @@ -178,7 +178,7 @@ type PolledBackchannelAuthentication struct { // BackchannelAuthenticationAlreadyRedeemedError mirrors // AuthorizationCodeAlreadyRedeemedError exactly: returned when a poll // observes an already-consumed Approved record — a CIBA auth_req_id -// issues tokens exactly once (CIBA §10.3). +// issues tokens exactly once (CIBA §10.1.1). type BackchannelAuthenticationAlreadyRedeemedError struct { IssuedAccessTokenKey string IssuedRefreshTokenHash *[32]byte @@ -261,7 +261,7 @@ type BackchannelAuthenticationStore interface { // the record redeemed and returns it; every subsequent poll for // the same AuthReqIDHash returns // *BackchannelAuthenticationAlreadyRedeemedError — an approved - // auth_req_id issues tokens exactly once (CIBA §10.3), the same + // auth_req_id issues tokens exactly once (CIBA §10.1.1), the same // single-use guarantee RedeemAuthorizationCode has. // It returns a plain error if AuthReqIDHash is unknown, and, before // any of the above, a plain error that leaves the record unchanged diff --git a/storage/client_repository.go b/storage/client_repository.go index 63c75124..e042bed3 100644 --- a/storage/client_repository.go +++ b/storage/client_repository.go @@ -172,7 +172,7 @@ type BackchannelTokenDeliveryMode uint8 const ( // BackchannelTokenDeliveryModePoll means the client itself polls - // the token endpoint on a schedule (CIBA §10.3) — the zero value, + // the token endpoint on a schedule (CIBA §10.1) — the zero value, // so every registered client that predates this field keeps // behaving exactly as it did before. BackchannelTokenDeliveryModePoll BackchannelTokenDeliveryMode = iota @@ -181,7 +181,7 @@ const ( // notifies the client's own BackchannelClientNotificationEndpoint // once a decision is reached (CIBA §10.2), so the client can poll // immediately instead of on a fixed schedule. The client's backup - // polling (CIBA §10.3) remains valid regardless — a missed or + // polling (CIBA §10.1) remains valid regardless — a missed or // failed notification is never itself an error condition. BackchannelTokenDeliveryModePing )