Remove dependency on AadIssuerValidator.GetTenantIdFromToken in ValidateIssuerSigningKey (#2680)

keegan-caruso · Keegan Caruso · web-flow · commit 9f3c7f3f53d6 · 2024-07-03T17:01:35.000-07:00
* Remove dep on AadIssuerValidator
* Add DontFailOnMissingTidValidateIssuerSigning AppContextSwitch
* Fail if multiple tids

---------

Co-authored-by: Keegan Caruso &lt;keegancaruso@microsoft.com&gt;
diff --git a/src/Microsoft.IdentityModel.JsonWebTokens/InternalsVisibleTo.cs b/src/Microsoft.IdentityModel.JsonWebTokens/InternalsVisibleTo.cs
@@ -1,4 +1,5 @@
 ﻿// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("Microsoft.IdentityModel.Validators, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]
 [assembly: System.Runtime.CompilerServices.InternalsVisibleTo("Microsoft.IdentityModel.JsonWebTokens.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]
diff --git a/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs b/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs
@@ -625,6 +625,11 @@ public Claim GetClaim(string key)
             return Payload.GetClaim(key, Issuer ?? ClaimsIdentity.DefaultIssuer);
         }
 
+        /// <summary>
+        /// Gets the names of the payload claims on the JsonWebToken.
+        /// </summary>
+        internal IReadOnlyCollection<string> PayloadClaimNames => Payload._jsonClaims.Keys;
+
         internal ClaimsIdentity ClaimsIdentity
         {
             get
diff --git a/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs b/src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs
@@ -2,7 +2,11 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Collections.Generic;
+using System.IdentityModel.Tokens.Jwt;
 using System.Linq;
+using System.Security.Claims;
+using Microsoft.IdentityModel.JsonWebTokens;
 using Microsoft.IdentityModel.Logging;
 using Microsoft.IdentityModel.Protocols.OpenIdConnect;
 using Microsoft.IdentityModel.Tokens;
@@ -41,6 +45,13 @@ public static void EnableAadSigningKeyIssuerValidation(this TokenValidationParam
             };
         }
 
+        internal const string DontFailOnMissingTidSwitch = "Switch.Microsoft.IdentityModel.DontFailOnMissingTidValidateIssuerSigning";
+
+        private static bool DontFailOnMissingTid()
+        {
+            return (AppContext.TryGetSwitch(DontFailOnMissingTidSwitch, out bool dontFailOnMissingTid) && dontFailOnMissingTid);
+        }
+
         /// <summary>
         /// Validates the issuer signing key.
         /// </summary>
@@ -67,9 +78,14 @@ 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 (DontFailOnMissingTid())
+                        return true; 
+                    
+                    throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogMessages.IDX40009));
+                }
 
                 var tokenIssuer = securityToken.Issuer;
 
@@ -91,7 +107,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 +117,48 @@ internal static bool ValidateIssuerSigningKey(SecurityKey securityKey, SecurityT
             return true;
         }
 
+        private static string GetTid(SecurityToken securityToken)
+        {
+            switch (securityToken)
+            {
+                case JsonWebToken jsonWebToken:
+                    if (jsonWebToken.TryGetPayloadValue<string>(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<string> 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;
+                }
+            }
+        }
+
         /// <summary>
         /// Validates the issuer signing key certificate.
         /// </summary>
diff --git a/src/Microsoft.IdentityModel.Validators/LogMessages.cs b/src/Microsoft.IdentityModel.Validators/LogMessages.cs
@@ -24,5 +24,9 @@ internal static class LogMessages
         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 IDX40008 = "IDX40008: When setting LastKnownGoodLifetime, the value must be greater than or equal to zero. value: '{0}'.";
+
+        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
@@ -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<AadSigningKeyIssuerTheoryData> 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,94 @@ public static TheoryData<AadSigningKeyIssuerTheoryData> ValidateIssuerSigningKey
                     ExpectedException = ExpectedException.SecurityTokenInvalidIssuerException("IDX40004")
                 });
 
+                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)
+                });
+
+                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 +452,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; }
         }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -625,6 +625,11 @@ public Claim GetClaim(string key)`
`625`	`625`	`return Payload.GetClaim(key, Issuer ?? ClaimsIdentity.DefaultIssuer);`
`626`	`626`	`}`
`627`	`627`
	`628`	`+ /// <summary>`
	`629`	`+ /// Gets the names of the payload claims on the JsonWebToken.`
	`630`	`+ /// </summary>`
	`631`	`+ internal IReadOnlyCollection<string> PayloadClaimNames => Payload._jsonClaims.Keys;`
	`632`	`+`
`628`	`633`	`internal ClaimsIdentity ClaimsIdentity`
`629`	`634`	`{`
`630`	`635`	`get`
Original file line number	Diff line number	Diff line change
`@@ -24,5 +24,9 @@ internal static class LogMessages`
`24`	`24`	`public const string IDX40005 = "IDX40005: Token issuer: '{0}', does not match the signing key issuer: '{1}'.";`
`25`	`25`	`public const string IDX40007 = "IDX40007: RequireSignedTokens property on ValidationParameters is set to true, but the issuer signing key is null.";`
`26`	`26`	`public const string IDX40008 = "IDX40008: When setting LastKnownGoodLifetime, the value must be greater than or equal to zero. value: '{0}'.";`
	`27`	`+`
	`28`	`+ public const string IDX40009 = "IDX40009: Either the 'tid' claim was not found or it didn't have a value.";`
	`29`	`+ public const string IDX40010 = "IDX40010: The SecurityToken must be a 'JsonWebToken' or 'JwtSecurityToken'";`
	`30`	`+ public const string IDX40011 = "IDX40011: The SecurityToken has multiple instances of the '{0}' claim.";`
`27`	`31`	`}`
`28`	`32`	`}`