From ddca8f7cef9c250c12a75c5661f94db705c47d55 Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Wed, 20 May 2026 18:26:49 -0400 Subject: [PATCH 1/7] Detect orphaned KG certs via public key modulus comparison After a VM or app restart, KeyGuard regenerates the per-boot private key in the same CNG container while the old cert remains on disk. The cert appears valid (time-wise, HasPrivateKey=true) but the key material has changed. Fix: compare the cert's embedded public key modulus against the public key currently exported from the CNG container. A mismatch conclusively means the container was regenerated and the cert is orphaned. The check runs inside WindowsPersistentCertificateCache.Read during candidate selection, so the loop continues looking for a valid cert rather than returning an unusable one. Non-CNG keys (software CSP) are accepted on faith since KeyGuard-style regeneration does not apply to them. CryptographicException during key load is treated as orphaned (key inaccessible = unusable). The CNG container modified-time heuristic (Check 3 from the original proposal) is intentionally omitted. Both it and the modulus comparison detect the same event, but the modulus check is definitive: independently generated RSA keys sharing a modulus is computationally infeasible. The modified-time check is a heuristic with a known false-negative window and adds no additional coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../V2/MtlsCertificateCache.cs | 72 ++++++++++++++++ .../V2/WindowsPersistentCertificateCache.cs | 8 ++ .../PersistentCertificateStoreUnitTests.cs | 82 +++++++++++++++++++ 3 files changed, 162 insertions(+) diff --git a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertificateCache.cs b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertificateCache.cs index 538c0fd249..dbe9b832e2 100644 --- a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertificateCache.cs +++ b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertificateCache.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Concurrent; +using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Threading; using System.Threading.Tasks; @@ -185,5 +186,76 @@ public void RemoveBadCert(string cacheKey, ILoggerAdapter logger) logger?.Verbose(() => $"[PersistentCert] Error removing from persistent cache: {ex.Message}"); } } + + /// + /// Returns if the cert's embedded public key does not match the + /// public key currently in the associated CNG container, indicating the container was + /// regenerated (e.g. by KeyGuard on reboot) while the cert on disk still references the + /// old key material. + /// + internal static bool IsCertKeyOrphaned(X509Certificate2 cert, ILoggerAdapter logger) + { + if (cert is null) + return true; + + try + { + using var rsaKey = cert.GetRSAPrivateKey(); + if (rsaKey is not RSACng rsaCng) + { + // Non-CNG key (e.g. software CSP) — cannot perform KG container check; accept. + return false; + } + + return !PublicKeyMatchesCert(rsaCng, cert, logger); + } + catch (CryptographicException ex) + { + logger?.Verbose(() => + $"[PersistentCert] Cannot load private key for orphan check: {ex.Message}. Treating cert as unusable."); + return true; + } + } + + /// + /// Returns if the public key exported from + /// matches the public key embedded in . + /// A mismatch means the container holds different key material than when the cert was issued. + /// + /// + /// Check 3 from the original proposal — comparing the CNG container's + /// NCRYPT_LAST_MODIFIED_PROPERTY against the cert's NotBefore — is + /// intentionally omitted. Both Check 3 and this modulus comparison detect the same event: + /// KeyGuard regenerating the key in the container post-reboot. This check is definitive: + /// two independently generated RSA keys sharing a modulus is computationally infeasible, + /// so a mismatch conclusively means the container was regenerated. Check 3 is a heuristic + /// with a known false-negative window (a reboot occurring within one minute of cert + /// issuance), and adds no coverage that this check does not already provide. + /// + internal static bool PublicKeyMatchesCert(RSACng containerKey, X509Certificate2 cert, ILoggerAdapter logger) + { + try + { + var containerParams = containerKey.ExportParameters(includePrivateParameters: false); + using var certPubKey = cert.GetRSAPublicKey(); + if (certPubKey is null) + return false; + + var certParams = certPubKey.ExportParameters(includePrivateParameters: false); + + return containerParams.Modulus is not null + && certParams.Modulus is not null + && containerParams.Modulus.AsSpan().SequenceEqual(certParams.Modulus) + && containerParams.Exponent is not null + && certParams.Exponent is not null + && containerParams.Exponent.AsSpan().SequenceEqual(certParams.Exponent); + } + catch (CryptographicException ex) + { + logger?.Verbose(() => + $"[PersistentCert] Public key export failed during orphan check: {ex.Message}. Treating cert as orphaned."); + return false; + } + } } } diff --git a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs index 2f1ddff3cb..803577d003 100644 --- a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs +++ b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs @@ -84,6 +84,14 @@ public bool Read(string alias, out CertificateCacheValue value, ILoggerAdapter l continue; } + // Skip certs whose CNG container key no longer matches the cert's public key. + // This detects orphaned certs left on disk after a reboot regenerates the KG per-boot key. + if (MtlsBindingCache.IsCertKeyOrphaned(candidate, logger)) + { + logger.Verbose(() => "[PersistentCert] Candidate skipped: CNG container key does not match cert public key (orphaned post-reboot)."); + continue; + } + if (candidate.NotAfter > bestNotAfter) { best?.Dispose(); diff --git a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs index 4777a88a10..6c2e8baa80 100644 --- a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs @@ -843,5 +843,87 @@ private static bool WaitForAliasCount(string alias, int expected, int retries = } #endregion + + #region IsCertKeyOrphaned tests + + [TestMethod] + public void IsCertKeyOrphaned_ReturnsTrue_For_NullCert() + { + // Arrange (no setup needed) + + // Act + bool result = MtlsBindingCache.IsCertKeyOrphaned(null, null); + + // Assert + Assert.IsTrue(result, "Null cert should be treated as orphaned."); + } + + [TestMethod] + public void IsCertKeyOrphaned_ReturnsFalse_For_ValidCert() + { + WindowsOnly(); + + // Arrange - cert whose private key in the CNG container matches the cert's embedded public key + using var cert = CreateSelfSignedCert(TimeSpan.FromDays(14), "CN=ValidCertOrphanTest"); + var logger = Substitute.For(); + + // Act + bool result = MtlsBindingCache.IsCertKeyOrphaned(cert, logger); + + // Assert + Assert.IsFalse(result, "A cert whose private key matches its embedded public key should not be considered orphaned."); + } + + [TestMethod] + public void PublicKeyMatchesCert_ReturnsTrue_When_KeyMatchesCert() + { + WindowsOnly(); + + // Arrange - cert created with key1; pass key1 as the container key + using var key1 = RSA.Create(2048); + var rsaCng1 = key1 as RSACng; + Assert.IsNotNull(rsaCng1, "Expected RSACng on Windows."); + + var req = new System.Security.Cryptography.X509Certificates.CertificateRequest( + new X500DistinguishedName("CN=KeyMatchTest"), + key1, + HashAlgorithmName.SHA256, + RSASignaturePadding.Pkcs1); + using var cert = req.CreateSelfSigned(DateTimeOffset.UtcNow.AddMinutes(-1), DateTimeOffset.UtcNow.AddDays(14)); + + // Act + bool result = MtlsBindingCache.PublicKeyMatchesCert(rsaCng1, cert, null); + + // Assert + Assert.IsTrue(result, "The key used to create the cert should match the cert's embedded public key."); + } + + [TestMethod] + public void PublicKeyMatchesCert_ReturnsFalse_When_ModulusMismatch() + { + WindowsOnly(); + + // Arrange - cert created with key1, but we pass key2 as the container key + // (simulates post-reboot KG regeneration: same container, new key material) + using var key1 = RSA.Create(2048); + using var key2 = RSA.Create(2048); + var rsaCng2 = key2 as RSACng; + Assert.IsNotNull(rsaCng2, "Expected RSACng on Windows."); + + var req = new System.Security.Cryptography.X509Certificates.CertificateRequest( + new X500DistinguishedName("CN=KeyMismatchTest"), + key1, + HashAlgorithmName.SHA256, + RSASignaturePadding.Pkcs1); + using var cert = req.CreateSelfSigned(DateTimeOffset.UtcNow.AddMinutes(-1), DateTimeOffset.UtcNow.AddDays(14)); + + // Act + bool result = MtlsBindingCache.PublicKeyMatchesCert(rsaCng2, cert, null); + + // Assert + Assert.IsFalse(result, "A different key than the one used to create the cert should produce a modulus mismatch."); + } + + #endregion } } From bfa01fe1588226c8d0515ffac6295706654bf164 Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Wed, 20 May 2026 18:35:52 -0400 Subject: [PATCH 2/7] Fix PublicKeyMatchesCert tests: use new RSACng(2048) directly RSA.Create(2048) does not return RSACng on ARM64 Windows (.NET 8). Use new RSACng(2048) explicitly so the cast is always valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../PersistentCertificateStoreUnitTests.cs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs index 6c2e8baa80..95aa384f08 100644 --- a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs @@ -880,9 +880,7 @@ public void PublicKeyMatchesCert_ReturnsTrue_When_KeyMatchesCert() WindowsOnly(); // Arrange - cert created with key1; pass key1 as the container key - using var key1 = RSA.Create(2048); - var rsaCng1 = key1 as RSACng; - Assert.IsNotNull(rsaCng1, "Expected RSACng on Windows."); + using var key1 = new RSACng(2048); var req = new System.Security.Cryptography.X509Certificates.CertificateRequest( new X500DistinguishedName("CN=KeyMatchTest"), @@ -892,7 +890,7 @@ public void PublicKeyMatchesCert_ReturnsTrue_When_KeyMatchesCert() using var cert = req.CreateSelfSigned(DateTimeOffset.UtcNow.AddMinutes(-1), DateTimeOffset.UtcNow.AddDays(14)); // Act - bool result = MtlsBindingCache.PublicKeyMatchesCert(rsaCng1, cert, null); + bool result = MtlsBindingCache.PublicKeyMatchesCert(key1, cert, null); // Assert Assert.IsTrue(result, "The key used to create the cert should match the cert's embedded public key."); @@ -905,10 +903,8 @@ public void PublicKeyMatchesCert_ReturnsFalse_When_ModulusMismatch() // Arrange - cert created with key1, but we pass key2 as the container key // (simulates post-reboot KG regeneration: same container, new key material) - using var key1 = RSA.Create(2048); - using var key2 = RSA.Create(2048); - var rsaCng2 = key2 as RSACng; - Assert.IsNotNull(rsaCng2, "Expected RSACng on Windows."); + using var key1 = new RSACng(2048); + using var key2 = new RSACng(2048); var req = new System.Security.Cryptography.X509Certificates.CertificateRequest( new X500DistinguishedName("CN=KeyMismatchTest"), @@ -918,7 +914,7 @@ public void PublicKeyMatchesCert_ReturnsFalse_When_ModulusMismatch() using var cert = req.CreateSelfSigned(DateTimeOffset.UtcNow.AddMinutes(-1), DateTimeOffset.UtcNow.AddDays(14)); // Act - bool result = MtlsBindingCache.PublicKeyMatchesCert(rsaCng2, cert, null); + bool result = MtlsBindingCache.PublicKeyMatchesCert(key2, cert, null); // Assert Assert.IsFalse(result, "A different key than the one used to create the cert should produce a modulus mismatch."); From b8cc13bad18d778d91230616ef3956e65fa0501b Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Wed, 20 May 2026 18:57:37 -0400 Subject: [PATCH 3/7] Remove orphaned KG certs from store on detection When IsCertKeyOrphaned detects a cert whose CNG container key no longer matches the cert's public key (post-reboot KeyGuard regeneration), the cert is now removed from the Windows cert store instead of just skipped. Previously the orphaned cert stayed in the store indefinitely, causing a redundant modulus check on every Read call. Changes: - WindowsPersistentCertificateCache.Read: collect orphaned cert thumbprints during the scan loop; after the loop, open ReadWrite store and remove them (best-effort, wrapped in try/catch) - Added internal Read overload accepting Func isOrphaned delegate for testability; IPersistentCertificateCache interface unchanged - Added unit test: Read_RemovesOrphanedCert_FromStore Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../V2/WindowsPersistentCertificateCache.cs | 67 ++++++++++++++++++- .../PersistentCertificateStoreUnitTests.cs | 32 +++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs index 803577d003..9335bed8dd 100644 --- a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs +++ b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.Collections.Generic; using System.Linq; using System.Security.Cryptography.X509Certificates; using Microsoft.Identity.Client.Core; @@ -32,6 +33,16 @@ namespace Microsoft.Identity.Client.ManagedIdentity.V2 internal sealed class WindowsPersistentCertificateCache : IPersistentCertificateCache { public bool Read(string alias, out CertificateCacheValue value, ILoggerAdapter logger) + { + return Read(alias, out value, logger, MtlsBindingCache.IsCertKeyOrphaned); + } + + // Internal overload for testability: accepts an injectable orphan-check delegate. + internal bool Read( + string alias, + out CertificateCacheValue value, + ILoggerAdapter logger, + Func isOrphaned) { value = default; @@ -56,6 +67,7 @@ public bool Read(string alias, out CertificateCacheValue value, ILoggerAdapter l X509Certificate2 best = null; string bestEndpoint = null; DateTime bestNotAfter = DateTime.MinValue; + List orphanedThumbprints = null; foreach (var candidate in items) { @@ -86,9 +98,12 @@ public bool Read(string alias, out CertificateCacheValue value, ILoggerAdapter l // Skip certs whose CNG container key no longer matches the cert's public key. // This detects orphaned certs left on disk after a reboot regenerates the KG per-boot key. - if (MtlsBindingCache.IsCertKeyOrphaned(candidate, logger)) + // Collect thumbprints for post-loop removal so the store is only opened once for writes. + if (isOrphaned(candidate, logger)) { logger.Verbose(() => "[PersistentCert] Candidate skipped: CNG container key does not match cert public key (orphaned post-reboot)."); + orphanedThumbprints ??= new List(); + orphanedThumbprints.Add(candidate.Thumbprint); continue; } @@ -106,6 +121,12 @@ public bool Read(string alias, out CertificateCacheValue value, ILoggerAdapter l } } + // Remove any orphaned certs discovered during the scan. + if (orphanedThumbprints is { Count: > 0 }) + { + RemoveByThumbprints(alias, orphanedThumbprints, logger); + } + if (best != null) { // CN (GUID) → canonical client_id @@ -139,6 +160,50 @@ public bool Read(string alias, out CertificateCacheValue value, ILoggerAdapter l return false; } + private static void RemoveByThumbprints(string alias, List thumbprints, ILoggerAdapter logger) + { + try + { + using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); + store.Open(OpenFlags.ReadWrite); + + X509Certificate2[] items; + try + { + items = new X509Certificate2[store.Certificates.Count]; + store.Certificates.CopyTo(items, 0); + } + catch (Exception ex) + { + logger.Verbose(() => "[PersistentCert] Orphan-removal snapshot via CopyTo failed; falling back to enumeration. Details: " + ex.Message); + items = store.Certificates.Cast().ToArray(); + } + + int removed = 0; + foreach (var cert in items) + { + try + { + if (thumbprints.Contains(cert.Thumbprint)) + { + store.Remove(cert); + removed++; + } + } + finally + { + cert.Dispose(); + } + } + + logger.Verbose(() => $"[PersistentCert] Removed {removed} orphaned cert(s) for alias '{alias}'."); + } + catch (Exception ex) + { + logger.Verbose(() => "[PersistentCert] Orphan removal failed (best-effort): " + ex.Message); + } + } + public void Write(string alias, X509Certificate2 cert, string endpointBase, ILoggerAdapter logger) { if (cert == null) diff --git a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs index 95aa384f08..56e66fb4be 100644 --- a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs @@ -920,6 +920,38 @@ public void PublicKeyMatchesCert_ReturnsFalse_When_ModulusMismatch() Assert.IsFalse(result, "A different key than the one used to create the cert should produce a modulus mismatch."); } + [TestMethod] + public void Read_RemovesOrphanedCert_FromStore() + { + WindowsOnly(); + + // Arrange + var alias = "alias-orphan-remove-" + Guid.NewGuid().ToString("N"); + var ep = "https://fake_mtls/orphan"; + var guid = Guid.NewGuid().ToString("D"); + + try + { + using var cert = CreateSelfSignedWithKey("CN=" + guid, TimeSpan.FromDays(14)); + _cache.Write(alias, cert, ep, Logger); + + // Verify cert is in the store + Assert.AreEqual(1, CountAliasInStore(alias), "Cert should be in store after Write."); + + // Act — use the injectable overload: treat every cert as orphaned + var concreteCache = (WindowsPersistentCertificateCache)_cache; + bool readResult = concreteCache.Read(alias, out _, Logger, (_, __) => true); + + // Assert + Assert.IsFalse(readResult, "Read should return false when all candidates are orphaned."); + Assert.IsTrue(WaitForAliasCount(alias, 0), "Orphaned cert should have been removed from the store."); + } + finally + { + RemoveAliasFromStore(alias); + } + } + #endregion } } From 6fb42a9ddfc45a123d052d15fc7e42128d1cd664 Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Wed, 20 May 2026 19:09:03 -0400 Subject: [PATCH 4/7] Handle null GetRSAPrivateKey for RSA certs in IsCertKeyOrphaned GetRSAPrivateKey() returns null for both non-RSA certs (ECDSA, DSA) and for RSA certs with an inaccessible private key. Previously both fell into the 'accept on faith' branch, which could allow a useless RSA cert to pass the orphan check. Fix: check GetRSAPublicKey() to distinguish RSA certs from non-RSA certs. An RSA cert with no accessible private key is treated as orphaned. Non-RSA certs continue to be accepted on faith (KG regeneration does not apply). Add unit test: IsCertKeyOrphaned_ReturnsTrue_For_RsaCert_WithNoPrivateKey Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../V2/MtlsCertificateCache.cs | 13 +++++++++++- .../PersistentCertificateStoreUnitTests.cs | 21 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertificateCache.cs b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertificateCache.cs index dbe9b832e2..f7f128389c 100644 --- a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertificateCache.cs +++ b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MtlsCertificateCache.cs @@ -201,9 +201,20 @@ internal static bool IsCertKeyOrphaned(X509Certificate2 cert, ILoggerAdapter log try { using var rsaKey = cert.GetRSAPrivateKey(); + if (rsaKey is null) + { + // GetRSAPrivateKey() returns null for non-RSA certs (e.g. ECDSA) AND for RSA + // certs where the private key is inaccessible. Distinguish the two cases: + // if the cert has an RSA public key, the private key should be present but isn't + // → the cert is unusable. If there is no RSA public key, it's a non-RSA cert + // that we can't check → accept on faith. + using var pubKey = cert.GetRSAPublicKey(); + return pubKey is not null; // RSA cert + inaccessible private key = orphaned + } + if (rsaKey is not RSACng rsaCng) { - // Non-CNG key (e.g. software CSP) — cannot perform KG container check; accept. + // Non-CNG RSA key (e.g. software CSP) — cannot perform KG container check; accept. return false; } diff --git a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs index 56e66fb4be..8089ae4f08 100644 --- a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs @@ -874,6 +874,27 @@ public void IsCertKeyOrphaned_ReturnsFalse_For_ValidCert() Assert.IsFalse(result, "A cert whose private key matches its embedded public key should not be considered orphaned."); } + [TestMethod] + public void IsCertKeyOrphaned_ReturnsTrue_For_RsaCert_WithNoPrivateKey() + { + // Arrange - public-only RSA cert (no private key associated) + using var rsa = RSA.Create(2048); + var req = new System.Security.Cryptography.X509Certificates.CertificateRequest( + new X500DistinguishedName("CN=NoPrivKeyTest"), + rsa, + HashAlgorithmName.SHA256, + RSASignaturePadding.Pkcs1); + // Create a cert without associating the private key (CopyWithPrivateKey not called) + using var cert = req.CreateSelfSigned(DateTimeOffset.UtcNow.AddMinutes(-1), DateTimeOffset.UtcNow.AddDays(14)); + using var publicOnlyCert = new X509Certificate2(cert.Export(X509ContentType.Cert)); + + // Act — public-only RSA cert: GetRSAPrivateKey() returns null, GetRSAPublicKey() succeeds + bool result = MtlsBindingCache.IsCertKeyOrphaned(publicOnlyCert, null); + + // Assert + Assert.IsTrue(result, "An RSA cert with no accessible private key should be treated as orphaned."); + } + [TestMethod] public void PublicKeyMatchesCert_ReturnsTrue_When_KeyMatchesCert() { From 686d28d947f9003e7b9f1fd1764a9b23bf7a119b Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Thu, 21 May 2026 09:30:54 -0400 Subject: [PATCH 5/7] Address PR review: rename TryRead, add SnapshotCertificates helper, lock orphan removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename internal Read overload to public TryRead with out param last (public Read delegates to TryRead with the default IsCertKeyOrphaned check) - Extract SnapshotCertificates(store, logger) helper to eliminate 5 duplicate try/catch CopyTo fallback blocks across Read, Write, DeleteAllForAlias, PruneExpiredForAlias, and RemoveByThumbprints - Wrap RemoveByThumbprints in InterprocessLock.TryWithAliasLock (300ms, best-effort) — same pattern as Write/Delete/DeleteAllForAlias to prevent concurrent store writes during orphan cleanup - Update orphan removal test to call TryRead with the new signature Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../V2/WindowsPersistentCertificateCache.cs | 139 +++++++----------- .../PersistentCertificateStoreUnitTests.cs | 2 +- 2 files changed, 57 insertions(+), 84 deletions(-) diff --git a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs index 9335bed8dd..44ffe9578c 100644 --- a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs +++ b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs @@ -34,15 +34,15 @@ internal sealed class WindowsPersistentCertificateCache : IPersistentCertificate { public bool Read(string alias, out CertificateCacheValue value, ILoggerAdapter logger) { - return Read(alias, out value, logger, MtlsBindingCache.IsCertKeyOrphaned); + return TryRead(alias, logger, MtlsBindingCache.IsCertKeyOrphaned, out value); } - // Internal overload for testability: accepts an injectable orphan-check delegate. - internal bool Read( + // Overload for testability: accepts an injectable orphan-check delegate. + public bool TryRead( string alias, - out CertificateCacheValue value, ILoggerAdapter logger, - Func isOrphaned) + Func isOrphaned, + out CertificateCacheValue value) { value = default; @@ -52,17 +52,7 @@ internal bool Read( store.Open(OpenFlags.OpenExistingOnly | OpenFlags.ReadOnly); // Snapshot first to avoid provider quirks ("collection modified" during enumeration). - X509Certificate2[] items; - try - { - items = new X509Certificate2[store.Certificates.Count]; - store.Certificates.CopyTo(items, 0); - } - catch (Exception ex) - { - logger.Verbose(() => "[PersistentCert] Store snapshot via CopyTo failed; falling back to enumeration. Details: " + ex.Message); - items = store.Certificates.Cast().ToArray(); - } + var items = SnapshotCertificates(store, logger); X509Certificate2 best = null; string bestEndpoint = null; @@ -162,46 +152,43 @@ internal bool Read( private static void RemoveByThumbprints(string alias, List thumbprints, ILoggerAdapter logger) { - try - { - using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); - store.Open(OpenFlags.ReadWrite); - - X509Certificate2[] items; - try - { - items = new X509Certificate2[store.Certificates.Count]; - store.Certificates.CopyTo(items, 0); - } - catch (Exception ex) - { - logger.Verbose(() => "[PersistentCert] Orphan-removal snapshot via CopyTo failed; falling back to enumeration. Details: " + ex.Message); - items = store.Certificates.Cast().ToArray(); - } - - int removed = 0; - foreach (var cert in items) + InterprocessLock.TryWithAliasLock( + alias, + timeout: TimeSpan.FromMilliseconds(300), + action: () => { try { - if (thumbprints.Contains(cert.Thumbprint)) + using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); + store.Open(OpenFlags.ReadWrite); + + var items = SnapshotCertificates(store, logger); + int removed = 0; + + foreach (var cert in items) { - store.Remove(cert); - removed++; + try + { + if (thumbprints.Contains(cert.Thumbprint)) + { + store.Remove(cert); + removed++; + } + } + finally + { + cert.Dispose(); + } } + + logger.Verbose(() => $"[PersistentCert] Removed {removed} orphaned cert(s) for alias '{alias}'."); } - finally + catch (Exception ex) { - cert.Dispose(); + logger.Verbose(() => "[PersistentCert] Orphan removal failed (best-effort): " + ex.Message); } - } - - logger.Verbose(() => $"[PersistentCert] Removed {removed} orphaned cert(s) for alias '{alias}'."); - } - catch (Exception ex) - { - logger.Verbose(() => "[PersistentCert] Orphan removal failed (best-effort): " + ex.Message); - } + }, + logVerbose: s => logger.Verbose(() => s)); } public void Write(string alias, X509Certificate2 cert, string endpointBase, ILoggerAdapter logger) @@ -241,17 +228,7 @@ public void Write(string alias, X509Certificate2 cert, string endpointBase, ILog // Skip write if a newer/equal, non-expired binding for this alias already exists. DateTime newestForAliasUtc = DateTime.MinValue; - X509Certificate2[] present; - try - { - present = new X509Certificate2[store.Certificates.Count]; - store.Certificates.CopyTo(present, 0); - } - catch (Exception ex) - { - logger.Verbose(() => "[PersistentCert] Store snapshot via CopyTo failed; falling back to enumeration. Details: " + ex.Message); - present = store.Certificates.Cast().ToArray(); - } + var present = SnapshotCertificates(store, logger); foreach (var existing in present) { @@ -356,18 +333,7 @@ public void DeleteAllForAlias(string alias, ILoggerAdapter logger) using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); store.Open(OpenFlags.ReadWrite); - X509Certificate2[] items; - try - { - items = new X509Certificate2[store.Certificates.Count]; - store.Certificates.CopyTo(items, 0); - } - catch (Exception ex) - { - logger.Verbose(() => "[PersistentCert] Store snapshot via CopyTo failed; falling back to enumeration. Details: " + ex.Message); - items = store.Certificates.Cast().ToArray(); - } - + var items = SnapshotCertificates(store, logger); int removed = 0; foreach (var existing in items) @@ -399,28 +365,35 @@ public void DeleteAllForAlias(string alias, ILoggerAdapter logger) } /// - /// Deletes only certificates that are actually expired (NotAfter < nowUtc), - /// scoped to the given alias (cache key) via FriendlyName. + /// Safely snapshots all certificates in into an array. + /// Falls back to LINQ enumeration if CopyTo throws (some providers don't support it). /// - private static void PruneExpiredForAlias( - X509Store store, - string aliasCacheKey, - DateTime nowUtc, - ILoggerAdapter logger) + private static X509Certificate2[] SnapshotCertificates(X509Store store, ILoggerAdapter logger) { - X509Certificate2[] items; try { - items = new X509Certificate2[store.Certificates.Count]; - // Safe snapshot for providers that throw if removing while iterating + var items = new X509Certificate2[store.Certificates.Count]; store.Certificates.CopyTo(items, 0); + return items; } catch (Exception ex) { - logger.Verbose(() => "[PersistentCert] Prune snapshot via CopyTo failed; falling back to enumeration. Details: " + ex.Message); - items = store.Certificates.Cast().ToArray(); + logger.Verbose(() => "[PersistentCert] Store snapshot via CopyTo failed; falling back to enumeration. Details: " + ex.Message); + return store.Certificates.Cast().ToArray(); } + } + /// + /// Deletes only certificates that are actually expired (NotAfter < nowUtc), + /// scoped to the given alias (cache key) via FriendlyName. + /// + private static void PruneExpiredForAlias( + X509Store store, + string aliasCacheKey, + DateTime nowUtc, + ILoggerAdapter logger) + { + var items = SnapshotCertificates(store, logger); int removed = 0; foreach (var existing in items) diff --git a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs index 8089ae4f08..12d43e053f 100644 --- a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/PersistentCertificateStoreUnitTests.cs @@ -961,7 +961,7 @@ public void Read_RemovesOrphanedCert_FromStore() // Act — use the injectable overload: treat every cert as orphaned var concreteCache = (WindowsPersistentCertificateCache)_cache; - bool readResult = concreteCache.Read(alias, out _, Logger, (_, __) => true); + bool readResult = concreteCache.TryRead(alias, Logger, (_, __) => true, out _); // Assert Assert.IsFalse(readResult, "Read should return false when all candidates are orphaned."); From 8b0b9a81e9c94d2f470e8de3dd14b39285cfa403 Mon Sep 17 00:00:00 2001 From: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com> Date: Wed, 27 May 2026 14:28:25 -0700 Subject: [PATCH 6/7] [ManagedIdentity] Detect dead KeyGuard keys and purge orphan IMDSv2 mTLS certs on reboot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the post-reboot recovery path for IMDSv2 mTLS PoP token acquisition. On Azure VM restart the per-boot KeyGuard key (NCryptUsePerBootKeyFlag) is reaped by VBS, but the persisted binding cert under CN=managedidentitysnissuer.login.microsoft.com still references the old public key. The next call then either burns a failed TLS handshake before the reactive SChannel catch kicks in, or — in the zombie-handle variant — falls through entirely because the cert's modulus still matches the dead container. Changes ------- - Add CanSign liveness probe right after CngKey.Open in WindowsCngKeyOperations.TryGetOrCreateKeyGuard. 1-byte RSA-SHA256 PKCS1 sign; ~1-3ms, runs once per process (result is cached in WindowsManagedIdentityKeyProvider._cachedKey). Catches zombie-VBS state where Open succeeds but private material is dead. - Add PurgeManagedIdentityCertificates: one-shot issuer-CN substring sweep of CurrentUser\My, invoked at the moment a fresh KeyGuard key is minted (both the probe-failed path and the Open-threw path). Removes orphaned binding certs at the cause site so the next request doesn't pay any per-Read discovery cost and multi-identity hosts (SAMI + UAMIs sharing the KeyGuard container) are cleaned up uniformly. - Add 4 Windows-only unit tests for the purge filter behavior (matching, non-matching, case-insensitive, only-removes-matching). The reactive SChannel catch in ImdsV2ManagedIdentitySource is retained as a defensive backstop. Validation ---------- Validated E2E on a Server 2022 KeyGuard VM across multiple reboots and mixed SAMI/UAMI cases. Canonical post-reboot first call: - CngKey.Open threw CryptographicException HR=0x8009003A - Fresh KeyGuard key created - PurgeManagedIdentityCertificates removed orphan cert (Inspected=4) - MAA attestation OK - POST /issuecredential -> 200 - mTLS handshake -> 200 on first try (no reactive catch invoked) - Total ~2.8s on cold start Full unit suite green on net8.0: 2069 passed, 0 failed, 19 skipped. Refs #6031. Complementary to #6020 (cert-side modulus comparison): this PR adds the key-side liveness probe and broad issuer-CN sweep at the mint site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../KeyProviders/WindowsCngKeyOperations.cs | 235 +++++++++++- .../WindowsCngKeyOperationsPurgeUnitTests.cs | 359 ++++++++++++++++++ 2 files changed, 592 insertions(+), 2 deletions(-) create mode 100644 tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/WindowsCngKeyOperationsPurgeUnitTests.cs diff --git a/src/client/Microsoft.Identity.Client/ManagedIdentity/KeyProviders/WindowsCngKeyOperations.cs b/src/client/Microsoft.Identity.Client/ManagedIdentity/KeyProviders/WindowsCngKeyOperations.cs index d672689d43..aaecc0b66c 100644 --- a/src/client/Microsoft.Identity.Client/ManagedIdentity/KeyProviders/WindowsCngKeyOperations.cs +++ b/src/client/Microsoft.Identity.Client/ManagedIdentity/KeyProviders/WindowsCngKeyOperations.cs @@ -3,6 +3,7 @@ using System; using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; using Microsoft.Identity.Client.Core; using Microsoft.Identity.Client.Internal; @@ -29,6 +30,12 @@ internal static class WindowsCngKeyOperations private const string KeyGuardVirtualIsoProperty = "Virtual Iso"; private const string VbsNotAvailable = "VBS key isolation is not available"; + // Issuer used by IMDSv2 mTLS PoP binding certificates. Matched as a case-insensitive + // substring against the certificate's Issuer DN, so any cert in CurrentUser\My issued + // by IMDSv2 can be wiped when we mint a fresh KeyGuard key (the previously persisted + // certs are bound to the now-replaced key by name and would fail the mTLS handshake). + internal const string ManagedIdentityIssuerCnFragment = "managedidentitysnissuer.login.microsoft.com"; + // KeyGuard + per-boot flags private const CngKeyCreationOptions NCryptUseVirtualIsolationFlag = (CngKeyCreationOptions)0x00020000; private const CngKeyCreationOptions NCryptUsePerBootKeyFlag = (CngKeyCreationOptions)0x00040000; @@ -66,16 +73,75 @@ public static bool TryGetOrCreateKeyGuard(ILoggerAdapter logger, out RSA rsa) CngKey key; try { + logger?.Info(() => $"[MI][WinKeyProvider] Attempting to open existing KeyGuard key. " + + $"Provider='{SoftwareKspName}', KeyName='{KeyGuardKeyName}', Scope=UserKey, Silent=true."); + key = CngKey.Open( KeyGuardKeyName, new CngProvider(SoftwareKspName), CngKeyOpenOptions.UserKey | CngKeyOpenOptions.Silent); + + logger?.Info(() => $"[MI][WinKeyProvider] CngKey.Open succeeded for '{KeyGuardKeyName}'. " + + "Running liveness sign probe to detect stale per-boot key material " + + "(metadata file can survive a reboot while the VBS-isolated key material is destroyed)."); + + // Liveness probe: per-boot KeyGuard keys (NCryptUsePerBootKeyFlag) leave a stale + // metadata file on disk after reboot. CngKey.Open returns a handle, but the actual + // VBS-protected key material is gone, so the first real sign operation fails. + // Detect this here so we can recreate cleanly instead of failing later in the + // mTLS handshake or signing path. + if (!CanSign(key, logger)) + { + logger?.Info(() => "[MI][WinKeyProvider] KeyGuard liveness sign probe FAILED. " + + "Treating handle as stale (likely post-reboot per-boot key reaped). " + + "Disposing stale handle and recreating fresh KeyGuard key."); + key.Dispose(); + key = CreateFresh(logger); + + if (key == null) + { + logger?.Info(() => "[MI][WinKeyProvider] CreateFresh returned null after failed liveness probe " + + "(VBS unavailable). KeyGuard path will be skipped."); + } + else + { + logger?.Info(() => "[MI][WinKeyProvider] Fresh KeyGuard key created successfully after stale handle replacement. " + + "Purging persisted IMDSv2 mTLS binding certificates that were bound to the replaced key."); + + // The new KeyGuard key reuses the container name 'KeyGuardRSAKey', but its + // public/private pair is different from the one any persisted cert was issued + // against. Wipe all certs in CurrentUser\My issued by IMDSv2 so the next request + // mints fresh instead of failing the mTLS handshake. + PurgeManagedIdentityCertificates(logger); + } + } + else + { + logger?.Info(() => "[MI][WinKeyProvider] KeyGuard liveness sign probe PASSED. Reusing existing handle."); + } } - catch (CryptographicException) + catch (CryptographicException openEx) { // Not found -> create fresh (helper may return null if VBS unavailable) - logger?.Info(() => "[MI][WinKeyProvider] CredentialGuard key not found; creating fresh."); + logger?.Info(() => $"[MI][WinKeyProvider] CngKey.Open threw CryptographicException for '{KeyGuardKeyName}'. " + + $"HR=0x{openEx.HResult:X8}, Message='{openEx.Message}'. " + + "Treating as 'key not found' and creating fresh."); key = CreateFresh(logger); + + if (key == null) + { + logger?.Info(() => "[MI][WinKeyProvider] CreateFresh returned null after Open failure (VBS unavailable)."); + } + else + { + logger?.Info(() => "[MI][WinKeyProvider] Fresh KeyGuard key created successfully after Open failure. " + + "Purging persisted IMDSv2 mTLS binding certificates that were bound to the replaced key."); + + // Same rationale as the probe-failed branch: any persisted IMDSv2 cert in + // CurrentUser\My is bound to the previous KeyGuard key and will fail the mTLS + // handshake. Wipe them so the next request mints fresh. + PurgeManagedIdentityCertificates(logger); + } } // If VBS is unavailable, CreateFresh() returns null. Bail out cleanly. @@ -277,6 +343,171 @@ public static bool IsKeyGuardProtected(CngKey key) return val?.Length > 0 && val[0] != 0; } + /// + /// Performs a small RSA sign operation against the supplied CNG key to verify the + /// underlying key material is actually usable. + /// + /// The CNG key handle returned from . + /// Logger for diagnostic output. + /// + /// if the key signs successfully; otherwise . + /// + /// + /// + /// KeyGuard keys created with NCryptUsePerBootKeyFlag have their VBS-isolated + /// key material destroyed on every reboot, but the on-disk metadata file produced by the + /// Microsoft Software KSP often survives. As a result, + /// can return a handle that looks valid (correct algorithm, "Virtual Iso" property still set) + /// but whose first real cryptographic operation throws. + /// + /// + /// Probing with a one-byte sign here surfaces that condition cheaply (~1-3 ms for RSA-2048) + /// on the cold-start path. Subsequent calls reuse the cached key in + /// WindowsManagedIdentityKeyProvider, so the probe runs at most once per process. + /// + /// + private static bool CanSign(CngKey key, ILoggerAdapter logger) + { + try + { + logger?.Verbose(() => "[MI][WinKeyProvider] Liveness probe: attempting RSA-SHA256 sign of 1-byte payload."); + + using (var rsa = new RSACng(key)) + { + _ = rsa.SignData( + new byte[] { 0 }, + HashAlgorithmName.SHA256, + RSASignaturePadding.Pkcs1); + } + + logger?.Verbose(() => "[MI][WinKeyProvider] Liveness probe: sign succeeded; key material is live."); + return true; + } + catch (CryptographicException ex) + { + logger?.Info(() => $"[MI][WinKeyProvider] Liveness probe: sign threw CryptographicException. " + + $"HR=0x{ex.HResult:X8}, Message='{ex.Message}'. Key handle is stale."); + return false; + } + catch (Exception ex) + { + logger?.Info(() => $"[MI][WinKeyProvider] Liveness probe: sign threw unexpected exception. " + + $"{ex.GetType().Name}: '{ex.Message}'. Treating as stale."); + return false; + } + } + + /// + /// Deletes every certificate in the CurrentUser\My store whose issuer matches the + /// IMDSv2 mTLS PoP binding-certificate issuer. + /// + /// Logger for diagnostic output. + /// + /// + /// IMDSv2 binding certificates are issued by + /// CN=managedidentitysnissuer.login.microsoft.com and stored in the user's personal + /// store. They reference the private key by KSP container name (KeyGuardRSAKey), + /// not by key material. When the KeyGuard key is re-minted (post-reboot, or after a failed + /// liveness probe), the new key reuses the same container name but with different + /// public/private parameters — leaving the persisted certs bound to a key that no longer + /// matches them, which then fails the mTLS handshake. + /// + /// + /// Purging the store at the moment we mint a fresh KeyGuard key eliminates the + /// failed-handshake + retry round trip that the SChannel-error catch in + /// ImdsV2ManagedIdentitySource.AuthenticateAsync would otherwise have to recover from. + /// + /// + /// All store I/O is best-effort and non-throwing. + /// + /// + internal static void PurgeManagedIdentityCertificates(ILoggerAdapter logger) + { + int removed = 0; + int inspected = 0; + + try + { + logger?.Info(() => + $"[MI][WinKeyProvider] PurgeManagedIdentityCertificates: opening CurrentUser\\My to remove " + + $"certs whose Issuer contains '{ManagedIdentityIssuerCnFragment}'."); + + using (var store = new X509Store(StoreName.My, StoreLocation.CurrentUser)) + { + store.Open(OpenFlags.ReadWrite); + + // Snapshot to avoid 'collection modified during enumeration' provider quirks. + var snapshot = new X509Certificate2[store.Certificates.Count]; + try + { + store.Certificates.CopyTo(snapshot, 0); + } + catch (Exception copyEx) + { + logger?.Info(() => + $"[MI][WinKeyProvider] PurgeManagedIdentityCertificates: store snapshot via CopyTo failed " + + $"({copyEx.GetType().Name}: {copyEx.Message}). Falling back to enumeration."); + + int i = 0; + snapshot = new X509Certificate2[store.Certificates.Count]; + foreach (X509Certificate2 c in store.Certificates) + { + snapshot[i++] = c; + } + } + + foreach (X509Certificate2 candidate in snapshot) + { + try + { + inspected++; + + string issuer = candidate.Issuer ?? string.Empty; + if (issuer.IndexOf(ManagedIdentityIssuerCnFragment, StringComparison.OrdinalIgnoreCase) < 0) + { + continue; + } + + string thumb = candidate.Thumbprint; + DateTime notAfter = candidate.NotAfter; + + try + { + store.Remove(candidate); + removed++; + logger?.Info(() => + $"[MI][WinKeyProvider] PurgeManagedIdentityCertificates: removed cert. " + + $"Thumbprint={thumb}, NotAfter={notAfter:O}, Issuer='{issuer}'."); + } + catch (Exception removeEx) + { + logger?.Info(() => + $"[MI][WinKeyProvider] PurgeManagedIdentityCertificates: failed to remove cert " + + $"Thumbprint={thumb}. {removeEx.GetType().Name}: '{removeEx.Message}'."); + } + } + finally + { + candidate.Dispose(); + } + } + } + } + catch (Exception ex) + { + logger?.Info(() => + $"[MI][WinKeyProvider] PurgeManagedIdentityCertificates: store access failed. " + + $"{ex.GetType().Name}: '{ex.Message}'. Removed={removed}, Inspected={inspected}."); + return; + } + + int removedFinal = removed; + int inspectedFinal = inspected; + logger?.Info(() => + $"[MI][WinKeyProvider] PurgeManagedIdentityCertificates: complete. " + + $"Removed={removedFinal}, Inspected={inspectedFinal}."); + } + /// /// Determines whether a cryptographic exception indicates that VBS is unavailable. /// diff --git a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/WindowsCngKeyOperationsPurgeUnitTests.cs b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/WindowsCngKeyOperationsPurgeUnitTests.cs new file mode 100644 index 0000000000..93340f9978 --- /dev/null +++ b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/WindowsCngKeyOperationsPurgeUnitTests.cs @@ -0,0 +1,359 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Linq; +using System.Runtime.InteropServices; +using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; +using Microsoft.Identity.Client.Core; +using Microsoft.Identity.Client.ManagedIdentity.KeyProviders; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using NSubstitute; + +namespace Microsoft.Identity.Test.Unit.ManagedIdentityTests +{ + /// + /// Tests for . + /// The purge sweeps CurrentUser\My and removes every certificate whose issuer + /// contains . + /// It runs after a fresh KeyGuard key is minted so that persisted IMDSv2 binding + /// certs (which are bound by container name to the now-replaced key) are not left + /// behind to fail the next mTLS handshake. + /// + [TestClass] + public class WindowsCngKeyOperationsPurgeUnitTests + { + // Discriminator we plant in each test cert's Subject so cleanup can find leftovers + // from a failed run without touching unrelated certs in the developer's store. + private const string TestSubjectDiscriminatorPrefix = "MSAL-Purge-Test-"; + + private static bool IsWindows => RuntimeInformation.IsOSPlatform(OSPlatform.Windows); + + private static ILoggerAdapter Logger => Substitute.For(); + + [TestInitialize] + public void Init() + { + // Reuse the existing broad sweep so prior test runs don't leak state. + if (ImdsV2TestStoreCleaner.IsWindows) + { + ImdsV2TestStoreCleaner.RemoveAllTestArtifacts(); + } + + // Also remove any leftover purge-test certs from a previous failed run. + RemoveAllPurgeTestArtifacts(); + } + + [TestCleanup] + public void Cleanup() + { + RemoveAllPurgeTestArtifacts(); + } + + private static void WindowsOnly() + { + if (!IsWindows) + { + Assert.Inconclusive("Windows-only"); + } + } + + [TestMethod] + public void PurgeManagedIdentityCertificates_RemovesCertWithMatchingIssuer() + { + WindowsOnly(); + + // Arrange + string discriminator = TestSubjectDiscriminatorPrefix + Guid.NewGuid().ToString("N"); + string subject = + "CN=" + WindowsCngKeyOperations.ManagedIdentityIssuerCnFragment + + ", OU=" + discriminator; + + string plantedThumbprint; + using (var cert = CreateSelfSignedWithKey(subject, TimeSpan.FromDays(2))) + { + plantedThumbprint = cert.Thumbprint; + AddToCurrentUserMyStore(cert); + } + + Assert.IsTrue( + IsInCurrentUserMyStore(plantedThumbprint), + "Test setup precondition: planted cert must be present in CurrentUser\\My before purge."); + + // Act + WindowsCngKeyOperations.PurgeManagedIdentityCertificates(Logger); + + // Assert + Assert.IsFalse( + IsInCurrentUserMyStore(plantedThumbprint), + "Purge should remove certs whose Issuer contains the managed identity issuer CN."); + } + + [TestMethod] + public void PurgeManagedIdentityCertificates_LeavesCertWithNonMatchingIssuer() + { + WindowsOnly(); + + // Arrange + string discriminator = TestSubjectDiscriminatorPrefix + Guid.NewGuid().ToString("N"); + // Subject/Issuer that does NOT contain the managed identity issuer fragment. + string subject = "CN=unrelated.example.test, OU=" + discriminator; + + string plantedThumbprint; + using (var cert = CreateSelfSignedWithKey(subject, TimeSpan.FromDays(2))) + { + plantedThumbprint = cert.Thumbprint; + AddToCurrentUserMyStore(cert); + } + + Assert.IsTrue( + IsInCurrentUserMyStore(plantedThumbprint), + "Test setup precondition: planted cert must be present in CurrentUser\\My before purge."); + + try + { + // Act + WindowsCngKeyOperations.PurgeManagedIdentityCertificates(Logger); + + // Assert + Assert.IsTrue( + IsInCurrentUserMyStore(plantedThumbprint), + "Purge must not remove certs whose Issuer does not contain the managed identity issuer CN."); + } + finally + { + RemoveByThumbprintFromCurrentUserMyStore(plantedThumbprint); + } + } + + [TestMethod] + public void PurgeManagedIdentityCertificates_MatchIsCaseInsensitive() + { + WindowsOnly(); + + // Arrange + string discriminator = TestSubjectDiscriminatorPrefix + Guid.NewGuid().ToString("N"); + // Uppercase the issuer fragment to ensure the match is OrdinalIgnoreCase. + string subject = + "CN=" + WindowsCngKeyOperations.ManagedIdentityIssuerCnFragment.ToUpperInvariant() + + ", OU=" + discriminator; + + string plantedThumbprint; + using (var cert = CreateSelfSignedWithKey(subject, TimeSpan.FromDays(2))) + { + plantedThumbprint = cert.Thumbprint; + AddToCurrentUserMyStore(cert); + } + + Assert.IsTrue( + IsInCurrentUserMyStore(plantedThumbprint), + "Test setup precondition: planted cert must be present in CurrentUser\\My before purge."); + + // Act + WindowsCngKeyOperations.PurgeManagedIdentityCertificates(Logger); + + // Assert + Assert.IsFalse( + IsInCurrentUserMyStore(plantedThumbprint), + "Purge issuer match should be case-insensitive."); + } + + [TestMethod] + public void PurgeManagedIdentityCertificates_OnlyRemovesMatching_LeavesOtherCertsAlone() + { + WindowsOnly(); + + // Arrange: plant one matching and one non-matching cert + string matchDiscriminator = TestSubjectDiscriminatorPrefix + Guid.NewGuid().ToString("N"); + string nonMatchDiscriminator = TestSubjectDiscriminatorPrefix + Guid.NewGuid().ToString("N"); + + string matchingSubject = + "CN=" + WindowsCngKeyOperations.ManagedIdentityIssuerCnFragment + + ", OU=" + matchDiscriminator; + string nonMatchingSubject = "CN=unrelated.example.test, OU=" + nonMatchDiscriminator; + + string matchingThumb; + string nonMatchingThumb; + + using (var matching = CreateSelfSignedWithKey(matchingSubject, TimeSpan.FromDays(2))) + using (var nonMatching = CreateSelfSignedWithKey(nonMatchingSubject, TimeSpan.FromDays(2))) + { + matchingThumb = matching.Thumbprint; + nonMatchingThumb = nonMatching.Thumbprint; + + AddToCurrentUserMyStore(matching); + AddToCurrentUserMyStore(nonMatching); + } + + Assert.IsTrue(IsInCurrentUserMyStore(matchingThumb), "Matching cert must be planted."); + Assert.IsTrue(IsInCurrentUserMyStore(nonMatchingThumb), "Non-matching cert must be planted."); + + try + { + // Act + WindowsCngKeyOperations.PurgeManagedIdentityCertificates(Logger); + + // Assert + Assert.IsFalse(IsInCurrentUserMyStore(matchingThumb), "Matching cert should be purged."); + Assert.IsTrue(IsInCurrentUserMyStore(nonMatchingThumb), "Non-matching cert should survive."); + } + finally + { + RemoveByThumbprintFromCurrentUserMyStore(nonMatchingThumb); + } + } + + // ---------------- helpers ---------------- + + /// + /// Creates a self-signed RSA cert with a persistable private key. + /// Mirrors the pattern used by PersistentCertificateStoreUnitTests.CreateSelfSignedWithKey. + /// + private static X509Certificate2 CreateSelfSignedWithKey(string subject, TimeSpan lifetime) + { + using var rsa = RSA.Create(2048); + + var req = new CertificateRequest( + new X500DistinguishedName(subject), + rsa, + HashAlgorithmName.SHA256, + RSASignaturePadding.Pkcs1); + + DateTimeOffset notBefore = DateTimeOffset.UtcNow.AddMinutes(-2); + DateTimeOffset notAfter = notBefore.Add(lifetime); + + using var ephemeral = req.CreateSelfSigned(notBefore, notAfter); + + // Re-import as PFX so the private key is persisted and the store will accept it. + var pfx = ephemeral.Export(X509ContentType.Pfx, ""); + return new X509Certificate2( + pfx, + "", + X509KeyStorageFlags.Exportable | X509KeyStorageFlags.PersistKeySet); + } + + private static void AddToCurrentUserMyStore(X509Certificate2 cert) + { + using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); + store.Open(OpenFlags.ReadWrite); + store.Add(cert); + } + + private static bool IsInCurrentUserMyStore(string thumbprint) + { + using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); + store.Open(OpenFlags.OpenExistingOnly | OpenFlags.ReadOnly); + + foreach (X509Certificate2 c in store.Certificates) + { + try + { + if (string.Equals(c.Thumbprint, thumbprint, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + finally + { + c.Dispose(); + } + } + + return false; + } + + private static void RemoveByThumbprintFromCurrentUserMyStore(string thumbprint) + { + try + { + using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); + store.Open(OpenFlags.ReadWrite); + + X509Certificate2[] snapshot; + try + { + snapshot = new X509Certificate2[store.Certificates.Count]; + store.Certificates.CopyTo(snapshot, 0); + } + catch + { + snapshot = store.Certificates.Cast().ToArray(); + } + + foreach (var c in snapshot) + { + try + { + if (string.Equals(c.Thumbprint, thumbprint, StringComparison.OrdinalIgnoreCase)) + { + try + { store.Remove(c); } + catch { /* best-effort */ } + } + } + finally + { + c.Dispose(); + } + } + } + catch + { + // best-effort cleanup + } + } + + /// + /// Removes any leftover purge-test certificates from CurrentUser\My. + /// Matches our unique Subject OU discriminator to avoid touching unrelated certs. + /// Best-effort, no-throw. + /// + private static void RemoveAllPurgeTestArtifacts() + { + if (!IsWindows) + { + return; + } + + try + { + using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); + store.Open(OpenFlags.ReadWrite); + + X509Certificate2[] snapshot; + try + { + snapshot = new X509Certificate2[store.Certificates.Count]; + store.Certificates.CopyTo(snapshot, 0); + } + catch + { + snapshot = store.Certificates.Cast().ToArray(); + } + + foreach (var c in snapshot) + { + try + { + string subject = c.Subject ?? string.Empty; + if (subject.IndexOf(TestSubjectDiscriminatorPrefix, StringComparison.Ordinal) >= 0) + { + try + { store.Remove(c); } + catch { /* best-effort */ } + } + } + finally + { + c.Dispose(); + } + } + } + catch + { + // best-effort + } + } + } +} From 33eba2034ffbdf43c8ea23078de4e549ca56f94c Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Wed, 27 May 2026 18:17:17 -0400 Subject: [PATCH 7/7] Address Copilot review: thumbprint OrdinalIgnoreCase, null guards - RemoveByThumbprints: use HashSet(OrdinalIgnoreCase) for thumbprint lookup (defensive; .NET's Thumbprint getter always returns uppercase, but normalize anyway). - PurgeManagedIdentityCertificates: skip null entries in the snapshot consuming loop (defensive against TOCTOU between Certificates.Count and enumeration). - WindowsPersistentCertificateCache.TryRead (testability overload): throw ArgumentNullException when the injected isOrphaned delegate is null. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../KeyProviders/WindowsCngKeyOperations.cs | 7 +++++++ .../V2/WindowsPersistentCertificateCache.cs | 9 ++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/client/Microsoft.Identity.Client/ManagedIdentity/KeyProviders/WindowsCngKeyOperations.cs b/src/client/Microsoft.Identity.Client/ManagedIdentity/KeyProviders/WindowsCngKeyOperations.cs index aaecc0b66c..2763781382 100644 --- a/src/client/Microsoft.Identity.Client/ManagedIdentity/KeyProviders/WindowsCngKeyOperations.cs +++ b/src/client/Microsoft.Identity.Client/ManagedIdentity/KeyProviders/WindowsCngKeyOperations.cs @@ -458,6 +458,13 @@ internal static void PurgeManagedIdentityCertificates(ILoggerAdapter logger) foreach (X509Certificate2 candidate in snapshot) { + if (candidate is null) + { + // Defensive: snapshot slot may be null if the store enumeration + // yielded fewer items than Certificates.Count reported (TOCTOU). + continue; + } + try { inspected++; diff --git a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs index 44ffe9578c..a9b96d65ff 100644 --- a/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs +++ b/src/client/Microsoft.Identity.Client/ManagedIdentity/V2/WindowsPersistentCertificateCache.cs @@ -46,6 +46,11 @@ public bool TryRead( { value = default; + if (isOrphaned is null) + { + throw new ArgumentNullException(nameof(isOrphaned)); + } + try { using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser); @@ -152,6 +157,8 @@ public bool TryRead( private static void RemoveByThumbprints(string alias, List thumbprints, ILoggerAdapter logger) { + var thumbprintSet = new HashSet(thumbprints, StringComparer.OrdinalIgnoreCase); + InterprocessLock.TryWithAliasLock( alias, timeout: TimeSpan.FromMilliseconds(300), @@ -169,7 +176,7 @@ private static void RemoveByThumbprints(string alias, List thumbprints, { try { - if (thumbprints.Contains(cert.Thumbprint)) + if (thumbprintSet.Contains(cert.Thumbprint)) { store.Remove(cert); removed++;