fix(server)!: confine resource-less requests to AllowedResources; resources at consent

GETTING_STARTED.md

@@ -1053,8 +1053,9 @@ func (p credentialIssuerPolicy) AllowResources(ctx context.Context, client stora
 
 A resource is checked against, in order: the client's own
 `storage.RegisteredClientConfig.AllowedResources`, if it registered any
-(a client registered automatically through OpenID Federation has none,
-so it may name any resource the server accepts); then
+(a client registered automatically through OpenID Federation gets
+`AutomaticRegistrationConfig.AllowedResources`, the same for all of
+them, and without that may name any resource the server accepts); then
 `Config.Resources`; then, only for those the list doesn't name,
 `ResourcePolicy`. Anything refused is `invalid_target`. The client
 names resources at PAR — repeated `resource` form parameters, or a
@@ -1065,7 +1066,18 @@ refresh; a refresh token always keeps the grant's whole set. The access
 token's `aud` is then the resource (a JSON string), or, for several,
 an array of them. A request naming no resource still gets the issuer as
 its audience, and with neither `Config.Resources` nor `ResourcePolicy`
-set, `resource` is ignored as it always was. For a grant you serve
+set, `resource` is ignored as it always was.
+
+A client confined by `AllowedResources` never gets an issuer-audience
+token, though: every resource server accepting the issuer as an
+audience would take it, whatever the list says. If it lists exactly one
+resource, a request naming none — at PAR, in a CIBA request, for
+client credentials, or to `IssueAccessToken`/`IssueRefreshToken` — is
+for that one, recorded with the grant as if the client had named it and
+checked against `Config.Resources` and `ResourcePolicy` the same way;
+if it lists several, the request must name one, or it's
+`invalid_target`. A grant made without resources before the client was
+confined gets the same treatment at its next token request. For a grant you serve
 yourself (`Config.AdditionalGrantTypes`), pass
 `TokenEndpointRequest.Resources()` as `IssueAccessTokenRequest.Resources`
 and `IssueRefreshTokenRequest.Resources`, and check the resources your
@@ -1112,10 +1124,14 @@ present it to another. Every FAPIgo access token is sender-constrained
 that doesn't want its resource servers to share a token narrows each
 token request to the one resource it's for.
 
-The consent screen isn't told which resources a request names
-(`InteractionRequest` doesn't carry them), so it can neither show them
-to the user nor narrow them: they're the client's to choose and the
-server's to allow.
+The consent screen is told which resources the tokens would be for —
+`InteractionRequest.Resources` (and `BackchannelInteractionRequest`'s
+for CIBA), the client's default included — so where several resource
+servers share the AS, show them: the user can then tell which one a
+token is for. Approving, `GrantedAuthorization.Resources` can narrow
+them to a subset of those; leaving it nil grants all of them, and one
+that wasn't requested makes the completion fail, as an unrequested
+scope does.
 
 While clients move to sending `resource`, a resource server can also
 accept tokens issued without one: `AcceptIssuerAudience: true` in
@@ -1125,7 +1141,10 @@ server verifying the issuer audience accepts, works here too. A UserInfo
 endpoint keeps the default (no `Resource`): it accepts only tokens
 issued without a resource, so a client that also calls UserInfo names
 the AS's issuer as one of its resources (list it in `Config.Resources`)
-and narrows each token request to the resource it's for.
+and narrows each token request to the resource it's for. A client
+confined by `AllowedResources` must list the issuer there too to call
+UserInfo at all, and then, listing more than one, name a resource in
+every request.
 
 ### Verify a request
 

UPGRADING.md

@@ -17,6 +17,44 @@ and `keys/ephemeral` needs only the steps not marked *production only*.
 
 ## v0.61.0
 
+### A client confined by `AllowedResources` no longer gets issuer-audience tokens (server, storage)
+
+**Affects:** a server with resource indicators enabled (`Config.Resources`
+or `Dependencies.ResourcePolicy`) where a client registered with
+`storage.RegisteredClientConfig.AllowedResources` makes a request that
+names no resource: at PAR, in a CIBA request, for client credentials, or
+through `IssueAccessToken`/`IssueRefreshToken`. Also such a client's
+grants made without resources, at their next code exchange, refresh or
+CIBA token request.
+
+**Why:** a request naming no resource got an access token addressed to
+the issuer, which `AllowedResources` was never checked against. Every
+resource server accepting the issuer as an audience took it — one
+migrating with `serverresource.Options.AcceptIssuerAudience` or the
+issuer in `AdditionalAudiences`, and the UserInfo endpoint — so a client
+refused a resource could get a token it accepts simply by naming none.
+
+**What fails:** for a client with exactly one `AllowedResources` entry, a
+request naming no resource is now for that resource: the grant records
+it, and its access tokens are addressed to it, not the issuer. It's
+checked against `Config.Resources` and the `ResourcePolicy` as a named
+one is, so if the server doesn't accept it the request fails with
+`invalid_target`. For a client with several entries, a request naming
+none fails with `invalid_target` ("a resource is required"). A grant made
+without resources before the client was confined gets the same: its
+single resource, or `invalid_target`, so the client must authorize
+again. Nothing changes for clients without `AllowedResources`, or while
+resource indicators are disabled.
+
+**What to change:** a confined client that also calls the UserInfo
+endpoint, which accepts only issuer-audience tokens, lists the issuer
+identifier in its `AllowedResources` (and the server accepts it, in
+`Config.Resources`, say); having more than one entry, it then names a
+resource in every request. A confined client with several entries names
+the resource it wants on every request. To confine clients registered
+through OpenID Federation too, set the new
+`AutomaticRegistrationConfig.AllowedResources`.
+
 ### A statically registered client authenticates with its static keys only (federation, server)
 
 **Affects:** a server with OpenID Federation automatic registration (or

fapitest/resource_indicators_allowed_test.go

@@ -0,0 +1,91 @@
+package fapitest_test
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/idfoundry/fapigo/client"
+	"github.com/idfoundry/fapigo/fapitest"
+	"github.com/idfoundry/fapigo/server"
+	"github.com/idfoundry/fapigo/serverresource"
+)
+
+// TestResourceIndicatorsAllowedResourcesDefault: a client confined by
+// AllowedResources to one resource server that names no resource gets
+// tokens for that resource, end to end, never ones addressed to the
+// issuer, which another resource server migrating with
+// AcceptIssuerAudience (or the issuer's own, UserInfo say) would accept.
+func TestResourceIndicatorsAllowedResourcesDefault(t *testing.T) {
+	for _, opaque := range []bool{false, true} {
+		t.Run(fmt.Sprintf("opaque=%v", opaque), func(t *testing.T) {
+			h := fapitest.New(t, fapitest.Config{
+				Profile: server.ProfileFAPISecurity, Resources: servedResources,
+				AllowedResources: []string{credentials}, OpaqueAccessTokens: opaque,
+			})
+			ctx := context.Background()
+			_, err := h.RunAuthorizationCodeFlowWithRequest(ctx, client.BeginAuthorizationRequest{Scope: offlineScope, Resource: []string{payments}})
+			wantServerError(t, "RunAuthorizationCodeFlow(payments)", err, "invalid_target")
+
+			tokens, err := h.RunAuthorizationCodeFlow(ctx, offlineScope)
+			if err != nil {
+				t.Fatalf("RunAuthorizationCodeFlow: %v", err)
+			}
+			checkConfinedToCredentials(t, h, tokens, opaque)
+
+			// A refresh naming no resource gets the grant's: the default.
+			refreshed, err := h.Client.RefreshTokens(ctx, client.RefreshTokenRequest{Tokens: tokens})
+			if err != nil {
+				t.Fatalf("RefreshTokens: %v", err)
+			}
+			checkConfinedToCredentials(t, h, refreshed, opaque)
+		})
+	}
+}
+
+// TestResourceIndicatorsAllowedResourcesDefaultClientCredentials is
+// TestResourceIndicatorsAllowedResourcesDefault with no user involved.
+func TestResourceIndicatorsAllowedResourcesDefaultClientCredentials(t *testing.T) {
+	h := fapitest.New(t, fapitest.Config{
+		Profile: server.ProfileFAPISecurity, ClientCredentialsGrant: true,
+		Resources: servedResources, AllowedResources: []string{credentials},
+	})
+	result, err := h.Client.RequestClientCredentialsToken(context.Background(), client.ClientCredentialsTokenRequest{Scope: []string{"accounts"}})
+	if err != nil {
+		t.Fatalf("RequestClientCredentialsToken: %v", err)
+	}
+	checkConfinedToCredentials(t, h, client.TokenSet{AccessToken: result.AccessToken}, false)
+}
+
+// TestResourceIndicatorsAllowedResourcesRequired: a client confined to
+// several resource servers must name which a request is for.
+func TestResourceIndicatorsAllowedResourcesRequired(t *testing.T) {
+	h := fapitest.New(t, fapitest.Config{
+		Profile: server.ProfileFAPISecurity, ClientCredentialsGrant: true,
+		Resources: servedResources, AllowedResources: []string{credentials, payments},
+	})
+	ctx := context.Background()
+	_, err := h.RunAuthorizationCodeFlow(ctx, offlineScope)
+	wantServerError(t, "RunAuthorizationCodeFlow", err, "invalid_target")
+	_, err = h.Client.RequestClientCredentialsToken(ctx, client.ClientCredentialsTokenRequest{Scope: []string{"accounts"}})
+	wantServerError(t, "RequestClientCredentialsToken", err, "invalid_target")
+
+	tokens, err := h.RunAuthorizationCodeFlowWithRequest(ctx, client.BeginAuthorizationRequest{Scope: offlineScope, Resource: []string{payments}})
+	if err != nil {
+		t.Fatalf("RunAuthorizationCodeFlow(payments): %v", err)
+	}
+	checkVerifierAudience(t, h, tokens, payments, []string{payments})
+}
+
+// checkConfinedToCredentials checks tokens are for credentials alone:
+// accepted there, and refused by payments even while it accepts the
+// issuer as an audience, and by the issuer's own resource server.
+func checkConfinedToCredentials(t *testing.T, h *fapitest.Harness, tokens client.TokenSet, opaque bool) {
+	t.Helper()
+	if !opaque {
+		checkJWTAudience(t, tokens, `"`+credentials+`"`)
+	}
+	checkVerifierAudience(t, h, tokens, credentials, []string{credentials})
+	refuseAtResource(t, h, tokens, serverresource.Options{Resource: payments, AcceptIssuerAudience: true})
+	refuseAt(t, h, h.Resource, tokens)
+}

fapitest/resource_indicators_test.go

@@ -422,13 +422,12 @@ func TestResourceIndicatorsClientCredentials(t *testing.T) {
 				refuseAtResource(t, h, tokens, serverresource.Options{Resource: payments})
 			})
 
+			// Confined to several resources, the client must name
+			// which: a token addressed to the issuer would be accepted
+			// wherever the issuer is.
 			t.Run("no resource", func(t *testing.T) {
-				tokens, err := request(nil)
-				if err != nil {
-					t.Fatalf("RequestClientCredentialsToken: %v", err)
-				}
-				verifyAt(t, h, h.Resource, tokens)
-				refuseAtResource(t, h, tokens, serverresource.Options{Resource: accounts})
+				_, err := request(nil)
+				wantServerError(t, "RequestClientCredentialsToken", err, "invalid_target")
 			})
 
 			for _, tc := range []struct {

federation/automatic_registration.go

@@ -19,6 +19,7 @@ import (
 	intfed "github.com/idfoundry/fapigo/internal/federation"
 	"github.com/idfoundry/fapigo/internal/jose"
 	"github.com/idfoundry/fapigo/internal/mtls"
+	"github.com/idfoundry/fapigo/internal/resourceindicator"
 	"github.com/idfoundry/fapigo/internal/strictjson"
 	"github.com/idfoundry/fapigo/keys"
 	"github.com/idfoundry/fapigo/storage"
@@ -51,6 +52,19 @@ type AutomaticRegistrationConfig struct {
 	// self-published authorization_details_types. Empty means none.
 	AuthorizationDetailsTypes []string
 
+	// AllowedResources confines every automatically-registered client to
+	// these RFC 8707 resource indicators
+	// (storage.RegisteredClientConfig.AllowedResources) — for the same
+	// reason as AllowedScopes, never anything an RP publishes about
+	// itself: which resource servers a client may get tokens for is the
+	// operator's decision. Each an absolute URI without a fragment, none
+	// repeated. Empty means unconfined: an RP may name any resource the
+	// server accepts, and one naming none gets tokens addressed to the
+	// issuer. Set, a request naming none is for its single entry, or,
+	// with several, invalid_target (see
+	// storage.RegisteredClientConfig.AllowedResources).
+	AllowedResources []string
+
 	// MaxCacheAge bounds how long a resolved client's Trust Chain is
 	// reused before it is resolved again — the actual cache lifetime
 	// for a given client is min(MaxCacheAge, its own Trust Chain's
@@ -373,6 +387,10 @@ func NewAutomaticClientRepository(underlying storage.ClientRepository, resolver
 	if err := validateSubjectTypes(cfg.SubjectTypes, cfg.DefaultSubjectType); err != nil {
 		return nil, err
 	}
+	if err := validateAllowedResources(cfg.AllowedResources); err != nil {
+		return nil, err
+	}
+	cfg.AllowedResources = slices.Clone(cfg.AllowedResources)
 	if clock == nil {
 		return nil, fmt.Errorf("federation: clock is required")
 	}
@@ -384,6 +402,20 @@ func NewAutomaticClientRepository(underlying storage.ClientRepository, resolver
 	}, nil
 }
 
+// validateAllowedResources checks AutomaticRegistrationConfig.AllowedResources:
+// each a resource indicator RFC 8707 §2 allows, none repeated.
+func validateAllowedResources(resources []string) error {
+	for i, r := range resources {
+		if err := resourceindicator.Check(r); err != nil {
+			return fmt.Errorf("federation: config: allowed_resources[%d]: %w", i, err)
+		}
+		if slices.Contains(resources[:i], r) {
+			return fmt.Errorf("federation: config: allowed_resources[%d]: %q is repeated", i, r)
+		}
+	}
+	return nil
+}
+
 // ResolveClient implements storage.ClientRepository.
 func (a *AutomaticClientRepository) ResolveClient(ctx context.Context, id fapi.ClientID) (storage.RegisteredClient, error) {
 	client, err := a.underlying.ResolveClient(ctx, id)
@@ -755,7 +787,7 @@ type relyingPartyMetadata struct {
 // choiceLimits).
 //
 // a.cfg supplies the operator-level capability grants (AllowedScopes,
-// AllowsClientCredentialsGrant, AllowsCIBA) that must never be inferred
+// AllowedResources, 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.
@@ -810,6 +842,7 @@ func (a *AutomaticClientRepository) registeredClientConfigFromMetadata(ctx conte
 		SenderConstrain:                 senderConstrain,
 		AllowedScopes:                   a.cfg.AllowedScopes,
 		AuthorizationDetailsTypes:       a.cfg.AuthorizationDetailsTypes,
+		AllowedResources:                a.cfg.AllowedResources,
 		AutomaticFederationRegistration: true,
 		AllowsClientCredentialsGrant:    a.cfg.AllowsClientCredentialsGrant,
 	}

federation/automatic_registration_test.go

@@ -292,6 +292,16 @@ func TestNewAutomaticClientRepositoryRejectsInvalidConfig(t *testing.T) {
 			cfg.FailureCacheAge = -time.Second
 			return alwaysFailsRepository{}, resolver, f.fetcher, cfg, fixedClock{now: f.now}
 		},
+		"relative allowed resource": func() (storage.ClientRepository, *federation.Resolver, *fapihttp.Client, federation.AutomaticRegistrationConfig, federation.Clock) {
+			cfg := validAutomaticRegistrationConfig()
+			cfg.AllowedResources = []string{"/api"}
+			return alwaysFailsRepository{}, resolver, f.fetcher, cfg, fixedClock{now: f.now}
+		},
+		"repeated allowed resource": func() (storage.ClientRepository, *federation.Resolver, *fapihttp.Client, federation.AutomaticRegistrationConfig, federation.Clock) {
+			cfg := validAutomaticRegistrationConfig()
+			cfg.AllowedResources = []string{"https://rs.example", "https://rs.example"}
+			return alwaysFailsRepository{}, resolver, f.fetcher, cfg, fixedClock{now: f.now}
+		},
 		"invalid allowed client auth method": func() (storage.ClientRepository, *federation.Resolver, *fapihttp.Client, federation.AutomaticRegistrationConfig, federation.Clock) {
 			cfg := validAutomaticRegistrationConfig()
 			cfg.AllowedClientAuthMethods = []storage.ClientAuthMethod{storage.ClientAuthMethod(255)}
@@ -377,6 +387,9 @@ func TestAutomaticClientRepositoryResolveClientFallsBackToFederation(t *testing.
 	if got.AllowsAuthorizationDetailsType("payment") {
 		t.Error("AllowsAuthorizationDetailsType(payment) = true with no AutomaticRegistrationConfig.AuthorizationDetailsTypes")
 	}
+	if got.AllowedResources() != nil {
+		t.Errorf("AllowedResources() = %q with no AutomaticRegistrationConfig.AllowedResources", got.AllowedResources())
+	}
 }
 
 // unavailableRepository is a storage.ClientRepository whose backend
@@ -428,6 +441,31 @@ func TestAutomaticClientRepositoryAuthorizationDetailsTypesComeFromConfig(t *tes
 	}
 }
 
+// TestAutomaticClientRepositoryAllowedResourcesComeFromConfig covers an
+// automatically registered client being confined to the operator's
+// resources, never ones from the RP's own metadata, and the config's
+// slice being copied.
+func TestAutomaticClientRepositoryAllowedResourcesComeFromConfig(t *testing.T) {
+	f := setupAutomaticRegistrationFixture(t, rpMetadataBuilder(t))
+	cfg := validAutomaticRegistrationConfig()
+	cfg.AllowedResources = []string{"https://rs.example", "https://other.example"}
+	repo, err := federation.NewAutomaticClientRepository(alwaysFailsRepository{}, f.newResolver(t), f.fetcher, cfg, fixedClock{now: f.now})
+	if err != nil {
+		t.Fatalf("NewAutomaticClientRepository: %v", err)
+	}
+	cfg.AllowedResources[0] = "https://changed.example"
+	got, err := repo.ResolveClient(context.Background(), fapi.ClientID(f.rpID))
+	if err != nil {
+		t.Fatalf("ResolveClient: %v", err)
+	}
+	if want := []string{"https://other.example", "https://rs.example"}; !slices.Equal(got.AllowedResources(), want) {
+		t.Errorf("AllowedResources() = %q, want %q", got.AllowedResources(), want)
+	}
+	if got.AllowsResource("https://changed.example") {
+		t.Error("a change to the config's slice after construction reached the client")
+	}
+}
+
 func TestAutomaticClientRepositoryResolveClientDoesNotAllowClientCredentialsByDefault(t *testing.T) {
 	f := setupAutomaticRegistrationFixture(t, rpMetadataBuilder(t))
 	resolver := f.newResolver(t)

server/authorization.go

@@ -3,6 +3,7 @@ package server
 import (
 	"context"
 	"encoding/json"
+	"slices"
 	"strings"
 	"time"
 
@@ -152,6 +153,7 @@ func (s *Server) BeginAuthorization(ctx context.Context, req BeginAuthorizationR
 	interaction.RequiredSubject = request.requiredSubjectForApplication()
 	interaction.ClientDisplay = client.Display()
 	interaction.Extensions = extensions
+	interaction.Resources = slices.Clone(request.Resources)
 
 	action := InteractionRequired{
 		Handle:      InteractionHandle{value: handle},