-
Notifications
You must be signed in to change notification settings - Fork 425
Integrate Config based env retrieval #1978
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 all commits
db749c5
58e138b
4d342f6
62fda13
8e7a3c9
09d15b6
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 |
|---|---|---|
|
|
@@ -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); | ||
|
|
||
|
|
@@ -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"]; | ||
| bool enableForwardedHeaders = bool.TryParse(forwardedHeadersSetting, out bool parsedForwardedHeaders) && parsedForwardedHeaders; | ||
|
|
||
| // Configure logging | ||
| builder.Logging.ClearProviders(); | ||
|
|
@@ -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(); | ||
|
|
||
|
|
@@ -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(';')) | ||
| { | ||
|
|
@@ -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}'. " + | ||
|
|
@@ -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(); | ||
| } | ||
|
|
@@ -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>. | ||
|
|
@@ -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) | ||
| { | ||
|
|
@@ -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"]; | ||
|
Member
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. Routing through |
||
| if (!isTelemetryEnabled || string.IsNullOrWhiteSpace(connectionString)) | ||
| { | ||
| return null; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||||||||||
|
|
@@ -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; } | ||||||||||||||
|
Member
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 wonder if
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
Contributor
Author
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 assume, @srnagar's direction was to make it similar to how we support Configuration in Azure Core in Java Azure SDKs.
Contributor
Author
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. We can add this selectively too, by design we can exclude certain areas where we want to restrict to env var only.
Member
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. Yeah, I was trying to understand the general pattern libs use. Taking the telemetry SDK as an example: The appsettings.json equivalent is the hierarchical 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.
Contributor
Author
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 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
Contributor
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'd make this check a bit more verbose as there's a different between
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| /// <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). | ||||||||||||||
|
|
@@ -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 | ||||||||||||||
|
|
@@ -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)) | ||||||||||||||
| { | ||||||||||||||
|
|
@@ -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) | ||||||||||||||
| { | ||||||||||||||
|
|
@@ -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(); | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -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) | ||||||||||||||
| { | ||||||||||||||
|
|
@@ -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 | ||||||||||||||
|
|
@@ -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)) | ||||||||||||||
|
|
@@ -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 | ||||||||||||||
| { | ||||||||||||||
|
|
@@ -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); | ||||||||||||||
|
|
||||||||||||||
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.
So we're now reading
ALLOW_INSECURE_EXTERNAL_BINDING,AZURE_MCP_DANGEROUSLY_DISABLE_HTTPS_REDIRECTION, andAZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERSvia 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
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.
We can opt to selectively restrict it to env here by design.