Remove MCP direct identity-provider mode - #1334
Closed
jiaenren wants to merge 2 commits into
Closed
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
3 tasks
jiaenren
added a commit
that referenced
this pull request
Aug 27, 2026
#1334 removed oidcProxy.enabled from values.yaml but left the template reading it, so it fell through to its default of false. Every OIDC variable sits behind that gate, which means a values file written to the contract this design documents renders with the whole auth block missing and the container fails at start on absent configuration. The render fixture only passed because it still carried the flag. The service has no auth_enabled field any more -- test_auth asserts that -- so nothing consumed OSMO_MCP_AUTH_ENABLED either. Authentication is not a mode, and the chart now says so: the gate, the environment variable and the flag in the fixture are gone, and the required messages name MCP rather than a proxy that could be switched off. Guarded by a render assertion that the auth block renders from a fixture carrying no enable flag, and that no enable switch is emitted.
3 tasks
Three reductions to what a deployer must state, none of which changes what the deployment does. **Values the chart already knew.** accessTokenAudience had to equal resourceUrl, scope had to equal resourceUrl plus the scope name, and the issuer had to be resourceUrl minus /mcp -- and the chart refused to render unless each matched what it had already computed. A deployer was asked for an answer the chart held, then rejected for giving a different one. They are derived now, so the 30-line cross-checking block in MCPAuthConfig, the matching Helm fail blocks, the three environment variables carrying the restated values, and the three CI cases that existed only to prove the cross-checks fired all go. This also drops trustedHttpsRedirectOrigins, which no deployment sets and which cost a CSV environment variable, two properties, a validator branch and a Helm regex loop. Native clients use loopback redirects, still allowed. **A URL discovery publishes.** accessTokenJwksUrl is jwks_uri in every OIDC discovery document, which FastMCP already fetches at startup. OSMO hand-built a JWTVerifier only to pin it, and carried a process-lifetime httpx client to serve it. OIDCProxy builds the verifier now, and OSMO overrides only what discovery cannot supply: an Entra resource application configured for v1 access tokens issues them from https://sts.windows.net/<tenant>/ even when its discovery document advertises the v2.0 issuer, so accessTokenIssuer stays configured. The audience is applied inside that override rather than through OIDCProxy's own audience argument, because that argument is forwarded to the provider's authorize and token endpoints (oidc_proxy.py:432-434), which Entra does not accept. An existing test asserting that no extra token parameters are sent caught this. **Redis that every sibling already shares.** MCP carried its own host, port and TLS settings. serviceName and port fell back to services.redis but tlsEnabled did not, so a deployment whose Redis requires TLS could satisfy every other service and still CrashLoop MCP. All three come from services.redis now; only dbNumber stays, to isolate proxy state from OSMO's other Redis users, and its default moves off 0 for that reason. The chart also refused more than one MCP replica, on the premise that FastMCP serializes token refresh within a process. FastMCP says the opposite -- "All state is stored in the configured client_storage backend (Redis, disk, etc.) enabling horizontal scaling across multiple instances" (oauth_proxy/proxy.py:212-213), with :1903-1912 handling the distributed refresh race explicitly. The rule also contradicted itself, since sharing state through Redis only matters across replicas it forbade. The negative test asserting the ban is replaced by a positive one that two replicas render, negative-tested by reintroducing a ban. Required OIDC proxy values: 10 -> 6. Note for anyone using --config rather than environment variables: a stale file carrying issuer_url, auth_scope, oidc_access_token_audience or oidc_access_token_jwks_url now fails at startup with an unrecognized-key error rather than being ignored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MCP shipped two ways to authenticate a caller: the Gateway validating a bearer and MCP trusting injected headers, or FastMCP verifying the caller itself. Only one can be the answer, and the published documentation already recommended the second. This removes the first. request_context.py loses the header trust boundary entirely: the ASGI middleware, the per-request state, the header parsing, and the user_name field that no consumer read. get_request_credentials collapses to the FastMCP access token, which is now the only source of caller identity, so identity can no longer come from a header a caller could set. The module goes from 355 lines to 194. auth_enabled disappears with it. Authentication is not a mode any more, so its configuration is required rather than conditionally required, and a deployment that cannot reach an identity provider fails at load instead of starting up unauthenticated. The Gateway stops synthesising what FastMCP serves: the protected-resource route forwards unconditionally, and the local_reply_config that rewrote the /mcp 401 challenge is gone because FastMCP emits it. services.mcp loses authorizationServers and scopes, and oidcProxy loses its enabled toggle, which could only ever be true. Tests authenticate the way the service does. protocol_harness gains a static token verifier and a service_config helper, so suites that exercise the tool pipeline get a real auth provider instead of trusted headers. The smoke checks come with it, because deploying proxy mode for the first time showed the suite failing before it reached any assertion: it authenticates with the OSMO-issued OETF token, which FastMCP rejects. The unauthenticated surface a client uses to bootstrap OAuth needs no token and is now its own test -- the 401 challenge and its resource_metadata pointer, the RFC 9728 document, that FastMCP advertises its authorization endpoint under /mcp, and that the health endpoints the /mcp prefix would otherwise publish return 404. The authenticated catalog and workflow checks name OSMO_MCP_ACCESS_TOKEN, which runs them when an identity-provider token is supplied. Two smoke-check fixes ride along. The expected catalog was missing osmo_list_tasks, added by #1311, so its exact-equality assertion would have failed at 25 versus 26 had it ever run, and the two counts in the service README were stale for the same reason. The suite also carried an `auth` tag that the kind environment's exclude_tags dropped, so it never ran anywhere -- which is why the staleness went unnoticed. It is tagged by what it needs deployed now, with kind excluding `mcp` explicitly. Two behaviours changed and are recorded rather than hidden. An unauthenticated request is now rejected after RequestBodyLimitMiddleware has buffered its bounded body rather than before, because FastMCP's auth runs inside the route. And whether a follow-up request succeeds after a cancelled one is FastMCP's to decide; verified against the pre-change tree that an authenticated deployment already behaved this way, so this change does not introduce it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jiaenren
force-pushed
the
jiaenr/mcp-remove-direct-mode
branch
from
August 28, 2026 19:35
9147844 to
3f9b509
Compare
3 tasks
Collaborator
Author
|
Superseded by #1350, which is the same branch and the same content with the smoke-check commit from #1333 folded in. GitHub refused to retarget this PR's base ( |
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.
Stacked on #1333. The last structural PR in the MCP authentication simplification stack.
Issue #None
+251 / -1,285.
Why
MCP shipped two ways to authenticate: the Gateway validating a bearer and MCP trusting injected headers, or FastMCP verifying the caller itself. Only one can be the answer, and the published docs already recommended the second. Every route, matcher, config field and code path was conditional on which one was active.
Change
request_context.py355 → 194 lines. The ASGI middleware, per-request state, header parsing, and theuser_namefield no consumer read are gone.get_request_credentials()collapses to FastMCP's access token — so caller identity can no longer come from a header a caller could set. That is now structural rather than validated.auth_enabledgoes with it. Authentication isn't a mode, so its configuration is required rather than conditionally required: a deployment that cannot reach an identity provider fails at load instead of starting unauthenticated.The Gateway stops synthesising what FastMCP serves — the protected-resource route forwards unconditionally, and the
local_reply_configthat rewrote the/mcp401 is gone because FastMCP emits it.authorizationServers,scopes, andoidcProxy.enabled(which could only ever be true) are removed, along with the direct-mode CI fixture and the validator half that tested it.Tests authenticate the way the service does:
protocol_harnessgainsAnyTokenVerifierand aservice_confighelper, so suites exercising the tool pipeline get a real auth provider instead of trusted headers.Two behaviour changes, recorded not hidden
1. Unauthenticated requests are rejected after the body is buffered, not before. The deleted middleware enforced fail-before-body; FastMCP's auth runs inside the route, so
RequestBodyLimitMiddleware— which is outermost — buffers the bounded body first. The request is still rejected and the body still bounded; only the order changed. A test asserts this explicitly rather than leaving it undiscovered.2. After a cancelled request, a follow-up on the same client is FastMCP's to decide. I verified against the pre-change tree, with an auth provider installed, that an authenticated deployment already behaved this way — so this PR does not introduce it. It is pre-existing in OIDC-proxy mode and worth its own investigation; a development deployment has it today.
Verification
bazel test //src/service/mcp/... //test/smoke/...— 76/76 passrender-tests.sh,validate-mcp-chart.sh— pass🤖 Generated with Claude Code
Checklist