Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Reflection;
using Azure.Core;
using Azure.Identity;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Logging;
using Xunit;

Expand Down Expand Up @@ -400,6 +401,66 @@ public void PinnedCredentialMode_DoesNotAddDeviceCodeFallback_CreatesCredentialS
Assert.IsAssignableFrom<TokenCredential>(credential);
}

/// <summary>
/// Tests that when Configuration is set, token credentials are read from IConfiguration
/// rather than only from environment variables.
/// </summary>
[Fact]
public void Configuration_WhenSet_ReadsTokenCredentialsFromConfiguration()
{
// Arrange: env var cleared; IConfiguration provides the credential type
using var env = new EnvironmentScope("AZURE_TOKEN_CREDENTIALS");
Environment.SetEnvironmentVariable("AZURE_TOKEN_CREDENTIALS", null);

var credentialType = GetCustomChainedCredentialType();
var configProp = credentialType.GetProperty("Configuration",
BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public);
Assert.NotNull(configProp);

var config = new ConfigurationBuilder()
.AddInMemoryCollection(new Dictionary<string, string?> { ["AZURE_TOKEN_CREDENTIALS"] = "AzureCliCredential" })
.Build();
configProp.SetValue(null, config);

try
{
// Act
var credential = CreateCustomChainedCredential();

// Assert
Assert.NotNull(credential);
Assert.IsAssignableFrom<TokenCredential>(credential);
}
finally
{
configProp.SetValue(null, null);
}
}

/// <summary>
/// Tests that when Configuration is null, token credentials fall back to environment variables.
/// </summary>
[Fact]
public void Configuration_WhenNull_FallsBackToEnvironmentVariable()
{
// Arrange: Configuration is null; env var provides the credential type
using var env = new EnvironmentScope("AZURE_TOKEN_CREDENTIALS");
Environment.SetEnvironmentVariable("AZURE_TOKEN_CREDENTIALS", "AzureCliCredential");

var credentialType = GetCustomChainedCredentialType();
var configProp = credentialType.GetProperty("Configuration",
BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public);
Assert.NotNull(configProp);
configProp.SetValue(null, null); // Ensure Configuration is null

// Act
var credential = CreateCustomChainedCredential();

// Assert
Assert.NotNull(credential);
Assert.IsAssignableFrom<TokenCredential>(credential);
}

/// <summary>
/// Tests that prod mode with forceBrowserFallback=true does NOT add a browser fallback.
/// prod always signals a non-interactive environment — the browser popup must never appear.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,21 @@ public override async Task<CommandResponse> ExecuteAsync(CommandContext context,

