- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.7k
 
Implement RFC 7523 JWT flows #1247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement RFC 7523 JWT flows #1247
Conversation
This reverts commit 20b5dfc.
…-sdk into feat/client-credentials
…-sdk into feat/client-credentials
…-sdk into feat/client-credentials
No longer doing external integration examples as of modelcontextprotocol#1011. Will likely bring this back later as a standalone example.
Python default parameters reuse their references, so we can't use a collection like a dict as a default parameter value or we'll dirty our state.
| 
           Marking this as a draft for now to remove from our review queue while we wait for the SEP approval - modelcontextprotocol/modelcontextprotocol#1047 Please reopen once the SEP has been accepted or if you specifically need input beforehand!  | 
    
| 
           SEP-1046 is accepted; reopening this (will fix merge conflicts).  | 
    
| 
           Per discussion on Discord,   | 
    
| token_data["client_assertion_type"] = "urn:ietf:params:oauth:client-assertion-type:jwt-bearer" | ||
| # We need to set the audience to the token endpoint, the audience is difference from the one in claims | ||
| # it represents the resource server that will validate the token | ||
| token_data["audience"] = self.context.get_resource_url() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you'll want to use the issuer URL here:
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-rfc7523bis-01#name-updates-to-rfc-7523
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment here didn't match the required change; I've updated it in the latest commit - I believe the aud claim is supposed to be updated to the issuer, not this.
issuer = str(self.context.oauth_metadata.issuer)
assertion = self.jwt_parameters.to_assertion(with_audience_fallback=issuer)This line is actually setting the audience parameter in the token exchange request:
token_data["audience"] = self.context.get_resource_url()as described in RFC 8693.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yep i had the wrong line. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| Implements authorization code flow with PKCE and automatic token refresh. | ||
| """ | ||
| 
               | 
          ||
| from mcp.client.auth.oauth2 import * # noqa: F403 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the right imports here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean all classes one by one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in #1532
This addresses issues introduced in PR #1247: 1. Fixed pyright reportMissingTypeStubs error by adding __init__.py to the extensions directory, making it a proper Python package 2. Replaced wildcard import in auth/__init__.py with explicit imports as requested in code review The changes ensure type checking passes and maintain backward compatibility with existing code that imports from mcp.client.auth.
Implements support for the RFC 7523 authentication flows. This PR is a trimmed-down version of #1020, but will likely be superceded by a future
authlib-based implementation (see #1240).Compared to #1020, this implements the flow via a separate
httpx.Authsubclass, which is in-line with prior maintainer feedback on how to grow these auth implementations.Motivation and Context
Implementation example for modelcontextprotocol/modelcontextprotocol#1046.
How Has This Been Tested?
Unit tests (TBD: unit tests for section 2.2 flow). Planning to spot test with Keycloak setup once that gets published.
Breaking Changes
None.
Types of changes
Checklist
Additional context