fix(federation)!: federation fixes from the 2026-10-18 review

UPGRADING.md

@@ -140,6 +140,25 @@ following §5.3.1 item 4 refuses a longer one.
 **What to change:** set it to 60 minutes or less.
 `RecommendedLimits` uses 60 seconds.
 
+### The federation entity key must not be a protocol key (server)
+
+**Affects:** a server with `Config.Federation.EntityID` set whose
+`Dependencies.Keys` returns the same key for
+`keys.FederationEntitySigning` as for any protocol signing purpose (ID
+tokens, JARM, UserInfo, or access tokens).
+
+**Why:** the entity key signs this server's Entity Configuration, which
+vouches for its protocol keys. Reusing one of those for it lets anything
+that key signs be presented as a statement of the entity itself.
+`Config.Federation` already said the key is never reused; now `New`
+checks.
+
+**What fails:** `server.New` returns an error saying the federation
+entity key is also a protocol key.
+
+**What to change:** give `keys.FederationEntitySigning` its own key in
+your key manager.
+
 ### Attestation-based client authentication follows draft-12 (server, client)
 
 Not a breaking change: clients built to draft-07 keep working, and so

federation/automatic_registration.go

@@ -432,25 +432,30 @@ func (a *AutomaticClientRepository) resolve(ctx context.Context, id fapi.ClientI
 		a.mu.Unlock()
 		return cachedClient{}, failed.err
 	}
-	if call, ok := a.inflight[id]; ok {
-		a.mu.Unlock()
-		select {
-		case <-call.done:
-			return call.entry, call.err
-		case <-ctx.Done():
-			return cachedClient{}, ctx.Err()
-		}
+	call, ok := a.inflight[id]
+	if !ok {
+		call = &inflightResolution{done: make(chan struct{})}
+		a.inflight[id] = call
+		// The resolution is shared by every caller for id, so it runs
+		// detached from this one's cancellation: one caller giving up
+		// (or an attacker cancelling its own request on purpose) must
+		// not fail the others. It keeps ctx's values, and the fetcher's
+		// own limits bound how long it can take.
+		go func() {
+			call.entry, call.err = a.resolveUncached(context.WithoutCancel(ctx), id)
+			a.mu.Lock()
+			delete(a.inflight, id)
+			a.mu.Unlock()
+			close(call.done)
+		}()
 	}
-	call := &inflightResolution{done: make(chan struct{})}
-	a.inflight[id] = call
-	a.mu.Unlock()
-
-	call.entry, call.err = a.resolveUncached(ctx, id)
-	a.mu.Lock()
-	delete(a.inflight, id)
 	a.mu.Unlock()
-	close(call.done)
-	return call.entry, call.err
+	select {
+	case <-call.done:
+		return call.entry, call.err
+	case <-ctx.Done():
+		return cachedClient{}, ctx.Err()
+	}
 }
 
 // resolveUncached resolves id's Trust Chain and builds its registration,
