From 1671ccde7952669f9d0985587170a65638955549 Mon Sep 17 00:00:00 2001 From: Keegan Caruso Date: Wed, 3 Jul 2024 17:01:35 -0700 Subject: [PATCH] Remove dependency on AadIssuerValidator.GetTenantIdFromToken in ValidateIssuerSigningKey (#2680) * Remove dep on AadIssuerValidator * Add DontFailOnMissingTidValidateIssuerSigning AppContextSwitch * Fail if multiple tids --------- Co-authored-by: Keegan Caruso --- .../InternalsVisibleTo.cs | 4 + .../JsonWebToken.cs | 20 +++ .../AadTokenValidationParametersExtension.cs | 67 +++++++++- .../LogMessages.cs | 4 + .../AadSigningKeyIssuerValidatorTests.cs | 114 +++++++++++++++++- 5 files changed, 205 insertions(+), 4 deletions(-) create mode 100644 src/Microsoft.IdentityModel.JsonWebTokens/InternalsVisibleTo.cs diff --git a/src/Microsoft.IdentityModel.JsonWebTokens/InternalsVisibleTo.cs b/src/Microsoft.IdentityModel.JsonWebTokens/InternalsVisibleTo.cs new file mode 100644 index 0000000000..28fee0f4ca --- /dev/null +++ b/src/Microsoft.IdentityModel.JsonWebTokens/InternalsVisibleTo.cs @@ -0,0 +1,4 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("Microsoft.IdentityModel.Validators, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")] diff --git a/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs b/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs index 767d24d51e..0f838506c2 100644 --- a/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs +++ b/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs @@ -190,6 +190,26 @@ internal byte[] CipherTextBytes set; } + /// + /// Gets the names of the payload claims on the JsonWebToken. + /// + internal IReadOnlyCollection PayloadClaimNames + { +#if NET45 + get + { + var payloadClaimNames = new List(); + + foreach (Claim claim in Claims) + payloadClaimNames.Add(claim.Type); + + return payloadClaimNames; + } +#else + get => Payload.Elements.Keys; +#endif + } + internal ClaimsIdentity ClaimsIdentity { get diff --git a/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs b/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs index d745bf124c..1a0e132dfd 100644 --- a/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs +++ b/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs @@ -2,7 +2,10 @@ // Licensed under the MIT License. using System; +using System.Collections.Generic; +using System.IdentityModel.Tokens.Jwt; using System.Linq; +using Microsoft.IdentityModel.JsonWebTokens; using Microsoft.IdentityModel.Logging; using Microsoft.IdentityModel.Protocols.OpenIdConnect; using Microsoft.IdentityModel.Tokens; @@ -41,6 +44,15 @@ public static void EnableAadSigningKeyIssuerValidation(this TokenValidationParam }; } +#if !NET45 + internal const string DontFailOnMissingTidSwitch = "Switch.Microsoft.IdentityModel.DontFailOnMissingTidValidateIssuerSigning"; + + private static bool DontFailOnMissingTid() + { + return (AppContext.TryGetSwitch(DontFailOnMissingTidSwitch, out bool dontFailOnMissingTid) && dontFailOnMissingTid); + } +#endif + /// /// Validates the issuer signing key. /// @@ -67,9 +79,16 @@ internal static bool ValidateIssuerSigningKey(SecurityKey securityKey, SecurityT if (string.IsNullOrWhiteSpace(signingKeyIssuer)) return true; - var tenantIdFromToken = AadIssuerValidator.GetTenantIdFromToken(securityToken); + var tenantIdFromToken = GetTid(securityToken); if (string.IsNullOrEmpty(tenantIdFromToken)) - return true; + { +#if !NET45 + if (DontFailOnMissingTid()) + return true; +#endif + + throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogMessages.IDX40009)); + } var tokenIssuer = securityToken.Issuer; @@ -91,7 +110,7 @@ internal static bool ValidateIssuerSigningKey(SecurityKey securityKey, SecurityT // 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 + // 2. service receives 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 (effectiveSigningKeyIssuer != tokenIssuer && effectiveSigningKeyIssuer != v2TokenIssuer) @@ -101,6 +120,48 @@ internal static bool ValidateIssuerSigningKey(SecurityKey securityKey, SecurityT return true; } + private static string GetTid(SecurityToken securityToken) + { + switch (securityToken) + { + case JsonWebToken jsonWebToken: + if (jsonWebToken.TryGetPayloadValue(AadIssuerValidatorConstants.Tid, out string tid)) + { + EnforceSingleClaimCaseInsensitive(jsonWebToken.PayloadClaimNames, AadIssuerValidatorConstants.Tid); + return tid; + } + + return string.Empty; + + case JwtSecurityToken jwtSecurityToken: + if ((jwtSecurityToken.Payload.TryGetValue(AadIssuerValidatorConstants.Tid, out object tidObject) && tidObject is string jwtTid)) + { + EnforceSingleClaimCaseInsensitive(jwtSecurityToken.Payload.Keys, AadIssuerValidatorConstants.Tid); + return jwtTid; + } + + return string.Empty; + + default: + throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogMessages.IDX40010)); + } + } + + private static void EnforceSingleClaimCaseInsensitive(IEnumerable keys, string claimType) + { + bool claimSeen = false; + foreach (var key in keys) + { + if (string.Equals(key, claimType, StringComparison.OrdinalIgnoreCase)) + { + if (claimSeen) + throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogHelper.FormatInvariant(LogMessages.IDX40011, claimType))); + + claimSeen = true; + } + } + } + /// /// Validates the issuer signing key certificate. /// diff --git a/src/Microsoft.IdentityModel.Validators/LogMessages.cs b/src/Microsoft.IdentityModel.Validators/LogMessages.cs index e8edf46300..d3cd79324c 100644 --- a/src/Microsoft.IdentityModel.Validators/LogMessages.cs +++ b/src/Microsoft.IdentityModel.Validators/LogMessages.cs @@ -23,5 +23,9 @@ internal static class LogMessages public const string IDX40004 = "IDX40004: Token issuer: '{0}', does not contain the `tid` or `tenantId` claim present in the token: '{1}'."; public const string IDX40005 = "IDX40005: Token issuer: '{0}', does not match the signing key issuer: '{1}'."; public const string IDX40007 = "IDX40007: RequireSignedTokens property on ValidationParameters is set to true, but the issuer signing key is null."; + + public const string IDX40009 = "IDX40009: Either the 'tid' claim was not found or it didn't have a value."; + public const string IDX40010 = "IDX40010: The SecurityToken must be a 'JsonWebToken' or 'JwtSecurityToken'"; + public const string IDX40011 = "IDX40011: The SecurityToken has multiple instances of the '{0}' claim."; } } diff --git a/test/Microsoft.IdentityModel.Validators.Tests/AadSigningKeyIssuerValidatorTests.cs b/test/Microsoft.IdentityModel.Validators.Tests/AadSigningKeyIssuerValidatorTests.cs index e768a2372f..3da5af997c 100644 --- a/test/Microsoft.IdentityModel.Validators.Tests/AadSigningKeyIssuerValidatorTests.cs +++ b/test/Microsoft.IdentityModel.Validators.Tests/AadSigningKeyIssuerValidatorTests.cs @@ -11,12 +11,15 @@ using Microsoft.IdentityModel.Protocols.OpenIdConnect; using Microsoft.IdentityModel.TestUtils; using Microsoft.IdentityModel.Tokens; +using Microsoft.IdentityModel.Tokens.Saml2; using Xunit; #pragma warning disable CS3016 // Arrays as attribute arguments is not CLS-compliant namespace Microsoft.IdentityModel.Validators.Tests { + // Serialize as one of the tests depends on static state (app context) + [Collection(nameof(AadSigningKeyIssuerValidatorTests))] public class AadSigningKeyIssuerValidatorTests { [Theory, MemberData(nameof(EnableAadSigningKeyIssuerValidationTestCases))] @@ -167,6 +170,7 @@ public void ValidateIssuerSigningKeyTests(AadSigningKeyIssuerTheoryData theoryDa try { + theoryData.SetupAction?.Invoke(); var result = AadTokenValidationParametersExtension.ValidateIssuerSigningKey(theoryData.SecurityKey, theoryData.SecurityToken, theoryData.OpenIdConnectConfiguration); theoryData.ExpectedException.ProcessNoException(context); Assert.True(result); @@ -175,6 +179,10 @@ public void ValidateIssuerSigningKeyTests(AadSigningKeyIssuerTheoryData theoryDa { theoryData.ExpectedException.ProcessException(ex, context); } + finally + { + theoryData.TearDownAction?.Invoke(); + } TestUtilities.AssertFailIfErrors(context); } @@ -294,7 +302,17 @@ public static TheoryData ValidateIssuerSigningKey TestId = "MissingTenantIdClaimInToken", SecurityKey = KeyingMaterial.JsonWebKeyP256, SecurityToken = new JwtSecurityToken(), - OpenIdConnectConfiguration = mockConfiguration + OpenIdConnectConfiguration = mockConfiguration, + ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40009") + }); + + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "WrongSecurityKeyType", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new Saml2SecurityToken(new Saml2Assertion(new Saml2NameIdentifier("nameIdentifier"))), + OpenIdConnectConfiguration = mockConfiguration, + ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40010") }); theoryData.Add(new AadSigningKeyIssuerTheoryData @@ -321,6 +339,96 @@ public static TheoryData ValidateIssuerSigningKey ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40004") }); +#if !NET452 + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "Doesnt_Fail_With_Switch", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new JwtSecurityToken(), + OpenIdConnectConfiguration = mockConfiguration, + SetupAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, true), + TearDownAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, false) + }); + + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "Fail_With_Switch_False", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new JwtSecurityToken(), + OpenIdConnectConfiguration = mockConfiguration, + ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40009"), + SetupAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, false), + TearDownAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, isEnabled: false) + }); + + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "Doesnt_Fail_With_Switch", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new JwtSecurityToken(), + OpenIdConnectConfiguration = mockConfiguration, + SetupAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, true), + TearDownAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, false) + }); + + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "Fail_With_Switch_False_JsonWebToken", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new JsonWebToken(Default.Jwt(Default.SecurityTokenDescriptor(Default.SymmetricSigningCredentials, [issClaim]))), + OpenIdConnectConfiguration = mockConfiguration, + ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40009"), + SetupAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, false), + TearDownAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, isEnabled: false) + }); + + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "Doesnt_Fail_With_Switch_JsonWebToken", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new JsonWebToken(Default.Jwt(Default.SecurityTokenDescriptor(Default.SymmetricSigningCredentials, [issClaim]))), + OpenIdConnectConfiguration = mockConfiguration, + SetupAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, true), + TearDownAction = () => AppContext.SetSwitch(AadTokenValidationParametersExtension.DontFailOnMissingTidSwitch, false) + }); +#endif + + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "Fails_With_Multiple_tids", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new JsonWebToken( + Default.Jwt(Default.SecurityTokenDescriptor( + Default.SymmetricSigningCredentials, + [tidClaim, issClaim, new Claim("TID", Guid.NewGuid().ToString())]))), + ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40011"), + OpenIdConnectConfiguration = mockConfiguration + }); + + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "Fails_With_Multiple_tids_alternate_order", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new JsonWebToken( + Default.Jwt(Default.SecurityTokenDescriptor( + Default.SymmetricSigningCredentials, + [issClaim, new Claim("TID", Guid.NewGuid().ToString()), tidClaim]))), + ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40011"), + OpenIdConnectConfiguration = mockConfiguration + }); + + theoryData.Add(new AadSigningKeyIssuerTheoryData + { + TestId = "Fails_With_no standard_tid", + SecurityKey = KeyingMaterial.JsonWebKeyP256, + SecurityToken = new JsonWebToken( + Default.Jwt(Default.SecurityTokenDescriptor( + Default.SymmetricSigningCredentials, + [issClaim, new Claim("TID", Guid.NewGuid().ToString())]))), + ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40009"), + OpenIdConnectConfiguration = mockConfiguration + }); + return theoryData; } } @@ -346,6 +454,10 @@ public class AadSigningKeyIssuerTheoryData : TheoryDataBase public bool SetDelegateUsingConfig { get; set; } = false; public bool SetDelegateWithoutConfig { get; set; } = false; + + public Action SetupAction { get; set; } + + public Action TearDownAction { get; set; } } } }