Skip to content

Improvements to dynamic client registration #609

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using ModelContextProtocol.Client;
using ModelContextProtocol.Protocol;
using System.Net.Http.Headers;

namespace ModelContextProtocol.Authentication;

Expand All @@ -20,7 +19,7 @@ internal override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage r
{
if (request.Headers.Authorization == null)
{
await AddAuthorizationHeaderAsync(request, _currentScheme, cancellationToken).ConfigureAwait(false);
await credentialProvider.AddAuthorizationHeaderAsync(request, _currentScheme, cancellationToken).ConfigureAwait(false);
}

var response = await base.SendAsync(request, message, cancellationToken).ConfigureAwait(false);
Expand Down Expand Up @@ -78,7 +77,7 @@ private async Task<HttpResponseMessage> HandleUnauthorizedResponseAsync(
}
}

await AddAuthorizationHeaderAsync(retryRequest, _currentScheme, cancellationToken).ConfigureAwait(false);
await credentialProvider.AddAuthorizationHeaderAsync(retryRequest, _currentScheme, cancellationToken).ConfigureAwait(false);
return await base.SendAsync(retryRequest, originalJsonRpcMessage, cancellationToken).ConfigureAwait(false);
}

Expand All @@ -96,23 +95,4 @@ private static HashSet<string> ExtractServerSupportedSchemes(HttpResponseMessage

return serverSchemes;
}

/// <summary>
/// Adds an authorization header to the request.
/// </summary>
private async Task AddAuthorizationHeaderAsync(HttpRequestMessage request, string scheme, CancellationToken cancellationToken)
{
if (request.RequestUri is null)
{
return;
}

var token = await credentialProvider.GetCredentialAsync(scheme, request.RequestUri, cancellationToken).ConfigureAwait(false);
if (string.IsNullOrEmpty(token))
{
return;
}

request.Headers.Authorization = new AuthenticationHeaderValue(scheme, token);
}
}
41 changes: 41 additions & 0 deletions src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,19 @@ public sealed class ClientOAuthOptions
/// </remarks>
public AuthorizationRedirectDelegate? AuthorizationRedirectDelegate { get; set; }

