diff --git a/src/client/Microsoft.Identity.Client/AuthScheme/PoP/MtlsPopAuthenticationOperation.cs b/src/client/Microsoft.Identity.Client/AuthScheme/PoP/MtlsPopAuthenticationOperation.cs index 4bd1913331..81a76cb6d4 100644 --- a/src/client/Microsoft.Identity.Client/AuthScheme/PoP/MtlsPopAuthenticationOperation.cs +++ b/src/client/Microsoft.Identity.Client/AuthScheme/PoP/MtlsPopAuthenticationOperation.cs @@ -19,7 +19,7 @@ internal class MtlsPopAuthenticationOperation : IAuthenticationOperation2 public MtlsPopAuthenticationOperation(X509Certificate2 mtlsCert) { _mtlsCert = mtlsCert; - KeyId = CoreHelpers.ComputeX5tS256KeyId(_mtlsCert); + KeyId = _mtlsCert.ComputeX5tS256KeyId(); } public int TelemetryTokenType => TelemetryTokenTypeConstants.MtlsPop; diff --git a/src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs b/src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs index 3abdfb0006..831faf2214 100644 --- a/src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs +++ b/src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs @@ -401,19 +401,19 @@ private bool ShouldUseCachedToken(MsalAccessTokenCacheItem cacheItem) if (requestCert != null && AuthenticationRequestParameters.IsMtlsPopRequested) { - string expectedKid = CoreHelpers.ComputeX5tS256KeyId(requestCert); + string expectedKid = requestCert.ComputeX5tS256KeyId(); - // If the certificate cannot produce a valid KeyId (SPKI-SHA256), expectedKid will be null or empty. + // If the certificate cannot produce a valid KeyId (the SHA256 cert thumbprint, equivalent to X5T#SHA256), expectedKid will be null or empty. // In this case, the cache will be bypassed, as we cannot safely match the cached token to the certificate. if (!string.Equals(cacheItem.KeyId, expectedKid, StringComparison.Ordinal)) { AuthenticationRequestParameters.RequestContext.Logger.Verbose(() => - "[ClientCredentialRequest] Cached token KeyId does not match request certificate (SPKI-SHA256 mismatch). Bypassing cache."); + "[ClientCredentialRequest] Cached token KeyId does not match request certificate (x5t#S256 DER mismatch). Bypassing cache."); return false; } AuthenticationRequestParameters.RequestContext.Logger.Verbose(() => - "[ClientCredentialRequest] Cached token KeyId matches request certificate (SPKI-SHA256). Using cached token."); + "[ClientCredentialRequest] Cached token KeyId matches request certificate (x5t#S256 DER). Using cached token."); } // 3) If the token's hash matches AccessTokenHashToRefresh, ignore it diff --git a/src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs b/src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs index 09a3b36a24..ead3058e5d 100644 --- a/src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs +++ b/src/client/Microsoft.Identity.Client/Utils/CoreHelpers.cs @@ -6,7 +6,6 @@ using System.Globalization; using System.Linq; using System.Security.Cryptography; -using System.Security.Cryptography.X509Certificates; using System.Text; using Microsoft.Identity.Client.Core; using Microsoft.Identity.Client.Internal; @@ -235,20 +234,5 @@ internal static string ComputeAccessTokenExtCacheKey(SortedList return Base64UrlHelpers.Encode(hashBytes); } } - - internal static string ComputeX5tS256KeyId(X509Certificate2 certificate) - { - // Extract the raw bytes of the certificate’s public key. - var publicKey = certificate.GetPublicKey(); - - // Compute the SHA-256 hash of the public key. - using (var sha256 = SHA256.Create()) - { - byte[] hash = sha256.ComputeHash(publicKey); - - // Return the hash encoded in Base64 URL format. - return Base64UrlHelpers.Encode(hash); - } - } } } diff --git a/src/client/Microsoft.Identity.Client/Utils/X509Certificate2Extensions.cs b/src/client/Microsoft.Identity.Client/Utils/X509Certificate2Extensions.cs new file mode 100644 index 0000000000..8aac1f51c4 --- /dev/null +++ b/src/client/Microsoft.Identity.Client/Utils/X509Certificate2Extensions.cs @@ -0,0 +1,24 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; + +namespace Microsoft.Identity.Client.Utils +{ + internal static class X509Certificate2Extensions + { + /// + /// Computes the mTLS PoP cache KeyId (x5t#S256): base64url SHA-256 of the DER-encoded + /// certificate (RFC 8705), matching what ESTS/MSS bind the token to. + /// + internal static string ComputeX5tS256KeyId(this X509Certificate2 certificate) + { + using (var sha256 = SHA256.Create()) + { + byte[] hash = sha256.ComputeHash(certificate.RawData); + return Base64UrlHelpers.Encode(hash); + } + } + } +} diff --git a/tests/Microsoft.Identity.Test.Common/TestCommon.cs b/tests/Microsoft.Identity.Test.Common/TestCommon.cs index 88965e04d4..1195d17c2c 100644 --- a/tests/Microsoft.Identity.Test.Common/TestCommon.cs +++ b/tests/Microsoft.Identity.Test.Common/TestCommon.cs @@ -194,7 +194,8 @@ internal static MsalAccessTokenCacheItem WithRefreshOn(this MsalAccessTokenCache atItem.KeyId, refreshOn, atItem.TokenType, - atItem.OboCacheKey); + atItem.OboCacheKey, + atItem.AdditionalCacheKeyComponents); return newAtItem; } diff --git a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ImdsV2Tests.cs b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ImdsV2Tests.cs index af4bc5048c..e99e9e8f2e 100644 --- a/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ImdsV2Tests.cs +++ b/tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/ImdsV2Tests.cs @@ -21,6 +21,7 @@ using Microsoft.Identity.Client.ManagedIdentity.V2; using Microsoft.Identity.Client.PlatformsCommon.Interfaces; using Microsoft.Identity.Client.PlatformsCommon.Shared; +using Microsoft.Identity.Test.Common; using Microsoft.Identity.Test.Common.Core.Helpers; using Microsoft.Identity.Test.Common.Core.Mocks; using Microsoft.Identity.Test.Unit.Helpers; @@ -2686,6 +2687,105 @@ public async Task mTLSPop_CertAtLeast24h_IsCached_ReusedOnSecondAcquire( Assert.AreEqual(first.BindingCertificate.Thumbprint, second.BindingCertificate.Thumbprint, $"[{label}] cached cert must be reused."); } } + + /// + /// Encodes the proactive-refresh + cert-rotation trace for the pure MI mTLS PoP flow. + /// + /// A cached token is served (bound to cert 1) and, because it is past RefreshOn, a background + /// proactive refresh runs and mints a NEW binding certificate (cert 2 — same CSR key, new DER; + /// short-lived certs are never cached, so every acquisition re-mints). + /// + /// Invariant under test: the SERVED (cache-hit) result stays a consistent pair — it carries + /// cert 1, the certificate its token is bound to — and is NOT corrupted by the cert 2 the + /// background refresh is minting. cert 2 becomes visible only on the NEXT request. In other + /// words, the binding certificate MSAL hands back is pinned to the served token; a background + /// rotation never leaks into the in-flight result. This demonstrates the proactive-refresh + /// race is not a source of the certificate/assertion mismatch. + /// + [TestMethod] + public async Task mTLSPop_ProactiveRefresh_RotatesCert_ServedResultStaysConsistentAsync() + { + using (new EnvVariableContext()) + using (var httpManager = new MockHttpManager()) + { + ManagedIdentityClient.ResetSourceForTest(); + SetEnvironmentVariables(ManagedIdentitySource.Imds, TestConstants.ImdsEndpoint); + + // cert 1 and cert 2 share the CSR key but differ in DER; both < 24h, so neither is cached + // and every acquisition re-mints a fresh cert (this is what lets the proactive refresh rotate). + var cert1Raw = CreateRawCertForCsrKeyWithCnDc( + Constants.ManagedIdentityDefaultClientId, TestConstants.TenantId, DateTimeOffset.UtcNow.AddHours(23)); + var cert2Raw = CreateRawCertForCsrKeyWithCnDc( + Constants.ManagedIdentityDefaultClientId, TestConstants.TenantId, DateTimeOffset.UtcNow.AddHours(23).AddMinutes(5)); + + var mi = (await CreateManagedIdentityAsync( + httpManager, + managedIdentityKeyType: ManagedIdentityKeyType.KeyGuard).ConfigureAwait(false)) as ManagedIdentityApplication; + + // --- Initial mint: token 1 bound to cert 1 --- + AddMocksToGetEntraToken(httpManager, certificateRequestCertificate: cert1Raw); + + var first = await mi.AcquireTokenForManagedIdentity(ManagedIdentityTests.Resource) + .WithMtlsProofOfPossession() + .WithAttestationSupport() + .ExecuteAsync().ConfigureAwait(false); + + Assert.AreEqual(TokenSource.IdentityProvider, first.AuthenticationResultMetadata.TokenSource); + string thumb1 = first.BindingCertificate.Thumbprint; + + // Mark the cached token as needing a background (proactive) refresh. + TestCommon.UpdateATWithRefreshOn(mi.AppTokenCacheInternal.Accessor); + + // The background refresh will mint cert 2 + token 2. + AddMocksToGetEntraToken(httpManager, certificateRequestCertificate: cert2Raw); + + var cacheAccess = mi.AppTokenCacheInternal.RecordAccess(); + + // --- The traced request: cache hit serves cert 1 + token 1, and fires the proactive + // refresh that rotates to cert 2 in the background. --- + var served = await mi.AcquireTokenForManagedIdentity(ManagedIdentityTests.Resource) + .WithMtlsProofOfPossession() + .WithAttestationSupport() + .ExecuteAsync().ConfigureAwait(false); + + // Capture the exact object reference handed back, so we can prove later that a background + // rotation does not repoint it. + X509Certificate2 servedCert = served.BindingCertificate; + + // INVARIANT: the served result is the cached cert-1 pair, captured before the background + // refresh completes — NOT the cert 2 the refresh is minting. + Assert.AreEqual(TokenSource.Cache, served.AuthenticationResultMetadata.TokenSource); + Assert.AreEqual(CacheRefreshReason.ProactivelyRefreshed, served.AuthenticationResultMetadata.CacheRefreshReason); + Assert.AreEqual(thumb1, served.BindingCertificate.Thumbprint, + "The served (cache-hit) result must carry cert 1 - the cert its token is bound to - NOT the " + + "cert 2 minted by the background proactive refresh. A background rotation must never leak into the in-flight result."); + + // Let the background refresh finish (consumes the cert-2 mocks, rotates + // RuntimeMtlsBindingCertificate to cert 2, and writes token 2 to the cache). + Assert.IsTrue(TestCommon.YieldTillSatisfied(() => httpManager.QueueSize == 0), "Background proactive refresh did not run."); + cacheAccess.WaitTo_AssertAcessCounts(1, 1); + + // REFERENCE INVARIANT: even though the background refresh has now rotated the runtime binding + // certificate to cert 2, the previously-returned result must STILL reference cert 1. + // AuthenticationResult.BindingCertificate is a plain snapshot property, not a live alias to the + // rotating cert - so the reference handed to the caller never changes underneath them. + Assert.AreEqual(thumb1, served.BindingCertificate.Thumbprint, + "After the background rotation completed, the already-returned result's BindingCertificate must STILL be cert 1."); + Assert.IsTrue(ReferenceEquals(servedCert, served.BindingCertificate), + "BindingCertificate must remain the exact same object reference - a background rotation must not repoint it to cert 2."); + + // --- Next request: the cache now holds token 2 (cert 2). The rotation is visible only here. --- + var next = await mi.AcquireTokenForManagedIdentity(ManagedIdentityTests.Resource) + .WithMtlsProofOfPossession() + .WithAttestationSupport() + .ExecuteAsync().ConfigureAwait(false); + + Assert.AreEqual(TokenSource.Cache, next.AuthenticationResultMetadata.TokenSource); + Assert.AreNotEqual(thumb1, next.BindingCertificate.Thumbprint, + "After the background refresh completes, the next request serves the rotated cert 2 - " + + "the rotation affects only subsequent requests, never the one that triggered it."); + } + } #endregion #region Cert cache test helpers diff --git a/tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs b/tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs index 2827030acc..cdf66805fa 100644 --- a/tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs @@ -301,7 +301,7 @@ public void Constructor_ValidCertificate() { var scheme = new MtlsPopAuthenticationOperation(s_testCertificate); - // Compute the expected KeyId using SHA-256 on the public key + // Compute the expected KeyId using SHA-256 over the certificate DER (x5t#S256) var expectedKeyId = ComputeExpectedKeyId(s_testCertificate); Assert.AreEqual(expectedKeyId, scheme.KeyId); @@ -332,13 +332,11 @@ public async Task SchemeSetsCertAsync() private static string ComputeExpectedKeyId(X509Certificate2 certificate) { - // Get the raw public key bytes - var publicKey = certificate.GetPublicKey(); - - // Compute the SHA-256 hash of the public key + // Compute the SHA-256 hash of the full DER-encoded certificate (x5t#S256, RFC 8705), + // matching what ESTS/MSS bind the mTLS PoP token to. using (var sha256 = SHA256.Create()) { - byte[] hash = sha256.ComputeHash(publicKey); + byte[] hash = sha256.ComputeHash(certificate.RawData); return Base64UrlHelpers.Encode(hash); } } @@ -530,6 +528,123 @@ public async Task AcquireMtlsPopTokenForClientWithTenantIdCertChecks_Async() } } + /// + /// Repro for AADSTS500181 (CertificateValidationFailedTlsCertMismatch) in the two-leg mTLS PoP flow. + /// + /// MSAL keys the mTLS-PoP token cache with , which hashes + /// only the certificate's PUBLIC KEY. ESTS/MSS, however, bind and validate the token against the DER of + /// the presented certificate (x5t#S256, RFC 8705). + /// + /// When the binding certificate is renewed with the SAME key but a new DER (serial/validity) — exactly + /// what IMDS/KeyGuard produces when it reissues the cert over the same non-exportable key — the + /// public-key-hash cache key is unchanged. MSAL therefore cannot tell the old cert from the renewed one + /// and serves the STALE token (bound to the old cert's DER) while the renewed cert is on the wire, + /// producing the certificate/assertion mismatch. + /// + /// Correct behavior: a same-key renewal is a DIFFERENT certificate and MUST cause a cache miss, re-minting + /// a token bound to the renewed cert. This test FAILS on current code (second acquisition returns + /// TokenSource.Cache with the cert-A-bound token) and PASSES once the cache key is derived from the DER. + /// + [TestMethod] + public async Task MtlsPop_SameKeyCertRenewal_MustNotServeStaleCachedTokenAsync() + { + const string region = "eastus"; + + // Two certificates that share ONE RSA key pair but differ in DER (validity + serial). + // This models a same-key certificate renewal. + using RSA sharedKey = RSA.Create(2048); + + X509Certificate2 certA = new CertificateRequest( + "CN=MtlsPopSameKeyRenewal", sharedKey, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1) + .CreateSelfSigned(DateTimeOffset.UtcNow.AddDays(-2), DateTimeOffset.UtcNow.AddDays(30)); + + X509Certificate2 certB = new CertificateRequest( + "CN=MtlsPopSameKeyRenewal", sharedKey, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1) + .CreateSelfSigned(DateTimeOffset.UtcNow.AddDays(-1), DateTimeOffset.UtcNow.AddDays(60)); + + try + { + // Precondition 1: identical public key (same key pair). + CollectionAssert.AreEqual( + certA.GetPublicKey(), certB.GetPublicKey(), + "Test setup invalid: the two certificates must share the same public key."); + + // Precondition 2: different certificates (different DER => different x5t#S256 => different thumbprint). + Assert.AreNotEqual( + certA.Thumbprint, certB.Thumbprint, + "Test setup invalid: the two certificates must be different (different DER)."); + + // The binding certificate presented on the wire. Starts as cert A, then "renews" to cert B. + X509Certificate2 currentCert = certA; + + using (var envContext = new EnvVariableContext()) + { + Environment.SetEnvironmentVariable("REGION_NAME", region); + + using (var httpManager = new MockHttpManager()) + { + // Distinct tokens per mint so we can prove exactly which one is served. + httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage( + token: "token_bound_to_certA", tokenType: "mtls_pop"); + + var app = ConfidentialClientApplicationBuilder.Create(TestConstants.ClientId) + .WithExperimentalFeatures() + .WithCertificate(_ => Task.FromResult(currentCert), new CertificateOptions()) + .WithAuthority("https://login.microsoftonline.com/123456-1234-2345-1234561234") + .WithAzureRegion(ConfidentialClientApplication.AttemptRegionDiscovery) + .WithHttpManager(httpManager) + .BuildConcrete(); + + // ── Request 1: bind a PoP token to cert A. Hits the IdP and caches. ── + AuthenticationResult first = await app.AcquireTokenForClient(TestConstants.s_scope) + .WithMtlsProofOfPossession() + .ExecuteAsync() + .ConfigureAwait(false); + + Assert.AreEqual(TokenSource.IdentityProvider, first.AuthenticationResultMetadata.TokenSource); + Assert.AreEqual("token_bound_to_certA", first.AccessToken); + Assert.AreEqual(certA.Thumbprint, first.BindingCertificate.Thumbprint); + + // ── Same-key renewal: the platform reissues the binding cert over the same key. ── + currentCert = certB; + + // A fresh IdP response is available for the (expected) cache miss on the renewed cert. + httpManager.AddMockHandlerSuccessfulClientCredentialTokenResponseMessage( + token: "token_bound_to_certB", tokenType: "mtls_pop"); + + // ── Request 2: cert B is now on the wire. ── + AuthenticationResult second = await app.AcquireTokenForClient(TestConstants.s_scope) + .WithMtlsProofOfPossession() + .ExecuteAsync() + .ConfigureAwait(false); + + // ─────────────────────────────── The bug ─────────────────────────────── + // Cert B has a different DER than cert A, so the cert-A-bound cached token is invalid for + // cert B. Correct behavior is a cache MISS and a re-mint bound to cert B. On current code, + // ComputeX5tS256KeyId hashes the public key (identical across the renewal), so the lookup + // HITS the cert-A-bound token and returns it from cache while cert B is on the wire — + // reproducing AADSTS500181. + Assert.AreEqual( + TokenSource.IdentityProvider, + second.AuthenticationResultMetadata.TokenSource, + "A same-key certificate renewal (same public key, different DER) MUST invalidate the " + + "cached mTLS-PoP token. Serving the stale cert-A-bound token while cert B is on the wire " + + "is the root cause of AADSTS500181 (CertificateValidationFailedTlsCertMismatch)."); + + Assert.AreEqual( + "token_bound_to_certB", + second.AccessToken, + "MSAL served the cert-A-bound token for a request presenting cert B."); + } + } + } + finally + { + certA.Dispose(); + certB.Dispose(); + } + } + [TestMethod] public async Task MtlsPop_KnownRegionAsync() {