Skip to content

Commit 0b2f269

Browse files
George KrecharGeorge Krechar
George Krechar
authored and
George Krechar
committed
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.
1 parent c3e99cd commit 0b2f269

File tree

5 files changed

+177
-5
lines changed

5 files changed

+177
-5
lines changed

‎src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/LogMessages.cs

+2
Original file line numberDiff line numberDiff line change
@@ -46,5 +46,7 @@ internal static class LogMessages
4646
public const string IDX23034 = "IDX23034: Signed http request signature validation failed. SignedHttpRequest: '{0}'";
4747
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.";
4848
public const string IDX23036 = "IDX23036: Signed http request nonce validation failed. Exceptions caught: '{0}'.";
49+
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.";
50+
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.";
4951
}
5052
}

‎src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestHandler.cs

+32-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,15 @@ public class SignedHttpRequestHandler
5656
};
5757

5858
private readonly Uri _baseUriHelper = new Uri("http://localhost", UriKind.Absolute);
59-
private readonly HttpClient _defaultHttpClient = new HttpClient();
59+
internal readonly HttpClient _defaultHttpClient = new HttpClient();
60+
61+
/// <summary>
62+
/// Initializes a new instance of <see cref="SignedHttpRequestHandler"/>.
63+
/// </summary>
64+
public SignedHttpRequestHandler()
65+
{
66+
_defaultHttpClient.Timeout = TimeSpan.FromSeconds(10);
67+
}
6068

6169
#region SignedHttpRequest creation
6270
/// <summary>
@@ -1102,6 +1110,17 @@ internal virtual async Task<SecurityKey> ResolvePopKeyFromJweAsync(string jwe, S
11021110
/// <returns>A resolved PoP <see cref="SecurityKey"/>.</returns>
11031111
internal virtual async Task<SecurityKey> ResolvePopKeyFromJkuAsync(string jkuSetUrl, JObject cnf, SignedHttpRequestValidationContext signedHttpRequestValidationContext, CancellationToken cancellationToken)
11041112
{
1113+
if (signedHttpRequestValidationContext.SignedHttpRequestValidationParameters.AllowResolvingPopKeyFromJku == false)
1114+
{
1115+
throw LogHelper.LogExceptionMessage(new SignedHttpRequestInvalidPopKeyException(LogHelper.FormatInvariant(LogMessages.IDX23037)));
1116+
}
1117+
1118+
if (!IsJkuUriInListOfAllowedDomains(jkuSetUrl, signedHttpRequestValidationContext))
1119+
{
1120+
var allowedDomains = string.Join(", ", signedHttpRequestValidationContext.SignedHttpRequestValidationParameters.AllowedDomainsForJkuRetrieval ?? new List<string>());
1121+
throw LogHelper.LogExceptionMessage(new SignedHttpRequestInvalidPopKeyException(LogHelper.FormatInvariant(LogMessages.IDX23038, jkuSetUrl, allowedDomains)));
1122+
}
1123+
11051124
var popKeys = await GetPopKeysFromJkuAsync(jkuSetUrl, signedHttpRequestValidationContext, cancellationToken).ConfigureAwait(false);
11061125

11071126
if (popKeys == null || popKeys.Count == 0)
@@ -1248,6 +1267,18 @@ private Uri EnsureAbsoluteUri(Uri uri)
12481267
}
12491268
}
12501269

1270+
private static bool IsJkuUriInListOfAllowedDomains(string jkuSetUrl, SignedHttpRequestValidationContext signedHttpRequestValidationContext)
1271+
{
1272+
if (string.IsNullOrEmpty(jkuSetUrl))
1273+
return false;
1274+
1275+
if (signedHttpRequestValidationContext.SignedHttpRequestValidationParameters.AllowedDomainsForJkuRetrieval.Count == 0)
1276+
return false;
1277+
1278+
var uri = new Uri(jkuSetUrl, UriKind.RelativeOrAbsolute);
1279+
return signedHttpRequestValidationContext.SignedHttpRequestValidationParameters.AllowedDomainsForJkuRetrieval.Any(domain => uri.Host.EndsWith(domain, StringComparison.OrdinalIgnoreCase));
1280+
}
1281+
12511282
/// <summary>
12521283
/// Sanitizes the query params to comply with the specification.
12531284
/// </summary>

‎src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestValidationParameters.cs

+21
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ public class SignedHttpRequestValidationParameters
8484
{
8585
private TimeSpan _signedHttpRequestLifetime = DefaultSignedHttpRequestLifetime;
8686
private TokenHandler _tokenHandler = new JsonWebTokenHandler();
87+
private ICollection<string> _allowedDomainsForJkuRetrieval;
8788

8889
/// <summary>
8990
/// Gets or sets a value indicating whether the unsigned query parameters are accepted or not.
@@ -97,6 +98,26 @@ public class SignedHttpRequestValidationParameters
9798
/// <remarks>https://datatracker.ietf.org/doc/html/draft-ietf-oauth-signed-http-request-03#section-5.1</remarks>
9899
public bool AcceptUnsignedHeaders { get; set; } = true;
99100

101+
/// <summary>
102+
/// Gets or sets a value indicating whether PoP key can be resolved from 'jku' claim.
103+
/// If you set this property to true, you must set values in <see cref="AllowedDomainsForJkuRetrieval"/>.
104+
/// </summary>
105+
/// <remarks>https://datatracker.ietf.org/doc/html/rfc7800#section-3.5</remarks>
106+
public bool AllowResolvingPopKeyFromJku { get; set; } = false;
107+
108+
/// <summary>
109+
/// Gets or sets a list of allowed domains for 'jku' claim retrieval.
110+
/// The domains are not directly compared with the 'jku' claim. Allowed domain would be
111+
/// deemed valid if the host specified in the 'jku' claim ends with the domain value.
112+
/// </summary>
113+
/// <remarks>
114+
/// Domains should be provided in the following format:
115+
/// "contoso.com"
116+
/// "abc.fabrikam.com"
117+
/// </remarks>
118+
public ICollection<string> AllowedDomainsForJkuRetrieval => _allowedDomainsForJkuRetrieval ??
119+
Interlocked.CompareExchange(ref _allowedDomainsForJkuRetrieval, new List<string>(), null) ?? _allowedDomainsForJkuRetrieval;
120+
100121
/// <summary>
101122
/// Gets or sets the claims to validate if present.
102123
/// </summary>

‎test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/PopKeyResolvingTests.cs

+114-4
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ namespace Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests
1818
{
1919
public class PopKeyResolvingTests
2020
{
21+
internal const string _defaultJkuDomain = "contoso.com";
22+
2123
[Theory, MemberData(nameof(ResolvePopKeyFromCnfClaimAsyncTheoryData))]
2224
public async Task ResolvePopKeyFromCnfClaimAsync(ResolvePopKeyTheoryData theoryData)
2325
{
@@ -396,7 +398,7 @@ public async Task ResolvePopKeyFromJkuAsync(ResolvePopKeyTheoryData theoryData)
396398
{
397399
var signedHttpRequestValidationContext = theoryData.BuildSignedHttpRequestValidationContext();
398400
var handler = new SignedHttpRequestHandlerPublic();
399-
var popKey = await handler.ResolvePopKeyFromJkuPublicAsync(string.Empty, null, null, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false);
401+
var popKey = await handler.ResolvePopKeyFromJkuPublicAsync(theoryData.JkuSetUrl, null, null, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false);
400402

401403
if (popKey == null)
402404
context.AddDiff("Resolved Pop key is null.");
@@ -427,6 +429,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuTheoryData
427429
{"mockGetPopKeysFromJkuAsync_return0Keys", null }
428430
}
429431
},
432+
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
430433
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"),
431434
TestId = "InvalidZeroKeysReturned",
432435
},
@@ -439,6 +442,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuTheoryData
439442
{"mockGetPopKeysFromJkuAsync_returnNull", null }
440443
}
441444
},
445+
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
442446
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"),
443447
TestId = "InvalidNullReturned",
444448
},
@@ -451,8 +455,106 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuTheoryData
451455
{"mockGetPopKeysFromJkuAsync_return1Key", null }
452456
}
453457
},
458+
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
454459
TestId = "ValidOneKeyReturned",
455460
},
461+
new ResolvePopKeyTheoryData
462+
{
463+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = false },
464+
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23037"),
465+
TestId = "JkuTurnedOff",
466+
},
467+
new ResolvePopKeyTheoryData
468+
{
469+
JkuSetUrl = "",
470+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true },
471+
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), string.Format(LogMessages.IDX23038, "" , "")),
472+
TestId = "JkuTurnedOnEmptyUrl"
473+
},
474+
new ResolvePopKeyTheoryData
475+
{
476+
JkuSetUrl = "https://www.contoso.com",
477+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true },
478+
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), string.Format(LogMessages.IDX23038, "https://www.contoso.com" , "")),
479+
TestId = "JkuTurnedOnNullDomains"
480+
},
481+
new ResolvePopKeyTheoryData
482+
{
483+
JkuSetUrl = "https://www.contoso.com",
484+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "contoso1.com", "test.contoso.com" }},
485+
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), string.Format(LogMessages.IDX23038, "https://www.contoso.com" , "contoso1.com, test.contoso.com")),
486+
TestId = "JkuTurnedOnDomainsMissmatch"
487+
},
488+
new ResolvePopKeyTheoryData
489+
{
490+
CallContext = new CallContext()
491+
{
492+
PropertyBag = new Dictionary<string, object>()
493+
{
494+
// to simulate http call and satisfy test requirements
495+
{"mockGetPopKeysFromJkuAsync_return1Key", null }
496+
}
497+
},
498+
JkuSetUrl = "https://www.contoso.com",
499+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { ".com" }},
500+
TestId = "JkuTurnedOnTopLevelDomainMatch"
501+
},
502+
new ResolvePopKeyTheoryData
503+
{
504+
CallContext = new CallContext()
505+
{
506+
PropertyBag = new Dictionary<string, object>()
507+
{
508+
// to simulate http call and satisfy test requirements
509+
{"mockGetPopKeysFromJkuAsync_return1Key", null }
510+
}
511+
},
512+
JkuSetUrl = "https://www.contoso.com",
513+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "contoso.com" }},
514+
TestId = "JkuTurnedOnDomainsMatch"
515+
},
516+
new ResolvePopKeyTheoryData
517+
{
518+
CallContext = new CallContext()
519+
{
520+
PropertyBag = new Dictionary<string, object>()
521+
{
522+
// to simulate http call and satisfy test requirements
523+
{"mockGetPopKeysFromJkuAsync_return1Key", null }
524+
}
525+
},
526+
JkuSetUrl = "https://www.contoso.com",
527+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "Contoso.com" }},
528+
TestId = "JkuTurnedOnDomainsMatchCaseInsensitive"
529+
},
530+
new ResolvePopKeyTheoryData
531+
{
532+
CallContext = new CallContext()
533+
{
534+
PropertyBag = new Dictionary<string, object>()
535+
{
536+
// to simulate http call and satisfy test requirements
537+
{"mockGetPopKeysFromJkuAsync_return1Key", null }
538+
}
539+
},
540+
JkuSetUrl = "https://contoso.com/mykeys/key/1?test=true",
541+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "Contoso.com" }},
542+
TestId = "JkuTurnedOnUrlWithPathAndQueryParam"
543+
},
544+
new ResolvePopKeyTheoryData
545+
{
546+
CallContext = new CallContext()
547+
{
548+
PropertyBag = new Dictionary<string, object>()
549+
{
550+
// to simulate http call and satisfy test requirements
551+
{"mockGetPopKeysFromJkuAsync_return1Key", null }
552+
}
553+
},
554+
JkuSetUrl = "https://localhost/keys",
555+
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "localhost" }},
556+
TestId = "JkuTurnedOnLocalUrl"
557+
}
456558
};
457559
}
458560
}
@@ -528,7 +630,7 @@ public async Task ResolvePopKeyFromJkuKidAsync(ResolvePopKeyTheoryData theoryDat
528630
{
529631
var signedHttpRequestValidationContext = theoryData.BuildSignedHttpRequestValidationContext();
530632
var handler = new SignedHttpRequestHandlerPublic();
531-
var popKey = await handler.ResolvePopKeyFromJkuPublicAsync(string.Empty, JObject.Parse($@"{{""kid"": ""{theoryData.Kid}""}}"), null, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false);
633+
var popKey = await handler.ResolvePopKeyFromJkuPublicAsync(theoryData.JkuSetUrl, JObject.Parse($@"{{""kid"": ""{theoryData.Kid}""}}"), null, null, signedHttpRequestValidationContext, CancellationToken.None).ConfigureAwait(false);
532634

533635
if (popKey == null)
534636
context.AddDiff("Resolved Pop key is null.");
@@ -560,6 +662,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
560662
{"mockGetPopKeysFromJkuAsync_return0Keys", null }
561663
}
562664
},
665+
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
563666
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"),
564667
TestId = "InvalidZeroKeysReturned",
565668
},
@@ -573,6 +676,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
573676
{"mockGetPopKeysFromJkuAsync_returnNull", null }
574677
}
575678
},
679+
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
576680
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"),
577681
TestId = "InvalidNullReturned",
578682
},
@@ -586,6 +690,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
586690
{"mockGetPopKeysFromJkuAsync_return2Keys", null }
587691
}
588692
},
693+
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
589694
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23021"),
590695
TestId = "InvalidNoKidMatch",
591696
},
@@ -599,6 +704,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
599704
{"mockGetPopKeysFromJkuAsync_return2Keys", null }
600705
}
601706
},
707+
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
602708
TestId = "ValidOneKidMatch",
603709
},
604710
new ResolvePopKeyTheoryData
@@ -611,6 +717,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
611717
{"mockGetPopKeysFromJkuAsync_return1Key", null }
612718
}
613719
},
720+
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
614721
TestId = "ValidKidMatch",
615722
},
616723
};
@@ -845,6 +952,8 @@ public SignedHttpRequestValidationContext BuildSignedHttpRequestValidationContex
845952
return new SignedHttpRequestValidationContext(SignedHttpRequestToken is JsonWebToken jwt ? jwt.EncodedToken : "dummy", httpRequestData, SignedHttpRequestTestUtils.DefaultTokenValidationParameters, SignedHttpRequestValidationParameters, callContext);
846953
}
847954

955+
internal const string _defaultJkuUri = "https://contoso.com/jku";
956+
848957
internal JObject ConfirmationClaim { get; set; }
849958

850959
public string MethodToCall { get; set; }
@@ -865,7 +974,8 @@ public SignedHttpRequestValidationContext BuildSignedHttpRequestValidationContex
865974
ValidateP = true,
866975
ValidateQ = true,
867976
ValidateTs = true,
868-
ValidateU = true
977+
ValidateU = true,
978+
AllowResolvingPopKeyFromJku = true
869979
};
870980

871981
public SigningCredentials SigningCredentials { get; set; } = SignedHttpRequestTestUtils.DefaultSigningCredentials;
@@ -880,7 +990,7 @@ public SignedHttpRequestValidationContext BuildSignedHttpRequestValidationContex
880990

881991
public string Kid { get; set; }
882992

883-
public string JkuSetUrl { get; set; }
993+
public string JkuSetUrl { get; set; } = _defaultJkuUri;
884994

885995
public int ExpectedNumberOfPopKeysReturned { get; set; }
886996
}

‎test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/SignedHttpRequestUtilityTests.cs

+8
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,14 @@ namespace Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests
2020
{
2121
public class SignedHttpRequestUtilityTests
2222
{
23+
[Fact]
24+
public void SignedHttpRequestCtorTests()
25+
{
26+
var signedHttpRequestHandler = new SignedHttpRequestHandler();
27+
Assert.NotNull(signedHttpRequestHandler);
28+
Assert.Equal(TimeSpan.FromSeconds(10), signedHttpRequestHandler._defaultHttpClient.Timeout);
29+
}
30+
2331
[Fact]
2432
public void CreateSignedHttpRequestHeader()
2533
{

0 commit comments

Comments
 (0)