Skip to content

Take the MCP token-verification JWKS URI from OIDC discovery - #1330

Closed
jiaenren wants to merge 2 commits into
jiaenr/mcp-derive-auth-configfrom
jiaenr/mcp-derive-jwks-from-discovery
Closed

Take the MCP token-verification JWKS URI from OIDC discovery#1330
jiaenren wants to merge 2 commits into
jiaenr/mcp-derive-auth-configfrom
jiaenr/mcp-derive-jwks-from-discovery

Conversation

@jiaenren

@jiaenren jiaenren commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #1329. Third of the MCP authentication simplification stack.

Issue #None

Problem

Deployers supplied accessTokenJwksUrl — a value every OIDC discovery document publishes as jwks_uri, and which FastMCP already fetches at startup. OSMO hand-built a JWTVerifier purely to pin that URL, and carried a process-lifetime httpx client to serve it.

Change

Let OIDCProxy build the verifier (oidc_proxy.py:381-395) and override 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 JWKS URI now comes from discovery.

Required OIDC-proxy values: 7 → 6.

One subtlety worth reviewer attention

The audience is applied inside the override rather than through OIDCProxy's own audience argument. That argument is forwarded to the provider's authorize and token endpoints (oidc_proxy.py:432-434) — an Auth0-style parameter that Entra does not accept.

I got this wrong first. An existing assertion that no extra token parameters are sent caught it:

AssertionError: {'audience': 'https://osmo.example/mcp'} != {}

That test was written to prevent exactly this class of mistake, and it worked.

Also removed

The http_close_fails test, which existed solely to verify the try/finally ordering between the httpx client and Redis — both the client and the ordering are gone.

Verification

  • bazel test //src/service/mcp/... — 72/72 pass, including a new assertion that the verifier's jwks_uri comes from the mocked discovery document while its issuer stays the configured one
  • render-tests.sh, validate-mcp-chart.sh — pass

🤖 Generated with Claude Code

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@jiaenren
jiaenren requested a review from a team as a code owner August 26, 2026 02:03
@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bd7f0eb7-93f0-4ca3-8aad-5265426745ab

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

jiaenren and others added 2 commits August 26, 2026 16:56
The chart demanded values it already knew, then refused to render unless
each equalled what it had computed: accessTokenAudience had to equal
resourceUrl, scope had to equal resourceUrl plus the scope name, and the
issuer had to be resourceUrl minus /mcp. A deployer was asked for an
answer the chart already held, and rejected for giving a different one.

Derive them instead. auth_scope becomes a property of resource_url and
the audience is the resource URL at its one use site, so the 30-line
cross-checking block in MCPAuthConfig and the matching Helm fail blocks
both disappear, along with the three environment variables that carried
the restated values and the three CI cases that existed only to prove the
cross-checks fired.

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, which are
still allowed.

The deployer-facing documentation is updated to match: both READMEs and
the deployment guide no longer list the removed keys, and the four places
that still named the pre-/mcp callback URL are corrected.

Required OIDC proxy values: 10 -> 7.

Note for anyone using --config rather than environment variables: a stale
file carrying issuer_url, auth_scope or oidc_access_token_audience will
now fail at startup with an unrecognized-key error rather than being
ignored.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The deployer supplied accessTokenJwksUrl, which every OIDC discovery
document already publishes as jwks_uri and which FastMCP already fetches
at startup. OSMO hand-built a JWTVerifier only so it could pin that URL,
and carried a process-lifetime httpx client to serve it.

Let OIDCProxy build the verifier and override only the piece 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
a configured value; the JWKS URI now comes from discovery.

The audience is applied inside the 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.

Drops the accessTokenJwksUrl value, its environment variable, its two
Helm validations, the hand-built verifier, the httpx client, and the
try/finally that existed to close it.

Required OIDC proxy values: 7 -> 6.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jiaenren
jiaenren force-pushed the jiaenr/mcp-derive-jwks-from-discovery branch from 921bd4c to 223664a Compare August 26, 2026 23:57
@jiaenren

Copy link
Copy Markdown
Collaborator Author

Consolidated into #1329 to reduce the number of PRs in this stack for review.

Deriving the JWKS URI from OIDC discovery is part of the same theme: stop asking the deployer for values the deployment already knows or can discover.

No content is lost: the cumulative diff of the stack against main is byte-identical to before the consolidation (verified by comparing the resulting tree). Closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant