Reduce the MCP configuration to the four values only a deployer knows - #1342
Open
jiaenren wants to merge 1 commit into
Open
Reduce the MCP configuration to the four values only a deployer knows#1342jiaenren wants to merge 1 commit into
jiaenren wants to merge 1 commit into
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 |
What remains after this is resourceUrl, the OIDC configuration URL, the client ID, and a Secret holding the client secret. Everything else is derived, inherited, or defaulted. **Secret paths follow the mount.** 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 derived from the mount now, and the mount itself defaults, so an existing-Secret deployment states neither. Three fail blocks and their cross-checks go with them. A deployment supplying the secret another way, such as Vault injection, still states clientSecretFile, the one case where it is genuinely input. **The Redis password is opt-in.** Deriving the password file from the mount left it keyed on redisPasswordKey, defaulting to redis-password -- so every existing-Secret deployment carried that key whether or not its Redis wanted a password, and one whose Redis needs none failed to mount a key it had no reason to create. Naming the key is what opts in now. **The access-token issuer is optional.** It was required even though only a provider issuing v1-format access tokens needs it: every other issues from the issuer its discovery document advertises, which is what FastMCP defaults to. The verifier falls back to the discovery issuer. **Authentication is not a mode.** An earlier commit in this stack 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 documented contract rendered with the whole auth block missing and the container failed at start on absent configuration; the fixture only passed because it still carried the flag. The gate, the environment variable and the flag are gone, and the required messages name MCP rather than a proxy that could be switched off. **The MCP audience joins the provider that already exists.** Enabling MCP required a gateway.envoy.jwt.providers entry carrying the resource URL as its audience, and in every deployment that entry was a copy of one already present -- same issuer, JWKS URI, claim and cluster -- differing only in audience, because the relayed upstream token comes from the identity provider already configured for this deployment's own clients. Envoy accepts several audiences per provider, so the resource URL is appended to the entry whose issuer matches. The issuer is derivable, since OpenID Connect Discovery defines the configuration URL as the issuer plus /.well-known/openid-configuration; accessTokenIssuer still overrides it. The comparison ignores a trailing slash, and an entry already carrying the audience does not get it twice. Failing to find a provider now names the issuer rather than saying an entry is required, which was the error a deployer hit after supplying one. **The signing key is resolved.** The derived signing and storage keys are only as strong as the client secret, and nothing checked it; the service now refuses to start below 32 characters, which rejects a hand-written placeholder. The HKDF derivation is pinned locally rather than calling fastmcp.server.auth.jwt_issuer.derive_jwt_key, because an upstream change to that derivation would silently make every stored registration and token undecryptable; the bytes are identical today and test_auth asserts that equivalence against FastMCP so a divergence fails the build. The production-readiness warning telling operators to assess the derived key is replaced by that entropy requirement -- it does not survive reading FastMCP 3.4.7, where high-entropy material goes through HKDF and the PBKDF2 path with a length warning is reserved for low-entropy strings. **mcp:Access is retired.** The action mapped to exactly one path, /mcp, where ext_authz is disabled -- so it was never evaluated, and a role granting it implied an access control that did not run. The wholesale gate is the identity-provider scope: JWTVerifier rejects a token without it, so a caller cannot reach the protocol endpoint. Per-operation authorization is unchanged, because every tool call reaches OSMO through /api where gateway JWT validation and semantic RBAC apply as they do for the CLI and UI. resourceTypeMCP and ResourceTypeMCP go too. Existing deployments need no migration: IsValidAction has no callers, so a role row still granting mcp:Access keeps an action matching no path, which is already true today. Rendering the proxy fixture is byte-identical apart from the removed enable switch. Each change is covered by a render assertion and negative-tested by reintroducing the defect it guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jiaenren
force-pushed
the
jiaenr/mcp-reduce-required-values
branch
from
August 28, 2026 19:35
1daa472 to
07e55b2
Compare
This was referenced Aug 28, 2026
This was referenced Aug 28, 2026
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
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.
Stack (6): #1328 → #1329 → #1350 → #1335 → #1342 → #1347
Stacked on #1335.
Issue - None
+248 / -170. What remains after this is
resourceUrl, the OIDC configuration URL, the client ID, and a Secret holding the client secret. Everything else is derived, inherited, or defaulted. Consolidates the former #1342, #1343, #1344 and #1346.Secret paths follow the mount
clientSecretFileandredis.passwordFilehad to equal the existing Secret's mount path plus a fixed filename — the chart failed to render otherwise, while the volume already hardcodes both filenames. The deployer was asked to restate a value the chart held, and rejected for restating it differently. Both are derived from the mount now, and the mount itself defaults, so an existing-Secret deployment states neither. Threefailblocks go with them.Deployments supplying the secret another way, such as Vault injection, still state
clientSecretFile— the one case where it is genuinely input.The Redis password is opt-in
Deriving the password file from the mount left it keyed on
redisPasswordKey, defaulting toredis-password. That made every existing-Secret deployment carry the key whether or not its Redis wanted a password, and a deployment whose Redis needs none failed to mount a key it had no reason to create. Naming the key is what opts in now.The access-token issuer is optional
Required, though only a provider issuing v1-format access tokens needs it: every other issues from the issuer its discovery document advertises, which is what FastMCP defaults to (
oidc_proxy.py:521). Now optional, with the verifier falling back to the discovery issuer.Authentication is not a mode
An earlier commit in this stack removed
oidcProxy.enabledfromvalues.yamlbut left the template reading it, so it fell through to its default offalse. Every OIDC variable sits behind that gate — meaning a values file written to the documented contract rendered with the whole auth block missing, and the container failed at start on absent configuration. The render fixture only passed because it still carried the flag. The gate, the environment variable and the flag are gone.The MCP audience joins the provider that already exists
Enabling MCP required a
gateway.envoy.jwt.providersentry carrying the resource URL as its audience. In every deployment that entry was a copy of one already present — same issuer, JWKS URI, claim and cluster — differing only in audience, because the relayed upstream token comes from the identity provider already configured for this deployment's own clients.Envoy accepts several audiences per provider, so the resource URL is appended to the entry whose issuer matches. The issuer is derivable, since OpenID Connect Discovery defines the configuration URL as the issuer plus
/.well-known/openid-configuration;accessTokenIssuerstill overrides it. The comparison ignores a trailing slash, and an entry already carrying the audience does not get it twice. Failing to find a provider now names the issuer rather than saying an entry is required — which was the error a deployer hit after supplying one.The signing key is resolved
The derived signing and storage keys are only as strong as the client secret, and nothing checked it; the service now refuses to start below 32 characters, which rejects a hand-written placeholder. The deployment guide already told operators this was enforced — it was not.
The HKDF derivation is pinned locally rather than calling
fastmcp.server.auth.jwt_issuer.derive_jwt_key, because an upstream change to that derivation would silently make every stored registration and token undecryptable. The bytes are identical today, andtest_authasserts that equivalence against FastMCP so a divergence fails the build.The production-readiness warning telling operators to assess the derived key is replaced by that entropy requirement — it does not survive reading FastMCP 3.4.7, where high-entropy material goes through HKDF and the PBKDF2 path with a length warning is reserved for low-entropy operator strings.
mcp:Accessis retiredThe action mapped to exactly one path,
/mcp, whereext_authzis disabled — so it was never evaluated, and a role granting it implied an access control that did not run. The wholesale gate is the identity-provider scope:JWTVerifierrejects a token without it, so a caller cannot reach the protocol endpoint at all. Per-operation authorization is unchanged, because every tool call reaches OSMO through/apiwhere gateway JWT validation and semantic RBAC apply exactly as they do for the CLI and UI.Existing deployments need no migration:
IsValidActionhas no callers, so a role row still grantingmcp:Accesskeeps an action that matches no path — already true today.Verification
bazel test //src/service/mcp/... //test/smoke/...— 76/76 pass at this commitrender-tests.sh— pass at this commit, with assertions for each change above🤖 Generated with Claude Code
Checklist