Add loginPolicy to upstream provider CRD config#5767
Open
onepabz wants to merge 1 commit into
Open
Conversation
The embedded auth server walks every configured upstream provider at every login: the first authorize leg always targets upstreams[0] and callback.go's continueChainOrComplete then walks all remaining upstreams. Since stacklok#5725/stacklok#5733 the server honors Config.UpstreamFilter to narrow that chain, but nothing feeds it: the runner never sets the field and no CRD, Helm, or file surface reaches it. A declarative operator deployment therefore cannot stop a secondary SaaS provider (e.g. GitHub next to the identity IdP) from forcing its consent screen on every user at every login. Expose the existing narrowing through the CRD surface: - UpstreamProviderConfig gains an optional loginPolicy enum field (required|onDemand; empty means required, today's behavior). - The operator copies it into authserver.UpstreamRunConfig via the shared BuildAuthServerRunConfig, covering MCPServer, MCPRemoteProxy, and VirtualMCPServer alike. - The runner translates onDemand entries into a StaticUpstreamFilter (new, implements handlers.UpstreamFilter) and sets Config.UpstreamFilter, activating the previously dormant wiring. - Validation rejects onDemand on the first upstream (it anchors the chain and resolves the user's identity) at CRD validation time, at VirtualMCPServer reconcile time, and at RunConfig load. Tokens previously stored for an onDemand provider are still injected and refreshed at MCP-request time; only login-time acquisition is skipped. There is intentionally no in-band flow yet for a user to authorize an onDemand provider after login - that is the linked-account half of stacklok#5383 and is left for maintainer direction. Toward stacklok#5383. Signed-off-by: Pablo Sanchez <494757+onepabz@users.noreply.github.com>
onepabz
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt,
jerm-dro,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Since #5725/#5733 the embedded auth server honors a non-nil
Config.UpstreamFilter— but nothing feeds it:pkg/authserver/runner/embeddedauthserver.gobuildsresolvedCfgwithout ever setting it, and no CRD/Helm/env surface reaches it. So a declarative operator deployment still chains everyupstreamProvidersentry at every login — adding GitHub next to the identity IdP forces GitHub consent on every user at every login, even for users who never call a GitHub-backed tool.This is a minimal strawman toward #5383: it exposes the already-merged chain narrowing through the CRD.
UpstreamProviderConfiggains an optionalloginPolicyenum (required|onDemand; empty =required= today's behavior).authserver.UpstreamRunConfigin the sharedBuildAuthServerRunConfig(so MCPServer / MCPRemoteProxy / VirtualMCPServer all inherit it).onDemandentries into aStaticUpstreamFilterand setsConfig.UpstreamFilter, activating the dormant wiring.onDemandon the first upstream is rejected at three layers (CRD validation, vMCP reconcile,RunConfig.Validate) — the first upstream anchors identity and the filter contract never removes it.Additive and opt-in — filter nil / field empty is byte-identical to today; the CRD schema-compat check passes against the
v0.34.0baseline.Scope (step 1 of 2):
onDemandonly skips login-time acquisition; already-stored tokens still inject/refresh at request time. There's no in-band flow yet for a user to grant anonDemandprovider after login —SingleLegexists in storage/callback but nothing originates it. So this removes the forced-consent blocker to aggregating backends under one endpoint; the on-demand connect flow (a browser-facing surface that originatesSingleLeg— the linked-account half of #5383) is the natural follow-up and deserves maintainer direction first. Happy to build it.Testing
go build ./...,gofmt, andgolangci-lint runclean on touched packages;pkg/authserver/...,pkg/vmcp/config, andcmd/thv-operator/...unit suites pass (new tests at each layer). Generated artifacts viatask operator-generate/operator-manifests/crdref-gen.Notes
Marked draft — a strawman to help unstick #5383; happy to rename the field/values or reshape per maintainer direction. Context: we run ToolHive in production on Kubernetes (operator + vMCP + MCPRemoteProxy, an OIDC IdP as primary upstream, per-user SaaS tokens via
upstream_inject), today working around the forced chain with one single-backend vMCP per SaaS provider.Refs #5383