From e986e22225fed7425c5e37a94d2d75e0c6cac1d1 Mon Sep 17 00:00:00 2001 From: George Krechar Date: Thu, 12 Oct 2023 18:07:12 +0000 Subject: [PATCH] Merged PR 10198: Don't resolve jku claim by default #### AI-Generated Description This pull request introduces the following changes: - It adds a new constructor for the `SignedHttpRequestHandler` class that sets the default timeout for the `_defaultHttpClient` field to 10 seconds. - It changes the `_defaultHttpClient` field from private to internal for testing purposes. - It adds a new property `AllowResolvingPopKeyFromJku` to the `SignedHttpRequestValidationParameters` class that indicates whether PoP key can be resolved from the 'jku' claim. - It adds a new property `AllowedDomainsForJkuRetrieval` to the `SignedHttpRequestValidationParameters` class that specifies a list of allowed domains for 'jku' claim retrieval. - It adds logic to the `ResolvePopKeyFromJkuAsync` method in the `SignedHttpRequestHandler` class to check the `AllowResolvingPopKeyFromJku` and `AllowedDomainsForJkuRetrieval` properties before resolving a PoP key from the 'jku' claim. - It adds a new method `IsJkuUriInListOfAllowedDomains` to the `SignedHttpRequestHandler` class that checks if a given 'jku' URI belongs to one of the allowed domains. - It adds new unit tests for the `SignedHttpRequestHandler` constructor, the `IsJkuUriInListOfAllowedDomains` method, and the `ResolvePopKeyFromJkuAsync` method in the `SignedHttpRequestHandler` class. - It adds new unit tests for the `SignedHttpRequestCtorTests` and the `SignedHttpRequestUtilityTests` classes. - It adds new exception messages to the `LogMessages` class related to the 'jku' claim validation. --- .../LogMessages.cs | 2 + .../SignedHttpRequestHandler.cs | 33 ++++- .../SignedHttpRequestValidationParameters.cs | 21 ++++ .../PopKeyResolvingTests.cs | 119 +++++++++++++++++- .../SignedHttpRequestUtilityTests.cs | 8 ++ 5 files changed, 178 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/LogMessages.cs b/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/LogMessages.cs index 057ffb9563..6e84fd75ef 100644 --- a/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/LogMessages.cs +++ b/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/LogMessages.cs @@ -46,5 +46,7 @@ internal static class LogMessages public const string IDX23034 = "IDX23034: Signed http request signature validation failed. SignedHttpRequest: '{0}'"; public const string IDX23035 = "IDX23035: Unable to resolve a PoP key from the 'jku' claim. Multiple keys are found in the referenced JWK Set document and the 'cnf' claim doesn't contain a 'kid' value."; public const string IDX23036 = "IDX23036: Signed http request nonce validation failed. Exceptions caught: '{0}'."; + public const string IDX23037 = "IDX23037: Resolving a PoP key from the 'jku' claim is not allowed. To allow it, set AllowResolvingPopKeyFromJku property on SignedHttpRequestValidationParameters to true and provide a list of trusted domains via AllowedDomainsForJkuRetrieval."; + public const string IDX23038 = "IDX23038: Resolving a PoP key from the 'jku' claim is not allowed as '{0}' is not present in the list of allowed domains for 'jku' retrieval: '{1}'. If '{0}' belongs to a trusted domain, add it to AllowedDomainsForJkuRetrieval property on SignedHttpRequestValidationParameters."; } } diff --git a/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestHandler.cs b/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestHandler.cs index de08e08808..3cfcdf5324 100644 --- a/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestHandler.cs +++ b/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestHandler.cs @@ -37,7 +37,15 @@ public class SignedHttpRequestHandler }; private readonly Uri _baseUriHelper = new Uri("http://localhost", UriKind.Absolute); - private readonly HttpClient _defaultHttpClient = new HttpClient(); + internal readonly HttpClient _defaultHttpClient = new HttpClient(); + + /// + /// Initializes a new instance of . + /// + public SignedHttpRequestHandler() + { + _defaultHttpClient.Timeout = TimeSpan.FromSeconds(10); + } #region SignedHttpRequest creation /// @@ -1121,6 +1129,17 @@ internal virtual async Task ResolvePopKeyFromJweAsync(string jwe, S /// A resolved PoP . internal virtual async Task ResolvePopKeyFromJkuAsync(string jkuSetUrl, Cnf cnf, SignedHttpRequestValidationContext signedHttpRequestValidationContext, CancellationToken cancellationToken) { + if (signedHttpRequestValidationContext.SignedHttpRequestValidationParameters.AllowResolvingPopKeyFromJku == false) + { + throw LogHelper.LogExceptionMessage(new SignedHttpRequestInvalidPopKeyException(LogHelper.FormatInvariant(LogMessages.IDX23037))); + } + + if (!IsJkuUriInListOfAllowedDomains(jkuSetUrl, signedHttpRequestValidationContext)) + { + var allowedDomains = string.Join(", ", signedHttpRequestValidationContext.SignedHttpRequestValidationParameters.AllowedDomainsForJkuRetrieval ?? new List()); + throw LogHelper.LogExceptionMessage(new SignedHttpRequestInvalidPopKeyException(LogHelper.FormatInvariant(LogMessages.IDX23038, jkuSetUrl, allowedDomains))); + } + var popKeys = await GetPopKeysFromJkuAsync(jkuSetUrl, signedHttpRequestValidationContext, cancellationToken).ConfigureAwait(false); if (popKeys == null || popKeys.Count == 0) @@ -1281,6 +1300,18 @@ private Uri EnsureAbsoluteUri(Uri uri) } } + private static bool IsJkuUriInListOfAllowedDomains(string jkuSetUrl, SignedHttpRequestValidationContext signedHttpRequestValidationContext) + { + if (string.IsNullOrEmpty(jkuSetUrl)) + return false; + + if (signedHttpRequestValidationContext.SignedHttpRequestValidationParameters.AllowedDomainsForJkuRetrieval.Count == 0) + return false; + + var uri = new Uri(jkuSetUrl, UriKind.RelativeOrAbsolute); + return signedHttpRequestValidationContext.SignedHttpRequestValidationParameters.AllowedDomainsForJkuRetrieval.Any(domain => uri.Host.EndsWith(domain, StringComparison.OrdinalIgnoreCase)); + } + /// /// Sanitizes the query params to comply with the specification. /// diff --git a/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestValidationParameters.cs b/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestValidationParameters.cs index 65403fc8a9..858c948acd 100644 --- a/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestValidationParameters.cs +++ b/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestValidationParameters.cs @@ -84,6 +84,7 @@ public class SignedHttpRequestValidationParameters { private TimeSpan _signedHttpRequestLifetime = DefaultSignedHttpRequestLifetime; private TokenHandler _tokenHandler = new JsonWebTokenHandler(); + private ICollection _allowedDomainsForJkuRetrieval; /// /// Gets or sets a value indicating whether the unsigned query parameters are accepted or not. @@ -97,6 +98,26 @@ public class SignedHttpRequestValidationParameters /// https://datatracker.ietf.org/doc/html/draft-ietf-oauth-signed-http-request-03#section-5.1 public bool AcceptUnsignedHeaders { get; set; } = true; + /// + /// Gets or sets a value indicating whether PoP key can be resolved from 'jku' claim. + /// If you set this property to true, you must set values in . + /// + /// https://datatracker.ietf.org/doc/html/rfc7800#section-3.5 + public bool AllowResolvingPopKeyFromJku { get; set; } = false; + + /// + /// Gets or sets a list of allowed domains for 'jku' claim retrieval. + /// The domains are not directly compared with the 'jku' claim. Allowed domain would be + /// deemed valid if the host specified in the 'jku' claim ends with the domain value. + /// + /// + /// Domains should be provided in the following format: + /// "contoso.com" + /// "abc.fabrikam.com" + /// + public ICollection AllowedDomainsForJkuRetrieval => _allowedDomainsForJkuRetrieval ?? + Interlocked.CompareExchange(ref _allowedDomainsForJkuRetrieval, new List(), null) ?? _allowedDomainsForJkuRetrieval; + /// /// Gets or sets the claims to validate if present. /// diff --git a/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/PopKeyResolvingTests.cs b/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/PopKeyResolvingTests.cs index 72cf29b517..fbc9088962 100644 --- a/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/PopKeyResolvingTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/PopKeyResolvingTests.cs @@ -18,6 +18,8 @@ namespace Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests { public class PopKeyResolvingTests { + internal const string _defaultJkuDomain = "contoso.com"; + [Theory, MemberData(nameof(ResolvePopKeyFromCnfClaimAsyncTheoryData))] public async Task ResolvePopKeyFromCnfClaimAsync(ResolvePopKeyTheoryData theoryData) { @@ -398,7 +400,7 @@ public async Task ResolvePopKeyFromJkuAsync(ResolvePopKeyTheoryData theoryData) { var signedHttpRequestValidationContext = theoryData.BuildSignedHttpRequestValidationContext(); var handler = new SignedHttpRequestHandlerPublic(); - var popKey = await handler.ResolvePopKeyFromJkuAsync(string.Empty, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false); + var popKey = await handler.ResolvePopKeyFromJkuAsync(theoryData.JkuSetUrl, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false); if (popKey == null) context.AddDiff("Resolved Pop key is null."); @@ -429,6 +431,7 @@ public static TheoryData ResolvePopKeyFromJkuTheoryData {"mockGetPopKeysFromJkuAsync_return0Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"), TestId = "InvalidZeroKeysReturned", }, @@ -441,6 +444,7 @@ public static TheoryData ResolvePopKeyFromJkuTheoryData {"mockGetPopKeysFromJkuAsync_returnNull", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"), TestId = "InvalidNullReturned", }, @@ -453,8 +457,106 @@ public static TheoryData ResolvePopKeyFromJkuTheoryData {"mockGetPopKeysFromJkuAsync_return1Key", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, TestId = "ValidOneKeyReturned", }, + new ResolvePopKeyTheoryData + { + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = false }, + ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23037"), + TestId = "JkuTurnedOff", + }, + new ResolvePopKeyTheoryData + { + JkuSetUrl = "", + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true }, + ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), string.Format(LogMessages.IDX23038, "" , "")), + TestId = "JkuTurnedOnEmptyUrl" + }, + new ResolvePopKeyTheoryData + { + JkuSetUrl = "https://www.contoso.com", + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true }, + ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), string.Format(LogMessages.IDX23038, "https://www.contoso.com" , "")), + TestId = "JkuTurnedOnNullDomains" + }, + new ResolvePopKeyTheoryData + { + JkuSetUrl = "https://www.contoso.com", + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "contoso1.com", "test.contoso.com" }}, + ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), string.Format(LogMessages.IDX23038, "https://www.contoso.com" , "contoso1.com, test.contoso.com")), + TestId = "JkuTurnedOnDomainsMissmatch" + }, + new ResolvePopKeyTheoryData + { + CallContext = new CallContext() + { + PropertyBag = new Dictionary() + { + // to simulate http call and satisfy test requirements + {"mockGetPopKeysFromJkuAsync_return1Key", null } + } + }, + JkuSetUrl = "https://www.contoso.com", + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { ".com" }}, + TestId = "JkuTurnedOnTopLevelDomainMatch" + }, + new ResolvePopKeyTheoryData + { + CallContext = new CallContext() + { + PropertyBag = new Dictionary() + { + // to simulate http call and satisfy test requirements + {"mockGetPopKeysFromJkuAsync_return1Key", null } + } + }, + JkuSetUrl = "https://www.contoso.com", + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "contoso.com" }}, + TestId = "JkuTurnedOnDomainsMatch" + }, + new ResolvePopKeyTheoryData + { + CallContext = new CallContext() + { + PropertyBag = new Dictionary() + { + // to simulate http call and satisfy test requirements + {"mockGetPopKeysFromJkuAsync_return1Key", null } + } + }, + JkuSetUrl = "https://www.contoso.com", + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "Contoso.com" }}, + TestId = "JkuTurnedOnDomainsMatchCaseInsensitive" + }, + new ResolvePopKeyTheoryData + { + CallContext = new CallContext() + { + PropertyBag = new Dictionary() + { + // to simulate http call and satisfy test requirements + {"mockGetPopKeysFromJkuAsync_return1Key", null } + } + }, + JkuSetUrl = "https://contoso.com/mykeys/key/1?test=true", + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "Contoso.com" }}, + TestId = "JkuTurnedOnUrlWithPathAndQueryParam" + }, + new ResolvePopKeyTheoryData + { + CallContext = new CallContext() + { + PropertyBag = new Dictionary() + { + // to simulate http call and satisfy test requirements + {"mockGetPopKeysFromJkuAsync_return1Key", null } + } + }, + JkuSetUrl = "https://localhost/keys", + SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "localhost" }}, + TestId = "JkuTurnedOnLocalUrl" + } }; } } @@ -531,7 +633,7 @@ public async Task ResolvePopKeyFromJkuKidAsync(ResolvePopKeyTheoryData theoryDat var signedHttpRequestValidationContext = theoryData.BuildSignedHttpRequestValidationContext(); var handler = new SignedHttpRequestHandlerPublic(); Cnf cnf = new Cnf { Kid = theoryData.Kid }; - var popKey = await handler.ResolvePopKeyFromJkuAsync(string.Empty, cnf, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false); + var popKey = await handler.ResolvePopKeyFromJkuAsync(theoryData.JkuSetUrl, cnf, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false); if (popKey == null) context.AddDiff("Resolved Pop key is null."); @@ -562,6 +664,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return2Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23021"), TestId = "InvalidNoKidMatch", }, @@ -576,6 +679,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return0Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"), TestId = "InvalidZeroKeysReturned", }, @@ -589,6 +693,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_returnNull", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"), TestId = "InvalidNullReturned", }, @@ -602,6 +707,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return2Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23021"), TestId = "InvalidNoKidMatch", }, @@ -615,6 +721,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return2Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, TestId = "ValidOneKidMatch", }, new ResolvePopKeyTheoryData @@ -627,6 +734,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return1Key", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, TestId = "ValidKidMatch", }, }; @@ -861,6 +969,8 @@ public SignedHttpRequestValidationContext BuildSignedHttpRequestValidationContex return new SignedHttpRequestValidationContext(SignedHttpRequestToken is JsonWebToken jwt ? jwt.EncodedToken : "dummy", httpRequestData, SignedHttpRequestTestUtils.DefaultTokenValidationParameters, SignedHttpRequestValidationParameters, callContext); } + internal const string _defaultJkuUri = "https://contoso.com/jku"; + internal JObject ConfirmationClaim { get; set; } public string MethodToCall { get; set; } @@ -881,7 +991,8 @@ public SignedHttpRequestValidationContext BuildSignedHttpRequestValidationContex ValidateP = true, ValidateQ = true, ValidateTs = true, - ValidateU = true + ValidateU = true, + AllowResolvingPopKeyFromJku = true }; public SigningCredentials SigningCredentials { get; set; } = SignedHttpRequestTestUtils.DefaultSigningCredentials; @@ -896,7 +1007,7 @@ public SignedHttpRequestValidationContext BuildSignedHttpRequestValidationContex public string Kid { get; set; } - public string JkuSetUrl { get; set; } + public string JkuSetUrl { get; set; } = _defaultJkuUri; public int ExpectedNumberOfPopKeysReturned { get; set; } } diff --git a/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/SignedHttpRequestUtilityTests.cs b/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/SignedHttpRequestUtilityTests.cs index a1896c6995..663d8c0c1b 100644 --- a/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/SignedHttpRequestUtilityTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/SignedHttpRequestUtilityTests.cs @@ -19,6 +19,14 @@ namespace Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests { public class SignedHttpRequestUtilityTests { + [Fact] + public void SignedHttpRequestCtorTests() + { + var signedHttpRequestHandler = new SignedHttpRequestHandler(); + Assert.NotNull(signedHttpRequestHandler); + Assert.Equal(TimeSpan.FromSeconds(10), signedHttpRequestHandler._defaultHttpClient.Timeout); + } + [Fact] public void CreateSignedHttpRequestHeader() {