Resolve the MCP signing key - #1343
Closed
jiaenren wants to merge 11 commits into
Closed
Conversation
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>
The chart refused to render more than one MCP replica whenever the OIDC proxy was enabled, 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), and :1903-1912 handles the distributed refresh race explicitly. The rule also contradicted itself, since sharing state through Redis only matters across replicas that were forbidden from existing. MCP also carried its own Redis host, port and TLS settings. serviceName and port already 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. Take all three from services.redis like every sibling and keep only dbNumber, which exists to isolate proxy state from OSMO's other Redis users; its default moves off 0 for the same reason. The CI negative test that asserted the replica ban is replaced by a positive assertion that two replicas render, negative-tested by reintroducing a ban and confirming the check fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A dev-instance deploy of OIDC-proxy mode showed this suite failing before it reached any assertion: it authenticates with the OSMO-issued OETF token, which FastMCP rejects because it verifies caller tokens against the identity provider. The suite was written for direct mode, where the Gateway authenticates and MCP trusts injected headers, and staging still runs that mode -- so the incompatibility only surfaced once proxy mode was deployed for the first time. Split the suite along the line that actually matters. The unauthenticated surface a client uses to bootstrap OAuth needs no token and is now its own test that runs in both modes: the 401 challenge and its resource_metadata pointer, the RFC 9728 document, and -- in proxy mode -- 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 skip in proxy mode with a message naming OSMO_MCP_ACCESS_TOKEN, which runs them there when an identity-provider token is supplied. Two related fixes. The expected catalog was missing osmo_list_tasks, added by #1311, so the exact-equality assertion would have failed at 25 versus 26 had it ever run; the two counts in the service README were stale for the same reason. And the suite carried an `auth` tag, which the kind environment's exclude_tags dropped -- so it never ran anywhere, which is why the staleness went unnoticed. It is now tagged by what it needs deployed, with kind excluding `mcp` explicitly. 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 an AnyTokenVerifier and a service_config helper, so suites that exercise the tool pipeline get a real auth provider instead of trusted headers. 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>
MCP was the only chart area in the repository with a dedicated ci/validate-*.sh. It had grown to 360 lines against a house harness of 78 that covered MCP not at all, and it ran from its own CI step, so MCP chart rendering was checked by a mechanism nothing else in the repository used. Most of what it asserted was not behaviour. Numeric ranges, URL shapes and scope patterns are enforced by MCPAuthConfig at start-up, so re-proving them at render time stated the same contract twice in two languages, and each new bound needed editing in both. Move the behavioural assertions into tests/render-tests.sh: that MCP renders nothing when disabled, that the gateway publishes one /mcp prefix route and rewrites it off, that the health carve-out answers 404 itself rather than leaning on jwt_authn, that Redis is inherited from services.redis, that derived values do not reappear as deployer inputs, that the proxy scales out, and that extraEnv cannot redirect the relay. The roles Lua filter assertions come along because they are its only coverage anywhere in the repository. Each new assertion was negative-tested by reintroducing the defect it guards and confirming the harness fails. The proxy fixture moves to tests/ alongside the harness that uses it, and ci/ is now empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mcp_route ended a route at the first nested "- name:" line. Every route with a headers matcher has one at line five, so the extractor returned only the match block and the prefix_rewrite assertion for mcp-authorization-server-metadata could never see the value it checks. render-tests.sh therefore failed at this branch tip. Terminate on the next entry at the routes own indentation instead. Negative-tested by deleting the prefix_rewrite line and confirming the harness fails, then restoring it and confirming it passes.
Two values remained that the deployment already knows. clientSecretFile and redis.passwordFile had to equal the existing Secret's mount path plus a fixed filename -- the chart failed to render otherwise, and the volume already hardcodes both filenames. They are now derived from the mount, and the mount itself defaults, so a deployer using an existing Secret states neither. Three fail blocks and their cross-checks go with them. accessTokenIssuer was required even though only an Entra application configured for v1 access tokens needs it: every other provider issues from the issuer its discovery document advertises, which is what FastMCP itself defaults to. It is now optional, and the verifier falls back to the discovery issuer. Rendering the proxy fixture is byte-identical to before.
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 |
#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.
jiaenren
force-pushed
the
jiaenr/mcp-resolve-signing-key
branch
from
August 27, 2026 23:37
53f3e6d to
3540fc9
Compare
Deriving the password file from existingSecret.mountPath left it keyed on redisPasswordKey, which defaulted to redis-password -- so every deployment using an existing Secret had to carry that key whether or not its Redis wanted a password, and a deployment whose Redis needs none failed to mount a key it never had reason to create. Naming the key is now what opts in. A render assertion covers the case where none is named.
Three loose ends from the design's signing-key step. The derived signing and storage keys are only as strong as the client secret they come from, and nothing checked it. The service now refuses to start below 32 characters. Identity providers issue well above that; the check exists to reject a hand-written placeholder. The deployment guide already told operators the service enforced this -- it did not. derive_jwt_key was imported from fastmcp.server.auth.jwt_issuer, a private symbol with no deprecation contract: a minor release could drop it, or change its derivation and silently invalidate every stored registration and token. The derivation is now local, with the same HKDF parameters, so the bytes are identical and existing state stays readable. test_auth asserts that equivalence against FastMCP directly, so a change on their side surfaces as a failure rather than as undecryptable Redis state. The production-readiness warning said the chart exposes no alternative to the derived key and told operators to assess the limitation before rolling out. That framing does not survive reading FastMCP 3.4.7: high-entropy material goes through HKDF, and the PBKDF2 path with a length warning is reserved for low-entropy operator strings. The warning is replaced with the entropy requirement the design states.
jiaenren
force-pushed
the
jiaenr/mcp-resolve-signing-key
branch
from
August 28, 2026 00:00
3540fc9 to
2ef969b
Compare
jiaenren
force-pushed
the
jiaenr/mcp-reduce-required-values
branch
from
August 28, 2026 19:35
1daa472 to
07e55b2
Compare
Collaborator
Author
|
Consolidated into #1342 to reduce the number of PRs in this stack for review. Resolving the signing key is part of the same theme: settling what a deployer must supply and what the service derives or validates for itself. No content is lost: the cumulative diff of the stack against |
3 tasks
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 #1342. Resolves the signing key, the one step of the simplification that had no PR, plus the key-derivation change #1331 left behind.
Issue #None
Enforce the entropy the derived keys depend on
Both the proxy-token signing key and the Redis storage key are derived from the client secret, so its entropy is theirs, and nothing checked it. The service now refuses to start below 32 characters.
Worth flagging: the deployment runbook already told operators the service refused to start below that length, by design. It did not. An operator reading that reasonably believed a guard existed. Now one does.
Stop importing a private symbol
derive_jwt_keycame fromfastmcp.server.auth.jwt_issuer: private, with no deprecation contract. A minor release could drop it, which is loud, or change its derivation, which is quiet and much worse, since every stored registration and token would become undecryptable.The derivation is now local with the same HKDF parameters (
SHA256, length 32, salt,info=b"Fernet", urlsafe-base64), so the bytes are identical and existing state stays readable.test_authasserts that equivalence against FastMCP directly, so a change on their side surfaces as a test failure rather than as unreadable Redis.Replace the production-readiness warning
The deployment guide said the chart exposes no alternative to the derived key and told operators to assess the limitation before rolling out, text that cannot ship with an official release.
That framing does not survive reading FastMCP 3.4.7:
proxy.py:487-495passes the client secret ashigh_entropy_materialthrough HKDF, while the PBKDF2 path carrying a length warning (:497-506) is reserved for low-entropy operator-supplied strings. The property the derived key depends on is client-secret entropy, which is now stated and enforced.jwtSigningKeyis deliberately not added. With the entropy check and the corrected documentation in place the derived key is defensible on FastMCP's own terms. Deployments requiring key separation can add it later without changing this contract.Verification
bazel test //src/service/mcp/... //test/smoke/...passes 76/76, including a new test that a short secret fails at startup, and the storage-key test now proving byte equivalence with FastMCPrender-tests.shpasses🤖 Generated with Claude Code