/// <summary>
/// Gets or sets the delegate used for handling the dynamic client registration response.
/// </summary>
/// <remarks>
/// <para>
/// This delegate is responsible for processing the response from the dynamic client registration endpoint.
/// </para>
/// <para>
/// The implementation should save the client credentials securely for future use.
/// </para>
/// </remarks>
public DynamicClientRegistrationDelegate? DynamicClientRegistrationDelegate { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move all the dynamic client registration related configuration to a subproperty called DynamicClientRegistration (the type could be call DynamicClientRegistrationOptions). Then you'd have clientOAuthOptions.DynamicClientRegistration.ResponseDelegate, clientOAuthOptions.DynamicClientRegistration.ClientType, and clientOAuthOptions.DynamicClientRegistration.InitialAccessToken. I think it helps clarify these are only for dynamic client registration when dealing with ClientType and InitialAccessToken in particular.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want the existing properties ClientName and ClientUri in this new class as well? It would be a breaking change though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Good call. We're not too concerned about making these kinds of breaking changes this early.


/// <summary>
/// Gets or sets the authorization server selector function.
/// </summary>
Expand Down Expand Up @@ -85,6 +98,34 @@ public sealed class ClientOAuthOptions
/// </remarks>
public Uri? ClientUri { get; set; }

/// <summary>
/// Gets or sets the client type to use during dynamic client registration.
/// </summary>
/// <remarks>
/// <para>
/// This indicates whether the client is confidential (requires a client secret) or public (does not require a client secret).
/// Only used when a <see cref="ClientId"/> is not specified.
/// </para>
/// <para>
/// When not specified, the client type will default to <see cref="OAuthClientType.Confidential"/>.
/// </para>
/// </remarks>
public OAuthClientType? ClientType { get; set; }

/// <summary>
/// Gets or sets the initial access token to use during dynamic client registration.
/// </summary>
/// <remarks>
/// <para>
/// This token is used to authenticate the client during the registration process.
/// Only used when a <see cref="ClientId"/> is not specified.
/// </para>
/// <para>
/// This is required if the authorization server does not allow anonymous client registration.
/// </para>
/// </remarks>
public string? InitialAccessToken { get; set; }

/// <summary>
/// Gets or sets additional parameters to include in the query string of the OAuth authorization request
/// providing extra information or fulfilling specific requirements of the OAuth provider.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using Microsoft.Extensions.Logging.Abstractions;
using System.Collections.Specialized;
using System.Diagnostics.CodeAnalysis;
using System.Net.Http.Headers;
using System.Security.Cryptography;
using System.Text;
using System.Text.Json;
Expand All @@ -27,10 +28,12 @@ internal sealed partial class ClientOAuthProvider
private readonly IDictionary<string, string> _additionalAuthorizationParameters;
private readonly Func<IReadOnlyList<Uri>, Uri?> _authServerSelector;
private readonly AuthorizationRedirectDelegate _authorizationRedirectDelegate;
private readonly DynamicClientRegistrationDelegate? _dynamicClientRegistrationDelegate;

// _clientName and _client URI is used for dynamic client registration (RFC 7591)
// _clientName, _clientUri, and _clientType is used for dynamic client registration (RFC 7591)
private readonly string? _clientName;
private readonly Uri? _clientUri;
private readonly OAuthClientType _clientType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private readonly OAuthClientType _clientType;
private readonly OAuthClientType _dcrRequestedAuthMethod;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the enum itself should be changed to TokenEndpointAuthMethod as well, because client_secret_post and none are not the only options available. Or instead of an enum it can simply be a string.


private readonly HttpClient _httpClient;
private readonly ILogger _logger;
Expand Down Expand Up @@ -69,6 +72,7 @@ public ClientOAuthProvider(
_redirectUri = options.RedirectUri ?? throw new ArgumentException("ClientOAuthOptions.RedirectUri must configured.");
_clientName = options.ClientName;
_clientUri = options.ClientUri;
_clientType = options.ClientType ?? OAuthClientType.Confidential;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be clearer if the default value for clientOAuthOptions.DynamicClientRegistration.ClientType was OAuthClientType.Confidential rather than null if that's how we always interpret it.

I'm curious why you added the option for a public client though if you're going through the effort of dynamic client registration? Why not just use a secret if the server is willing to give you one?

If you're not doing dynamic client registration, you can already specify a client ID without a client secret if you want to act as a public client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you added the option for a public client though if you're going through the effort of dynamic client registration? Why not just use a secret if the server is willing to give you one?

If you're not doing dynamic client registration, you can already specify a client ID without a client secret if you want to act as a public client.

I'm only using DCR because the MCP client I was trying to get this working with does not support specifying the client ID. If you create a new client every time it doesn't really matter if it's public or confidential, but I also included the DynamicClientRegistrationDelegate so that the client can be re-used, and in that case the choice for public or confidential does matter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not convinced that the choice of public or confidential matters unless there's some authorization server that supports DCR but doesn't support client secrets which I have not seen yet.

I can see how the DynamicClientRegistrationDelegate is useful, but if you've retained tokens from a previous session using that delegate and use those, you're no longer doing DCR for the subsequent sessions. At that point you're just a normal confidential client like any other time you manually configure ClientOAuthOptions.ClientSecret.

I think we should remove ClientType/OAuthClientType entirely, and just add InitialAccessToken and the DynamicClientRegistrationDelegate to the DynamicClientRegistrationOptions.

_scopes = options.Scopes?.ToArray();
_additionalAuthorizationParameters = options.AdditionalAuthorizationParameters;

Expand All @@ -77,6 +81,20 @@ public ClientOAuthProvider(

// Set up authorization URL handler (use default if not provided)
_authorizationRedirectDelegate = options.AuthorizationRedirectDelegate ?? DefaultAuthorizationUrlHandler;

// Set up dynamic client registration delegate
_dynamicClientRegistrationDelegate = options.DynamicClientRegistrationDelegate;

if (options.InitialAccessToken is not null)
{
_token = new()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use a new field for the dynamic client registration initial access token, so we don't conflate it with a normal access token.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I thought it would make sense but in hindsight this approach only overcomplicates things.

{
AccessToken = options.InitialAccessToken,
ExpiresIn = 900,
TokenType = BearerScheme,
ObtainedAt = DateTimeOffset.UtcNow,
};
}
}

/// <summary>
Expand Down Expand Up @@ -175,6 +193,25 @@ public async Task HandleUnauthorizedResponseAsync(
await PerformOAuthAuthorizationAsync(response, cancellationToken).ConfigureAwait(false);
}

/// <summary>
/// Adds an authorization header to the request.
/// </summary>
internal async Task AddAuthorizationHeaderAsync(HttpRequestMessage request, string scheme, CancellationToken cancellationToken)
{
if (request.RequestUri is null)
{
return;
}

var token = await GetCredentialAsync(scheme, request.RequestUri, cancellationToken).ConfigureAwait(false);
if (string.IsNullOrEmpty(token))
{
return;
}

request.Headers.Authorization = new AuthenticationHeaderValue(scheme, token);
}

/// <summary>
/// Performs OAuth authorization by selecting an appropriate authorization server and completing the OAuth flow.
/// </summary>
Expand Down Expand Up @@ -442,7 +479,7 @@ private async Task PerformDynamicClientRegistrationAsync(
RedirectUris = [_redirectUri.ToString()],
GrantTypes = ["authorization_code", "refresh_token"],
ResponseTypes = ["code"],
TokenEndpointAuthMethod = "client_secret_post",
TokenEndpointAuthMethod = _clientType == OAuthClientType.Confidential ? "client_secret_post" : "none",
ClientName = _clientName,
ClientUri = _clientUri?.ToString(),
Scope = _scopes is not null ? string.Join(" ", _scopes) : null
Expand All @@ -456,6 +493,11 @@ private async Task PerformDynamicClientRegistrationAsync(
Content = requestContent
};

if (_token is not null)
{
await AddAuthorizationHeaderAsync(request, _token.TokenType, cancellationToken).ConfigureAwait(false);
}

using var httpResponse = await _httpClient.SendAsync(request, cancellationToken).ConfigureAwait(false);

if (!httpResponse.IsSuccessStatusCode)
Expand Down Expand Up @@ -483,6 +525,11 @@ private async Task PerformDynamicClientRegistrationAsync(
}

LogDynamicClientRegistrationSuccessful(_clientId!);

if (_dynamicClientRegistrationDelegate is not null)
{
await _dynamicClientRegistrationDelegate(registrationResponse, cancellationToken).ConfigureAwait(false);
}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

namespace ModelContextProtocol.Authentication;

/// <summary>
/// Represents a method that handles the dynamic client registration response.
/// </summary>
/// <param name="response">The dynamic client registration response containing the client credentials.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>A task that represents the asynchronous operation.</returns>
/// <remarks>
/// The implementation should save the client credentials securely for future use.
/// </remarks>
public delegate Task DynamicClientRegistrationDelegate(DynamicClientRegistrationResponse response, CancellationToken cancellationToken);
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ namespace ModelContextProtocol.Authentication;
/// <summary>
/// Represents a client registration response for OAuth 2.0 Dynamic Client Registration (RFC 7591).
/// </summary>
internal sealed class DynamicClientRegistrationResponse
public sealed class DynamicClientRegistrationResponse
{
/// <summary>
/// Gets or sets the client identifier.
Expand Down
17 changes: 17 additions & 0 deletions src/ModelContextProtocol.Core/Authentication/OAuthClientType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
namespace ModelContextProtocol.Authentication;

/// <summary>
/// Represents the type of OAuth client.
/// </summary>
public enum OAuthClientType
{
/// <summary>
/// A confidential client, typically a server-side application that can securely store credentials.
/// </summary>
Confidential,

/// <summary>
/// A public client, typically a client-side application that cannot securely store credentials.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not for dynamic client registration, this would be true. But given that we do support DCR, why shouldn't we always use a confidential flow given we can just store the secret in memory? It's not like a typically client-side OAuth application without DCR that would need to ship the credential with the executable.

This is what make it weird to me why we'd even give the option to request we do no token endpoint authentication when using DCR. Why even add the option? Does Keycloak not support using client secrets for the token exchange when using DCR?

/// </summary>
Public,
}
Loading