Add client_secret_basic authentication support

[jonshea]

Add support for HTTP Basic Authentication (client_secret_basic) as a
client authentication method for the token and revoke endpoints, alongside
the existing client_secret_post method. This improves compatibility with
OAuth servers like Keycloak that use Basic auth.

Key changes:

• Update OAuthClientMetadata to accept "client_secret_basic" as valid
  token_endpoint_auth_method
• Return 401 status for authentication failures (was 400)
• Update metadata endpoints to advertise both auth methods
• Add tests for both auth methods and edge cases

[ochafik]

Hi @jonshea, thanks for sending this PR!

Looks generally great. Client-side support would be good to add too, e.g. in src/mcp/client/auth.py decide to use basic auth if it's the preferred (first) or only method listed in self.context.oauth_metadata.token_endpoint_auth_methods_supported, and in that case set something like headers["Authorization"] = f"Basic {64encode(f"{self.client_info.client_id}:{self.client_info.client_secret}")}" in both _exchange_token and _refresh_token?

[ochafik]

Maybe except (ValueError, UnicodeDecodeError, base64.binascii.Error): to be more explicit about what exceptions we should tolerate here?

[ochafik]

Are we not reading the request.form() twice here? (once in authenticate_request, and here again? (Think starlette might complain about this)

Might wanna push the form data (maybe other request fields, e.g. auth header) to the authenticator method?

[jonshea]

My understanding is that the current version of starlette caches calls to form(), json(), and body(), so it is safe to call them multiple times. For example, https://github.com/Kludex/starlette/blob/main/starlette/requests.py#L254-L287 . But I am not a starlette expert and I might be misunderstanding. There seems to be an edge case
with reading form() and then body() later. But if it was ever unsafe to call form() multiple times, I was unable to find when it changed after a modest search.

My first implementation parsed out the form in the request handler and passed it into the authenticate method, as you suggest, but I thought it felt clunky to duplicate the code that parsed "form data and auth header" across both the token and revoke endpoints. It’s not the end of the world, but it turns

# Current version
async def handle(self, request: Request):
    try:
        client_info = await self.client_authenticator.authenticate_request(request)

into something like this

# Parse out form and auth_header
async def handle(self, request: Request):
    try:
        form_data = await request.form()
    except Exception:
        return self.response(
            TokenErrorResponse(
                error="invalid_request",
                error_description="Unable to parse request body",
            )
        )

    auth_header = request.headers.get("Authorization")

    try:
        client_info = await self.client_authenticator.authenticate_request(form_data, auth_header)

Or I suppose we could handle invalid for data in the authenticate method:

# Parse out form and auth_header, handle form error in client_authenticator
async def handle(self, request: Request):
    form_data = None
    try:
        form_data = await request.form()
    except Exception:
        pass

    auth_header = request.headers.get("Authorization")

    try:
        client_info = await self.client_authenticator.authenticate_request(form_data, auth_header)

Anyway, I defer to the maintainers. If you would like me to switch to one of the above implementations, I would be happy to do so.

[ochafik]

We should probably urldecode both parts, as per RFC 6749 Section 2.3.1

The client identifier is encoded using the 'application/x-www-form-urlencoded' encoding algorithm per Appendix B, and the encoded value is used as the username; the client password is encoded using the same algorithm and used as the password.

[jonshea]

Good catch. Thank you.

[ochafik]

meganit: in theory we should use compare_digest() here to avoid any chance of timing attack, although it's probably drowned in the noise of network and Python execution overheads.

[jonshea]

I am torn on whether or not we should allow auto-selecting "none". It seems possibly like bad security to allow that, but I suppose if the server allows it then it is ok?

I suppose ideally we should allow the user to pick a list of auth methods they want to allow to be auto-configured, but I am not sure anyone cares enough to want to use it.

[jonshea]

Thank you for the thoughtful review, @ochafik. I believe I have addressed all of the provided feedback, though with a reply / question about the concern around calling request.form() twice rather than a fix.

As suggested, I also added support for client_secret_basic to the MCP client. I made OAuthClientMetadata.token_endpoint_auth_method optional to support auto-configuring a token_endpoint_auth_method. I think this is a reasonable change, but I wanted to call it out. I have a question here about whether or not we should allow the token_endpoint_auth_method auto-configure to pick "none".

I put the client-side client_secret_basic auth support in its own commit in case we would like to pull it into its own PR.

[HenriChabert]

Hello @ochafik and @pcarleton !
Any update on this PR ? Does it seem good enough ?
client_secret_basic is pretty common in the industry, and it would be great if we could integrate servers using it.
Thanks 🙏

changes addressed

[HenriChabert]

Thanks for handling it @pcarleton !