MSI V2 (IMDSv2): Identity‑scoped mTLS certificate cache #5469

gladjohn · 2025-09-09T23:56:15Z

This PR adds a process‑wide, identity‑scoped cache for the IMDSv2 mTLS client certificate, wires it into the MSI V2 flow

First iteration: cache the certificate
Next iteration: add tests, plug-in mTLS Authentication Operation to support Pop flows

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/CertificateCache.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/CertificateCache.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/CertificateCache.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/CertificateCache.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/CertificateCache.cs

bgavrilMS · 2025-09-17T12:04:55Z

src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs

                .ConfigureAwait(false);

+            // If using IMDSv2 (mTLS) and the mTLS cert is near expiry, skip the AT cache
+            if (cachedAccessTokenItem != null && ManagedIdentityClient.s_sourceName == ManagedIdentitySource.ImdsV2)


The condition here does not match the comment. You should only look for a certificate if an mtls pop token is requested

Let's please avoid peppering the codebase with if (static.sourceName == IMDSv2). First of all, it creates possible race conditions, because you don't know if the source has been evaluated (which is the case here, why would I care what the source is if I have a cached token?). Second, because there should be 1 place (Single Responsability Principle) that decides if MTLS POP is supported or not (e.g. each of the MSI sources should say this)

bgavrilMS · 2025-09-17T12:05:46Z

src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs

+            // If using IMDSv2 (mTLS) and the mTLS cert is near expiry, skip the AT cache
+            if (cachedAccessTokenItem != null && ManagedIdentityClient.s_sourceName == ManagedIdentitySource.ImdsV2)
+            {
+                string identityKey = ServiceBundle.Config.ClientId;


Naming is confusing. avoid using identity as it has too many meanings. How about mtlsCertCacheKey ?

bgavrilMS · 2025-09-17T12:06:50Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs

        }
+
+        // Identity-aware check over the mTLS cert cache
+        internal static bool IsMtlsCertExpiringSoon(string identityKey)


THis is not a responsability of the ManagedIdentityClient.

bgavrilMS · 2025-09-17T12:09:41Z

src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs

+            {
+                string identityKey = ServiceBundle.Config.ClientId;
+
+                if (ManagedIdentityClient.IsMtlsCertExpiringSoon(identityKey))


How about smth like if (cert = cache.GetCertifiacate(key) && ~cert.IsExpiringSoon())? This will avoid having to expose a new complex API that does 2 things at once.

Alternatively, the cache.GetCertificate() can simply return NULL if the certificate is close to expiry. This is what we do for tokens.

bgavrilMS · 2025-09-17T12:10:38Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs

+        internal static readonly ConcurrentDictionary<string,
+            (X509Certificate2 Cert, string ClientId, string TenantId, string Endpoint)> s_miCerts = new();

        internal static void ResetSourceForTest()


Name no longer matches intent

bgavrilMS · 2025-09-17T12:11:59Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs

        {
            s_sourceName = ManagedIdentitySource.None;
+            s_miCerts.Clear();
+            s_timeService = new TimeService();


The point of the time service is that it should be a singleton. A single instance is injected in all classes that deal with time. That instance can either be the real time or a test time. Instead, you are using many instances, so you are not really sure what you test.

But I agree that a big refactor here could take time, maybe add a task to use TimeService everywhere in MSAL where DateTime.now is used.

bgavrilMS · 2025-09-17T12:17:52Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs

        private const string WindowsHimdsFilePath = "%Programfiles%\\AzureConnectedMachineAgent\\himds.exe";
        private const string LinuxHimdsFilePath = "/opt/azcmagent/bin/himds";
        internal static ManagedIdentitySource s_sourceName = ManagedIdentitySource.None;
+        internal static ITimeService s_timeService = new TimeService();


internal variables are not prefixed with s_ (see MtlsCertRefreshSkew )

bgavrilMS · 2025-09-17T12:19:26Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs

+        }
+
+        // Test-only helpers
+        internal static bool IsCertExpiringSoon(X509Certificate2 cert, DateTime nowUtc, TimeSpan skew)


Why did you bother introducing a test service then?

bgavrilMS · 2025-09-17T12:21:45Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs

+            return nowUtc >= (notAfterUtc - skew);
+        }
+
+        internal static void SetTimeServiceForTest(ITimeService timeService) /* internal - test only usage */


It's already a internal property, why do you need an extra setter?

bgavrilMS · 2025-09-17T12:22:05Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs

+        internal static ITimeService s_timeService = new TimeService();
+        internal static readonly TimeSpan MtlsCertRefreshSkew = TimeSpan.FromMinutes(5);
+
+        internal static readonly ConcurrentDictionary<string,


have you considered moving the cache elsewhere? It seems that this class doesn't even use it?

bgavrilMS · 2025-09-17T12:23:20Z

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs

-                privateKey);
+            // 2) Try cached binding; refresh if cert expires within 5 minutes
+            if (!ManagedIdentityClient.s_miCerts.TryGetValue(key, out var binding) ||
+                ManagedIdentityClient.IsMtlsCertExpiringSoon(key))


another example why IsMtlsCertExpiringSoon is a bad API. You perform a cache GET twice even though you have your value.

bgavrilMS · 2025-09-17T12:30:27Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs

+        internal static readonly TimeSpan MtlsCertRefreshSkew = TimeSpan.FromMinutes(5);
+
+        internal static readonly ConcurrentDictionary<string,
+            (X509Certificate2 Cert, string ClientId, string TenantId, string Endpoint)> s_miCerts = new();


You are caching a lot more than certs, so I am not sure this is a good name.

bgavrilMS · 2025-09-17T13:12:22Z

src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs

            MsalAccessTokenCacheItem cachedAccessTokenItem = await GetCachedAccessTokenAsync()
                .ConfigureAwait(false);

+            // If using IMDSv2 (mTLS) and the mTLS cert is near expiry, skip the AT cache


In this file, you do not take care of requesting a POP token form the cache (for which an IAuthenticationOperation is needed)

initial

59da533

gladjohn requested a review from a team as a code owner September 9, 2025 23:56

bgavrilMS reviewed Sep 10, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/CertificateCache.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Sep 10, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Sep 10, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/CertificateCache.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Sep 10, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Sep 10, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/CertificateCache.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Sep 10, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs Outdated Show resolved Hide resolved

bgavrilMS reviewed Sep 10, 2025

View reviewed changes

src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs Outdated Show resolved Hide resolved

Robbie-Microsoft reviewed Sep 10, 2025

View reviewed changes

AzureAD deleted a comment from Robbie-Microsoft Sep 10, 2025

gladjohn added 2 commits September 11, 2025 10:27

address pr comments

50910c0

cache + cert check

a5e2d41

bgavrilMS reviewed Sep 17, 2025

View reviewed changes

gladjohn closed this Oct 1, 2025

MSI V2 (IMDSv2): Identity‑scoped mTLS certificate cache #5469

MSI V2 (IMDSv2): Identity‑scoped mTLS certificate cache #5469

Uh oh!

Conversation

gladjohn commented Sep 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Sep 17, 2025 • 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!

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!

Uh oh!

bgavrilMS Sep 17, 2025 •

edited

Loading