-
Notifications
You must be signed in to change notification settings - Fork 584
Make Www-Authenticate resource optional #1041
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -193,39 +193,56 @@ private async Task PerformOAuthAuthorizationAsync( | |
| CancellationToken cancellationToken) | ||
| { | ||
| // Get available authorization servers from the 401 response | ||
| var protectedResourceMetadata = await ExtractProtectedResourceMetadata(response, _serverUrl, cancellationToken).ConfigureAwait(false); | ||
| var availableAuthorizationServers = protectedResourceMetadata.AuthorizationServers; | ||
|
|
||
| if (availableAuthorizationServers.Count == 0) | ||
| ProtectedResourceMetadata? protectedResourceMetadata = null; | ||
| try | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse("No authorization servers found in authentication challenge"); | ||
| protectedResourceMetadata = await ExtractProtectedResourceMetadata(response, _serverUrl, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| // Select authorization server using configured strategy | ||
| var selectedAuthServer = _authServerSelector(availableAuthorizationServers); | ||
|
|
||
| if (selectedAuthServer is null) | ||
| catch | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse($"Authorization server selection returned null. Available servers: {string.Join(", ", availableAuthorizationServers)}"); | ||
| protectedResourceMetadata = null; | ||
| } | ||
|
|
||
| if (!availableAuthorizationServers.Contains(selectedAuthServer)) | ||
| Uri authServer; | ||
| if (protectedResourceMetadata is not null) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse($"Authorization server selector returned a server not in the available list: {selectedAuthServer}. Available servers: {string.Join(", ", availableAuthorizationServers)}"); | ||
| } | ||
| var availableAuthorizationServers = protectedResourceMetadata.AuthorizationServers; | ||
| if (availableAuthorizationServers.Count == 0) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse("No authorization servers found in authentication challenge"); | ||
| } | ||
|
|
||
| LogSelectedAuthorizationServer(selectedAuthServer, availableAuthorizationServers.Count); | ||
| // Select authorization server using configured strategy | ||
| var selectedAuthServer = _authServerSelector(availableAuthorizationServers); | ||
| if (selectedAuthServer is null) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse($"Authorization server selection returned null. Available servers: {string.Join(", ", availableAuthorizationServers)}"); | ||
| } | ||
|
|
||
| if (!availableAuthorizationServers.Contains(selectedAuthServer)) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse($"Authorization server selector returned a server not in the available list: {selectedAuthServer}. Available servers: {string.Join(", ", availableAuthorizationServers)}"); | ||
| } | ||
|
|
||
| LogSelectedAuthorizationServer(selectedAuthServer, availableAuthorizationServers.Count); | ||
|
|
||
| authServer = selectedAuthServer; | ||
| } | ||
| else | ||
| { | ||
| authServer = new(_serverUrl.GetLeftPart(UriPartial.Authority)); | ||
| } | ||
|
|
||
| // Get auth server metadata | ||
| var authServerMetadata = await GetAuthServerMetadataAsync(selectedAuthServer, cancellationToken).ConfigureAwait(false); | ||
| var authServerMetadata = await GetAuthServerMetadataAsync(authServer, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // Store auth server metadata for future refresh operations | ||
| _authServerMetadata = authServerMetadata; | ||
|
|
||
| // The existing access token must be invalid to have resulted in a 401 response, but refresh might still work. | ||
| if (await _tokenCache.GetTokensAsync(cancellationToken).ConfigureAwait(false) is { RefreshToken: { } refreshToken }) | ||
| { | ||
| var refreshedTokens = await RefreshTokenAsync(refreshToken, protectedResourceMetadata.Resource, authServerMetadata, cancellationToken).ConfigureAwait(false); | ||
| var refreshedTokens = await RefreshTokenAsync(refreshToken, protectedResourceMetadata?.Resource, authServerMetadata, cancellationToken).ConfigureAwait(false); | ||
| if (refreshedTokens is not null) | ||
| { | ||
| // A non-null result indicates the refresh succeeded and the new tokens have been stored. | ||
|
|
@@ -325,20 +342,24 @@ private async Task<AuthorizationServerMetadata> GetAuthServerMetadataAsync(Uri a | |
| throw new McpException($"Failed to find .well-known/openid-configuration or .well-known/oauth-authorization-server metadata for authorization server: '{authServerUri}'"); | ||
| } | ||
|
|
||
| private async Task<TokenContainer?> RefreshTokenAsync(string refreshToken, Uri resourceUri, AuthorizationServerMetadata authServerMetadata, CancellationToken cancellationToken) | ||
| private async Task<TokenContainer?> RefreshTokenAsync(string refreshToken, Uri? resourceUri, AuthorizationServerMetadata authServerMetadata, CancellationToken cancellationToken) | ||
| { | ||
| var requestContent = new FormUrlEncodedContent(new Dictionary<string, string> | ||
| var requestContent = new Dictionary<string, string> | ||
| { | ||
| ["grant_type"] = "refresh_token", | ||
| ["refresh_token"] = refreshToken, | ||
| ["client_id"] = GetClientIdOrThrow(), | ||
| ["client_secret"] = _clientSecret ?? string.Empty, | ||
| ["resource"] = resourceUri.ToString(), | ||
| }); | ||
| }; | ||
|
|
||
| if (resourceUri is not null) | ||
| { | ||
| requestContent.Add("resource", resourceUri.ToString()); | ||
| } | ||
|
|
||
| using var request = new HttpRequestMessage(HttpMethod.Post, authServerMetadata.TokenEndpoint) | ||
| { | ||
| Content = requestContent | ||
| Content = new FormUrlEncodedContent(requestContent) | ||
| }; | ||
|
|
||
| using var httpResponse = await _httpClient.SendAsync(request, cancellationToken).ConfigureAwait(false); | ||
|
|
@@ -352,7 +373,7 @@ private async Task<AuthorizationServerMetadata> GetAuthServerMetadataAsync(Uri a | |
| } | ||
|
|
||
| private async Task InitiateAuthorizationCodeFlowAsync( | ||
| ProtectedResourceMetadata protectedResourceMetadata, | ||
| ProtectedResourceMetadata? protectedResourceMetadata, | ||
| AuthorizationServerMetadata authServerMetadata, | ||
| CancellationToken cancellationToken) | ||
| { | ||
|
|
@@ -371,7 +392,7 @@ private async Task InitiateAuthorizationCodeFlowAsync( | |
| } | ||
|
|
||
| private Uri BuildAuthorizationUrl( | ||
| ProtectedResourceMetadata protectedResourceMetadata, | ||
| ProtectedResourceMetadata? protectedResourceMetadata, | ||
| AuthorizationServerMetadata authServerMetadata, | ||
| string codeChallenge) | ||
| { | ||
|
|
@@ -382,13 +403,17 @@ private Uri BuildAuthorizationUrl( | |
| ["response_type"] = "code", | ||
| ["code_challenge"] = codeChallenge, | ||
| ["code_challenge_method"] = "S256", | ||
| ["resource"] = protectedResourceMetadata.Resource.ToString(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This resource is not optional. This is part of the reason why we cannot swallow exceptions from
The spec mandates this, because otherwise the OAuth server has no way to know where the client plans to and therefore could not prevent a phishing attack causing the client to send tokens to an untrusted MCP server. This is explained in https://den.dev/blog/mcp-authorization-resource/. |
||
| }; | ||
|
|
||
| var scopesSupported = protectedResourceMetadata.ScopesSupported; | ||
| if (_scopes is not null || scopesSupported.Count > 0) | ||
| if (protectedResourceMetadata is not null) | ||
| { | ||
| queryParamsDictionary["scope"] = string.Join(" ", _scopes ?? scopesSupported.ToArray()); | ||
| queryParamsDictionary.Add("resource", protectedResourceMetadata.Resource.ToString()); | ||
|
|
||
| var scopesSupported = protectedResourceMetadata.ScopesSupported; | ||
| if (_scopes is not null || scopesSupported.Count > 0) | ||
| { | ||
| queryParamsDictionary["scope"] = string.Join(" ", _scopes ?? scopesSupported.ToArray()); | ||
| } | ||
| } | ||
|
|
||
| // Add extra parameters if provided. Load into a dictionary before constructing to avoid overwiting values. | ||
|
|
@@ -412,26 +437,30 @@ private Uri BuildAuthorizationUrl( | |
| } | ||
|
|
||
| private async Task ExchangeCodeForTokenAsync( | ||
| ProtectedResourceMetadata protectedResourceMetadata, | ||
| ProtectedResourceMetadata? protectedResourceMetadata, | ||
| AuthorizationServerMetadata authServerMetadata, | ||
| string authorizationCode, | ||
| string codeVerifier, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| var requestContent = new FormUrlEncodedContent(new Dictionary<string, string> | ||
| var requestContent = new Dictionary<string, string> | ||
| { | ||
| ["grant_type"] = "authorization_code", | ||
| ["code"] = authorizationCode, | ||
| ["redirect_uri"] = _redirectUri.ToString(), | ||
| ["client_id"] = GetClientIdOrThrow(), | ||
| ["code_verifier"] = codeVerifier, | ||
| ["client_secret"] = _clientSecret ?? string.Empty, | ||
| ["resource"] = protectedResourceMetadata.Resource.ToString(), | ||
| }); | ||
| }; | ||
|
|
||
| if (protectedResourceMetadata is not null) | ||
| { | ||
| requestContent.Add("resource", protectedResourceMetadata.Resource.ToString()); | ||
| } | ||
|
|
||
| using var request = new HttpRequestMessage(HttpMethod.Post, authServerMetadata.TokenEndpoint) | ||
| { | ||
| Content = requestContent | ||
| Content = new FormUrlEncodedContent(requestContent) | ||
| }; | ||
|
|
||
| using var httpResponse = await _httpClient.SendAsync(request, cancellationToken).ConfigureAwait(false); | ||
|
|
@@ -627,27 +656,45 @@ private async Task<ProtectedResourceMetadata> ExtractProtectedResourceMetadata(H | |
| } | ||
|
|
||
| // Look for the Bearer authentication scheme with resource_metadata parameter | ||
| string? resourceMetadataUrl = null; | ||
| List<Uri> candidateMetadataUris = []; | ||
| foreach (var header in response.Headers.WwwAuthenticate) | ||
| { | ||
| if (string.Equals(header.Scheme, BearerScheme, StringComparison.OrdinalIgnoreCase) && !string.IsNullOrEmpty(header.Parameter)) | ||
| { | ||
| resourceMetadataUrl = ParseWwwAuthenticateParameters(header.Parameter, "resource_metadata"); | ||
| var resourceMetadataUrl = ParseWwwAuthenticateParameters(header.Parameter, "resource_metadata"); | ||
| if (resourceMetadataUrl != null) | ||
| { | ||
| candidateMetadataUris.Add(new(resourceMetadataUrl)); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (resourceMetadataUrl == null) | ||
| Uri wellKnownResourceUriRoot = new(serverUrl.GetLeftPart(UriPartial.Authority) + "/.well-known/oauth-protected-resource"); | ||
| candidateMetadataUris.Add(wellKnownResourceUriRoot); | ||
| candidateMetadataUris.Add(new(wellKnownResourceUriRoot, serverUrl.AbsolutePath)); | ||
|
|
||
| ProtectedResourceMetadata? metadata = null; | ||
| foreach (var candidateUri in candidateMetadataUris) | ||
| { | ||
| throw new McpException("The WWW-Authenticate header does not contain a resource_metadata parameter"); | ||
| try | ||
| { | ||
| metadata = await FetchProtectedResourceMetadataAsync(candidateUri, cancellationToken).ConfigureAwait(false); | ||
| if (metadata is not null) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| Uri metadataUri = new(resourceMetadataUrl); | ||
| var metadata = await FetchProtectedResourceMetadataAsync(metadataUri, cancellationToken).ConfigureAwait(false) | ||
| ?? throw new McpException($"Failed to fetch resource metadata from {resourceMetadataUrl}"); | ||
| if (metadata == null) | ||
| { | ||
| throw new McpException($"Failed to fetch resource metadata"); | ||
| } | ||
|
|
||
| // Per RFC: The resource value must be identical to the URL that the client used | ||
| // to make the request to the resource server | ||
|
|
||
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 shouldn't blindly catch all exceptions. For example, this swallows the
throw McpException($"Failed to fetch resource metadata")this PR adds to ExtractProtectedResourceMetadata even though this should be enough to cause McpClient.CreateAsync to throw due to an authentication failure.