@@ -654,10 +659,10 @@ type relyingPartyMetadata struct {
 	JWKS                                       json.RawMessage `json:"jwks"`
 	JWKSURI                                    string          `json:"jwks_uri"`
 	TLSClientCertificateBoundAccessTokens      bool            `json:"tls_client_certificate_bound_access_tokens"`
-	IDTokenEncryptedResponseAlg                string          `json:"id_token_encrypted_response_alg"`
-	IDTokenEncryptedResponseEnc                string          `json:"id_token_encrypted_response_enc"`
-	UserinfoEncryptedResponseAlg               string          `json:"userinfo_encrypted_response_alg"`
-	UserinfoEncryptedResponseEnc               string          `json:"userinfo_encrypted_response_enc"`
+	IDTokenEncryptedResponseAlg                *string         `json:"id_token_encrypted_response_alg"`
+	IDTokenEncryptedResponseEnc                *string         `json:"id_token_encrypted_response_enc"`
+	UserinfoEncryptedResponseAlg               *string         `json:"userinfo_encrypted_response_alg"`
+	UserinfoEncryptedResponseEnc               *string         `json:"userinfo_encrypted_response_enc"`
 
 	// BackchannelAuthenticationRequestSigningAlg/BackchannelTokenDeliveryMode/
 	// BackchannelClientNotificationEndpoint are read only when
@@ -1053,20 +1058,29 @@ func (a *AutomaticClientRepository) applyBackchannelAuthenticationFields(cfg *st
 // encLabel name the two metadata fields, for error messages. Shared by
 // registeredClientConfigFromMetadata's id_token and userinfo encryption
 // handling, which are otherwise identical.
-func parseEncryptionAlgorithmPair(algValue, encValue, algLabel, encLabel string) (fapi.KeyManagementAlgorithm, fapi.ContentEncryptionAlgorithm, error) {
-	algSet := algValue != ""
-	encSet := encValue != ""
-	if algSet != encSet {
+//
+// Each is nil when the metadata leaves it out; one that is present must
+// name an algorithm. An empty string would otherwise read as "no
+// encryption" while counting as present, so a superior's "essential"
+// policy on the parameter would be met by a value that disables it.
+func parseEncryptionAlgorithmPair(algValue, encValue *string, algLabel, encLabel string) (fapi.KeyManagementAlgorithm, fapi.ContentEncryptionAlgorithm, error) {
+	if algValue != nil && *algValue == "" {
+		return 0, 0, fmt.Errorf("%s must not be empty", algLabel)
+	}
+	if encValue != nil && *encValue == "" {
+		return 0, 0, fmt.Errorf("%s must not be empty", encLabel)
+	}
+	if (algValue != nil) != (encValue != nil) {
 		return 0, 0, fmt.Errorf("%s and %s must both be set, or neither", algLabel, encLabel)
 	}
-	if !algSet {
+	if algValue == nil {
 		return 0, 0, nil
 	}
-	alg, err := fapi.ParseKeyManagementAlgorithm(algValue)
+	alg, err := fapi.ParseKeyManagementAlgorithm(*algValue)
 	if err != nil {
 		return 0, 0, fmt.Errorf("%s: %w", algLabel, err)
 	}
-	enc, err := fapi.ParseContentEncryptionAlgorithm(encValue)
+	enc, err := fapi.ParseContentEncryptionAlgorithm(*encValue)
 	if err != nil {
 		return 0, 0, fmt.Errorf("%s: %w", encLabel, err)
 	}

federation/automatic_registration_internal_test.go

@@ -788,3 +788,34 @@ func TestResolveDropsAnExpiredRegistration(t *testing.T) {
 		t.Fatal("expired registration still cached after its lookup")
 	}
 }
+
+// TestRegisteredClientConfigFromMetadataRejectsEmptyEncryptionAlgorithms:
+// a present but empty encryption algorithm would read as "no encryption"
+// while counting as present, so a superior's essential policy on it
+// would be met by a value that turns it off. It's refused; null, which
+// states nothing, is no encryption, as an absent one is.
+func TestRegisteredClientConfigFromMetadataRejectsEmptyEncryptionAlgorithms(t *testing.T) {
+	repo := newTestAutomaticClientRepository(AutomaticRegistrationConfig{AllowedScopes: []string{"openid"}})
+	metadata := func(fields string) string {
+		return `{
+			"redirect_uris": ["https://rp.example.org/cb"],
+			"token_endpoint_auth_method": "private_key_jwt",
+			"token_endpoint_auth_signing_alg": "ES256",
+			` + fields + `
+			"jwks": ` + testRPJWKS + `
+		}`
+	}
+	for _, fields := range []string{
+		`"id_token_encrypted_response_alg": "", "id_token_encrypted_response_enc": "",`,
+		`"id_token_encrypted_response_alg": "", "id_token_encrypted_response_enc": "A256GCM",`,
+		`"userinfo_encrypted_response_alg": "ECDH-ES+A256KW", "userinfo_encrypted_response_enc": "",`,
+	} {
+		if _, _, err := call(t, repo, metadata(fields)); err == nil || !strings.Contains(err.Error(), "must not be empty") {
+			t.Errorf("registeredClientConfigFromMetadata(%s) = %v, want an empty-value error", fields, err)
+		}
+	}
+	cfg, _, err := call(t, repo, metadata(`"id_token_encrypted_response_alg": null, "id_token_encrypted_response_enc": null,`))
+	if err != nil || cfg.IDTokenEncryptionKeyManagement != 0 {
+		t.Errorf("registeredClientConfigFromMetadata(null) = %v, %v; want no encryption", cfg.IDTokenEncryptionKeyManagement, err)
+	}
+}

federation/fetch_limits_test.go

@@ -2,6 +2,7 @@ package federation_test
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -135,3 +136,60 @@ func TestAutomaticRegistrationSharesConcurrentResolution(t *testing.T) {
 		t.Fatalf("entity configuration fetched %d times, want 1", n)
 	}
 }
+
+// TestAutomaticRegistrationSharedResolutionOutlivesTheFirstCaller: the
+// first caller for a client_id starts the resolution every other caller
+// waits on. Cancelling that first request returns context.Canceled to
+// it alone; the others get the resolution's own result, so one client
+// can't fail everyone's requests naming an RP by cancelling its own.
+func TestAutomaticRegistrationSharedResolutionOutlivesTheFirstCaller(t *testing.T) {
+	started := make(chan struct{})
+	release := make(chan struct{})
+	var once sync.Once
+	ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		once.Do(func() { close(started) })
+		select {
+		case <-release:
+		case <-r.Context().Done():
+		}
+		http.NotFound(w, nil)
+	}))
+	t.Cleanup(ts.Close)
+
+	resolver, err := federation.NewResolver(federation.Config{
+		TrustAnchors: []federation.TrustAnchor{{EntityID: "https://ta.example.org", JWKS: jwksFor(t, "a", generateKey(t))}},
+		Limits:       federation.Limits{MaxPathLength: 5, MaxAuthorityHints: 5, MaxStatementLifetime: 2 * time.Hour, MaxClockSkew: 5 * time.Second},
+	}, federation.Dependencies{HTTP: fetcherFor(t, ts), Clock: federation.SystemClock{}})
+	if err != nil {
+		t.Fatalf("NewResolver: %v", err)
+	}
+	repo, err := federation.NewAutomaticClientRepository(memstore.NewClientRepository(nil), resolver, fetcherFor(t, ts),
+		federation.AutomaticRegistrationConfig{AllowedScopes: []string{"openid"}, MaxCacheAge: time.Minute}, federation.SystemClock{})
+	if err != nil {
+		t.Fatalf("NewAutomaticClientRepository: %v", err)
+	}
+
+	id := fapi.ClientID(ts.URL)
+	firstCtx, cancelFirst := context.WithCancel(context.Background())
+	first := make(chan error, 1)
+	go func() {
+		_, err := repo.ResolveClient(firstCtx, id)
+		first <- err
+	}()
+	<-started
+	second := make(chan error, 1)
+	go func() {
+		_, err := repo.ResolveClient(context.Background(), id)
+		second <- err
+	}()
+	time.Sleep(100 * time.Millisecond) // let the second caller join the in-flight resolution
+	cancelFirst()
+	if err := <-first; !errors.Is(err, context.Canceled) {
+		t.Fatalf("first ResolveClient = %v, want context.Canceled", err)
+	}
+	close(release)
+	err = <-second
+	if err == nil || errors.Is(err, context.Canceled) {
+		t.Fatalf("second ResolveClient = %v, want the fetch failure, not the first caller's cancellation", err)
+	}
+}

federation/resolve_endpoint.go

@@ -5,6 +5,7 @@ import (
 	"crypto"
 	"fmt"
 	"net/url"
+	"slices"
 
 	"github.com/idfoundry/fapigo/fapihttp"
 	intfed "github.com/idfoundry/fapigo/internal/federation"
@@ -94,6 +95,12 @@ func (r *Resolver) ResolveViaEndpoint(ctx context.Context, req ResolveRequest) (
 	if req.TrustAnchor == "" {
 		return ResolveResponseClaims{}, fmt.Errorf("federation: trust anchor is empty")
 	}
+	if !slices.ContainsFunc(r.cfg.TrustAnchors, func(ta TrustAnchor) bool { return ta.EntityID == req.TrustAnchor }) {
+		// The resolver would answer for a Trust Anchor this Resolver
+		// doesn't trust, and the caller would take the metadata as
+		// resolved under one it does.
+		return ResolveResponseClaims{}, fmt.Errorf("federation: trust anchor %q is not one of this Resolver's Config.TrustAnchors", req.TrustAnchor)
+	}
 	if err := ValidEntityID(req.ExpectedIssuer); err != nil {
 		return ResolveResponseClaims{}, fmt.Errorf("federation: expected resolver issuer: %w", err)
 	}

federation/resolve_endpoint_test.go

@@ -273,3 +273,17 @@ func TestResolveViaEndpointRefusesAnotherIssuer(t *testing.T) {
 		t.Fatalf("ResolveViaEndpoint(response from another issuer) = %v, want refusal before resolving that issuer", err)
 	}
 }
+
+// TestResolveViaEndpointRefusesAnUnconfiguredTrustAnchor: a Trust Anchor
+// this Resolver doesn't trust is refused before anything is fetched,
+// so the caller can't take a response as resolved under one it trusts.
+func TestResolveViaEndpointRefusesAnUnconfiguredTrustAnchor(t *testing.T) {
+	metadata := map[string]json.RawMessage{"openid_provider": json.RawMessage(`{}`)}
+	entityID, resolveEndpoint, resolver, _ := selfAnchoredResolveEndpoint(t, "https://op.example.org", metadata)
+	_, err := resolver.ResolveViaEndpoint(context.Background(), federation.ResolveRequest{
+		Endpoint: resolveEndpoint, Subject: "https://op.example.org", TrustAnchor: "https://other-ta.example.org", ExpectedIssuer: entityID,
+	})
+	if err == nil || !strings.Contains(err.Error(), "not one of this Resolver's Config.TrustAnchors") {
+		t.Fatalf("ResolveViaEndpoint(unconfigured trust anchor) = %v, want refused", err)
+	}
+}

federation/trustmark.go

@@ -147,10 +147,16 @@ func checkTrustMarkAccreditation(trustAnchor ResolvedEntity, claims TrustMarkCla
 
 // checkTrustMarkDelegation requires and validates a "delegation" claim
 // when trustAnchor names the mark's type in its "trust_mark_owners"
-// claim (OpenID Federation 1.0 §7.3 steps 8-9).
+// claim (OpenID Federation 1.0 §7.3 step 8), and validates one the mark
+// carries anyway (step 9): one whose type has no owner there can't be
+// validated, since the owner's keys come from that claim (§7.2.2 step
+// 9), so the mark is refused rather than its delegation ignored.
 func (r *Resolver) checkTrustMarkDelegation(trustAnchor ResolvedEntity, tm intfed.TrustMark, claims TrustMarkClaims) error {
 	owner, required := trustAnchor.TrustMarkOwners[claims.TrustMarkType]
 	if !required {
+		if tm.ClaimedDelegation() != "" {
+			return fmt.Errorf("the trust mark carries a delegation claim, but trust_mark_type %q has no owner in the trust anchor's trust_mark_owners claim to validate it against", claims.TrustMarkType)
+		}
 		return nil
 	}
 	if tm.ClaimedDelegation() == "" {

federation/trustmark_test.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -436,12 +437,13 @@ func TestVerifyTrustMarkRejectsMalformedDelegationJWT(t *testing.T) {
 	}
 }
 
-func TestVerifyTrustMarkIgnoresDelegationWhenTypeNotOwned(t *testing.T) {
-	// TA's own trust_mark_owners is nil (no type named at all), so no
-	// delegation is required and none is present — the existing
-	// TestVerifyTrustMarkSucceeds already covers this baseline; this
-	// test instead confirms a PRESENT delegation is simply not checked
-	// when the type isn't owned, even a broken one.
+// TestVerifyTrustMarkRefusesDelegationWhenTypeNotOwned: OpenID
+// Federation 1.0 §7.3 step 9 has a validator validate any delegation
+// claim a mark carries, and §7.2.2 step 9 takes the owner's keys from
+// the Trust Anchor's trust_mark_owners. A type with no owner there
+// leaves nothing to validate it against, so the mark is refused rather
+// than its delegation ignored.
+func TestVerifyTrustMarkRefusesDelegationWhenTypeNotOwned(t *testing.T) {
 	f := setupTrustMarkFederation(t, func(taID, leID string) map[string]any {
 		return map[string]any{"delegation": "not-a-jwt"}
 	}, nil)
@@ -451,8 +453,8 @@ func TestVerifyTrustMarkIgnoresDelegationWhenTypeNotOwned(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Resolve: %v", err)
 	}
-	if _, err := r.VerifyTrustMark(context.Background(), resolved, resolved.TrustMarks[0], federation.AcceptAnyFederationIssuer); err != nil {
-		t.Fatalf("VerifyTrustMark(broken delegation, type not owned, federation.AcceptAnyFederationIssuer): %v, want nil error", err)
+	if _, err := r.VerifyTrustMark(context.Background(), resolved, resolved.TrustMarks[0], federation.AcceptAnyFederationIssuer); err == nil || !strings.Contains(err.Error(), "has no owner") {
+		t.Fatalf("VerifyTrustMark(delegation, type not owned) = %v, want refused", err)
 	}
 }