Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -185,5 +186,87 @@ public void RemoveBadCert(string cacheKey, ILoggerAdapter logger)
logger?.Verbose(() => $"[PersistentCert] Error removing from persistent cache: {ex.Message}");
}
}

/// <summary>
/// Returns <see langword="true"/> 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.
/// </summary>
internal static bool IsCertKeyOrphaned(X509Certificate2 cert, ILoggerAdapter logger)
{
if (cert is null)
return true;

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 RSA key (e.g. software CSP) — cannot perform KG container check; accept.
return false;
}
Comment thread
Robbie-Microsoft marked this conversation as resolved.

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;
}
}

/// <summary>
/// Returns <see langword="true"/> if the public key exported from <paramref name="containerKey"/>
/// matches the public key embedded in <paramref name="cert"/>.
/// A mismatch means the container holds different key material than when the cert was issued.
/// </summary>
/// <remarks>
/// Check 3 from the original proposal — comparing the CNG container's
/// <c>NCRYPT_LAST_MODIFIED_PROPERTY</c> against the cert's <c>NotBefore</c> — 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.
/// </remarks>
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;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Comment thread
Robbie-Microsoft marked this conversation as resolved.
Outdated
string alias,
out CertificateCacheValue value,
Comment thread
Robbie-Microsoft marked this conversation as resolved.
Outdated
ILoggerAdapter logger,
Func<X509Certificate2, ILoggerAdapter, bool> isOrphaned)
{
value = default;

Comment thread
Robbie-Microsoft marked this conversation as resolved.
Expand All @@ -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<string> orphanedThumbprints = null;

foreach (var candidate in items)
{
Expand Down Expand Up @@ -84,6 +96,17 @@ 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.
// 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<string>();
orphanedThumbprints.Add(candidate.Thumbprint);
continue;
}

if (candidate.NotAfter > bestNotAfter)
{
best?.Dispose();
Expand All @@ -98,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);
}
Comment thread
Robbie-Microsoft marked this conversation as resolved.

if (best != null)
{
// CN (GUID) → canonical client_id
Expand Down Expand Up @@ -131,6 +160,50 @@ public bool Read(string alias, out CertificateCacheValue value, ILoggerAdapter l
return false;
}

private static void RemoveByThumbprints(string alias, List<string> 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];
Comment thread
Robbie-Microsoft marked this conversation as resolved.
Outdated
store.Certificates.CopyTo(items, 0);
Comment thread
Robbie-Microsoft marked this conversation as resolved.
Outdated
}
catch (Exception ex)
{
logger.Verbose(() => "[PersistentCert] Orphan-removal snapshot via CopyTo failed; falling back to enumeration. Details: " + ex.Message);
items = store.Certificates.Cast<X509Certificate2>().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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -843,5 +843,136 @@ 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<ILoggerAdapter>();

// 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 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()
{
WindowsOnly();

// Arrange - cert created with key1; pass key1 as the container key
using var key1 = new RSACng(2048);

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(key1, 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 = new RSACng(2048);
using var key2 = new RSACng(2048);

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(key2, cert, null);

// Assert
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
}
}
Loading