Skip to content

Support MCP OAuth through the configured OIDC issuer - #63

Merged
backnotprop merged 4 commits into
plannotator:mainfrom
yalait:oidc-mcp-oauth
Sep 22, 2026
Merged

backnotprop merged 4 commits into
plannotator:mainfrom
yalait:oidc-mcp-oauth

Conversation

@yalait

@yalait yalait commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Closes #57.

/mcp now accepts end-user access tokens from the OIDC issuer already configured for
browser login, through the existing ExternalMcpBearerVerifier seam and the same
oidc:<issuer> plus sub binding. A token must name the configured issuer and carry
<origin>/mcp in aud. ID tokens, tokens for another resource, and browser cookies are
refused. The server publishes RFC 9728 protected-resource metadata and answers an
unauthenticated call with 401 and resource_metadata. Managed API keys and the WorkOS
path are untouched.

0028 records the reversal, 0020's exclusion points at it, MCP-013 takes the generic path,
and docs/deployment.md explains the audience mapper and the two client-registration
options: the issuer's own RFC 7591 endpoint, or one client an administrator registers.
Artifact Server issues no client credentials, so MCP-014 stays as it is.

resolveIdentity now also receives the credential. WorkOS ignores it. The OIDC path
needs it, because userinfo answers the presenter of the token, and a Keycloak realm can
leave email out of the access token. The caching alternative is in 0028.

Two calls for you

aud must contain <origin>/mcp, but a multi-valued aud passes. The Keycloak harness
in this PR mints this token from a client whose only extra configuration is the audience
mapper from the deployment guide:

{"aud": ["http://127.0.0.1:44835/mcp", "account"], "typ": "Bearer"}

account comes from Keycloak's audience-resolve mapper, so a single-valued check would
refuse every Keycloak token. A token without this resource in aud is refused either
way. Tell me if you meant single-valued and I will tighten it.

A valid token from a person who was never admitted gets 401 at /mcp, while the
browser callback answers 403 for the same identity. I kept the existing mapping.

Verification

pnpm lint, pnpm typecheck, pnpm build, pnpm conformance:validate,
pnpm conformance:tests, pnpm test:oidc with two new MCP cases against the pinned
Keycloak, and pnpm test with 320 passed. Three tests/cli process-spawning tests time
out under parallel load here and pass when run alone, the same way they do on an
unmodified checkout. I did not run the Compose, object-storage, Kubernetes, coverage, or
performance legs of verify:iteration.

Hostile cases in tests/conformance/oidc-mcp-authorization.test.ts: expired, wrong
issuer, a second issuer signing with the same key, wrong and multi-valued audiences, ID
token, HS256, unknown kid, missing subject, missing claims, userinfo outage, userinfo
rejection, key-set outage. That last case turned up a bug worth naming: jose reports a
non-200 JWKS response as a generic error, which read as an invalid token and answered
401. The key-set fetch now raises its own failure.

@backnotprop backnotprop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES. The design fits well: it reuses the existing ExternalMcpBearerVerifier seam, pins the algorithms, and matches issuer and audience exactly. Managed API keys and WorkOS behave as before. Before merge, the ID-token refusal needs to live up to what the ledger and docs promise, the conformance tests need their requirement IDs, and two policy and doc points need a decision.

Blocking

1. The ID-token refusal only works for Keycloak, but the ledger and docs describe it as generic

  • Where: src/identity/oidc-mcp-bearer-verifier.ts:22 and :240.
  • What happens: The only ID-token check is the payload claim typ === "ID", which only Keycloak emits. Other issuers are refused only because an ID token's aud is normally the client ID, not <origin>/mcp.
  • Failure case: Some issuers let operators pick any client ID string. If an operator names a client after the resource URL (a common resource-server convention), that client's ID tokens carry aud = <origin>/mcp. They are then accepted at /mcp.
  • Reproduced: A token signed by the issuer with nonce, at_hash, auth_time, header typ: JWT, no payload typ, and aud = <origin>/mcp returned 200 and admitted the user. With header typ: logout+jwt it also returned 200.
  • Why it matters: MCP-013-F, docs/deployment.md ("ID tokens … are refused") and decision 0028 all state this as a general rule.
  • Suggested fix: Refuse tokens that carry ID-token-only claims (nonce, at_hash, c_hash). Also refuse a JOSE header typ that is present and not at+jwt or JWT, with a note that RFC 9068 at+jwt is preferred. Add a hostile test for each shape.
  • Severity: Medium. Exploiting it needs an unusual client-ID setup, but the promise is recorded in the ledger.

