Skip to content

Commit

Permalink
Merged PR 10198: Don't resolve jku claim by default
Browse files Browse the repository at this point in the history 
#### 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.
  • Loading branch information
George Krechar authored and George Krechar committed Oct 12, 2023
1 parent a62cd3b commit e986e22
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 5 deletions.
2 changes: 2 additions & 0 deletions src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/LogMessages.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.";
}
}
33 changes: 32 additions & 1 deletion src/Microsoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

/// <summary>
/// Initializes a new instance of <see cref="SignedHttpRequestHandler"/>.
/// </summary>
public SignedHttpRequestHandler()
{
_defaultHttpClient.Timeout = TimeSpan.FromSeconds(10);
}

#region SignedHttpRequest creation
/// <summary>
Expand Down Expand Up @@ -1121,6 +1129,17 @@ internal virtual async Task<SecurityKey> ResolvePopKeyFromJweAsync(string jwe, S
/// <returns>A resolved PoP <see cref="SecurityKey"/>.</returns>
internal virtual async Task<SecurityKey> 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<string>());
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)
Expand Down Expand Up @@ -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));
}

/// <summary>
/// Sanitizes the query params to comply with the specification.
/// </summary>
Expand Down
21 changes: 21 additions & 0 deletions ...rosoft.IdentityModel.Protocols.SignedHttpRequest/SignedHttpRequestValidationParameters.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ public class SignedHttpRequestValidationParameters
{
private TimeSpan _signedHttpRequestLifetime = DefaultSignedHttpRequestLifetime;
private TokenHandler _tokenHandler = new JsonWebTokenHandler();
private ICollection<string> _allowedDomainsForJkuRetrieval;

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

/// <summary>
/// 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 <see cref="AllowedDomainsForJkuRetrieval"/>.
/// </summary>
/// <remarks>https://datatracker.ietf.org/doc/html/rfc7800#section-3.5</remarks>
public bool AllowResolvingPopKeyFromJku { get; set; } = false;

/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// Domains should be provided in the following format:
/// "contoso.com"
/// "abc.fabrikam.com"
/// </remarks>
public ICollection<string> AllowedDomainsForJkuRetrieval => _allowedDomainsForJkuRetrieval ??
Interlocked.CompareExchange(ref _allowedDomainsForJkuRetrieval, new List<string>(), null) ?? _allowedDomainsForJkuRetrieval;

/// <summary>
/// Gets or sets the claims to validate if present.
/// </summary>
Expand Down
119 changes: 115 additions & 4 deletions test/Microsoft.IdentityModel.Protocols.SignedHttpRequest.Tests/PopKeyResolvingTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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.");
Expand Down Expand Up @@ -429,6 +431,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuTheoryData
{"mockGetPopKeysFromJkuAsync_return0Keys", null }
}
},
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"),
TestId = "InvalidZeroKeysReturned",
},
Expand All @@ -441,6 +444,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuTheoryData
{"mockGetPopKeysFromJkuAsync_returnNull", null }
}
},
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"),
TestId = "InvalidNullReturned",
},
Expand All @@ -453,8 +457,106 @@ public static TheoryData<ResolvePopKeyTheoryData> 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<string, object>()
{
// 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<string, object>()
{
// 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<string, object>()
{
// 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<string, object>()
{
// 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<string, object>()
{
// to simulate http call and satisfy test requirements
{"mockGetPopKeysFromJkuAsync_return1Key", null }
}
},
JkuSetUrl = "https://localhost/keys",
SignedHttpRequestValidationParameters = { AllowResolvingPopKeyFromJku = true, AllowedDomainsForJkuRetrieval = { "localhost" }},
TestId = "JkuTurnedOnLocalUrl"
}
};
}
}
Expand Down Expand Up @@ -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.");
Expand Down Expand Up @@ -562,6 +664,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
{"mockGetPopKeysFromJkuAsync_return2Keys", null }
}
},
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23021"),
TestId = "InvalidNoKidMatch",
},
Expand All @@ -576,6 +679,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
{"mockGetPopKeysFromJkuAsync_return0Keys", null }
}
},
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"),
TestId = "InvalidZeroKeysReturned",
},
Expand All @@ -589,6 +693,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
{"mockGetPopKeysFromJkuAsync_returnNull", null }
}
},
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23031"),
TestId = "InvalidNullReturned",
},
Expand All @@ -602,6 +707,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
{"mockGetPopKeysFromJkuAsync_return2Keys", null }
}
},
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
ExpectedException = new ExpectedException(typeof(SignedHttpRequestInvalidPopKeyException), "IDX23021"),
TestId = "InvalidNoKidMatch",
},
Expand All @@ -615,6 +721,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
{"mockGetPopKeysFromJkuAsync_return2Keys", null }
}
},
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
TestId = "ValidOneKidMatch",
},
new ResolvePopKeyTheoryData
Expand All @@ -627,6 +734,7 @@ public static TheoryData<ResolvePopKeyTheoryData> ResolvePopKeyFromJkuKidTheoryD
{"mockGetPopKeysFromJkuAsync_return1Key", null }
}
},
SignedHttpRequestValidationParameters = { AllowedDomainsForJkuRetrieval = { _defaultJkuDomain } },
TestId = "ValidKidMatch",
},
};
Expand Down Expand Up @@ -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; }
Expand All @@ -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;
Expand All @@ -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; }
}
Expand Down

0 comments on commit e986e22

Please sign in to comment.