From 890c5092a281ee8cba761bce3c3104b664f29cb6 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 28 Sep 2026 00:06:12 +0800 Subject: [PATCH] docs: correct stale and inaccurate documentation across the repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full review of the Markdown docs and package doc comments against the current code. The notable corrections: - server/doc.go said access tokens were DPoP-only with "mTLS binding not supported", and that refresh tokens rotate on every use; mTLS sender-constraining is supported, and refresh tokens are deliberately not rotated (FAPI 2.0 SP §5.3.2.1). - GETTING_STARTED.md's server.New example omitted the required ClientCertificateTrust dependency and failed as written, and pointed at a removed conformance-as helper instead of keys.NewLocalIssuerKeys. - ARCHITECTURE.md named types and APIs that don't exist or were never built as described (root Scope/Issuer types, DPoPKeyHandle, SelectSigningKey, AuthorizationPolicy, an Exposure tag on errors, configurable fail-closed audit, a "client:jarm" replay namespace, stores in the client/server packages), and linked a nonexistent "Hardening rules" section. It now also opens its conformance history with current results. - README.md omitted attestation-based client authentication and the client-side OAuthOnly option; SECURITY.md said there were no tagged releases; conformance docs carried outdated CIBA framing, module counts and a "twenty-one" leg count (there are 22). - UPGRADING.md gains v0.38.0's invalid_scope and error-text behaviour changes; CONTRIBUTING.md and AGENTS.md ask breaking PRs to add an UPGRADING.md section. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/conformance.yml | 8 +- AGENTS.md | 4 +- ARCHITECTURE.md | 168 +++++++++++++++------------ CONTRIBUTING.md | 3 + GETTING_STARTED.md | 23 ++-- README.md | 9 +- SECURITY.md | 8 +- UPGRADING.md | 20 ++++ client/doc.go | 21 ++-- conformance/README.md | 26 +++-- conformance/server/scripts/README.md | 2 +- keys/doc.go | 10 ++ resource/doc.go | 6 +- server/assurance.go | 3 - server/doc.go | 55 +++++---- storage/doc.go | 9 +- 16 files changed, 229 insertions(+), 146 deletions(-) diff --git a/.github/workflows/conformance.yml b/.github/workflows/conformance.yml index fb163002..5d9ecede 100644 --- a/.github/workflows/conformance.yml +++ b/.github/workflows/conformance.yml @@ -1,8 +1,8 @@ name: FAPI Conformance -# Runs all twenty-one FAPI2/FAPI-CIBA/OpenID-Federation test +# Runs all twenty-two FAPI2/FAPI-CIBA/OpenID-Federation test # configurations this repo has driver support for — one underlying OIDF -# conformance suite, exercised under twenty-one different plan/variant +# conformance suite, exercised under twenty-two different plan/variant # combinations: including the CIBA/mTLS pair (AS ciba-mtls, RP # ciba-mtls), AS ciba-ping, the client-auth-mtls pair (AS/RP), the # client-auth-mtls-and-mtls pair (AS/RP), the two ciba-*-client-auth-mtls @@ -94,14 +94,14 @@ jobs: run: ./conformance/server/scripts/generate-server-cert.sh # run-all.sh itself waits for the suite to become reachable, waits - # for the AS containers to come up, runs all twenty-one test + # for the AS containers to come up, runs all twenty-two test # configurations (including bringing up its own federation # containers and syncing their ephemeral keys — no separate step # needed here for that), retries the one known suite-internal # flake automatically, and writes report.md — nothing CI-specific # needed here beyond pointing it at the suite checkout and a # predictable results directory. - - name: Run all twenty-one conformance test configurations + - name: Run all twenty-two conformance test configurations env: CONFORMANCE_SUITE_CHECKOUT: /tmp/conformance-suite WORKDIR: ${{ runner.temp }}/conformance-results diff --git a/AGENTS.md b/AGENTS.md index 2a919115..640b3b26 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,6 +21,8 @@ where deeper, hard-won knowledge already lives. `chore:`/`test:`/`refactor:`/`ci:` for anything that shouldn't bump the version) — see CONTRIBUTING.md's "Commit messages" section for the full rationale. Don't invent non-standard types. +- A breaking PR (`feat!:`/`fix!:`) also adds its section to + [UPGRADING.md](UPGRADING.md) in the same PR — see CONTRIBUTING.md. - Merging a PR here triggers an automatic release-please PR bumping the version and `CHANGELOG.md` — that PR needs its own merge (by a human or on explicit instruction) before the new version is actually @@ -69,7 +71,7 @@ flag and use the resulting page instead of hand-constructing ## Client alias naming convention When registering conformance-testing clients (locally or for a hosted -run), this session settled on `fapigo-{family}-{axis1}-{axis2}`, +run), this repo uses `fapigo-{family}-{axis1}-{axis2}`, mirroring the OIDF certification matrix's own column names: - `fapigo-sp-{auth}-{sender-constrain}` — FAPI2SP authorization_code diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 2e620576..fe639762 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -23,7 +23,7 @@ A caller decides who authenticated, what access was approved and which durable infrastructure to use. It must never be able to weaken request validation, bypass replay prevention, modify a signed output, or construct a security-sensitive protocol artefact by hand. See -"Hardening rules for every role's public API" below. +"Design rules" below. ## The one rule everything else follows @@ -106,13 +106,14 @@ Not `fapi.New(RoleClient, ...)`. ### 2. Shared value types only where semantics match Identifiers and enums with one wire-level meaning regardless of role -(`ClientID`, `Scope`, `Issuer`, `SignatureAlgorithm`, `SenderConstraint`) -live in the root `fapi` package. Anything whose meaning depends on -trust state does not — `client.AuthorizationRequest` (an instruction to -construct a request) and `server.ValidatedAuthorizationRequest` -(something already checked) are deliberately different types, so code -can never accidentally treat untrusted input as validated protocol -state. +(`ClientID`, `RegisteredRedirectURI`, `SignatureAlgorithm`, +`KeyManagementAlgorithm`, `ContentEncryptionAlgorithm`) live in the root +`fapi` package. Anything whose meaning depends on trust state does not — +`client.BeginAuthorizationRequest` (an instruction to construct a +request) and `server.InteractionRequest` (a request the server has +already validated, handed to the login UI) are deliberately different +types, so code can never accidentally treat untrusted input as +validated protocol state. Two more value types belong here because every role needs them identically: @@ -127,8 +128,7 @@ identically: enabled loopback development mode), no fragment, no embedded credentials, normalized host. Registered redirect URIs are compared under OAuth registration semantics, never generic URL equivalence or - automatic normalization — see `RegisteredRedirectURI` in `client` and - `server`. + automatic normalization — see `fapi.RegisteredRedirectURI`. `SignatureAlgorithm` is a closed enum (`ES256`, `PS256`, ...), never a bare string accepted from a caller or read directly out of a JWT header @@ -177,23 +177,24 @@ against `internal/dpop` itself. ### 4. Client session storage is atomic-consume, not CRUD -`SessionStore.Create` / `SessionStore.Consume` — no `GetSession` / -`DeleteSession`. `Consume` atomically validates and retires `state`, -nonce, PKCE verifier, expected issuer, expected redirect URI, expected -response mode, DPoP key reference and request-object identifier in one -step, to prevent callback replay and race conditions. +`storage.SessionStore.Create` / `Consume` — no `GetSession` / +`DeleteSession`. `Consume` atomically looks up and retires a session by +its `state`, returning the nonce, PKCE verifier, expected issuer, +expected redirect URI, expected response mode and expiry recorded with +it, in one step, to prevent callback replay and race conditions. ### 5. Keys are handles and operations, never raw private keys `KeyManager.Sign` / `KeyManager.PublicKey`, keyed by purpose (`ClientAuthentication`, `RequestObjectSigning`, `DPoPProofSigning`). A -session refers to a DPoP key by an opaque `DPoPKeyHandle`, never a -`crypto.PrivateKey` — DPoP's value depends on the private key never -leaving its holder ([RFC 9449][dpop]). +client's DPoP proofs are signed through its `KeyManager` under the +`DPoPProofSigning` purpose, never with a `crypto.PrivateKey` this module +holds — DPoP's value depends on the private key never leaving its +holder ([RFC 9449][dpop]). `keys.KeyManager` never hands back a `crypto.Signer`, only a `Sign` -operation and a public JWK — the same model `server` uses to select and -use its own signing keys (`SelectSigningKey` / `Sign` / `PublicJWKS`), +operation and a public JWK — the same model `server` uses for its own +signing keys (`KeyManager.Sign`, published through `Server.PublicJWKS`), so both a client's and a server's key material can be backed by an HSM or a remote signing service without the module ever holding private key material in process. `client.PublicJWKS` is this same publication @@ -377,9 +378,9 @@ looks like a normal redirect: invalid combinations of subject/grant/denial can't be constructed. Untrusted hints stay untrusted: `AuthenticationHints.LoginHint` is a -plain string-wrapping type, never a `SubjectID` — only a -`SubjectProvider` or the application's own authentication result can -produce a verified `SubjectID`. +plain string-wrapping type, never a `SubjectID` — only the +application's own authentication result (an `AuthenticatedSubject` +passed to `Authorize`) produces a verified subject. **Assurance levels.** `Config.Assurance` is `AssuranceDevelopment` or `AssuranceProduction`. `New` fails construction unless every @@ -399,25 +400,28 @@ and endpoint URLs. The assurance level is itself a required `Config.Assurance` choice with no default, so a caller can never end up on the development level by omission. -**Policy is a bounded deployment decision, not a bypass.** -`AuthorizationPolicy.Evaluate` receives only already-validated protocol -values (`RegisteredClient`, `AuthenticatedSubject`, -`RequestedAuthorization`, `AuthenticationContext`, validated extensions) -and may decide allowed scopes, claims, authorization details, consent -requirements and token lifetime within configured bounds — it cannot -disable PAR, PKCE, sender constraint, redirect URI validation, client -authentication, replay protection, required signed request objects, or -profile algorithm restrictions. - -**Audit is a typed dependency**, not a side channel a caller can leave -disconnected: `Dependencies.Audit` records structured `AuditEvent`s -(id, time, type, outcome, client/subject/transaction references, typed -attributes — never a bare `map[string]any`, so a sensitive value can't -end up in an audit record by accident). Whether audit failure is -fail-closed for issuance events, buffered through a durable outbox, or -tolerated for low-value diagnostics is a `server` configuration -decision, not something left to `AuditSink` implementers to each decide -differently. +**Policy is a bounded deployment decision, not a bypass.** The +application's decision is a `GrantedAuthorization` passed to +`Authorize`, made against an already-validated `InteractionRequest`: +the granted scope (which `CompleteAuthorization` rejects if it isn't a +subset of what was requested), approved `AuthorizationDetails` (each an +acceptable narrowing of a requested object, per +`RARDefinition.ValidateGrant`), and extra ID-token claims (server-managed +names rejected, total size bounded by `Limits.MaxIDTokenClaimsBytes`). +An optional `RARPolicy` narrows what a client may request before a +resource owner ever sees it. None of this can disable PAR, PKCE, sender +constraint, redirect URI validation, client authentication, replay +protection, required signed request objects, or profile algorithm +restrictions. + +**Audit is a typed dependency**, required under `AssuranceProduction`: +`Dependencies.Audit` records structured `AuditEvent`s (type, time, +client ID, outcome and a short safe description such as an error code +— never a bare `map[string]any`, a raw internal error, or anything that +could carry a token, key or assertion value). Recording is best-effort: +a failing `AuditSink.Record` never fails the request it describes, so a +sink that must not lose events has to provide its own durability (a +local outbox, for example). **Dynamic client registration ([RFC 7591][dcr] / [RFC 7592][dcrm]), if added, extends this state machine — it doesn't bypass it.** Not @@ -499,10 +503,10 @@ policy. ### 10–11. Extension and RAR parameters are defined once, used by both sides -A `extension.Definition[T]` captures wire name, cardinality, encoding, -allowed source, max size, sensitivity, validator, whether it's -integrity-protected, whether it may appear in request objects, and -whether it may be returned in token claims — once. Client sets a value +An `extension.Definition[T]` captures wire name, cardinality, allowed +sources (a plain parameter, an integrity-protected request object, or +both), max size, sensitivity, validator, and whether it may be returned +in token claims — once. Client sets a value against the definition with `extension.Set(&req.Extensions, Definition, value)`; server registers the same definition in `server.Config.Extensions` (an `*extension.Registry`) and reads the validated value back out through @@ -603,14 +607,14 @@ validated objects back out, typed, with `extension.RARGet`. ### 12. Configuration is per-role, not one shared struct `client.Config`, `server.Config` and `resource.Config` are separate -types. They may reference the same validated value types (`Issuer`, -algorithm policy types) but are not merged into one struct with +types. They may reference the same validated value types (`fapi.URL`, +algorithm types) but are not merged into one struct with role-conditional fields. ### 13. Storage contracts are per-role, with one shared replay primitive -`client.SessionStore`, `server.TransactionStore`, `server.GrantStore` are -distinct, and none of them expose generic CRUD (`GetSession`, +`storage.SessionStore` (client), `storage.TransactionStore` and +`storage.GrantStore` (server) are distinct, and none of them expose generic CRUD (`GetSession`, `GetCode`, `UpdateCode`, `DeleteCode`, ...) — every method is a named security operation. `GrantStore.RedeemAuthorizationCode` in particular must atomically look up and consume a code by its hash, in one step; @@ -622,11 +626,10 @@ the request's parameters, the granted scope, subject, authentication context and claims — is one opaque, versioned JSON value (`Request` or `Grant`) the store persists and returns without interpreting, so a feature that changes what a grant carries never changes a store. -`replay.Store` -stores only a digest and expiry per use (`ReplayUse{Namespace, Digest, +`storage.ReplayStore` stores only a digest and expiry per use (`ReplayUse{Namespace, Digest, ExpiresAt}`) — never a complete client assertion, DPoP proof or other sensitive payload — and callers must assign it a namespaced identifier -per role/subsystem (`client:jarm`, `server:request-object`, +per role/subsystem (`server:client-assertion`, `server:request-object`, `server:dpop`, `resource:dpop`, ...) so different subsystems can never collide on the same use-once token. @@ -637,9 +640,12 @@ cross-instance-consistent, encrypted-at-rest) that `server`'s `AssuranceProduction` mode checks at construction time, and a reusable contract test suite (e.g. a `storage.TestGrantStoreContract(t, factory)` helper) that any storage implementation — first-party or -downstream — runs against concurrent redemption, expiry boundaries, -cancellation, transaction rollback and cross-connection consistency, -rather than relying on the capability declaration alone. +downstream — runs for single-use redemption under concurrency, field +round-tripping, unknown-key handling and revocation, rather than +relying on the capability declaration alone. What it can't observe +through the interface (cross-instance atomicity, expiry eviction, +encryption at rest) stays the backend's own responsibility — see +`storage/doc.go`. ### 14. No cross-role dependency cycles @@ -679,16 +685,20 @@ repo's own non-test Go files." ### 16. Errors carry their own exposure — the caller doesn't decide -`client`, `server` and `resource` all return a typed `Error` (`Code()`, -`PublicDescription()`, `HTTPStatus()`, `Unwrap()`) tagged with an -`Exposure` — `ExposureLocal`, `ExposureRedirect`, `ExposureTokenEndpoint` -for `server`; the equivalent split for `client` and `resource`. The -engine, not the embedding application, decides whether a failure is safe -to put in a redirect query string versus a response body versus neither; -internal diagnostic detail is never copied into a public -`error_description`-style field. This is what makes rule 7's "an -unvalidated `redirect_uri` must produce a local error, never a redirect" -enforceable in the type system rather than by convention. +`client`, `server` and `resource` all return a typed `Error` — `Code()`, +`PublicDescription()` and `Unwrap()` everywhere, `HTTPStatus()` on +`server` and `resource`, and on `client` a `ServerResponse()` carrying +the error response a server sent, if any. The engine, not the embedding +application, decides whether a failure is safe to put in a redirect +query string versus a response body versus neither, and it does so +through the result's shape: `server`'s authorization-endpoint methods +return a local-error variant (`LocalErrorResponse`, +`AuthorizationLocalError`) distinct from a redirect, and a token or PAR +failure is written with `Error.WriteJSON`. Internal diagnostic detail is +never copied into a public `error_description`-style field. This is what +makes rule 7's "an unvalidated `redirect_uri` must produce a local +error, never a redirect" enforceable in the type system rather than by +convention. `server.Error` goes one step further for its own token-endpoint-exposure case: `WriteJSON(w http.ResponseWriter)` also owns *how* to encode that @@ -713,12 +723,24 @@ passing its suite is not evidence the other role conforms, even where both share internal JOSE code — protocol behaviour and negative-test expectations differ per role. +**Current results.** `conformance/scripts/run-all.sh` runs 22 test +configurations, and the daily `conformance.yml` run executes all of +them. As of the 2026-09-27 run, all 14 AS configurations (baseline, +message-signing, mtls, message-signing-mtls, client-auth-mtls, +client-auth-mtls-and-mtls, four CIBA poll/ping variants, and four +client-credentials variants) pass with 0 failures and 0 warnings; all 6 +FAPI RP configurations pass every module; the federation RP plan passes +6/10, exactly matching its list of known suite-side failures; and the +federation deployed-entity plan passes 5/5. The per-profile counts in +the history below are from each profile's first clean run; the suite +has added modules since, so the daily run's own report is the source of +truth for today's numbers. + CIBA (`server.BeginBackchannelAuthentication`/`CompleteBackchannelAuthentication`/ -`ExchangeBackchannelAuthentication`) is deliberately not part of this -automated certification loop. It implements base OIDC CIBA and -FAPI-CIBA's other requirements (a mandatory signed authentication -request with `jti`/`nbf`, poll and ping delivery, DPoP- or -mTLS-bound tokens), verified by unit/integration tests instead — but +`ExchangeBackchannelAuthentication`) was initially left out of this +automated loop. It implements base OIDC CIBA and FAPI-CIBA's other +requirements (a mandatory signed authentication request with +`jti`/`nbf`, poll and ping delivery, DPoP- or mTLS-bound tokens) — but the OIDF suite's own `fapi-ciba-id1-test-plan` requires MTLS-bound access tokens unconditionally, even under `client_auth_type=private_key_jwt` (confirmed directly from the @@ -1256,8 +1278,8 @@ fourth role: see `federation/doc.go`), CIBA ping-delivery notification **Not shared**: role-level configuration, workflow APIs, transaction types, untrusted vs. validated request types, storage interfaces where -semantics differ, generic JWT/DPoP verification methods, audit sink -wiring (each role's `Dependencies` wires its own). +semantics differ, generic JWT/DPoP verification methods, and audit +(only `server` has an audit sink). ## No public JOSE utility package diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 57caee07..983f7d45 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -57,6 +57,9 @@ decision, not something a commit message alone should trigger. a `govulncheck` finding is almost always a standard-library CVE fixed in a newer Go patch release, not something to fix in this repo's own code — bump the toolchain instead. +- A breaking change (`feat!:`/`fix!:`) adds its own section to + [UPGRADING.md](UPGRADING.md), under the version it will ship in: + who's affected, why, and what to change. - Include tests for the behavior you're changing, not just the happy path — this codebase leans on tests as part of the actual specification (see e.g. `storage/contract.go`'s reusable contract diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index 723657b2..2ff37d98 100644 --- a/GETTING_STARTED.md +++ b/GETTING_STARTED.md @@ -12,10 +12,11 @@ study it directly and treat this as the map. ## 1. Dependencies: use the reference implementations to start -`server.New` requires ten dependencies — a client repository, +`server.New` requires eleven dependencies — a client repository, transaction/grant/replay stores, a key manager, a client key source, an -access-token issuer, a revocation sink, a clock, a randomness source — -with no implicit defaults for any of them (plus an audit sink, required +access-token issuer, a revocation sink, a client-certificate trust +choice, a clock, a randomness source — with no implicit defaults for +any of them (plus an audit sink, required only under `AssuranceProduction` — see step 2's `Assurance` field). Writing real, production-shaped persistence and key management for all of that is real work, and not the place to start. Two packages exist @@ -146,8 +147,14 @@ deps := server.Dependencies{ // explicitly decline — see its doc comment for why declining must // be a conscious choice, not a silent default. Revocation: memstore.NewRevocationStore(), - Clock: server.SystemClock{}, - Random: rand.Reader, + // Whether this package re-verifies an mTLS client certificate's + // chain itself. NoClientCertificateChainTrust{} declines — right when + // no client uses mTLS, or when your TLS termination already verifies + // the chain; server.TrustedClientCAs{Roots: pool} has this package + // check it against pool instead. + ClientCertificateTrust: server.NoClientCertificateChainTrust{}, + Clock: server.SystemClock{}, + Random: rand.Reader, // crypto/rand; production assurance requires exactly this reader } srv, err := server.New(cfg, deps) @@ -310,9 +317,9 @@ the reason this section exists at all. Pick the side matching your AS: ```go // If the AS issues JWTAccessTokens: resolve its verification key(s), -// typically by fetching its published JWKS live (or, for a co-located -// deployment, an IssuerKeySource reading the key manager directly, the -// way cmd/conformance-as's own selfIssuerKeySource does). +// typically by fetching its published JWKS live (or, for a resource +// server in the AS's own process, keys.NewLocalIssuerKeys(issuer, +// keyManager), which reads the AS's key manager directly). issuerKeys, err := keys.NewJWKSIssuerKeySource(fetcher, asJWKSURL, 10*time.Minute) accessTokens, err := resource.NewJWTAccessTokens( issuerKeys, asIssuer, asIssuer.String(), // audience: matches server/accesstoken.go's own self-addressed aud claim diff --git a/README.md b/README.md index b5def602..ad897a0a 100644 --- a/README.md +++ b/README.md @@ -36,12 +36,13 @@ and RFC 9700 identify as insecure — the implicit/hybrid response types, `client_secret_basic`/`client_secret_post` authentication — aren't configuration options that happen to be off, they simply aren't implemented. `server` only ever accepts `response_type=code`, and -`ClientAuthMethod` is a closed enum of `private_key_jwt` and the mTLS -variants. +`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 - PAR (RFC 9126) · DPoP (RFC 9449) · mTLS client auth & cert-bound tokens (RFC 8705) - private_key_jwt client authentication +- OAuth 2.0 Attestation-Based Client Authentication, including HAIP 1.0 x5c attester certificate chains - JAR / JARM · RAR (RFC 9396) · CIBA (poll & ping delivery) - OpenID Federation 1.0 (trust chains, automatic client registration, trust marks) - OpenID Certified™ for OP, RP and FAPI-CIBA OP conformance profiles — see below @@ -113,7 +114,9 @@ the granted scope includes `"openid"`, and `client` populates response actually carried one — leaving `TokenSet.HasIDToken` false is a normal outcome, not an error. A deployment that only needs access tokens can drop `"openid"` from a client's `AllowedScopes` entirely and -run this library as plain OAuth 2.0 + FAPI 2.0. +run this library as plain OAuth 2.0 + FAPI 2.0 — and `Config.OAuthOnly` +(on `server` and `client` alike) makes that a checked configuration +rather than a convention. See [ARCHITECTURE.md](ARCHITECTURE.md) for the full design rationale and package layout, and [conformance/](conformance/README.md) for how each diff --git a/SECURITY.md b/SECURITY.md index b4c7aaf5..dfc7eb0f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -19,9 +19,11 @@ include vulnerability details in a public issue. ## Supported versions -FAPIgo is under active development (see the README's work-in-progress -notice) and does not yet have tagged releases. Reports against `main` -are the ones we can act on. +FAPIgo is pre-1.0 and under active development (see the README's +work-in-progress notice). Security fixes go into the next release from +`main`; earlier releases aren't patched separately, so report against +the latest release or `main`, and upgrade to the release that carries +a fix. ## What to include diff --git a/UPGRADING.md b/UPGRADING.md index e239047d..7355d0a9 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -38,6 +38,26 @@ token `cnf` claim or AS metadata document whose member names only case-fold to a known name (`"ALG"` for `"alg"`) is now rejected instead of being read as that member. No code change is needed. +### Scope errors are `invalid_scope` (server) + +**Affects:** anything matching on the error code. A pushed +authorization request or CIBA backchannel authentication request naming +a scope the client isn't allowed (or `openid` under `Config.OAuthOnly`) +now gets `invalid_scope` instead of `invalid_request`, as RFC 6749 and +CIBA Core §13 define. A missing or non-string scope is still +`invalid_request`. + +### Server error text is character-checked (client) + +**Affects:** callers reading server-supplied error text. `error`, +`error_description` and `error_uri` values outside RFC 6749 §5.2's +character set are now dropped from `Error.PublicDescription`, +`CallbackDenied.Description` and `BackchannelAuthenticationDenied.Description`, +and an authorization error redirect whose `error` code is malformed is +rejected as `invalid_response` rather than returned as `CallbackDenied`. +Well-formed responses are unaffected. `Error.ServerResponse()` is new in +this release and exposes the code, description, URI and HTTP status. + ## v0.37.0 ### `AuthorizationCallback.Session` is required (client) diff --git a/client/doc.go b/client/doc.go index 35c53b73..48881988 100644 --- a/client/doc.go +++ b/client/doc.go @@ -3,8 +3,9 @@ // flow against a FAPI-conformant authorization server. // // The package exposes workflow methods (BeginAuthorization, -// HandleAuthorizationResponse, ExchangeCode, CompleteAuthorization) rather -// than low-level JWT, PAR or DPoP primitives — those live under internal/ +// HandleAuthorizationResponse, ExchangeCode, CompleteAuthorization, and +// for CIBA BeginBackchannelAuthentication and PollBackchannelAuthentication) +// rather than low-level JWT, PAR or DPoP primitives — those live under internal/ // and are composed here behind a state machine that a caller cannot drive // out of order. In particular, only this package may construct request // objects and PAR submissions; verifying them is the server package's @@ -56,9 +57,12 @@ // server; internal/requestobject signs here, but verifies in server). // // It follows the same hardening rules as server and resource (see -// ARCHITECTURE.md, "Hardening rules for every role's public API"): -// AuthorizationSession and SessionHandle are opaque with no public -// constructor; HandleAuthorizationResponse returns a closed sum type +// ARCHITECTURE.md, "Design rules"): AuthorizationSession is opaque with +// no public constructor, and a SessionHandle can only be recovered from +// its own String form (ParseSessionHandle) — the caller stores it with +// the user agent that began the flow, and HandleAuthorizationResponse +// rejects a callback that doesn't carry the matching one; +// HandleAuthorizationResponse returns a closed sum type // rather than one struct with optional fields, so a caller can't assume // every callback carries a code; every DPoP proof, request-object // signature and client assertion is produced through Dependencies.Keys' @@ -67,7 +71,8 @@ // constructs, holds or is handed a crypto.PrivateKey, the same model // server uses for its own signing keys; TokenSet fields that carry raw // token values use fapi.Secret so they can't leak into a log line by -// accident; and a validation failure is a typed Error tagged with where -// it's safe to expose the description, not a bare error the caller has -// to string-match. +// accident; and a failure is a typed Error — with the server's own +// error response, when there was one, available through +// Error.ServerResponse — not a bare error the caller has to +// string-match. package client diff --git a/conformance/README.md b/conformance/README.md index 9cd0e126..98b9ce38 100644 --- a/conformance/README.md +++ b/conformance/README.md @@ -75,11 +75,15 @@ behaviour and negative-test expectations differ. See `server`'s CIBA support (`BeginBackchannelAuthentication`/ `CompleteBackchannelAuthentication`/`ExchangeBackchannelAuthentication`, -poll and ping delivery) is verified by unit/integration tests, not the live -OIDF suite as its primary gate: `fapi-ciba-id1-test-plan` requires -MTLS-bound access tokens unconditionally. Now that this module supports -mTLS sender-constraining (RFC 8705 §3, `-mtls`), this was genuinely -re-attempted live against `ciba-mtls.config.json` — **34/34 PASS.** +poll and ping delivery) runs against the live OIDF suite on every +conformance run, in four AS legs (poll and ping, each with +`private_key_jwt` and mTLS client authentication) plus the RP-side +"RP ciba-mtls" leg. It was first verified by unit/integration tests only, +because `fapi-ciba-id1-test-plan` requires MTLS-bound access tokens +unconditionally. Once this module supported mTLS sender-constraining +(RFC 8705 §3, `-mtls`), it was re-attempted live against +`ciba-mtls.config.json` — **34/34 PASS** on that first run; the suite +has since added a module, and the daily run passes all 35. Seven real library/harness gaps the attempt surfaced were all fixed along the way: `tls_client_certificate_bound_access_tokens` metadata, client-assertion `aud` acceptance being far too narrow, a stricter @@ -102,8 +106,7 @@ display with `invalid_binding_message`, and the display-verification check is never reached. Full breakdown, live findings and reproduction steps in [`server/oidf-config/README.md`](server/oidf-config/README.md#ciba-mtlsconfigjson--the-genuine-mtls-re-attempt-automated). -Wired into `scripts/run-all.sh` as its own "AS ciba-mtls" leg now that -it's a clean 34/34. +Wired into `scripts/run-all.sh` as its own "AS ciba-mtls" leg. CIBA §10.2 ping delivery mode got its own AS-side re-attempt too, once `server.BackchannelNotifier`/`storage.BackchannelTokenDeliveryModePing` @@ -165,9 +168,7 @@ has no resource owner at all (RFC 9396 §6's own "client's policy" framing). None of the three fields' absence is permissive — an unconfigured policy refuses any `authorization_details`, the same stance an unconfigured `Config.RAR` itself takes (see ARCHITECTURE.md's -own RAR section for the full detail, including why this is a -`server.RARPolicy` rename from an earlier client_credentials-only -`ClientCredentialsRARPolicy` type). Unlike CIBA, +own RAR section for the full detail). Unlike CIBA, the OIDF suite has no dedicated RAR conformance plan at all, so this is deliberately outside the automated live-suite loop entirely, verified instead by `extension/rar_test.go`, `server/rar_test.go` @@ -209,7 +210,10 @@ client-auth-mtls-and-mtls}` containers rather than standing up new ones — this grant has no PAR/authorize/redirect_uri/browser hop at all, so nothing about container topology needed to change, only the token endpoint's own `grant_type` dispatch. **Confirmed live: all four -clean — 15/15, 11/11, 10/10, 6/6 modules, 0 failures/0 warnings.** Also +clean, 0 failures/0 warnings** — as of the 2026-09-27 daily run, +private key+DPoP (`baseline`) 16 modules, private key+MTLS (`mtls`) 12, +MTLS+DPoP (`client-auth-mtls`) 10, and MTLS+MTLS +(`client-auth-mtls-and-mtls`) 6. Also supports Rich Authorization Requests (RFC 9396 §6) when both `Config.RAR` and `Dependencies.ClientCredentialsRARPolicy` are configured — see the [RAR](#rar) section above. `cmd/conformance-as`'s diff --git a/conformance/server/scripts/README.md b/conformance/server/scripts/README.md index 00bb7e7a..0f5e0c9c 100644 --- a/conformance/server/scripts/README.md +++ b/conformance/server/scripts/README.md @@ -357,7 +357,7 @@ the manual flow above has), so `*-plan.json` files (and updates the matching `oidf-config/*.config.json`) in one shot — see [../oidf-config/README.md](../oidf-config/README.md)'s "Quick start". `conformance/scripts/run-all.sh`, which drives this -CI-style flow for all twenty-one test configurations at once (including +CI-style flow for all twenty-two test configurations at once (including the federation leg above), expects exactly the files that command produces. diff --git a/keys/doc.go b/keys/doc.go index 1c6f6c1d..a1538d6e 100644 --- a/keys/doc.go +++ b/keys/doc.go @@ -35,6 +35,16 @@ // all satisfy crypto.Signer directly) and an arbitrary KMS/HSM // backend, with no FAPIgo-specific glue code in either case. // +// Only the caller knows how the keys behind NewKeyManagerFromSigners, +// NewDecrypter and NewSingleKeyDecrypter are held, so each takes a +// DeclareCustody option (custody.go): server and client +// production assurance require every signing KeyManager and Decrypter +// to declare durable KeyCustody through KeyCustodyAssurance. Remote +// verification keys come from JWKSIssuerKeySource (a live, fapihttp- +// hardened JWKS fetch) or LocalIssuerKeys (an authorization server's own +// KeyManager, for a verifier in the same process); production assurance +// requires such a key source to declare KeySourceAssurance. +// // The one exception to "production-suitable" above is keys/ephemeral, // an in-tree, in-memory KeyManager/Decrypter/ClientKeySource set that // always generates a fresh key rather than taking one — for local diff --git a/resource/doc.go b/resource/doc.go index eb1ca68e..cc020cf8 100644 --- a/resource/doc.go +++ b/resource/doc.go @@ -11,11 +11,11 @@ // method, target URI, access-token hash, expected nonce and JTI replay // state alongside it. Verify is the primary API and takes the full // request context needed for issuer/audience/expiry checks, DPoP proof -// validation, ath, method/URI binding, replay detection and cnf.jkt -// binding. +// validation, ath, method/URI binding, replay detection, and cnf +// binding (a DPoP key's jkt, or an mTLS certificate's x5t#S256). // // AuthorizationContext.Claims uses fapi.Secret for any raw token value it // carries, and Verify returns a typed Error tagged with what's safe to // expose in a response, matching the pattern used by client and server — -// see ARCHITECTURE.md, "Hardening rules for every role's public API". +// see ARCHITECTURE.md, "Design rules". package resource diff --git a/server/assurance.go b/server/assurance.go index 8a464ad5..46ffeda1 100644 --- a/server/assurance.go +++ b/server/assurance.go @@ -70,9 +70,6 @@ const ( // loopback http redirect URI is refused per request, at the pushed // authorization request, as invalid_request — redirect URIs belong // to client registrations, which New never sees. - // Further checks (HSM-backed keys where required, and the rest of - // the checklist ARCHITECTURE.md describes) will be added here as the - // mechanisms to check them are built. AssuranceProduction ) diff --git a/server/doc.go b/server/doc.go index dddd15b5..6ced1b1a 100644 --- a/server/doc.go +++ b/server/doc.go @@ -5,8 +5,11 @@ // // The package exposes workflow methods — PushAuthorizationRequest, // BeginAuthorization, CompleteAuthorization, ExchangeAuthorizationCode, -// RefreshAccessToken, SignUserInfoResponse, Metadata and PublicJWKS — -// that only ever consume client-generated artefacts and validate them +// RefreshAccessToken, the CIBA trio BeginBackchannelAuthentication, +// CompleteBackchannelAuthentication and ExchangeBackchannelAuthentication, +// RequestClientCredentialsToken, SignUserInfoResponse, Metadata, +// PublicJWKS and (for OpenID Federation) EntityConfiguration — that +// only ever consume client-generated artefacts and validate them // against server-held state and policy. Metadata and PublicJWKS are the // exceptions: Metadata describes the server itself rather than // processing a request, and is derived entirely from Config with no @@ -39,11 +42,12 @@ // a second BeginAuthorization with the same request_uri, or a second // CompleteAuthorization with the same handle, fails. An authorization // code is likewise single-use: a second ExchangeAuthorizationCode -// with the same code fails. A refresh token is single-use too, but -// via rotation rather than outright consumption: every successful -// RefreshAccessToken call retires the presented token and returns a -// new one, so a stolen-and-replayed old token is detectable — it -// will already be consumed by the time it's misused. +// with the same code fails. A refresh token is deliberately not +// rotated — FAPI 2.0 Security Profile Final §5.3.2.1 says an +// authorization server "shall not use refresh token rotation except +// in extraordinary circumstances" — so it stays valid for repeated +// use until it expires or is revoked, and is bound to its client +// and (under DPoP) to the key it was issued under. // - AuthorizationAction (from BeginAuthorization) and AuthorizationResult // (from CompleteAuthorization) are closed sum types, not structs with // optional fields, so a caller can never mistake a local error for a @@ -55,23 +59,26 @@ // cause is available via Unwrap for logs only. // - New fails unless every dependency (client lookup, transaction // store, grant store, replay store, client key resolution, this -// server's own signing key manager, clock, randomness) is present, -// and unless every configured limit, endpoint and algorithm is -// valid. Config.Assurance additionally requires an AuditSink under -// AssuranceProduction; Config.Profile additionally requires a JARM -// algorithm under ProfileFAPISecurityWithMessageSigning. Further -// production-only checks (store durability/atomicity capabilities, -// HSM-required keys) will be added once the mechanisms to check them -// exist. -// - Every access token this server issues is DPoP sender-constrained -// (RFC 9449) — ExchangeAuthorizationCode and RefreshAccessToken each -// require a valid DPoP proof bound to the token endpoint and reject -// a replayed proof jti the same way they reject a replayed client -// assertion or request object. A refresh token is itself bound to -// the DPoP key it was issued under: RefreshAccessToken rejects a -// proof from any other key, even one belonging to the same client. -// Bearer (non-sender-constrained) tokens and mTLS binding are not -// supported. +// server's own signing key manager, access-token issuer, revocation, +// client-certificate trust, clock, randomness) is present, and +// unless every configured limit, endpoint and algorithm is valid. +// Config.Profile additionally requires a JARM algorithm under +// ProfileFAPISecurityWithMessageSigning. AssuranceProduction +// additionally requires an AuditSink, stores and key sources that +// declare production capabilities (storage.StoreAssurance, +// keys.KeySourceAssurance), signing and decryption keys with declared +// durable custody (keys.KeyCustodyAssurance), and crypto/rand.Reader +// as Dependencies.Random — see AssuranceProduction. +// - Every access token this server issues is sender-constrained, +// either by DPoP (RFC 9449) or by the client's mTLS certificate (RFC +// 8705 §3), per the client's registered SenderConstrain. Under DPoP, +// ExchangeAuthorizationCode and RefreshAccessToken each require a +// valid DPoP proof bound to the token endpoint and reject a replayed +// proof jti the same way they reject a replayed client assertion or +// request object, and a refresh token is bound to the DPoP key it +// was issued under: RefreshAccessToken rejects a proof from any +// other key, even one belonging to the same client. Bearer +// (non-sender-constrained) tokens are not supported. // - An ID token is issued alongside the access token exactly when the // (possibly refresh-narrowed) granted scope includes "openid"; // nonce, auth_time, acr and amr come from what CompleteAuthorization diff --git a/storage/doc.go b/storage/doc.go index a8aa72b4..6ed1347e 100644 --- a/storage/doc.go +++ b/storage/doc.go @@ -17,15 +17,16 @@ // self-contained JWT access token (CreateAccessToken/LookupAccessToken // only — existence and expiry, never revocation, see that file's own // doc comment for why) — and replay.go defines a single-use ReplayStore -// keyed by a namespaced identifier (e.g. "client:jarm", "server:dpop") +// keyed by a namespaced identifier (e.g. "server:dpop", "resource:dpop") // so that different roles and subsystems can never collide on the same -// use-once token. The client's own session store will follow the same -// per-role-type pattern once the endpoint that needs it exists. No +// use-once token. session.go defines the client's own SessionStore, +// nonce.go the DPoP NonceStore, and backchannel.go the server's CIBA +// BackchannelAuthenticationStore, on the same per-role pattern. No // interface here exposes GetX/UpdateX/DeleteX-style CRUD — // every method is a named security operation (Create, Consume, Redeem, // UseOnce), and redemption-style operations verify and consume state // atomically in one call rather than as separate check-then-act steps. -// replay.Store persists only a digest and expiry per use, never a +// ReplayStore persists only a digest and expiry per use, never a // complete client assertion or DPoP proof. // // Records keep as explicit fields only what a store itself acts on — a