-
Notifications
You must be signed in to change notification settings - Fork 748
Implement RFC 9207 issuer validation in ClientOAuthProvider #1605
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?
Changes from all commits
0bb5fb7
83f95f4
cfadb00
5fa4edf
dc79995
82a9d74
ea8c1e0
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 |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| namespace ModelContextProtocol.Authentication; | ||
|
|
||
| /// <summary> | ||
| /// Provides the information needed to complete an OAuth authorization request. | ||
| /// </summary> | ||
| public sealed class AuthorizationCallbackContext | ||
| { | ||
| /// <summary> | ||
| /// Gets the authorization URI that the user needs to visit. | ||
| /// </summary> | ||
| public required Uri AuthorizationUri { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the redirect URI where the authorization response will be sent. | ||
| /// </summary> | ||
| public required Uri RedirectUri { get; init; } | ||
| } |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| namespace ModelContextProtocol.Authentication; | ||
|
|
||
| /// <summary> | ||
| /// Represents the result of an OAuth authorization redirect, containing the authorization code | ||
| /// and optionally the issuer identifier from the authorization response. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// The <see cref="Iss"/> property should be populated from the <c>iss</c> query parameter in the | ||
| /// redirect URI when present, as specified by | ||
| /// <see href="https://datatracker.ietf.org/doc/html/rfc9207">RFC 9207</see>. | ||
| /// This enables the SDK to validate that the authorization response originated from the expected | ||
| /// authorization server, mitigating mix-up attacks. | ||
| /// </para> | ||
| /// </remarks> | ||
| public sealed class AuthorizationResult | ||
| { | ||
| /// <summary> | ||
| /// Gets the authorization code returned by the authorization server. | ||
| /// </summary> | ||
| public string? Code { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the issuer identifier returned in the authorization response per | ||
| /// <see href="https://datatracker.ietf.org/doc/html/rfc9207">RFC 9207</see>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// This value should be extracted from the <c>iss</c> query parameter of the redirect URI. | ||
| /// When present, the SDK validates it against the expected authorization server issuer to | ||
| /// prevent mix-up attacks. | ||
| /// </para> | ||
| /// <para> | ||
| /// Implementations of <see cref="ClientOAuthOptions.AuthorizationCallbackHandler"/> should populate this | ||
| /// property whenever the <c>iss</c> parameter is present in the redirect URI callback. | ||
| /// </para> | ||
| /// </remarks> | ||
| public string? Iss { get; init; } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient | |
| private readonly ScopeSelectorDelegate? _scopeSelector; | ||
| private readonly IDictionary<string, string> _additionalAuthorizationParameters; | ||
| private readonly Func<IReadOnlyList<Uri>, Uri?> _authServerSelector; | ||
| private readonly AuthorizationRedirectDelegate _authorizationRedirectDelegate; | ||
| private readonly Func<AuthorizationCallbackContext, CancellationToken, Task<AuthorizationResult?>> _authorizationCallbackHandler; | ||
| private readonly Uri? _clientMetadataDocumentUri; | ||
|
|
||
| // _dcrClientName, _dcrClientUri, _dcrInitialAccessToken and _dcrResponseDelegate are used for dynamic client registration (RFC 7591) | ||
|
|
@@ -92,8 +92,8 @@ public ClientOAuthProvider( | |
| // Set up authorization server selection strategy | ||
| _authServerSelector = options.AuthServerSelector ?? DefaultAuthServerSelector; | ||
|
|
||
| // Set up authorization URL handler (use default if not provided) | ||
| _authorizationRedirectDelegate = options.AuthorizationRedirectDelegate ?? DefaultAuthorizationUrlHandler; | ||
| // Set up authorization callback handler (use default if not provided) | ||
| _authorizationCallbackHandler = options.AuthorizationCallbackHandler ?? DefaultAuthorizationUrlHandler; | ||
|
|
||
| _dcrClientName = options.DynamicClientRegistration?.ClientName; | ||
| _dcrClientUri = options.DynamicClientRegistration?.ClientUri; | ||
|
|
@@ -112,18 +112,19 @@ public ClientOAuthProvider( | |
| /// <summary> | ||
| /// Default authorization URL handler that displays the URL to the user for manual input. | ||
| /// </summary> | ||
| /// <param name="authorizationUrl">The authorization URL to handle.</param> | ||
| /// <param name="redirectUri">The redirect URI where the authorization code will be sent.</param> | ||
| /// <param name="context">The context containing the authorization and redirect URIs.</param> | ||
| /// <param name="cancellationToken">The <see cref="CancellationToken"/> to monitor for cancellation requests.</param> | ||
| /// <returns>The authorization code entered by the user, or null if none was provided.</returns> | ||
| private static Task<string?> DefaultAuthorizationUrlHandler(Uri authorizationUrl, Uri redirectUri, CancellationToken cancellationToken) | ||
| /// <returns>The authorization result entered by the user, or null if none was provided.</returns> | ||
| private static Task<AuthorizationResult?> DefaultAuthorizationUrlHandler( | ||
| AuthorizationCallbackContext context, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| Console.WriteLine($"Please open the following URL in your browser to authorize the application:"); | ||
| Console.WriteLine($"{authorizationUrl}"); | ||
| Console.WriteLine($"{context.AuthorizationUri}"); | ||
| Console.WriteLine(); | ||
| Console.Write("Enter the authorization code from the redirect URL: "); | ||
| var authorizationCode = Console.ReadLine(); | ||
| return Task.FromResult<string?>(authorizationCode); | ||
| return Task.FromResult<AuthorizationResult?>(new() { Code = authorizationCode }); | ||
| } | ||
|
|
||
| internal override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, JsonRpcMessage? message, CancellationToken cancellationToken) | ||
|
|
@@ -400,6 +401,24 @@ private async Task<AuthorizationServerMetadata> GetAuthServerMetadataAsync(Uri a | |
| metadata.TokenEndpointAuthMethodsSupported ??= ["client_secret_post"]; | ||
| metadata.CodeChallengeMethodsSupported ??= ["S256"]; | ||
|
|
||
| // Validate the issuer in the metadata document per RFC 8414 Section 3.3: | ||
| // the issuer value MUST be identical to the issuer identifier used to construct | ||
| // the well-known URL. | ||
| // Skip validation in legacy backcompat mode (resourceUri is null) because the | ||
| // authServerUri was derived from the server origin rather than from Protected | ||
| // Resource Metadata, so it may not match the server's canonical issuer. | ||
| // Note: resourceUri is null exclusively in the 2025-03-26 legacy path. For newer | ||
| // protocol versions, ExtractProtectedResourceMetadata throws if the PRM document | ||
| // omits the resource field (VerifyResourceMatch returns false for null Resource), | ||
| // so we never reach this point with resourceUri == null in non-legacy flows. | ||
| if (resourceUri is not null && | ||
| metadata.Issuer is not null && | ||
| !string.Equals(metadata.Issuer.OriginalString, authServerUri.OriginalString, StringComparison.Ordinal)) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse( | ||
| $"Authorization server metadata issuer '{metadata.Issuer}' does not match the expected issuer '{authServerUri}' (RFC 8414 Section 3.3)."); | ||
| } | ||
|
|
||
| return metadata; | ||
| } | ||
| catch (Exception ex) | ||
|
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. These validations (the issuer mismatch here, plus the Suggest letting it propagate, e.g. add a typed |
||
|
|
@@ -492,14 +511,28 @@ private async Task<string> InitiateAuthorizationCodeFlowAsync( | |
| var codeChallenge = GenerateCodeChallenge(codeVerifier); | ||
|
|
||
| var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge); | ||
|
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. nit (pre-existing, out of scope): the authorization URL built here carries no state parameter, which is the usual CSRF/binding defense for the redirect. Not introduced by this PR, but flagging since it is adjacent to this auth work. |
||
| var authCode = await _authorizationRedirectDelegate(authUrl, _redirectUri, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| if (string.IsNullOrEmpty(authCode)) | ||
| var authResult = await _authorizationCallbackHandler( | ||
| new AuthorizationCallbackContext | ||
| { | ||
| AuthorizationUri = authUrl, | ||
| RedirectUri = _redirectUri, | ||
| }, | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
||
| if (authResult is null || string.IsNullOrEmpty(authResult.Code)) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse($"The {nameof(AuthorizationRedirectDelegate)} returned a null or empty authorization code."); | ||
| ThrowFailedToHandleUnauthorizedResponse($"The {nameof(ClientOAuthOptions.AuthorizationCallbackHandler)} returned a null or empty authorization code."); | ||
| } | ||
|
|
||
| return await ExchangeCodeForTokenAsync(protectedResourceMetadata, authServerMetadata, authCode!, codeVerifier, cancellationToken).ConfigureAwait(false); | ||
| ValidateIssuerResponse(authResult!.Iss, authServerMetadata); | ||
|
|
||
| return await ExchangeCodeForTokenAsync( | ||
| protectedResourceMetadata, | ||
| authServerMetadata, | ||
| authResult.Code!, | ||
| codeVerifier, | ||
| cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private Uri BuildAuthorizationUrl( | ||
|
|
@@ -879,6 +912,47 @@ private bool ChallengeIntroducesNewScopes(ProtectedResourceMetadata protectedRes | |
| return scope + " " + OfflineAccess; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Validates the <c>iss</c> parameter from an authorization response per | ||
| /// <see href="https://datatracker.ietf.org/doc/html/rfc9207">RFC 9207</see>. | ||
| /// </summary> | ||
| /// <param name="iss">The issuer identifier received in the authorization response, or null if absent.</param> | ||
| /// <param name="authServerMetadata">The authorization server metadata containing the expected issuer.</param> | ||
| private void ValidateIssuerResponse(string? iss, AuthorizationServerMetadata authServerMetadata) | ||
| { | ||
| var expectedIssuer = authServerMetadata.Issuer?.OriginalString; | ||
|
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. nit: expectedIssuer can be null when the server metadata omits issuer. If the server advertises iss support (or returns an iss without advertising it), the comparison below runs against null and always fails with a confusing |
||
|
|
||
| if (authServerMetadata.AuthorizationResponseIssParameterSupported) | ||
|
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. nit: iss validation is gated entirely on the server advertising AuthorizationResponseIssParameterSupported. A mix-up authorization server can skip validation by omitting both the advertisement and the iss value. This is inherent to RFC 9207, but a short comment documenting the limitation would help. |
||
| { | ||
| // Server advertises iss support: iss MUST be present and match. | ||
| if (string.IsNullOrEmpty(iss)) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse( | ||
| "Authorization server advertises RFC 9207 iss parameter support but none was received in the authorization response."); | ||
| } | ||
|
|
||
| // Use exact string comparison per RFC 9207 / RFC 3986 §6.2.1. | ||
| if (!string.Equals(iss, expectedIssuer, StringComparison.Ordinal)) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse( | ||
| $"Authorization response issuer '{iss}' does not match expected issuer '{expectedIssuer}'."); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Server does not advertise iss support: if iss is present, still validate it. | ||
| if (!string.IsNullOrEmpty(iss)) | ||
| { | ||
| if (!string.Equals(iss, expectedIssuer, StringComparison.Ordinal)) | ||
| { | ||
| ThrowFailedToHandleUnauthorizedResponse( | ||
| $"Authorization response issuer '{iss}' does not match expected issuer '{expectedIssuer}'."); | ||
| } | ||
| } | ||
| // If iss is absent and not advertised, proceed normally. | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Verifies that the resource URI in the metadata matches the original request URL. | ||
| /// Accepts either an exact match with the full request URL, or a match with the base 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.
nit: OriginalString + Ordinal is spec-correct for RFC 8414's exact match, but comparing raw original strings can reject otherwise-compliant servers over trailing-slash, case, or percent-encoding differences. Worth considering light normalization, or at least a note about the strictness.