mcp: add client-side OAuth flow (preliminary)#176
Conversation
|
Ignore the first two commits. They should be in separate PRs. |
|
/cc @RoyceLeonD |
|
For testing, Google supports DCR and there is an example of Spotify MCP server to include client id and client secret for the fallback here. This was used to support the vscode's client (currently in insider) at the moment to do exactly this fallback mechanism when DCR does not exist at the IDP. |
|
@jba Sorry It took so long for me to be active on this. Will take a deeper look at the code and revert back in the next day or so. |
| Endpoint: oauth2.Endpoint{ | ||
| AuthURL: asm.AuthorizationEndpoint, | ||
| TokenURL: asm.TokenEndpoint, | ||
| // DeviceAuthURL: "", |
There was a problem hiding this comment.
adding auth style should be necessary depending on the server metadata
https://pkg.go.dev/golang.org/x/oauth2#section-readme:~:text=type-,AuthStyle,-%C2%B6
depending on the AS's token_endpoint_auth_methods_supported.
|
|
||
| // Get an access token from the auth server. | ||
| config := &oauth2.Config{ | ||
| ClientID: "TODO: from registration", |
There was a problem hiding this comment.
I think you should add two options, one for passed in client ids/secrets and one for PKCE for public clients, for a more AUTH2.1 compliant solution.
| } | ||
| // TODO: try more than one? | ||
| authServer := prm.AuthorizationServers[0] | ||
| // TODO: which scopes to ask for? All of them? |
There was a problem hiding this comment.
I don't think we should ask for all the scopes, I think the best solution would be for the client to specify in its options which scopes it wants and validate against the supported ones, failing if not supported.
There was a problem hiding this comment.
and we could default to all, if no scope is passed.
| // AuthStyle: "from auth meta?", | ||
| }, | ||
| RedirectURL: "", // ??? | ||
| Scopes: scopes, |
There was a problem hiding this comment.
If you also request openid, it needs to generate and pass a nonce and validate it in the ID Token.
|
Hi, is there an expected timeline for this feature/pr to be reviewed and merged? |
|
@besmiralia I will make progress on it this week, but client-side auth probably won't be ready until after v1 is out. |
|
would like to help if needed, I have much experience with oauth2 and MCP, we are waiting this feature. |
|
See auth/client.go for where I'm currently at with this. Basically, we do very little for you, other than provide a hook. I'd like to see whether this is sufficient. We can certainly do more, and you can find the PRM and ASM implementations in internal/oauthex. |
|
Heya @jba -- In my opinion, the However, I can also see arguments that it would be great to have another option that supports only the DCR flow (creating and registering them). At the end, it still returns the |
|
I can review this, but don't have much experience with oauth2. It would be really helpful if someone could opine on whether this is necessary and sufficient. Thanks! |
|
Here is a nice writeup around DCR. If anything additional would be added it would be this flow and ignoring the rest -- since there are many different ways the OAuth handshake could occur (device authorization, oidc client id/secret flow, needing to use PKCE over authorization flow) on top of the many different encryption algorithms. |
This is a preliminary implementation of OAuth 2.1 for the client. It provides an http.RoundTripper, auth.HTTPTransport, that invokes a user-provided callback of type auth.OAuthHandler. The latter is responsible for all the OAuth work. We will add code to make that easier in later PRs. Much remains to be done : Dynamic client registration is not implemented. Since it is optional, we also need another way of supplying the client ID and secret to this code. Resource Indicators, as described in section 2.5.1 of the MCP spec. And, of course, tests. We should test against fake implementations but also, if we can find any, real reference implementations.
|
This had gotten quite stale, so I squashed and force-pushed. I will add commits to address Roland's recent comment, and to put this behind an experiment so we can land it and people can start using it. |
| } | ||
| if haveTokenSource { | ||
| // We failed to authorize even with a token source; give up. | ||
| return resp, nil |
There was a problem hiding this comment.
Should we return an error here explaining that we tried to authorize and it failed, or is that going to be handled higher in the stack?
There was a problem hiding this comment.
Higher up. The caller will see the unauthorized status and should perform the OAuth dance again.
| // The handler is invoked when an HTTP request results in a 401 Unauthorized status. | ||
| // It is called only once per transport. Once a TokenSource is obtained, it is used | ||
| // for the lifetime of the transport; subsequent 401s are not processed. | ||
| func NewHTTPTransport(handler OAuthHandler, opts *HTTPTransportOptions) (*HTTPTransport, error) { |
There was a problem hiding this comment.
Check if handler is nil, otherwise we will panic when trying to initialize the token source in RoundTrip.
This is a preliminary implementation of OAuth 2.1 for the client.
When a StreamableClientTransport encounters a 401 Unauthorized response
from the server, it initiates the OAuth flow described in thec
authorization section of the MCP spec
(https://modelcontextprotocol.io/specification/2025-06-18/basic/authorization).
On success, the transport obtains an access token which it passes
to all subsequent requests.
Much remains to be done here:
Dynamic client registration is not implemented. Since it is optional,
we also need another way of supplying the client ID and secret to
this code.
Resource Indicators, as described in section 2.5.1 of the MCP spec.
There is no way for the user to provide a redirect URL.
All of this is unexported, so it is available only to our own
StreamingClientTransport. We should add API so people can use it
with their own transports.
And, of course, tests. We should test against fake implementations
but also, if we can find any, real reference implementations.