try
{
using var tracerProvider = AddIncomingAndOutgoingHttpSpans(options);
// Build configuration using the same sources the host will use so AddIncomingAndOutgoingHttpSpans
// has access to IConfiguration before the host DI container is constructed.
// This mirrors Host.CreateDefaultBuilder / WebApplication.CreateBuilder source order:
// appsettings.json → appsettings.{env}.json → environment variables.
string hostEnvironment = Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")
?? Environment.GetEnvironmentVariable("DOTNET_ENVIRONMENT")
?? "Production";
IConfiguration preHostConfiguration = new ConfigurationBuilder()
.SetBasePath(AppContext.BaseDirectory)
.AddJsonFile("appsettings.json", optional: true, reloadOnChange: false)
.AddJsonFile($"appsettings.{hostEnvironment}.json", optional: true, reloadOnChange: false)
.AddEnvironmentVariables()
.Build();

using var tracerProvider = AddIncomingAndOutgoingHttpSpans(options, preHostConfiguration);

using var host = CreateHost(options);

Expand Down Expand Up @@ -462,14 +476,11 @@ private IHost CreateHttpHost(ServiceStartOptions serverOptions)
{
WebApplicationBuilder builder = WebApplication.CreateBuilder(HttpWebApplicationOptions);

// Read once at host setup time — this env var is process-wide and effectively static,
// Read once at host setup time — this value is process-wide and effectively static,
// so there is no need to re-read it on every incoming request.
// Default to false; the env var must be present and parse to "true" to enable.
bool enableForwardedHeaders =
bool.TryParse(
Environment.GetEnvironmentVariable("AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERS"),
out bool parsedEnvVar)
&& parsedEnvVar;
// Default to false; the configuration value must parse to "true" to enable.
string? forwardedHeadersSetting = builder.Configuration["AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERS"];
Copy link
Member

Choose a reason for hiding this comment

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

So we're now reading ALLOW_INSECURE_EXTERNAL_BINDING, AZURE_MCP_DANGEROUSLY_DISABLE_HTTPS_REDIRECTION, and AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERS via WebApplicationBuilder - meaning they can now be set via "appsettings.json". These are security-sensitive opt-ins, so this is worth scrutinizing: a checked-in config file could accidentally enable insecure settings, whereas env vars require explicit runtime intent. Would be good to get a read from the security folks before we settle on this.

Tagging Xiang, Srikanta - \cc @xiangyan99 , @srnagar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can opt to selectively restrict it to env here by design.

bool enableForwardedHeaders = bool.TryParse(forwardedHeadersSetting, out bool parsedForwardedHeaders) && parsedForwardedHeaders;

// Configure logging
builder.Logging.ClearProviders();
Expand Down Expand Up @@ -555,7 +566,6 @@ private IHost CreateHttpHost(ServiceStartOptions serverOptions)
// Add a multi-user, HTTP context-aware caching strategy to isolate cache entries.
services.AddHttpServiceCacheService();


// Configure non-MCP controllers/endpoints/routes/etc.
services.AddHealthChecks();

Expand Down Expand Up @@ -820,7 +830,7 @@ private static void InitializeListingUrls(WebApplicationBuilder builder, Service
return;
}

string url = Environment.GetEnvironmentVariable("ASPNETCORE_URLS") ?? "http://127.0.0.1:5001";
string url = builder.Configuration["ASPNETCORE_URLS"] ?? "http://127.0.0.1:5001";

if (url.Contains(';'))
{
Expand Down Expand Up @@ -849,7 +859,13 @@ private static void InitializeListingUrls(WebApplicationBuilder builder, Service
throw new InvalidOperationException($"Explicit external binding is not supported for '{url}'.");
}

if (isWildcard && !EnvironmentHelpers.GetEnvironmentVariableAsBool("ALLOW_INSECURE_EXTERNAL_BINDING"))
var allowInsecureExternalBindingRaw = builder.Configuration["ALLOW_INSECURE_EXTERNAL_BINDING"];
bool allowInsecureExternalBinding = false;
if (!string.IsNullOrWhiteSpace(allowInsecureExternalBindingRaw))
{
_ = bool.TryParse(allowInsecureExternalBindingRaw, out allowInsecureExternalBinding);
}
if (isWildcard && !allowInsecureExternalBinding)
{
throw new InvalidOperationException(
$"External binding blocked for '{url}'. " +
Expand Down Expand Up @@ -917,8 +933,9 @@ private static WebApplication UseHttpsRedirectionIfEnabled(WebApplication app)
// - The application or server's HTTP stack is not listening for non-HTTPS requests.
//
// Safe default to enable HTTPS redirection unless explicitly opted-out.
string? httpsRedirectionOptOut = Environment.GetEnvironmentVariable("AZURE_MCP_DANGEROUSLY_DISABLE_HTTPS_REDIRECTION");
if (!bool.TryParse(httpsRedirectionOptOut, out bool isOptedOut) || !isOptedOut)
var disableHttpsRedirectionSetting = app.Configuration["AZURE_MCP_DANGEROUSLY_DISABLE_HTTPS_REDIRECTION"];
var httpsRedirectionDisabled = bool.TryParse(disableHttpsRedirectionSetting, out var parsedValue) && parsedValue;
if (!httpsRedirectionDisabled)
{
app.UseHttpsRedirection();
}
Expand All @@ -930,6 +947,7 @@ private static WebApplication UseHttpsRedirectionIfEnabled(WebApplication app)
/// Configures incoming and outgoing HTTP spans for self-hosted HTTP mode with Azure Monitor exporter.
/// </summary>
/// <param name="options">The server configuration options.</param>
/// <param name="configuration">The configuration used to read telemetry settings.</param>
/// <returns>
/// A <see cref="TracerProvider"/> instance if telemetry is enabled and properly configured for HTTP transport;
/// otherwise, <c>null</c>.
Expand All @@ -939,17 +957,17 @@ private static WebApplication UseHttpsRedirectionIfEnabled(WebApplication app)
/// <list type="bullet">
/// <item><description>The transport is HTTP (not STDIO)</description></item>
/// <item><description>AZURE_MCP_COLLECT_TELEMETRY is not explicitly set to false</description></item>
/// <item><description>APPLICATIONINSIGHTS_CONNECTION_STRING environment variable is set</description></item>
/// <item><description>APPLICATIONINSIGHTS_CONNECTION_STRING is set</description></item>
/// </list>
/// The tracer provider includes ASP.NET Core and HttpClient instrumentation with filtering
/// to avoid duplicate spans and telemetry loops.
/// This telemetry configuration is intended for self-hosted scenarios where
/// the MCP server is running in HTTP mode. This creates an independent telemetry pipeline using TracerProvider to export
/// traces to user-configured Application Insights instance only when the necessary environment variables are set. This also honors
/// the AZURE_MCP_COLLECT_TELEMETRY environment variable to allow users to disable telemetry collection if desired. Note that this is
/// traces to user-configured Application Insights instance only when the necessary configuration values are set. This also honors
/// the AZURE_MCP_COLLECT_TELEMETRY setting to allow users to disable telemetry collection if desired. Note that this is
/// in addition to the telemetry configured in <see cref="OpenTelemetryExtensions"/>.
/// </remarks>
private static TracerProvider? AddIncomingAndOutgoingHttpSpans(ServiceStartOptions options)
private static TracerProvider? AddIncomingAndOutgoingHttpSpans(ServiceStartOptions options, IConfiguration configuration)
{
if (options.Transport != TransportTypes.Http)
{
Expand All @@ -964,11 +982,20 @@ private static WebApplication UseHttpsRedirectionIfEnabled(WebApplication app)
return null;
}

string? collectTelemetry = Environment.GetEnvironmentVariable("AZURE_MCP_COLLECT_TELEMETRY");
bool isTelemetryEnabled = string.IsNullOrWhiteSpace(collectTelemetry) ||
(bool.TryParse(collectTelemetry, out bool shouldCollectTelemetry) && shouldCollectTelemetry);
string? telemetrySetting = configuration["AZURE_MCP_COLLECT_TELEMETRY"];
bool isTelemetryEnabled;
if (string.IsNullOrWhiteSpace(telemetrySetting))
{
// Preserve default behavior when the setting is not provided.
isTelemetryEnabled = true;
}
else if (!bool.TryParse(telemetrySetting, out isTelemetryEnabled))
{
// Treat invalid/unknown values as telemetry disabled to avoid startup failures.
isTelemetryEnabled = false;
}

string? connectionString = Environment.GetEnvironmentVariable("APPLICATIONINSIGHTS_CONNECTION_STRING");
string? connectionString = configuration["APPLICATIONINSIGHTS_CONNECTION_STRING"];
Copy link
Member

Choose a reason for hiding this comment

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

Routing through IConfiguration appears to unlock setting APPLICATIONINSIGHTS_CONNECTION_STRING via appsettings.json, but this flat key is an env var convention.

if (!isTelemetryEnabled || string.IsNullOrWhiteSpace(connectionString))
{
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using Microsoft.AspNetCore.Authentication;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -41,7 +42,9 @@ public static IServiceCollection AddSingleIdentityTokenCredentialProvider(this I
services.TryAddSingleton<IAzureTokenCredentialProvider>(sp =>
{
var cloudConfig = sp.GetRequiredService<IAzureCloudConfiguration>();
var configuration = sp.GetService<IConfiguration>();
CustomChainedCredential.CloudConfiguration = cloudConfig;
CustomChainedCredential.Configuration = configuration;
return new SingleIdentityTokenCredentialProvider(sp.GetRequiredService<ILoggerFactory>());
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
using Azure.Core;
using Azure.Identity;
using Azure.Identity.Broker;
using Azure.Mcp.Core.Helpers;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Logging;

namespace Azure.Mcp.Core.Services.Azure.Authentication;
Expand Down Expand Up @@ -94,6 +94,20 @@ internal class CustomChainedCredential(string? tenantId = null, ILogger<CustomCh
/// </summary>
internal static IAzureCloudConfiguration? CloudConfiguration { get; set; }

/// <summary>
/// Configuration for reading settings. Set by DI container during service registration.
/// When set, configuration-based retrieval is preferred over direct environment variable access,
/// allowing values to come from non-environment sources (e.g. appsettings.json) in addition to environment variables.
/// </summary>
internal static IConfiguration? Configuration { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if IConfiguration indirection layer we've here earn its keep? -

  1. "AZURE_CLIENT_ID" is a standard Azure Identity convention. The EnvironmentCredential in the same chain reads it directly from env via EnvironmentCredentialOptions - routing our read through IConfiguration while the azure-identity's own read bypasses it creates inconsistency within the same credential chain.
  2. "VSCODE_PID" is process introspection set by VS Code it will never appear in appsettings.json.

These values are lazily initialized and cached, so env reads happen once per instance - IConfiguration indirection doesn't improve caching or correctness.

My two cents, I'd lean toward keeping Environment.GetEnvironmentVariable as-is. If there's a specific scenario where appsettings.json sourcing is needed for certain values later, we could scope the change to just that variable rather than adding a blanket IConfiguration layer.

Copy link
Contributor Author

@g2vinay g2vinay Mar 16, 2026

Choose a reason for hiding this comment

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

I assume, @srnagar's direction was to make it similar to how we support Configuration in Azure Core in Java Azure SDKs.
Read from both code level config, properties, env var etc.
If that's still the goal, then the code level can override the env var, which is fine, as it won't regress users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add this selectively too, by design we can exclude certain areas where we want to restrict to env var only.
Will bring downside of incosnsitency, but if its justified by design, its fine, for Credential purposes, I don't see strong reason to restrict it, its fine to support config there too.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was trying to understand the general pattern libs use.

Taking the telemetry SDK as an example: APPLICATIONINSIGHTS_CONNECTION_STRING (ALL_CAPS) is a flat "env var" read directly via Environment.GetEnvironmentVariable, not through IConfiguration.

The appsettings.json equivalent is the hierarchical ApplicationInsights:ConnectionString section, which maps to ApplicationInsights__ConnectionString (double underscore) "env-var" for IConfiguration resolution.

So my main point was that reading ALL_CAPS "env vars" through IConfiguration is uncommon - users understand IConfiguration work with the double-underscore "env var" form of a hierarchical section. That said, I don't see a concrete issue with us supporting ALL_CAPS via appsettings.json - yet.

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 see, then perhaps we can also honor the double-underscore format as best practice.


/// <summary>
/// Reads a configuration value, preferring <see cref="Configuration"/> when available
/// and falling back to <see cref="Environment.GetEnvironmentVariable"/> otherwise.
/// </summary>
private static string? GetConfigValue(string key) =>
Configuration?[key] ?? Environment.GetEnvironmentVariable(key);
Comment on lines +108 to +109
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make this check a bit more verbose as there's a different between Configuration is null and fallback to Environment.GetEnvironmentVariable and Configuration returned null from the lookup, as Configuration will fallback to looking at Environment.GetEnvironmentVariable.

Suggested change
private static string? GetConfigValue(string key) =>
Configuration?[key] ?? Environment.GetEnvironmentVariable(key);
private static string? GetConfigValue(string key) =>
(Configuration == null)
? Environment.GetEnvironmentVariable(key)
: Configuration[key];


/// <summary>
/// Active transport type ("stdio" or "http"). Set by <see cref="Microsoft.Mcp.Core.Areas.Server.Commands.ServiceStartCommand"/>
/// before the credential chain is first used. Empty when not running as a server (e.g. direct CLI invocation).
Expand All @@ -120,13 +134,14 @@ public override ValueTask<AccessToken> GetTokenAsync(TokenRequestContext request

private static bool ShouldUseOnlyBrokerCredential()
{
return EnvironmentHelpers.GetEnvironmentVariableAsBool(OnlyUseBrokerCredentialEnvVarName);
string? value = GetConfigValue(OnlyUseBrokerCredentialEnvVarName);
return value is "true" or "True" or "T" or "1";
}

private static TokenCredential CreateCredential(string? tenantId, ILogger<CustomChainedCredential>? logger = null, bool forceBrowserFallback = false)
{
// Check if AZURE_TOKEN_CREDENTIALS is explicitly set
string? tokenCredentials = Environment.GetEnvironmentVariable(TokenCredentialsEnvVarName);
string? tokenCredentials = GetConfigValue(TokenCredentialsEnvVarName);
bool hasExplicitCredentialSetting = !string.IsNullOrEmpty(tokenCredentials);

#if DEBUG
Expand All @@ -139,7 +154,7 @@ private static TokenCredential CreateCredential(string? tenantId, ILogger<Custom
}
#endif

string? authRecordJson = Environment.GetEnvironmentVariable(AuthenticationRecordEnvVarName);
string? authRecordJson = GetConfigValue(AuthenticationRecordEnvVarName);
AuthenticationRecord? authRecord = null;
if (!string.IsNullOrEmpty(authRecordJson))
{
Expand All @@ -156,7 +171,7 @@ private static TokenCredential CreateCredential(string? tenantId, ILogger<Custom
var creds = new List<TokenCredential>();

// Check if we are running in a VS Code context. VSCODE_PID is set by VS Code when launching processes, and is a reliable indicator for VS Code-hosted processes.
bool isVsCodeContext = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("VSCODE_PID"));
bool isVsCodeContext = !string.IsNullOrEmpty(GetConfigValue("VSCODE_PID"));

if (isVsCodeContext && !hasExplicitCredentialSetting)
{
Expand Down Expand Up @@ -220,7 +235,7 @@ private static TokenCredential CreateCredential(string? tenantId, ILogger<Custom

private static TokenCredential CreateBrowserCredential(string? tenantId, AuthenticationRecord? authRecord)
{
string? clientId = Environment.GetEnvironmentVariable(ClientIdEnvVarName);
string? clientId = GetConfigValue(ClientIdEnvVarName);

IntPtr handle = WindowHandleProvider.GetWindowHandle();

Expand All @@ -247,8 +262,8 @@ private static TokenCredential CreateBrowserCredential(string? tenantId, Authent

var browserCredential = new InteractiveBrowserCredential(brokerOptions);

// Check for timeout value in the environment variable
string? timeoutValue = Environment.GetEnvironmentVariable(BrowserAuthenticationTimeoutEnvVarName);
// Check for timeout value in the configuration
string? timeoutValue = GetConfigValue(BrowserAuthenticationTimeoutEnvVarName);
int timeoutSeconds = 300; // Default to 300 seconds (5 minutes)
if (!string.IsNullOrEmpty(timeoutValue) && int.TryParse(timeoutValue, out int parsedTimeout) && parsedTimeout > 0)
{
Expand All @@ -259,7 +274,7 @@ private static TokenCredential CreateBrowserCredential(string? tenantId, Authent

private static ChainedTokenCredential CreateDefaultCredential(string? tenantId)
{
string? tokenCredentials = Environment.GetEnvironmentVariable(TokenCredentialsEnvVarName);
string? tokenCredentials = GetConfigValue(TokenCredentialsEnvVarName);
var credentials = new List<TokenCredential>();

// Handle specific credential targeting
Expand Down Expand Up @@ -367,7 +382,7 @@ private static void AddWorkloadIdentityCredential(List<TokenCredential> credenti
private static void AddManagedIdentityCredential(List<TokenCredential> credentials)
{
// Check if AZURE_CLIENT_ID is set for User-Assigned Managed Identity
string? clientId = Environment.GetEnvironmentVariable("AZURE_CLIENT_ID");
string? clientId = GetConfigValue("AZURE_CLIENT_ID");

ManagedIdentityCredential managedIdentityCredential;
if (!string.IsNullOrEmpty(clientId))
Expand Down Expand Up @@ -474,7 +489,7 @@ private static void AddDeviceCodeCredential(List<TokenCredential> credentials, s
"DeviceCodeCredential requires an interactive terminal to display the device code prompt.");
}

string? clientId = Environment.GetEnvironmentVariable(ClientIdEnvVarName);
string? clientId = GetConfigValue(ClientIdEnvVarName);

var deviceCodeOptions = new DeviceCodeCredentialOptions
{
Expand All @@ -492,8 +507,8 @@ private static void AddDeviceCodeCredential(List<TokenCredential> credentials, s
deviceCodeOptions.AuthorityHost = CloudConfiguration.AuthorityHost;
}

// Hydrate an existing AuthenticationRecord from the environment to enable silent token cache reuse
string? authRecordJson = Environment.GetEnvironmentVariable(AuthenticationRecordEnvVarName);
// Hydrate an existing AuthenticationRecord from the configuration to enable silent token cache reuse
string? authRecordJson = GetConfigValue(AuthenticationRecordEnvVarName);
if (!string.IsNullOrEmpty(authRecordJson))
{
byte[] bytes = Encoding.UTF8.GetBytes(authRecordJson);
Expand Down