[MSI v2] Cache MSI v2 cert #5557

gladjohn · 2025-10-28T20:26:39Z

commit 1 - caches MSI v2 in-memory + tests (ready)
commit 2 - stores cert to store and if there is a cache miss, checks store (in-progress)
commit 3 - adds mutex (in-progress)

This PR adds a minimal, internal in‑memory certificate cache for IMDS v2 mTLS PoP:

One entry per cache key (the key is exactly what the caller passes: client_id, object_id, or resource_id), by design.
Entries are returned only if valid ≥ 24 hours, otherwise they are ignored and evicted (mTLS tokens are valid up to 24h and must be bound to a cert valid for the full token lifetime).
The cache returns a clone of the X509Certificate2 so callers can dispose independently. The cache owns its copy and disposes on eviction/clear.
Normalized to UTC for time comparisons.
Thread‑safe: ConcurrentDictionary + an external per‑key SemaphoreSlim (unchanged) prevents double‑mints.

Why 24 hours?

mTLS PoP tokens can be valid up to 24h; reusing a cert that expires sooner would create a token that cannot be used by the resource throughout its validity. The cache therefore refuses to store certs with < 24h remaining and evicts entries that have hit their NotAfter.

MZOLN · 2025-10-28T21:14:48Z

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

+        internal static readonly ICertificateCache s_mtlsCertificateCache = new InMemoryCertificateCache();
+
+        // Per-key async de-duplication so concurrent callers don’t double-mint.
+        internal static readonly ConcurrentDictionary<string, SemaphoreSlim> s_perKeyGates =


This design introduces unnecessary coupling and breaks encapsulation, leading to class envy. The responsibility should either reside where it’s used or be abstracted into a dedicated component.

Additionally, usage should avoid direct field access. Instead, expose functionality through well-defined internal or public methods behind a thoughtful interface.

For instance, why isn’t s_perKeyGates treated as an implementation detail of the imdsv2 component, given that it appears to be used only there? The same applies to the cache instance.

MZOLN · 2025-10-28T21:15:16Z

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

+            Endpoint = endpoint ?? throw new MsalClientException(nameof(endpoint));
+            ClientId = clientId ?? throw new MsalClientException(nameof(clientId));
+        }
+


Wouldnt we need comments for public members?

these are internal

MZOLN · 2025-10-28T21:19:28Z

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

+        /// </summary>
+        bool TryGet(
+            string cacheKey,
+            out X509Certificate2 certificate,


This is a very classic way of doing it, wouldnt it be better to use a tuple, valuetype result or even a record/structure here so that we have some flexibility with adding fields without breaking the contract?

MZOLN · 2025-10-28T21:20:43Z

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

+    /// Process-local cache for an mTLS certificate and its endpoint.
+    /// Expiration is based solely on certificate.NotAfter.
+    /// </summary>
+    internal interface ICertificateCache : IDisposable


I'd say that it shouldnt inerit form IDisposable, but the implementation has.

MZOLN · 2025-10-28T21:21:03Z

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

+    /// certificate+endpoint+clientId cache stored in process memory.
+    /// </summary>
+    internal sealed class InMemoryCertificateCache : ICertificateCache
+    {


This should implement IDisposable, not the interface, imho

agreed, updated

MZOLN · 2025-10-28T21:40:18Z

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

+        }
+
+        /// <summary>
+        /// used for logging cache keys without exposing full values.


nit: Start from capital letter

MZOLN · 2025-10-28T21:40:39Z

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

+        /// <summary>
+        /// used for logging cache keys without exposing full values.
+        /// </summary>
+        /// <param name="s"></param>


Should we fill those up as well?

updated but this is internal

MZOLN · 2025-10-28T21:41:14Z

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

+        {
+            if (string.IsNullOrEmpty(s))
+                return "<empty>";
+            var take = Math.Min(8, s.Length);


so it would expose full value if it is less than 8? Is that ok?

MZOLN · 2025-10-28T21:43:18Z

tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ImdsV2Tests.cs

+            using (var httpManager = new MockHttpManager())
+            {
+                // Start clean across tests
+                ManagedIdentityClient.ResetSourceForTest();


How do you ensure that those tests dont race? Because we are using static state here, this could cause instabilities.

removed this - and each test before initializing will always call ResetSourceForTest, so the static caches are cleared in PRODUCT.

MZOLN · 2025-10-28T21:45:28Z

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

+
        internal static void ResetSourceForTest()
        {
            s_sourceName = ManagedIdentitySource.None;


Since we leak the dependency to test, this utility metod can belong in test itself....
A better approach would be to expose appropriate public methods on well encapsulated components.

moved the one in IMDS V2 to it's own method and call it from here

…_cert - Resolve conflicts in ManagedIdentityClient.cs and ImdsV2ManagedIdentitySource.cs - Update tests (ImdsV2Tests, MockHelpers)

MZOLN · 2025-10-29T00:41:20Z

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

+                return; // already disposed
+            }
+
+            try


try/catch is expensive. You won't double dispose with that atomic switch you got on line 70, would you?

MZOLN · 2025-10-29T00:42:16Z

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

+        /// </summary>
+        void Set(
+            string cacheKey,
+            X509Certificate2 certificate,


should we adjust the set as well? This looks better actually ;)

agree - missed updating this

init

56d0b6b

MZOLN requested changes Oct 28, 2025

View reviewed changes

gladjohn added 2 commits October 28, 2025 15:03

Merge origin/rginsburg/msiv2_feature_branch into gladjohn/cahed_msiv2…

f346915

…_cert - Resolve conflicts in ManagedIdentityClient.cs and ImdsV2ManagedIdentitySource.cs - Update tests (ImdsV2Tests, MockHelpers)

pr comments

7431502

MZOLN reviewed Oct 29, 2025

View reviewed changes

pr comments

98098bb

gladjohn marked this pull request as ready for review October 29, 2025 03:52

gladjohn requested a review from a team as a code owner October 29, 2025 03:52

gladjohn requested a review from MZOLN October 29, 2025 03:52

MZOLN merged commit 4b284c3 into rginsburg/msiv2_feature_branch Oct 29, 2025
3 checks passed

MZOLN deleted the gladjohn/cahed_msiv2_cert branch October 29, 2025 17:59

MZOLN approved these changes Oct 29, 2025

View reviewed changes

[MSI v2] Cache MSI v2 cert #5557

[MSI v2] Cache MSI v2 cert #5557

Uh oh!

Conversation

gladjohn commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR adds a minimal, internal in‑memory certificate cache for IMDS v2 mTLS PoP:

Why 24 hours?

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!

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!

MZOLN Oct 29, 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gladjohn commented Oct 28, 2025 •

edited

Loading

MZOLN Oct 29, 2025 •

edited

Loading