-
Notifications
You must be signed in to change notification settings - Fork 434
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
5496590
febcbf1
5700cbc
0ec0e61
210b6f2
cf15212
b84a954
6a301fe
5816a68
d9fb5cb
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 | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
@@ -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; | ||||||
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.
Suggested change
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. Maybe the enum itself should be changed to |
||||||
|
||||||
private readonly HttpClient _httpClient; | ||||||
private readonly ILogger _logger; | ||||||
|
@@ -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; | ||||||
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. I think it would be clearer if the default value for 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. 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.
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 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. 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 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; | ||||||
|
||||||
|
@@ -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() | ||||||
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. 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. 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. 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> | ||||||
|
@@ -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> | ||||||
|
@@ -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 | ||||||
|
@@ -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) | ||||||
|
@@ -483,6 +525,11 @@ private async Task PerformDynamicClientRegistrationAsync( | |||||
} | ||||||
|
||||||
LogDynamicClientRegistrationSuccessful(_clientId!); | ||||||
|
||||||
if (_dynamicClientRegistrationDelegate is not null) | ||||||
{ | ||||||
await _dynamicClientRegistrationDelegate(registrationResponse, cancellationToken).ConfigureAwait(false); | ||||||
halter73 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
|
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 |
---|---|---|
@@ -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. | ||
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. 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, | ||
} |
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.
Could we move all the dynamic client registration related configuration to a subproperty called
DynamicClientRegistration
(the type could be callDynamicClientRegistrationOptions
). Then you'd haveclientOAuthOptions.DynamicClientRegistration.ResponseDelegate
,clientOAuthOptions.DynamicClientRegistration.ClientType
, andclientOAuthOptions.DynamicClientRegistration.InitialAccessToken
. I think it helps clarify these are only for dynamic client registration when dealing with ClientType and InitialAccessToken in particular.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.
Do you want the existing properties
ClientName
andClientUri
in this new class as well? It would be a breaking change though.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.
Yes. Good call. We're not too concerned about making these kinds of breaking changes this early.