[Client] Add OAuth authorization to the HTTP transport - #53
Open
chr-hertel wants to merge 5 commits into
Open
chr-hertel wants to merge 5 commits into
chr-hertel wants to merge 5 commits into
Conversation
Pass `auth:` to `HttpTransport` and a 401 turns into discovery, an authorization code and a token, then a retry. `Mcp\Client\Auth\OAuth` builds the authenticator; `BearerToken` sends one you already have. Covers RFC 9728/8414 discovery, PKCE, dynamic registration, the four token endpoint auth methods, scope selection and step-up, refresh, client credentials, cross-app access, and the RFC 9207 iss check. Closes modelcontextprotocol#315, modelcontextprotocol#316, modelcontextprotocol#317, modelcontextprotocol#318, modelcontextprotocol#319, modelcontextprotocol#320, modelcontextprotocol#321, modelcontextprotocol#322, modelcontextprotocol#323, modelcontextprotocol#324, modelcontextprotocol#325, modelcontextprotocol#326, modelcontextprotocol#329, modelcontextprotocol#360, modelcontextprotocol#361, modelcontextprotocol#363, modelcontextprotocol#376, modelcontextprotocol#377
chr-hertel
force-pushed
the
feat-client-auth
branch
from
September 8, 2026 07:09
1069695 to
cc4cf09
Compare
Six changes, from reviewing the flow with the MCP server treated as attacker-controlled — it picks the challenge, the metadata, and thus the authorization server. * Only attach a token to a request within the resource it was minted for. The transport builds one authenticator per endpoint, but nothing stopped a caller sharing one across two servers. * Require `state` on the authorization response instead of checking it only when present. The loopback listener accepts a connection from any local process, and an absent state skipped the binding entirely. * Stop logging the redirect Location, which carried the authorization code. Log the parameter names instead. * Refuse authorization, token and registration endpoints that are not https, unless the host is loopback. * Write the credential file through a 0600 temporary file and rename it into place, so it is never briefly world-readable or half-written. * Default `legacyDiscovery` to off. A server that publishes no protected resource metadata leaves nothing to check its authorization server against; `setLegacyDiscovery(true)` opts back in. The console handler no longer accepts a bare authorization code, since one carries neither state nor iss to check. Conformance unchanged: 234/241 on 2025-11-25, 405/405 on 2026-07-28, and both 2025-03-26 scenarios still pass with legacy discovery enabled.
The `resource_metadata` parameter of the challenge is chosen by the server, and the client fetched whatever it named. That is a request made on the server's behalf, to a host it picked, from wherever the client runs — which may be inside a network the server cannot reach. RFC 9728 derives the location from the resource identifier, so a document describing this resource lives on this resource. A location anywhere else is now ignored and the well-known paths are probed instead, exactly as for a challenge that named none.
From a second review pass over the same threat model. * Validate the authorization server issuer before its metadata is fetched. The https check sat in `fromArray()`, which only ever sees a response body — so `authorization_servers: ["http://10.0.0.5:8080/x"]` still produced two GETs to that host before anything objected. * Never quote a 200 response body into the token exception. The guard fired on "200 but not JSON" too, so a token endpoint answering form-encoded put a live access token into a message that reaches the application and its logs. * Bind the token to the resource the server published, not to the RFC 8707 `resource` parameter. Overriding that parameter chooses which token to ask for, not where it may be spent — as written, the guard added in 8ebdc85 dropped the header forever and re-ran the browser flow on every request. * Unescape challenge parameters the way HTTP defines it, one backslash before one character, rather than as C escapes. * Refuse to hand a non-HTTPS URL to the desktop's URL opener.
Replaces the hand-rolled RFC 7523 assertion, including the ECDSA DER-to-JOSE conversion, with the library the server side already uses for the other direction. `TokenEndpoint` goes from 237 to 182 lines and loses the most delicate code in the component. Suggested rather than required, and guarded with class_exists like `JwtTokenValidator` is: private_key_jwt is the only path that needs it. Signing with the HMAC family is now refused outright. That is what `client_secret_jwt` uses, and handing it a private key would sign the assertion with the key material as a shared secret — a downgrade the hand-rolled version could not express, since it only knew RS and ES. Gains PS256, ES256K and EdDSA; loses ES512, which the library does not support. The conformance ES256 scenario still passes 8/8.
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.
Client-side authorization. Pass
auth:toHttpTransportand a401turns intodiscovery, an authorization code and a token, then a retry — nothing else changes at
the call site:
Nothing happens until a server actually asks: a client configured with
auth:talks toan unprotected server exactly as one without it does.
What it covers
application_type, a pre-registered client id, or a client id metadata document URLnone,client_secret_basic,client_secret_post,private_key_jwt(viafirebase/php-jwt, suggested not required)scopes_supported, else omitted; step-up unions with what was already granted;offline_accessonly where the server offers it, and dropped if refused; re-authorization cappedOAuth::forApplication()for a user with a browser,OAuth::forServiceAccount()for adaemon,
BearerTokenfor a token you already have. Three seams are interfaces:AuthorizationHandlerInterface(loopback listener, console prompt, headless),CredentialStorageInterface(in-memory,0600file), andAuthenticatingHttpClient,which is the same behaviour as a plain PSR-18 decorator.
Documented in
docs/client/authorization.md, with a runnable example inexamples/client/oauth_client.php.Security posture
An MCP server is not a trusted party: it chooses its challenge, its metadata, and
therefore which authorization server the client is about to talk to. Three of the four
commits here are a review of the flow with that assumption, and closed nine issues that
conformance does not exercise — it drives the happy path with a cooperative server.
What the client now refuses:
resourcethat is not the endpoint being calledissueris not the issuer it was fetched forresource_metadatalocation on another originhttpsstatethis client sentissthat is not the issuer the flow started with, byte for byteissmissing when the server said it would send onesetLegacyDiscovery(true)401again, foreverTwo properties keep a mistake elsewhere from becoming a leak: a token is only sent to
the resource it was minted for (not the host —
…/mcpdoes not authorize…/other),and credentials are keyed by authorization server issuer, so a resource that moves
gets a fresh registration rather than the previous server's client id.
Also fixed along the way: the authorization code was reaching debug logs, a live access
token could reach an exception message via a
200with a non-JSON body, and thecredential file was briefly world-readable between write and
chmod.The security of this PR lives in about fourteen decisions, not three thousand lines. For
a reviewer short on time, these are the ones that matter:
What no client-side check can catch: if a hostile server points at an authorization
server and the user signs in and consents there, the token is what they agreed to. The
authorization URL goes in front of the user for that reason.
Conformance
Client suite,
--suite all, against@modelcontextprotocol/conformance@0.2.0-alpha.11:2025-11-252026-07-28Every
auth/*scenario passes at both revisions, plus the extension and back-compat onesthe suite does not run by default (
client-credentials-basic,client-credentials-jwt,enterprise-managed-authorization, both2025-03-26scenarios). The seven remainingfailures at
2025-11-25areelicitation-sep1034-client-defaultsandsse-retry, bothunrelated and still baselined.
http-invalid-tool-headerscame off the baseline: itpasses now that a rejected request stops instead of being retried four times.
Server conformance unchanged at 100% on both revisions.
Tests
Three layers, because conformance proves the client follows the specification but not
that it works against a real identity provider:
including a
private_key_jwtassertion verified against its public key and bothsocket-based authorization handlers driven over a real socket.
authorization server that enforces PKCE, and an MCP server built with this SDK.
Asserts what actually went on the wire.
make e2e-tests) — Docker Compose with Keycloak, the SDK'sauthorization middleware validating Keycloak's own signed tokens, and a client that
starts with nothing and signs
demoin at the real login form, unattended. Wired intothe pipeline as its own job;
tests/E2E/README.mdhas the details.Decisions
Both settled; one request follows them.
1. The JWT signing moved to a library —
firebase/php-jwt. The hand-rolled RFC 7523 assertion and its ECDSA DER→JOSE conversion are gone;TokenEndpointdrops from 237 to 182 lines and loses the most delicate code in thecomponent. It is a
suggest, guarded withclass_existsexactly asJwtTokenValidatoralready guards the same package on the server side, so
private_key_jwtis the onlypath that needs it and nothing else changes for a consumer.
Two behaviour notes worth a reviewer's eye: signing with the HMAC family is now refused
outright, because that is
client_secret_jwtand handing it a private key would sign theassertion with the key material as a shared secret — a downgrade the old code could not
express, since it only knew RS and ES. And the supported set gains
PS256,ES256KandEdDSAbut losesES512, which the library does not implement.For the rest of the flow the answer is no, and it was measured rather than asserted.
A spike replacing the authorization code, refresh and client credentials grants with
league/oauth2-clientpasses conformance, and makes the code larger — 3024 → 3063lines — while adding six runtime packages
including Guzzle, because
AbstractProvidertypes its HTTP collaborator againstGuzzleHttp\ClientInterfacerather than PSR-18. That breaks 13 unit tests and means theclient a caller configures never reaches the token endpoint. Only ~14% of this code is
generic OAuth at all; the rest is MCP-specific. Symfony has nothing client-side —
symfony/security-httpis the resource-server half, which this SDK already implements.2.
setLegacyDiscovery()defaults to off, and stays off. A server that publishes noprotected resource metadata leaves nothing to check its authorization server against,
and every revision since
2025-06-18requires the document. Reaching a2025-03-26-eraserver is therefore opt-in rather than automatic — a deliberate compatibility break in
favour of the safer default.
A request, not a decision — a second reviewer on the fourteen lines above. Two review passes over this found
nine issues, several of them after the code was already passing 100% of the conformance
suite — including one where the first fix was in the wrong place and had to be moved.
Neither pass found anything by running the suite. Given where this code sits, more eyes
on that short list are worth more than more tests.
Issues
Closes modelcontextprotocol#315, modelcontextprotocol#316, modelcontextprotocol#317, modelcontextprotocol#318, modelcontextprotocol#319, modelcontextprotocol#320, modelcontextprotocol#321, modelcontextprotocol#322, modelcontextprotocol#323, modelcontextprotocol#324, modelcontextprotocol#325, modelcontextprotocol#326, modelcontextprotocol#329,
modelcontextprotocol#360, modelcontextprotocol#361, modelcontextprotocol#363, modelcontextprotocol#376, modelcontextprotocol#377. Covers the client half of modelcontextprotocol#338.
Not attempted: DPoP (
auth/dpop,auth/dpop-nonce) and workload identity federation(
auth/wif-jwt-bearer) — conformance extensions with no issue filed against them.