Integrate Config based env retrieval by g2vinay · Pull Request #1978 · microsoft/mcp

g2vinay · 2026-03-10T05:18:08Z

No description provided.

…ls with IConfiguration All direct Environment.GetEnvironmentVariable calls in the Remote Azure MCP Server auth and startup paths are replaced with IConfiguration-based retrieval, since the configuration system already handles environment variables and other sources (appsettings.json, etc.). Changes: - CustomChainedCredential: add static Configuration property (mirrors the existing CloudConfiguration pattern) and GetConfigValue helper that prefers IConfiguration when set and falls back to env vars for backward compatibility. All env var reads in CreateCredential, CreateBrowserCredential, CreateDefaultCredential, AddManagedIdentityCredential, and ShouldUseOnlyBrokerCredential now use GetConfigValue. - AuthenticationServiceCollectionExtensions: wire the IConfiguration singleton (if registered) into CustomChainedCredential.Configuration when building the single-identity token credential provider. - ServiceStartCommand (HTTP paths): use builder/app IConfiguration instead of Environment.GetEnvironmentVariable for ASPNETCORE_URLS, ALLOW_INSECURE_EXTERNAL_BINDING, AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERS, AZURE_MCP_DANGEROUSLY_DISABLE_HTTPS_REDIRECTION, AZURE_MCP_COLLECT_TELEMETRY, and APPLICATIONINSIGHTS_CONNECTION_STRING. AddIncomingAndOutgoingHttpSpans now receives an IConfiguration instance built from env vars before the host is constructed.

…d-env-retrieval

MontaEllis8

✅ Code Review 通过

代码审查通过，建议合并。

Copilot

Pull request overview

Integrates IConfiguration-based settings retrieval into authentication credential selection and server startup, reducing direct Environment.GetEnvironmentVariable usage and centralizing settings access through configuration.

Changes:

Add IConfiguration support to CustomChainedCredential and prefer configuration values when available.
Wire IConfiguration into CustomChainedCredential during DI registration.
Switch several server startup settings (telemetry, URLs, forwarded headers, HTTPS redirect opt-out) from direct env-var reads to IConfiguration.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
core/Microsoft.Mcp.Core/src/Services/Azure/Authentication/CustomChainedCredential.cs	Adds configuration-backed value lookup for credential chain behavior.
core/Microsoft.Mcp.Core/src/Services/Azure/Authentication/AuthenticationServiceCollectionExtensions.cs	Passes `IConfiguration` from DI into `CustomChainedCredential` static state.
core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ServiceStartCommand.cs	Uses `IConfiguration` for multiple server startup toggles and telemetry settings.

core/Microsoft.Mcp.Core/src/Services/Azure/Authentication/CustomChainedCredential.cs

core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ServiceStartCommand.cs

…d-env-retrieval

anuchandy

Thanks Vinay! @g2vinay, these changes route env var reads through IConfiguration, but most of the values here are env var conventions by design - so wondering if this routing is necessary. Since it shifts expectations around how these are configured, it might be worth holding off on including this in GA.

anuchandy · 2026-03-16T20:25:23Z

core/Microsoft.Mcp.Core/src/Services/Azure/Authentication/CustomChainedCredential.cs

+    /// 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; }


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

"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.

"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.

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.

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.

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.

I see, then perhaps we can also honor the double-underscore format as best practice.

anuchandy · 2026-03-16T20:48:40Z

core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ServiceStartCommand.cs

-                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"];


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

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

anuchandy · 2026-03-16T21:07:16Z

core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ServiceStartCommand.cs

+        }

-        string? connectionString = Environment.GetEnvironmentVariable("APPLICATIONINSIGHTS_CONNECTION_STRING");
+        string? connectionString = configuration["APPLICATIONINSIGHTS_CONNECTION_STRING"];


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

alzimmermsft · 2026-03-16T21:50:53Z

core/Microsoft.Mcp.Core/src/Services/Azure/Authentication/CustomChainedCredential.cs

+    private static string? GetConfigValue(string key) =>
+        Configuration?[key] ?? Environment.GetEnvironmentVariable(key);


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];

g2vinay added 2 commits March 9, 2026 21:55

Merge remote-tracking branch 'upstream/main' into feature/config-base…

58e138b

…d-env-retrieval

github-project-automation bot added this to Azure MCP Server Mar 10, 2026

github-project-automation bot moved this to Untriaged in Azure MCP Server Mar 10, 2026

MontaEllis8 approved these changes Mar 10, 2026

View reviewed changes

g2vinay marked this pull request as ready for review March 16, 2026 10:01

g2vinay requested review from a team as code owners March 16, 2026 10:01

g2vinay requested review from JonathanCrd, alzimmermsft, Copilot, hallipr, msalaman, saikoumudi, vukelich and xiangyan99 March 16, 2026 10:01

Copilot started reviewing on behalf of g2vinay March 16, 2026 10:02 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

g2vinay added 4 commits March 16, 2026 10:07

Address feedback

4d342f6

add creential unit tests.

62fda13

Merge remote-tracking branch 'upstream/main' into feature/config-base…

8e7a3c9

…d-env-retrieval

fix format issues.

09d15b6

anuchandy reviewed Mar 16, 2026

View reviewed changes

alzimmermsft reviewed Mar 16, 2026

View reviewed changes

anuchandy added the Do Not Merge Do Not Merge / WIP PRs label Mar 17, 2026

joshfree added this to the 2026-04 milestone Mar 17, 2026

joshfree moved this from Untriaged to In Progress in Azure MCP Server Mar 17, 2026

joshfree assigned g2vinay Mar 17, 2026

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

Conversation

g2vinay commented Mar 10, 2026

Uh oh!

MontaEllis8 left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anuchandy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g2vinay Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

g2vinay Mar 16, 2026 •

edited

Loading