From b5828d33672babc1b209ab4ca26595d13df232a5 Mon Sep 17 00:00:00 2001 From: Aron Gates Date: Thu, 15 Oct 2026 12:43:54 +0000 Subject: [PATCH 1/2] Add authorization-endpoint DCR registration probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cache-hit probe redeems a random authorization code at the token endpoint and reads invalid_client as "registration gone". It decides only for upstreams that authenticate the client before validating the grant; a vendor whose token endpoint answers invalid_grant for any client_id keeps presenting a dropped registration, and the user sees the rejection in the browser. dcrConfig.registrationProbe selects the strategy per upstream: tokenEndpoint (the default, unchanged), authorizationEndpoint, or none. The authorization-endpoint probe sends an authorization request for the cached client without following redirects: RFC 6749 §4.1.2.1 has the server answer an unknown client_id or redirect_uri directly, so a direct 4xx (other than 404, 405 and 429) means the registration is gone and the resolver re-registers through the same replacement path; 2xx and 3xx mean the client is known. The request carries no credentials and the response body is never read or logged. Also loosen the rotationKey docs now that a probe can detect some rejections, and log the configured probe on the cache-hit records. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Aron Gates --- .../v1beta1/mcpexternalauthconfig_types.go | 30 +- .../pkg/controllerutil/authserver.go | 1 + .../pkg/controllerutil/authserver_test.go | 2 + ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 132 ++++-- ...olhive.stacklok.dev_virtualmcpservers.yaml | 132 ++++-- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 132 ++++-- ...olhive.stacklok.dev_virtualmcpservers.yaml | 132 ++++-- docs/arch/11-auth-server-storage.md | 2 +- docs/operator/crd-api.md | 3 +- docs/server/docs.go | 6 +- docs/server/swagger.json | 6 +- docs/server/swagger.yaml | 31 +- pkg/auth/dcr/request.go | 67 ++- pkg/auth/dcr/resolver.go | 33 +- pkg/auth/dcr/resolver_test.go | 10 + pkg/auth/dcr/validate.go | 237 ++++++++-- pkg/auth/dcr/validate_test.go | 414 +++++++++++++++++- pkg/authserver/config.go | 37 +- pkg/authserver/config_test.go | 23 + pkg/authserver/runner/dcr_adapter.go | 1 + pkg/authserver/runner/dcr_adapter_test.go | 19 + pkg/authserver/storage/types.go | 2 +- sdk/go/client/oas_json_gen.go | 25 +- sdk/go/client/oas_schemas_gen.go | 39 +- sdk/go/openapi.json | 6 +- sdk/go/openapi.yaml | 31 +- 26 files changed, 1308 insertions(+), 245 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 262c9df25e..04ae5a5bae 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -1879,19 +1879,33 @@ type DCRUpstreamConfig struct { // Change it to make the proxies register a fresh client at their next // start instead of reusing the cached one: for example after the // upstream has dropped the registration, or when a rotation is required. - // At start ToolHive checks the cached client at the upstream's token - // endpoint and registers a new one by itself when the upstream answers - // invalid_client. Where that check cannot tell (an upstream that answers - // a dropped public client like a live one, or a private_key_jwt client), - // the stale client_id is only rejected in the user's browser, and - // changing this key is the remedy. The previous registration's row stays - // in storage, and upstream refresh tokens bound to the previous client_id - // stop refreshing, so affected users sign in again. + // At start ToolHive checks the cached client at the upstream (see + // RegistrationProbe) and registers a new one by itself when the upstream + // rejects it. Where the configured probe cannot tell, the stale client_id + // may only be rejected in the user's browser, and changing this key is + // the remedy. The previous registration's row stays in storage, and + // upstream refresh tokens bound to the previous client_id stop + // refreshing, so affected users sign in again. // +optional // +kubebuilder:validation:Pattern=`^[A-Za-z0-9._-]+$` // +kubebuilder:validation:MaxLength=64 RotationKey string `json:"rotationKey,omitempty"` + // RegistrationProbe selects how ToolHive checks the cached upstream client + // at start. tokenEndpoint (the default) redeems a random authorization + // code with the cached client and reads invalid_client as "registration + // gone"; it decides only for upstreams that authenticate the client + // before validating the grant. authorizationEndpoint sends an + // authorization request for the cached client without following + // redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + // upstreams whose token endpoint answers invalid_grant for any client_id; + // do not use it behind bot protection that answers 4xx to non-browser + // requests. none skips the check; the registration is then retired only + // through RotationKey. + // +optional + // +kubebuilder:validation:Enum=tokenEndpoint;authorizationEndpoint;none + RegistrationProbe string `json:"registrationProbe,omitempty"` + // InitialAccessTokenRef is an optional reference to a Kubernetes Secret // carrying an RFC 7591 §3 initial access token. When set, the resolver // presents the token value as a Bearer credential on the registration diff --git a/cmd/thv-operator/pkg/controllerutil/authserver.go b/cmd/thv-operator/pkg/controllerutil/authserver.go index 210ed343e3..95d49ff900 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver.go @@ -1449,6 +1449,7 @@ func buildDCRUpstreamRunConfig( DiscoveryURL: dcr.DiscoveryURL, RegistrationEndpoint: dcr.RegistrationEndpoint, RotationKey: dcr.RotationKey, + RegistrationProbe: dcr.RegistrationProbe, SoftwareID: dcr.SoftwareID, SoftwareStatement: dcr.SoftwareStatement, } diff --git a/cmd/thv-operator/pkg/controllerutil/authserver_test.go b/cmd/thv-operator/pkg/controllerutil/authserver_test.go index 54d916046f..cd996634cb 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver_test.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver_test.go @@ -1815,6 +1815,7 @@ func TestBuildAuthServerRunConfig(t *testing.T) { DCRConfig: &mcpv1beta1.DCRUpstreamConfig{ RegistrationEndpoint: "https://idp.example.com/register", RotationKey: "2026-10", + RegistrationProbe: "authorizationEndpoint", }, }, }, @@ -1832,6 +1833,7 @@ func TestBuildAuthServerRunConfig(t *testing.T) { upstream.OAuth2Config.DCRConfig.RegistrationEndpoint) assert.Empty(t, upstream.OAuth2Config.DCRConfig.DiscoveryURL) assert.Equal(t, "2026-10", upstream.OAuth2Config.DCRConfig.RotationKey) + assert.Equal(t, "authorizationEndpoint", upstream.OAuth2Config.DCRConfig.RegistrationProbe) // No InitialAccessTokenRef set: env var name should stay empty. assert.Empty(t, upstream.OAuth2Config.DCRConfig.InitialAccessTokenEnvVar) }, diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index 8e7820925d..3bd97df9fe 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -1736,6 +1736,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -1743,14 +1761,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -2144,6 +2161,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -2151,14 +2186,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -4678,6 +4712,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -4685,14 +4737,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -5086,6 +5137,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -5093,14 +5162,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index 7bfeb57393..bd7baa5724 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1623,6 +1623,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -1630,14 +1648,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -2031,6 +2048,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -2038,14 +2073,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -6953,6 +6987,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -6960,14 +7012,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -7361,6 +7412,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -7368,14 +7437,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index 2af6596308..d320aa9a74 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -1739,6 +1739,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -1746,14 +1764,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -2147,6 +2164,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -2154,14 +2189,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -4681,6 +4715,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -4688,14 +4740,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -5089,6 +5140,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -5096,14 +5165,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index 72a656d095..5c7345cb99 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1626,6 +1626,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -1633,14 +1651,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -2034,6 +2051,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -2041,14 +2076,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -6956,6 +6990,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -6963,14 +7015,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string @@ -7364,6 +7415,24 @@ spec: maxLength: 2048 pattern: ^https://[^\s?#]+[^/\s?#]$ type: string + registrationProbe: + description: |- + RegistrationProbe selects how ToolHive checks the cached upstream client + at start. tokenEndpoint (the default) redeems a random authorization + code with the cached client and reads invalid_client as "registration + gone"; it decides only for upstreams that authenticate the client + before validating the grant. authorizationEndpoint sends an + authorization request for the cached client without following + redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for + upstreams whose token endpoint answers invalid_grant for any client_id; + do not use it behind bot protection that answers 4xx to non-browser + requests. none skips the check; the registration is then retired only + through RotationKey. + enum: + - tokenEndpoint + - authorizationEndpoint + - none + type: string rotationKey: description: |- RotationKey is an optional operator-chosen label that is part of the @@ -7371,14 +7440,13 @@ spec: Change it to make the proxies register a fresh client at their next start instead of reusing the cached one: for example after the upstream has dropped the registration, or when a rotation is required. - At start ToolHive checks the cached client at the upstream's token - endpoint and registers a new one by itself when the upstream answers - invalid_client. Where that check cannot tell (an upstream that answers - a dropped public client like a live one, or a private_key_jwt client), - the stale client_id is only rejected in the user's browser, and - changing this key is the remedy. The previous registration's row stays - in storage, and upstream refresh tokens bound to the previous client_id - stop refreshing, so affected users sign in again. + At start ToolHive checks the cached client at the upstream (see + RegistrationProbe) and registers a new one by itself when the upstream + rejects it. Where the configured probe cannot tell, the stale client_id + may only be rejected in the user's browser, and changing this key is + the remedy. The previous registration's row stays in storage, and + upstream refresh tokens bound to the previous client_id stop + refreshing, so affected users sign in again. maxLength: 64 pattern: ^[A-Za-z0-9._-]+$ type: string diff --git a/docs/arch/11-auth-server-storage.md b/docs/arch/11-auth-server-storage.md index ed6d8d92a6..2e927616d1 100644 --- a/docs/arch/11-auth-server-storage.md +++ b/docs/arch/11-auth-server-storage.md @@ -62,7 +62,7 @@ The storage layer implements multiple interfaces from the [fosite](https://githu - `PendingAuthorizationStorage` — In-flight authorization tracking. Interactive requests have a single-use `awaiting` consent stage before upstream login; only an `approved` record can complete a callback. Each `PendingAuthorization` carries a `BrowserBindingHash`, the SHA-256 of a random cookie value set on the user agent. The consent POST and `/oauth/callback` require the matching cookie before consuming their respective records. Only the hash is stored; an older row without a binding or approval stage cannot complete the flow and must be restarted. `PendingDeviceLogin` carries the same browser binding for the separate device-flow verification-page login, which shares `/oauth/callback`. - `RememberedConsentStorage` — Optional server-side browser sessions and one approval per `(userID, clientID)`. The browser holds only a random `__Host-thv_consent_` secret; storage holds its SHA-256 digest. Each approval records the redirect URI, scopes, and resource selected for that client. Memory and Redis implement this capability; decorators expose the underlying backend via `storage.Unwrap`. See [Interactive client approval](#interactive-client-approval). - `UserStorage` — Internal user accounts and provider identity linking -- `DCRCredentialStore` — DCR client metadata and credential persistence; intentionally NOT embedded in `Storage` (each backend implements it separately and call sites reach it via an explicit `stor.(DCRCredentialStore)` type assertion). Secret-based clients store hashed secrets; `private_key_jwt` clients store their inline public JWKS and signing algorithm. Outbound (upstream) registrations resolved by `pkg/auth/dcr` are keyed by `(Issuer, UpstreamID, RedirectURI, ScopesHash)` plus the optional operator-set `dcrConfig.rotationKey`. Changing the rotation key addresses a new row, so the next start registers a fresh upstream client; the previous row stays in place and carries no TTL unless the upstream asserted `client_secret_expires_at`. On a cache hit the resolver also checks the cached client at the upstream token endpoint and replaces the row when the upstream answers `invalid_client`, provided the store implements `dcr.Replacer`. The rotation key covers the upstreams where that check cannot tell a dropped registration from a live one: an upstream that validates the code before it looks up a public client, or a `private_key_jwt` client. +- `DCRCredentialStore` — DCR client metadata and credential persistence; intentionally NOT embedded in `Storage` (each backend implements it separately and call sites reach it via an explicit `stor.(DCRCredentialStore)` type assertion). Secret-based clients store hashed secrets; `private_key_jwt` clients store their inline public JWKS and signing algorithm. Outbound (upstream) registrations resolved by `pkg/auth/dcr` are keyed by `(Issuer, UpstreamID, RedirectURI, ScopesHash)` plus the optional operator-set `dcrConfig.rotationKey`. Changing the rotation key addresses a new row, so the next start registers a fresh upstream client; the previous row stays in place and carries no TTL unless the upstream asserted `client_secret_expires_at`. On a cache hit the resolver also checks the cached client against the upstream and replaces the row when the upstream rejects it, provided the store implements `dcr.Replacer`. `dcrConfig.registrationProbe` selects the check: `tokenEndpoint` (default) redeems a random authorization code and reads `invalid_client` as "gone", which decides only for upstreams that authenticate the client before validating the grant; `authorizationEndpoint` sends an authorization request without following redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers `invalid_grant` for any client_id; `none` skips the check. The rotation key covers whatever the configured probe cannot tell: a `private_key_jwt` client, an upstream that renders its unknown-client error with 200 or a redirect, or an operator who wants a deliberate rotation. - `AssertionJWTConsumer` — atomic single-use consumption for validated JWT assertions. It is deliberately separate from `Storage`, so only assertion-grant composition requires replay protection. **Implementation:** diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 5eedb96843..76849f569c 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -1961,7 +1961,8 @@ _Appears in:_ | --- | --- | --- | --- | | `discoveryUrl` _string_ | DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver
issues a single GET against this URL (no well-known-path fallback) and
reads registration_endpoint, authorization_endpoint, token_endpoint,
token_endpoint_auth_methods_supported, and scopes_supported from the
response.
Mutually exclusive with RegistrationEndpoint.
HTTPS is required because the registration endpoint resolved from this
document carries the initial access token and the issued client_secret
(RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd
object budget, regex evaluation cost) and matches the conventional URL
length cap. | | MaxLength: 2048
Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| | `registrationEndpoint` _string_ | RegistrationEndpoint is the RFC 7591 registration endpoint URL used
directly, bypassing discovery. When using this field, the caller is
expected to also supply AuthorizationEndpoint, TokenEndpoint, and an
explicit Scopes list on the parent OAuth2UpstreamConfig.
Mutually exclusive with DiscoveryURL.
HTTPS is required because the registration endpoint carries the initial
access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3).
MaxLength is a defensive size cap (etcd object budget, regex evaluation
cost) and matches the conventional URL length cap. | | MaxLength: 2048
Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| -| `rotationKey` _string_ | RotationKey is an optional operator-chosen label that is part of the
cached registration's identity but is never sent to the upstream.
Change it to make the proxies register a fresh client at their next
start instead of reusing the cached one: for example after the
upstream has dropped the registration, or when a rotation is required.
At start ToolHive checks the cached client at the upstream's token
endpoint and registers a new one by itself when the upstream answers
invalid_client. Where that check cannot tell (an upstream that answers
a dropped public client like a live one, or a private_key_jwt client),
the stale client_id is only rejected in the user's browser, and
changing this key is the remedy. The previous registration's row stays
in storage, and upstream refresh tokens bound to the previous client_id
stop refreshing, so affected users sign in again. | | MaxLength: 64
Pattern: `^[A-Za-z0-9._-]+$`
Optional: \{\}
| +| `rotationKey` _string_ | RotationKey is an optional operator-chosen label that is part of the
cached registration's identity but is never sent to the upstream.
Change it to make the proxies register a fresh client at their next
start instead of reusing the cached one: for example after the
upstream has dropped the registration, or when a rotation is required.
At start ToolHive checks the cached client at the upstream (see
RegistrationProbe) and registers a new one by itself when the upstream
rejects it. Where the configured probe cannot tell, the stale client_id
may only be rejected in the user's browser, and changing this key is
the remedy. The previous registration's row stays in storage, and
upstream refresh tokens bound to the previous client_id stop
refreshing, so affected users sign in again. | | MaxLength: 64
Pattern: `^[A-Za-z0-9._-]+$`
Optional: \{\}
| +| `registrationProbe` _string_ | RegistrationProbe selects how ToolHive checks the cached upstream client
at start. tokenEndpoint (the default) redeems a random authorization
code with the cached client and reads invalid_client as "registration
gone"; it decides only for upstreams that authenticate the client
before validating the grant. authorizationEndpoint sends an
authorization request for the cached client without following
redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for
upstreams whose token endpoint answers invalid_grant for any client_id;
do not use it behind bot protection that answers 4xx to non-browser
requests. none skips the check; the registration is then retired only
through RotationKey. | | Enum: [tokenEndpoint authorizationEndpoint none]
Optional: \{\}
| | `initialAccessTokenRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | InitialAccessTokenRef is an optional reference to a Kubernetes Secret
carrying an RFC 7591 §3 initial access token. When set, the resolver
presents the token value as a Bearer credential on the registration
request. Mirrors the ClientSecretRef pattern. | | Optional: \{\}
| | `softwareId` _string_ | SoftwareID is the RFC 7591 "software_id" registration metadata value,
identifying the client software independent of any particular
registration instance. Typically a UUID or short identifier. | | MaxLength: 255
Optional: \{\}
| | `softwareStatement` _string_ | SoftwareStatement is the RFC 7591 "software_statement" JWT asserting
metadata about the client software, signed by a party the authorization
server trusts.
Stored inline on the CR. The JWT is signed but not encrypted, so its
contents are visible to anyone with get/list/watch on this resource and
appear in etcd backups in plaintext. Treat the value as non-confidential
(signed attestation, not a secret). Operators that rotate software
statements like bearer credentials should keep them at the authorization
server side and rely on the registration endpoint's initial access
token (see InitialAccessTokenRef) instead of placing them on the CR.
Bounded to 16384 characters as a defensive size cap (etcd object
budget, regex evaluation cost). Real-world signed statements with
embedded x5c certificate chains, JWKS keys, or OIDC-Federation
trust-framework metadata routinely exceed 4 KB. | | MaxLength: 16384
Optional: \{\}
| diff --git a/docs/server/docs.go b/docs/server/docs.go index f440436150..1549d70b45 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -157,8 +157,12 @@ const docTemplate = `{ "description": "RegistrationEndpoint is the RFC 7591 registration endpoint URL used\ndirectly, bypassing discovery. Because no discovery is performed,\nserver-capability fields (token_endpoint_auth_methods_supported,\nscopes_supported) are unavailable on this code path; the caller is\nexpected to also supply AuthorizationEndpoint, TokenEndpoint, and an\nexplicit Scopes list on the parent OAuth2UpstreamRunConfig. Auth\nmethod falls back to the resolver's default (client_secret_basic).\n\nMutually exclusive with DiscoveryURL.", "type": "string" }, + "registration_probe": { + "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 4xx as \"gone\"\n(RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers\ninvalid_grant for any client_id; do not use it behind bot protection\nthat answers 4xx to non-browser requests. \"none\" skips the check; the\nregistration is then retired only through RotationKey.", + "type": "string" + }, "rotation_key": { - "description": "RotationKey is an optional operator-chosen label (1-64 characters from\n[A-Za-z0-9._-]) that becomes part of the cached registration's identity\nwithout being sent to the upstream. Changing it makes the next start\nregister a fresh client instead of reusing the cached one. At start the\nresolver checks the cached client at the upstream's token endpoint and\nreplaces it by itself when the upstream answers invalid_client. Use\nRotationKey when the upstream has dropped the registration and that\ncheck cannot tell (an upstream that answers a dropped public client\nlike a live one, or a private_key_jwt client), so the rejection is only\nvisible in the user's browser, or when a registration must be rotated\non request. The row written under the previous value is left in place. Upstream refresh tokens\nbound to the previous client_id stop refreshing once the client\nchanges; affected users sign in again.", + "description": "RotationKey is an optional operator-chosen label (1-64 characters from\n[A-Za-z0-9._-]) that becomes part of the cached registration's identity\nwithout being sent to the upstream. Changing it makes the next start\nregister a fresh client instead of reusing the cached one. At start the\nresolver checks the cached client at the upstream (see RegistrationProbe)\nand replaces it by itself when the upstream rejects it. Use RotationKey\nwhen the upstream has dropped the registration and the configured probe\ncannot tell, so the rejection may not be visible to the resolver, or when\na registration must be rotated on request. The row written under the\nprevious value is left in place. Upstream refresh tokens bound to the\nprevious client_id stop refreshing once the client changes; affected\nusers sign in again.", "type": "string" }, "software_id": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index eee9599a8d..a528af446b 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -150,8 +150,12 @@ "description": "RegistrationEndpoint is the RFC 7591 registration endpoint URL used\ndirectly, bypassing discovery. Because no discovery is performed,\nserver-capability fields (token_endpoint_auth_methods_supported,\nscopes_supported) are unavailable on this code path; the caller is\nexpected to also supply AuthorizationEndpoint, TokenEndpoint, and an\nexplicit Scopes list on the parent OAuth2UpstreamRunConfig. Auth\nmethod falls back to the resolver's default (client_secret_basic).\n\nMutually exclusive with DiscoveryURL.", "type": "string" }, + "registration_probe": { + "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 4xx as \"gone\"\n(RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers\ninvalid_grant for any client_id; do not use it behind bot protection\nthat answers 4xx to non-browser requests. \"none\" skips the check; the\nregistration is then retired only through RotationKey.", + "type": "string" + }, "rotation_key": { - "description": "RotationKey is an optional operator-chosen label (1-64 characters from\n[A-Za-z0-9._-]) that becomes part of the cached registration's identity\nwithout being sent to the upstream. Changing it makes the next start\nregister a fresh client instead of reusing the cached one. At start the\nresolver checks the cached client at the upstream's token endpoint and\nreplaces it by itself when the upstream answers invalid_client. Use\nRotationKey when the upstream has dropped the registration and that\ncheck cannot tell (an upstream that answers a dropped public client\nlike a live one, or a private_key_jwt client), so the rejection is only\nvisible in the user's browser, or when a registration must be rotated\non request. The row written under the previous value is left in place. Upstream refresh tokens\nbound to the previous client_id stop refreshing once the client\nchanges; affected users sign in again.", + "description": "RotationKey is an optional operator-chosen label (1-64 characters from\n[A-Za-z0-9._-]) that becomes part of the cached registration's identity\nwithout being sent to the upstream. Changing it makes the next start\nregister a fresh client instead of reusing the cached one. At start the\nresolver checks the cached client at the upstream (see RegistrationProbe)\nand replaces it by itself when the upstream rejects it. Use RotationKey\nwhen the upstream has dropped the registration and the configured probe\ncannot tell, so the rejection may not be visible to the resolver, or when\na registration must be rotated on request. The row written under the\nprevious value is left in place. Upstream refresh tokens bound to the\nprevious client_id stop refreshing once the client changes; affected\nusers sign in again.", "type": "string" }, "software_id": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index b182fe000b..d62b2e40a3 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -203,21 +203,34 @@ components: Mutually exclusive with DiscoveryURL. type: string + registration_probe: + description: |- + RegistrationProbe selects how the resolver checks a cached registration + against the upstream at start. "tokenEndpoint" (the default) redeems a + random authorization code with the cached client and reads + invalid_client as "registration gone"; it decides only for upstreams + that authenticate the client before validating the grant. + "authorizationEndpoint" sends an authorization request for the cached + client without following redirects and reads a direct 4xx as "gone" + (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers + invalid_grant for any client_id; do not use it behind bot protection + that answers 4xx to non-browser requests. "none" skips the check; the + registration is then retired only through RotationKey. + type: string rotation_key: description: |- RotationKey is an optional operator-chosen label (1-64 characters from [A-Za-z0-9._-]) that becomes part of the cached registration's identity without being sent to the upstream. Changing it makes the next start register a fresh client instead of reusing the cached one. At start the - resolver checks the cached client at the upstream's token endpoint and - replaces it by itself when the upstream answers invalid_client. Use - RotationKey when the upstream has dropped the registration and that - check cannot tell (an upstream that answers a dropped public client - like a live one, or a private_key_jwt client), so the rejection is only - visible in the user's browser, or when a registration must be rotated - on request. The row written under the previous value is left in place. Upstream refresh tokens - bound to the previous client_id stop refreshing once the client - changes; affected users sign in again. + resolver checks the cached client at the upstream (see RegistrationProbe) + and replaces it by itself when the upstream rejects it. Use RotationKey + when the upstream has dropped the registration and the configured probe + cannot tell, so the rejection may not be visible to the resolver, or when + a registration must be rotated on request. The row written under the + previous value is left in place. Upstream refresh tokens bound to the + previous client_id stop refreshing once the client changes; affected + users sign in again. type: string software_id: description: |- diff --git a/pkg/auth/dcr/request.go b/pkg/auth/dcr/request.go index 53aa1f7790..dc854de58a 100644 --- a/pkg/auth/dcr/request.go +++ b/pkg/auth/dcr/request.go @@ -3,6 +3,57 @@ package dcr +import "fmt" + +// RegistrationProbe selects how ResolveCredentials confirms, on a cache hit, +// that the upstream still recognises the cached client (see +// verifyCachedRegistration). The zero value means ProbeTokenEndpoint. +type RegistrationProbe string + +const ( + // ProbeTokenEndpoint redeems a random authorization code at the token + // endpoint with the cached credentials. invalid_client means the + // registration is gone; invalid_grant means it is alive. It decides only + // for upstreams that authenticate the client before validating the grant + // (a ToolHive auth server does). This is the default. + ProbeTokenEndpoint RegistrationProbe = "tokenEndpoint" + + // ProbeAuthorizationEndpoint sends an authorization request for the + // cached client without following redirects. RFC 6749 §4.1.2.1 has the + // server answer an unknown client_id or redirect_uri directly, with no + // redirect, while a known client is redirected (to login, or back to + // redirect_uri with an error). A direct 4xx therefore means the + // registration is gone. Use it for upstreams whose token endpoint + // validates the grant before the client, where ProbeTokenEndpoint cannot + // decide; not behind bot protection that answers 4xx to non-browser + // requests, which would re-register at every start. + ProbeAuthorizationEndpoint RegistrationProbe = "authorizationEndpoint" + + // ProbeNone never contacts the upstream on a cache hit; the cached + // registration is used as-is and retired only through RotationKey. + ProbeNone RegistrationProbe = "none" +) + +// Validate rejects a RegistrationProbe value other than the empty default +// and the three named strategies. +func (p RegistrationProbe) Validate() error { + switch p { + case "", ProbeTokenEndpoint, ProbeAuthorizationEndpoint, ProbeNone: + return nil + default: + return fmt.Errorf("registration_probe must be one of %q, %q or %q (got %q)", + ProbeTokenEndpoint, ProbeAuthorizationEndpoint, ProbeNone, string(p)) + } +} + +// effective returns the strategy to run, resolving the empty default. +func (p RegistrationProbe) effective() RegistrationProbe { + if p == "" { + return ProbeTokenEndpoint + } + return p +} + // Request is the profile-neutral input to ResolveCredentials. Each consumer // (embedded authserver, CLI OAuth flow) translates its domain types into a // Request at the call site so the resolver does not import any consumer's @@ -96,13 +147,19 @@ type Request struct { // key (and the in-process singleflight key) when non-empty. It is never // sent to the upstream. Changing it forces a fresh RFC 7591 registration // on the next resolve, without editing the credential store by hand. - // ResolveCredentials replaces a registration the upstream rejects with - // invalid_client on its own (see verifyCachedRegistration); RotationKey - // is how an operator retires one the probe cannot tell is gone, or - // rotates one on request. An empty value addresses the same cache row as - // a Request written before the field existed. + // ResolveCredentials replaces a registration the upstream rejects on its + // own when the configured RegistrationProbe can tell (see + // verifyCachedRegistration); RotationKey is how an operator retires one + // the probe cannot tell is gone, or rotates one on request. An empty + // value addresses the same cache row as a Request written before the + // field existed. RotationKey string + // RegistrationProbe selects how a cached registration is checked against + // the upstream on a cache hit. Empty means ProbeTokenEndpoint. See the + // RegistrationProbe constants for when each strategy can decide. + RegistrationProbe RegistrationProbe + // CodeChallengeMethodsSupported lists the PKCE challenge methods the // upstream advertises. Only consulted when RegistrationEndpoint is set; // on the DiscoveryURL branch the resolver reads this value from the diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index f044294c29..7a67198d0a 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -319,16 +319,20 @@ func newDCRStepError(step, issuer, redirectURI string, err error) *dcrStepError // the upstream authorization server identified by req.DiscoveryURL or // req.RegistrationEndpoint, caching the resulting credentials in cache. // On cache hit, when cache implements Replacer, the resolver sends one -// token-endpoint request to confirm the upstream still recognises the cached -// client (verifyCachedRegistration). When the upstream answers invalid_client, -// the resolver registers a new client and replaces the cached entry; -// otherwise, and always when cache does not implement Replacer, it returns -// the cached resolution without discovery or registration I/O. +// request to confirm the upstream still recognises the cached client, using +// the strategy req.RegistrationProbe selects (verifyCachedRegistration): the +// token endpoint by default, or the authorization endpoint. When the upstream +// rejects the client, the resolver registers a new client and replaces the +// cached entry; otherwise, and always when cache does not implement Replacer +// or req.RegistrationProbe is ProbeNone, it returns the cached resolution +// without discovery or registration I/O. // -// The probe cannot tell every dropped registration apart from a live one: an -// upstream that validates the grant before authenticating a public client, -// or a private_key_jwt client, leaves the cached registration in use. For -// those, the operator retires the registration with Request.RotationKey. +// Neither probe can tell every dropped registration apart from a live one: +// the token-endpoint probe is blind to an upstream that validates the grant +// before authenticating a public client, the authorization-endpoint probe to +// one that renders its unknown-client error with 200 or a redirect, and +// private_key_jwt clients are never probed. For those, the operator retires +// the registration with Request.RotationKey. // // The check runs only here, so a registration the upstream drops while the // caller keeps using a returned resolution is recovered on the next @@ -476,6 +480,7 @@ func revalidateCachedResolution( "upstream_id", key.UpstreamID, "redirect_uri", redirectURI, "rotation_key", key.RotationKey, + "registration_probe", string(req.RegistrationProbe.effective()), "client_id", cached.ClientID, ) resolution, err := registerClient(ctx, req, redirectURI, scopes) @@ -499,8 +504,8 @@ func revalidateCachedResolution( // logUnverifiedRegistration records why a cached registration is used // without the upstream having confirmed it. A private_key_jwt client is -// never probed, so it logs at Debug on every resolve; any other reason is -// unexpected and logs at Warn. +// never probed and ProbeNone asks for no probe, so those log at Debug on +// every resolve; any other reason is unexpected and logs at Warn. func logUnverifiedRegistration( ctx context.Context, req *Request, key Key, redirectURI string, cached *Resolution, reason error, ) { @@ -510,10 +515,11 @@ func logUnverifiedRegistration( "upstream_id", key.UpstreamID, "redirect_uri", redirectURI, "rotation_key", key.RotationKey, + "registration_probe", string(req.RegistrationProbe.effective()), "client_id", cached.ClientID, "token_endpoint_auth_method", cached.TokenEndpointAuthMethod, } - if errors.Is(reason, errProbeUnsupportedAuthMethod) { + if errors.Is(reason, errProbeUnsupportedAuthMethod) || errors.Is(reason, errProbeDisabled) { level = slog.LevelDebug } else { attrs = append(attrs, probeErrorAttrs(reason)...) @@ -816,6 +822,9 @@ func validateResolveInputs( if req.DiscoveryURL != "" && req.RegistrationEndpoint != "" { return fmt.Errorf("dcr: discovery_url and registration_endpoint are mutually exclusive") } + if err := req.RegistrationProbe.Validate(); err != nil { + return fmt.Errorf("dcr: %w", err) + } if cache == nil { return fmt.Errorf("dcr: credential store is required") } diff --git a/pkg/auth/dcr/resolver_test.go b/pkg/auth/dcr/resolver_test.go index 5df14bd05d..08929ece19 100644 --- a/pkg/auth/dcr/resolver_test.go +++ b/pkg/auth/dcr/resolver_test.go @@ -1211,6 +1211,16 @@ func TestResolveDCRCredentials_RejectsInvalidInputs(t *testing.T) { cache: nil, wantErrSub: "credential store is required", }, + { + name: "unknown registration probe", + req: &Request{ + Issuer: "https://example.com", + RegistrationEndpoint: "https://example.com/register", + RegistrationProbe: "userinfo", + }, + cache: newMemoryDCRStore(t), + wantErrSub: "registration_probe must be one of", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/auth/dcr/validate.go b/pkg/auth/dcr/validate.go index 25d24fd95c..1059eda64e 100644 --- a/pkg/auth/dcr/validate.go +++ b/pkg/auth/dcr/validate.go @@ -8,7 +8,11 @@ import ( "crypto/rand" "errors" "fmt" + "io" + "log/slog" "net/http" + "net/url" + "strings" "time" "golang.org/x/oauth2" @@ -18,11 +22,16 @@ import ( "github.com/stacklok/toolhive/pkg/oauthproto" ) -// registrationProbeTimeout bounds the token-endpoint request that checks a -// cached registration, so an unresponsive upstream delays resolution by at -// most this long before the cached registration is used as-is. +// registrationProbeTimeout bounds the request that checks a cached +// registration, so an unresponsive upstream delays resolution by at most this +// long before the cached registration is used as-is. const registrationProbeTimeout = 10 * time.Second +// authorizationProbeDrainLimit caps how much of an authorization endpoint's +// response body the probe reads before closing it. The body is never +// inspected; draining only lets the connection be reused. +const authorizationProbeDrainLimit = 1 << 20 + // registrationStatus is the upstream's verdict on a cached registration. type registrationStatus int @@ -36,41 +45,66 @@ const ( registrationRejected ) -// errProbeRedirectRefused stops the probe from following a redirect, which -// would re-send the client credentials to another origin. +// errProbeRedirectRefused stops the token-endpoint probe from following a +// redirect, which would re-send the client credentials to another origin. var errProbeRedirectRefused = errors.New("dcr: token endpoint returned a redirect; refusing to follow") // errProbeUnsupportedAuthMethod reports a cached registration whose token // endpoint auth method the probe cannot authenticate with. var errProbeUnsupportedAuthMethod = errors.New("dcr: token endpoint auth method cannot be probed") -// verifyCachedRegistration asks the upstream token endpoint whether it still -// recognises the cached client. An upstream can drop a registration that the -// resolver still caches: RFC 7591 lets client_secret_expires_at be 0 while -// the server evicts idle clients anyway (a ToolHive auth server does exactly -// this), or it can lose its client storage. The browser is the only party -// that sees the resulting authorize error, so the resolver must ask. +// errProbeDisabled reports that the caller configured ProbeNone, so the cached +// registration is used without asking the upstream. +var errProbeDisabled = errors.New("dcr: registration probe disabled by configuration") + +// verifyCachedRegistration asks the upstream whether it still recognises the +// cached client, using the strategy req.RegistrationProbe selects. An +// upstream can drop a registration that the resolver still caches: RFC 7591 +// lets client_secret_expires_at be 0 while the server evicts idle clients +// anyway (a ToolHive auth server does exactly this), or it can lose its +// client storage. The browser is the only party that sees the resulting +// authorize error, so the resolver must ask. // -// The probe redeems a random authorization code with the cached client -// credentials. Servers that authenticate the client before validating the -// grant (as fosite does, for public and confidential clients alike) answer -// invalid_client when the registration is gone and invalid_grant when it is -// alive. No token can be issued for the random code. RFC 6749 §4.1.3 only -// requires confidential clients to authenticate, so for a public client some -// servers validate the code first and answer invalid_grant for any client_id; -// against those servers the probe cannot detect a deleted registration and -// the cached one is kept, as before. Every other outcome, including transport -// failures and private_key_jwt clients (which the probe does not sign for), -// is unverified: callers keep the cached registration so an unreachable -// upstream never blocks startup. +// ProbeTokenEndpoint (the default) is verifyViaTokenEndpoint; +// ProbeAuthorizationEndpoint is verifyViaAuthorizationEndpoint. Each returns +// registrationUnverified for every outcome that is not a verdict (transport +// failures, 5xx, rate limiting, shapes the strategy cannot read), and callers +// then keep the cached registration so an unreachable upstream never blocks +// startup. ProbeNone returns registrationUnverified with errProbeDisabled +// without contacting the upstream. // // The returned error is non-nil only for an unverified status and says why; -// it never carries the token endpoint's response body (see probeErrorAttrs). +// it never carries the upstream's response body (see probeErrorAttrs). +func verifyCachedRegistration(ctx context.Context, req *Request, cached *Resolution) (registrationStatus, error) { + switch probe := req.RegistrationProbe.effective(); probe { + case ProbeTokenEndpoint: + return verifyViaTokenEndpoint(ctx, req, cached) + case ProbeAuthorizationEndpoint: + return verifyViaAuthorizationEndpoint(ctx, req, cached) + case ProbeNone: + return registrationUnverified, errProbeDisabled + default: + // validateResolveInputs rejects unknown values before resolution starts. + return registrationUnverified, fmt.Errorf("dcr: unknown registration probe %q", string(probe)) + } +} + +// verifyViaTokenEndpoint redeems a random authorization code with the cached +// client credentials. Servers that authenticate the client before validating +// the grant (as fosite does, for public and confidential clients alike) +// answer invalid_client when the registration is gone and invalid_grant when +// it is alive. No token can be issued for the random code. RFC 6749 §4.1.3 +// only requires confidential clients to authenticate, so for a public client +// some servers validate the code first and answer invalid_grant for any +// client_id; against those servers this probe cannot detect a deleted +// registration (ProbeAuthorizationEndpoint can, when the server follows RFC +// 6749 §4.1.2.1). private_key_jwt clients are not probed: the probe does not +// sign for them. // // The configured token endpoint wins over the cached one, mirroring // applyExplicitEndpointOverrides, so a tampered cached endpoint cannot // receive the client secret when the operator configured the endpoint. -func verifyCachedRegistration(ctx context.Context, req *Request, cached *Resolution) (registrationStatus, error) { +func verifyViaTokenEndpoint(ctx context.Context, req *Request, cached *Resolution) (registrationStatus, error) { authStyle, ok := probeAuthStyle(cached) if !ok { return registrationUnverified, errProbeUnsupportedAuthMethod @@ -81,10 +115,14 @@ func verifyCachedRegistration(ctx context.Context, req *Request, cached *Resolut tokenEndpoint = cached.TokenEndpoint operatorSupplied = networking.AuthorityMatchesAny(tokenEndpoint, req.DiscoveryURL, req.RegistrationEndpoint) } - client, err := newProbeHTTPClient(req, tokenEndpoint, !operatorSupplied) + client, err := newProbeHTTPClient(req, tokenEndpoint, "token_endpoint", !operatorSupplied) if err != nil { return registrationUnverified, fmt.Errorf("dcr: build token endpoint client: %w", err) } + // The request carries client credentials; never re-send them to another origin. + client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + return errProbeRedirectRefused + } clientSecret := cached.ClientSecret if cached.TokenEndpointAuthMethod == oauthproto.TokenEndpointAuthMethodNone { @@ -108,13 +146,128 @@ func verifyCachedRegistration(ctx context.Context, req *Request, cached *Resolut return status, err } +// verifyViaAuthorizationEndpoint sends an authorization request for the cached +// client and reads the first response without following redirects. Per RFC +// 6749 §4.1.2.1 the server MUST NOT redirect when the client_id or +// redirect_uri is invalid and informs the user directly instead, so a direct +// 4xx is the signal that the registration is gone, while any 2xx (a login or +// consent page) or 3xx (to the IdP, or back to redirect_uri with an error +// such as invalid_scope) shows the client is known. The request carries no +// credentials: only the public client_id, the registered redirect_uri, the +// configured scopes and a throwaway state and PKCE challenge, so the server +// records at most an abandoned authorization request. +// +// Not every 4xx is a verdict: 404 and 405 describe the endpoint rather than +// the client, and 429 is rate limiting. Those, 5xx and transport failures are +// unverified. A server that renders its unknown-client error with 200, or +// redirects to its own error page, reads as alive; the cached registration is +// then kept exactly as without a probe. The response body is never read for +// content or logged. +// +// The configured authorization endpoint wins over the cached one, mirroring +// verifyViaTokenEndpoint. +func verifyViaAuthorizationEndpoint(ctx context.Context, req *Request, cached *Resolution) (registrationStatus, error) { + authzEndpoint, operatorSupplied := req.AuthorizationEndpoint, true + if authzEndpoint == "" { + authzEndpoint = cached.AuthorizationEndpoint + operatorSupplied = networking.AuthorityMatchesAny(authzEndpoint, req.DiscoveryURL, req.RegistrationEndpoint) + } + client, err := newProbeHTTPClient(req, authzEndpoint, "authorization_endpoint", !operatorSupplied) + if err != nil { + return registrationUnverified, fmt.Errorf("dcr: build authorization endpoint client: %w", err) + } + // A redirect is the "known client" answer; read it, do not follow it. + client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + } + + redirectURI := cached.RedirectURI + if redirectURI == "" { + if redirectURI, err = resolveUpstreamRedirectURI(req.RedirectURI, req.Issuer); err != nil { + return registrationUnverified, fmt.Errorf("dcr: authorization endpoint probe: %w", err) + } + } + probeURL, err := authorizationProbeURL(authzEndpoint, cached.ClientID, redirectURI, req.Scopes) + if err != nil { + return registrationUnverified, err + } + + probeCtx, cancel := context.WithTimeout(ctx, registrationProbeTimeout) + defer cancel() + httpReq, err := http.NewRequestWithContext(probeCtx, http.MethodGet, probeURL, nil) + if err != nil { + return registrationUnverified, fmt.Errorf("dcr: authorization endpoint probe: %w", err) + } + resp, err := client.Do(httpReq) + if err != nil { + return registrationUnverified, fmt.Errorf("dcr: authorization endpoint probe: %w", err) + } + defer func() { + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, authorizationProbeDrainLimit)) + _ = resp.Body.Close() + }() + + status := classifyAuthorizationProbe(resp.StatusCode) + //nolint:gosec // G706: client_id is public metadata per RFC 7591. + slog.Debug("dcr: authorization endpoint probe answered", + "client_id", cached.ClientID, + "status", resp.StatusCode, + "registration_live", status == registrationLive, + "registration_rejected", status == registrationRejected, + ) + if status != registrationUnverified { + return status, nil + } + return status, fmt.Errorf("dcr: authorization endpoint answered HTTP %d", resp.StatusCode) +} + +// authorizationProbeURL builds the probe's authorization request URL, +// preserving any query the configured endpoint already carries (some +// providers key tenants that way). +func authorizationProbeURL(endpoint, clientID, redirectURI string, scopes []string) (string, error) { + u, err := url.Parse(endpoint) + if err != nil { + return "", fmt.Errorf("dcr: parse authorization endpoint: %w", err) + } + q := u.Query() + q.Set("response_type", "code") + q.Set("client_id", clientID) + q.Set("redirect_uri", redirectURI) + q.Set("state", rand.Text()) + q.Set("code_challenge", oauth2.S256ChallengeFromVerifier(rand.Text()+rand.Text())) + q.Set("code_challenge_method", "S256") + if len(scopes) > 0 { + q.Set("scope", strings.Join(scopes, " ")) + } + u.RawQuery = q.Encode() + return u.String(), nil +} + +// classifyAuthorizationProbe maps the authorization endpoint's first response +// to a registrationStatus; see verifyViaAuthorizationEndpoint for the rules. +func classifyAuthorizationProbe(statusCode int) registrationStatus { + switch { + case statusCode >= http.StatusOK && statusCode < http.StatusBadRequest: + return registrationLive + case statusCode == http.StatusNotFound, + statusCode == http.StatusMethodNotAllowed, + statusCode == http.StatusTooManyRequests: + return registrationUnverified + case statusCode >= http.StatusBadRequest && statusCode < http.StatusInternalServerError: + return registrationRejected + default: + return registrationUnverified + } +} + // probeErrorAttrs renders why a probe was unverified as log attributes. An // OAuth error response is reduced to its HTTP status and RFC 6749 error // code: oauth2.RetrieveError's message includes the server's // error_description, or its raw response body when there is no error code, // and none of that belongs in our logs. Other errors (transport failures, -// refused redirects, an unusable endpoint) originate on our side and go -// through SanitizeErrorForLog. +// refused redirects, an unusable endpoint, a non-verdict status from the +// authorization endpoint) originate on our side and go through +// SanitizeErrorForLog. func probeErrorAttrs(err error) []any { if retrieveErr, ok := errors.AsType[*oauth2.RetrieveError](err); ok { attrs := []any{"error_code", retrieveErr.ErrorCode} @@ -143,30 +296,24 @@ func probeAuthStyle(cached *Resolution) (oauth2.AuthStyle, bool) { } } -// newProbeHTTPClient builds the guarded client for the probe. It reuses the -// resolver's SSRF policy (newGuardedDCRClient) and refuses redirects, because -// the request carries client credentials. -func newProbeHTTPClient(req *Request, tokenEndpoint string, serverSupplied bool) (*http.Client, error) { - if err := validateUpstreamEndpointURL(tokenEndpoint, "token_endpoint"); err != nil { +// newProbeHTTPClient builds the guarded client for a probe against endpoint, +// validated under label. It reuses the resolver's SSRF policy +// (newGuardedDCRClient); callers set CheckRedirect for their strategy. +func newProbeHTTPClient(req *Request, endpoint, label string, serverSupplied bool) (*http.Client, error) { + if err := validateUpstreamEndpointURL(endpoint, label); err != nil { return nil, err } - host, err := hostFromURL(tokenEndpoint) + host, err := hostFromURL(endpoint) if err != nil { return nil, err } - client, err := newGuardedDCRClient(host, req.AllowPrivateIPs, req.ServerSuppliedEndpoints || serverSupplied, req.CAFilePath) - if err != nil { - return nil, err - } - client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { - return errProbeRedirectRefused - } - return client, nil + return newGuardedDCRClient(host, req.AllowPrivateIPs, req.ServerSuppliedEndpoints || serverSupplied, req.CAFilePath) } -// classifyProbe turns the probe's exchange error into a registrationStatus. -// Only a structured RFC 6749 error code is a verdict; transient shapes (5xx, -// 429, non-OAuth bodies) and transport errors are unverified. +// classifyProbe turns the token-endpoint probe's exchange error into a +// registrationStatus. Only a structured RFC 6749 error code is a verdict; +// transient shapes (5xx, 429, non-OAuth bodies) and transport errors are +// unverified. func classifyProbe(err error) registrationStatus { if err == nil { // The upstream issued tokens for a random code; the client is diff --git a/pkg/auth/dcr/validate_test.go b/pkg/auth/dcr/validate_test.go index d75907c8dd..b9ccac64c7 100644 --- a/pkg/auth/dcr/validate_test.go +++ b/pkg/auth/dcr/validate_test.go @@ -23,15 +23,20 @@ import ( // probeUpstream is an authorization server double for cache-hit // verification: RFC 8414 metadata, a registration endpoint that mints -// "fresh-client-id", and a token endpoint whose answer each test chooses. +// "fresh-client-id", a token endpoint whose answer each test chooses, and an +// authorization endpoint whose answer a test sets with setAuthorize (404 +// until then). type probeUpstream struct { *httptest.Server registrations atomic.Int32 tokenRequests atomic.Int32 + authzRequests atomic.Int32 mu sync.Mutex lastToken *http.Request lastForm url.Values + lastAuthz *http.Request + authorize http.HandlerFunc } func newProbeUpstream(t *testing.T, token http.HandlerFunc, registerStatus int) *probeUpstream { @@ -72,11 +77,30 @@ func newProbeUpstream(t *testing.T, token http.HandlerFunc, registerStatus int) u.mu.Unlock() token(w, r) }) + mux.HandleFunc("/authorize", func(w http.ResponseWriter, r *http.Request) { + u.authzRequests.Add(1) + u.mu.Lock() + u.lastAuthz = r + authorize := u.authorize + u.mu.Unlock() + if authorize == nil { + w.WriteHeader(http.StatusNotFound) + return + } + authorize(w, r) + }) u.Server = httptest.NewServer(mux) t.Cleanup(u.Close) return u } +// setAuthorize chooses the authorization endpoint's answer. +func (u *probeUpstream) setAuthorize(h http.HandlerFunc) { + u.mu.Lock() + defer u.mu.Unlock() + u.authorize = h +} + // tokenRequest returns the last token request and its parsed form. func (u *probeUpstream) tokenRequest() (*http.Request, url.Values) { u.mu.Lock() @@ -84,6 +108,13 @@ func (u *probeUpstream) tokenRequest() (*http.Request, url.Values) { return u.lastToken, u.lastForm } +// authorizeRequest returns the last authorization request. +func (u *probeUpstream) authorizeRequest() *http.Request { + u.mu.Lock() + defer u.mu.Unlock() + return u.lastAuthz +} + func oauthErrorResponse(status int, code string) http.HandlerFunc { return func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -220,6 +251,7 @@ func TestResolveCredentials_CacheHitVerifiesRegistration(t *testing.T) { assert.Equal(t, tt.wantRegistrations, upstream.registrations.Load(), "registrations") assert.Equal(t, tt.wantTokenRequests, upstream.tokenRequests.Load(), "token requests") + assert.Zero(t, upstream.authzRequests.Load(), "the default probe uses the token endpoint only") if tt.wantErr { require.Error(t, err) return @@ -429,6 +461,278 @@ func TestResolveCredentials_RejectedRegistrationReplacedOncePerProcess(t *testin "the rejected registration must be replaced by exactly one new client") } +// preloadForAuthorizationProbe stores a confidential registration for +// upstream and returns a request that selects the authorization-endpoint +// probe for it. +func preloadForAuthorizationProbe(t *testing.T, cache CredentialStore, upstream *probeUpstream) *Request { + t.Helper() + req := preloadCachedRegistration(t, cache, upstream, "client_secret_basic") + req.RegistrationProbe = ProbeAuthorizationEndpoint + return req +} + +func TestResolveCredentials_CacheHitAuthorizationEndpointProbe(t *testing.T) { + t.Parallel() + + var redirectFollowed atomic.Int32 + idp := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + redirectFollowed.Add(1) + })) + t.Cleanup(idp.Close) + // Cleanups run after the parallel subtests finish. + t.Cleanup(func() { + assert.Zero(t, redirectFollowed.Load(), "the probe must not follow the redirect to the identity provider") + }) + + statusOnly := func(status int) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(status) } + } + tests := []struct { + name string + authorize http.HandlerFunc + registerStatus int + wantClientID string + wantErr bool + wantRegistrations int32 + }{ + { + name: "login page keeps the registration", + authorize: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html") + _, _ = w.Write([]byte("sign in")) + }, + wantClientID: "cached-client-id", + }, + { + name: "redirect to the identity provider keeps the registration", + authorize: func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, idp.URL+"/login", http.StatusFound) + }, + wantClientID: "cached-client-id", + }, + { + name: "redirect back to redirect_uri with an error keeps the registration", + authorize: func(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query() + http.Redirect(w, r, q.Get("redirect_uri")+"?error=invalid_scope&state="+q.Get("state"), http.StatusFound) + }, + wantClientID: "cached-client-id", + }, + { + name: "direct 400 replaces the registration", + authorize: oauthErrorResponse(http.StatusBadRequest, "invalid_request"), + wantClientID: "fresh-client-id", wantRegistrations: 1, + }, + { + name: "direct 401 replaces the registration", + authorize: oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), + wantClientID: "fresh-client-id", wantRegistrations: 1, + }, + { + name: "direct 403 replaces the registration", + authorize: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte("Unknown client")) + }, + wantClientID: "fresh-client-id", wantRegistrations: 1, + }, + {name: "404 keeps the registration", authorize: statusOnly(http.StatusNotFound), wantClientID: "cached-client-id"}, + {name: "405 keeps the registration", authorize: statusOnly(http.StatusMethodNotAllowed), wantClientID: "cached-client-id"}, + {name: "429 keeps the registration", authorize: statusOnly(http.StatusTooManyRequests), wantClientID: "cached-client-id"}, + {name: "500 keeps the registration", authorize: statusOnly(http.StatusInternalServerError), wantClientID: "cached-client-id"}, + { + name: "failed re-registration is an error", + authorize: oauthErrorResponse(http.StatusBadRequest, "invalid_request"), + registerStatus: http.StatusInternalServerError, + wantErr: true, wantRegistrations: 1, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + registerStatus := tt.registerStatus + if registerStatus == 0 { + registerStatus = http.StatusCreated + } + // The token endpoint would reject the cached client; the + // authorization-endpoint probe must never ask it. + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), registerStatus) + upstream.setAuthorize(tt.authorize) + cache := newMemoryDCRStore(t) + req := preloadForAuthorizationProbe(t, cache, upstream) + + got, err := ResolveCredentials(context.Background(), req, cache) + + assert.Equal(t, int32(1), upstream.authzRequests.Load(), "authorization requests") + assert.Zero(t, upstream.tokenRequests.Load(), "token requests") + assert.Equal(t, tt.wantRegistrations, upstream.registrations.Load(), "registrations") + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantClientID, got.ClientID) + + stored, ok, err := cache.Get(context.Background(), Key{ + Issuer: upstream.URL, UpstreamID: upstream.URL, + RedirectURI: upstream.URL + "/oauth/callback", ScopesHash: storage.ScopesHash([]string{"openid"}), + }) + require.NoError(t, err) + require.True(t, ok) + assert.Equal(t, tt.wantClientID, stored.ClientID, "cache must hold the returned registration") + + r := upstream.authorizeRequest() + require.NotNil(t, r) + assert.Equal(t, http.MethodGet, r.Method) + assert.Empty(t, r.Header.Get("Authorization"), "the probe carries no client credentials") + q := r.URL.Query() + assert.Equal(t, "code", q.Get("response_type")) + assert.Equal(t, "cached-client-id", q.Get("client_id")) + assert.Equal(t, upstream.URL+"/oauth/callback", q.Get("redirect_uri")) + assert.Equal(t, "openid", q.Get("scope")) + assert.Equal(t, "S256", q.Get("code_challenge_method")) + assert.NotEmpty(t, q.Get("code_challenge")) + assert.NotEmpty(t, q.Get("state")) + assert.Empty(t, q.Get("client_secret")) + }) + } +} + +// TestResolveCredentials_CacheHitProbesConfiguredAuthorizationEndpoint pins +// that the probe goes to the operator-configured authorization endpoint, not +// to the endpoint recorded in the cached row. +func TestResolveCredentials_CacheHitProbesConfiguredAuthorizationEndpoint(t *testing.T) { + t.Parallel() + + var decoyRequests atomic.Int32 + decoy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + decoyRequests.Add(1) + w.WriteHeader(http.StatusBadRequest) + })) + t.Cleanup(decoy.Close) + + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + upstream.setAuthorize(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, upstream.URL+"/login", http.StatusFound) + }) + cache := newMemoryDCRStore(t) + key := Key{ + Issuer: upstream.URL, UpstreamID: upstream.URL, + RedirectURI: upstream.URL + "/oauth/callback", ScopesHash: storage.ScopesHash([]string{"openid"}), + } + _, err := cache.PutIfAbsent(context.Background(), key, &Resolution{ + ClientID: "cached-client-id", + ClientSecret: "cached-client-secret", + AuthorizationEndpoint: decoy.URL + "/authorize", + TokenEndpoint: upstream.URL + "/token", + TokenEndpointAuthMethod: "client_secret_basic", + }) + require.NoError(t, err) + + got, err := ResolveCredentials(context.Background(), &Request{ + Issuer: upstream.URL, + Scopes: []string{"openid"}, + DiscoveryURL: upstream.URL + "/.well-known/oauth-authorization-server", + AuthorizationEndpoint: upstream.URL + "/authorize", + RegistrationProbe: ProbeAuthorizationEndpoint, + }, cache) + require.NoError(t, err) + assert.Equal(t, "cached-client-id", got.ClientID) + assert.Equal(t, int32(1), upstream.authzRequests.Load()) + assert.Zero(t, decoyRequests.Load(), "the cached authorization endpoint must not receive the probe") +} + +func TestResolveCredentials_AuthorizationProbeTransportErrorKeepsRegistration(t *testing.T) { + t.Parallel() + + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + unreachable := httptest.NewServer(http.NotFoundHandler()) + unreachableURL := unreachable.URL + unreachable.Close() + cache := newMemoryDCRStore(t) + req := preloadForAuthorizationProbe(t, cache, upstream) + req.AuthorizationEndpoint = unreachableURL + "/authorize" + + got, err := ResolveCredentials(context.Background(), req, cache) + require.NoError(t, err) + assert.Equal(t, "cached-client-id", got.ClientID) + assert.Zero(t, upstream.registrations.Load()) + assert.Zero(t, upstream.tokenRequests.Load()) +} + +// TestResolveCredentials_ProbeNoneSkipsUpstream pins that ProbeNone returns +// the cached registration without contacting the upstream, even when both +// endpoints would reject the cached client. +func TestResolveCredentials_ProbeNoneSkipsUpstream(t *testing.T) { + t.Parallel() + + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + upstream.setAuthorize(oauthErrorResponse(http.StatusBadRequest, "invalid_request")) + cache := newMemoryDCRStore(t) + req := preloadCachedRegistration(t, cache, upstream, "client_secret_basic") + req.RegistrationProbe = ProbeNone + + got, err := ResolveCredentials(context.Background(), req, cache) + require.NoError(t, err) + assert.Equal(t, "cached-client-id", got.ClientID) + assert.Zero(t, upstream.tokenRequests.Load()) + assert.Zero(t, upstream.authzRequests.Load()) + assert.Zero(t, upstream.registrations.Load()) +} + +func TestClassifyAuthorizationProbe(t *testing.T) { + t.Parallel() + + tests := []struct { + status int + want registrationStatus + }{ + {http.StatusOK, registrationLive}, + {http.StatusSeeOther, registrationLive}, + {http.StatusBadRequest, registrationRejected}, + {http.StatusUnprocessableEntity, registrationRejected}, + {http.StatusNotFound, registrationUnverified}, + {http.StatusMethodNotAllowed, registrationUnverified}, + {http.StatusTooManyRequests, registrationUnverified}, + {http.StatusBadGateway, registrationUnverified}, + {http.StatusContinue, registrationUnverified}, + } + for _, tt := range tests { + t.Run(http.StatusText(tt.status), func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, classifyAuthorizationProbe(tt.status)) + }) + } +} + +// TestAuthorizationProbeErrorAttrs_OmitsServerResponse pins that an +// inconclusive authorization-endpoint answer is logged by status only. +func TestAuthorizationProbeErrorAttrs_OmitsServerResponse(t *testing.T) { + t.Parallel() + + const secretish = "do-not-log-this-response-text" + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + upstream.setAuthorize(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(secretish)) + }) + req := preloadForAuthorizationProbe(t, newMemoryDCRStore(t), upstream) + + status, reason := verifyCachedRegistration(context.Background(), req, &Resolution{ + ClientID: "cached-client-id", + ClientSecret: "cached-client-secret", + AuthorizationEndpoint: upstream.URL + "/authorize", + TokenEndpoint: upstream.URL + "/token", + TokenEndpointAuthMethod: "client_secret_basic", + RedirectURI: upstream.URL + "/oauth/callback", + }) + require.Equal(t, registrationUnverified, status) + require.Error(t, reason) + assert.NotContains(t, reason.Error(), secretish) + assert.Equal(t, []any{"error", "dcr: authorization endpoint answered HTTP 429"}, probeErrorAttrs(reason)) +} + func TestProbeErrorAttrs_OmitsServerResponse(t *testing.T) { t.Parallel() @@ -477,3 +781,111 @@ func TestProbeErrorAttrs_OmitsServerResponse(t *testing.T) { }) } } + +// TestVerifyCachedRegistration_InvalidProbeEndpointIsUnverified pins that a +// configured probe endpoint the resolver's URL policy rejects keeps the cached +// registration (unverified) instead of failing the resolve or probing anyway. +func TestVerifyCachedRegistration_InvalidProbeEndpointIsUnverified(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + probe RegistrationProbe + set func(*Request) + }{ + { + name: "token endpoint with a non-https scheme", + probe: ProbeTokenEndpoint, + set: func(r *Request) { r.TokenEndpoint = "ftp://upstream.example.com/token" }, + }, + { + name: "authorization endpoint with a non-https scheme", + probe: ProbeAuthorizationEndpoint, + set: func(r *Request) { r.AuthorizationEndpoint = "ftp://upstream.example.com/authorize" }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + upstream.setAuthorize(oauthErrorResponse(http.StatusBadRequest, "invalid_request")) + req := preloadCachedRegistration(t, newMemoryDCRStore(t), upstream, "client_secret_basic") + req.RegistrationProbe = tt.probe + tt.set(req) + + status, reason := verifyCachedRegistration(context.Background(), req, &Resolution{ + ClientID: "cached-client-id", + ClientSecret: "cached-client-secret", + AuthorizationEndpoint: upstream.URL + "/authorize", + TokenEndpoint: upstream.URL + "/token", + TokenEndpointAuthMethod: "client_secret_basic", + RedirectURI: upstream.URL + "/oauth/callback", + }) + assert.Equal(t, registrationUnverified, status) + require.Error(t, reason) + assert.Zero(t, upstream.tokenRequests.Load(), "the cached token endpoint must not be probed instead") + assert.Zero(t, upstream.authzRequests.Load(), "the cached authorization endpoint must not be probed instead") + }) + } +} + +// TestVerifyCachedRegistration_AuthorizationProbeRedirectURIFallback pins +// that a cached row without a redirect URI probes with the resolver's +// redirect URI (configured, else derived from the local issuer), and that an +// underivable one is reported rather than probed with an empty value. +func TestVerifyCachedRegistration_AuthorizationProbeRedirectURIFallback(t *testing.T) { + t.Parallel() + + cached := func(upstream *probeUpstream) *Resolution { + return &Resolution{ + ClientID: "cached-client-id", + AuthorizationEndpoint: upstream.URL + "/authorize", + TokenEndpoint: upstream.URL + "/token", + TokenEndpointAuthMethod: "none", + } + } + + t.Run("configured redirect URI is used", func(t *testing.T) { + t.Parallel() + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + upstream.setAuthorize(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) }) + req := preloadForAuthorizationProbe(t, newMemoryDCRStore(t), upstream) + req.RedirectURI = "https://proxy.example.com/oauth/callback" + + status, reason := verifyCachedRegistration(context.Background(), req, cached(upstream)) + require.NoError(t, reason) + assert.Equal(t, registrationLive, status) + r := upstream.authorizeRequest() + require.NotNil(t, r) + assert.Equal(t, "https://proxy.example.com/oauth/callback", r.URL.Query().Get("redirect_uri")) + }) + + t.Run("underivable redirect URI is an error", func(t *testing.T) { + t.Parallel() + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + upstream.setAuthorize(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) }) + req := preloadForAuthorizationProbe(t, newMemoryDCRStore(t), upstream) + req.Issuer = "not a url" + + status, reason := verifyCachedRegistration(context.Background(), req, cached(upstream)) + assert.Equal(t, registrationUnverified, status) + require.Error(t, reason) + assert.Zero(t, upstream.authzRequests.Load(), "no probe without a redirect URI") + }) +} + +func TestAuthorizationProbeURL_RejectsUnparsableEndpoint(t *testing.T) { + t.Parallel() + + _, err := authorizationProbeURL("://missing-scheme", "client", "https://proxy.example.com/oauth/callback", nil) + require.Error(t, err) +} + +// TestClassifyProbe_SuccessfulExchangeIsLive pins the token-probe verdict +// when the upstream issues tokens for the random code: the client +// authenticated, so the registration is live. +func TestClassifyProbe_SuccessfulExchangeIsLive(t *testing.T) { + t.Parallel() + + assert.Equal(t, registrationLive, classifyProbe(nil)) +} diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index 13a59f13f8..f359a1a735 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -15,6 +15,7 @@ import ( "strings" "time" + "github.com/stacklok/toolhive/pkg/auth/dcr" oauthserver "github.com/stacklok/toolhive/pkg/authserver/server" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" "github.com/stacklok/toolhive/pkg/authserver/server/handlers" @@ -914,17 +915,29 @@ type DCRUpstreamConfig struct { // [A-Za-z0-9._-]) that becomes part of the cached registration's identity // without being sent to the upstream. Changing it makes the next start // register a fresh client instead of reusing the cached one. At start the - // resolver checks the cached client at the upstream's token endpoint and - // replaces it by itself when the upstream answers invalid_client. Use - // RotationKey when the upstream has dropped the registration and that - // check cannot tell (an upstream that answers a dropped public client - // like a live one, or a private_key_jwt client), so the rejection is only - // visible in the user's browser, or when a registration must be rotated - // on request. The row written under the previous value is left in place. Upstream refresh tokens - // bound to the previous client_id stop refreshing once the client - // changes; affected users sign in again. + // resolver checks the cached client at the upstream (see RegistrationProbe) + // and replaces it by itself when the upstream rejects it. Use RotationKey + // when the upstream has dropped the registration and the configured probe + // cannot tell, so the rejection may not be visible to the resolver, or when + // a registration must be rotated on request. The row written under the + // previous value is left in place. Upstream refresh tokens bound to the + // previous client_id stop refreshing once the client changes; affected + // users sign in again. RotationKey string `json:"rotation_key,omitempty" yaml:"rotation_key,omitempty"` + // RegistrationProbe selects how the resolver checks a cached registration + // against the upstream at start. "tokenEndpoint" (the default) redeems a + // random authorization code with the cached client and reads + // invalid_client as "registration gone"; it decides only for upstreams + // that authenticate the client before validating the grant. + // "authorizationEndpoint" sends an authorization request for the cached + // client without following redirects and reads a direct 4xx as "gone" + // (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers + // invalid_grant for any client_id; do not use it behind bot protection + // that answers 4xx to non-browser requests. "none" skips the check; the + // registration is then retired only through RotationKey. + RegistrationProbe string `json:"registration_probe,omitempty" yaml:"registration_probe,omitempty"` + // InitialAccessTokenFile is the path to a file containing the RFC 7591 // initial access token presented to the registration endpoint. Mutually // exclusive with InitialAccessTokenEnvVar. Both may be omitted for open @@ -1658,6 +1671,12 @@ func (c *DCRUpstreamConfig) Validate() error { return fmt.Errorf("dcr_config: rotation_key must be 1-64 characters from [A-Za-z0-9._-]") } + // Mirrors the CRD's Enum marker on registrationProbe; the resolver owns + // the set of strategies. + if err := dcr.RegistrationProbe(c.RegistrationProbe).Validate(); err != nil { + return fmt.Errorf("dcr_config: %w", err) + } + return nil } diff --git a/pkg/authserver/config_test.go b/pkg/authserver/config_test.go index fa762f44d4..af6c713119 100644 --- a/pkg/authserver/config_test.go +++ b/pkg/authserver/config_test.go @@ -1318,6 +1318,29 @@ func TestDCRUpstreamConfigValidate(t *testing.T) { wantErr: true, errMsg: "rotation_key must be 1-64 characters", }, + { + name: "registration_probe authorizationEndpoint accepts", + config: DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + RegistrationProbe: "authorizationEndpoint", + }, + }, + { + name: "registration_probe none accepts", + config: DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + RegistrationProbe: "none", + }, + }, + { + name: "unknown registration_probe rejects", + config: DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + RegistrationProbe: "userinfo", + }, + wantErr: true, + errMsg: "registration_probe must be one of", + }, { name: "both initial_access_token_file and initial_access_token_env_var rejects", config: DCRUpstreamConfig{ diff --git a/pkg/authserver/runner/dcr_adapter.go b/pkg/authserver/runner/dcr_adapter.go index da9a00adba..7099ddfe3c 100644 --- a/pkg/authserver/runner/dcr_adapter.go +++ b/pkg/authserver/runner/dcr_adapter.go @@ -88,6 +88,7 @@ func newDCRRequest(rc *authserver.UpstreamRunConfig, localIssuer string) (*dcr.R Issuer: localIssuer, RedirectURI: redirectURI, Scopes: scopes, DiscoveryURL: discoveryURL, RegistrationEndpoint: dcrConfig.RegistrationEndpoint, RotationKey: dcrConfig.RotationKey, + RegistrationProbe: dcr.RegistrationProbe(dcrConfig.RegistrationProbe), AuthorizationEndpoint: authorizationEndpoint, TokenEndpoint: tokenEndpoint, InitialAccessToken: initialAccessToken, CAFilePath: caFilePath, AllowPrivateIPs: allowPrivateIPs, }, nil diff --git a/pkg/authserver/runner/dcr_adapter_test.go b/pkg/authserver/runner/dcr_adapter_test.go index f2201f8817..0c267f9d7e 100644 --- a/pkg/authserver/runner/dcr_adapter_test.go +++ b/pkg/authserver/runner/dcr_adapter_test.go @@ -284,6 +284,7 @@ func TestNewDCRRequest(t *testing.T) { wantAllowPrivateIPs bool wantCAFilePath string wantRotationKey string + wantRegistrationProbe dcr.RegistrationProbe }{ { name: "discovery_url branch resolves file-based initial access token", @@ -348,6 +349,23 @@ func TestNewDCRRequest(t *testing.T) { wantRegistration: "https://idp.example.com/register", wantRotationKey: "2026-10", }, + { + // Pins the RegistrationProbe copy onto dcr.Request: without it an + // upstream configured for the authorization-endpoint probe would + // silently be probed at its token endpoint. + name: "registration_probe propagates from run-config", + rc: &authserver.OAuth2UpstreamRunConfig{ + Scopes: []string{"openid"}, + DCRConfig: &authserver.DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + RegistrationProbe: "authorizationEndpoint", + }, + }, + localIssuer: "https://thv.example.com", + wantIssuer: "https://thv.example.com", + wantRegistration: "https://idp.example.com/register", + wantRegistrationProbe: dcr.ProbeAuthorizationEndpoint, + }, { name: "nil run-config rejected", rc: nil, @@ -373,6 +391,7 @@ func TestNewDCRRequest(t *testing.T) { assert.Equal(t, tc.wantAllowPrivateIPs, req.AllowPrivateIPs) assert.Equal(t, tc.wantCAFilePath, req.CAFilePath) assert.Equal(t, tc.wantRotationKey, req.RotationKey) + assert.Equal(t, tc.wantRegistrationProbe, req.RegistrationProbe) }) } } diff --git a/pkg/authserver/storage/types.go b/pkg/authserver/storage/types.go index 3e2776bdd2..1a9824ec44 100644 --- a/pkg/authserver/storage/types.go +++ b/pkg/authserver/storage/types.go @@ -240,7 +240,7 @@ type DCRKey struct { // cache identity but carries no OAuth meaning and is never sent to the // upstream. Changing it makes the resolver miss the cache and register a // fresh client. It is how an operator retires a registration the upstream - // has dropped when the resolver's own token-endpoint check cannot tell + // has dropped when the resolver's own registration probe cannot tell // (see authserver.DCRUpstreamConfig.RotationKey). // Empty for every row written before the field existed; backends MUST // encode an empty value exactly as they encoded a key without the field diff --git a/sdk/go/client/oas_json_gen.go b/sdk/go/client/oas_json_gen.go index 58e49caa3e..7c88798f06 100644 --- a/sdk/go/client/oas_json_gen.go +++ b/sdk/go/client/oas_json_gen.go @@ -1051,6 +1051,12 @@ func (s *AuthenticationDCRUpstreamConfig) encodeFields(e *jx.Encoder) { s.RegistrationEndpoint.Encode(e) } } + { + if s.RegistrationProbe.Set { + e.FieldStart("registration_probe") + s.RegistrationProbe.Encode(e) + } + } { if s.RotationKey.Set { e.FieldStart("rotation_key") @@ -1071,14 +1077,15 @@ func (s *AuthenticationDCRUpstreamConfig) encodeFields(e *jx.Encoder) { } } -var jsonFieldsNameOfAuthenticationDCRUpstreamConfig = [7]string{ +var jsonFieldsNameOfAuthenticationDCRUpstreamConfig = [8]string{ 0: "discovery_url", 1: "initial_access_token_env_var", 2: "initial_access_token_file", 3: "registration_endpoint", - 4: "rotation_key", - 5: "software_id", - 6: "software_statement", + 4: "registration_probe", + 5: "rotation_key", + 6: "software_id", + 7: "software_statement", } // Decode decodes AuthenticationDCRUpstreamConfig from json. @@ -1129,6 +1136,16 @@ func (s *AuthenticationDCRUpstreamConfig) Decode(d *jx.Decoder) error { }(); err != nil { return errors.Wrap(err, "decode field \"registration_endpoint\"") } + case "registration_probe": + if err := func() error { + s.RegistrationProbe.Reset() + if err := s.RegistrationProbe.Decode(d); err != nil { + return err + } + return nil + }(); err != nil { + return errors.Wrap(err, "decode field \"registration_probe\"") + } case "rotation_key": if err := func() error { s.RotationKey.Reset() diff --git a/sdk/go/client/oas_schemas_gen.go b/sdk/go/client/oas_schemas_gen.go index fad1e4db75..546f2710ab 100644 --- a/sdk/go/client/oas_schemas_gen.go +++ b/sdk/go/client/oas_schemas_gen.go @@ -622,19 +622,30 @@ type AuthenticationDCRUpstreamConfig struct { // method falls back to the resolver's default (client_secret_basic). // Mutually exclusive with DiscoveryURL. RegistrationEndpoint OptString `json:"registration_endpoint"` + // RegistrationProbe selects how the resolver checks a cached registration + // against the upstream at start. "tokenEndpoint" (the default) redeems a + // random authorization code with the cached client and reads + // invalid_client as "registration gone"; it decides only for upstreams + // that authenticate the client before validating the grant. + // "authorizationEndpoint" sends an authorization request for the cached + // client without following redirects and reads a direct 4xx as "gone" + // (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers + // invalid_grant for any client_id; do not use it behind bot protection + // that answers 4xx to non-browser requests. "none" skips the check; the + // registration is then retired only through RotationKey. + RegistrationProbe OptString `json:"registration_probe"` // RotationKey is an optional operator-chosen label (1-64 characters from // [A-Za-z0-9._-]) that becomes part of the cached registration's identity // without being sent to the upstream. Changing it makes the next start // register a fresh client instead of reusing the cached one. At start the - // resolver checks the cached client at the upstream's token endpoint and - // replaces it by itself when the upstream answers invalid_client. Use - // RotationKey when the upstream has dropped the registration and that - // check cannot tell (an upstream that answers a dropped public client - // like a live one, or a private_key_jwt client), so the rejection is only - // visible in the user's browser, or when a registration must be rotated - // on request. The row written under the previous value is left in place. Upstream refresh tokens - // bound to the previous client_id stop refreshing once the client - // changes; affected users sign in again. + // resolver checks the cached client at the upstream (see RegistrationProbe) + // and replaces it by itself when the upstream rejects it. Use RotationKey + // when the upstream has dropped the registration and the configured probe + // cannot tell, so the rejection may not be visible to the resolver, or when + // a registration must be rotated on request. The row written under the + // previous value is left in place. Upstream refresh tokens bound to the + // previous client_id stop refreshing once the client changes; affected + // users sign in again. RotationKey OptString `json:"rotation_key"` // SoftwareID is the RFC 7591 "software_id" registration metadata value, // identifying the client software independent of any particular @@ -666,6 +677,11 @@ func (s *AuthenticationDCRUpstreamConfig) GetRegistrationEndpoint() OptString { return s.RegistrationEndpoint } +// GetRegistrationProbe returns the value of RegistrationProbe. +func (s *AuthenticationDCRUpstreamConfig) GetRegistrationProbe() OptString { + return s.RegistrationProbe +} + // GetRotationKey returns the value of RotationKey. func (s *AuthenticationDCRUpstreamConfig) GetRotationKey() OptString { return s.RotationKey @@ -701,6 +717,11 @@ func (s *AuthenticationDCRUpstreamConfig) SetRegistrationEndpoint(val OptString) s.RegistrationEndpoint = val } +// SetRegistrationProbe sets the value of RegistrationProbe. +func (s *AuthenticationDCRUpstreamConfig) SetRegistrationProbe(val OptString) { + s.RegistrationProbe = val +} + // SetRotationKey sets the value of RotationKey. func (s *AuthenticationDCRUpstreamConfig) SetRotationKey(val OptString) { s.RotationKey = val diff --git a/sdk/go/openapi.json b/sdk/go/openapi.json index 746852ed7a..051eefc285 100644 --- a/sdk/go/openapi.json +++ b/sdk/go/openapi.json @@ -231,8 +231,12 @@ "description": "RegistrationEndpoint is the RFC 7591 registration endpoint URL used\ndirectly, bypassing discovery. Because no discovery is performed,\nserver-capability fields (token_endpoint_auth_methods_supported,\nscopes_supported) are unavailable on this code path; the caller is\nexpected to also supply AuthorizationEndpoint, TokenEndpoint, and an\nexplicit Scopes list on the parent OAuth2UpstreamRunConfig. Auth\nmethod falls back to the resolver's default (client_secret_basic).\n\nMutually exclusive with DiscoveryURL.", "type": "string" }, + "registration_probe": { + "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 4xx as \"gone\"\n(RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers\ninvalid_grant for any client_id; do not use it behind bot protection\nthat answers 4xx to non-browser requests. \"none\" skips the check; the\nregistration is then retired only through RotationKey.", + "type": "string" + }, "rotation_key": { - "description": "RotationKey is an optional operator-chosen label (1-64 characters from\n[A-Za-z0-9._-]) that becomes part of the cached registration's identity\nwithout being sent to the upstream. Changing it makes the next start\nregister a fresh client instead of reusing the cached one. At start the\nresolver checks the cached client at the upstream's token endpoint and\nreplaces it by itself when the upstream answers invalid_client. Use\nRotationKey when the upstream has dropped the registration and that\ncheck cannot tell (an upstream that answers a dropped public client\nlike a live one, or a private_key_jwt client), so the rejection is only\nvisible in the user's browser, or when a registration must be rotated\non request. The row written under the previous value is left in place. Upstream refresh tokens\nbound to the previous client_id stop refreshing once the client\nchanges; affected users sign in again.", + "description": "RotationKey is an optional operator-chosen label (1-64 characters from\n[A-Za-z0-9._-]) that becomes part of the cached registration's identity\nwithout being sent to the upstream. Changing it makes the next start\nregister a fresh client instead of reusing the cached one. At start the\nresolver checks the cached client at the upstream (see RegistrationProbe)\nand replaces it by itself when the upstream rejects it. Use RotationKey\nwhen the upstream has dropped the registration and the configured probe\ncannot tell, so the rejection may not be visible to the resolver, or when\na registration must be rotated on request. The row written under the\nprevious value is left in place. Upstream refresh tokens bound to the\nprevious client_id stop refreshing once the client changes; affected\nusers sign in again.", "type": "string" }, "software_id": { diff --git a/sdk/go/openapi.yaml b/sdk/go/openapi.yaml index 7efe19a959..954200a0ab 100644 --- a/sdk/go/openapi.yaml +++ b/sdk/go/openapi.yaml @@ -279,21 +279,34 @@ components: Mutually exclusive with DiscoveryURL. type: string + registration_probe: + description: |- + RegistrationProbe selects how the resolver checks a cached registration + against the upstream at start. "tokenEndpoint" (the default) redeems a + random authorization code with the cached client and reads + invalid_client as "registration gone"; it decides only for upstreams + that authenticate the client before validating the grant. + "authorizationEndpoint" sends an authorization request for the cached + client without following redirects and reads a direct 4xx as "gone" + (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers + invalid_grant for any client_id; do not use it behind bot protection + that answers 4xx to non-browser requests. "none" skips the check; the + registration is then retired only through RotationKey. + type: string rotation_key: description: |- RotationKey is an optional operator-chosen label (1-64 characters from [A-Za-z0-9._-]) that becomes part of the cached registration's identity without being sent to the upstream. Changing it makes the next start register a fresh client instead of reusing the cached one. At start the - resolver checks the cached client at the upstream's token endpoint and - replaces it by itself when the upstream answers invalid_client. Use - RotationKey when the upstream has dropped the registration and that - check cannot tell (an upstream that answers a dropped public client - like a live one, or a private_key_jwt client), so the rejection is only - visible in the user's browser, or when a registration must be rotated - on request. The row written under the previous value is left in place. Upstream refresh tokens - bound to the previous client_id stop refreshing once the client - changes; affected users sign in again. + resolver checks the cached client at the upstream (see RegistrationProbe) + and replaces it by itself when the upstream rejects it. Use RotationKey + when the upstream has dropped the registration and the configured probe + cannot tell, so the rejection may not be visible to the resolver, or when + a registration must be rotated on request. The row written under the + previous value is left in place. Upstream refresh tokens bound to the + previous client_id stop refreshing once the client changes; affected + users sign in again. type: string software_id: description: |- From cf6d46cbfb90bf2f9e73fc7c3879b40b9bec01e6 Mon Sep 17 00:00:00 2001 From: Aron Date: Fri, 16 Oct 2026 14:21:11 +0000 Subject: [PATCH 2/2] Narrow the authorization-endpoint probe to structured OAuth errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6749 §4.1.2.1 only says an invalid client_id or redirect_uri must not be redirected; a direct 4xx on its own does not mean the client is invalid, since a gateway, an IP policy or bot protection in front of the server answers that way for a live client too. The probe now treats a direct answer as a rejection only when the server itself spoke: status 400, 401 or 403 with an RFC 6749 JSON error object whose error is invalid_client, unauthorized_client or invalid_request. A server that had validated client_id and redirect_uri would have delivered the error by redirect, so the direct structured error is the server reporting it could not. HTML pages, bare gateway statuses and other error codes are unverified and keep the cached registration. The probe reads up to 16 KiB of the body for the error code and repeats only a code from a short lower-case alphabet in its error and logs, never the description. Scope the private_key_jwt comments to the token-endpoint probe (the authorization-endpoint probe sends no credentials and covers every auth method) and spell out each probe setting's blind spot in the field docs and the storage arch doc, including that a false rejection re-registers at every start and fails startup when registration also fails. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Aron --- .../v1beta1/mcpexternalauthconfig_types.go | 13 +- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 52 ++++--- ...olhive.stacklok.dev_virtualmcpservers.yaml | 52 ++++--- ...e.stacklok.dev_mcpexternalauthconfigs.yaml | 52 ++++--- ...olhive.stacklok.dev_virtualmcpservers.yaml | 52 ++++--- docs/arch/11-auth-server-storage.md | 2 +- docs/operator/crd-api.md | 2 +- docs/server/docs.go | 2 +- docs/server/swagger.json | 2 +- docs/server/swagger.yaml | 13 +- pkg/auth/dcr/request.go | 20 +-- pkg/auth/dcr/resolver.go | 20 +-- pkg/auth/dcr/validate.go | 115 ++++++++++----- pkg/auth/dcr/validate_test.go | 131 +++++++++++++++--- pkg/authserver/config.go | 13 +- sdk/go/client/oas_schemas_gen.go | 13 +- sdk/go/openapi.json | 2 +- sdk/go/openapi.yaml | 13 +- 18 files changed, 392 insertions(+), 177 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 04ae5a5bae..858d799d04 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -1897,11 +1897,14 @@ type DCRUpstreamConfig struct { // gone"; it decides only for upstreams that authenticate the client // before validating the grant. authorizationEndpoint sends an // authorization request for the cached client without following - // redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - // upstreams whose token endpoint answers invalid_grant for any client_id; - // do not use it behind bot protection that answers 4xx to non-browser - // requests. none skips the check; the registration is then retired only - // through RotationKey. + // redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + // JSON error object (error invalid_client, unauthorized_client or + // invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + // token endpoint answers invalid_grant for any client_id; an HTML page or + // a bare gateway status is not a verdict. A false rejection re-registers + // at every start and, when registration also fails, fails startup. none + // skips the check; the registration is then retired only through + // RotationKey. // +optional // +kubebuilder:validation:Enum=tokenEndpoint;authorizationEndpoint;none RegistrationProbe string `json:"registrationProbe,omitempty"` diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index 3bd97df9fe..74cf032fb1 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -1744,11 +1744,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -2169,11 +2172,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -4720,11 +4726,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -5145,11 +5154,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index bd7baa5724..dbd1c73801 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1631,11 +1631,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -2056,11 +2059,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -6995,11 +7001,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -7420,11 +7429,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index d320aa9a74..e79f661463 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -1747,11 +1747,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -2172,11 +2175,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -4723,11 +4729,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -5148,11 +5157,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index 5c7345cb99..d6e47d8875 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1634,11 +1634,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -2059,11 +2062,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -6998,11 +7004,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint @@ -7423,11 +7432,14 @@ spec: gone"; it decides only for upstreams that authenticate the client before validating the grant. authorizationEndpoint sends an authorization request for the cached client without following - redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for - upstreams whose token endpoint answers invalid_grant for any client_id; - do not use it behind bot protection that answers 4xx to non-browser - requests. none skips the check; the registration is then retired only - through RotationKey. + redirects and reads a direct 400, 401 or 403 that carries an RFC 6749 + JSON error object (error invalid_client, unauthorized_client or + invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose + token endpoint answers invalid_grant for any client_id; an HTML page or + a bare gateway status is not a verdict. A false rejection re-registers + at every start and, when registration also fails, fails startup. none + skips the check; the registration is then retired only through + RotationKey. enum: - tokenEndpoint - authorizationEndpoint diff --git a/docs/arch/11-auth-server-storage.md b/docs/arch/11-auth-server-storage.md index 2e927616d1..4edc8a4fa7 100644 --- a/docs/arch/11-auth-server-storage.md +++ b/docs/arch/11-auth-server-storage.md @@ -62,7 +62,7 @@ The storage layer implements multiple interfaces from the [fosite](https://githu - `PendingAuthorizationStorage` — In-flight authorization tracking. Interactive requests have a single-use `awaiting` consent stage before upstream login; only an `approved` record can complete a callback. Each `PendingAuthorization` carries a `BrowserBindingHash`, the SHA-256 of a random cookie value set on the user agent. The consent POST and `/oauth/callback` require the matching cookie before consuming their respective records. Only the hash is stored; an older row without a binding or approval stage cannot complete the flow and must be restarted. `PendingDeviceLogin` carries the same browser binding for the separate device-flow verification-page login, which shares `/oauth/callback`. - `RememberedConsentStorage` — Optional server-side browser sessions and one approval per `(userID, clientID)`. The browser holds only a random `__Host-thv_consent_` secret; storage holds its SHA-256 digest. Each approval records the redirect URI, scopes, and resource selected for that client. Memory and Redis implement this capability; decorators expose the underlying backend via `storage.Unwrap`. See [Interactive client approval](#interactive-client-approval). - `UserStorage` — Internal user accounts and provider identity linking -- `DCRCredentialStore` — DCR client metadata and credential persistence; intentionally NOT embedded in `Storage` (each backend implements it separately and call sites reach it via an explicit `stor.(DCRCredentialStore)` type assertion). Secret-based clients store hashed secrets; `private_key_jwt` clients store their inline public JWKS and signing algorithm. Outbound (upstream) registrations resolved by `pkg/auth/dcr` are keyed by `(Issuer, UpstreamID, RedirectURI, ScopesHash)` plus the optional operator-set `dcrConfig.rotationKey`. Changing the rotation key addresses a new row, so the next start registers a fresh upstream client; the previous row stays in place and carries no TTL unless the upstream asserted `client_secret_expires_at`. On a cache hit the resolver also checks the cached client against the upstream and replaces the row when the upstream rejects it, provided the store implements `dcr.Replacer`. `dcrConfig.registrationProbe` selects the check: `tokenEndpoint` (default) redeems a random authorization code and reads `invalid_client` as "gone", which decides only for upstreams that authenticate the client before validating the grant; `authorizationEndpoint` sends an authorization request without following redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers `invalid_grant` for any client_id; `none` skips the check. The rotation key covers whatever the configured probe cannot tell: a `private_key_jwt` client, an upstream that renders its unknown-client error with 200 or a redirect, or an operator who wants a deliberate rotation. +- `DCRCredentialStore` — DCR client metadata and credential persistence; intentionally NOT embedded in `Storage` (each backend implements it separately and call sites reach it via an explicit `stor.(DCRCredentialStore)` type assertion). Secret-based clients store hashed secrets; `private_key_jwt` clients store their inline public JWKS and signing algorithm. Outbound (upstream) registrations resolved by `pkg/auth/dcr` are keyed by `(Issuer, UpstreamID, RedirectURI, ScopesHash)` plus the optional operator-set `dcrConfig.rotationKey`. Changing the rotation key addresses a new row, so the next start registers a fresh upstream client; the previous row stays in place and carries no TTL unless the upstream asserted `client_secret_expires_at`. On a cache hit the resolver also checks the cached client against the upstream and replaces the row when the upstream rejects it, provided the store implements `dcr.Replacer`. `dcrConfig.registrationProbe` selects the check: `tokenEndpoint` (default) redeems a random authorization code and reads `invalid_client` as "gone"; `authorizationEndpoint` sends an authorization request without following redirects and reads a direct 400/401/403 carrying an RFC 6749 JSON error object with `error` `invalid_client`, `unauthorized_client` or `invalid_request` as "gone" (RFC 6749 §4.1.2.1); `none` skips the check. Each setting has a blind spot the rotation key covers: `tokenEndpoint` misses an upstream that validates the grant before authenticating a public client (it then answers `invalid_grant` for any client_id) and skips `private_key_jwt` clients, which it cannot sign for; `authorizationEndpoint` misses an upstream that renders its unknown-client error with 200, a redirect, or a page rather than a JSON error object; with `none` the rotation key is the only way to retire a registration. A false rejection by either probe re-registers at every start and, when registration also fails, fails startup. - `AssertionJWTConsumer` — atomic single-use consumption for validated JWT assertions. It is deliberately separate from `Storage`, so only assertion-grant composition requires replay protection. **Implementation:** diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 76849f569c..e3c0d8e79a 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -1962,7 +1962,7 @@ _Appears in:_ | `discoveryUrl` _string_ | DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver
issues a single GET against this URL (no well-known-path fallback) and
reads registration_endpoint, authorization_endpoint, token_endpoint,
token_endpoint_auth_methods_supported, and scopes_supported from the
response.
Mutually exclusive with RegistrationEndpoint.
HTTPS is required because the registration endpoint resolved from this
document carries the initial access token and the issued client_secret
(RFC 7591 §3, RFC 8414 §3). MaxLength is a defensive size cap (etcd
object budget, regex evaluation cost) and matches the conventional URL
length cap. | | MaxLength: 2048
Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| | `registrationEndpoint` _string_ | RegistrationEndpoint is the RFC 7591 registration endpoint URL used
directly, bypassing discovery. When using this field, the caller is
expected to also supply AuthorizationEndpoint, TokenEndpoint, and an
explicit Scopes list on the parent OAuth2UpstreamConfig.
Mutually exclusive with DiscoveryURL.
HTTPS is required because the registration endpoint carries the initial
access token and the issued client_secret (RFC 7591 §3, RFC 8414 §3).
MaxLength is a defensive size cap (etcd object budget, regex evaluation
cost) and matches the conventional URL length cap. | | MaxLength: 2048
Pattern: `^https://[^\s?#]+[^/\s?#]$`
Optional: \{\}
| | `rotationKey` _string_ | RotationKey is an optional operator-chosen label that is part of the
cached registration's identity but is never sent to the upstream.
Change it to make the proxies register a fresh client at their next
start instead of reusing the cached one: for example after the
upstream has dropped the registration, or when a rotation is required.
At start ToolHive checks the cached client at the upstream (see
RegistrationProbe) and registers a new one by itself when the upstream
rejects it. Where the configured probe cannot tell, the stale client_id
may only be rejected in the user's browser, and changing this key is
the remedy. The previous registration's row stays in storage, and
upstream refresh tokens bound to the previous client_id stop
refreshing, so affected users sign in again. | | MaxLength: 64
Pattern: `^[A-Za-z0-9._-]+$`
Optional: \{\}
| -| `registrationProbe` _string_ | RegistrationProbe selects how ToolHive checks the cached upstream client
at start. tokenEndpoint (the default) redeems a random authorization
code with the cached client and reads invalid_client as "registration
gone"; it decides only for upstreams that authenticate the client
before validating the grant. authorizationEndpoint sends an
authorization request for the cached client without following
redirects and reads a direct 4xx as "gone" (RFC 6749 §4.1.2.1), for
upstreams whose token endpoint answers invalid_grant for any client_id;
do not use it behind bot protection that answers 4xx to non-browser
requests. none skips the check; the registration is then retired only
through RotationKey. | | Enum: [tokenEndpoint authorizationEndpoint none]
Optional: \{\}
| +| `registrationProbe` _string_ | RegistrationProbe selects how ToolHive checks the cached upstream client
at start. tokenEndpoint (the default) redeems a random authorization
code with the cached client and reads invalid_client as "registration
gone"; it decides only for upstreams that authenticate the client
before validating the grant. authorizationEndpoint sends an
authorization request for the cached client without following
redirects and reads a direct 400, 401 or 403 that carries an RFC 6749
JSON error object (error invalid_client, unauthorized_client or
invalid_request) as "gone" (RFC 6749 §4.1.2.1), for upstreams whose
token endpoint answers invalid_grant for any client_id; an HTML page or
a bare gateway status is not a verdict. A false rejection re-registers
at every start and, when registration also fails, fails startup. none
skips the check; the registration is then retired only through
RotationKey. | | Enum: [tokenEndpoint authorizationEndpoint none]
Optional: \{\}
| | `initialAccessTokenRef` _[api.v1beta1.SecretKeyRef](#apiv1beta1secretkeyref)_ | InitialAccessTokenRef is an optional reference to a Kubernetes Secret
carrying an RFC 7591 §3 initial access token. When set, the resolver
presents the token value as a Bearer credential on the registration
request. Mirrors the ClientSecretRef pattern. | | Optional: \{\}
| | `softwareId` _string_ | SoftwareID is the RFC 7591 "software_id" registration metadata value,
identifying the client software independent of any particular
registration instance. Typically a UUID or short identifier. | | MaxLength: 255
Optional: \{\}
| | `softwareStatement` _string_ | SoftwareStatement is the RFC 7591 "software_statement" JWT asserting
metadata about the client software, signed by a party the authorization
server trusts.
Stored inline on the CR. The JWT is signed but not encrypted, so its
contents are visible to anyone with get/list/watch on this resource and
appear in etcd backups in plaintext. Treat the value as non-confidential
(signed attestation, not a secret). Operators that rotate software
statements like bearer credentials should keep them at the authorization
server side and rely on the registration endpoint's initial access
token (see InitialAccessTokenRef) instead of placing them on the CR.
Bounded to 16384 characters as a defensive size cap (etcd object
budget, regex evaluation cost). Real-world signed statements with
embedded x5c certificate chains, JWKS keys, or OIDC-Federation
trust-framework metadata routinely exceed 4 KB. | | MaxLength: 16384
Optional: \{\}
| diff --git a/docs/server/docs.go b/docs/server/docs.go index 1549d70b45..03f28daccc 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -158,7 +158,7 @@ const docTemplate = `{ "type": "string" }, "registration_probe": { - "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 4xx as \"gone\"\n(RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers\ninvalid_grant for any client_id; do not use it behind bot protection\nthat answers 4xx to non-browser requests. \"none\" skips the check; the\nregistration is then retired only through RotationKey.", + "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 400, 401 or 403\nthat carries an RFC 6749 JSON error object (error invalid_client,\nunauthorized_client or invalid_request) as \"gone\" (RFC 6749 §4.1.2.1),\nfor upstreams whose token endpoint answers invalid_grant for any\nclient_id; an HTML page or a bare gateway status is not a verdict. A\nfalse rejection re-registers at every start and, when registration also\nfails, fails startup. \"none\" skips the check; the registration is then\nretired only through RotationKey.", "type": "string" }, "rotation_key": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index a528af446b..540302d54e 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -151,7 +151,7 @@ "type": "string" }, "registration_probe": { - "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 4xx as \"gone\"\n(RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers\ninvalid_grant for any client_id; do not use it behind bot protection\nthat answers 4xx to non-browser requests. \"none\" skips the check; the\nregistration is then retired only through RotationKey.", + "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 400, 401 or 403\nthat carries an RFC 6749 JSON error object (error invalid_client,\nunauthorized_client or invalid_request) as \"gone\" (RFC 6749 §4.1.2.1),\nfor upstreams whose token endpoint answers invalid_grant for any\nclient_id; an HTML page or a bare gateway status is not a verdict. A\nfalse rejection re-registers at every start and, when registration also\nfails, fails startup. \"none\" skips the check; the registration is then\nretired only through RotationKey.", "type": "string" }, "rotation_key": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index d62b2e40a3..658f4f07f5 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -211,11 +211,14 @@ components: invalid_client as "registration gone"; it decides only for upstreams that authenticate the client before validating the grant. "authorizationEndpoint" sends an authorization request for the cached - client without following redirects and reads a direct 4xx as "gone" - (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers - invalid_grant for any client_id; do not use it behind bot protection - that answers 4xx to non-browser requests. "none" skips the check; the - registration is then retired only through RotationKey. + client without following redirects and reads a direct 400, 401 or 403 + that carries an RFC 6749 JSON error object (error invalid_client, + unauthorized_client or invalid_request) as "gone" (RFC 6749 §4.1.2.1), + for upstreams whose token endpoint answers invalid_grant for any + client_id; an HTML page or a bare gateway status is not a verdict. A + false rejection re-registers at every start and, when registration also + fails, fails startup. "none" skips the check; the registration is then + retired only through RotationKey. type: string rotation_key: description: |- diff --git a/pkg/auth/dcr/request.go b/pkg/auth/dcr/request.go index dc854de58a..00c0a84971 100644 --- a/pkg/auth/dcr/request.go +++ b/pkg/auth/dcr/request.go @@ -19,14 +19,18 @@ const ( ProbeTokenEndpoint RegistrationProbe = "tokenEndpoint" // ProbeAuthorizationEndpoint sends an authorization request for the - // cached client without following redirects. RFC 6749 §4.1.2.1 has the - // server answer an unknown client_id or redirect_uri directly, with no - // redirect, while a known client is redirected (to login, or back to - // redirect_uri with an error). A direct 4xx therefore means the - // registration is gone. Use it for upstreams whose token endpoint - // validates the grant before the client, where ProbeTokenEndpoint cannot - // decide; not behind bot protection that answers 4xx to non-browser - // requests, which would re-register at every start. + // cached client without following redirects and no credentials, so it + // covers every auth method. RFC 6749 §4.1.2.1 has the server answer an + // unknown client_id or redirect_uri directly, with no redirect, while a + // known client is redirected (to login, or back to redirect_uri with an + // error). A direct 400, 401 or 403 carrying an RFC 6749 JSON error object + // with error invalid_client, unauthorized_client or invalid_request means + // the registration is gone; any other direct answer (an HTML page, a + // gateway's 401/403, another error code) is unverified and keeps the + // cached client. Use it for upstreams whose token endpoint validates the + // grant before the client, where ProbeTokenEndpoint cannot decide. A + // false rejection re-registers at every start and, when registration + // also fails, fails startup. ProbeAuthorizationEndpoint RegistrationProbe = "authorizationEndpoint" // ProbeNone never contacts the upstream on a cache hit; the cached diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index 7a67198d0a..6e66aecf72 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -327,12 +327,14 @@ func newDCRStepError(step, issuer, redirectURI string, err error) *dcrStepError // or req.RegistrationProbe is ProbeNone, it returns the cached resolution // without discovery or registration I/O. // -// Neither probe can tell every dropped registration apart from a live one: -// the token-endpoint probe is blind to an upstream that validates the grant -// before authenticating a public client, the authorization-endpoint probe to -// one that renders its unknown-client error with 200 or a redirect, and -// private_key_jwt clients are never probed. For those, the operator retires -// the registration with Request.RotationKey. +// Neither probe can tell every dropped registration apart from a live one. +// The token-endpoint probe is blind to an upstream that validates the grant +// before authenticating a public client, and it does not sign for +// private_key_jwt clients, so it skips them. The authorization-endpoint probe +// sends no credentials and probes every auth method, but it is blind to an +// upstream that renders its unknown-client error with 200, a redirect, or a +// page instead of an RFC 6749 JSON error. For those, the operator retires the +// registration with Request.RotationKey. // // The check runs only here, so a registration the upstream drops while the // caller keeps using a returned resolution is recovered on the next @@ -503,9 +505,9 @@ func revalidateCachedResolution( } // logUnverifiedRegistration records why a cached registration is used -// without the upstream having confirmed it. A private_key_jwt client is -// never probed and ProbeNone asks for no probe, so those log at Debug on -// every resolve; any other reason is unexpected and logs at Warn. +// without the upstream having confirmed it. The token-endpoint probe skips +// private_key_jwt clients and ProbeNone asks for no probe, so those log at +// Debug on every resolve; any other reason is unexpected and logs at Warn. func logUnverifiedRegistration( ctx context.Context, req *Request, key Key, redirectURI string, cached *Resolution, reason error, ) { diff --git a/pkg/auth/dcr/validate.go b/pkg/auth/dcr/validate.go index 1059eda64e..8f3154e8d1 100644 --- a/pkg/auth/dcr/validate.go +++ b/pkg/auth/dcr/validate.go @@ -6,12 +6,14 @@ package dcr import ( "context" "crypto/rand" + "encoding/json" "errors" "fmt" "io" "log/slog" "net/http" "net/url" + "regexp" "strings" "time" @@ -27,10 +29,27 @@ import ( // long before the cached registration is used as-is. const registrationProbeTimeout = 10 * time.Second -// authorizationProbeDrainLimit caps how much of an authorization endpoint's -// response body the probe reads before closing it. The body is never -// inspected; draining only lets the connection be reused. -const authorizationProbeDrainLimit = 1 << 20 +// authorizationProbeBodyLimit caps how much of an authorization endpoint's +// direct error response the probe reads to find an RFC 6749 error code. An +// error object is a few hundred bytes; anything larger is a page, not a +// verdict. The body is never logged. +const authorizationProbeBodyLimit = 16 << 10 + +// authorizationProbeRejectionCodes are the RFC 6749 error codes a direct +// (non-redirected) authorization response may carry when the server could +// not validate client_id or redirect_uri. invalid_request is included because +// §4.1.2.1 names no code for that case and servers report an unregistered +// redirect_uri that way (a 2026 vendor incident did exactly this), while a +// valid client with a malformed request would be answered by redirect. +var authorizationProbeRejectionCodes = map[string]struct{}{ + "invalid_client": {}, + "unauthorized_client": {}, + "invalid_request": {}, +} + +// authorizationProbeErrorCodePattern bounds what the probe is willing to +// repeat from a server's error code in its own error and logs. +var authorizationProbeErrorCodePattern = regexp.MustCompile(`^[a-z_]{1,40}$`) // registrationStatus is the upstream's verdict on a cached registration. type registrationStatus int @@ -98,8 +117,9 @@ func verifyCachedRegistration(ctx context.Context, req *Request, cached *Resolut // some servers validate the code first and answer invalid_grant for any // client_id; against those servers this probe cannot detect a deleted // registration (ProbeAuthorizationEndpoint can, when the server follows RFC -// 6749 §4.1.2.1). private_key_jwt clients are not probed: the probe does not -// sign for them. +// 6749 §4.1.2.1). This probe skips private_key_jwt clients: it does not sign +// for them (the authorization-endpoint probe sends no credentials and does +// cover them). // // The configured token endpoint wins over the cached one, mirroring // applyExplicitEndpointOverrides, so a tampered cached endpoint cannot @@ -147,22 +167,35 @@ func verifyViaTokenEndpoint(ctx context.Context, req *Request, cached *Resolutio } // verifyViaAuthorizationEndpoint sends an authorization request for the cached -// client and reads the first response without following redirects. Per RFC -// 6749 §4.1.2.1 the server MUST NOT redirect when the client_id or -// redirect_uri is invalid and informs the user directly instead, so a direct -// 4xx is the signal that the registration is gone, while any 2xx (a login or +// client and reads the first response without following redirects. RFC 6749 +// §4.1.2.1 only says that a server MUST NOT redirect when client_id or +// redirect_uri is invalid; the converse does not hold, since anything in +// front of the server (a gateway wanting a login, an IP policy, bot +// protection) can answer a direct 4xx for a live client. A 2xx (a login or // consent page) or 3xx (to the IdP, or back to redirect_uri with an error -// such as invalid_scope) shows the client is known. The request carries no -// credentials: only the public client_id, the registered redirect_uri, the -// configured scopes and a throwaway state and PKCE challenge, so the server -// records at most an abandoned authorization request. +// such as invalid_scope) shows the client is known. A direct 4xx is a verdict +// only when the server itself answered: status 400, 401 or 403 with an RFC +// 6749 JSON error object whose error is one of +// authorizationProbeRejectionCodes. A server that had validated client_id +// and redirect_uri would have delivered that error by redirect, so the +// direct structured error is the server reporting it could not. The request +// carries no credentials: only the public client_id, the registered +// redirect_uri, the configured scopes and a throwaway state and PKCE +// challenge, so the server records at most an abandoned authorization +// request. +// +// Everything else is unverified and keeps the cached registration: other +// 4xx, a 4xx without a JSON error object (Keycloak and Auth0 render HTML), +// a JSON error outside the rejection set, 5xx and transport failures. A +// server that renders its unknown-client error with 200, or redirects to its +// own error page, reads as alive. Up to authorizationProbeBodyLimit of the +// body is read for the error code and is never logged; only a code matching +// authorizationProbeErrorCodePattern is repeated. // -// Not every 4xx is a verdict: 404 and 405 describe the endpoint rather than -// the client, and 429 is rate limiting. Those, 5xx and transport failures are -// unverified. A server that renders its unknown-client error with 200, or -// redirects to its own error page, reads as alive; the cached registration is -// then kept exactly as without a probe. The response body is never read for -// content or logged. +// A false rejection is costly: the resolver re-registers at every start (so +// users of an upstream that binds refresh tokens to the client sign in again +// each time) and, when registration also fails, startup fails. Hence the +// narrow verdict. // // The configured authorization endpoint wins over the cached one, mirroring // verifyViaTokenEndpoint. @@ -203,21 +236,29 @@ func verifyViaAuthorizationEndpoint(ctx context.Context, req *Request, cached *R return registrationUnverified, fmt.Errorf("dcr: authorization endpoint probe: %w", err) } defer func() { - _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, authorizationProbeDrainLimit)) + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, authorizationProbeBodyLimit)) _ = resp.Body.Close() }() + body, err := io.ReadAll(io.LimitReader(resp.Body, authorizationProbeBodyLimit)) + if err != nil { + return registrationUnverified, fmt.Errorf("dcr: authorization endpoint probe: read response: %w", err) + } - status := classifyAuthorizationProbe(resp.StatusCode) + status, errorCode := classifyAuthorizationProbe(resp.StatusCode, body) //nolint:gosec // G706: client_id is public metadata per RFC 7591. slog.Debug("dcr: authorization endpoint probe answered", "client_id", cached.ClientID, "status", resp.StatusCode, + "error_code", errorCode, "registration_live", status == registrationLive, "registration_rejected", status == registrationRejected, ) if status != registrationUnverified { return status, nil } + if errorCode != "" { + return status, fmt.Errorf("dcr: authorization endpoint answered HTTP %d (error %q)", resp.StatusCode, errorCode) + } return status, fmt.Errorf("dcr: authorization endpoint answered HTTP %d", resp.StatusCode) } @@ -245,19 +286,27 @@ func authorizationProbeURL(endpoint, clientID, redirectURI string, scopes []stri // classifyAuthorizationProbe maps the authorization endpoint's first response // to a registrationStatus; see verifyViaAuthorizationEndpoint for the rules. -func classifyAuthorizationProbe(statusCode int) registrationStatus { - switch { - case statusCode >= http.StatusOK && statusCode < http.StatusBadRequest: - return registrationLive - case statusCode == http.StatusNotFound, - statusCode == http.StatusMethodNotAllowed, - statusCode == http.StatusTooManyRequests: - return registrationUnverified - case statusCode >= http.StatusBadRequest && statusCode < http.StatusInternalServerError: - return registrationRejected +// The second result is the RFC 6749 error code found in a direct error +// response, filtered through authorizationProbeErrorCodePattern, or "". +func classifyAuthorizationProbe(statusCode int, body []byte) (registrationStatus, string) { + if statusCode >= http.StatusOK && statusCode < http.StatusBadRequest { + return registrationLive, "" + } + switch statusCode { + case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden: default: - return registrationUnverified + return registrationUnverified, "" + } + var errResp struct { + Error string `json:"error"` + } + if json.Unmarshal(body, &errResp) != nil || !authorizationProbeErrorCodePattern.MatchString(errResp.Error) { + return registrationUnverified, "" + } + if _, rejected := authorizationProbeRejectionCodes[errResp.Error]; rejected { + return registrationRejected, errResp.Error } + return registrationUnverified, errResp.Error } // probeErrorAttrs renders why a probe was unverified as log attributes. An diff --git a/pkg/auth/dcr/validate_test.go b/pkg/auth/dcr/validate_test.go index b9ccac64c7..d5602b502f 100644 --- a/pkg/auth/dcr/validate_test.go +++ b/pkg/auth/dcr/validate_test.go @@ -519,23 +519,53 @@ func TestResolveCredentials_CacheHitAuthorizationEndpointProbe(t *testing.T) { wantClientID: "cached-client-id", }, { - name: "direct 400 replaces the registration", + // The shape a vendor answered with for a purged public client + // (its description named the redirect_uri, not the client). + name: "direct 400 invalid_request replaces the registration", authorize: oauthErrorResponse(http.StatusBadRequest, "invalid_request"), wantClientID: "fresh-client-id", wantRegistrations: 1, }, { - name: "direct 401 replaces the registration", + // The shape a ToolHive auth server answers with (fosite). + name: "direct 401 invalid_client replaces the registration", authorize: oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), wantClientID: "fresh-client-id", wantRegistrations: 1, }, { - name: "direct 403 replaces the registration", + name: "direct 403 unauthorized_client replaces the registration", + authorize: oauthErrorResponse(http.StatusForbidden, "unauthorized_client"), + wantClientID: "fresh-client-id", wantRegistrations: 1, + }, + { + name: "direct 403 HTML page keeps the registration", authorize: func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "text/html") w.WriteHeader(http.StatusForbidden) _, _ = w.Write([]byte("Unknown client")) }, - wantClientID: "fresh-client-id", wantRegistrations: 1, + wantClientID: "cached-client-id", + }, + { + name: "direct 401 from a gateway without a body keeps the registration", + authorize: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("WWW-Authenticate", `Bearer realm="gateway"`) + w.WriteHeader(http.StatusUnauthorized) + }, + wantClientID: "cached-client-id", + }, + { + name: "direct 400 with a non-client error code keeps the registration", + authorize: oauthErrorResponse(http.StatusBadRequest, "server_error"), + wantClientID: "cached-client-id", + }, + { + name: "direct 400 with a JSON body that is not an error object keeps the registration", + authorize: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"message":"Unknown client"}`)) + }, + wantClientID: "cached-client-id", }, {name: "404 keeps the registration", authorize: statusOnly(http.StatusNotFound), wantClientID: "cached-client-id"}, {name: "405 keeps the registration", authorize: statusOnly(http.StatusMethodNotAllowed), wantClientID: "cached-client-id"}, @@ -684,24 +714,42 @@ func TestResolveCredentials_ProbeNoneSkipsUpstream(t *testing.T) { func TestClassifyAuthorizationProbe(t *testing.T) { t.Parallel() + invalidClient := []byte(`{"error":"invalid_client","error_description":"The requested OAuth 2.0 Client does not exist."}`) tests := []struct { - status int - want registrationStatus + name string + status int + body []byte + want registrationStatus + wantCode string }{ - {http.StatusOK, registrationLive}, - {http.StatusSeeOther, registrationLive}, - {http.StatusBadRequest, registrationRejected}, - {http.StatusUnprocessableEntity, registrationRejected}, - {http.StatusNotFound, registrationUnverified}, - {http.StatusMethodNotAllowed, registrationUnverified}, - {http.StatusTooManyRequests, registrationUnverified}, - {http.StatusBadGateway, registrationUnverified}, - {http.StatusContinue, registrationUnverified}, + {name: "200 page", status: http.StatusOK, body: []byte(""), want: registrationLive}, + {name: "303 redirect", status: http.StatusSeeOther, want: registrationLive}, + {name: "400 invalid_request", status: http.StatusBadRequest, + body: []byte(`{"error": "invalid_request", "error_description": "redirect_uri is not in the allowlist"}`), + want: registrationRejected, wantCode: "invalid_request"}, + {name: "401 invalid_client", status: http.StatusUnauthorized, body: invalidClient, + want: registrationRejected, wantCode: "invalid_client"}, + {name: "403 unauthorized_client", status: http.StatusForbidden, body: []byte(`{"error":"unauthorized_client"}`), + want: registrationRejected, wantCode: "unauthorized_client"}, + {name: "400 other error code", status: http.StatusBadRequest, body: []byte(`{"error":"server_error"}`), + want: registrationUnverified, wantCode: "server_error"}, + {name: "400 error code outside the log-safe alphabet", status: http.StatusBadRequest, + body: []byte(`{"error":""}`), want: registrationUnverified}, + {name: "403 HTML", status: http.StatusForbidden, body: []byte("Unknown client"), want: registrationUnverified}, + {name: "401 empty body", status: http.StatusUnauthorized, want: registrationUnverified}, + {name: "422 invalid_client", status: http.StatusUnprocessableEntity, body: invalidClient, want: registrationUnverified}, + {name: "404 invalid_client", status: http.StatusNotFound, body: invalidClient, want: registrationUnverified}, + {name: "405", status: http.StatusMethodNotAllowed, want: registrationUnverified}, + {name: "429 invalid_client", status: http.StatusTooManyRequests, body: invalidClient, want: registrationUnverified}, + {name: "502 invalid_client", status: http.StatusBadGateway, body: invalidClient, want: registrationUnverified}, + {name: "100", status: http.StatusContinue, want: registrationUnverified}, } for _, tt := range tests { - t.Run(http.StatusText(tt.status), func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tt.want, classifyAuthorizationProbe(tt.status)) + got, code := classifyAuthorizationProbe(tt.status, tt.body) + assert.Equal(t, tt.want, got) + assert.Equal(t, tt.wantCode, code) }) } } @@ -733,6 +781,55 @@ func TestAuthorizationProbeErrorAttrs_OmitsServerResponse(t *testing.T) { assert.Equal(t, []any{"error", "dcr: authorization endpoint answered HTTP 429"}, probeErrorAttrs(reason)) } +// TestAuthorizationProbeErrorAttrs_RepeatsOnlyTheErrorCode pins that a +// structured error outside the rejection set is reported by status and +// error code, never by its error_description. +func TestAuthorizationProbeErrorAttrs_RepeatsOnlyTheErrorCode(t *testing.T) { + t.Parallel() + + const secretish = "do-not-log-this-description" + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + upstream.setAuthorize(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(map[string]string{"error": "server_error", "error_description": secretish}) + }) + req := preloadForAuthorizationProbe(t, newMemoryDCRStore(t), upstream) + + status, reason := verifyCachedRegistration(context.Background(), req, &Resolution{ + ClientID: "cached-client-id", + AuthorizationEndpoint: upstream.URL + "/authorize", + TokenEndpoint: upstream.URL + "/token", + TokenEndpointAuthMethod: "none", + RedirectURI: upstream.URL + "/oauth/callback", + }) + require.Equal(t, registrationUnverified, status) + require.Error(t, reason) + assert.NotContains(t, reason.Error(), secretish) + assert.Equal(t, []any{"error", `dcr: authorization endpoint answered HTTP 400 (error "server_error")`}, probeErrorAttrs(reason)) +} + +// TestResolveCredentials_AuthorizationProbeVerifiesPrivateKeyJWTClients pins +// that the authorization-endpoint probe, which sends no credentials, does +// probe a private_key_jwt client (the token-endpoint probe cannot sign for +// one and skips it). +func TestResolveCredentials_AuthorizationProbeVerifiesPrivateKeyJWTClients(t *testing.T) { + t.Parallel() + + upstream := newProbeUpstream(t, oauthErrorResponse(http.StatusUnauthorized, "invalid_client"), http.StatusCreated) + upstream.setAuthorize(oauthErrorResponse(http.StatusBadRequest, "invalid_client")) + cache := newMemoryDCRStore(t) + req := preloadCachedRegistration(t, cache, upstream, "private_key_jwt") + req.RegistrationProbe = ProbeAuthorizationEndpoint + + got, err := ResolveCredentials(context.Background(), req, cache) + require.NoError(t, err) + assert.Equal(t, "fresh-client-id", got.ClientID) + assert.Equal(t, int32(1), upstream.authzRequests.Load()) + assert.Equal(t, int32(1), upstream.registrations.Load()) + assert.Zero(t, upstream.tokenRequests.Load()) +} + func TestProbeErrorAttrs_OmitsServerResponse(t *testing.T) { t.Parallel() diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index f359a1a735..36f063aad2 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -931,11 +931,14 @@ type DCRUpstreamConfig struct { // invalid_client as "registration gone"; it decides only for upstreams // that authenticate the client before validating the grant. // "authorizationEndpoint" sends an authorization request for the cached - // client without following redirects and reads a direct 4xx as "gone" - // (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers - // invalid_grant for any client_id; do not use it behind bot protection - // that answers 4xx to non-browser requests. "none" skips the check; the - // registration is then retired only through RotationKey. + // client without following redirects and reads a direct 400, 401 or 403 + // that carries an RFC 6749 JSON error object (error invalid_client, + // unauthorized_client or invalid_request) as "gone" (RFC 6749 §4.1.2.1), + // for upstreams whose token endpoint answers invalid_grant for any + // client_id; an HTML page or a bare gateway status is not a verdict. A + // false rejection re-registers at every start and, when registration also + // fails, fails startup. "none" skips the check; the registration is then + // retired only through RotationKey. RegistrationProbe string `json:"registration_probe,omitempty" yaml:"registration_probe,omitempty"` // InitialAccessTokenFile is the path to a file containing the RFC 7591 diff --git a/sdk/go/client/oas_schemas_gen.go b/sdk/go/client/oas_schemas_gen.go index 546f2710ab..3423895aca 100644 --- a/sdk/go/client/oas_schemas_gen.go +++ b/sdk/go/client/oas_schemas_gen.go @@ -628,11 +628,14 @@ type AuthenticationDCRUpstreamConfig struct { // invalid_client as "registration gone"; it decides only for upstreams // that authenticate the client before validating the grant. // "authorizationEndpoint" sends an authorization request for the cached - // client without following redirects and reads a direct 4xx as "gone" - // (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers - // invalid_grant for any client_id; do not use it behind bot protection - // that answers 4xx to non-browser requests. "none" skips the check; the - // registration is then retired only through RotationKey. + // client without following redirects and reads a direct 400, 401 or 403 + // that carries an RFC 6749 JSON error object (error invalid_client, + // unauthorized_client or invalid_request) as "gone" (RFC 6749 §4.1.2.1), + // for upstreams whose token endpoint answers invalid_grant for any + // client_id; an HTML page or a bare gateway status is not a verdict. A + // false rejection re-registers at every start and, when registration also + // fails, fails startup. "none" skips the check; the registration is then + // retired only through RotationKey. RegistrationProbe OptString `json:"registration_probe"` // RotationKey is an optional operator-chosen label (1-64 characters from // [A-Za-z0-9._-]) that becomes part of the cached registration's identity diff --git a/sdk/go/openapi.json b/sdk/go/openapi.json index 051eefc285..3ebb9754cd 100644 --- a/sdk/go/openapi.json +++ b/sdk/go/openapi.json @@ -232,7 +232,7 @@ "type": "string" }, "registration_probe": { - "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 4xx as \"gone\"\n(RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers\ninvalid_grant for any client_id; do not use it behind bot protection\nthat answers 4xx to non-browser requests. \"none\" skips the check; the\nregistration is then retired only through RotationKey.", + "description": "RegistrationProbe selects how the resolver checks a cached registration\nagainst the upstream at start. \"tokenEndpoint\" (the default) redeems a\nrandom authorization code with the cached client and reads\ninvalid_client as \"registration gone\"; it decides only for upstreams\nthat authenticate the client before validating the grant.\n\"authorizationEndpoint\" sends an authorization request for the cached\nclient without following redirects and reads a direct 400, 401 or 403\nthat carries an RFC 6749 JSON error object (error invalid_client,\nunauthorized_client or invalid_request) as \"gone\" (RFC 6749 §4.1.2.1),\nfor upstreams whose token endpoint answers invalid_grant for any\nclient_id; an HTML page or a bare gateway status is not a verdict. A\nfalse rejection re-registers at every start and, when registration also\nfails, fails startup. \"none\" skips the check; the registration is then\nretired only through RotationKey.", "type": "string" }, "rotation_key": { diff --git a/sdk/go/openapi.yaml b/sdk/go/openapi.yaml index 954200a0ab..8773f6dc32 100644 --- a/sdk/go/openapi.yaml +++ b/sdk/go/openapi.yaml @@ -287,11 +287,14 @@ components: invalid_client as "registration gone"; it decides only for upstreams that authenticate the client before validating the grant. "authorizationEndpoint" sends an authorization request for the cached - client without following redirects and reads a direct 4xx as "gone" - (RFC 6749 §4.1.2.1), for upstreams whose token endpoint answers - invalid_grant for any client_id; do not use it behind bot protection - that answers 4xx to non-browser requests. "none" skips the check; the - registration is then retired only through RotationKey. + client without following redirects and reads a direct 400, 401 or 403 + that carries an RFC 6749 JSON error object (error invalid_client, + unauthorized_client or invalid_request) as "gone" (RFC 6749 §4.1.2.1), + for upstreams whose token endpoint answers invalid_grant for any + client_id; an HTML page or a bare gateway status is not a verdict. A + false rejection re-registers at every start and, when registration also + fails, fails startup. "none" skips the check; the registration is then + retired only through RotationKey. type: string rotation_key: description: |-