[MSI v2 - Feature] Add cert cache and Auth Operation to IMDS V2 #5509
Conversation
| RevokedTokenHash: {!string.IsNullOrEmpty(RevokedTokenHash)} | ||
| """); | ||
|
|
||
| logger.Info(() => |
There was a problem hiding this comment.
nit: add it to the prev logger.Info, it's important info.
There was a problem hiding this comment.
fixed, next commit will include it.
tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ImdsV2Tests.cs
Outdated
Show resolved
Hide resolved
| { | ||
| try | ||
| { store.Remove(existing); } | ||
| catch { } |
There was a problem hiding this comment.
I think you should at least log if there are errors here?
|
|
||
| internal static void ResetSourceForTest() | ||
| // Per-identity, process-wide. Identity key = MSAL Config.ClientId (SAMI/UAMI). | ||
| internal static readonly ConcurrentDictionary<string, ImdsV2BindingMetadata> s_imdsV2Binding = |
There was a problem hiding this comment.
Please stop using internal fields. Use internal properties.
There was a problem hiding this comment.
Naming: IdToBindingMetadataMap ?
| } | ||
|
|
||
| // Test only method to remove all test binding certs from user store. | ||
| internal static void RemoveAllTestBindingCertsFromUserStoreForTest() |
There was a problem hiding this comment.
Why is this part of the product? Does eSTS send these certificates?
| { store.Remove(c); } | ||
| catch { } | ||
| } | ||
| store.Close(); |
There was a problem hiding this comment.
use a finally clause to dispose of unamaged resources
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces comprehensive support for mTLS (mutual TLS) certificate binding in Managed Identity authentication flows for IMDS v2. The main focus is adding certificate caching capabilities and authentication operation overrides to support both bearer and proof-of-possession (PoP) token scenarios.
Key changes include:
- Implementation of certificate caching mechanism with user store persistence for mTLS bindings
- Addition of authentication operation override capability at the request level
- Enhanced test coverage for certificate lifecycle management and identity isolation
- Streamlined request handling logic with improved error handling and proactive refresh capabilities
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertStore.cs |
New certificate store utility for installing and retrieving mTLS certificates from user store |
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs |
Enhanced IMDS v2 source with certificate caching and binding metadata management |
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs |
Added certificate binding cache and test cleanup utilities |
src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs |
Refactored request logic with improved cache handling and PoP support |
src/client/Microsoft.Identity.Client/Internal/Requests/AuthenticationRequestParameters.cs |
Added authentication operation override capability |
src/client/Microsoft.Identity.Client/ApiConfig/Parameters/AcquireTokenForManagedIdentityParameters.cs |
Added certificate and CSR response properties for mTLS support |
tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ImdsV2Tests.cs |
Comprehensive test updates with new certificate caching and lifecycle tests |
| try | ||
| { store.Remove(existing); } | ||
| catch { } |
There was a problem hiding this comment.
Empty catch blocks swallow all exceptions and make debugging difficult. Consider logging the exception or catching specific exception types that are expected and safe to ignore.
Copilot uses AI. Check for mistakes.
| try | ||
| { store.Remove(c); } | ||
| catch { } |
There was a problem hiding this comment.
Empty catch blocks swallow all exceptions and make debugging difficult. Consider logging the exception or catching specific exception types that are expected and safe to ignore.
Copilot uses AI. Check for mistakes.
| catch { } | ||
| } | ||
| store.Close(); | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
Empty catch blocks swallow all exceptions and make debugging difficult. Consider logging the exception or catching specific exception types that are expected and safe to ignore.
| catch { } | |
| } | |
| store.Close(); | |
| } | |
| catch { } | |
| catch (Exception ex) | |
| { | |
| Console.WriteLine($"Exception removing certificate: {ex}"); | |
| } | |
| store.Close(); | |
| } | |
| catch (Exception ex) | |
| { | |
| Console.WriteLine($"Exception in RemoveAllTestBindingCertsFromUserStoreForTest: {ex}"); | |
| } |
Copilot uses AI. Check for mistakes.
|
|
||
| var mi = miBuilder.Build(); | ||
|
|
||
| // IMPORTANT: Use in‑memory keys for bearer path (no attestation) |
There was a problem hiding this comment.
Replace Unicode hyphen with ASCII hyphen in comment.
| // IMPORTANT: Use in‑memory keys for bearer path (no attestation) | |
| // IMPORTANT: Use in-memory keys for bearer path (no attestation) |
Copilot uses AI. Check for mistakes.
|
|
||
| var sami = samiBuilder.Build(); | ||
|
|
||
| // In‑memory keys for bearer path |
There was a problem hiding this comment.
Replace Unicode hyphen with ASCII hyphen in comment.
| // In‑memory keys for bearer path | |
| // In-memory keys for bearer path |
Copilot uses AI. Check for mistakes.
|
|
||
| var uami = uamiBuilder.Build(); | ||
|
|
||
| // In‑memory keys for bearer path |
There was a problem hiding this comment.
Replace Unicode hyphen with ASCII hyphen in comment.
| // In‑memory keys for bearer path | |
| // In-memory keys for bearer path |
Copilot uses AI. Check for mistakes.
|
|
||
| var mi = miBuilder.Build(); | ||
|
|
||
| // In‑memory keys for bearer path |
There was a problem hiding this comment.
Replace Unicode hyphen with ASCII hyphen in comment.
| // In‑memory keys for bearer path | |
| // In-memory keys for bearer path |
Copilot uses AI. Check for mistakes.
|
|
||
| var mi = miBuilder.Build(); | ||
|
|
||
| // In‑memory keys for bearer path |
There was a problem hiding this comment.
Replace Unicode hyphen with ASCII hyphen in comment.
| // In‑memory keys for bearer path | |
| // In-memory keys for bearer path |
Copilot uses AI. Check for mistakes.
| requestFromCache.MtlsCertificate = certFromStore; // attach for TLS (and PoP) | ||
| requestFromCache.CertificateRequestResponse = cachedResponse; | ||
|
|
||
| _requestContext.Logger.Info("[IMDSv2] Using user‑store mTLS binding and cached IMDSv2 metadata for identity."); |
There was a problem hiding this comment.
Replace Unicode hyphen with ASCII hyphen in log message.
| _requestContext.Logger.Info("[IMDSv2] Using user‑store mTLS binding and cached IMDSv2 metadata for identity."); | |
| _requestContext.Logger.Info("[IMDSv2] Using user-store mTLS binding and cached IMDSv2 metadata for identity."); |
Copilot uses AI. Check for mistakes.
| if (cachedAccessTokenItem != null) | ||
| { | ||
| string cachedTokenHash = _cryptoManager.CreateSha256HashHex(cachedAccessTokenItem.Secret); | ||
| // Compute revoked‑token hash from the cached token |
There was a problem hiding this comment.
Replace Unicode hyphen with ASCII hyphen in comment.
| // Compute revoked‑token hash from the cached token | |
| // Compute revoked-token hash from the cached token |
Copilot uses AI. Check for mistakes.
| ILoggerAdapter logger = AuthenticationRequestParameters.RequestContext.Logger; | ||
|
|
||
| // 1. FIRST, handle ForceRefresh | ||
| bool popRequested = _managedIdentityParameters.IsMtlsPopRequested || AuthenticationRequestParameters.IsMtlsPopRequested; |
There was a problem hiding this comment.
Why do we have 2 flags for this?
| bool popRequested = _managedIdentityParameters.IsMtlsPopRequested || AuthenticationRequestParameters.IsMtlsPopRequested; | ||
|
|
||
| // If mtls_pop was requested and a binding exists for this identity, load it from the user store before cache lookup | ||
| ApplyMtlsOverrideFromUserStoreIfAvailable(popRequested, logger); |
There was a problem hiding this comment.
- Design - do not pass the popRequested flag to this method.
- Naming - name is confusing. How about "SetPopAuthenticationSchemeWhenCached()` or smth ?
| //log a warning if Claims are also set | ||
| if (!string.IsNullOrEmpty(AuthenticationRequestParameters.Claims)) | ||
| { | ||
| logger.Warning("[ManagedIdentityRequest] Both ForceRefresh and Claims are set. Using ForceRefresh to skip cache."); |
| // Copy claims (if any) so MI sources can add them | ||
| if (string.IsNullOrEmpty(_managedIdentityParameters.Claims) && !string.IsNullOrEmpty(AuthenticationRequestParameters.Claims)) | ||
| { | ||
| _managedIdentityParameters.Claims = AuthenticationRequestParameters.Claims; |
There was a problem hiding this comment.
Can this copying of parameters happen somewhere else, in a centralized way? When we add a new param, do we need to remember to change smth?
There was a problem hiding this comment.
Also, why do you set it here? This block of code deals with force refresh only?
| MsalAccessTokenCacheItem cachedAccessTokenItem = await GetCachedAccessTokenAsync() | ||
| .ConfigureAwait(false); | ||
| // 2) Single cache lookup (do NOT count a hit here) | ||
| MsalAccessTokenCacheItem cachedAccessTokenItem = await GetCachedAccessTokenAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
But you haven't set the pop key here? What are you looking for in the cache? This will do a Bearer cache lookup even in the POP case.
| _managedIdentityKeyProvider = serviceBundle.PlatformProxy.ManagedIdentityKeyProvider; | ||
| } | ||
|
|
||
| protected override async Task<AuthenticationResult> ExecuteAsync(CancellationToken cancellationToken) |
There was a problem hiding this comment.
Consider splitting the orchestration logic between Bearer and POP. They are sufficiently different to warrant their own code paths. Use private methods to minimize code duplication.
There was a problem hiding this comment.
The cyclomatic complexity of this method is quite high - 10 before the changes, 13 after. There are so many "if pop"
| MsalAccessTokenCacheItem cachedAccessTokenItem = await GetCachedAccessTokenAsync().ConfigureAwait(false); | ||
|
|
||
| // 3) Claims present => force network and compute revoked-token hash (original behavior) | ||
| bool hasClaims = |
There was a problem hiding this comment.
Pls don't use 2 flags for the same property.
| _managedIdentityParameters.Claims = AuthenticationRequestParameters.Claims; | ||
| if (string.IsNullOrEmpty(_managedIdentityParameters.Claims)) | ||
| { | ||
| _managedIdentityParameters.Claims = AuthenticationRequestParameters.Claims; |
There was a problem hiding this comment.
Duplicate code, this already exists at line 59
| { | ||
| if (!popRequested) | ||
| return; | ||
| if (AuthenticationRequestParameters.AuthenticationOperationOverride != null) |
There was a problem hiding this comment.
Ideally this method should not have to deal with this/
| // 3. If we have no ForceRefresh and no claims, we can use the cache | ||
| if (cachedAccessTokenItem != null) | ||
| // 4) If PoP requested but no binding cert has been applied yet, bypass cache and mint one | ||
| if (popRequested && AuthenticationRequestParameters.AuthenticationOperationOverride == null) |
There was a problem hiding this comment.
Can we remove AuthenticationOperationOverride and use just AuthenticationOperation with a setter?
| // 3. If we have no ForceRefresh and no claims, we can use the cache | ||
| if (cachedAccessTokenItem != null) | ||
| // 4) If PoP requested but no binding cert has been applied yet, bypass cache and mint one | ||
| if (popRequested && AuthenticationRequestParameters.AuthenticationOperationOverride == null) |
There was a problem hiding this comment.
Checking this property means is a convoluted way of saying "do I have a cached cert?". Can you check the cert instead?
| if (cacheReason == CacheRefreshReason.NoCachedAccessToken) | ||
| { | ||
| authResult = await SendTokenRequestForManagedIdentityAsync(logger, cancellationToken).ConfigureAwait(false); | ||
| var recheck = await GetCachedAccessTokenAsync().ConfigureAwait(false); |
| if (cacheReason == CacheRefreshReason.NoCachedAccessToken) | ||
| { | ||
| authResult = await SendTokenRequestForManagedIdentityAsync(logger, cancellationToken).ConfigureAwait(false); | ||
| var recheck = await GetCachedAccessTokenAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
This will fetch a bearer token even in the POP case. I think you are missing a test like:
- get a bearer token
- get a pop token (and expect it to get it from STS)
| var recheck = await GetCachedAccessTokenAsync().ConfigureAwait(false); | ||
| if (recheck != null) | ||
| { | ||
| return CreateAuthenticationResultFromCache(recheck); |
There was a problem hiding this comment.
if there's a bearer token cached, you just returned a result
| if (fallback != null) | ||
| { | ||
| logger.Info("[ManagedIdentityRequest] Proactive refresh canceled before send; returning cached access token."); | ||
| return CreateAuthenticationResultFromCache(fallback); |
There was a problem hiding this comment.
this gets a bearer token in pop case
| // Propagate PoP and claims from common params to MI params | ||
| _managedIdentityParameters.IsMtlsPopRequested = popRequested; | ||
|
|
||
| if (string.IsNullOrEmpty(_managedIdentityParameters.Claims) && |
There was a problem hiding this comment.
This is the 3rd time we deal with claims.
| } | ||
|
|
||
| // Ensure the attestation provider reaches RequestContext for IMDSv2 | ||
| AuthenticationRequestParameters.RequestContext.AttestationTokenProvider ??= |
There was a problem hiding this comment.
Should't this be at the Config / App level?
| namespace Microsoft.Identity.Client.ManagedIdentity.V2 | ||
| { | ||
| /// <summary> | ||
| /// Provides persistence mechanisms for certificate binding metadata in the Windows certificate store. |
There was a problem hiding this comment.
Everything that is Windows specific should be named "Windows" and should be used behind an interface, so as to be able to provide a Linux implementaiton.
| /// <remarks> | ||
| /// This class uses the X509Certificate2.FriendlyName property to store encoded metadata that links | ||
| /// certificates to specific managed identities and token types. The metadata includes: | ||
| /// - Identity key (hashed for privacy) |
| /// <param name="tokenType">The token type (Bearer or PoP)</param> | ||
| /// <param name="resp">The certificate response containing endpoint and identity information</param> | ||
| /// <returns>A formatted string for use as certificate FriendlyName</returns> | ||
| public static string BuildFriendlyName(string identityKey, string tokenType, CertificateRequestResponse resp) |
| /// The persistence mechanism enables MSAL to find previously created certificates for an identity | ||
| /// across application restarts, reducing the need to repeatedly mint new certificates. | ||
| /// </remarks> | ||
| internal static class BindingMetadataPersistence |
There was a problem hiding this comment.
Think about SOLID and how much power there is in using interfaces and abstractions. Avoid using static everywhere.
| // Prefix that identifies certificates managed by MSAL for Managed Identities | ||
| private const string Prefix = "MSAL_MI_MTLS|v1|"; | ||
| private const char Sep = '|'; | ||
|
|
There was a problem hiding this comment.
This class should take in a logger and do some logging.
|
|
||
| return string.Concat(Prefix, tokenType, Sep, hid, Sep, client, Sep, tenant, Sep, ep); | ||
| } | ||
| catch { return null; } |
There was a problem hiding this comment.
Please stop catching all errors. We will never know if smth is wrong. Let everything bubble up.
| /// <param name="subject">Output parameter for the certificate subject</param> | ||
| /// <param name="thumbprint">Output parameter for the certificate thumbprint</param> | ||
| /// <returns>True if binding metadata was successfully recovered, false otherwise</returns> | ||
| public static bool TryRehydrateFromStore( |
There was a problem hiding this comment.
Consider using regular caching / persistance names like Get or TryGet
| store.Open(OpenFlags.ReadOnly); | ||
|
|
||
| // Find all certificates with our prefix in the FriendlyName | ||
| var candidates = store.Certificates.OfType<X509Certificate2>() |
There was a problem hiding this comment.
As per copilot "Summary:
Due to .NET's API limitations, searching by FriendlyName requires enumerating and filtering all certificates. If you can use another property for more selective searching, do so. Otherwise, the current method is standard practice, but may not scale well with a large certificate store."
So this doesn't really scale. Have you tested the perf for this? Do you plan to add a memory cache in front of this?
| foreach (var c in candidates) | ||
| { | ||
| // Parse the FriendlyName to extract the encoded metadata | ||
| if (!TryParse(c.FriendlyName, out var tType, out var h, out var clientId, out var tenantId, out var ep)) |
There was a problem hiding this comment.
Pls stop using these cryptic short variable names, they are very hard to read.
| // Find all certificates with our prefix in the FriendlyName | ||
| var candidates = store.Certificates.OfType<X509Certificate2>() | ||
| .Where(c => !string.IsNullOrEmpty(c.FriendlyName) && | ||
| c.FriendlyName.StartsWith(Prefix, StringComparison.Ordinal)) |
There was a problem hiding this comment.
Why can't you do a full match on the key?
| logger?.Info("[IMDSv2] Rehydrated binding metadata from certificate store (FriendlyName tag)."); | ||
| return true; | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Why don't you let this bubble up?
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger?.Verbose(() => $"[IMDSv2] Store rehydration failed: {ex.GetType().Name}: {ex.Message}"); |
There was a problem hiding this comment.
Why not logger.error? In any case, you should let it bubble up.
| /// <summary> | ||
| /// Encodes binary data as base64url without padding to avoid separator conflicts. | ||
| /// </summary> | ||
| private static string Base64UrlNoPad(byte[] data) |
There was a problem hiding this comment.
What's wrong with Base64Url encoding which we already use eveyrwhere ?
| /// <summary> | ||
| /// Decodes base64url-formatted string back to binary data. | ||
| /// </summary> | ||
| private static byte[] Base64UrlNoPadDecode(string s) |
There was a problem hiding this comment.
Why not use base64url encoding helpers?
| statusCode); | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
The default should be no comments for private methods. Comments denote smth exceptional. Otherwise the name of the method should be all it is needed
| string v = null; | ||
| try | ||
| { v = Environment.GetEnvironmentVariable(EnvImdsV2Bearer); } | ||
| catch { /* ignore */ } |
| /// <summary> | ||
| /// Creates a deterministic jitter delay from identity information. | ||
| /// This ensures multiple processes don't all try to rotate at exactly the same moment, | ||
| /// while maintaining stability (same input always produces same delay). |
There was a problem hiding this comment.
Why do you need this stability? We use a random -5 +5 min jitter for proactive token refresh and that seems to work fine.
| int seconds = val % (maxSeconds + 1); | ||
| return TimeSpan.FromSeconds(seconds); | ||
| } | ||
| catch { return TimeSpan.Zero; } |
| /// - Rotation uses a cross-process named mutex + stable jitter so only one process mints. | ||
| /// - Do NOT delete the existing valid binding (A). Only purge certs expired > 7 days. | ||
| /// </remarks> | ||
| internal sealed class MsiCertManager |
There was a problem hiding this comment.
@mzervos-msft - pls review this
| m = null; | ||
| return false; | ||
| } | ||
| catch // Handle access denied or other mutex-related exceptions |
|
|
||
| // Helper method to sanitize strings for mutex name creation | ||
| // Mutex names have character restrictions across platforms | ||
| static string Sanitize(string s) |
There was a problem hiding this comment.
This is base64url encoding and we have helpers for it - https://base64.guru/standards/base64url
| var globalName = @"Global\MSAL_MI_mTLS_ROT_" + suffix; | ||
|
|
||
| if (TryOpenAndLock(globalName, out var mGlobal)) | ||
| return mGlobal; // Successfully acquired Global mutex |
There was a problem hiding this comment.
You generally want to use a try / finally pattern here to release the mutex.
| /// Proactive rotation at half-life ensures new certificates are created well before | ||
| /// expiration, reducing the risk of authentication failures during certificate transitions. | ||
| /// </remarks> | ||
| internal static bool IsBeyondHalfLife(X509Certificate2 cert, ILoggerAdapter logger = null) |
There was a problem hiding this comment.
I think we call this "NeedsRefresh"
| /// - Foreground rotation: Remove older certificates, keep only specified thumbprint | ||
| /// - Background rotation: Only purge very stale certificates, preserve valid ones | ||
| /// | ||
| /// All store operations are best-effort with appropriate error handling to accommodate |
There was a problem hiding this comment.
"best-effort" is exceptional. Use normal effort, i.e. no try catch
| /// Proactive rotation at half-life ensures new certificates are created well before | ||
| /// expiration, reducing the risk of authentication failures during certificate transitions. | ||
| /// </remarks> | ||
| internal static bool IsBeyondHalfLife(X509Certificate2 cert, ILoggerAdapter logger = null) |
There was a problem hiding this comment.
I would move these helper methods elsewhere, this file is already large
| string keepThumbprint, | ||
| ILoggerAdapter logger) | ||
| bool cleanupOlder, | ||
| out string resolvedThumbprint, |
There was a problem hiding this comment.
Don't you have X509Certiicate2.Thumbprint ?
3 participants
Fixes - adds cert cache logic to MSI V2 flow
Changes proposed in this request
This pull request introduces support for mTLS (mutual TLS) certificate binding in Managed Identity token acquisition, along with enhancements to request parameter handling and logging. The changes add new properties for managing mTLS certificates and certificate responses, update test reset logic, and refactor how authentication operations are selected for requests.
Managed Identity mTLS Support and Parameter Enhancements:
MtlsCertificateandCertificateRequestResponseproperties toAcquireTokenForManagedIdentityParametersto support mTLS certificate binding and handling of IMDSv2 certificate responses.AcquireTokenForManagedIdentityParametersto include mTLS-related information, such as whether mTLS PoP is requested and the thumbprint of the bound certificate.Authentication Operation Handling:
AuthenticationOperationOverrideinAuthenticationRequestParametersto allow per-request overrides of the authentication operation, and updated theAuthenticationSchemeproperty to prefer the override if present. [1] [2]Test and Codebase Maintenance:
ApplicationBase.ResetStateForTest()to callResetSourceAndBindingForTest()for proper cleanup of Managed Identity client state, supporting the new mTLS logic.Refactoring and Dependency Updates:
ManagedIdentityAuthRequestto align with the new mTLS and Managed Identity v2 support. [1] [2]Testing
unit
Performance impact
none
Documentation
n/a now