Skip to content

Commit 338e32f

Browse files
committed
Fix thread-safety race in CustomChainedCredential credential initialization
The ??= compound assignment is not atomic in C#. Under concurrent GetToken / GetTokenAsync calls before the credential chain was initialized (e.g. parallel tool calls on server startup in HTTP mode), two threads could both observe _credential == null, construct separate ChainedTokenCredential instances, and have one silently discarded. The two chains may hold diverged MSAL token cache handles, causing intermittent authentication failures. Fix: replace the nullable field + ??= pattern with Lazy<TokenCredential> using LazyThreadSafetyMode.ExecutionAndPublication, which provides guaranteed single initialization under concurrent access with no manual locking required. Also adds GetToken_ConcurrentCalls_InitializesCredentialExactlyOnce test to verify the Lazy<T>.IsValueCreated post-concurrent access. Closes #398
1 parent 6393ea8 commit 338e32f

3 files changed

Lines changed: 48 additions & 7 deletions

File tree

core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Services/Azure/Authentication/CustomChainedCredentialTests.cs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,38 @@ public void UnknownCredentialValue_LogsWarning_AndFallsBackToDefaultChain()
469469
Assert.Contains("AZURE_TOKEN_CREDENTIALS", warning);
470470
}
471471

472+
/// <summary>
473+
/// Tests that concurrent calls to GetToken initialize the credential chain exactly once.
474+
/// Validates that the Lazy&lt;T&gt; (ExecutionAndPublication) fix for the ??= race condition
475+
/// ensures a single shared credential instance under concurrent access.
476+
/// </summary>
477+
[Fact]
478+
public async Task GetToken_ConcurrentCalls_InitializesCredentialExactlyOnce()
479+
{
480+
// Arrange
481+
var credential = CreateCustomChainedCredential();
482+
var credentialType = GetCustomChainedCredentialType();
483+
var lazyField = credentialType.GetField("_credential",
484+
BindingFlags.Instance | BindingFlags.NonPublic);
485+
Assert.NotNull(lazyField);
486+
487+
// Act — fire 20 concurrent GetToken calls before the chain is initialized
488+
var tasks = Enumerable.Range(0, 20).Select(_ => Task.Run(() =>
489+
{
490+
try { credential.GetToken(new TokenRequestContext(["https://management.azure.com/.default"]), CancellationToken.None); }
491+
catch { /* auth failure expected in test environment */ }
492+
})).ToArray();
493+
494+
await Task.WhenAll(tasks);
495+
496+
// Assert — the Lazy<T> value was created and is a single shared instance
497+
var lazyValue = lazyField.GetValue(credential);
498+
Assert.NotNull(lazyValue);
499+
var isValueCreated = (bool)lazyValue.GetType()
500+
.GetProperty("IsValueCreated")!.GetValue(lazyValue)!;
501+
Assert.True(isValueCreated);
502+
}
503+
472504
/// <summary>
473505
/// Helper method to create CustomChainedCredential using reflection since it's an internal class.
474506
/// </summary>

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

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,18 @@ namespace Microsoft.Mcp.Core.Services.Azure.Authentication;
8484
/// If not set, System-Assigned Managed Identity will be used.
8585
/// </para>
8686
/// </remarks>
87-
internal class CustomChainedCredential(string? tenantId = null, ILogger<CustomChainedCredential>? logger = null, bool forceBrowserFallback = false) : TokenCredential
87+
internal class CustomChainedCredential : TokenCredential
8888
{
89-
private TokenCredential? _credential;
90-
private readonly ILogger<CustomChainedCredential>? _logger = logger;
89+
private readonly Lazy<TokenCredential> _credential;
90+
private readonly ILogger<CustomChainedCredential>? _logger;
91+
92+
internal CustomChainedCredential(string? tenantId = null, ILogger<CustomChainedCredential>? logger = null, bool forceBrowserFallback = false)
93+
{
94+
_logger = logger;
95+
_credential = new Lazy<TokenCredential>(
96+
() => CreateCredential(tenantId, logger, forceBrowserFallback),
97+
LazyThreadSafetyMode.ExecutionAndPublication);
98+
}
9199

92100
/// <summary>
93101
/// Cloud configuration for authority host. Set by DI container during service registration.
@@ -102,14 +110,12 @@ internal class CustomChainedCredential(string? tenantId = null, ILogger<CustomCh
102110

103111
public override AccessToken GetToken(TokenRequestContext requestContext, CancellationToken cancellationToken)
104112
{
105-
_credential ??= CreateCredential(tenantId, _logger, forceBrowserFallback);
106-
return _credential.GetToken(requestContext, cancellationToken);
113+
return _credential.Value.GetToken(requestContext, cancellationToken);
107114
}
108115

109116
public override ValueTask<AccessToken> GetTokenAsync(TokenRequestContext requestContext, CancellationToken cancellationToken)
110117
{
111-
_credential ??= CreateCredential(tenantId, _logger, forceBrowserFallback);
112-
return _credential.GetTokenAsync(requestContext, cancellationToken);
118+
return _credential.Value.GetTokenAsync(requestContext, cancellationToken);
113119
}
114120

115121
private const string AuthenticationRecordEnvVarName = "AZURE_MCP_AUTHENTICATION_RECORD";
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
changes:
2+
- section: "Bugs Fixed"
3+
description: "Fixed a thread-safety issue in CustomChainedCredential where concurrent calls to GetToken/GetTokenAsync before initialization could construct duplicate credential chains. The credential is now initialized via Lazy<T> (ExecutionAndPublication) to guarantee single initialization under concurrent access."

0 commit comments

Comments
 (0)