Derive the MCP proxy configuration and share OSMO's Redis - #1329
Conversation
|
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 |
51de8a7 to
8987c52
Compare
8987c52 to
13141f1
Compare
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>
13141f1 to
26bfd8e
Compare
|
📖 Docs preview: https://d3in15bfzp49i0.cloudfront.net/1329/index.html |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jiaenr/mcp-oauth-under-mcp-path #1329 +/- ##
===================================================================
+ Coverage 74.54% 74.55% +0.01%
===================================================================
Files 243 243
Lines 29942 29925 -17
Branches 4538 4533 -5
===================================================================
- Hits 22320 22312 -8
+ Misses 6718 6713 -5
+ Partials 904 900 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Stack (6): #1328 → #1329 → #1350 → #1335 → #1342 → #1347
Stacked on #1328.
Issue - None
+151 / -257. Three reductions to what a deployer must state, none of which changes what the deployment does. Consolidates the former #1329, #1330 and #1331.
Values the chart already knew
accessTokenAudiencehad to equalresourceUrl,scopehad to equalresourceUrlplus the scope name, and the issuer had to beresourceUrlminus/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 Helmfailblocks, 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 already publishes
accessTokenJwksUrlisjwks_uriin every OIDC discovery document, which FastMCP already fetches at startup. OSMO hand-built aJWTVerifieronly to pin it, and carried a process-lifetime httpx client to serve it.OIDCProxybuilds the verifier now, and OSMO overrides only what discovery cannot supply: an Entra resource application configured for v1 access tokens issues them fromhttps://sts.windows.net/<tenant>/even when its discovery document advertises the v2.0 issuer, soaccessTokenIssuerstays configured.The audience is applied inside that override rather than through
OIDCProxy's ownaudienceargument, 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.
serviceNameandportfell back toservices.redisbuttlsEnableddid not, so a deployment whose Redis requires TLS could satisfy every other service and still CrashLoop MCP. All three come fromservices.redisnow; onlydbNumberstays, to isolate proxy state from OSMO's other Redis users.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-1912handling the distributed refresh race explicitly. The rule also contradicted itself, since sharing state through Redis only matters across replicas it forbade.Required OIDC proxy values: 10 → 6.
Migration note
Environment-variable deployments are unaffected. A stale
--configfile carryingissuer_url,auth_scope,oidc_access_token_audienceoroidc_access_token_jwks_urlwill now fail at startup with an unrecognized-key error rather than being ignored.Verification
bazel test //src/service/mcp/... //test/smoke/...— 76/76 pass at this commitrender-tests.sh— pass at this commit🤖 Generated with Claude Code
Checklist