-
Notifications
You must be signed in to change notification settings - Fork 294
WIP: Authorization Support (Using ASP.NET Core Native AuthN/AuthZ Integration) #377
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
base: main
Are you sure you want to change the base?
Conversation
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.AspNetCore/Auth/McpAuthenticationResponseMiddlewareExtensions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Halter <[email protected]>
Co-authored-by: Stephen Halter <[email protected]>
…sions.cs Co-authored-by: Stephen Halter <[email protected]>
src/ModelContextProtocol.AspNetCore/Auth/McpAuthorizationExtensions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol/Protocol/Transport/SseClientTransportOptions.cs
Outdated
Show resolved
Hide resolved
// Copy the request content if present | ||
if (request.Content != null) | ||
{ | ||
var contentBytes = await request.Content.ReadAsByteArrayAsync(); |
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 don't think we should put this retry logic in the DelegatingHandler. And even if we keep the AuthorizationDelegatingHandler, we should make it internal so we can change the implementation later.
Instead, we should instead surface the 401 Unauthorized HttpResponseMessages to the SseClientTransport and then have it call into the IMcpAuthorizationProvider and update the Authorization header before resending the request. That way we don't have to worry about the DelegatingHandler ever getting used with non-replayable request bodies.
I don't care that if ExtractServerSupportedSchemes and the like literally live in SseClientTransport.cs, but it should not be part of a DelegatingHandler. We should try to factor the code to make it clear we are only replaying JsonRpcMessage request bodies that we created ourselves.
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.
Thanks. The main reason for suggesting adding McpAuthenticationEvents now is because it would be technically breaking to add later. But looking at the current state, I think that McpAuthenticationOptions.UseDynamicResourceMetadata(Func<HttpContext, ProtectedResourceMetadata> provider) andFunc<HttpContext, ProtectedResourceMetadata> McpAuthenticationOptions.ProtectedResourceMetadataProvider { get; set; } should be replaced by a Func<RetrieveResourceMetadataContext, Task> McpAuthenticationEvents.RetrieveResourceMetadata { get; set; } event similar to what we have for JwtBearerEvents.OnChallenge.
I think the "Use" in UseDynamicResourceMetadata wrongly implies that it's configuring a pipeline rather than being another way for overriding a Func property. All the other callbacks I can think that you can configure for any other auth handlers are always on WhateverAuthHandlerOptions.Events.Whatever, and not directly on the options type.
I know that unlike JWT's OnChallenge event, this gets called for both the challenge and the /.well-known/oauth-protected resource callback, but I think that's fine. We can also continue to just use the _resourceMetadata field by default when the event doesn't configure RetrieveResourceMetadataContext.ResourceMetadataAddress. We'll just document it. The RetreiveResourceMetadataContext would derive from BaseContext like JwtBearerChallengeContext does, and that exposes the HttpContext plus more like the current scheme name.
The other thing you'll notice about the events pattern that types like JwtBearerEvents follow is that that also have virtual methods you can override to implement events as an alternative to setting the property. We should also do that if we make the McpAuthenticationHandler an unsealed public type, since we should design it to be inherited from if that's the case. The alternative is to make the handler internal and seal that plus the still-public options and event types. This would make the AuthenticationBuilder.AddMcp extension method the only way to add the handler, but I think that's okay. We did just that with the AuthenticationBuilder.AddBearerToken method in the internal BearerTokenHandler (not to be confused with the JwtBearerHandler).
I also wonder if it'd be more consistent to make the ResourceMetadataUri Uri a ResourceMetadataAddress string instead for consistency with the type JwtBearerOptions.MetadataAddress and JwtBearerOptions.Authority. I'm aware these are not precisely the same thing. I'm just talking about it in terms of string vs Uri and consistency. This is much more of a nit, because Uri obviously works. And I think it should still bind from config because Uri implements ISpanParsable or there's a TypeConverter or something like that (although we should verify). But when in Rome
This is a lot of functionality without any new tests yet, but I realize this PR is still in WIP state. It shouldn't be too hard to make sure we properly throw for bad arguments and configuration, but end-to-end testing is obviously more difficult since it'd have to involve and OAuth server. How hard would it be to add a super basic fake OAuth server for testing like this?
src/ModelContextProtocol/Authentication/AuthorizationDelegatingHandler.cs
Outdated
Show resolved
Hide resolved
var (handled, recommendedScheme) = await _credentialProvider.HandleUnauthorizedResponseAsync( | ||
response, | ||
bestSchemeMatch, | ||
cancellationToken).ConfigureAwait(false); | ||
|
||
if (!handled) | ||
{ | ||
throw new McpException( | ||
$"Failed to handle unauthorized response with scheme '{bestSchemeMatch}'. " + | ||
"The authentication provider was unable to process the authentication challenge."); | ||
} |
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 don't love having a handled/Success bool and then just immediately rethrowing. Looking at the BasicOAuthProvider, it just logs any exceptions to the console and returns false. Bubbling up the original exception to the caller would make things much easier to diagnose.
I don't mind wrapping any exceptions thrown by HandleUnauthorizedResponseAsync with another exception containing this message, but the only reason to use something like handled in C# is if there's a chance you don't need to throw at all.
/// <returns>The resource metadata if the resource matches the server, otherwise throws an exception.</returns> | ||
/// <exception cref="InvalidOperationException">Thrown when the response is not a 401, lacks a WWW-Authenticate header, | ||
/// lacks a resource_metadata parameter, the metadata can't be fetched, or the resource URI doesn't match the server URL.</exception> | ||
public async Task<ProtectedResourceMetadata> ExtractProtectedResourceMetadata( |
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.
Nit: Move public methods to the top.
But I don't know why this is the public API we're exposing to do OAuth in the client to begin with. I think this should probably be internal, and we should have a public IMcpCredentialProvider that uses this internal method that looks a lot like the current BasicOAuthProvider in the sample accept the part that gets the authorization code should be pluggable. We don't even need a default implementation for that part since we don't want to encourage HttpListener usage.
We should look at making IMcpCredentialProvider look a lot more similar to the typescript-sdk's OAuthClientProvider. Things that are fully dependent on the application's UI framework like string RedirectUri { get; }
which might be a web URL if the client is hosted in an ASP.NET Core app or an "app://" URL for a WPF or MAUI app. That and RedirectToAuthorization(string uri)
should really be the kinds of things the IMcpCredentialProvider is responsible for like typescript's OAuthClientProvider.
I know there was suggestions that the client has to do a lot more validation. I don't have an opinion on that. But whatever we do, we should do as much as we can ourselves and not shirk responsibility for doing things like making the request to the /token endpoint with the appropriate PKCE challenge and leave that up fully to our customers, because it's unnecessary pain for our customers, and at least some our customers are likely to do in incorrectly.
…gHandler.cs Co-authored-by: Stephen Halter <[email protected]>
…Options.cs Co-authored-by: Stephen Halter <[email protected]>
This might not make it in the current PR, but flagging for future use: https://datatracker.ietf.org/doc/html/rfc9728#name-signed-protected-resource-m |
src/ModelContextProtocol/Authentication/AuthorizationDelegatingHandler.cs
Outdated
Show resolved
Hide resolved
Resource = new Uri("http://localhost"), | ||
BearerMethodsSupported = { "header" }, | ||
ResourceDocumentation = new Uri("https://docs.example.com/api/weather"), | ||
AuthorizationServers = { new Uri($"{instance}{tenantId}/v2.0") } |
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.
AuthorizationServers = { new Uri($"{instance}{tenantId}/v2.0") } | |
AuthorizationServers = { new Uri($"{instance}{tenantId}/oauth2/v2.0") } |
shouldn't it be this?
https://learn.microsoft.com/en-us/entra/identity-platform/v2-protocols
|
||
namespace ProtectedMCPClient; | ||
|
||
/// <summary> |
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.
Putting a comment about not using something in production is not enough to stop people from copying for prod code. I've seen people still do it. The only way to prevent it is to not have insecure examples at all.
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.
@DavidParks8 - this is not an inherently insecure example - it shows basic OAuth client implementation. Happy to address specific security issues with it. Please tag the lines where there are potential vulnerabilities/attack vectors.
{ | ||
base.ForwardAuthenticate = "Bearer"; | ||
ResourceMetadataUri = DefaultResourceMetadataUri; | ||
_resourceMetadata = new ProtectedResourceMetadata() { Resource = new Uri("http://localhost") }; |
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.
Should we be using a different default uri? It doesn't seem right to put localhost in something that could be misused. It would probably be better to always require a resource uri and throw if one is not provided.
{ | ||
// Create a policy builder with default MCP configuration | ||
var policyBuilder = new AuthorizationPolicyBuilder() | ||
.RequireAuthenticatedUser() |
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.
We likely should not assume they want RequireAuthenticatedUser. They may want to use an ip-based policy, or some other policy.
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 agree, I would like to create a lot of different policies and like MVC I would like to use the Authorize annotation on the Tool to set the policy
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.
We should just delete this entire McpAuthorizationExtensions class.
People can define their own policies if they want to specify a specific scheme or schemes for specific endpoint. People can also just set AuthorizeAttribute.AuthenticationSchemes without needing to define any policies.
A lot of MCP servers very well might not have any non-MCP endpoints, so making McpAuthenticationDefaults.AuthenticationScheme for all schemes makes perfect sense negating the need for this class. And even if I was writing a server with other API endpoints expecting a JWT, it's very likely I could still use McpAuthenticationDefaults.AuthenticationScheme for every endpoint since it forwards auth to the JwtBearerHandler and effectively just gives a more informational challenge response.
{ | ||
try | ||
{ | ||
var request = new HttpRequestMessage(HttpMethod.Get, metadataUrl); |
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.
inline disposables should be disposed.
/// <summary> | ||
/// The well-known path prefix for resource metadata. | ||
/// </summary> | ||
private static readonly string WellKnownPathPrefix = "/.well-known/"; |
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.
this should be a const
/// </summary> | ||
/// <param name="httpClient">The HTTP client to use for requests. If null, a default HttpClient will be used.</param> | ||
/// <param name="logger">The logger to use. If null, a NullLogger will be used.</param> | ||
public AuthorizationHelpers(HttpClient? httpClient = null, ILogger? logger = null) |
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.
Taking HttpClient directly in the constructor is not going to do so well with dependency injection since raw HttpClient is not typically injected. IHttpClientFactory would be a better choice from a DI and resiliency perspective. As the code is right now, people would have a resiliency problem with a non-recycling connection if they had an AuthorizationHelpers singleton.
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.
since raw HttpClient is not typically injected
It absolutely is. The vast majority case is code gets an HttpClient injected, and the HttpClientFactory in DI handles constructing an appropriate one.
IHttpClientFactory would be a better choice from a DI and resiliency perspective
It's not. We discourage libraries from accepting IHttpClientFactory. It makes them very hard to use in common cases where you already have an HttpClient, and it ties the library to Microsoft.Extensions.Http, which is generally an unnecessary dependency of the library and is instead intended for app-level consumption.
|
||
// Extract the base URI from the metadata URL | ||
Uri urlToValidate = ExtractBaseResourceUri(metadataUri); | ||
_logger.LogDebug($"Validating resource metadata against base URL: {urlToValidate}"); |
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 get this repo isn't performance optimized, but the logging deserves some TLC to stop allocating and boxing so much.
Path = wellKnownIndex > 0 ? metadataUri.AbsolutePath.Substring(0, wellKnownIndex) : "/", | ||
Fragment = string.Empty, | ||
Query = string.Empty, | ||
Port = -1 // Remove port |
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.
Why is this removed? What if the port is non standard?
|
||
var authHandler = new AuthorizationDelegatingHandler(credentialProvider) | ||
{ | ||
InnerHandler = baseMessageHandler ?? new HttpClientHandler() |
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.
The HttpClient needs to be controllable by the consumers of the library (as in, so people can simply use IHttpClientFactory to use its existing handler pipelines).
@@ -34,7 +34,7 @@ public void Constructor_Throws_For_Null_Options() | |||
[Fact] | |||
public void Constructor_Throws_For_Null_HttpClient() | |||
{ | |||
var exception = Assert.Throws<ArgumentNullException>(() => new SseClientTransport(_transportOptions, null!, LoggerFactory)); | |||
var exception = Assert.Throws<ArgumentNullException>(() => new SseClientTransport(_transportOptions, httpClient: null!, LoggerFactory)); |
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.
The new code needs a lot more tests.
…gHandler.cs Co-authored-by: Tyler James Leonhardt <[email protected]>
Implements the authorization flow for clients and servers, per specification. Instead of re-implementing everything from scratch, this follows the suggestions from #349 and uses the native ASP.NET Core constructs to handle post-discovery steps server-side.
Developer experience
Server
HTTP context in tools
.AddHttpContextAccessor
is used to ensure that tools can access the HTTP context (such as the authorization header contents).Tools that want to use the HTTP context will need to amend their signatures to include a reference to
IHttpContextAccessor
, like this:Client