2. The conformance tests have no requirement IDs in their titles

  • Where: tests/conformance/oidc-mcp-authorization.test.ts:80, 117, 153, 163, 215, 250, 259.
  • What happens: None of the seven titles includes MCP-013-B or MCP-013-F.
  • Why it matters: AGENTS.md requires conformance tests to be named with their requirement IDs, so this evidence cannot be attached to the ledger.
  • Suggested fix: Put MCP-013-B on the positive tests (metadata plus successful token, userinfo fallback, several audiences) and MCP-013-F on the hostile ones. workos-mcp-authorization.test.ts has the same gap already on main, so it is not a precedent.

3. Tokens without email_verified are treated as verified, and they can claim the bootstrap administrator

  • Where: src/identity/oidc-mcp-bearer-verifier.ts:223, emailVerified: claims.email_verified !== false.
  • Reproduced: On a fresh installation, the first token to arrive carried email = <bootstrap admin email>, no email_verified, and a new sub. It returned 200 and was bound as the administrator. resolveExternalMember (src/application/installation-access.ts:445) also links by email to any admitted member who is not yet bound.
  • This is not new: Browser login uses the same rule (oidc-identity-provider.ts:177). The PR carries that rule over to access tokens.
  • Why it still matters here: The rule is riskier on this path. The docs name Entra, whose email optional claim is mutable, is not verified, and comes with no email_verified claim.
  • Needs a maintainer decision: Either require email_verified === true on the MCP path, or record in 0028 that the browser-login rule is inherited on purpose and explain why.

4. The Entra guidance in docs/deployment.md:60 is probably wrong

  • What the doc says: "Entra exposes an API and uses its application ID URI."
  • What Microsoft documents: v2.0 access tokens always carry the API's client ID (a GUID) in aud, not the application ID URI. v1.0 tokens can carry the URI, but their iss is https://sts.windows.net/{tid}/, which never matches the v2.0 discovery issuer.
  • Userinfo does not help: Entra's userinfo endpoint only accepts tokens issued for Microsoft Graph, so a token with aud = /mcp cannot reach it.
  • Caveat: This was checked against Microsoft's documented claim rules, not a live tenant.
  • Suggested fix: Remove Entra from the list, or test it and document a setup that works.

Non-blocking

  • Unchecked discovery fields are re-served (src/identity/oidc-oauth-metadata.ts:100). ...document passes every discovery field through to /.well-known/oauth-authorization-server, including end_session_endpoint, device_authorization_endpoint, the raw userinfo_endpoint and mtls_endpoint_aliases. Only the registration and revocation endpoints are checked, so the comment at :79 ("a client must not be sent anywhere this server would have refused") overstates it. Consider an allowlist of fields. The risk is low because the operator configures the issuer.
  • Ledger proof gap is not stated fully (MCP-013). The new MCP-013-B text says "connect … through browser approval including a generic OIDC issuer". The Keycloak test gets its tokens through the password grant. No MCP client runs the authorization code, PKCE or dynamic registration flow against Keycloak. The status is implementing with empty evidence, which is accurate, but proof_gap should say this.
  • Cookie refusal is not tested. MCP-013-F lists browser login cookies. Neither new test sends a real browser session cookie to /mcp. A bogus cookie returns 401. In the Keycloak test, a cookie from the real browser login is available and would make this a real proof.
  • Startup wiring and fallback are not tested (src/cli/lifecycle-commands.ts:434-447). No test covers oidcAuthenticationOrBrowserOnly: the compiled server turning on MCP OAuth when OIDC is configured, or the discovery_failed warning and browser-only fallback. After a startup discovery failure, MCP OAuth stays off until restart. That is worth one sentence in the deployment guide.
  • Trailing-slash issuers fail. An issuer whose tokens carry a trailing slash in iss (Auth0 does this) is refused, because the configured issuer is normalized without the slash and jose compares exactly (returned 401 in a probe). Browser login already behaves the same way, so this is not a regression, but Auth0 is named as compatible in 0028.
  • Cosmetic: resolveIdentity runs jwtVerify a second time on every first-use request. That is cheap because the key set is cached. It is defensible as defense in depth, but a comment saying so would help.

What was verified

Run against this branch (gh pr checkout 63; no conflicts with current main):

  • pnpm install --frozen-lockfile, pnpm exec tsc -p tsconfig.json --noEmit, and pnpm lint all pass.
  • pnpm exec vitest run tests/conformance/oidc-mcp-authorization.test.ts tests/conformance/workos-mcp-authorization.test.ts — 10/10 pass; WorkOS path unchanged.
  • pnpm test:oidc (Docker, pinned Keycloak) — 4/4 pass.
  • Hostile probes over real HTTP against the provider boundary (scratch tests, since deleted): alg: none, empty aud, missing exp, future nbf, trailing-slash iss, garbage segments (generic error, no filesystem paths), email_verified: false, and bogus-cookie-only all correctly return 401. The three 200s are the reproductions in findings 1 and 3.

