Skip to content

[Client] Add OAuth authorization to the HTTP transport - #53

Open
chr-hertel wants to merge 5 commits into
mainfrom
feat-client-auth
Open

chr-hertel wants to merge 5 commits into
mainfrom
feat-client-auth

Conversation

@chr-hertel

@chr-hertel chr-hertel commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Client-side authorization. Pass auth: to HttpTransport and a 401 turns into
discovery, an authorization code and a token, then a retry — nothing else changes at
the call site:

$client->connect(new HttpTransport(
    'https://mcp.example.com/mcp',
    auth: OAuth::forApplication('My App')->build(),
));

Nothing happens until a server actually asks: a client configured with auth: talks to
an unprotected server exactly as one without it does.

What it covers

Discovery Protected resource metadata (RFC 9728) and authorization server metadata (RFC 8414), including the OpenID Connect spellings
Grants Authorization code with PKCE, refresh token, client credentials, and cross-app access (RFC 8693 token exchange + RFC 7523 assertion)
Client identity Dynamic registration (RFC 7591) with application_type, a pre-registered client id, or a client id metadata document URL
Client authentication none, client_secret_basic, client_secret_post, private_key_jwt (via firebase/php-jwt, suggested not required)
Scopes Challenge first, then the resource's scopes_supported, else omitted; step-up unions with what was already granted; offline_access only where the server offers it, and dropped if refused; re-authorization capped

OAuth::forApplication() for a user with a browser, OAuth::forServiceAccount() for a
daemon, BearerToken for a token you already have. Three seams are interfaces:
AuthorizationHandlerInterface (loopback listener, console prompt, headless),
CredentialStorageInterface (in-memory, 0600 file), and AuthenticatingHttpClient,
which is the same behaviour as a plain PSR-18 decorator.

Documented in docs/client/authorization.md, with a runnable example in
examples/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:

The server says The client does
metadata naming a resource that is not the endpoint being called refuses
AS metadata whose own issuer is not the issuer it was fetched for refuses, and uses no endpoint from that document
a resource_metadata location on another origin ignores it, probes the well-known paths instead
an authorization server, or an endpoint, that is not https refuses, unless the host is loopback — checked before the fetch
an authorization response without the state this client sent refuses
an iss that is not the issuer the flow started with, byte for byte refuses
an iss missing when the server said it would send one refuses
no protected resource metadata at all refuses, unless setLegacyDiscovery(true)
401 again, forever gives up after three authorization attempts

Two properties keep a mistake elsewhere from becoming a leak: a token is only sent to
the resource it was minted for
(not the host — …/mcp does 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 200 with a non-JSON body, and the
credential 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:

OAuthAuthenticator.php:78     token only sent to its own resource
OAuthAuthenticator.php:148    PRM must describe the endpoint being called
OAuthAuthenticator.php:165    issuer validated before its metadata is fetched
OAuthAuthenticator.php:189    metadata location must be same-origin
OAuthAuthenticator.php:268    don't replay a token the server just rejected
OAuthAuthenticator.php:361    PKCE pair, and state
OAuthAuthenticator.php:450    state required and compared
OAuthAuthenticator.php:458    RFC 9207 iss, present and absent cases
MetadataDiscovery.php:92      AS metadata issuer must match what was asked for
ProtectedResourceMetadata.php:75     the coverage rule the rest leans on
AuthorizationServerMetadata.php:123  endpoint transport security
AuthenticatingHttpClient.php:54      re-authorization cap
TokenEndpoint.php             client auth switch, createClientAssertion
FileCredentialStorage.php     the write path

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:

Revision Before After
2025-11-25 12/54 (22%) 234/241 (97%)
2026-07-28 405/405 (100%)

Every auth/* scenario passes at both revisions, plus the extension and back-compat ones
the suite does not run by default (client-credentials-basic, client-credentials-jwt,
enterprise-managed-authorization, both 2025-03-26 scenarios). The seven remaining
failures at 2025-11-25 are elicitation-sep1034-client-defaults and sse-retry, both
unrelated and still baselined. http-invalid-tool-headers came off the baseline: it
passes 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:

  • Unit — 124 tests over the flow, the value objects, the storages and the handlers,
    including a private_key_jwt assertion verified against its public key and both
    socket-based authorization handlers driven over a real socket.
  • Integration — the whole flow over real HTTP against two spawned processes: an
    authorization server that enforces PKCE, and an MCP server built with this SDK.
    Asserts what actually went on the wire.
  • End to end (make e2e-tests) — Docker Compose with Keycloak, the SDK's
    authorization middleware validating Keycloak's own signed tokens, and a client that
    starts with nothing and signs demo in at the real login form, unattended. Wired into
    the pipeline as its own job; tests/E2E/README.md has 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;
TokenEndpoint drops from 237 to 182 lines and loses the most delicate code in the
component. It is a suggest, guarded with class_exists exactly as JwtTokenValidator
already guards the same package on the server side, so private_key_jwt is the only
path 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_jwt and handing it a private key would sign the
assertion 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, ES256K and
EdDSA but loses ES512, 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-client passes conformance, and makes the code larger — 3024 → 3063
lines — while adding six runtime packages
including Guzzle, because AbstractProvider types its HTTP collaborator against
GuzzleHttp\ClientInterface rather than PSR-18. That breaks 13 unit tests and means the
client 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-http is the resource-server half, which this SDK already implements.

2. setLegacyDiscovery() defaults to off, and stays off. A server that publishes no
protected resource metadata leaves nothing to check its authorization server against,
and every revision since 2025-06-18 requires the document. Reaching a 2025-03-26-era
server 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.

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
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.
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.

[Client] Add OAuth 2.0 credential storage interface (TokenStorageInterface)

1 participant