From 9701366306cee16fba6410430c18d844f6576449 Mon Sep 17 00:00:00 2001 From: avdunn Date: Mon, 23 Jun 2025 14:28:19 -0700 Subject: [PATCH 1/4] Add issuer validation when making call to OIDC endpoint --- .../Instance/Oidc/OidcMetadata.cs | 3 + .../Instance/Oidc/OidcRetrieverWithCache.cs | 61 ++++++++++++++++++- .../HeadlessTests/CiamIntegrationTests.cs | 25 ++++++++ .../InstanceTests/GenericAuthorityTests.cs | 38 ++++++++++++ 4 files changed, 125 insertions(+), 2 deletions(-) diff --git a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcMetadata.cs b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcMetadata.cs index d2aaba5530..f15dd9a188 100644 --- a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcMetadata.cs +++ b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcMetadata.cs @@ -19,6 +19,9 @@ namespace Microsoft.Identity.Client.Instance.Oidc [Preserve(AllMembers = true)] internal class OidcMetadata { + [JsonProperty("issuer")] + public string Issuer { get; set; } + [JsonProperty("token_endpoint")] public string TokenEndpoint { get; set; } diff --git a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs index 8875c1194f..a941f79f7c 100644 --- a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs +++ b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Concurrent; +using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.Identity.Client.Core; @@ -27,7 +28,7 @@ public static async Task GetOidcAsync( } await s_lockOidcRetrieval.WaitAsync().ConfigureAwait(false); - + Uri oidcMetadataEndpoint = null; try { @@ -44,9 +45,13 @@ public static async Task GetOidcAsync( builder.Path = existingPath.TrimEnd('/') + "/" + Constants.WellKnownOpenIdConfigurationPath; oidcMetadataEndpoint = builder.Uri; - var client = new OAuth2Client(requestContext.Logger, requestContext.ServiceBundle.HttpManager, null); + var client = new OAuth2Client(requestContext.Logger, requestContext.ServiceBundle.HttpManager, null); configuration = await client.DiscoverOidcMetadataAsync(oidcMetadataEndpoint, requestContext).ConfigureAwait(false); + // Validate the issuer before caching the configuration + requestContext.Logger.Verbose(() => $"[OIDC Discovery] Validating issuer: {configuration.Issuer} against authority: {authority}"); + ValidateIssuer(new Uri(authority), configuration.Issuer); + s_cache[authority] = configuration; requestContext.Logger.Verbose(() => $"[OIDC Discovery] OIDC discovery retrieved metadata from the network for {authority}"); @@ -70,6 +75,58 @@ public static async Task GetOidcAsync( } } + /// + /// Validates that the issuer in the OIDC metadata matches the authority. + /// + /// The authority URL. + /// The issuer from the OIDC metadata - the single source of truth. + /// Thrown when issuer validation fails. + private static void ValidateIssuer(Uri authority, string issuer) + { + + // Primary validation: check if authority begins with the issuer string (exact string comparison) + if (authority.AbsoluteUri.StartsWith(issuer, StringComparison.OrdinalIgnoreCase)) + { + return; + } + + // Extract tenant for CIAM-like scenarios + string tenant = null; + try + { + tenant = AuthorityInfo.GetFirstPathSegment(authority); + } + catch (InvalidOperationException) + { + // If no path segments exist, try to extract from hostname (first part) + var hostParts = authority.Host.Split('.'); + tenant = hostParts.Length > 0 ? hostParts[0] : null; + } + + // If tenant extraction failed or returned empty, validation fails + if (!string.IsNullOrEmpty(tenant)) + { + // Create a collection of valid CIAM issuer patterns for the tenant + string[] validCiamPatterns = + { + $"https://{tenant}{Constants.CiamAuthorityHostSuffix}", + $"https://{tenant}{Constants.CiamAuthorityHostSuffix}/{tenant}", + $"https://{tenant}{Constants.CiamAuthorityHostSuffix}/{tenant}/v2.0" + }; + + // Check if the issuer matches any of the valid patterns + if (validCiamPatterns.Any(pattern => string.Equals(issuer, pattern, StringComparison.OrdinalIgnoreCase))) + { + return; + } + } + + // Validation failed + throw new MsalServiceException( + "authority_validation_failed", + $"Issuer validation failed for {authority}. Issuer from OIDC endpoint does not match any expected pattern: {issuer}"); + } + // For testing purposes only public static void ResetCacheForTest() { diff --git a/tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs b/tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs index 02076ed760..3843023f0e 100644 --- a/tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs +++ b/tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs @@ -213,6 +213,31 @@ public async Task OBOCiam_CustomDomain_ReturnsValidTokens() Assert.AreEqual(TokenSource.Cache, resultObo.AuthenticationResultMetadata.TokenSource); } + [TestMethod] + public async Task WithOidcAuthority_ValidatesIssuerSuccessfully() + { + //Get lab details + var labResponse = await LabUserHelper.GetLabUserDataAsync(new UserQuery() + { + FederationProvider = FederationProvider.CIAMCUD, + SignInAudience = SignInAudience.AzureAdMyOrg + }).ConfigureAwait(false); + + //Test with different CIAM authority formats + string[] authorities = + { + string.Format("https://{0}.ciamlogin.com/", labResponse.User.LabName), + string.Format("https://{0}.ciamlogin.com/{1}.onmicrosoft.com", labResponse.User.LabName, labResponse.User.LabName), + string.Format("https://{0}.ciamlogin.com/{1}", labResponse.User.LabName, labResponse.Lab.TenantId), + string.Format("https://login.msidlabsciam.com/{0}/v2.0/", labResponse.Lab.TenantId) + }; + + foreach (var authority in authorities) + { + await RunCiamCCATest(authority, labResponse.App.AppId).ConfigureAwait(false); + } + } + private string GetCiamSecret() { KeyVaultSecretsProvider provider = new KeyVaultSecretsProvider(); diff --git a/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs b/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs index 98819354d5..8dacdda6ec 100644 --- a/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs @@ -333,6 +333,44 @@ public async Task BadOidcResponse_ThrowsException_Async(string badOidcResponseTy } } + [TestMethod] + public async Task OidcIssuerValidation_ThrowsForNonMatchingIssuer_Async() + { + using (var httpManager = new MockHttpManager()) + { + string authority = "https://demo.duendesoftware.com"; + IConfidentialClientApplication app = ConfidentialClientApplicationBuilder + .Create(TestConstants.ClientId) + .WithHttpManager(httpManager) + .WithOidcAuthority(authority) + .WithClientSecret(TestConstants.ClientSecret) + .Build(); + + // Create OIDC document with non-matching issuer + string validOidcDocumentWithWrongIssuer = TestConstants.GenericOidcResponse.Replace( + "\"issuer\":\"https://demo.duendesoftware.com\"", + "\"issuer\":\"https://wrong.issuer.com\""); + + // Mock OIDC endpoint response + httpManager.AddMockHandler(new MockHttpMessageHandler + { + ExpectedMethod = HttpMethod.Get, + ExpectedUrl = authority + "/.well-known/openid-configuration", + ResponseMessage = MockHelpers.CreateSuccessResponseMessage(validOidcDocumentWithWrongIssuer) + }); + + var ex = await AssertException.TaskThrowsAsync(() => + app.AcquireTokenForClient(new[] { "api" }).ExecuteAsync() + ).ConfigureAwait(false); + + Assert.AreEqual("authority_validation_failed", ex.ErrorCode); + Assert.IsTrue(ex.Message.Contains("Issuer validation failed"), + $"Expected error message to contain 'Issuer validation failed', but was: {ex.Message}"); + Assert.IsTrue(ex.Message.Contains("https://wrong.issuer.com"), + "Error message should include the non-matching issuer value"); + } + } + private static MockHttpMessageHandler CreateTokenResponseHttpHandler( string tokenEndpoint, string scopesInRequest, From aa69b45bdf04df811c6d0cc9c1e5ba297e7196f6 Mon Sep 17 00:00:00 2001 From: avdunn Date: Tue, 8 Jul 2025 14:30:14 -0700 Subject: [PATCH 2/4] Address PR feedback --- .../Instance/Oidc/OidcRetrieverWithCache.cs | 12 ++++++++---- .../HeadlessTests/CiamIntegrationTests.cs | 19 ++++++++----------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs index a941f79f7c..95266b34f5 100644 --- a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs +++ b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs @@ -83,9 +83,12 @@ public static async Task GetOidcAsync( /// Thrown when issuer validation fails. private static void ValidateIssuer(Uri authority, string issuer) { + // Normalize both URLs to handle trailing slash differences + string normalizedAuthority = authority.AbsoluteUri.TrimEnd('/'); + string normalizedIssuer = issuer?.TrimEnd('/'); - // Primary validation: check if authority begins with the issuer string (exact string comparison) - if (authority.AbsoluteUri.StartsWith(issuer, StringComparison.OrdinalIgnoreCase)) + // Primary validation: check if normalized authority equals normalized issuer (exact string comparison) + if (string.Equals(normalizedAuthority, normalizedIssuer, StringComparison.OrdinalIgnoreCase)) { return; } @@ -114,8 +117,9 @@ private static void ValidateIssuer(Uri authority, string issuer) $"https://{tenant}{Constants.CiamAuthorityHostSuffix}/{tenant}/v2.0" }; - // Check if the issuer matches any of the valid patterns - if (validCiamPatterns.Any(pattern => string.Equals(issuer, pattern, StringComparison.OrdinalIgnoreCase))) + // Normalize and check if the issuer matches any of the valid patterns + if (validCiamPatterns.Any(pattern => + string.Equals(normalizedIssuer, pattern.TrimEnd('/'), StringComparison.OrdinalIgnoreCase))) { return; } diff --git a/tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs b/tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs index 3843023f0e..c4f92c30f3 100644 --- a/tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs +++ b/tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs @@ -109,10 +109,10 @@ public async Task ClientCredentialCiam_WithClientCredentials_ReturnsValidTokens( //Ciam CUD authority = "https://login.msidlabsciam.com/fe362aec-5d43-45d1-b730-9755e60dc3b9/v2.0/"; string ciamClient = "b244c86f-ed88-45bf-abda-6b37aa482c79"; - await RunCiamCCATest(authority, ciamClient).ConfigureAwait(false); + await RunCiamCCATest(authority, ciamClient, true).ConfigureAwait(false); } - private async Task RunCiamCCATest(string authority, string appId) + private async Task RunCiamCCATest(string authority, string appId, bool useOidcAuthority = false) { //Acquire tokens var msalConfidentialClientBuilder = ConfidentialClientApplicationBuilder @@ -120,16 +120,15 @@ private async Task RunCiamCCATest(string authority, string appId) .WithCertificate(CertificateHelper.FindCertificateByName(TestConstants.AutomationTestCertName)) .WithExperimentalFeatures(); - if (authority.Contains(Constants.CiamAuthorityHostSuffix)) + if (useOidcAuthority) { - msalConfidentialClientBuilder.WithAuthority(authority, false); + msalConfidentialClientBuilder.WithOidcAuthority(authority); } else { - msalConfidentialClientBuilder.WithOidcAuthority(authority); + msalConfidentialClientBuilder.WithAuthority(authority); } - var msalConfidentialClient = msalConfidentialClientBuilder.Build(); var result = await msalConfidentialClient @@ -223,18 +222,16 @@ public async Task WithOidcAuthority_ValidatesIssuerSuccessfully() SignInAudience = SignInAudience.AzureAdMyOrg }).ConfigureAwait(false); - //Test with different CIAM authority formats + //Test with standard and CUD CIAM authorities string[] authorities = { - string.Format("https://{0}.ciamlogin.com/", labResponse.User.LabName), - string.Format("https://{0}.ciamlogin.com/{1}.onmicrosoft.com", labResponse.User.LabName, labResponse.User.LabName), - string.Format("https://{0}.ciamlogin.com/{1}", labResponse.User.LabName, labResponse.Lab.TenantId), + string.Format("https://{0}.ciamlogin.com/{1}/v2.0/", labResponse.Lab.TenantId, labResponse.Lab.TenantId), string.Format("https://login.msidlabsciam.com/{0}/v2.0/", labResponse.Lab.TenantId) }; foreach (var authority in authorities) { - await RunCiamCCATest(authority, labResponse.App.AppId).ConfigureAwait(false); + await RunCiamCCATest(authority, labResponse.App.AppId, true).ConfigureAwait(false); } } From f40bc8c1e1e3d1acb7185775e8465271b03f09a1 Mon Sep 17 00:00:00 2001 From: avdunn Date: Tue, 8 Jul 2025 16:56:45 -0700 Subject: [PATCH 3/4] PR feedback --- .../Instance/Oidc/OidcRetrieverWithCache.cs | 8 ++++---- .../MsalErrorMessage.cs | 2 ++ .../InstanceTests/GenericAuthorityTests.cs | 18 ++++++++++-------- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs index 95266b34f5..818fd930a4 100644 --- a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs +++ b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs @@ -87,8 +87,8 @@ private static void ValidateIssuer(Uri authority, string issuer) string normalizedAuthority = authority.AbsoluteUri.TrimEnd('/'); string normalizedIssuer = issuer?.TrimEnd('/'); - // Primary validation: check if normalized authority equals normalized issuer (exact string comparison) - if (string.Equals(normalizedAuthority, normalizedIssuer, StringComparison.OrdinalIgnoreCase)) + // Primary validation: check if normalized authority starts with normalized issuer (case-insensitive comparison) + if (normalizedAuthority.StartsWith(normalizedIssuer, StringComparison.OrdinalIgnoreCase)) { return; } @@ -127,8 +127,8 @@ private static void ValidateIssuer(Uri authority, string issuer) // Validation failed throw new MsalServiceException( - "authority_validation_failed", - $"Issuer validation failed for {authority}. Issuer from OIDC endpoint does not match any expected pattern: {issuer}"); + MsalError.AuthorityValidationFailed, + string.Format(MsalErrorMessage.IssuerValidationFailed, authority, issuer)); } // For testing purposes only diff --git a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs index b1895118aa..2465ef693e 100644 --- a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs +++ b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs @@ -149,6 +149,8 @@ public static string iOSBrokerKeySaveFailed(string keyChainResult) public const string AuthorityValidationFailed = "Authority validation failed. "; + public const string IssuerValidationFailed = "Issuer validation failed for {0}. Issuer from OIDC endpoint does not match any expected pattern: {1}"; + public const string AuthorityUriInsecure = "The authority must use HTTPS scheme. "; public const string AuthorityUriInvalidPath = diff --git a/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs b/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs index 8dacdda6ec..5efda46c12 100644 --- a/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs @@ -339,6 +339,8 @@ public async Task OidcIssuerValidation_ThrowsForNonMatchingIssuer_Async() using (var httpManager = new MockHttpManager()) { string authority = "https://demo.duendesoftware.com"; + string wrongIssuer = "https://wrong.issuer.com"; + IConfidentialClientApplication app = ConfidentialClientApplicationBuilder .Create(TestConstants.ClientId) .WithHttpManager(httpManager) @@ -348,14 +350,14 @@ public async Task OidcIssuerValidation_ThrowsForNonMatchingIssuer_Async() // Create OIDC document with non-matching issuer string validOidcDocumentWithWrongIssuer = TestConstants.GenericOidcResponse.Replace( - "\"issuer\":\"https://demo.duendesoftware.com\"", - "\"issuer\":\"https://wrong.issuer.com\""); + $"\"issuer\":\"{authority}\"", + $"\"issuer\":\"{wrongIssuer}\""); // Mock OIDC endpoint response httpManager.AddMockHandler(new MockHttpMessageHandler { ExpectedMethod = HttpMethod.Get, - ExpectedUrl = authority + "/.well-known/openid-configuration", + ExpectedUrl = authority + "/" + Constants.WellKnownOpenIdConfigurationPath, ResponseMessage = MockHelpers.CreateSuccessResponseMessage(validOidcDocumentWithWrongIssuer) }); @@ -363,11 +365,11 @@ public async Task OidcIssuerValidation_ThrowsForNonMatchingIssuer_Async() app.AcquireTokenForClient(new[] { "api" }).ExecuteAsync() ).ConfigureAwait(false); - Assert.AreEqual("authority_validation_failed", ex.ErrorCode); - Assert.IsTrue(ex.Message.Contains("Issuer validation failed"), - $"Expected error message to contain 'Issuer validation failed', but was: {ex.Message}"); - Assert.IsTrue(ex.Message.Contains("https://wrong.issuer.com"), - "Error message should include the non-matching issuer value"); + string expectedErrorMessage = string.Format(MsalErrorMessage.IssuerValidationFailed, app.Authority, wrongIssuer); + + Assert.AreEqual(MsalError.AuthorityValidationFailed, ex.ErrorCode); + Assert.AreEqual(expectedErrorMessage, ex.Message, + "Error message should match the expected error message."); } } From 049d80025731b906f2f4989d3999cf0a850fea24 Mon Sep 17 00:00:00 2001 From: avdunn Date: Tue, 15 Jul 2025 14:19:44 -0700 Subject: [PATCH 4/4] PR feedback and minor error message changes --- .../Instance/Oidc/OidcRetrieverWithCache.cs | 3 ++- src/client/Microsoft.Identity.Client/MsalErrorMessage.cs | 2 +- .../CoreTests/InstanceTests/GenericAuthorityTests.cs | 7 +++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs index 818fd930a4..2e774dcddf 100644 --- a/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs +++ b/src/client/Microsoft.Identity.Client/Instance/Oidc/OidcRetrieverWithCache.cs @@ -93,7 +93,8 @@ private static void ValidateIssuer(Uri authority, string issuer) return; } - // Extract tenant for CIAM-like scenarios + // Extract tenant for CIAM scenarios. In a CIAM scenario the issuer is expected to have "{tenant}.ciamlogin.com" + // as the host, even when using a custom domain. string tenant = null; try { diff --git a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs index 2465ef693e..f1318b9a0b 100644 --- a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs +++ b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs @@ -149,7 +149,7 @@ public static string iOSBrokerKeySaveFailed(string keyChainResult) public const string AuthorityValidationFailed = "Authority validation failed. "; - public const string IssuerValidationFailed = "Issuer validation failed for {0}. Issuer from OIDC endpoint does not match any expected pattern: {1}"; + public const string IssuerValidationFailed = "Issuer validation failed for authority: {0} . Issuer from OIDC endpoint does not match any expected pattern: {1} . "; public const string AuthorityUriInsecure = "The authority must use HTTPS scheme. "; diff --git a/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs b/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs index 5efda46c12..9f138860d4 100644 --- a/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/GenericAuthorityTests.cs @@ -338,26 +338,25 @@ public async Task OidcIssuerValidation_ThrowsForNonMatchingIssuer_Async() { using (var httpManager = new MockHttpManager()) { - string authority = "https://demo.duendesoftware.com"; string wrongIssuer = "https://wrong.issuer.com"; IConfidentialClientApplication app = ConfidentialClientApplicationBuilder .Create(TestConstants.ClientId) .WithHttpManager(httpManager) - .WithOidcAuthority(authority) + .WithOidcAuthority(TestConstants.GenericAuthority) .WithClientSecret(TestConstants.ClientSecret) .Build(); // Create OIDC document with non-matching issuer string validOidcDocumentWithWrongIssuer = TestConstants.GenericOidcResponse.Replace( - $"\"issuer\":\"{authority}\"", + $"\"issuer\":\"{TestConstants.GenericAuthority}\"", $"\"issuer\":\"{wrongIssuer}\""); // Mock OIDC endpoint response httpManager.AddMockHandler(new MockHttpMessageHandler { ExpectedMethod = HttpMethod.Get, - ExpectedUrl = authority + "/" + Constants.WellKnownOpenIdConfigurationPath, + ExpectedUrl = $"{TestConstants.GenericAuthority}/{Constants.WellKnownOpenIdConfigurationPath}", ResponseMessage = MockHelpers.CreateSuccessResponseMessage(validOidcDocumentWithWrongIssuer) });