Add remote HTTP server mode with OAuth authentication and downstream token acquisition. #910
Conversation
vukelich
requested review from
a team,
anuchandy,
conniey and
srnagar
as code owners
vukelich
requested review from
JonathanCrd,
jongio,
meirloichter,
wbreza and
xiangyan99
as code owners
vukelich
force-pushed
the
dev/svukel/tokenprovider
branch
from
381f0c5 to
407bec4
Compare
…token acquisition - Add RunAsRemoteHttpService and OutgoingAuthStrategy options to enable MCP server to run as a remote HTTP service - Introduce ITokenProvider abstraction for dependency injection of downstream authentication tokens and credentials - Add support for On-Behalf-Of (OBO) token flow and hosting environment identity modes for outgoing authentication - Implement OAuth Protected Resource Metadata endpoint at /.well-known/oauth-protected-resource with WWW-Authenticate challenge - Add Visual Studio launch profile for debugging remote MCP server with Microsoft.Identity.Web configuration - Modernize HTTP host creation using WebApplicationBuilder with authentication and authorization middleware
vukelich
force-pushed
the
dev/svukel/tokenprovider
branch
from
407bec4 to
9ecbc9b
Compare
|
|
||
| services.AddAuthorizationBuilder() | ||
| .SetFallbackPolicy(null) | ||
| .AddPolicy("McpAccess", policy => |
There was a problem hiding this comment.
Note - for Postgres and Speech, we’re currently using the McpToolExecutor policy with the role value Mcp.Tool.Executor
builder.Services.AddAuthorization(options =>
{
options.AddPolicy("McpToolExecutor", p => p.RequireRole("Mcp.Tool.Executor"));
...
});There was a problem hiding this comment.
Must we use McpToolExecutor and Mcp.Tool.Executor?
There was a problem hiding this comment.
No, we just need to agree on what goes in and make sure the Postgres demo code uses the same role. We should also document the same.
There was a problem hiding this comment.
Agreed. I knew Microsoft services, like MS Graph, have some established patterns, so I asked ChatGPT to come up with some thoughts: https://chatgpt.com/share/68f91558-b140-8013-a26e-5ef666821c56
TL;DR:
- Dot notation good as something like
Noun.VerborNoun.All. - Delegated permissions and app roles (aka app permissions) should have similar naming. The app roles typically have a similar set as the delegated permissions but with
.Allsuffixed.
I like the idea a middle ground of Mcp.Tools.ReadWrite for now. Mcp as a pretty meaningful root for all MCP goodness, Tools as child representing tool calling, and Verb suffix representing running tools that can read or read/write. Another child of Mcp at some point might be Agent if MCP introduces an agentic concept so we'd have Mcp.Agent.Verb. This incorporates our joint Mcp prefixing, your Tool hierarchy substring, and MS Graph's pattern and my usage of a Verb suffix.
How's that sound to you?
There was a problem hiding this comment.
Thanks for digging into this further and sharing the discussion link! This sounds good to me - I like the Mcp.Tool.{Verb} pattern. Agree that using verbs like Read or ReadWrite makes the intent much clearer (than a generic "execute") and, the pattern looks extensible, and keep things consistent, and easy to reason about.
There was a problem hiding this comment.
Updated locally. Pending push.
| var outgoingAuthStrategy = parseResult.GetValueOrDefault<OutgoingAuthenticationTypes>(ServiceOptionDefinitions.OutgoingAuthStrategy.Name); | ||
|
|
||
| // Apply safe defaults for outgoing authentication strategy based on hosting mode | ||
| if (outgoingAuthStrategy == OutgoingAuthenticationTypes.NotSet) |
There was a problem hiding this comment.
Should we align option name and its enum name - OutgoingAuthStrategy option and OutgoingAuthStratigies enum
| string? url = GetSafeAspNetCoreUrl(serverOptions); | ||
| if (url != null) | ||
| { | ||
| builder.WebHost.UseUrls(url); |
There was a problem hiding this comment.
Just wondering if it might be a bit more readable if we write it as:
if (serverOptions.EnableInsecureTransports)
{
// For 'EnableInsecureTransports' validate the url, when running as a remote
// HTTP service ('RunAsRemoteHttpService'), let the typical IConfiguration
// binding handle the ASPNETCORE_URLS setup.
//
string url = GetSafeAspNetCoreUrl();
builder.WebHost.UseUrls(url);
}Basically, if InsecureTransports then SafeAspNetCoreUrl go hand in hand, so grouping them together makes it a bit more readable, I feel.
There was a problem hiding this comment.
I think I'm going to update GetSafeAspNetCoreUrl to take in the builder so that it can encapsulate both possible paths then return void.
There was a problem hiding this comment.
Updated locally. Pending push.
| // When running as a remote HTTP service, let the typical IConfiguration | ||
| // binding handle the ASPNETCORE_URLS value without additional validation. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
If we make this change (link), we can keep this method as is and return string instead of string?.
There was a problem hiding this comment.
I think I'm going to update GetSafeAspNetCoreUrl to take in the builder so that it can encapsulate both possible paths then return void.
There was a problem hiding this comment.
Updated locally. Pending push.
...zure.Mcp.Core/src/Services/Azure/Authentication/AuthenticationServiceCollectionExtensions.cs
Show resolved
Hide resolved
core/Azure.Mcp.Core/src/Services/Azure/Authentication/HttpOnBehalfOfTokenCredentialProvider.cs
Show resolved
Hide resolved
| if (_httpContextAccessor.HttpContext is not HttpContext httpContext) | ||
| { | ||
| throw new InvalidOperationException("There is no ongoing HTTP request."); | ||
| } |
There was a problem hiding this comment.
Should we add a preemptive check to be more defensive?
if (httpContext.User.Identity?.IsAuthenticated != true)
{
throw new InvalidOperationException("User is not authenticated.");
}
...Mcp.Tools.Marketplace/tests/Azure.Mcp.Tools.Marketplace.LiveTests/ProductListCommandTests.cs
Show resolved
Hide resolved
| <PackageVersion Include="Microsoft.Extensions.Configuration.EnvironmentVariables" Version="9.0.9" /> | ||
| <PackageVersion Include="Microsoft.Extensions.Configuration.Json" Version="9.0.9" /> | ||
| <PackageVersion Include="Microsoft.Identity.Abstractions" Version="9.5.0" /> | ||
| <PackageVersion Include="Microsoft.Identity.Web" Version="3.14.0" /> |
There was a problem hiding this comment.
nit - Identity team suggested (in the AOT internal channel) today that we should use 4.0.0.
core/Azure.Mcp.Core/src/Areas/Server/Commands/ServiceCollectionExtensions.cs
Show resolved
Hide resolved
|
|
||
| // MicrosoftIdentityTokenCredential is registered as scoped, so we | ||
| // can get it from the request services to ensure we get the right instance. | ||
| MicrosoftIdentityTokenCredential tokenCredential = httpContext |
There was a problem hiding this comment.
Curious - any reason for us to use MicrosoftIdentityTokenCredential instead of ITokenAcquisition? MicrosoftIdentityTokenCredential adds a new dependency (Microsoft.Identity.Web.Azure), so just wondering if it provides enough value over ITokenAcquisition.GetAccessTokenForUserAsync(scopes). While exploring, I used ITokenAcquisition (example: JwtOboCredentialProvider.cs#L160) and wrapped it in a TokenCredential, so curious o learn.
joshfree
added
server-Azure.Mcp
|
adding |
| tenantId, | ||
| _loggerFactory.CreateLogger<CustomChainedCredential>() | ||
| ); | ||
| _tenantSpecificCredentials[tenantId] = tenantCredential; |
There was a problem hiding this comment.
If I understand correctly, when the strategy is UseHostingEnvironmentIdentity and the server runs with --run-as-remote-http-service or --enable-insecure-transports, multiple HTTP requests may execute concurrently and access the singleton SingleIdentityTokenCredentialProvider, which maintains the _tenantSpecificCredentials dictionary. Should we make this dictionary thread-safe to avoid potential races (e.g., that could corrupt its internal state during resize or rehash operations)?
private readonly ConcurrentDictionary<string, TokenCredential> _tenantCredentials = new();
...
var tenantCredential = _tenantCredentials.GetOrAdd(
tenantId,
id => new CustomChainedCredential(id, _loggerFactory.CreateLogger<CustomChainedCredential>())
);
return Task.FromResult(tenantCredential);| /// </exception> | ||
| Task<TokenCredential> GetTokenAsync( | ||
| string? tenantId, | ||
| CancellationToken cancellationToken); |
There was a problem hiding this comment.
I really like the idea of treating ITenantService as the provider of credentials. (so IAzureTokenCredentialProvider injected only in one place)
| /// <inheritdoc/> | ||
| public async Task<TokenCredential> GetTokenAsync(string? tenantId, CancellationToken cancellationToken) | ||
| { | ||
| return await _credentialProvider.GetTokenAsync(tenantId, cancellationToken); |
There was a problem hiding this comment.
Nit: one minor thing I got confused about - the method name GetTokenAsync initially made me think it returns an Azure.Core.AccessToken, but it actually returns a Azure.Core.TokenCredential. Since Azure.Core.TokenCredential itself has a GetTokenAsync method, should we consider renaming this to ITenantService::GetTokenCredentialAsync and IAzureTokenCredentialProvider::GetTokenCredentialAsync for better clarity?
|
|
||
| public class TenantService(ICacheService cacheService) | ||
| : BaseAzureService, ITenantService | ||
| public class TenantService: BaseAzureService, ITenantService |
There was a problem hiding this comment.
With TenantService now responsible for providing credentials, I’m wondering if there’s still a need for it to inherit from BaseAzureService?, if not it will let us avoid exposing the protected TenantService property in BaseAzureService, which is a workaround to break the circular dependency, thinking loud
| public static IServiceCollection AddAzureTenantService(this IServiceCollection services) | ||
| { | ||
| // !!! HACK !!! | ||
| // Program.cs for the CLI servers have their own DI containers vs ServiceStartCommand. |
There was a problem hiding this comment.
Completely out of scope for this work, but for future GA releases, we might want to revisit whether we still need to support the fully functional CLI mode (beyond server start and --version). I recall GHCP4z used to rely on the CLI mode at one point, but that’s no longer the case. Feel free to resolve the comment - I just wanted to highlight the additional overhead of maintaining a mode (CLI) whose value for MCP server users is unclear.
4 participants
What does this PR do?
[Provide a clear, concise description of the changes]Add remote HTTP server mode with OAuth authentication and downstream token acquisition.
[Any additional context, screenshots, or information that helps reviewers]This is for our goals of allowing Microsoft customers self-hosting remote MCP servers.
GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.mdToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test prompts/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline