From 0b2f26917d4694a71182b16861aaed716ad0c4fa Mon Sep 17 00:00:00 2001 From: George Krechar Date: Thu, 12 Oct 2023 18:07:04 +0000 Subject: [PATCH] Merged PR 10182: Don't resolve jku claim by default 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 internal HTTP client to 10 seconds. - It adds a new unit test for the `SignedHttpRequestHandler` constructor in the `SignedHttpRequestUtilityTests` class. - It changes the visibility of the `_defaultHttpClient` field in the `SignedHttpRequestHandler` class from private to internal, presumably for testing purposes. - It adds a new validation logic for the `jku` claim in the `SignedHttpRequestHandler` class, which checks if resolving a PoP key from the `jku` claim is allowed and if the `jku` claim value belongs to a trusted domain. - It adds several new unit tests for the `jku` claim validation logic in the `PopKeyResolvingTests` class. - It adds two new properties to the `SignedHttpRequestValidationParameters` class: `AllowResolvingPopKeyFromJku` and `AllowedDomainsForJkuRetrieval`, which control the behavior of the `jku` claim validation. - It adds two new constants to the `ResolvePopKeyTheoryData` class: `_defaultJkuUri` and `_defaultJkuDomain`, which are used in the unit tests for the `jku` claim validation. --- .../LogMessages.cs | 2 + .../SignedHttpRequestHandler.cs | 33 ++++- .../SignedHttpRequestValidationParameters.cs | 21 ++++ .../PopKeyResolvingTests.cs | 118 +++++++++++++++++- .../SignedHttpRequestUtilityTests.cs | 8 ++ 5 files changed, 177 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 6042fff1b7..361c1d984c 100644 --- a/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestHandler.cs +++ b/src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestHandler.cs @@ -56,7 +56,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 /// @@ -1102,6 +1110,17 @@ internal virtual async Task ResolvePopKeyFromJweAsync(string jwe, S /// A resolved PoP . internal virtual async Task ResolvePopKeyFromJkuAsync(string jkuSetUrl, JObject 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) @@ -1248,6 +1267,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 f0311e54a6..fc5a6687a0 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) { @@ -396,7 +398,7 @@ public async Task ResolvePopKeyFromJkuAsync(ResolvePopKeyTheoryData theoryData) { var signedHttpRequestValidationContext = theoryData.BuildSignedHttpRequestValidationContext(); var handler = new SignedHttpRequestHandlerPublic(); - var popKey = await handler.ResolvePopKeyFromJkuPublicAsync(string.Empty, null, null, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false); + var popKey = await handler.ResolvePopKeyFromJkuPublicAsync(theoryData.JkuSetUrl, null, null, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false); if (popKey == null) context.AddDiff("Resolved Pop key is null."); @@ -427,6 +429,7 @@ public static TheoryData ResolvePopKeyFromJkuTheoryData {"mockGetPopKeysFromJkuAsync_return0Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"), TestId = "InvalidZeroKeysReturned", }, @@ -439,6 +442,7 @@ public static TheoryData ResolvePopKeyFromJkuTheoryData {"mockGetPopKeysFromJkuAsync_returnNull", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"), TestId = "InvalidNullReturned", }, @@ -451,8 +455,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" + } }; } } @@ -528,7 +630,7 @@ public async Task ResolvePopKeyFromJkuKidAsync(ResolvePopKeyTheoryData theoryDat { var signedHttpRequestValidationContext = theoryData.BuildSignedHttpRequestValidationContext(); var handler = new SignedHttpRequestHandlerPublic(); - var popKey = await handler.ResolvePopKeyFromJkuPublicAsync(string.Empty, JObject.Parse($@"{{""kid"": ""{theoryData.Kid}""}}"), null, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false); + var popKey = await handler.ResolvePopKeyFromJkuPublicAsync(theoryData.JkuSetUrl, JObject.Parse($@"{{""kid"": ""{theoryData.Kid}""}}"), null, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false); if (popKey == null) context.AddDiff("Resolved Pop key is null."); @@ -560,6 +662,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return0Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"), TestId = "InvalidZeroKeysReturned", }, @@ -573,6 +676,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_returnNull", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"), TestId = "InvalidNullReturned", }, @@ -586,6 +690,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return2Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23021"), TestId = "InvalidNoKidMatch", }, @@ -599,6 +704,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return2Keys", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, TestId = "ValidOneKidMatch", }, new ResolvePopKeyTheoryData @@ -611,6 +717,7 @@ public static TheoryData ResolvePopKeyFromJkuKidTheoryD {"mockGetPopKeysFromJkuAsync_return1Key", null } } }, + SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } }, TestId = "ValidKidMatch", }, }; @@ -845,6 +952,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; } @@ -865,7 +974,8 @@ public SignedHttpRequestValidationContext BuildSignedHttpRequestValidationContex ValidateP = true, ValidateQ = true, ValidateTs = true, - ValidateU = true + ValidateU = true, + AllowResolvingPopKeyFromJku = true }; public SigningCredentials SigningCredentials { get; set; } = SignedHttpRequestTestUtils.DefaultSigningCredentials; @@ -880,7 +990,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 ede9f65313..46b9ff9fb8 100644 --- a/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/SignedHttpRequestUtilityTests.cs +++ b/test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/SignedHttpRequestUtilityTests.cs @@ -20,6 +20,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() {