From 5ad536dad9315340ead52c19fdc72a2dfbfdc28f Mon Sep 17 00:00:00 2001 From: eric Date: Mon, 9 Aug 2021 18:22:02 +0200 Subject: [PATCH] fix: check PKCE is used for FAPI using PAR authentication request fixes gravitee-io/issues#5973 --- .../handler/common/utils/ConstantKeys.java | 2 ++ .../gateway/handler/oauth2/OAuth2Provider.java | 2 +- .../authorization/AuthorizationEndpoint.java | 5 ++++- ...thorizationRequestParseParametersHandler.java | 16 +++++++++------- ...rizationRequestParseRequestObjectHandler.java | 7 ++++--- .../resources/request/TokenRequestFactory.java | 1 + .../code/AuthorizationCodeTokenGranter.java | 5 +++-- .../oauth2/service/request/TokenRequest.java | 1 - .../endpoint/AuthorizationEndpointTest.java | 2 +- .../oauth2/api/model/JdbcAuthorizationCode.java | 1 + .../liquibase/changelogs/v3_11_0/schema-par.yml | 2 +- .../api/AuthorizationCodeRepositoryTest.java | 3 ++- 12 files changed, 29 insertions(+), 18 deletions(-) diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-common/src/main/java/io/gravitee/am/gateway/handler/common/utils/ConstantKeys.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-common/src/main/java/io/gravitee/am/gateway/handler/common/utils/ConstantKeys.java index 0f63327346..34312c983d 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-common/src/main/java/io/gravitee/am/gateway/handler/common/utils/ConstantKeys.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-common/src/main/java/io/gravitee/am/gateway/handler/common/utils/ConstantKeys.java @@ -99,4 +99,6 @@ public interface ConstantKeys { String REQUEST_OBJECT_KEY = "requestObject"; // identifier of the Pushed Authorization Parameters String REQUEST_URI_ID_KEY = "requestUriId"; + String REQUEST_OBJECT_FROM_URI = "request-object-from-uri"; + } diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/OAuth2Provider.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/OAuth2Provider.java index 50efd133a1..7100f2038a 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/OAuth2Provider.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/OAuth2Provider.java @@ -213,7 +213,7 @@ private void initRouter() { .handler(authenticationFlowHandler.create()) .handler(new AuthorizationRequestResolveHandler()) .handler(new AuthorizationRequestEndUserConsentHandler(userConsentService)) - .handler(new AuthorizationEndpoint(flow, thymeleafTemplateEngine, parService)) + .handler(new AuthorizationEndpoint(flow, thymeleafTemplateEngine, parService, domain)) .failureHandler(new AuthorizationRequestFailureHandler(openIDDiscoveryService, jwtService, jweService)); // Authorization consent endpoint diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/endpoint/authorization/AuthorizationEndpoint.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/endpoint/authorization/AuthorizationEndpoint.java index f68284c582..a2b4c1c6d0 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/endpoint/authorization/AuthorizationEndpoint.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/endpoint/authorization/AuthorizationEndpoint.java @@ -24,6 +24,7 @@ import io.gravitee.am.gateway.handler.oauth2.service.request.AuthorizationRequest; import io.gravitee.am.gateway.handler.oauth2.service.response.AuthorizationResponse; import io.gravitee.am.gateway.handler.oidc.service.flow.Flow; +import io.gravitee.am.model.Domain; import io.gravitee.am.model.oidc.Client; import io.gravitee.common.http.HttpHeaders; import io.gravitee.common.http.MediaType; @@ -55,10 +56,12 @@ public class AuthorizationEndpoint implements Handler { private final Flow flow; private final ThymeleafTemplateEngine engine; private final PushedAuthorizationRequestService parService; + private final Domain domain; - public AuthorizationEndpoint(Flow flow, ThymeleafTemplateEngine engine, PushedAuthorizationRequestService parService) { + public AuthorizationEndpoint(Flow flow, ThymeleafTemplateEngine engine, PushedAuthorizationRequestService parService, Domain domain) { this.flow = flow; this.engine = engine; + this.domain = domain; this.parService = parService; } diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/handler/authorization/AuthorizationRequestParseParametersHandler.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/handler/authorization/AuthorizationRequestParseParametersHandler.java index 17e84bc07e..564b005378 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/handler/authorization/AuthorizationRequestParseParametersHandler.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/handler/authorization/AuthorizationRequestParseParametersHandler.java @@ -46,12 +46,10 @@ import java.net.MalformedURLException; import java.net.URISyntaxException; import java.net.URL; -import java.util.Arrays; -import java.util.Collections; -import java.util.Date; -import java.util.List; +import java.util.*; import static io.gravitee.am.gateway.handler.common.utils.ConstantKeys.PROVIDER_METADATA_CONTEXT_KEY; +import static io.gravitee.am.gateway.handler.common.utils.ConstantKeys.REQUEST_OBJECT_FROM_URI; import static io.gravitee.am.gateway.handler.common.vertx.utils.UriBuilderRequest.CONTEXT_PATH; import static io.gravitee.am.gateway.handler.oauth2.resources.handler.authorization.ParamUtils.getOAuthParameter; import static io.gravitee.am.service.utils.ResponseTypeUtils.requireNonce; @@ -145,7 +143,8 @@ private void parsePKCEParameter(RoutingContext context, Client client) { throw new InvalidRequestException("Missing parameter: code_challenge"); } - if (codeChallenge == null && client.isForcePKCE()) { + final boolean pkceRequiredByFapi = this.domain.usePlainFapiProfile() && Optional.ofNullable((Boolean)context.get(REQUEST_OBJECT_FROM_URI)).orElse(Boolean.FALSE); + if (codeChallenge == null && (client.isForcePKCE() || pkceRequiredByFapi)) { throw new InvalidRequestException("Missing parameter: code_challenge"); } @@ -153,8 +152,11 @@ private void parsePKCEParameter(RoutingContext context, Client client) { if (codeChallengeMethod != null) { // https://tools.ietf.org/html/rfc7636#section-4.2 // It must be plain or S256 - if (!CodeChallengeMethod.S256.equalsIgnoreCase(codeChallengeMethod) && - !CodeChallengeMethod.PLAIN.equalsIgnoreCase(codeChallengeMethod)) { + // For FAPI, only S256 is allowed for PKCE + // https://openid.net/specs/openid-financial-api-part-2-1_0.html#authorization-server (point 18) + if ((this.domain.usePlainFapiProfile() && !CodeChallengeMethod.S256.equalsIgnoreCase(codeChallengeMethod)) || + (!CodeChallengeMethod.S256.equalsIgnoreCase(codeChallengeMethod) && + !CodeChallengeMethod.PLAIN.equalsIgnoreCase(codeChallengeMethod))) { throw new InvalidRequestException("Invalid parameter: code_challenge_method"); } } else { diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/handler/authorization/AuthorizationRequestParseRequestObjectHandler.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/handler/authorization/AuthorizationRequestParseRequestObjectHandler.java index 28904f83b5..99e0194bde 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/handler/authorization/AuthorizationRequestParseRequestObjectHandler.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/resources/handler/authorization/AuthorizationRequestParseRequestObjectHandler.java @@ -52,8 +52,7 @@ * @author David BRASSELY (david.brassely at graviteesource.com) * @author GraviteeSource Team */ -public class AuthorizationRequestParseRequestObjectHandler implements Handler { - private static final String REQUEST_OBJECT_FROM_URI = "ro-from-uri"; +public class AuthorizationRequestParseRequestObjectHandler extends AbstractAuthorizationRequestHandler implements Handler { private static final String HTTPS_SCHEME = "https"; // When the request parameter is used, the OpenID Connect request parameter values contained in the JWT supersede those passed using the OAuth 2.0 request syntax. @@ -67,7 +66,9 @@ public class AuthorizationRequestParseRequestObjectHandler implements Handler(Arrays.asList(scope.split("\\s+"))) : null); tokenRequest.setAdditionalParameters(extractAdditionalParameters(request)); + return tokenRequest; } diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/service/granter/code/AuthorizationCodeTokenGranter.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/service/granter/code/AuthorizationCodeTokenGranter.java index 0618917f34..e3c4ab2e9d 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/service/granter/code/AuthorizationCodeTokenGranter.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/service/granter/code/AuthorizationCodeTokenGranter.java @@ -29,8 +29,9 @@ import io.gravitee.am.gateway.handler.oauth2.service.request.TokenRequestResolver; import io.gravitee.am.gateway.handler.oauth2.service.token.TokenService; import io.gravitee.am.model.AuthenticationFlowContext; -import io.gravitee.am.model.oidc.Client; +import io.gravitee.am.model.Domain; import io.gravitee.am.model.User; +import io.gravitee.am.model.oidc.Client; import io.gravitee.am.repository.oauth2.model.AuthorizationCode; import io.gravitee.am.service.AuthenticationFlowContextService; import io.gravitee.common.util.MultiValueMap; @@ -92,7 +93,7 @@ protected Single parseRequest(TokenRequest tokenRequest, Client cl .onErrorResumeNext(error -> (exitOnError) ? Maybe.error(error) : Maybe.just(new AuthenticationFlowContext())) .map(ctx -> { checkRedirectUris(tokenRequest1, authorizationCode); - checkPKCE(tokenRequest1, authorizationCode); + checkPKCE( tokenRequest1, authorizationCode); // set resource owner tokenRequest1.setSubject(authorizationCode.getSubject()); // set original scopes diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/service/request/TokenRequest.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/service/request/TokenRequest.java index fcbb08b754..0ee9c971ca 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/service/request/TokenRequest.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/main/java/io/gravitee/am/gateway/handler/oauth2/service/request/TokenRequest.java @@ -65,7 +65,6 @@ public class TokenRequest extends OAuth2Request { */ private String requestingPartyToken; - public String getUsername() { return username; } diff --git a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/test/java/io/gravitee/am/gateway/handler/oauth2/resources/endpoint/AuthorizationEndpointTest.java b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/test/java/io/gravitee/am/gateway/handler/oauth2/resources/endpoint/AuthorizationEndpointTest.java index 641136b5e0..2b1cf3cd22 100644 --- a/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/test/java/io/gravitee/am/gateway/handler/oauth2/resources/endpoint/AuthorizationEndpointTest.java +++ b/gravitee-am-gateway/gravitee-am-gateway-handler/gravitee-am-gateway-handler-oidc/src/test/java/io/gravitee/am/gateway/handler/oauth2/resources/endpoint/AuthorizationEndpointTest.java @@ -101,7 +101,7 @@ public class AuthorizationEndpointTest extends RxWebTestBase { public void setUp() throws Exception { super.setUp(); - AuthorizationEndpoint authorizationEndpointHandler = new AuthorizationEndpoint(flow, thymeleafTemplateEngine, parService); + AuthorizationEndpoint authorizationEndpointHandler = new AuthorizationEndpoint(flow, thymeleafTemplateEngine, parService, domain); // set openid provider service OpenIDProviderMetadata openIDProviderMetadata = new OpenIDProviderMetadata(); diff --git a/gravitee-am-repository/gravitee-am-repository-jdbc/src/main/java/io/gravitee/am/repository/jdbc/oauth2/api/model/JdbcAuthorizationCode.java b/gravitee-am-repository/gravitee-am-repository-jdbc/src/main/java/io/gravitee/am/repository/jdbc/oauth2/api/model/JdbcAuthorizationCode.java index 7e5ba7fb7f..30c185df90 100644 --- a/gravitee-am-repository/gravitee-am-repository-jdbc/src/main/java/io/gravitee/am/repository/jdbc/oauth2/api/model/JdbcAuthorizationCode.java +++ b/gravitee-am-repository/gravitee-am-repository-jdbc/src/main/java/io/gravitee/am/repository/jdbc/oauth2/api/model/JdbcAuthorizationCode.java @@ -135,4 +135,5 @@ public String getRequestParameters() { public void setRequestParameters(String requestParameters) { this.requestParameters = requestParameters; } + } diff --git a/gravitee-am-repository/gravitee-am-repository-jdbc/src/main/resources/liquibase/changelogs/v3_11_0/schema-par.yml b/gravitee-am-repository/gravitee-am-repository-jdbc/src/main/resources/liquibase/changelogs/v3_11_0/schema-par.yml index 462a6fc0d9..f660567767 100644 --- a/gravitee-am-repository/gravitee-am-repository-jdbc/src/main/resources/liquibase/changelogs/v3_11_0/schema-par.yml +++ b/gravitee-am-repository/gravitee-am-repository-jdbc/src/main/resources/liquibase/changelogs/v3_11_0/schema-par.yml @@ -1,6 +1,6 @@ databaseChangeLog: - changeSet: - id: 3.11.0 + id: 3.11.0-PAR-Table author: GraviteeSource Team changes: diff --git a/gravitee-am-repository/gravitee-am-repository-tests/src/test/java/io/gravitee/am/repository/oauth2/api/AuthorizationCodeRepositoryTest.java b/gravitee-am-repository/gravitee-am-repository-tests/src/test/java/io/gravitee/am/repository/oauth2/api/AuthorizationCodeRepositoryTest.java index 9e5f14d231..58ec2025f2 100644 --- a/gravitee-am-repository/gravitee-am-repository-tests/src/test/java/io/gravitee/am/repository/oauth2/api/AuthorizationCodeRepositoryTest.java +++ b/gravitee-am-repository/gravitee-am-repository-tests/src/test/java/io/gravitee/am/repository/oauth2/api/AuthorizationCodeRepositoryTest.java @@ -44,7 +44,8 @@ public void shouldStoreCode() { testObserver.assertComplete(); testObserver.assertNoErrors(); - testObserver.assertValue(authorizationCode1 -> authorizationCode1.getCode().equals(code) && authorizationCode1.getContextVersion() == 1); + testObserver.assertValue(authorizationCode1 -> authorizationCode1.getCode().equals(code) + && authorizationCode1.getContextVersion() == 1); } @Test