From 6d18e3039fef63b051d1ab714de2f269e088d3b4 Mon Sep 17 00:00:00 2001 From: Dominick Baier Date: Mon, 25 May 2020 16:14:31 +0200 Subject: [PATCH] Add strict JAR mode (#4409) * refactor JwtRequest client to be an interface * add option for strict JAR validation * add tests for strict validation --- src/Directory.Build.targets | 2 +- .../BuilderExtensions/Additional.cs | 8 +- .../Options/IdentityServerOptions.cs | 6 + ...t.cs => DefaultJwtRequestUriHttpClient.cs} | 44 ++-- .../src/Services/IJwtRequestUriHttpClient.cs | 19 ++ .../Default/AuthorizeRequestValidator.cs | 4 +- .../Validation/Default/JwtRequestValidator.cs | 17 +- .../Authorize/JwtRequestAuthorizeTests.cs | 197 +++++++++++++++++- .../Validation/Setup/Factory.cs | 4 +- 9 files changed, 271 insertions(+), 30 deletions(-) rename src/IdentityServer4/src/Services/Default/{JwtRequestUriHttpClient.cs => DefaultJwtRequestUriHttpClient.cs} (50%) create mode 100644 src/IdentityServer4/src/Services/IJwtRequestUriHttpClient.cs diff --git a/src/Directory.Build.targets b/src/Directory.Build.targets index 47397095b7..d87bae6623 100644 --- a/src/Directory.Build.targets +++ b/src/Directory.Build.targets @@ -22,7 +22,7 @@ - + diff --git a/src/IdentityServer4/src/Configuration/DependencyInjection/BuilderExtensions/Additional.cs b/src/IdentityServer4/src/Configuration/DependencyInjection/BuilderExtensions/Additional.cs index 040e5a8d52..1f60b9990d 100644 --- a/src/IdentityServer4/src/Configuration/DependencyInjection/BuilderExtensions/Additional.cs +++ b/src/IdentityServer4/src/Configuration/DependencyInjection/BuilderExtensions/Additional.cs @@ -10,6 +10,7 @@ using System; using System.Net.Http; using IdentityServer4; +using IdentityServer4.Configuration; using Microsoft.Extensions.Logging; namespace Microsoft.Extensions.DependencyInjection @@ -398,14 +399,15 @@ public static IHttpClientBuilder AddJwtRequestUriHttpClient(this IIdentityServer client.Timeout = TimeSpan.FromSeconds(IdentityServerConstants.HttpClients.DefaultTimeoutSeconds); }); } - - builder.Services.AddTransient(s => + + builder.Services.AddTransient(s => { var httpClientFactory = s.GetRequiredService(); var httpClient = httpClientFactory.CreateClient(name); var loggerFactory = s.GetRequiredService(); + var options = s.GetRequiredService(); - return new JwtRequestUriHttpClient(httpClient, loggerFactory); + return new DefaultJwtRequestUriHttpClient(httpClient, options, loggerFactory); }); return httpBuilder; diff --git a/src/IdentityServer4/src/Configuration/DependencyInjection/Options/IdentityServerOptions.cs b/src/IdentityServer4/src/Configuration/DependencyInjection/Options/IdentityServerOptions.cs index 50d040f626..cab2a3fd55 100644 --- a/src/IdentityServer4/src/Configuration/DependencyInjection/Options/IdentityServerOptions.cs +++ b/src/IdentityServer4/src/Configuration/DependencyInjection/Options/IdentityServerOptions.cs @@ -40,6 +40,12 @@ public class IdentityServerOptions /// Specifies whether scopes in JWTs are emitted as array or string /// public bool EmitScopesAsSpaceDelimitedStringInJwt { get; set; } = false; + + /// + /// Specifies whether the JWT typ and content-type for JWT secured authorization requests is checked according to IETF spec. + /// This might break older OIDC conformant request objects. + /// + public bool StrictJarValidation { get; set; } = false; /// /// Gets or sets the endpoint configuration. diff --git a/src/IdentityServer4/src/Services/Default/JwtRequestUriHttpClient.cs b/src/IdentityServer4/src/Services/Default/DefaultJwtRequestUriHttpClient.cs similarity index 50% rename from src/IdentityServer4/src/Services/Default/JwtRequestUriHttpClient.cs rename to src/IdentityServer4/src/Services/Default/DefaultJwtRequestUriHttpClient.cs index 465fb488e3..e894919af7 100644 --- a/src/IdentityServer4/src/Services/Default/JwtRequestUriHttpClient.cs +++ b/src/IdentityServer4/src/Services/Default/DefaultJwtRequestUriHttpClient.cs @@ -2,38 +2,39 @@ // Licensed under the Apache License, Version 2.0. See LICENSE in the project root for license information. +using System; using IdentityServer4.Models; using Microsoft.Extensions.Logging; using System.Net.Http; using System.Threading.Tasks; +using IdentityModel; +using IdentityServer4.Configuration; namespace IdentityServer4.Services { /// - /// Models making HTTP requests for JWTs from the authorize endpoint. + /// Default JwtRequest client /// - public class JwtRequestUriHttpClient + public class DefaultJwtRequestUriHttpClient : IJwtRequestUriHttpClient { private readonly HttpClient _client; - private readonly ILogger _logger; + private readonly IdentityServerOptions _options; + private readonly ILogger _logger; /// - /// Constructor for DefaultJwtRequestUriHttpClient. + /// ctor /// - /// - /// - public JwtRequestUriHttpClient(HttpClient client, ILoggerFactory loggerFactory) + /// An HTTP client + /// The logger factory + public DefaultJwtRequestUriHttpClient(HttpClient client, IdentityServerOptions options, ILoggerFactory loggerFactory) { _client = client; - _logger = loggerFactory.CreateLogger(); + _options = options; + _logger = loggerFactory.CreateLogger(); } - /// - /// Gets a JWT from the url. - /// - /// - /// - /// + + /// public async Task GetJwtAsync(string url, Client client) { var req = new HttpRequestMessage(HttpMethod.Get, url); @@ -42,16 +43,23 @@ public async Task GetJwtAsync(string url, Client client) var response = await _client.SendAsync(req); if (response.StatusCode == System.Net.HttpStatusCode.OK) { - _logger.LogDebug("Success http response from jwt url {url}", url); + if (_options.StrictJarValidation) + { + if (!string.Equals(response.Content.Headers.ContentType.MediaType, + $"application/{JwtClaimTypes.JwtTypes.AuthorizationRequest}", StringComparison.Ordinal)) + { + _logger.LogError("Invalid content type {type} from jwt url {url}", response.Content.Headers.ContentType.MediaType, url); + return null; + } + } - // todo: check for content-type of "application/oauth.authz.req+jwt"? - // this might break OIDC's + _logger.LogDebug("Success http response from jwt url {url}", url); + var json = await response.Content.ReadAsStringAsync(); return json; } _logger.LogError("Invalid http status code {status} from jwt url {url}", response.StatusCode, url); - return null; } } diff --git a/src/IdentityServer4/src/Services/IJwtRequestUriHttpClient.cs b/src/IdentityServer4/src/Services/IJwtRequestUriHttpClient.cs new file mode 100644 index 0000000000..3d3e4ae9ab --- /dev/null +++ b/src/IdentityServer4/src/Services/IJwtRequestUriHttpClient.cs @@ -0,0 +1,19 @@ +using System.Threading.Tasks; +using IdentityServer4.Models; + +namespace IdentityServer4.Services +{ + /// + /// Models making HTTP requests for JWTs from the authorize endpoint. + /// + public interface IJwtRequestUriHttpClient + { + /// + /// Gets a JWT from the url. + /// + /// + /// + /// + Task GetJwtAsync(string url, Client client); + } +} \ No newline at end of file diff --git a/src/IdentityServer4/src/Validation/Default/AuthorizeRequestValidator.cs b/src/IdentityServer4/src/Validation/Default/AuthorizeRequestValidator.cs index 8205e378bf..68c1ac9693 100644 --- a/src/IdentityServer4/src/Validation/Default/AuthorizeRequestValidator.cs +++ b/src/IdentityServer4/src/Validation/Default/AuthorizeRequestValidator.cs @@ -27,7 +27,7 @@ internal class AuthorizeRequestValidator : IAuthorizeRequestValidator private readonly IResourceValidator _resourceValidator; private readonly IUserSession _userSession; private readonly JwtRequestValidator _jwtRequestValidator; - private readonly JwtRequestUriHttpClient _jwtRequestUriHttpClient; + private readonly IJwtRequestUriHttpClient _jwtRequestUriHttpClient; private readonly ILogger _logger; private readonly ResponseTypeEqualityComparer @@ -41,7 +41,7 @@ public AuthorizeRequestValidator( IResourceValidator resourceValidator, IUserSession userSession, JwtRequestValidator jwtRequestValidator, - JwtRequestUriHttpClient jwtRequestUriHttpClient, + IJwtRequestUriHttpClient jwtRequestUriHttpClient, ILogger logger) { _options = options; diff --git a/src/IdentityServer4/src/Validation/Default/JwtRequestValidator.cs b/src/IdentityServer4/src/Validation/Default/JwtRequestValidator.cs index d04993e48a..38cea62036 100644 --- a/src/IdentityServer4/src/Validation/Default/JwtRequestValidator.cs +++ b/src/IdentityServer4/src/Validation/Default/JwtRequestValidator.cs @@ -8,6 +8,7 @@ using System.Linq; using System.Threading.Tasks; using IdentityModel; +using IdentityServer4.Configuration; using IdentityServer4.Extensions; using IdentityServer4.Models; using Microsoft.AspNetCore.Http; @@ -56,13 +57,20 @@ protected string AudienceUri /// The logger /// protected readonly ILogger Logger; + + /// + /// The optione + /// + protected readonly IdentityServerOptions Options; /// /// Instantiates an instance of private_key_jwt secret validator /// - public JwtRequestValidator(IHttpContextAccessor contextAccessor, ILogger logger) + public JwtRequestValidator(IHttpContextAccessor contextAccessor, IdentityServerOptions options, ILogger logger) { _httpContextAccessor = contextAccessor; + + Options = options; Logger = logger; } @@ -169,10 +177,13 @@ protected virtual Task ValidateJwtAsync(string jwtTokenString, RequireExpirationTime = true }; + if (Options.StrictJarValidation) + { + tokenValidationParameters.ValidTypes = new[] { JwtClaimTypes.JwtTypes.AuthorizationRequest }; + } + Handler.ValidateToken(jwtTokenString, tokenValidationParameters, out var token); - // todo: add validation for "typ" header - return Task.FromResult((JwtSecurityToken)token); } diff --git a/src/IdentityServer4/test/IdentityServer.IntegrationTests/Endpoints/Authorize/JwtRequestAuthorizeTests.cs b/src/IdentityServer4/test/IdentityServer.IntegrationTests/Endpoints/Authorize/JwtRequestAuthorizeTests.cs index baca2bce86..01206f02f1 100644 --- a/src/IdentityServer4/test/IdentityServer.IntegrationTests/Endpoints/Authorize/JwtRequestAuthorizeTests.cs +++ b/src/IdentityServer4/test/IdentityServer.IntegrationTests/Endpoints/Authorize/JwtRequestAuthorizeTests.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.IdentityModel.Tokens.Jwt; using System.Net.Http; +using System.Net.Http.Headers; using System.Security.Claims; using System.Security.Cryptography.X509Certificates; using System.Threading.Tasks; @@ -169,7 +170,7 @@ public JwtRequestAuthorizeTests() _mockPipeline.Initialize(); } - string CreateRequestJwt(string issuer, string audience, SigningCredentials credential, Claim[] claims) + string CreateRequestJwt(string issuer, string audience, SigningCredentials credential, Claim[] claims, bool setJwtTyp = false) { var handler = new JwtSecurityTokenHandler(); handler.OutboundClaimTypeMap.Clear(); @@ -180,6 +181,11 @@ string CreateRequestJwt(string issuer, string audience, SigningCredentials crede signingCredentials: credential, subject: Identity.Create("pwd", claims)); + if (setJwtTyp) + { + token.Header["typ"] = JwtClaimTypes.JwtTypes.AuthorizationRequest; + } + return handler.WriteToken(token); } @@ -346,6 +352,93 @@ public async Task authorize_should_accept_valid_JWT_request_object_parameters_us _mockPipeline.LoginRequest.RequestObjectValues["foo"].Should().Be("123foo"); } + [Fact] + [Trait("Category", Category)] + public async Task correct_jwt_typ_should_pass_strict_validation() + { + _mockPipeline.Options.StrictJarValidation = true; + + var requestJwt = CreateRequestJwt( + issuer: _client.ClientId, + audience: IdentityServerPipeline.BaseUrl, + credential: new SigningCredentials(_rsaKey, "RS256"), + claims: new[] { + new Claim("client_id", _client.ClientId), + new Claim("response_type", "id_token"), + new Claim("scope", "openid profile"), + new Claim("state", "123state"), + new Claim("nonce", "123nonce"), + new Claim("redirect_uri", "https://client/callback"), + new Claim("acr_values", "acr_1 acr_2 tenant:tenant_value idp:idp_value"), + new Claim("login_hint", "login_hint_value"), + new Claim("display", "popup"), + new Claim("ui_locales", "ui_locale_value"), + new Claim("foo", "123foo"), + }, setJwtTyp: true); + + var url = _mockPipeline.CreateAuthorizeUrl( + clientId: _client.ClientId, + responseType: "id_token", + extra: new + { + request = requestJwt + }); + var response = await _mockPipeline.BrowserClient.GetAsync(url); + + _mockPipeline.LoginRequest.Should().NotBeNull(); + _mockPipeline.LoginRequest.Client.ClientId.Should().Be(_client.ClientId); + _mockPipeline.LoginRequest.DisplayMode.Should().Be("popup"); + _mockPipeline.LoginRequest.UiLocales.Should().Be("ui_locale_value"); + _mockPipeline.LoginRequest.IdP.Should().Be("idp_value"); + _mockPipeline.LoginRequest.Tenant.Should().Be("tenant_value"); + _mockPipeline.LoginRequest.LoginHint.Should().Be("login_hint_value"); + _mockPipeline.LoginRequest.AcrValues.Should().BeEquivalentTo(new string[] { "acr_2", "acr_1" }); + + _mockPipeline.LoginRequest.Parameters.AllKeys.Should().Contain("foo"); + _mockPipeline.LoginRequest.Parameters["foo"].Should().Be("123foo"); + + _mockPipeline.LoginRequest.RequestObjectValues.Count.Should().Be(11); + _mockPipeline.LoginRequest.RequestObjectValues.Should().ContainKey("foo"); + _mockPipeline.LoginRequest.RequestObjectValues["foo"].Should().Be("123foo"); + } + + [Fact] + [Trait("Category", Category)] + public async Task missing_jwt_typ_should_error() + { + _mockPipeline.Options.StrictJarValidation = true; + + var requestJwt = CreateRequestJwt( + issuer: _client.ClientId, + audience: IdentityServerPipeline.BaseUrl, + credential: new SigningCredentials(_rsaKey, "RS256"), + claims: new[] { + new Claim("client_id", _client.ClientId), + new Claim("response_type", "id_token"), + new Claim("scope", "openid profile"), + new Claim("state", "123state"), + new Claim("nonce", "123nonce"), + new Claim("redirect_uri", "https://client/callback"), + new Claim("acr_values", "acr_1 acr_2 tenant:tenant_value idp:idp_value"), + new Claim("login_hint", "login_hint_value"), + new Claim("display", "popup"), + new Claim("ui_locales", "ui_locale_value"), + new Claim("foo", "123foo"), + }); + + var url = _mockPipeline.CreateAuthorizeUrl( + clientId: _client.ClientId, + responseType: "id_token", + extra: new + { + request = requestJwt + }); + var response = await _mockPipeline.BrowserClient.GetAsync(url); + + _mockPipeline.ErrorMessage.Error.Should().Be("invalid_request_object"); + _mockPipeline.LoginRequest.Should().BeNull(); + } + [Fact] [Trait("Category", Category)] public async Task mismatch_in_jwt_values_should_error() @@ -838,6 +931,108 @@ public async Task authorize_should_accept_request_uri_with_valid_jwt() _mockPipeline.JwtRequestMessageHandler.InvokeWasCalled.Should().BeTrue(); } + [Fact] + [Trait("Category", Category)] + public async Task authorize_should_accept_request_uri_with_valid_jwt_and_strict_validation() + { + _mockPipeline.Options.Endpoints.EnableJwtRequestUri = true; + _mockPipeline.Options.StrictJarValidation = true; + + var requestJwt = CreateRequestJwt( + issuer: _client.ClientId, + audience: IdentityServerPipeline.BaseUrl, + credential: new X509SigningCredentials(TestCert.Load()), + claims: new[] { + new Claim("client_id", _client.ClientId), + new Claim("response_type", "id_token"), + new Claim("scope", "openid profile"), + new Claim("state", "123state"), + new Claim("nonce", "123nonce"), + new Claim("redirect_uri", "https://client/callback"), + new Claim("acr_values", "acr_1 acr_2 tenant:tenant_value idp:idp_value"), + new Claim("login_hint", "login_hint_value"), + new Claim("display", "popup"), + new Claim("ui_locales", "ui_locale_value"), + new Claim("foo", "123foo"), + }, setJwtTyp: true); + _mockPipeline.JwtRequestMessageHandler.OnInvoke = req => + { + req.RequestUri.Should().Be(new Uri("http://client_jwt")); + return Task.CompletedTask; + }; + _mockPipeline.JwtRequestMessageHandler.Response.Content = new StringContent(requestJwt); + _mockPipeline.JwtRequestMessageHandler.Response.Content.Headers.ContentType = new MediaTypeHeaderValue($"application/{JwtClaimTypes.JwtTypes.AuthorizationRequest}"); + + + var url = _mockPipeline.CreateAuthorizeUrl( + clientId: _client.ClientId, + responseType: "id_token", + extra: new + { + request_uri = "http://client_jwt" + }); + var response = await _mockPipeline.BrowserClient.GetAsync(url); + + _mockPipeline.LoginRequest.Should().NotBeNull(); + _mockPipeline.LoginRequest.Client.ClientId.Should().Be(_client.ClientId); + _mockPipeline.LoginRequest.DisplayMode.Should().Be("popup"); + _mockPipeline.LoginRequest.UiLocales.Should().Be("ui_locale_value"); + _mockPipeline.LoginRequest.IdP.Should().Be("idp_value"); + _mockPipeline.LoginRequest.Tenant.Should().Be("tenant_value"); + _mockPipeline.LoginRequest.LoginHint.Should().Be("login_hint_value"); + _mockPipeline.LoginRequest.AcrValues.Should().BeEquivalentTo(new string[] { "acr_2", "acr_1" }); + _mockPipeline.LoginRequest.Parameters.AllKeys.Should().Contain("foo"); + _mockPipeline.LoginRequest.Parameters["foo"].Should().Be("123foo"); + + _mockPipeline.JwtRequestMessageHandler.InvokeWasCalled.Should().BeTrue(); + } + + [Fact] + [Trait("Category", Category)] + public async Task authorize_should_reject_request_uri_with_valid_jwt_and_strict_validation_but_invalid_content_type() + { + _mockPipeline.Options.Endpoints.EnableJwtRequestUri = true; + _mockPipeline.Options.StrictJarValidation = true; + + var requestJwt = CreateRequestJwt( + issuer: _client.ClientId, + audience: IdentityServerPipeline.BaseUrl, + credential: new X509SigningCredentials(TestCert.Load()), + claims: new[] { + new Claim("client_id", _client.ClientId), + new Claim("response_type", "id_token"), + new Claim("scope", "openid profile"), + new Claim("state", "123state"), + new Claim("nonce", "123nonce"), + new Claim("redirect_uri", "https://client/callback"), + new Claim("acr_values", "acr_1 acr_2 tenant:tenant_value idp:idp_value"), + new Claim("login_hint", "login_hint_value"), + new Claim("display", "popup"), + new Claim("ui_locales", "ui_locale_value"), + new Claim("foo", "123foo"), + }, setJwtTyp: true); + _mockPipeline.JwtRequestMessageHandler.OnInvoke = req => + { + req.RequestUri.Should().Be(new Uri("http://client_jwt")); + return Task.CompletedTask; + }; + _mockPipeline.JwtRequestMessageHandler.Response.Content = new StringContent(requestJwt); + + + var url = _mockPipeline.CreateAuthorizeUrl( + clientId: _client.ClientId, + responseType: "id_token", + extra: new + { + request_uri = "http://client_jwt" + }); + var response = await _mockPipeline.BrowserClient.GetAsync(url); + + _mockPipeline.ErrorMessage.Error.Should().Be("invalid_request_uri"); + _mockPipeline.LoginRequest.Should().BeNull(); + _mockPipeline.JwtRequestMessageHandler.InvokeWasCalled.Should().BeTrue(); + } + [Fact] [Trait("Category", Category)] public async Task request_uri_response_returns_500_should_fail() diff --git a/src/IdentityServer4/test/IdentityServer.UnitTests/Validation/Setup/Factory.cs b/src/IdentityServer4/test/IdentityServer.UnitTests/Validation/Setup/Factory.cs index 9a7b1c6d8f..3799c6cd16 100644 --- a/src/IdentityServer4/test/IdentityServer.UnitTests/Validation/Setup/Factory.cs +++ b/src/IdentityServer4/test/IdentityServer.UnitTests/Validation/Setup/Factory.cs @@ -166,7 +166,7 @@ public static AuthorizeRequestValidator CreateAuthorizeRequestValidator( IRedirectUriValidator uriValidator = null, IResourceValidator resourceValidator = null, JwtRequestValidator jwtRequestValidator = null, - JwtRequestUriHttpClient jwtRequestUriHttpClient = null) + IJwtRequestUriHttpClient jwtRequestUriHttpClient = null) { if (options == null) { @@ -205,7 +205,7 @@ public static AuthorizeRequestValidator CreateAuthorizeRequestValidator( if (jwtRequestUriHttpClient == null) { - jwtRequestUriHttpClient = new JwtRequestUriHttpClient(new HttpClient(new NetworkHandler(new Exception("no jwt request uri response configured"))), new LoggerFactory()); + jwtRequestUriHttpClient = new DefaultJwtRequestUriHttpClient(new HttpClient(new NetworkHandler(new Exception("no jwt request uri response configured"))), options, new LoggerFactory()); }