From 5293257e6e1baf57cf7cd4194305235b427fad6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Wed, 21 Oct 2020 19:55:27 +0200 Subject: [PATCH] Add a new option allowing to make PKCE mandatory --- samples/Mvc.Server/Startup.cs | 3 + .../OpenIddictServerBuilder.cs | 9 ++ ...OpenIddictServerHandlers.Authentication.cs | 27 +++- .../OpenIddictServerHandlers.Exchange.cs | 56 +++++++- .../OpenIddictServerOptions.cs | 8 ++ ...ctServerIntegrationTests.Authentication.cs | 123 ++++++++++++------ ...enIddictServerIntegrationTests.Exchange.cs | 79 +++++++++++ .../OpenIddictServerBuilderTests.cs | 16 +++ 8 files changed, 272 insertions(+), 49 deletions(-) diff --git a/samples/Mvc.Server/Startup.cs b/samples/Mvc.Server/Startup.cs index ba38ee1a..e125ba21 100644 --- a/samples/Mvc.Server/Startup.cs +++ b/samples/Mvc.Server/Startup.cs @@ -103,6 +103,9 @@ namespace Mvc.Server options.AddDevelopmentEncryptionCertificate() .AddDevelopmentSigningCertificate(); + // Force client applications to use Proof Key for Code Exchange (PKCE). + options.RequireProofKeyForCodeExchange(); + // Register the ASP.NET Core host and configure the ASP.NET Core-specific options. options.UseAspNetCore() .EnableStatusCodePagesIntegration() diff --git a/src/OpenIddict.Server/OpenIddictServerBuilder.cs b/src/OpenIddict.Server/OpenIddictServerBuilder.cs index 6a2b3b46..f866c607 100644 --- a/src/OpenIddict.Server/OpenIddictServerBuilder.cs +++ b/src/OpenIddict.Server/OpenIddictServerBuilder.cs @@ -1739,6 +1739,15 @@ namespace Microsoft.Extensions.DependencyInjection return Configure(options => options.Scopes.UnionWith(scopes)); } + /// + /// Configures OpenIddict to force client applications to use Proof Key for Code Exchange + /// (PKCE) when requesting an authorization code (e.g when using the code or hybrid flows). + /// When enforced, authorization requests that lack the code_challenge will be rejected. + /// + /// The . + public OpenIddictServerBuilder RequireProofKeyForCodeExchange() + => Configure(options => options.RequireProofKeyForCodeExchange = true); + /// /// Sets the access token lifetime, after which client applications must retrieve /// a new access token by making a grant_type=refresh_token token request diff --git a/src/OpenIddict.Server/OpenIddictServerHandlers.Authentication.cs b/src/OpenIddict.Server/OpenIddictServerHandlers.Authentication.cs index 2c4334e3..ec802f0c 100644 --- a/src/OpenIddict.Server/OpenIddictServerHandlers.Authentication.cs +++ b/src/OpenIddict.Server/OpenIddictServerHandlers.Authentication.cs @@ -47,7 +47,7 @@ namespace OpenIddict.Server ValidateScopeParameter.Descriptor, ValidateNonceParameter.Descriptor, ValidatePromptParameter.Descriptor, - ValidateCodeChallengeParameters.Descriptor, + ValidateProofKeyForCodeExchangeParameters.Descriptor, ValidateClientId.Descriptor, ValidateClientType.Descriptor, ValidateClientRedirectUri.Descriptor, @@ -838,16 +838,16 @@ namespace OpenIddict.Server } /// - /// Contains the logic responsible of rejecting authorization requests that don't specify valid code challenge parameters. + /// Contains the logic responsible of rejecting authorization requests that don't specify valid PKCE parameters. /// - public class ValidateCodeChallengeParameters : IOpenIddictServerHandler + public class ValidateProofKeyForCodeExchangeParameters : IOpenIddictServerHandler { /// /// Gets the default descriptor definition assigned to this handler. /// public static OpenIddictServerHandlerDescriptor Descriptor { get; } = OpenIddictServerHandlerDescriptor.CreateBuilder() - .UseSingletonHandler() + .UseSingletonHandler() .SetOrder(ValidatePromptParameter.Descriptor.Order + 1_000) .SetType(OpenIddictServerHandlerType.BuiltIn) .Build(); @@ -860,6 +860,23 @@ namespace OpenIddict.Server throw new ArgumentNullException(nameof(context)); } + // If OpenIddict was configured to require PKCE, reject the request if the code challenge + // is missing and if an authorization code was requested by the client application. + if (context.Options.RequireProofKeyForCodeExchange && + context.Request.HasResponseType(ResponseTypes.Code) && + string.IsNullOrEmpty(context.Request.CodeChallenge)) + { + context.Logger.LogError(SR.GetResourceString(SR.ID6033), Parameters.CodeChallenge); + + context.Reject( + error: Errors.InvalidRequest, + description: context.Localizer[SR.ID2029, Parameters.CodeChallenge]); + + return default; + } + + // At this point, stop validating the PKCE parameters if both the + // code_challenge and code_challenge_method parameter are missing. if (string.IsNullOrEmpty(context.Request.CodeChallenge) && string.IsNullOrEmpty(context.Request.CodeChallengeMethod)) { @@ -954,7 +971,7 @@ namespace OpenIddict.Server = OpenIddictServerHandlerDescriptor.CreateBuilder() .AddFilter() .UseScopedHandler() - .SetOrder(ValidateCodeChallengeParameters.Descriptor.Order + 1_000) + .SetOrder(ValidateProofKeyForCodeExchangeParameters.Descriptor.Order + 1_000) .SetType(OpenIddictServerHandlerType.BuiltIn) .Build(); diff --git a/src/OpenIddict.Server/OpenIddictServerHandlers.Exchange.cs b/src/OpenIddict.Server/OpenIddictServerHandlers.Exchange.cs index 757722f0..5e7a3d77 100644 --- a/src/OpenIddict.Server/OpenIddictServerHandlers.Exchange.cs +++ b/src/OpenIddict.Server/OpenIddictServerHandlers.Exchange.cs @@ -50,7 +50,8 @@ namespace OpenIddict.Server ValidateClientCredentialsParameters.Descriptor, ValidateDeviceCodeParameter.Descriptor, ValidateRefreshTokenParameter.Descriptor, - ValidatePasswordParameters.Descriptor, + ValidateResourceOwnerCredentialsParameters.Descriptor, + ValidateProofKeyForCodeExchangeParameters.Descriptor, ValidateScopes.Descriptor, ValidateClientId.Descriptor, ValidateClientType.Descriptor, @@ -605,14 +606,14 @@ namespace OpenIddict.Server /// Contains the logic responsible of rejecting token requests /// that specify invalid parameters for the password grant type. /// - public class ValidatePasswordParameters : IOpenIddictServerHandler + public class ValidateResourceOwnerCredentialsParameters : IOpenIddictServerHandler { /// /// Gets the default descriptor definition assigned to this handler. /// public static OpenIddictServerHandlerDescriptor Descriptor { get; } = OpenIddictServerHandlerDescriptor.CreateBuilder() - .UseSingletonHandler() + .UseSingletonHandler() .SetOrder(ValidateRefreshTokenParameter.Descriptor.Order + 1_000) .SetType(OpenIddictServerHandlerType.BuiltIn) .Build(); @@ -643,6 +644,53 @@ namespace OpenIddict.Server } } + /// + /// Contains the logic responsible of rejecting token requests that don't specify valid PKCE parameters. + /// + public class ValidateProofKeyForCodeExchangeParameters : IOpenIddictServerHandler + { + /// + /// Gets the default descriptor definition assigned to this handler. + /// + public static OpenIddictServerHandlerDescriptor Descriptor { get; } + = OpenIddictServerHandlerDescriptor.CreateBuilder() + .UseSingletonHandler() + .SetOrder(ValidateResourceOwnerCredentialsParameters.Descriptor.Order + 1_000) + .SetType(OpenIddictServerHandlerType.BuiltIn) + .Build(); + + /// + public ValueTask HandleAsync(ValidateTokenRequestContext context) + { + if (context is null) + { + throw new ArgumentNullException(nameof(context)); + } + + if (!context.Request.IsAuthorizationCodeGrantType()) + { + return default; + } + + // Optimization: the ValidateCodeVerifier event handler automatically rejects grant_type=authorization_code + // requests missing the code_verifier parameter when a challenge was specified in the authorization request. + // That check requires decrypting the authorization code and determining whether a code challenge was set. + // If OpenIddict was configured to require PKCE, this can be potentially avoided by making an early check here. + if (context.Options.RequireProofKeyForCodeExchange && string.IsNullOrEmpty(context.Request.CodeVerifier)) + { + context.Logger.LogError(SR.GetResourceString(SR.ID6033), Parameters.CodeVerifier); + + context.Reject( + error: Errors.InvalidRequest, + description: context.Localizer[SR.ID2029, Parameters.CodeVerifier]); + + return default; + } + + return default; + } + } + /// /// Contains the logic responsible of rejecting authorization requests that use unregistered scopes. /// Note: this handler partially works with the degraded mode but is not used when scope validation is disabled. @@ -665,7 +713,7 @@ namespace OpenIddict.Server = OpenIddictServerHandlerDescriptor.CreateBuilder() .AddFilter() .UseScopedHandler() - .SetOrder(ValidatePasswordParameters.Descriptor.Order + 1_000) + .SetOrder(ValidateProofKeyForCodeExchangeParameters.Descriptor.Order + 1_000) .SetType(OpenIddictServerHandlerType.BuiltIn) .Build(); diff --git a/src/OpenIddict.Server/OpenIddictServerOptions.cs b/src/OpenIddict.Server/OpenIddictServerOptions.cs index b9e3aedb..bd5cdae0 100644 --- a/src/OpenIddict.Server/OpenIddictServerOptions.cs +++ b/src/OpenIddict.Server/OpenIddictServerOptions.cs @@ -306,6 +306,14 @@ namespace OpenIddict.Server /// public HashSet GrantTypes { get; } = new HashSet(StringComparer.Ordinal); + /// + /// Gets or sets a boolean indicating whether PKCE must be used by client applications + /// when requesting an authorization code (e.g when using the code or hybrid flows). + /// If this property is set to , authorization requests that + /// lack the code_challenge will be automatically rejected by OpenIddict. + /// + public bool RequireProofKeyForCodeExchange { get; set; } + /// /// Gets the OAuth 2.0/OpenID Connect response types enabled for this application. /// diff --git a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Authentication.cs b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Authentication.cs index 524e9265..7ec61b37 100644 --- a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Authentication.cs +++ b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Authentication.cs @@ -416,6 +416,67 @@ namespace OpenIddict.Server.IntegrationTests Assert.NotNull(response.IdToken); } + [Fact] + public async Task ValidateAuthorizationRequest_RequestIsRejectedWhenPkceIsRequiredAndCodeChallengeIsMissing() + { + // Arrange + await using var server = await CreateServerAsync(options => + { + options.EnableDegradedMode(); + options.RequireProofKeyForCodeExchange(); + }); + + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/authorize", new OpenIddictRequest + { + ClientId = "Fabrikam", + CodeChallenge = null, + RedirectUri = "http://www.fabrikam.com/path", + ResponseType = ResponseTypes.Code + }); + + // Assert + Assert.Equal(Errors.InvalidRequest, response.Error); + Assert.Equal(SR.FormatID2029(Parameters.CodeChallenge), response.ErrorDescription); + } + + [Fact] + public async Task ValidateAuthorizationRequest_RequestIsValidateWhenPkceIsNotRequiredAndCodeChallengeIsMissing() + { + // Arrange + await using var server = await CreateServerAsync(options => + { + options.EnableDegradedMode(); + + options.AddEventHandler(builder => + builder.UseInlineHandler(context => + { + context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) + .SetClaim(Claims.Subject, "Bob le Magnifique"); + + return default; + })); + }); + + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/authorize", new OpenIddictRequest + { + ClientId = "Fabrikam", + CodeChallenge = null, + RedirectUri = "http://www.fabrikam.com/path", + ResponseType = ResponseTypes.Code + }); + + // Assert + Assert.Null(response.Error); + Assert.Null(response.ErrorDescription); + Assert.NotNull(response.Code); + } + [Theory] [InlineData("id_token")] [InlineData("id_token token")] @@ -579,6 +640,28 @@ namespace OpenIddict.Server.IntegrationTests Assert.Equal(SR.FormatID2032(Parameters.ResponseType), response.ErrorDescription); } + [Fact] + public async Task ValidateAuthorizationRequest_UnsupportedResponseModeCausesAnError() + { + // Arrange + await using var server = await CreateServerAsync(options => options.EnableDegradedMode()); + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/authorize", new OpenIddictRequest + { + ClientId = "Fabrikam", + RedirectUri = "http://www.fabrikam.com/path", + ResponseMode = "unsupported_response_mode", + ResponseType = ResponseTypes.Code, + Scope = Scopes.OpenId + }); + + // Assert + Assert.Equal(Errors.InvalidRequest, response.Error); + Assert.Equal(SR.FormatID2032(Parameters.ResponseMode), response.ErrorDescription); + } + [Fact] public async Task ValidateAuthorizationRequest_RequestIsRejectedWhenUnregisteredScopeIsSpecified() { @@ -2120,45 +2203,5 @@ namespace OpenIddict.Server.IntegrationTests // Assert Assert.Equal("custom_state", response.State); } - - [Fact] - public async Task ApplyAuthorizationResponse_UnsupportedResponseModeCausesAnError() - { - // Note: response_mode validation is deliberately delayed until an authorization response - // is returned to allow implementers to override the ApplyAuthorizationResponse event - // to support custom response modes. To test this scenario, the request is marked - // as validated and a signin grant is applied to return an authorization response. - - // Arrange - await using var server = await CreateServerAsync(options => - { - options.EnableDegradedMode(); - - options.AddEventHandler(builder => - builder.UseInlineHandler(context => - { - context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) - .SetClaim(Claims.Subject, "Bob le Magnifique"); - - return default; - })); - }); - - await using var client = await server.CreateClientAsync(); - - // Act - var response = await client.PostAsync("/connect/authorize", new OpenIddictRequest - { - ClientId = "Fabrikam", - RedirectUri = "http://www.fabrikam.com/path", - ResponseMode = "unsupported_response_mode", - ResponseType = ResponseTypes.Code, - Scope = Scopes.OpenId - }); - - // Assert - Assert.Equal(Errors.InvalidRequest, response.Error); - Assert.Equal(SR.FormatID2032(Parameters.ResponseMode), response.ErrorDescription); - } } } diff --git a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Exchange.cs b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Exchange.cs index dd843446..619b9dbe 100644 --- a/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Exchange.cs +++ b/test/OpenIddict.Server.IntegrationTests/OpenIddictServerIntegrationTests.Exchange.cs @@ -260,6 +260,85 @@ namespace OpenIddict.Server.IntegrationTests Assert.Equal(SR.FormatID2059(Parameters.Username, Parameters.Password), response.ErrorDescription); } + [Fact] + public async Task ValidateTokenRequest_AuthorizationCodeRequestIsRejectedWhenPkceIsRequiredAndCodeVerifierIsMissing() + { + // Arrange + await using var server = await CreateServerAsync(options => + { + options.EnableDegradedMode(); + options.RequireProofKeyForCodeExchange(); + }); + + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/token", new OpenIddictRequest + { + ClientId = "Fabrikam", + Code = "SplxlOBeZQQYbYS6WxSbIA", + CodeVerifier = null, + GrantType = GrantTypes.AuthorizationCode, + RedirectUri = "http://www.fabrikam.com/path" + }); + + // Assert + Assert.Equal(Errors.InvalidRequest, response.Error); + Assert.Equal(SR.FormatID2029(Parameters.CodeVerifier), response.ErrorDescription); + } + + [Fact] + public async Task ValidateTokenRequest_AuthorizationCodeRequestIsValidatedWhenPkceIsNotRequiredAndCodeVerifierIsMissing() + { + // Arrange + await using var server = await CreateServerAsync(options => + { + options.EnableDegradedMode(); + + options.AddEventHandler(builder => + { + builder.UseInlineHandler(context => + { + Assert.Equal("SplxlOBeZQQYbYS6WxSbIA", context.Token); + Assert.Equal(TokenTypeHints.AuthorizationCode, context.TokenType); + + context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) + .SetTokenType(TokenTypeHints.AuthorizationCode) + .SetPresenters("Fabrikam") + .SetClaim(Claims.Subject, "Bob le Bricoleur"); + + return default; + }); + + builder.SetOrder(ValidateIdentityModelToken.Descriptor.Order - 500); + }); + + options.AddEventHandler(builder => + builder.UseInlineHandler(context => + { + context.Principal = new ClaimsPrincipal(new ClaimsIdentity("Bearer")) + .SetClaim(Claims.Subject, "Bob le Magnifique"); + + return default; + })); + }); + + await using var client = await server.CreateClientAsync(); + + // Act + var response = await client.PostAsync("/connect/token", new OpenIddictRequest + { + ClientId = "Fabrikam", + Code = "SplxlOBeZQQYbYS6WxSbIA", + CodeVerifier = null, + GrantType = GrantTypes.AuthorizationCode, + RedirectUri = "http://www.fabrikam.com/path" + }); + + // Assert + Assert.NotNull(response.AccessToken); + } + [Fact] public async Task ValidateTokenRequest_InvalidAuthorizationCodeCausesAnError() { diff --git a/test/OpenIddict.Server.Tests/OpenIddictServerBuilderTests.cs b/test/OpenIddict.Server.Tests/OpenIddictServerBuilderTests.cs index 928a994c..5a7215b6 100644 --- a/test/OpenIddict.Server.Tests/OpenIddictServerBuilderTests.cs +++ b/test/OpenIddict.Server.Tests/OpenIddictServerBuilderTests.cs @@ -608,6 +608,22 @@ namespace OpenIddict.Server.Tests Assert.True(options.DisableAccessTokenEncryption); } + [Fact] + public void RequireProofKeyForCodeExchange_PkceIsEnforced() + { + // Arrange + var services = CreateServices(); + var builder = CreateBuilder(services); + + // Act + builder.RequireProofKeyForCodeExchange(); + + var options = GetOptions(services); + + // Assert + Assert.True(options.RequireProofKeyForCodeExchange); + } + [Fact] public void AddDeviceCodeFlow_AddsDeviceCodeGrantType() {