From 4d3e5b95b127cf5a55272d87a2f9fb42550331b3 Mon Sep 17 00:00:00 2001 From: sruthikeerthi <73967733+sruke@users.noreply.github.com> Date: Tue, 18 Jun 2024 23:08:34 -0700 Subject: [PATCH] Revert PR#2597 (#2650) Co-authored-by: Sruthi Keerthi Rangavajhula (from Dev Box) --- .../AadIssuerValidator/AadIssuerValidator.cs | 24 +++++------------ .../AadTokenValidationParametersExtension.cs | 27 ++++++++----------- .../AadIssuerValidatorTests.cs | 24 ----------------- .../AadSigningKeyIssuerValidatorTests.cs | 15 ----------- 4 files changed, 17 insertions(+), 73 deletions(-) delete mode 100644 test/Microsoft.IdentityModel.Validators.Tests/AadIssuerValidatorTests.cs diff --git a/src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs b/src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs index 445671d134..323718cc84 100644 --- a/src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs +++ b/src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs @@ -13,6 +13,7 @@ using Microsoft.IdentityModel.Protocols; using Microsoft.IdentityModel.Protocols.OpenIdConnect; using Microsoft.IdentityModel.Tokens; +using static Microsoft.IdentityModel.Validators.AadIssuerValidator; namespace Microsoft.IdentityModel.Validators { @@ -382,31 +383,18 @@ private ConfigurationManager CreateConfigManager( } } - internal static bool IsValidIssuer(string validIssuerTemplate, string tenantId, string actualIssuer) + private static bool IsValidIssuer(string validIssuerTemplate, string tenantId, string actualIssuer) { - if (string.IsNullOrEmpty(validIssuerTemplate) || string.IsNullOrEmpty(actualIssuer) || string.IsNullOrEmpty(tenantId)) + if (string.IsNullOrEmpty(validIssuerTemplate)) return false; - ReadOnlySpan validIssuerTemplateSpan = validIssuerTemplate.AsSpan(); - ReadOnlySpan actualIssuerSpan = actualIssuer.AsSpan(); - int indexOfTenantIdTemplate = validIssuerTemplate.IndexOf(TenantIdTemplate, StringComparison.Ordinal); - - if (indexOfTenantIdTemplate >= 0 && actualIssuer.Length > indexOfTenantIdTemplate) + if (validIssuerTemplate.Contains(TenantIdTemplate)) { - // ensure the first part of the validIssuerTemplate matches the first part of actualIssuer - if (!validIssuerTemplateSpan.Slice(0, indexOfTenantIdTemplate).SequenceEqual(actualIssuerSpan.Slice(0, indexOfTenantIdTemplate))) - return false; - - // ensure that actualIssuer contains the tenantId from indexOfTenantIdTemplate for the length of tenantId.Length - if (!actualIssuerSpan.Slice(indexOfTenantIdTemplate, tenantId.Length).SequenceEqual(tenantId.AsSpan())) - return false; - - // ensure the second halves are equal - return validIssuerTemplateSpan.Slice(indexOfTenantIdTemplate + TenantIdTemplate.Length).SequenceEqual(actualIssuerSpan.Slice(indexOfTenantIdTemplate + tenantId.Length)); + return validIssuerTemplate.Replace(TenantIdTemplate, tenantId) == actualIssuer; } else { - return validIssuerTemplateSpan.SequenceEqual(actualIssuerSpan); + return validIssuerTemplate == actualIssuer; } } diff --git a/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs b/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs index 5335284168..d745bf124c 100644 --- a/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs +++ b/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs @@ -76,36 +76,31 @@ internal static bool ValidateIssuerSigningKey(SecurityKey securityKey, SecurityT #if NET6_0_OR_GREATER if (!string.IsNullOrEmpty(tokenIssuer) && !tokenIssuer.Contains(tenantIdFromToken, StringComparison.Ordinal)) throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogHelper.FormatInvariant(LogMessages.IDX40004, LogHelper.MarkAsNonPII(tokenIssuer), LogHelper.MarkAsNonPII(tenantIdFromToken)))); + + // creating an effectiveSigningKeyIssuer is required as signingKeyIssuer might contain {tenantid} + var effectiveSigningKeyIssuer = signingKeyIssuer.Replace(AadIssuerValidator.TenantIdTemplate, tenantIdFromToken, StringComparison.Ordinal); + var v2TokenIssuer = openIdConnectConfiguration.Issuer?.Replace(AadIssuerValidator.TenantIdTemplate, tenantIdFromToken, StringComparison.Ordinal); #else if (!string.IsNullOrEmpty(tokenIssuer) && !tokenIssuer.Contains(tenantIdFromToken)) throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogHelper.FormatInvariant(LogMessages.IDX40004, LogHelper.MarkAsNonPII(tokenIssuer), LogHelper.MarkAsNonPII(tenantIdFromToken)))); + + // creating an effectiveSigningKeyIssuer is required as signingKeyIssuer might contain {tenantid} + var effectiveSigningKeyIssuer = signingKeyIssuer.Replace(AadIssuerValidator.TenantIdTemplate, tenantIdFromToken); + var v2TokenIssuer = openIdConnectConfiguration.Issuer?.Replace(AadIssuerValidator.TenantIdTemplate, tenantIdFromToken); #endif - // comparing effectiveSigningKeyIssuer with v2TokenIssuer is required because of the following scenario: + + // comparing effectiveSigningKeyIssuer with v2TokenIssuer is required as well because of the following scenario: // 1. service trusts /common/v2.0 endpoint // 2. service receieves a v1 token that has issuer like sts.windows.net // 3. signing key issuers will never match sts.windows.net as v1 endpoint doesn't have issuers attached to keys // v2TokenIssuer is the representation of Token.Issuer (if it was a v2 issuer) - if (!AadIssuerValidator.IsValidIssuer(signingKeyIssuer, tenantIdFromToken, tokenIssuer) - && !AadIssuerValidator.IsValidIssuer(signingKeyIssuer, tenantIdFromToken, openIdConnectConfiguration.Issuer)) - { - int templateStartIndex = signingKeyIssuer.IndexOf(AadIssuerValidator.TenantIdTemplate, StringComparison.Ordinal); - string effectiveSigningKeyIssuer = templateStartIndex > -1 ? CreateIssuer(signingKeyIssuer, AadIssuerValidator.TenantIdTemplate, tenantIdFromToken, templateStartIndex) : signingKeyIssuer; + if (effectiveSigningKeyIssuer != tokenIssuer && effectiveSigningKeyIssuer != v2TokenIssuer) throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogHelper.FormatInvariant(LogMessages.IDX40005, LogHelper.MarkAsNonPII(tokenIssuer), LogHelper.MarkAsNonPII(effectiveSigningKeyIssuer)))); - } } return true; } - internal static string CreateIssuer(string issuer, string tenantIdTemplate, string tenantId, int templateStartIndex) - { -#if NET6_0_OR_GREATER - return string.Concat(issuer.AsSpan(0, templateStartIndex), tenantId, issuer.AsSpan(templateStartIndex + tenantIdTemplate.Length, issuer.Length - tenantIdTemplate.Length - templateStartIndex)); -#else - return string.Concat(issuer.Substring(0, templateStartIndex), tenantId, issuer.Substring(templateStartIndex + tenantIdTemplate.Length)); -#endif - } - /// /// Validates the issuer signing key certificate. /// diff --git a/test/Microsoft.IdentityModel.Validators.Tests/AadIssuerValidatorTests.cs b/test/Microsoft.IdentityModel.Validators.Tests/AadIssuerValidatorTests.cs deleted file mode 100644 index 4e05268f1f..0000000000 --- a/test/Microsoft.IdentityModel.Validators.Tests/AadIssuerValidatorTests.cs +++ /dev/null @@ -1,24 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. -using Xunit; - -namespace Microsoft.IdentityModel.Validators.Tests -{ - public class AadIssuerValidatorTests - { - [Theory] - [InlineData(ValidatorConstants.AadInstance + AadIssuerValidator.TenantIdTemplate, ValidatorConstants.AadInstance + ValidatorConstants.TenantIdAsGuid, true)] - [InlineData(ValidatorConstants.AadInstancePPE + AadIssuerValidator.TenantIdTemplate, ValidatorConstants.AadInstance + ValidatorConstants.TenantIdAsGuid, false)] - [InlineData(ValidatorConstants.AadInstance + AadIssuerValidator.TenantIdTemplate, ValidatorConstants.AadInstance + ValidatorConstants.B2CTenantAsGuid, false)] - [InlineData("", ValidatorConstants.AadInstance + ValidatorConstants.TenantIdAsGuid, false)] - [InlineData(ValidatorConstants.AadInstance + AadIssuerValidator.TenantIdTemplate, "", false)] - public static void IsValidIssuer_CanValidateTemplatedIssuers(string templatedIssuer, string issuer, bool expectedResult) - { - // act - var result = AadIssuerValidator.IsValidIssuer(templatedIssuer, ValidatorConstants.TenantIdAsGuid, issuer); - - // assert - Assert.Equal(expectedResult, result); - } - } -} diff --git a/test/Microsoft.IdentityModel.Validators.Tests/AadSigningKeyIssuerValidatorTests.cs b/test/Microsoft.IdentityModel.Validators.Tests/AadSigningKeyIssuerValidatorTests.cs index b979f7c49e..e768a2372f 100644 --- a/test/Microsoft.IdentityModel.Validators.Tests/AadSigningKeyIssuerValidatorTests.cs +++ b/test/Microsoft.IdentityModel.Validators.Tests/AadSigningKeyIssuerValidatorTests.cs @@ -325,21 +325,6 @@ public static TheoryData ValidateIssuerSigningKey } } - [Fact] - public static void CreateIssuer_ReturnsExpectedIssuer() - { - // arrange - var issuerTemplate = "{tenantId}"; - var issuer = ValidatorConstants.AadInstance + issuerTemplate; - int templateStartIndex = issuer.IndexOf(issuerTemplate); - - // act - var result = AadTokenValidationParametersExtension.CreateIssuer(issuer, issuerTemplate, ValidatorConstants.TenantIdAsGuid, templateStartIndex); - - // assert - Assert.Equal(ValidatorConstants.AadInstance + ValidatorConstants.TenantIdAsGuid, result); - } - private static OpenIdConnectConfiguration GetConfigurationMock() { var config = new OpenIdConnectConfiguration();