Not run: pnpm verify:iteration.

Thank you

Thank you @yalait — this is the project's first substantial outside contribution, and it is a careful one: it extends an existing port instead of adding a parallel path, the decision record is honest about reversing 0020, key-set outages are told apart from bad tokens, and a real Keycloak harness backs it. The changes asked for above are small and focused. We would be glad to merge this once they are in.

This review was prepared with Claude under maintainer direction.

Picks up the dual-license change, effect 4.0.0-rc.115, hono 4.13.5,
vitest 4.1.11, and the dependency security overrides. No conflicts; the
branch type-checks against rc.115 unchanged.

Claude-Session: https://claude.ai/code/session_01EcbEctwH1oDTrYhqHe6n9g
The ID-token refusal only recognized Keycloak's payload typ of ID. A JWT
the issuer signed with nonce, at_hash, or c_hash, or with a JOSE header
typ other than at+jwt or JWT (logout+jwt, secevent+jwt, ID-JAG), is now
refused as well, following RFC 9068 section 4 and RFC 8725 explicit
typing. at+jwt, application/at+jwt, JWT, and a missing typ stay accepted.

On the MCP bearer path an email links a member or claims the bootstrap
administrator only when email_verified is true. A subject that is already
bound is still recognized by issuer and subject alone. Browser login is
unchanged.

Conformance tests now carry MCP-013-B and MCP-013-F, and the Keycloak
harness proves that the real browser session cookie is refused at /mcp.

Claude-Session: https://claude.ai/code/session_01EcbEctwH1oDTrYhqHe6n9g
Decision 0028 now records the access-token type checks and why the MCP
path requires email_verified when browser login does not. The deployment
guide drops the Entra audience claim, which v2.0 tokens cannot satisfy,
and notes that MCP OAuth stays off after a startup discovery failure. The
MCP-013 proof gap states that the Keycloak harness uses the password grant.

Claude-Session: https://claude.ai/code/session_01EcbEctwH1oDTrYhqHe6n9g
@backnotprop

Copy link
Copy Markdown
Contributor

Thanks again @yalait — this is a really solid contribution, and we wanted to get it over the line without another round trip, so we pushed the review fixes directly to your branch (hope that's okay!). Summary:

  • Merged main (license change, effect rc.115, dependency bumps) — no conflicts, builds cleanly.
  • ID-token refusal is now issuer-independent. Beyond Keycloak's typ: ID, the verifier refuses tokens carrying nonce/at_hash/c_hash and any JOSE header typ other than at+jwt or JWT (case-insensitive, application/ prefix optional; a missing typ is still fine). We checked this against the MCP spec and SDKs first: the spec only requires OAuth 2.1 resource-server validation and audience binding, so this is RFC 9068 / RFC 8725 hardening layered on top, and Keycloak's typ: JWT tokens keep working (your Keycloak harness still passes).
  • email_verified must be true on the MCP path before an email links a member or claims the bootstrap admin. An already-bound subject still gets in by issuer + sub. Browser login is unchanged. The reasoning is recorded in decision 0028.
  • Requirement IDs: MCP-013-B / MCP-013-F are on one positive and one hostile test (the ID checker allows one claim per ID). New hostile tests cover each token type and the unverified-email bootstrap case.
  • Docs: the Entra paragraph now says plainly that v2.0 tokens can't carry the /mcp audience, so Entra installations use API keys for MCP. We also noted the restart-after-discovery-failure behavior, and the MCP-013 proof gap now mentions that the harness uses the password grant.
  • The Keycloak integration test now also sends the real browser session cookie to /mcp and checks for a 401.

One review item we deliberately did not take: the authorization-server metadata allowlist. The official TypeScript SDK client reads a growing set of metadata fields (several are recent spec additions), so a fixed allowlist would silently break future clients while protecting nothing — spec-following clients fetch the same document directly from the issuer anyway.

Everything is green locally: lint, typecheck, the conformance checks, the Keycloak integration run (twice), and the full test suite. Take a look and tell us if anything doesn't match what you intended.

@backnotprop backnotprop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four blocking findings from the earlier review are addressed on the branch (maintainer-applied, vetted against the MCP 2026-07-28 authorization spec and the official SDKs). Approving; will merge once CI is green.

@backnotprop
backnotprop merged commit 454c45e into plannotator:main Sep 22, 2026
8 checks passed
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.

Generic OIDC installations cannot use per-user MCP authorization

2 participants