From b031a7e6984245a447cc935d2a44cea8d64875f6 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 25 Oct 2026 21:04:48 +0800 Subject: [PATCH 1/3] docs(federation): note that Trust Chain fetches happen before trust Resolving a Trust Chain fetches each authority hint's Entity Configuration and the Subordinate Statement from the federation_fetch_endpoint it names before any statement on the path is verified against a Trust Anchor, so the party controlling the subject chooses those URLs (host, port, path and query, with only "sub" added). On an authorization server with automatic registration this happens for any client_id an unauthenticated caller names, before client authentication. Document this on Resolver.Resolve, Dependencies.HTTP, AutomaticClientRepository, server.Dependencies.FederationHTTP and the OpenID Federation guide, and on fapihttp.Config.AllowedPrivateHosts: a host allow-listed there is reachable through this path by unauthenticated callers. Co-Authored-By: Claude Opus 5.5 --- docs/guides/openid-federation.md | 7 ++++++- fapihttp/client.go | 10 ++++++++++ federation/automatic_registration.go | 9 +++++++++ federation/resolver.go | 22 ++++++++++++++++++++++ server/dependencies.go | 7 ++++++- 5 files changed, 53 insertions(+), 2 deletions(-) diff --git a/docs/guides/openid-federation.md b/docs/guides/openid-federation.md index 2afd24a0..bbb9bdde 100644 --- a/docs/guides/openid-federation.md +++ b/docs/guides/openid-federation.md @@ -149,7 +149,12 @@ each hop, and applies: - **Bounded fetching**: `Limits.MaxPathLength` and `Limits.MaxAuthorityHints` cap one resolution at a fixed number of outbound fetches, since automatic registration resolves a `client_id` - chosen by an unauthenticated caller. + chosen by an unauthenticated caller. What isn't bounded is where they + go: a chain is fetched before it can be verified, so the + `federation_fetch_endpoint` URLs of its not-yet-trusted superiors + (host, port, path and query) are the caller's choice. The fetcher's + address checks are what limit them; a host you list in + `fapihttp.Config.AllowedPrivateHosts` is reachable by anyone this way. - **Strict JSON**: entity metadata and keys are decoded case-sensitively, so a member that differs only in case (`REDIRECT_URIS`) can't slip past a policy written for the real one. diff --git a/fapihttp/client.go b/fapihttp/client.go index 1544e5a5..25e6edf8 100644 --- a/fapihttp/client.go +++ b/fapihttp/client.go @@ -108,6 +108,16 @@ type Config struct { // wouldn't work — never for arbitrary or end-user-supplied input. // Nil/empty (the zero value) exempts nothing, matching every other // optional field here. + // + // The exemption holds for every URL the Client is given, not only + // the ones an operator configured. A Client that fetches URLs other + // parties choose — an OpenID Federation resolver following a Trust + // Chain, whose federation_fetch_endpoint URLs are fetched before the + // chain is trusted, and so, through automatic registration, for any + // client_id an unauthenticated caller names — lets them send a GET + // to any path and query on an allowed host. List a host here only if + // it is safe for anyone to reach that way, or give such fetches a + // Client without it. AllowedPrivateHosts []string } diff --git a/federation/automatic_registration.go b/federation/automatic_registration.go index 59887447..3ffbe03c 100644 --- a/federation/automatic_registration.go +++ b/federation/automatic_registration.go @@ -261,6 +261,15 @@ func newRememberedFailure(err error) error { // registered" here — an inherent limitation of that interface, not // something this type can work around. // +// A client_id is resolved before its client is authenticated — that is +// how automatic registration finds the keys to authenticate it with — +// so any caller can name one, and Resolver.Resolve then fetches the URLs +// that Entity Identifier's statements name, before any of them is +// trusted (see Resolver.Resolve and Dependencies.HTTP). The fetcher +// bounds which hosts that can reach; a host listed in its +// fapihttp.Config.AllowedPrivateHosts is reachable by unauthenticated +// callers this way. +// // The client_credentials grant and CIBA are each gated by their own // config-level switch (AutomaticRegistrationConfig.AllowsClientCredentialsGrant/ // AllowsCIBA) — off by default, since whether an automatically-resolved diff --git a/federation/resolver.go b/federation/resolver.go index 08c10629..ca9950b6 100644 --- a/federation/resolver.go +++ b/federation/resolver.go @@ -120,6 +120,16 @@ type Dependencies struct { // rebinding) can point at an internal host: and the entities a // resolver fetches are named by the Trust Chain it's resolving, so // by whoever controls the subject. + // + // Those fetches happen before anything is trusted: Resolve fetches + // each authority hint's Entity Configuration, and then the + // Subordinate Statement from the federation_fetch_endpoint that + // configuration names, before any signature on the path has been + // checked against a Trust Anchor (see Resolve). So whoever controls + // the subject chooses those URLs — host, port, path and query, with + // only "sub" added — and HTTP gets a GET to each, whatever the chain + // later turns out to be. Every host HTTP may reach, including any in + // fapihttp.Config.AllowedPrivateHosts, is reachable that way. HTTP *fapihttp.Client Clock Clock @@ -343,6 +353,18 @@ func newChainWalkState(subjectID string, leafStmt intfed.Statement, leafToken st // for VerifyTrustMark; issuing statements and serving federation // endpoints is SelfIssuer's and SubordinateIssuer's job, not a // Resolver's. See doc.go for the package's scope. +// +// Trust is established from the top down, but the chain is discovered +// from the bottom up: the subject's Entity Configuration names its +// authority hints, each hint's own Entity Configuration names its +// federation_fetch_endpoint, and the Subordinate Statement fetched from +// there names the next keys — all fetched before the Trust Anchor's key +// verifies any of them. Until then they are claims of whoever controls +// the subject, so the URLs Resolve fetches, up to Limits.MaxPathLength × +// Limits.MaxAuthorityHints superiors' worth, are that party's choice +// (see Dependencies.HTTP), and a caller that resolves an Entity +// Identifier an unauthenticated party names (as automatic registration +// does with a client_id) lets that party send them. func (r *Resolver) Resolve(ctx context.Context, subjectID string) (ResolvedEntity, error) { if subjectID == "" { return ResolvedEntity{}, fmt.Errorf("federation: subject entity ID is empty") diff --git a/server/dependencies.go b/server/dependencies.go index 2a9df97f..36f95920 100644 --- a/server/dependencies.go +++ b/server/dependencies.go @@ -185,7 +185,12 @@ type Dependencies struct { // holds under DNS rebinding. Under AssuranceProduction, New refuses a // fetcher that isn't fapihttp.Client.HardenedForProduction (one over // any other HTTPClient, a transport changed since NewClient built it, - // or a loopback exception on either). + // or a loopback exception on either). Resolution runs for a client_id + // before the client is authenticated, and fetches URLs the named + // Entity Identifier's statements choose before any is trusted (see + // federation.Resolver.Resolve), so every host this fetcher can reach, + // any in its fapihttp.Config.AllowedPrivateHosts included, can be + // sent a GET by an unauthenticated caller. FederationHTTP *fapihttp.Client // ClientCertificateTrust decides how chain-of-trust for a client From 15606f702d2b845c613ad82de811537269e9cf50 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 25 Oct 2026 21:05:18 +0800 Subject: [PATCH 2/3] fix(federation)!: keep static client keys, RP choices and empty values within policy AutomaticClientKeySource fell back to a client's federation-published keys whenever the underlying key source returned an error or no key, including for statically registered clients. A statically registered client whose client_id is also an Entity Identifier resolving under a configured Trust Anchor could therefore be authenticated with a key its federation metadata publishes, which any superior in its Trust Chain can set, and receive the static registration's scopes and grants. Keys now come from the same source as the registration: a client the underlying client repository resolves gets its keys from the underlying key source alone, a federation client from its openid_relying_party metadata alone, and an unavailable client store is an error. A superior's one_of or value policy on token_endpoint_auth_signing_alg or token_endpoint_auth_method only checked that parameter, and only when present (OpenID Federation 1.0 section 6.1.3.1.4), so an RP could leave it out and register a forbidden value through its OpenID Connect RP Metadata Choices 1.0 plural sibling, or take the OP's default algorithms. The resolver now keeps the merged metadata policy on the ResolvedEntity, and automatic registration limits the plural values and the defaults to what the policy permits for the singular parameter, refusing an RP left with none. An empty plural array, which subset_of can produce, is refused instead of being read as absent. Registering every mutually supported method stays as section 12.1.4 recommends. An empty string still counted as present to a metadata policy, so it met an essential policy, while automatic registration read it as unset for parameters other than the encryption algorithms. Automatic registration now refuses an empty string for every string parameter it reads. BREAKING CHANGE: a statically registered client is authenticated only with keys from Dependencies.ClientKeys, never with ones its federation metadata publishes, and a federation client only with its federation keys. Automatic registration no longer registers a client authentication method or assertion algorithm outside a superior's policy on token_endpoint_auth_method or token_endpoint_auth_signing_alg, refuses an empty token_endpoint_auth_methods_supported or token_endpoint_auth_signing_alg_values_supported, and refuses an empty string for any string parameter it reads. See UPGRADING.md. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 84 ++++++ federation/automatic_registration.go | 262 ++++++++++++++---- ...omatic_registration_choices_policy_test.go | 180 ++++++++++++ .../automatic_registration_internal_test.go | 2 +- federation/automatic_registration_test.go | 118 +++++++- federation/federation_fuzz_test.go | 2 +- federation/resolver.go | 21 +- internal/federation/policy.go | 42 +++ internal/federation/policy_permitted_test.go | 35 +++ ...automatic_registration_static_keys_test.go | 77 +++++ 10 files changed, 751 insertions(+), 72 deletions(-) create mode 100644 federation/automatic_registration_choices_policy_test.go create mode 100644 internal/federation/policy_permitted_test.go create mode 100644 server/automatic_registration_static_keys_test.go diff --git a/UPGRADING.md b/UPGRADING.md index acbbcb68..d9e0a6ea 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -15,6 +15,90 @@ Most production-assurance changes only affect `Config.Assurance = AssuranceProduction`. A development-assurance setup built on `memstore` and `keys/ephemeral` needs only the steps not marked *production only*. +## v0.61.0 + +### A statically registered client authenticates with its static keys only (federation, server) + +**Affects:** a server with OpenID Federation automatic registration (or +a `federation.AutomaticClientKeySource` built by hand) where a client in +`Dependencies.Clients` has a `client_id` that is also an Entity +Identifier resolving under one of `AutomaticRegistration.TrustAnchors`, +and that client signs with a key `Dependencies.ClientKeys` doesn't +return for it. Also a client `Dependencies.Clients` doesn't have, for +which `Dependencies.ClientKeys` does return keys. + +**Why:** when `Dependencies.ClientKeys` had no matching key for a client, +its keys were looked up in its federation metadata instead, even for a +statically registered client. Any superior in a Trust Chain can set that +metadata's `jwks`, so a key it published authenticated as the static +client, with the static registration's scopes and grants. + +**What fails:** a statically registered client's keys now come from +`Dependencies.ClientKeys` alone; an assertion or request object signed +with a key only its federation metadata publishes fails with +`invalid_client`. A client resolved through federation gets its keys from +its federation metadata alone, never from `Dependencies.ClientKeys`. +Whether a client is statically registered is decided as `ResolveClient` +decides it: `Dependencies.Clients` resolves it. If that store fails with +`storage.ErrStoreUnavailable`, key resolution now fails with that error +too. + +**What to change:** give each statically registered client's keys to +`Dependencies.ClientKeys`, or remove its static registration so that it +registers automatically (with automatic registration's scopes and +grants). + +### Automatic registration holds RP Metadata Choices to the policy on the singular parameter (federation) + +**Affects:** a Relying Party registered automatically under a Trust +Anchor or Intermediate whose `metadata_policy` sets `one_of` or `value` on +`token_endpoint_auth_signing_alg` or `token_endpoint_auth_method`, and +which declares algorithms or methods outside it in +`token_endpoint_auth_signing_alg_values_supported` or +`token_endpoint_auth_methods_supported`, or names no algorithm at all. +Also a Relying Party whose `token_endpoint_auth_signing_alg_values_supported` +or `token_endpoint_auth_methods_supported` is an empty array. + +**Why:** a policy operator applies only to the parameter it names, and +`one_of` only when that parameter is present (OpenID Federation 1.0 +§6.1.3.1.4). A Relying Party could leave out +`token_endpoint_auth_signing_alg` and list a forbidden algorithm in its +RP Metadata Choices sibling, or name none and get the server's defaults, +and register an algorithm or method the policy rules out. An empty array +(which a superior's `subset_of` can leave) was read as not declared, so +the defaults applied there too. + +**What fails:** algorithms and methods outside the policy on the singular +parameter are no longer registered, from either parameter or from +`AutomaticRegistrationConfig.ClientAssertionAlgorithms`. A Relying Party +left with none, or declaring an empty array, isn't registered, and its +requests fail with `invalid_client` (`OnResolutionFailure` is told why). +A Relying Party whose `token_endpoint_auth_method` or +`token_endpoint_auth_signing_alg` isn't in its own empty array is refused +too, as RP Metadata Choices requires it to be in the list. + +**What to change:** nothing on the OP. A refused Relying Party lists an +algorithm or method its superiors' policy permits. + +### Automatic registration refuses empty-string metadata (federation) + +**Affects:** a Relying Party registered automatically whose +`openid_relying_party` metadata gives `""` for a string parameter +automatic registration reads, such as `subject_type`, `client_name`, +`logo_uri` or `request_object_signing_alg`. v0.58.0 already refused it +for the ID token and UserInfo encryption parameters. + +**Why:** a metadata policy counts `""` as present, so it met an +`essential` policy (OpenID Federation 1.0 §6.1.3.1.7), while automatic +registration read it as left out and applied the OP's default instead, +such as its default `subject_type`. + +**What fails:** the Relying Party isn't registered, and its requests fail +with `invalid_client` (`OnResolutionFailure` names the parameter). + +**What to change:** nothing on the OP. A refused Relying Party leaves the +parameter out, or gives it a value. + ## v0.58.0 ### Automatic registration skips `tls_client_auth` unless chains are verified (server) diff --git a/federation/automatic_registration.go b/federation/automatic_registration.go index 3ffbe03c..c1a33219 100644 --- a/federation/automatic_registration.go +++ b/federation/automatic_registration.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/url" + "reflect" "slices" "strings" "sync" @@ -15,6 +16,7 @@ import ( fapi "github.com/idfoundry/fapigo" "github.com/idfoundry/fapigo/fapihttp" + intfed "github.com/idfoundry/fapigo/internal/federation" "github.com/idfoundry/fapigo/internal/jose" "github.com/idfoundry/fapigo/internal/mtls" "github.com/idfoundry/fapigo/internal/strictjson" @@ -139,7 +141,9 @@ type AutomaticRegistrationConfig struct { // any algorithm supported by the OP and the RP MAY be used". Typically // the server's own allowed client assertion algorithms (server.New // sets it so). Empty means an RP must name its algorithm to register - // for private_key_jwt. + // for private_key_jwt. A superior's metadata policy restricting + // token_endpoint_auth_signing_alg (one_of or value) restricts these + // defaults too, as it does token_endpoint_auth_signing_alg_values_supported. ClientAssertionAlgorithms []fapi.SignatureAlgorithm // SubjectTypes is the kinds of sub this OP gives an automatically @@ -253,13 +257,12 @@ func newRememberedFailure(err error) error { // Static registration always wins: Underlying is tried first, and its // result returned as-is when it succeeds — federation resolution is // only attempted when Underlying fails to resolve id AND id is itself -// a well-formed https Entity Identifier (ValidEntityID). Underlying's -// own interface has no way to distinguish "no such client" from a real -// backing-store failure (storage.ClientRepository's own contract), so -// a transient Underlying failure for an id that happens to look like -// an Entity Identifier is indistinguishable from "not statically -// registered" here — an inherent limitation of that interface, not -// something this type can work around. +// a well-formed https Entity Identifier (ValidEntityID). An Underlying +// failure wrapping storage.ErrStoreUnavailable is returned as it is; +// any other is read as "not statically registered". The same split +// decides where AutomaticClientKeySource takes a client's keys from, so +// a statically registered client is authenticated with its static keys +// only, never with ones its federation metadata publishes. // // A client_id is resolved before its client is authenticated — that is // how automatic registration finds the keys to authenticate it with — @@ -400,6 +403,21 @@ func (a *AutomaticClientRepository) ResolveClient(ctx context.Context, id fapi.C return entry.client, nil } +// staticallyRegistered reports whether Underlying resolves id — the +// same test ResolveClient uses to decide that a client is statically +// registered rather than resolved through federation. An unavailable +// store is an error, not a "no". +func (a *AutomaticClientRepository) staticallyRegistered(ctx context.Context, id fapi.ClientID) (bool, error) { + _, err := a.underlying.ResolveClient(ctx, id) + if err == nil { + return true, nil + } + if errors.Is(err, storage.ErrStoreUnavailable) { + return false, err + } + return false, nil +} + // resolveVerificationKeys resolves id's own client authentication keys // from its cached (or freshly resolved) openid_relying_party metadata — // the shared implementation behind AutomaticClientKeySource, factored @@ -536,7 +554,11 @@ func (a *AutomaticClientRepository) register(ctx context.Context, id fapi.Client if !ok { return cachedClient{}, fmt.Errorf("federation: %q has no %s metadata", id, relyingPartyEntityType) } - clientCfg, jwks, err := a.registeredClientConfigFromMetadata(ctx, id, raw) + limits, err := choiceLimitsFromPolicy(resolved.policy[relyingPartyEntityType]) + if err != nil { + return cachedClient{}, fmt.Errorf("federation: %q: %w", id, err) + } + clientCfg, jwks, err := a.registeredClientConfigFromMetadata(ctx, id, raw, limits) if err != nil { return cachedClient{}, fmt.Errorf("federation: %q: %w", id, err) } @@ -587,24 +609,26 @@ func (a *AutomaticClientRepository) fetchJWKS(ctx context.Context, uri string) ( // Entity Configuration's own top-level federation signing key — a // different key, for a different purpose (federation/doc.go). // -// Underlying is tried first, exactly mirroring -// AutomaticClientRepository's own "static registration always wins" -// precedence — a statically registered client's real keys must never -// be shadowed by a federation lookup. Unlike ResolveClient's own -// single-value result, a keys.ClientKeySource legitimately returns an -// empty, error-free VerificationKeySet to mean "no matching key" (e.g. -// a real client mid-rotation, or simply a kid Underlying doesn't -// recognize) — so an empty result from Underlying falls through to -// federation resolution exactly like an error would, on the -// expectation that an Underlying implementation unaware of a given -// client ID at all is at least as likely to signal that with an empty -// result as with an error (keys.ClientKeySource's own interface -// documents no requirement either way). This mirrors -// AutomaticClientRepository's own documented "Underlying's interface -// has no way to distinguish these cases" limitation — a statically -// registered client legitimately missing a requested key triggers one -// wasted federation resolution attempt (which then also fails), not a -// silent wrong answer. +// Keys come from the same source as the registration they authenticate. +// A client that repo's own underlying client repository (the one passed +// to NewAutomaticClientRepository) resolves is statically registered, +// and its keys come from Underlying (this type's key source) alone: +// whatever Underlying returns, an error or an empty set included, is +// the answer, exactly as it would be without automatic registration. +// Only a client that repository doesn't have is resolved through its +// Trust Chain, and its keys then come from its openid_relying_party +// metadata alone. A +// statically registered client whose client_id is also an Entity +// Identifier resolving under one of the configured Trust Anchors is +// therefore never authenticated with a key its federation metadata +// publishes (which any superior in its Trust Chain can set): that would +// let a federation key act as the static client, with the static +// registration's privileges. An error from that repository other than +// "not registered" — storage.ErrStoreUnavailable — is returned too, +// rather than read as "not statically registered", for the same reason +// ResolveClient does. Each call asks the client repository first, so a +// key resolution costs one more lookup there than it would without +// automatic registration. type AutomaticClientKeySource struct { underlying keys.ClientKeySource repo *AutomaticClientRepository @@ -630,10 +654,10 @@ func NewAutomaticClientKeySource(underlying keys.ClientKeySource, repo *Automati // federation-derived resolution path is always hardened (it goes // through AutomaticClientRepository's Resolver, which requires a real // fapihttp.Client), but that alone doesn't make the whole type safe to -// declare LiveFetchHardened unconditionally: Underlying is tried first -// (see this type's own doc comment), so an Underlying that isn't itself -// hardened — or doesn't declare — must not be silently papered over by -// this type's other, safe path. +// declare LiveFetchHardened unconditionally: Underlying answers for every +// statically registered client (see this type's own doc comment), so an +// Underlying that isn't itself hardened — or doesn't declare — must not +// be silently papered over by this type's other, safe path. func (s *AutomaticClientKeySource) Capabilities() keys.KeySourceCapabilities { if asserter, ok := s.underlying.(keys.KeySourceAssurance); ok { return asserter.Capabilities() @@ -643,8 +667,12 @@ func (s *AutomaticClientKeySource) Capabilities() keys.KeySourceCapabilities { // ResolveVerificationKeys implements keys.ClientKeySource. func (s *AutomaticClientKeySource) ResolveVerificationKeys(ctx context.Context, req keys.ClientKeyRequest) (keys.VerificationKeySet, error) { - if set, err := s.underlying.ResolveVerificationKeys(ctx, req); err == nil && len(set.Keys) > 0 { - return set, nil + static, err := s.repo.staticallyRegistered(ctx, req.ClientID) + if err != nil { + return keys.VerificationKeySet{}, err + } + if static { + return s.underlying.ResolveVerificationKeys(ctx, req) } return s.repo.resolveVerificationKeys(ctx, req) } @@ -721,16 +749,24 @@ type relyingPartyMetadata struct { // can even validate the resulting config, so there is no later point at // which deferring the fetch would still work for every auth method. // +// limits holds the choices taken from token_endpoint_auth_methods_supported +// and token_endpoint_auth_signing_alg_values_supported to what the Trust +// Chain's metadata policy permits for their singular parameters (see +// choiceLimits). +// // a.cfg supplies the operator-level capability grants (AllowedScopes, // AllowsClientCredentialsGrant, AllowsCIBA) that must never be inferred // from raw itself, plus the AllowedClientAuthMethods restriction raw's // own token_endpoint_auth_method is checked against — see AutomaticRegistrationConfig's own doc comments // for why. -func (a *AutomaticClientRepository) registeredClientConfigFromMetadata(ctx context.Context, id fapi.ClientID, raw json.RawMessage) (storage.RegisteredClientConfig, json.RawMessage, error) { +func (a *AutomaticClientRepository) registeredClientConfigFromMetadata(ctx context.Context, id fapi.ClientID, raw json.RawMessage, limits choiceLimits) (storage.RegisteredClientConfig, json.RawMessage, error) { var m relyingPartyMetadata if err := strictjson.Unmarshal(raw, &m); err != nil { return storage.RegisteredClientConfig{}, nil, fmt.Errorf("parse %s metadata: %w", relyingPartyEntityType, err) } + if err := refuseEmptyStrings(raw); err != nil { + return storage.RegisteredClientConfig{}, nil, err + } if len(m.RedirectURIs) == 0 { return storage.RegisteredClientConfig{}, nil, fmt.Errorf("%s metadata has no redirect_uris", relyingPartyEntityType) } @@ -738,7 +774,7 @@ func (a *AutomaticClientRepository) registeredClientConfigFromMetadata(ctx conte return storage.RegisteredClientConfig{}, nil, fmt.Errorf("%s metadata must declare exactly one of jwks or jwks_uri", relyingPartyEntityType) } - declared, err := a.declaredClientAuthMethods(m) + declared, err := a.declaredClientAuthMethods(m, limits.authMethods) if err != nil { return storage.RegisteredClientConfig{}, nil, err } @@ -787,7 +823,7 @@ func (a *AutomaticClientRepository) registeredClientConfigFromMetadata(ctx conte // client's own published certificate (via jwks/jwks_uri's "x5c") for // self_signed_tls_client_auth, and a plain metadata string for // tls_client_auth and its four SAN-typed siblings. - if err := a.applyClientAuthMethods(&cfg, declared, m, jwks); err != nil { + if err := a.applyClientAuthMethods(&cfg, declared, m, jwks, limits.signingAlgs); err != nil { return storage.RegisteredClientConfig{}, nil, err } @@ -845,10 +881,10 @@ func clientDisplayFromMetadata(m relyingPartyMetadata) (storage.ClientDisplay, e // applyClientAuthMethodFields sets cfg's authMethod-specific field from // m/jwks — split out of registeredClientConfigFromMetadata purely to // keep that method's own cognitive complexity manageable. -func applyClientAuthMethodFields(cfg *storage.RegisteredClientConfig, authMethod storage.ClientAuthMethod, m relyingPartyMetadata, jwks json.RawMessage, defaultAlgs []fapi.SignatureAlgorithm) error { +func applyClientAuthMethodFields(cfg *storage.RegisteredClientConfig, authMethod storage.ClientAuthMethod, m relyingPartyMetadata, jwks json.RawMessage, defaultAlgs []fapi.SignatureAlgorithm, permittedAlgs *permittedValues) error { switch authMethod { case storage.ClientAuthMethodPrivateKeyJWT: - algs, err := assertionAlgorithms(m, defaultAlgs) + algs, err := assertionAlgorithms(m, defaultAlgs, permittedAlgs) if err != nil { return err } @@ -912,13 +948,23 @@ type declaredMethod struct { // package infers no other. Listed methods it doesn't implement are // skipped; a token_endpoint_auth_method it doesn't implement, or one // AllowedClientAuthMethods refuses, fails the registration. -func (a *AutomaticClientRepository) declaredClientAuthMethods(m relyingPartyMetadata) ([]declaredMethod, error) { +// +// Every method that remains is registered, not only the +// token_endpoint_auth_method: §12.1.4 has the OP accept "any client +// authentication method that is mutually supported", since automatic +// registration never tells it which one the RP will use. permitted, when +// set, is what the metadata policy allows token_endpoint_auth_method to +// be, and listed methods outside it are skipped as well: the policy +// names the singular parameter only, and a superior restricting how its +// subordinates authenticate means the methods they register, whichever +// parameter declares them. +func (a *AutomaticClientRepository) declaredClientAuthMethods(m relyingPartyMetadata, permitted *permittedValues) ([]declaredMethod, error) { single := m.TokenEndpointAuthMethod list := m.TokenEndpointAuthMethodsSupported if single == "" && len(list) == 0 { return nil, fmt.Errorf("%s metadata declares no client authentication method (token_endpoint_auth_method or token_endpoint_auth_methods_supported)", relyingPartyEntityType) } - if single != "" && len(list) > 0 && !slices.Contains(list, single) { + if single != "" && list != nil && !slices.Contains(list, single) { return nil, fmt.Errorf("token_endpoint_auth_method %q is not among token_endpoint_auth_methods_supported", single) } var declared []declaredMethod @@ -934,13 +980,13 @@ func (a *AutomaticClientRepository) declaredClientAuthMethods(m relyingPartyMeta } for _, name := range list { method, err := storage.ParseClientAuthMethod(name) - if err != nil || !a.allowsClientAuthMethod(method) || slices.ContainsFunc(declared, func(d declaredMethod) bool { return d.method == method }) { + if err != nil || !a.allowsClientAuthMethod(method) || !permitted.allows(name) || slices.ContainsFunc(declared, func(d declaredMethod) bool { return d.method == method }) { continue } declared = append(declared, declaredMethod{method: method}) } if len(declared) == 0 { - return nil, fmt.Errorf("none of token_endpoint_auth_methods_supported %v is supported and permitted by allowed_client_auth_methods", list) + return nil, fmt.Errorf("none of token_endpoint_auth_methods_supported %v is supported and permitted by allowed_client_auth_methods and the metadata policy", list) } return declared, nil } @@ -951,10 +997,10 @@ func (a *AutomaticClientRepository) declaredClientAuthMethods(m relyingPartyMeta // tls_client_auth_subject_dn) is dropped; the explicitly requested // token_endpoint_auth_method must be usable. At least one method must // remain. -func (a *AutomaticClientRepository) applyClientAuthMethods(cfg *storage.RegisteredClientConfig, declared []declaredMethod, m relyingPartyMetadata, jwks json.RawMessage) error { +func (a *AutomaticClientRepository) applyClientAuthMethods(cfg *storage.RegisteredClientConfig, declared []declaredMethod, m relyingPartyMetadata, jwks json.RawMessage, permittedAlgs *permittedValues) error { var lastErr error for _, d := range declared { - if err := applyClientAuthMethodFields(cfg, d.method, m, jwks, a.cfg.ClientAssertionAlgorithms); err != nil { + if err := applyClientAuthMethodFields(cfg, d.method, m, jwks, a.cfg.ClientAssertionAlgorithms, permittedAlgs); err != nil { if d.explicit { return err } @@ -976,19 +1022,34 @@ func (a *AutomaticClientRepository) applyClientAuthMethods(cfg *storage.Register // token_endpoint_auth_signing_alg_values_supported (skipping algorithms // this package doesn't implement), else defaultAlgs — "any algorithm // supported by the OP and the RP". -func assertionAlgorithms(m relyingPartyMetadata, defaultAlgs []fapi.SignatureAlgorithm) ([]fapi.SignatureAlgorithm, error) { +// +// permitted, when set, is what the metadata policy allows +// token_endpoint_auth_signing_alg to be. The policy already holds a +// declared token_endpoint_auth_signing_alg to it, but not the other two +// sources, so algorithms outside it are left out of those, and an RP left +// with none is refused (RP Metadata Choices 1.0 §4: use a choice that is +// supported, or reject the registration when none is). A superior's +// one_of on token_endpoint_auth_signing_alg then means the algorithms +// the RP registers, however it declares them. +func assertionAlgorithms(m relyingPartyMetadata, defaultAlgs []fapi.SignatureAlgorithm, permitted *permittedValues) ([]fapi.SignatureAlgorithm, error) { single := m.TokenEndpointAuthSigningAlg list := m.TokenEndpointAuthSigningAlgValuesSupported if single != "" { return declaredAssertionAlgorithm(single, list) } - if len(list) > 0 { - return supportedAssertionAlgorithms(list) + if list != nil { + return supportedAssertionAlgorithms(list, permitted) + } + var algs []fapi.SignatureAlgorithm + for _, alg := range defaultAlgs { + if permitted.allows(alg.String()) { + algs = append(algs, alg) + } } - if len(defaultAlgs) == 0 { - return nil, fmt.Errorf("token_endpoint_auth_signing_alg is required for private_key_jwt: no default algorithms are configured") + if len(algs) == 0 { + return nil, fmt.Errorf("token_endpoint_auth_signing_alg is required for private_key_jwt: no default algorithm is configured that the metadata policy permits") } - return slices.Clone(defaultAlgs), nil + return algs, nil } // declaredAssertionAlgorithm is assertionAlgorithms' answer when the RP @@ -996,7 +1057,7 @@ func assertionAlgorithms(m relyingPartyMetadata, defaultAlgs []fapi.SignatureAlg // must be among list, its declared supported algorithms, when it // declared any. func declaredAssertionAlgorithm(single string, list []string) ([]fapi.SignatureAlgorithm, error) { - if len(list) > 0 && !slices.Contains(list, single) { + if list != nil && !slices.Contains(list, single) { return nil, fmt.Errorf("token_endpoint_auth_signing_alg %q is not among token_endpoint_auth_signing_alg_values_supported", single) } alg, err := fapi.ParseSignatureAlgorithm(single) @@ -1007,20 +1068,113 @@ func declaredAssertionAlgorithm(single string, list []string) ([]fapi.SignatureA } // supportedAssertionAlgorithms is list's algorithms this package -// implements, once each, in list's order. -func supportedAssertionAlgorithms(list []string) ([]fapi.SignatureAlgorithm, error) { +// implements and permitted allows, once each, in list's order. An empty +// list (which a superior's subset_of can leave) declares no algorithm, +// so it is refused like one with none usable, never read as absent. +func supportedAssertionAlgorithms(list []string, permitted *permittedValues) ([]fapi.SignatureAlgorithm, error) { var algs []fapi.SignatureAlgorithm for _, name := range list { + if !permitted.allows(name) { + continue + } if alg, err := fapi.ParseSignatureAlgorithm(name); err == nil && !slices.Contains(algs, alg) { algs = append(algs, alg) } } if len(algs) == 0 { - return nil, fmt.Errorf("none of token_endpoint_auth_signing_alg_values_supported %v is supported", list) + return nil, fmt.Errorf("none of token_endpoint_auth_signing_alg_values_supported %v is supported and permitted by the metadata policy", list) } return algs, nil } +// choiceLimits is what a Trust Chain's metadata policy permits for the +// singular parameters automatic registration also reads an OpenID +// Connect RP Metadata Choices 1.0 §2 plural sibling of. Policy +// operators act on the parameter they name only (OpenID Federation 1.0 +// §6.1.3.1), and one_of checks a value only "if the metadata parameter +// is present" (§6.1.3.1.4); so an RP leaving out +// token_endpoint_auth_signing_alg under a one_of on it, and declaring +// token_endpoint_auth_signing_alg_values_supported instead, would +// otherwise register a value the policy forbids. Each nil field means +// no policy restricts that parameter. +type choiceLimits struct { + authMethods *permittedValues // token_endpoint_auth_method + signingAlgs *permittedValues // token_endpoint_auth_signing_alg +} + +// permittedValues is the set of values a metadata policy allows a +// singular parameter; a nil *permittedValues allows any value. +type permittedValues struct{ values []string } + +// allows reports whether p permits v. +func (p *permittedValues) allows(v string) bool { + return p == nil || slices.Contains(p.values, v) +} + +// choiceLimitsFromPolicy reads choiceLimits from policy, the merged +// openid_relying_party metadata policy (nil when none applies). +func choiceLimitsFromPolicy(policy map[string]intfed.PolicyOperators) (choiceLimits, error) { + var limits choiceLimits + for _, field := range []struct { + param string + dst **permittedValues + }{ + {"token_endpoint_auth_method", &limits.authMethods}, + {"token_endpoint_auth_signing_alg", &limits.signingAlgs}, + } { + permitted, constrained, err := intfed.PermittedStrings(policy[field.param]) + if err != nil { + return choiceLimits{}, fmt.Errorf("metadata policy for %s: %w", field.param, err) + } + if constrained { + *field.dst = &permittedValues{values: permitted} + } + } + return limits, nil +} + +// refuseEmptyStrings refuses raw, an openid_relying_party metadata +// object, when a string parameter this package reads from it is the +// empty string. None of them has "" among its values, and each would +// otherwise read as left out while counting as present to a metadata +// policy (OpenID Federation 1.0 §6.1.3.1.7: essential requires only that +// the parameter "MUST be present"), so a superior's essential policy +// would be met by a value that registers the OP's default instead. +func refuseEmptyStrings(raw json.RawMessage) error { + var members map[string]json.RawMessage + if err := json.Unmarshal(raw, &members); err != nil { + return fmt.Errorf("parse %s metadata: %w", relyingPartyEntityType, err) + } + for _, name := range relyingPartyStringParameters { + // Unmarshalling null into a string leaves it "" too, so only a + // JSON string counts: null states nothing, and reads as left out. + var value string + if v, ok := members[name]; ok && len(v) > 0 && v[0] == '"' && json.Unmarshal(v, &value) == nil && value == "" { + return fmt.Errorf("%s must not be empty", name) + } + } + return nil +} + +// relyingPartyStringParameters is the name of every string parameter +// relyingPartyMetadata reads, from its json tags, so a parameter added +// there is covered by refuseEmptyStrings without being listed twice. +var relyingPartyStringParameters = func() []string { + var names []string + t := reflect.TypeFor[relyingPartyMetadata]() + for i := range t.NumField() { + f := t.Field(i) + kind := f.Type.Kind() + if kind == reflect.Pointer { + kind = f.Type.Elem().Kind() + } + if kind == reflect.String { + names = append(names, strings.Split(f.Tag.Get("json"), ",")[0]) + } + } + return names +}() + // allowsClientAuthMethod reports whether method is permitted by // AutomaticRegistrationConfig.AllowedClientAuthMethods — every method // when that list is empty. diff --git a/federation/automatic_registration_choices_policy_test.go b/federation/automatic_registration_choices_policy_test.go new file mode 100644 index 00000000..1f3f9945 --- /dev/null +++ b/federation/automatic_registration_choices_policy_test.go @@ -0,0 +1,180 @@ +package federation_test + +import ( + "context" + "slices" + "strings" + "testing" + + fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/federation" + "github.com/idfoundry/fapigo/storage" +) + +// resolveUnderPolicy registers the fixture RP publishing members (plus +// its jwks) under policy, a Trust Anchor's openid_relying_party +// metadata_policy, with ES256 and PS256 as the OP's default assertion +// algorithms. +func resolveUnderPolicy(t *testing.T, policy string, members map[string]any) (storage.RegisteredClient, error) { + t.Helper() + f := setupAutomaticRegistrationFixtureWithPolicy(t, rpMetadataMembers(t, members), + mustPolicy(t, `{"openid_relying_party":`+policy+`}`)) + cfg := validAutomaticRegistrationConfig() + cfg.ClientAssertionAlgorithms = []fapi.SignatureAlgorithm{fapi.ES256, fapi.PS256} + repo, err := federation.NewAutomaticClientRepository(alwaysFailsRepository{}, f.newResolver(t), f.fetcher, cfg, fixedClock{now: f.now}) + if err != nil { + t.Fatalf("NewAutomaticClientRepository: %v", err) + } + return repo.ResolveClient(context.Background(), fapi.ClientID(f.rpID)) +} + +// TestAutomaticRegistrationHoldsAlgorithmChoicesToPolicy: a superior's +// one_of (or value) on token_endpoint_auth_signing_alg bounds the +// algorithms the RP registers however it declares them — with the +// singular parameter, its RP Metadata Choices plural sibling, or neither +// (the OP's defaults) — since the policy itself checks only the singular +// parameter, and only when it is present. +func TestAutomaticRegistrationHoldsAlgorithmChoicesToPolicy(t *testing.T) { + base := map[string]any{"redirect_uris": []string{"https://rp.example.org/cb"}, "token_endpoint_auth_method": "private_key_jwt"} + with := func(extra map[string]any) map[string]any { + m := map[string]any{} + for k, v := range base { + m[k] = v + } + for k, v := range extra { + m[k] = v + } + return m + } + oneOfPS256 := `{"token_endpoint_auth_signing_alg":{"one_of":["PS256"]}}` + for _, tc := range []struct { + name string + policy string + members map[string]any + want []fapi.SignatureAlgorithm // nil: refused + }{ + {"plural outside one_of", oneOfPS256, with(map[string]any{"token_endpoint_auth_signing_alg_values_supported": []string{"ES256"}}), nil}, + {"plural narrowed to one_of", oneOfPS256, with(map[string]any{"token_endpoint_auth_signing_alg_values_supported": []string{"ES256", "PS256"}}), []fapi.SignatureAlgorithm{fapi.PS256}}, + {"defaults narrowed to one_of", oneOfPS256, base, []fapi.SignatureAlgorithm{fapi.PS256}}, + {"singular within one_of", oneOfPS256, with(map[string]any{"token_endpoint_auth_signing_alg": "PS256"}), []fapi.SignatureAlgorithm{fapi.PS256}}, + {"plural outside value", `{"token_endpoint_auth_signing_alg":{"value":"PS256"}}`, with(map[string]any{"token_endpoint_auth_signing_alg_values_supported": []string{"ES256"}}), nil}, + {"defaults outside one_of", `{"token_endpoint_auth_signing_alg":{"one_of":["RS256"]}}`, base, nil}, + // subset_of on the plural can leave it empty: that declares no + // algorithm, never "none declared, use the defaults". + {"plural emptied by subset_of", `{"token_endpoint_auth_signing_alg_values_supported":{"subset_of":["RS256"]}}`, with(map[string]any{"token_endpoint_auth_signing_alg_values_supported": []string{"ES256"}}), nil}, + {"no policy", `{}`, with(map[string]any{"token_endpoint_auth_signing_alg_values_supported": []string{"ES256"}}), []fapi.SignatureAlgorithm{fapi.ES256}}, + } { + t.Run(tc.name, func(t *testing.T) { + client, err := resolveUnderPolicy(t, tc.policy, tc.members) + if tc.want == nil { + if err == nil { + t.Fatalf("ResolveClient registered %v, want the registration refused", client.ClientAssertionAlgorithms()) + } + return + } + if err != nil { + t.Fatalf("ResolveClient: %v", err) + } + if got := client.ClientAssertionAlgorithms(); !slices.Equal(got, tc.want) { + t.Errorf("ClientAssertionAlgorithms() = %v, want %v", got, tc.want) + } + }) + } +} + +// TestAutomaticRegistrationHoldsMethodChoicesToPolicy: likewise for +// token_endpoint_auth_method and token_endpoint_auth_methods_supported. +func TestAutomaticRegistrationHoldsMethodChoicesToPolicy(t *testing.T) { + members := func(extra map[string]any) map[string]any { + m := map[string]any{ + "redirect_uris": []string{"https://rp.example.org/cb"}, + "token_endpoint_auth_signing_alg": "ES256", + "tls_client_auth_subject_dn": "CN=rp.example.org", + } + for k, v := range extra { + m[k] = v + } + return m + } + both := []string{"tls_client_auth", "private_key_jwt"} + for _, tc := range []struct { + name string + policy string + members map[string]any + want []storage.ClientAuthMethod // nil: refused + }{ + {"plural narrowed to one_of", `{"token_endpoint_auth_method":{"one_of":["private_key_jwt"]}}`, + members(map[string]any{"token_endpoint_auth_methods_supported": both}), + []storage.ClientAuthMethod{storage.ClientAuthMethodPrivateKeyJWT}}, + {"plural outside one_of", `{"token_endpoint_auth_method":{"one_of":["private_key_jwt"]}}`, + members(map[string]any{"token_endpoint_auth_methods_supported": []string{"tls_client_auth"}}), nil}, + {"plural narrowed to value", `{"token_endpoint_auth_method":{"value":"private_key_jwt"}}`, + members(map[string]any{"token_endpoint_auth_methods_supported": both}), + []storage.ClientAuthMethod{storage.ClientAuthMethodPrivateKeyJWT}}, + // §12.1.4: with no policy on the method, every mutually supported + // one is registered. + {"no policy", `{}`, + members(map[string]any{"token_endpoint_auth_methods_supported": both}), + []storage.ClientAuthMethod{storage.ClientAuthMethodTLSClientAuth, storage.ClientAuthMethodPrivateKeyJWT}}, + } { + t.Run(tc.name, func(t *testing.T) { + client, err := resolveUnderPolicy(t, tc.policy, tc.members) + if tc.want == nil { + if err == nil { + t.Fatalf("ResolveClient registered %v, want the registration refused", client.ClientAuthMethods()) + } + return + } + if err != nil { + t.Fatalf("ResolveClient: %v", err) + } + if got := client.ClientAuthMethods(); !slices.Equal(got, tc.want) { + t.Errorf("ClientAuthMethods() = %v, want %v", got, tc.want) + } + }) + } +} + +// TestAutomaticRegistrationRefusesEmptyStringParameters: an empty +// string is present to a metadata policy, so it meets essential, but +// would read as left out; every string parameter registration reads +// refuses it. null, which a policy treats as absent, still reads as +// left out. +func TestAutomaticRegistrationRefusesEmptyStringParameters(t *testing.T) { + base := map[string]any{ + "redirect_uris": []string{"https://rp.example.org/cb"}, + "token_endpoint_auth_method": "private_key_jwt", + "token_endpoint_auth_signing_alg": "ES256", + } + with := func(name string, value any) map[string]any { + m := map[string]any{name: value} + for k, v := range base { + if k != name { + m[k] = v + } + } + return m + } + for _, name := range []string{"subject_type", "client_name", "logo_uri", "request_object_signing_alg", "token_endpoint_auth_signing_alg", "backchannel_token_delivery_mode"} { + t.Run(name, func(t *testing.T) { + _, err := resolveUnderPolicy(t, `{"`+name+`":{"essential":true}}`, with(name, "")) + if err == nil || !strings.Contains(err.Error(), name+" must not be empty") { + t.Errorf("ResolveClient(%s \"\") = %v, want an empty-value error", name, err) + } + }) + } + t.Run("null subject_type", func(t *testing.T) { + client, err := resolveUnderPolicy(t, `{}`, with("subject_type", nil)) + if err != nil { + t.Fatalf("ResolveClient: %v", err) + } + if client.SubjectType() != fapi.SubjectTypePublic { + t.Errorf("SubjectType() = %v, want the default", client.SubjectType()) + } + }) + t.Run("null subject_type under essential", func(t *testing.T) { + if _, err := resolveUnderPolicy(t, `{"subject_type":{"essential":true}}`, with("subject_type", nil)); err == nil { + t.Error("ResolveClient = nil error, want the essential policy unmet") + } + }) +} diff --git a/federation/automatic_registration_internal_test.go b/federation/automatic_registration_internal_test.go index 17134ad7..7fe718cb 100644 --- a/federation/automatic_registration_internal_test.go +++ b/federation/automatic_registration_internal_test.go @@ -53,7 +53,7 @@ func newTestAutomaticClientRepositoryWithFetcher(t *testing.T, automaticCfg Auto func call(t *testing.T, repo *AutomaticClientRepository, raw string) (storage.RegisteredClientConfig, json.RawMessage, error) { t.Helper() - return repo.registeredClientConfigFromMetadata(context.Background(), "https://rp.example.org", json.RawMessage(raw)) + return repo.registeredClientConfigFromMetadata(context.Background(), "https://rp.example.org", json.RawMessage(raw), choiceLimits{}) } // testSelfSignedCert generates a fresh, real self-signed X.509 diff --git a/federation/automatic_registration_test.go b/federation/automatic_registration_test.go index d89ca305..6ebbb88e 100644 --- a/federation/automatic_registration_test.go +++ b/federation/automatic_registration_test.go @@ -664,37 +664,133 @@ func TestAutomaticClientRepositoryResolveClientFailsOnJWKSURIFetchError(t *testi } } -func TestAutomaticClientKeySourcePrefersUnderlying(t *testing.T) { +// TestAutomaticClientKeySourceStaticClientUsesUnderlyingOnly: a client +// the underlying repository registers statically gets its keys from the +// underlying key source alone, even when its client_id is also an Entity +// Identifier resolving under a configured Trust Anchor. An empty set or +// an error from the underlying source is the answer, not a cue to try +// the key its federation metadata publishes (rp-oidc), which a superior +// in its Trust Chain can set; and federation isn't consulted at all. +func TestAutomaticClientKeySourceStaticClientUsesUnderlyingOnly(t *testing.T) { + staticKey := generateKey(t) + staticKeys := keys.VerificationKeySet{Keys: []keys.VerificationKey{ + {KeyID: "static-kid", Algorithm: fapi.ES256, PublicKey: &staticKey.PublicKey}, + }} + for _, tc := range []struct { + name string + underlying keys.ClientKeySource + kid string + wantKID string + wantErr bool + }{ + {name: "static key", kid: "static-kid", wantKID: "static-kid"}, + {name: "any key", wantKID: "static-kid"}, + {name: "federation-published key", kid: "rp-oidc"}, + {name: "underlying error", underlying: alwaysFailsKeySource{}, kid: "rp-oidc", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + f := setupAutomaticRegistrationFixture(t, rpMetadataBuilder(t)) + staticClient, err := storage.NewRegisteredClient(storage.RegisteredClientConfig{ + ID: fapi.ClientID(f.rpID), RedirectURIs: []fapi.RegisteredRedirectURI{fapi.RegisteredRedirectURI(f.rpID + "/static-cb")}, + ClientAuthMethod: storage.ClientAuthMethodPrivateKeyJWT, ClientAssertionAlgorithm: fapi.ES256, + SenderConstrain: storage.SenderConstrainDPoP, AllowedScopes: []string{"openid"}, + }) + if err != nil { + t.Fatalf("storage.NewRegisteredClient: %v", err) + } + repo, err := federation.NewAutomaticClientRepository( + staticRepository{id: fapi.ClientID(f.rpID), client: staticClient}, f.newResolver(t), f.fetcher, + validAutomaticRegistrationConfig(), fixedClock{now: f.now}) + if err != nil { + t.Fatalf("NewAutomaticClientRepository: %v", err) + } + underlying := tc.underlying + if underlying == nil { + underlying = staticKeySource{id: fapi.ClientID(f.rpID), keySet: staticKeys} + } + src, err := federation.NewAutomaticClientKeySource(underlying, repo) + if err != nil { + t.Fatalf("NewAutomaticClientKeySource: %v", err) + } + + set, err := src.ResolveVerificationKeys(context.Background(), keys.ClientKeyRequest{ + ClientID: fapi.ClientID(f.rpID), Purpose: keys.ClientAssertionVerification, Algorithm: fapi.ES256, KeyID: tc.kid, + }) + if (err != nil) != tc.wantErr { + t.Fatalf("ResolveVerificationKeys error = %v, want error %v", err, tc.wantErr) + } + // staticKeySource returns its whole set; what matters is that no + // federation-published key is ever in it. + for _, k := range set.Keys { + if k.KeyID != "static-kid" { + t.Errorf("ResolveVerificationKeys returned key %q, want only the static client's own keys", k.KeyID) + } + } + if tc.wantKID != "" && (len(set.Keys) == 0 || set.Keys[0].KeyID != tc.wantKID) { + t.Errorf("ResolveVerificationKeys = %+v, want %q", set, tc.wantKID) + } + if n := atomic.LoadInt32(f.rpConfigCalls); n != 0 { + t.Errorf("RP's own well-known endpoint was fetched %d times, want 0 for a statically registered client", n) + } + }) + } +} + +// TestAutomaticClientKeySourceFederationClientUsesFederationKeysOnly: a +// client the underlying repository doesn't register is an automatically +// registered one, and its keys come from its federation metadata — not +// from the underlying key source, even one that answers for its +// client_id. +func TestAutomaticClientKeySourceFederationClientUsesFederationKeysOnly(t *testing.T) { f := setupAutomaticRegistrationFixture(t, rpMetadataBuilder(t)) - resolver := f.newResolver(t) - repo, err := federation.NewAutomaticClientRepository(alwaysFailsRepository{}, resolver, f.fetcher, validAutomaticRegistrationConfig(), fixedClock{now: f.now}) + repo, err := federation.NewAutomaticClientRepository(alwaysFailsRepository{}, f.newResolver(t), f.fetcher, validAutomaticRegistrationConfig(), fixedClock{now: f.now}) if err != nil { t.Fatalf("NewAutomaticClientRepository: %v", err) } - - underlyingKey := generateKey(t) + otherKey := generateKey(t) underlying := staticKeySource{ id: fapi.ClientID(f.rpID), keySet: keys.VerificationKeySet{Keys: []keys.VerificationKey{ - {KeyID: "static-kid", Algorithm: fapi.ES256, PublicKey: &underlyingKey.PublicKey}, + {KeyID: "static-kid", Algorithm: fapi.ES256, PublicKey: &otherKey.PublicKey}, }}, } src, err := federation.NewAutomaticClientKeySource(underlying, repo) if err != nil { t.Fatalf("NewAutomaticClientKeySource: %v", err) } - set, err := src.ResolveVerificationKeys(context.Background(), keys.ClientKeyRequest{ ClientID: fapi.ClientID(f.rpID), Purpose: keys.ClientAssertionVerification, Algorithm: fapi.ES256, }) if err != nil { t.Fatalf("ResolveVerificationKeys: %v", err) } - if len(set.Keys) != 1 || set.Keys[0].KeyID != "static-kid" { - t.Errorf("ResolveVerificationKeys = %+v, want the underlying static key, unshadowed by federation resolution", set) + if len(set.Keys) != 1 || set.Keys[0].KeyID != "rp-oidc" { + t.Errorf("ResolveVerificationKeys = %+v, want only the federation-published rp-oidc key", set) } - if atomic.LoadInt32(f.rpConfigCalls) != 0 { - t.Errorf("RP's own well-known endpoint was fetched, want underlying to have short-circuited federation resolution entirely") +} + +// TestAutomaticClientKeySourceDoesNotMaskAnUnavailableStore: when the +// client repository can't answer, whether the client is statically +// registered is unknown, so its error is returned rather than either +// source's keys. +func TestAutomaticClientKeySourceDoesNotMaskAnUnavailableStore(t *testing.T) { + f := setupAutomaticRegistrationFixture(t, rpMetadataBuilder(t)) + repo, err := federation.NewAutomaticClientRepository(unavailableRepository{}, f.newResolver(t), f.fetcher, validAutomaticRegistrationConfig(), fixedClock{now: f.now}) + if err != nil { + t.Fatalf("NewAutomaticClientRepository: %v", err) + } + src, err := federation.NewAutomaticClientKeySource(alwaysFailsKeySource{}, repo) + if err != nil { + t.Fatalf("NewAutomaticClientKeySource: %v", err) + } + _, err = src.ResolveVerificationKeys(context.Background(), keys.ClientKeyRequest{ + ClientID: fapi.ClientID(f.rpID), Purpose: keys.ClientAssertionVerification, Algorithm: fapi.ES256, + }) + if !errors.Is(err, storage.ErrStoreUnavailable) { + t.Errorf("ResolveVerificationKeys error = %v, want storage.ErrStoreUnavailable", err) + } + if n := atomic.LoadInt32(f.rpConfigCalls); n != 0 { + t.Errorf("RP's own well-known endpoint was fetched %d times, want 0", n) } } diff --git a/federation/federation_fuzz_test.go b/federation/federation_fuzz_test.go index 19b31380..06049a68 100644 --- a/federation/federation_fuzz_test.go +++ b/federation/federation_fuzz_test.go @@ -60,7 +60,7 @@ func FuzzRegisteredClientConfigFromMetadata(f *testing.F) { cfg := fuzzAutomaticRegistrationConfig(flags) repo := &AutomaticClientRepository{cfg: cfg} - got, jwks, err := repo.registeredClientConfigFromMetadata(context.Background(), id, json.RawMessage(raw)) + got, jwks, err := repo.registeredClientConfigFromMetadata(context.Background(), id, json.RawMessage(raw), choiceLimits{}) if err == nil { checkNoCaseVariantMember(t, members, names) diff --git a/federation/resolver.go b/federation/resolver.go index ca9950b6..9b6dc375 100644 --- a/federation/resolver.go +++ b/federation/resolver.go @@ -257,6 +257,14 @@ type ResolvedEntity struct { // (OpenID Federation 1.0 §8.3.2); most callers have no reason to // read this directly. Tokens []string + + // policy is the metadata policy Metadata was resolved with: every + // superior's, merged (§6.1.4.1). Automatic registration reads it to + // hold the values it takes from a parameter's RP Metadata Choices + // sibling to the policy on the parameter itself (see + // intfed.PermittedStrings). Nil for a Trust Anchor resolved as + // itself, whose metadata no policy applies to. + policy MetadataPolicy } // chainWalkState is Resolve's own mutable state as it walks upward from @@ -733,7 +741,7 @@ func (r *Resolver) finalizeTrustChain(st *chainWalkState, hop int, sup hopSuperi return ResolvedEntity{}, fmt.Errorf("federation: metadata %q's Immediate Superior sets for it: %w", st.subjectID, err) } leafMetadata := filterAllowedEntityTypes(st.subordinateConstraints, declared) - resolvedMetadata, err := r.resolveMetadata(st.subordinatePolicies, leafMetadata) + resolvedMetadata, policy, err := r.resolveMetadata(st.subordinatePolicies, leafMetadata) if err != nil { return ResolvedEntity{}, err } @@ -745,6 +753,7 @@ func (r *Resolver) finalizeTrustChain(st *chainWalkState, hop int, sup hopSuperi JWKS: st.subjectJWKS, TrustMarks: st.leafClaims.TrustMarks, TrustMarkOwners: st.leafClaims.TrustMarkOwners, TrustMarkIssuers: st.leafClaims.TrustMarkIssuers, Tokens: append(st.tokens, sup.token), + policy: policy, }, nil } @@ -881,16 +890,18 @@ func mergeCrit(a, b []string) []string { return out } -func (r *Resolver) resolveMetadata(policies []subordinatePolicy, leafMetadata map[string]json.RawMessage) (map[string]json.RawMessage, error) { +// resolveMetadata merges policies and applies the result to +// leafMetadata, returning the Resolved Metadata and the merged policy. +func (r *Resolver) resolveMetadata(policies []subordinatePolicy, leafMetadata map[string]json.RawMessage) (map[string]json.RawMessage, MetadataPolicy, error) { merged, crit, err := resolveMetadataPolicies(policies) if err != nil { - return nil, fmt.Errorf("federation: resolve metadata policy: %w", err) + return nil, nil, fmt.Errorf("federation: resolve metadata policy: %w", err) } resolved, err := intfed.ApplyPolicy(merged, crit, leafMetadata) if err != nil { - return nil, fmt.Errorf("federation: apply metadata policy: %w", err) + return nil, nil, fmt.Errorf("federation: apply metadata policy: %w", err) } - return resolved, nil + return resolved, merged, nil } // verifySelfSigned verifies stmt (expected to be entityID's own Entity diff --git a/internal/federation/policy.go b/internal/federation/policy.go index be52d6d0..e8b2a5a0 100644 --- a/internal/federation/policy.go +++ b/internal/federation/policy.go @@ -364,6 +364,48 @@ func isJSONNull(raw json.RawMessage) bool { return json.Unmarshal(raw, &v) == nil && v == nil } +// PermittedStrings is the set of string values ops — the merged policy +// for one single-valued metadata parameter — lets that parameter take: +// the value operator's own value when it sets one, else the one_of +// operator's values. constrained is false when ops has neither (value +// null removes the parameter rather than restricting it), so the +// parameter may take any value; an operand that isn't a string permits +// no string at all. ops must already have passed ValidatePolicy (as a +// policy ApplyPolicy accepted has). +// +// Policy operators act on one parameter only (§6.1.3.1), so a value +// can reach a Relying Party's registration through a parameter the +// policy never names: its OpenID Connect RP Metadata Choices 1.0 §2 +// plural sibling (token_endpoint_auth_signing_alg_values_supported for +// token_endpoint_auth_signing_alg). A caller choosing the effective +// value from that sibling uses this to keep its choice within what the +// policy permits for the singular parameter. +func PermittedStrings(ops PolicyOperators) (permitted []string, constrained bool, err error) { + if raw, ok := ops[opValue]; ok && !isJSONNull(raw) { + var v string + if json.Unmarshal(raw, &v) != nil { + return nil, true, nil + } + return []string{v}, true, nil + } + raw, ok := ops[opOneOf] + if !ok { + return nil, false, nil + } + options, err := decodeArray(raw) + if err != nil { + return nil, false, err + } + permitted = []string{} + for _, option := range options { + var v string + if json.Unmarshal(option, &v) == nil { + permitted = append(permitted, v) + } + } + return permitted, true, nil +} + // ValidatePolicy checks every metadata parameter policy in policy with // validateCombination. func ValidatePolicy(policy MetadataPolicy, crit []string) error { diff --git a/internal/federation/policy_permitted_test.go b/internal/federation/policy_permitted_test.go new file mode 100644 index 00000000..e01f1e20 --- /dev/null +++ b/internal/federation/policy_permitted_test.go @@ -0,0 +1,35 @@ +package federation + +import ( + "encoding/json" + "slices" + "testing" +) + +// TestPermittedStrings pins which policy shapes restrict a +// single-valued parameter's values, and to what. +func TestPermittedStrings(t *testing.T) { + for _, tc := range []struct { + ops string + want []string + wantConstrained bool + }{ + {`{}`, nil, false}, + {`{"default":"PS256"}`, nil, false}, + {`{"essential":true}`, nil, false}, + {`{"value":null}`, nil, false}, + {`{"value":"PS256"}`, []string{"PS256"}, true}, + {`{"value":"PS256","one_of":["PS256","ES256"]}`, []string{"PS256"}, true}, + {`{"one_of":["PS256","ES256"]}`, []string{"PS256", "ES256"}, true}, + {`{"value":1}`, nil, true}, + } { + var ops PolicyOperators + if err := json.Unmarshal([]byte(tc.ops), &ops); err != nil { + t.Fatal(err) + } + got, constrained, err := PermittedStrings(ops) + if err != nil || constrained != tc.wantConstrained || !slices.Equal(got, tc.want) { + t.Errorf("PermittedStrings(%s) = %v, %v, %v; want %v, %v", tc.ops, got, constrained, err, tc.want, tc.wantConstrained) + } + } +} diff --git a/server/automatic_registration_static_keys_test.go b/server/automatic_registration_static_keys_test.go new file mode 100644 index 00000000..6d047571 --- /dev/null +++ b/server/automatic_registration_static_keys_test.go @@ -0,0 +1,77 @@ +package server_test + +import ( + "context" + "crypto/ecdsa" + "testing" + "time" + + fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/internal/clientassertion" + "github.com/idfoundry/fapigo/keys/ephemeral" + "github.com/idfoundry/fapigo/server" + "github.com/idfoundry/fapigo/storage" +) + +// TestAutomaticRegistrationStaticClientRefusesFederationKey: a +// statically registered client whose client_id is also an Entity +// Identifier resolving under one of the server's Automatic Registration +// Trust Anchors authenticates with its static keys only. Its static +// registration grants client_credentials and a scope automatic +// registration doesn't; a client assertion signed with the key its +// federation metadata publishes (rp-oidc — a key any superior in its +// Trust Chain can set) must not authenticate as it. +func TestAutomaticRegistrationStaticClientRefusesFederationKey(t *testing.T) { + f := setupAutomaticRegistrationFixture(t, testRedirectURI) + + staticKey := generateKey(t) + staticClient, err := storage.NewRegisteredClient(storage.RegisteredClientConfig{ + ID: fapi.ClientID(f.rpID), + ClientAuthMethod: storage.ClientAuthMethodPrivateKeyJWT, + ClientAssertionAlgorithm: fapi.ES256, + AllowedScopes: []string{"accounts", "payments:admin"}, + AllowsClientCredentialsGrant: true, + }) + if err != nil { + t.Fatalf("NewRegisteredClient: %v", err) + } + staticKeys, err := ephemeral.NewClientKeySource(nil, []ephemeral.ClientKeySpec{{ClientID: fapi.ClientID(f.rpID), JWKS: fedJWKS(t, "static-kid", staticKey)}}) + if err != nil { + t.Fatalf("ephemeral.NewClientKeySource: %v", err) + } + + srv := newAutomaticRegistrationTestServer(t, f, func(cfg *server.Config, deps *server.Dependencies) { + cfg.ClientCredentialsGrant = true + cfg.AutomaticRegistration.AllowsClientCredentialsGrant = false + deps.Clients = &fakeClientRepository{clients: map[fapi.ClientID]storage.RegisteredClient{fapi.ClientID(f.rpID): staticClient}} + deps.ClientKeys = staticKeys + }) + + request := func(signer *ecdsa.PrivateKey, kid string) error { + t.Helper() + assertion, err := clientassertion.CreateAssertion(clientassertion.AssertionRequest{ + Signer: signer, Algorithm: fapi.ES256, KeyID: kid, + ClientID: f.rpID, Audience: testIssuer, Now: f.now, Lifetime: 30 * time.Second, + }) + if err != nil { + t.Fatalf("CreateAssertion: %v", err) + } + _, err = srv.RequestClientCredentialsToken(context.Background(), server.ClientCredentialsTokenRequest{ + HTTP: server.FormRequest{Parameters: []server.FormParameter{ + formParam("grant_type", "client_credentials"), + formParam("client_assertion", assertion), + formParam("client_assertion_type", clientassertion.AssertionType), + formParam("scope", "payments:admin"), + }}, + DPoPProofs: []string{createDPoPProof(t, generateKey(t), f.now)}, + }) + return err + } + + if err := request(staticKey, "static-kid"); err != nil { + t.Fatalf("assertion signed with the static key: %v, want a token", err) + } + if err := request(f.rpOIDCKey, "rp-oidc"); err == nil { + t.Fatalf("assertion signed with the federation-published key authenticated as the statically registered client, want invalid_client") + } +} From 08538dc74f7fd2724147bf01a9d502c66690221a Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 25 Oct 2026 21:09:31 +0800 Subject: [PATCH 3/3] test(federation): split the static key source and well-known reparse checks into helpers Keeps both tests' cognitive complexity within the analyzer's limit. Co-Authored-By: Claude Opus 5.5 --- federation/automatic_registration_test.go | 72 ++++++++++++++--------- federation/federation_fuzz_test.go | 26 +++++--- 2 files changed, 61 insertions(+), 37 deletions(-) diff --git a/federation/automatic_registration_test.go b/federation/automatic_registration_test.go index 6ebbb88e..68888949 100644 --- a/federation/automatic_registration_test.go +++ b/federation/automatic_registration_test.go @@ -690,28 +690,11 @@ func TestAutomaticClientKeySourceStaticClientUsesUnderlyingOnly(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { f := setupAutomaticRegistrationFixture(t, rpMetadataBuilder(t)) - staticClient, err := storage.NewRegisteredClient(storage.RegisteredClientConfig{ - ID: fapi.ClientID(f.rpID), RedirectURIs: []fapi.RegisteredRedirectURI{fapi.RegisteredRedirectURI(f.rpID + "/static-cb")}, - ClientAuthMethod: storage.ClientAuthMethodPrivateKeyJWT, ClientAssertionAlgorithm: fapi.ES256, - SenderConstrain: storage.SenderConstrainDPoP, AllowedScopes: []string{"openid"}, - }) - if err != nil { - t.Fatalf("storage.NewRegisteredClient: %v", err) - } - repo, err := federation.NewAutomaticClientRepository( - staticRepository{id: fapi.ClientID(f.rpID), client: staticClient}, f.newResolver(t), f.fetcher, - validAutomaticRegistrationConfig(), fixedClock{now: f.now}) - if err != nil { - t.Fatalf("NewAutomaticClientRepository: %v", err) - } underlying := tc.underlying if underlying == nil { underlying = staticKeySource{id: fapi.ClientID(f.rpID), keySet: staticKeys} } - src, err := federation.NewAutomaticClientKeySource(underlying, repo) - if err != nil { - t.Fatalf("NewAutomaticClientKeySource: %v", err) - } + src := newStaticClientKeySource(t, f, underlying) set, err := src.ResolveVerificationKeys(context.Background(), keys.ClientKeyRequest{ ClientID: fapi.ClientID(f.rpID), Purpose: keys.ClientAssertionVerification, Algorithm: fapi.ES256, KeyID: tc.kid, @@ -719,16 +702,7 @@ func TestAutomaticClientKeySourceStaticClientUsesUnderlyingOnly(t *testing.T) { if (err != nil) != tc.wantErr { t.Fatalf("ResolveVerificationKeys error = %v, want error %v", err, tc.wantErr) } - // staticKeySource returns its whole set; what matters is that no - // federation-published key is ever in it. - for _, k := range set.Keys { - if k.KeyID != "static-kid" { - t.Errorf("ResolveVerificationKeys returned key %q, want only the static client's own keys", k.KeyID) - } - } - if tc.wantKID != "" && (len(set.Keys) == 0 || set.Keys[0].KeyID != tc.wantKID) { - t.Errorf("ResolveVerificationKeys = %+v, want %q", set, tc.wantKID) - } + checkOnlyStaticKeys(t, set, tc.wantKID) if n := atomic.LoadInt32(f.rpConfigCalls); n != 0 { t.Errorf("RP's own well-known endpoint was fetched %d times, want 0 for a statically registered client", n) } @@ -736,6 +710,48 @@ func TestAutomaticClientKeySourceStaticClientUsesUnderlyingOnly(t *testing.T) { } } +// newStaticClientKeySource returns an AutomaticClientKeySource over +// underlying, whose client repository registers the fixture RP's +// Entity Identifier statically. +func newStaticClientKeySource(t *testing.T, f *automaticRegistrationFixture, underlying keys.ClientKeySource) *federation.AutomaticClientKeySource { + t.Helper() + staticClient, err := storage.NewRegisteredClient(storage.RegisteredClientConfig{ + ID: fapi.ClientID(f.rpID), RedirectURIs: []fapi.RegisteredRedirectURI{fapi.RegisteredRedirectURI(f.rpID + "/static-cb")}, + ClientAuthMethod: storage.ClientAuthMethodPrivateKeyJWT, ClientAssertionAlgorithm: fapi.ES256, + SenderConstrain: storage.SenderConstrainDPoP, AllowedScopes: []string{"openid"}, + }) + if err != nil { + t.Fatalf("storage.NewRegisteredClient: %v", err) + } + repo, err := federation.NewAutomaticClientRepository( + staticRepository{id: fapi.ClientID(f.rpID), client: staticClient}, f.newResolver(t), f.fetcher, + validAutomaticRegistrationConfig(), fixedClock{now: f.now}) + if err != nil { + t.Fatalf("NewAutomaticClientRepository: %v", err) + } + src, err := federation.NewAutomaticClientKeySource(underlying, repo) + if err != nil { + t.Fatalf("NewAutomaticClientKeySource: %v", err) + } + return src +} + +// checkOnlyStaticKeys fails t if set holds any key but the static +// client's own ("static-kid"), or, when wantKID is set, doesn't start +// with that key. staticKeySource returns its whole set whatever kid is +// asked for; what matters is that no federation-published key is in it. +func checkOnlyStaticKeys(t *testing.T, set keys.VerificationKeySet, wantKID string) { + t.Helper() + for _, k := range set.Keys { + if k.KeyID != "static-kid" { + t.Errorf("ResolveVerificationKeys returned key %q, want only the static client's own keys", k.KeyID) + } + } + if wantKID != "" && (len(set.Keys) == 0 || set.Keys[0].KeyID != wantKID) { + t.Errorf("ResolveVerificationKeys = %+v, want %q", set, wantKID) + } +} + // TestAutomaticClientKeySourceFederationClientUsesFederationKeysOnly: a // client the underlying repository doesn't register is an automatically // registered one, and its keys come from its federation metadata — not diff --git a/federation/federation_fuzz_test.go b/federation/federation_fuzz_test.go index 06049a68..fd308add 100644 --- a/federation/federation_fuzz_test.go +++ b/federation/federation_fuzz_test.go @@ -359,14 +359,22 @@ func FuzzEntityIDHostAgreement(f *testing.F) { if wk.Hostname() != host { t.Fatalf("%q: constraints see host %q, the fetch targets %q", id, host, wk.Hostname()) } - again, err := url.Parse(wk.String()) - if err != nil || again.Hostname() != host || again.Scheme != "https" || again.User != nil { - t.Fatalf("%q: .well-known URL %q re-parses to host %q (err %v), not %q", id, wk.String(), again.Hostname(), err, host) - } - for i := 0; i < len(host); i++ { - if host[i] >= 0x80 { - t.Fatalf("%q: non-ASCII host %q accepted", id, host) - } - } + checkWellKnownReparse(t, id, wk, host) }) } + +// checkWellKnownReparse fails t unless the serialized .well-known URL wk +// for id parses again to the same https host, with no userinfo, and that +// host is ASCII. +func checkWellKnownReparse(t *testing.T, id string, wk *url.URL, host string) { + t.Helper() + again, err := url.Parse(wk.String()) + if err != nil || again.Hostname() != host || again.Scheme != "https" || again.User != nil { + t.Fatalf("%q: .well-known URL %q re-parses to host %q (err %v), not %q", id, wk.String(), again.Hostname(), err, host) + } + for i := 0; i < len(host); i++ { + if host[i] >= 0x80 { + t.Fatalf("%q: non-ASCII host %q accepted", id, host) + } + } +}