Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6834 +/- ##
==========================================
+ Coverage 79.65% 79.69% +0.03%
==========================================
Files 816 818 +2
Lines 83125 83511 +386
==========================================
+ Hits 66214 66552 +338
- Misses 16906 16954 +48
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 <noreply@anthropic.com> Signed-off-by: Aron Gates <aron@muonspace.com>
23437a9 to
b5828d3
Compare
| statusCode == http.StatusMethodNotAllowed, | ||
| statusCode == http.StatusTooManyRequests: | ||
| return registrationUnverified | ||
| case statusCode >= http.StatusBadRequest && statusCode < http.StatusInternalServerError: |
There was a problem hiding this comment.
I'm a bit worried about this branch catching every 4xx other than 404/405/429. Two things.
First, §4.1.2.1 only goes one way: an invalid client_id/redirect_uri must not be redirected, but a direct 4xx doesn't mean the client is invalid. 401/403 in particular can come from a gateway wanting the user to log in or an IP policy, and 406/407/408/421 from whatever sits in front of the AS. The token probe only takes a structured error code as a verdict; this one decides on status alone, and the doc comment above reads the RFC in the converse direction. The tests pin it too: the 400 case uses invalid_request (not a client error) and the 403 case is an HTML page, both expected to re-register.
Second, the fallout: revalidateCachedResolution doesn't fall back to the cached client once it's rejected. If re-registration succeeds we mint a new client each start and users lose their upstream refresh tokens on every restart; if it fails, startup fails where the token probe would have kept the cached client.
Could we tighten what counts as a verdict? Ideally a body signal (e.g. JSON error=invalid_client), but I realise Keycloak renders HTML and I don't know what the #6821 vendor sends, so at minimum an explicit allowlist of status codes per known upstream behaviour, everything else unverified, plus a sentence in the field docs that a false rejection with a failed registration fails startup. What does the #6821 vendor actually return in the body for an unknown client?
There was a problem hiding this comment.
Agreed on both points, and thanks for pushing on it. Narrowed in cf6d46c.
What the two servers actually send for an unknown client (checked today, both GET /authorize with a bogus client_id):
- the Cached upstream DCR registration never recovers after the upstream evicts it #6821 vendor:
400,content-type: application/json,{"error": "invalid_request", "error_description": "redirect_uri is not in the allowlist of permitted redirect URIs"}(same body for a purged client and for a made-up one; a live client gets a 302 to its IdP); - a ToolHive auth server (fosite):
401,application/json;charset=UTF-8,{"error":"invalid_client","error_description":"Client authentication failed (...). The requested OAuth 2.0 Client does not exist."}.
So there is a body signal in both cases, and the probe now requires it: a direct answer is a rejection only when the status is 400, 401 or 403 and the body is an RFC 6749 JSON error object whose error is invalid_client, unauthorized_client or invalid_request. The reasoning is the spec read in the right direction this time: §4.1.2.1 does not say "direct 4xx means invalid client", but a server that had validated client_id and redirect_uri would deliver any error by redirect, so a direct structured OAuth error is the server itself reporting it could not validate them. A gateway's 401/403, 406/407/408/421, an HTML page (Keycloak, Auth0) or a JSON error outside that set is unverified and keeps the cached client, exactly as today. invalid_request stays in the set because §4.1.2.1 names no code for the unregistered-redirect_uri case and the vendor above reports it that way.
The body is read up to 16 KiB for the code only; the error and the debug log repeat the code when it matches ^[a-z_]{1,40}$, never the description (new TestAuthorizationProbeErrorAttrs_RepeatsOnlyTheErrorCode pins that). The table now has the 403 HTML page, a bodiless gateway 401, a JSON server_error and a non-error JSON object all keeping the registration, and TestClassifyAuthorizationProbe covers the status/body matrix.
On the fallout: I kept #6822's "failed re-registration is an error" semantics rather than falling back to a rejected client, since presenting it would only reproduce the rejection, but the cost is now stated in the field docs (CRD, run-config, request.go) and the arch doc: a false rejection re-registers at every start and, when registration also fails, fails startup. The doc comment above the function was rewritten to say what the RFC does and does not imply.
| // 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 |
There was a problem hiding this comment.
This (and the logUnverifiedRegistration doc at L506) is only true for the token-endpoint probe now. verifyViaAuthorizationEndpoint doesn't look at the auth method since it sends no credentials, so with authorizationEndpoint a private_key_jwt client does get probed. That's fine behaviour, the comments just need scoping, something like "the token-endpoint probe does not sign for private_key_jwt clients". A test row for private_key_jwt + authorizationEndpoint + 400 would pin it.
There was a problem hiding this comment.
Right, that was stale from the single-probe wording. Fixed in cf6d46c: ResolveCredentials and logUnverifiedRegistration now say the token-endpoint probe skips private_key_jwt because it cannot sign for it, and that the authorization-endpoint probe sends no credentials and covers every auth method (the verifyViaTokenEndpoint comment says the same from its side). Added TestResolveCredentials_AuthorizationProbeVerifiesPrivateKeyJWTClients: private_key_jwt + authorizationEndpoint + a direct 400 invalid_client re-registers, one authorization request, zero token requests.
| - `RememberedConsentStorage` — Optional server-side browser sessions and one approval per `(userID, clientID)`. The browser holds only a random `__Host-thv_consent_<sha256>` 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. |
There was a problem hiding this comment.
The last sentence lists what rotationKey is for, but it mixes the two probes. The 200/redirect case only applies to authorizationEndpoint, private_key_jwt is only skipped by tokenEndpoint (see the resolver.go comment), and it leaves out the main tokenEndpoint blind spot, an upstream that checks the code before the client, which is the #6821 case. none isn't there either. Maybe spell it out per setting: tokenEndpoint misses grant-first public clients and private_key_jwt, authorizationEndpoint misses unknown-client errors rendered as 200 or a redirect, and with none rotationKey is the only way to retire a registration.
There was a problem hiding this comment.
Rewritten per setting in cf6d46c: tokenEndpoint misses an upstream that validates the grant before authenticating a public client (it then answers invalid_grant for any client_id, the #6821 case) and skips private_key_jwt; 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. It also carries the false-rejection sentence. The same breakdown is in the CRD and run-config field docs.
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 <noreply@anthropic.com> Signed-off-by: Aron <aron@muonspace.com>
This branch has not been deployed
2 participants
Summary
#6822 checks a cached upstream DCR registration at startup by redeeming a random authorization code at the token endpoint:
invalid_clientmeans the registration is gone,invalid_grantmeans it is alive. That decides only for upstreams that authenticate the client before validating the grant. A vendor whose token endpoint answersinvalid_grantfor anyclient_id(the production case in #6821: dead, live and made-up clients all get400 invalid_grant "Invalid authorization code") keeps presenting a dropped registration, and the user sees the rejection in the browser while the resolver sees nothing.dcrConfig.registrationProbeonMCPExternalAuthConfigupstream providers (and the VirtualMCPServer inline copy) andregistration_probeon theauthserver.DCRUpstreamConfigrun-config, with valuestokenEndpoint(default, exactly Re-register DCR clients the upstream evicted #6822's behaviour),authorizationEndpointandnone. Enum-validated at admission and mirrored in the run-configValidateand the resolver's input check, so an unknown value fails at startup instead of silently probing the wrong endpoint.authorizationEndpointsends an authorization request for the cached client (response_type=code,client_id, the registeredredirect_uri, the configured scopes, a throwawaystateand S256 PKCE challenge) and reads the first response without following redirects. RFC 6749 §4.1.2.1 has the server answer an unknownclient_idorredirect_uridirectly rather than by redirect, so a direct 400, 401 or 403 carrying an RFC 6749 JSON error object witherrorinvalid_client,unauthorized_clientorinvalid_requestmeans the registration is gone, and the resolver re-registers through the sameReplacer.ReplaceIfUnchangedpath as the token-endpoint probe. 2xx (a login page) and 3xx (to the identity provider, or back toredirect_uriwith an error) mean the client is known. Everything else is inconclusive and keeps the cached registration: other 4xx, a 4xx without a JSON error object (an HTML page, a bare gateway status), a JSON error outside that set, 5xx and transport failures. The request carries no credentials; up to 16 KiB of the body is read for the error code, which is repeated in logs only when it matches^[a-z_]{1,40}$; the description is never logged. A false rejection re-registers at every start and, when registration also fails, fails startup, which is why the verdict is narrow. Because it sends no credentials it also coversprivate_key_jwtclients, which the token-endpoint probe skips.noneskips the check for upstreams that should not see probe traffic; the registration is then retired only throughrotationKey.rotationKeydocs now say the rejection "may not be visible to the resolver" and point at the probe setting; the cache-hit log records carryregistration_probe.Related to #6821. Builds on #6822.
Type of change
Test plan
task test) — every touched package passes with-race(pkg/auth/dcr,pkg/authserver,pkg/authserver/storage,pkg/authserver/runner,cmd/thv-operator/pkg/controllerutil,cmd/thv-operator/api/...). New coverage inpkg/auth/dcr/validate_test.go:TestResolveCredentials_CacheHitAuthorizationEndpointProbe: 200, 302 to the IdP, 302 back toredirect_uriwith an error, 400invalid_request, 401invalid_client, 403unauthorized_client, 403 HTML page, bodiless gateway 401, 400server_error, 400 non-error JSON, 404, 405, 429, 500, failed re-registration; asserts the request shape, that no credentials are sent, that the token endpoint is never called and that redirects are not followed...._AuthorizationProbeVerifiesPrivateKeyJWTClients,..._CacheHitProbesConfiguredAuthorizationEndpoint(decoy cached endpoint),..._AuthorizationProbeTransportErrorKeepsRegistration,..._ProbeNoneSkipsUpstream.TestClassifyAuthorizationProbe(status and body matrix),TestAuthorizationProbeErrorAttrs_OmitsServerResponse,..._RepeatsOnlyTheErrorCode,TestVerifyCachedRegistration_*(rejected probe endpoints, redirect-URI fallback).newDCRRequestpropagation and the operator CRD-to-run-config conversion each gained the new field.task test-e2e)task lint-fix)API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.registrationProbeis a new optional enum string onDCRUpstreamConfig; an absent or empty value keeps #6822's token-endpoint probe. No storage format or Redis key change.Changes
pkg/auth/dcr/request.goRegistrationProbetype, constants,Validate;Request.RegistrationProbepkg/auth/dcr/validate.goverifyViaAuthorizationEndpoint,classifyAuthorizationProbe; guarded client builder shared by both probespkg/auth/dcr/resolver.govalidateResolveInputs;registration_probelog attribute;ProbeNonelogs at Debugpkg/authserver/config.goregistration_probeon the run-config, validated through the resolver's enumpkg/authserver/runner/dcr_adapter.godcr.Requestcmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.goregistrationProbewith an Enum marker; rotationKey wordingcmd/thv-operator/pkg/controllerutil/authserver.gopkg/authserver/storage/types.godeploy/charts/operator-crds/**,docs/operator/crd-api.md,docs/server/*,sdk/go/*docs/arch/11-auth-server-storage.mdDoes this introduce a user-facing change?
Yes. Operators can set
dcrConfig.registrationProbe: authorizationEndpointon an upstream whose token endpoint cannot tell a dropped public client from a live one; the proxy then re-registers at its next start instead of presenting the deadclient_iduntil someone rotates it. The default is unchanged. The authorization endpoint sees one abandoned authorization request per upstream per start.Special notes for reviewers
400 {"error":"invalid_request","error_description":"redirect_uri is not in the allowlist..."}, a ToolHive auth server with401 {"error":"invalid_client", ...}; both decide. Servers that render an HTML page (Keycloak, Auth0), a gateway's 401/403, or a 200/redirect error page read as unverified or alive, which is today's behaviour;rotationKeyremains the remedy there.noneis included because an operator may not want probe traffic against a vendor that logs or alerts on failed requests; it is one case in the dispatcher and the resolver logs it at Debug rather than Warn.newProbeHTTPClient; each sets its ownCheckRedirect(refuse for the token probe, which carries credentials;ErrUseLastResponsefor the authorization probe, where the redirect is the answer).🤖 Generated with Claude Code