From 844b97805f415d4aa5dd37e0728e86eee36e497c Mon Sep 17 00:00:00 2001 From: Sergey Beryozkin Date: Fri, 28 Jan 2022 18:23:30 +0000 Subject: [PATCH] Process OIDC code query param only if the state cookie exists --- .../io/quarkus/oidc/OidcTenantConfig.java | 1 - .../runtime/CodeAuthenticationMechanism.java | 130 +++++++++++------- .../keycloak/CustomTenantConfigResolver.java | 48 +++++++ .../it/keycloak/CustomTenantResolver.java | 5 - .../it/keycloak/ProtectedResource3.java | 19 +++ .../io/quarkus/it/keycloak/TenantHttps.java | 4 +- .../src/main/resources/application.properties | 17 +-- .../io/quarkus/it/keycloak/CodeFlowTest.java | 13 +- 8 files changed, 161 insertions(+), 76 deletions(-) create mode 100644 integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java create mode 100644 integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/ProtectedResource3.java diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java index 023314f20896d..a1570bec62e95 100644 --- a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java +++ b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java @@ -699,7 +699,6 @@ public Optional getCookieSuffix() { public void setCookieSuffix(String cookieSuffix) { this.cookieSuffix = Optional.of(cookieSuffix); } - } @ConfigGroup diff --git a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java index 9ac994053d572..92cd20051b47c 100644 --- a/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java +++ b/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java @@ -52,6 +52,7 @@ public class CodeAuthenticationMechanism extends AbstractOidcAuthenticationMecha static final String SESSION_MAX_AGE_PARAM = "session-max-age"; static final Uni VOID_UNI = Uni.createFrom().voidItem(); static final Integer MAX_COOKIE_VALUE_LENGTH = 4096; + static final String NO_OIDC_COOKIES_AVAILABLE = "no_oidc_cookies"; private static final String INTERNAL_IDTOKEN_HEADER = "internal"; private static final Logger LOG = Logger.getLogger(CodeAuthenticationMechanism.class); @@ -63,7 +64,7 @@ public Uni authenticate(RoutingContext context, IdentityProviderManager identityProviderManager, OidcTenantConfig oidcTenantConfig) { final Cookie sessionCookie = context.request().getCookie(getSessionCookieName(oidcTenantConfig)); - // if session already established, try to re-authenticate + // if the session is already established then try to re-authenticate if (sessionCookie != null) { context.put(OidcUtils.SESSION_COOKIE_NAME, sessionCookie.getName()); Uni resolvedContext = resolver.resolveContext(context); @@ -76,21 +77,52 @@ public Uni apply(TenantConfigContext tenantContext) { }); } - final String code = context.request().getParam("code"); - if (code == null) { - return Uni.createFrom().optional(Optional.empty()); + final Cookie stateCookie = context.request().getCookie(getStateCookieName(oidcTenantConfig)); + + // if the state cookie is available then try to complete the code flow and start a new session + if (stateCookie != null) { + String[] parsedStateCookieValue = COOKIE_PATTERN.split(stateCookie.getValue()); + if (!isStateValid(context, parsedStateCookieValue[0])) { + OidcUtils.removeCookie(context, oidcTenantConfig, stateCookie.getName()); + return Uni.createFrom().failure(new AuthenticationCompletionException()); + } + + final String code = context.request().getParam(OidcConstants.CODE_FLOW_CODE); + if (code != null) { + // start a new session by starting the code flow dance + Uni resolvedContext = resolver.resolveContext(context); + return resolvedContext.onItem() + .transformToUni(new Function>() { + @Override + public Uni apply(TenantConfigContext tenantContext) { + return performCodeFlow(identityProviderManager, context, tenantContext, code, stateCookie, + parsedStateCookieValue); + } + }); + } else { + LOG.debug("State cookie is present but neither 'code' nor 'error' query parameter is returned"); + OidcUtils.removeCookie(context, oidcTenantConfig, stateCookie.getName()); + return Uni.createFrom().failure(new AuthenticationCompletionException()); + } } + // return an empty identity - this will lead to a challenge redirecting the user to OpenId Connect provider + // unless it is detected it is a redirect from the provider in which case HTTP 401 will be returned. + context.put(NO_OIDC_COOKIES_AVAILABLE, Boolean.TRUE); + return Uni.createFrom().optional(Optional.empty()); - // start a new session by starting the code flow dance - Uni resolvedContext = resolver.resolveContext(context); - return resolvedContext.onItem() - .transformToUni(new Function>() { - @Override - public Uni apply(TenantConfigContext tenantContext) { - return performCodeFlow(identityProviderManager, context, tenantContext, code); - } - }); + } + private boolean isStateValid(RoutingContext context, String cookieState) { + List values = context.queryParam(OidcConstants.CODE_FLOW_STATE); + // IDP must return a 'state' query parameter and the value of the state cookie must start with this parameter's value + if (values.size() != 1) { + LOG.debug("State parameter can not be empty or multi-valued"); + return false; + } else if (!cookieState.equals(values.get(0))) { + LOG.debug("State cookie value does not match the state query parameter value"); + return false; + } + return true; } private Uni reAuthenticate(Cookie sessionCookie, @@ -198,6 +230,16 @@ public Uni getChallengeInternal(RoutingContext context, TenantCon @Override public Uni apply(Void t) { + + if (context.get(NO_OIDC_COOKIES_AVAILABLE) != null + && context.request().getParam(OidcConstants.CODE_FLOW_CODE) != null + && isRedirectFromProvider(context, configContext)) { + LOG.debug( + "The state cookie is missing but the 'code' is available, authentication has failed"); + return Uni.createFrom().item(new ChallengeData(401, "WWW-Authenticate", "OIDC")); + + } + if (!shouldAutoRedirect(configContext, context)) { // If the client (usually an SPA) wants to handle the redirect manually, then // return status code 499 and WWW-Authenticate header with the 'OIDC' value. @@ -250,43 +292,31 @@ public Uni apply(Void t) { }); } - private Uni performCodeFlow(IdentityProviderManager identityProviderManager, - RoutingContext context, TenantConfigContext configContext, String code) { + private boolean isRedirectFromProvider(RoutingContext context, TenantConfigContext configContext) { + String referer = context.request().getHeader(HttpHeaders.REFERER); + return referer != null && referer.startsWith(configContext.provider.getMetadata().getAuthorizationUri()); + } - Cookie stateCookie = context.getCookie(getStateCookieName(configContext)); + private Uni performCodeFlow(IdentityProviderManager identityProviderManager, + RoutingContext context, TenantConfigContext configContext, String code, Cookie stateCookie, + String[] parsedStateCookieValue) { String userPath = null; String userQuery = null; - if (stateCookie != null) { - List values = context.queryParam("state"); - // IDP must return a 'state' query parameter and the value of the state cookie must start with this parameter's value - if (values.size() != 1) { - LOG.debug("State parameter can not be empty or multi-valued"); - return Uni.createFrom().failure(new AuthenticationCompletionException()); - } else if (!stateCookie.getValue().startsWith(values.get(0))) { - LOG.debug("State cookie value does not match the state query parameter value"); - return Uni.createFrom().failure(new AuthenticationCompletionException()); - } else { - // This is an original redirect from IDP, check if the original request path and query need to be restored - String[] pair = COOKIE_PATTERN.split(stateCookie.getValue()); - if (pair.length == 2) { - int userQueryIndex = pair[1].indexOf("?"); - if (userQueryIndex >= 0) { - userPath = pair[1].substring(0, userQueryIndex); - if (userQueryIndex + 1 < pair[1].length()) { - userQuery = pair[1].substring(userQueryIndex + 1); - } - } else { - userPath = pair[1]; - } + + // This is an original redirect from IDP, check if the original request path and query need to be restored + if (parsedStateCookieValue.length == 2) { + int userQueryIndex = parsedStateCookieValue[1].indexOf("?"); + if (userQueryIndex >= 0) { + userPath = parsedStateCookieValue[1].substring(0, userQueryIndex); + if (userQueryIndex + 1 < parsedStateCookieValue[1].length()) { + userQuery = parsedStateCookieValue[1].substring(userQueryIndex + 1); } - OidcUtils.removeCookie(context, configContext.oidcConfig, stateCookie.getName()); + } else { + userPath = parsedStateCookieValue[1]; } - } else { - // State cookie must be available to minimize the risk of CSRF - LOG.debug("The state cookie is missing after a redirect from IDP, authentication has failed"); - return Uni.createFrom().failure(new AuthenticationCompletionException()); } + OidcUtils.removeCookie(context, configContext.oidcConfig, stateCookie.getName()); final String finalUserPath = userPath; final String finalUserQuery = userQuery; @@ -463,13 +493,13 @@ private String generateCodeFlowState(RoutingContext context, TenantConfigContext cookieValue += (COOKIE_DELIM + requestPath); } } - createCookie(context, configContext.oidcConfig, getStateCookieName(configContext), cookieValue, 60 * 30); + createCookie(context, configContext.oidcConfig, getStateCookieName(configContext.oidcConfig), cookieValue, 60 * 30); return uuid; } private String generatePostLogoutState(RoutingContext context, TenantConfigContext configContext) { - OidcUtils.removeCookie(context, configContext.oidcConfig, getPostLogoutCookieName(configContext)); - return createCookie(context, configContext.oidcConfig, getPostLogoutCookieName(configContext), + OidcUtils.removeCookie(context, configContext.oidcConfig, getPostLogoutCookieName(configContext.oidcConfig)); + return createCookie(context, configContext.oidcConfig, getPostLogoutCookieName(configContext.oidcConfig), UUID.randomUUID().toString(), 60 * 30).getValue(); } @@ -639,12 +669,12 @@ public Void apply(Void t) { }); } - private static String getStateCookieName(TenantConfigContext configContext) { - return OidcUtils.STATE_COOKIE_NAME + getCookieSuffix(configContext.oidcConfig); + private static String getStateCookieName(OidcTenantConfig oidcConfig) { + return OidcUtils.STATE_COOKIE_NAME + getCookieSuffix(oidcConfig); } - private static String getPostLogoutCookieName(TenantConfigContext configContext) { - return OidcUtils.POST_LOGOUT_COOKIE_NAME + getCookieSuffix(configContext.oidcConfig); + private static String getPostLogoutCookieName(OidcTenantConfig oidcConfig) { + return OidcUtils.POST_LOGOUT_COOKIE_NAME + getCookieSuffix(oidcConfig); } private static String getSessionCookieName(OidcTenantConfig oidcConfig) { diff --git a/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java b/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java new file mode 100644 index 0000000000000..728d304a44297 --- /dev/null +++ b/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java @@ -0,0 +1,48 @@ +package io.quarkus.it.keycloak; + +import javax.annotation.PostConstruct; +import javax.enterprise.context.ApplicationScoped; +import javax.inject.Inject; + +import org.eclipse.microprofile.config.inject.ConfigProperty; + +import io.quarkus.oidc.OidcRequestContext; +import io.quarkus.oidc.OidcTenantConfig; +import io.quarkus.oidc.OidcTenantConfig.ApplicationType; +import io.quarkus.oidc.TenantConfigResolver; +import io.smallrye.mutiny.Uni; +import io.vertx.ext.web.RoutingContext; + +@ApplicationScoped +public class CustomTenantConfigResolver implements TenantConfigResolver { + + @Inject + @ConfigProperty(name = "quarkus.oidc.auth-server-url") + String authServerUrl; + + OidcTenantConfig config = new OidcTenantConfig(); + + public CustomTenantConfigResolver() { + } + + @PostConstruct + public void initConfig() { + config.setTenantId("tenant-before-wrong-redirect"); + config.setAuthServerUrl(authServerUrl); + config.setClientId("quarkus-app"); + config.getCredentials().setSecret("secret"); + config.setApplicationType(ApplicationType.WEB_APP); + } + + @Override + public Uni resolve(RoutingContext context, OidcRequestContext requestContext) { + if (context.request().path().contains("callback-before-wrong-redirect")) { + if (context.getCookie("q_auth_tenant-before-wrong-redirect") != null) { + // trigger the code to access token exchange failure due to a redirect uri mismatch + config.authentication.setRedirectPath("wrong-path"); + } + return Uni.createFrom().item(config); + } + return Uni.createFrom().nullItem(); + } +} diff --git a/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantResolver.java b/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantResolver.java index 14fe57742edd6..1186847417960 100644 --- a/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantResolver.java +++ b/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantResolver.java @@ -73,11 +73,6 @@ public String resolve(RoutingContext context) { return "tenant-cookie-path-header"; } - if (path.contains("callback-before-wrong-redirect")) { - return context.getCookie("q_auth_tenant-before-wrong-redirect") == null ? "tenant-before-wrong-redirect" - : "tenant-1"; - } - if (path.contains("callback-after-redirect") || path.contains("callback-before-redirect")) { return "tenant-1"; } diff --git a/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/ProtectedResource3.java b/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/ProtectedResource3.java new file mode 100644 index 0000000000000..161449c0a4d1e --- /dev/null +++ b/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/ProtectedResource3.java @@ -0,0 +1,19 @@ +package io.quarkus.it.keycloak; + +import javax.ws.rs.GET; +import javax.ws.rs.InternalServerErrorException; +import javax.ws.rs.Path; + +import io.quarkus.security.Authenticated; + +@Path("/web-app3") +@Authenticated +public class ProtectedResource3 { + + @GET + public String getName() { + // CodeFlowTest#testAuthenticationCompletionFailedNoStateCookie checks that if a state cookie is missing + // then 401 is returned when a redirect targets the endpoint requiring authentication + throw new InternalServerErrorException("This method must not be invoked"); + } +} diff --git a/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/TenantHttps.java b/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/TenantHttps.java index fb6dee72eff5b..5177d547dae55 100644 --- a/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/TenantHttps.java +++ b/integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/TenantHttps.java @@ -25,7 +25,7 @@ public String getTenant() { @GET @Path("query") - public String getTenantWithQuery(@QueryParam("a") String value) { - return getTenant() + "?a=" + value; + public String getTenantWithQuery(@QueryParam("code") String value) { + return getTenant() + "?code=" + value; } } diff --git a/integration-tests/oidc-code-flow/src/main/resources/application.properties b/integration-tests/oidc-code-flow/src/main/resources/application.properties index ba799461de52d..95252fcad23e8 100644 --- a/integration-tests/oidc-code-flow/src/main/resources/application.properties +++ b/integration-tests/oidc-code-flow/src/main/resources/application.properties @@ -1,4 +1,4 @@ -# Default tenant configuration +# Default tenant configurationf quarkus.oidc.auth-server-url=${keycloak.url}/realms/quarkus quarkus.oidc.client-id=quarkus-app quarkus.oidc.credentials.secret=secret @@ -26,13 +26,6 @@ quarkus.oidc.tenant-listener.credentials.secret=secret quarkus.oidc.tenant-listener.authentication.remove-redirect-parameters=false quarkus.oidc.tenant-listener.application-type=web-app - -quarkus.oidc.tenant-before-wrong-redirect.auth-server-url=${keycloak.url}/realms/quarkus -quarkus.oidc.tenant-before-wrong-redirect.client-id=quarkus-app -quarkus.oidc.tenant-before-wrong-redirect.credentials.client-secret.value=secret -quarkus.oidc.tenant-before-wrong-redirect.token.issuer=${keycloak.url}/realms/quarkus -quarkus.oidc.tenant-before-wrong-redirect.application-type=web-app - # Tenant which does not need to restore a request path after redirect, client_secret_post method quarkus.oidc.tenant-1.auth-server-url=${keycloak.url}/realms/quarkus quarkus.oidc.tenant-1.client-id=quarkus-app @@ -103,6 +96,7 @@ quarkus.oidc.tenant-https.authentication.extra-params.max-age=60 quarkus.oidc.tenant-https.application-type=web-app quarkus.oidc.tenant-https.authentication.force-redirect-https-scheme=true quarkus.oidc.tenant-https.authentication.cookie-suffix=test +quarkus.oidc.tenant-https.authentication.redirect-without-state-cookie=true quarkus.oidc.tenant-javascript.auth-server-url=${keycloak.url}/realms/quarkus quarkus.oidc.tenant-javascript.client-id=quarkus-app @@ -144,9 +138,6 @@ quarkus.oidc.tenant-split-tokens.application-type=web-app quarkus.http.auth.permission.roles1.paths=/index.html quarkus.http.auth.permission.roles1.policy=authenticated -quarkus.http.auth.permission.callback.paths=/web-app3 -quarkus.http.auth.permission.callback.policy=authenticated - quarkus.http.auth.permission.logout.paths=/tenant-logout quarkus.http.auth.permission.logout.policy=authenticated @@ -169,9 +160,5 @@ quarkus.http.proxy.allow-forwarded=true quarkus.log.category."io.quarkus.oidc.runtime.CodeAuthenticationMechanism".min-level=TRACE quarkus.log.category."io.quarkus.oidc.runtime.CodeAuthenticationMechanism".level=TRACE -quarkus.log.category."io.vertx.ext.auth.oauth2.impl.OAuth2UserImpl".min-level=TRACE -quarkus.log.category."io.vertx.ext.auth.oauth2.impl.OAuth2UserImpl".level=TRACE -#quarkus.log.category."org.apache.http".min-level=TRACE -#quarkus.log.category."org.apache.http".level=TRACE quarkus.log.category."com.gargoylesoftware.htmlunit.javascript.host.css.CSSStyleSheet".level=FATAL diff --git a/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java b/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java index b8eb0507bda2d..a91f2f632fc50 100644 --- a/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java +++ b/integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java @@ -46,6 +46,8 @@ public void testCodeFlowNoConsent() throws IOException { .loadWebResponse(new WebRequest(URI.create("http://localhost:8081/index.html").toURL())); verifyLocationHeader(webClient, webResponse.getResponseHeaderValue("location"), null, "web-app", false); + webClient.getCookieManager().clearCookies(); + webClient.getOptions().setRedirectEnabled(true); HtmlPage page = webClient.getPage("http://localhost:8081/index.html"); @@ -147,7 +149,7 @@ public void testCodeFlowForceHttpsRedirectUriWithQuery() throws IOException { WebResponse webResponse = webClient .loadWebResponse( - new WebRequest(URI.create("http://localhost:8081/tenant-https/query?a=b").toURL())); + new WebRequest(URI.create("http://localhost:8081/tenant-https/query?code=b").toURL())); String keycloakUrl = webResponse.getResponseHeaderValue("location"); verifyLocationHeader(webClient, keycloakUrl, "tenant-https_test", "tenant-https", true); @@ -178,10 +180,10 @@ public void testCodeFlowForceHttpsRedirectUriWithQuery() throws IOException { endpointLocationWithoutQuery = "http" + endpointLocationWithoutQuery.substring(5); URI endpointLocationWithoutQueryUri = URI.create(endpointLocationWithoutQuery); - assertEquals("a=b", endpointLocationWithoutQueryUri.getRawQuery()); + assertEquals("code=b", endpointLocationWithoutQueryUri.getRawQuery()); page = webClient.getPage(endpointLocationWithoutQueryUri.toURL()); - assertEquals("tenant-https:reauthenticated?a=b", page.getBody().asText()); + assertEquals("tenant-https:reauthenticated?code=b", page.getBody().asText()); Cookie sessionCookie = getSessionCookie(webClient, "tenant-https_test"); assertNotNull(sessionCookie); webClient.getCookieManager().clearCookies(); @@ -241,6 +243,7 @@ public Boolean call() throws Exception { }); webClient.getOptions().setRedirectEnabled(true); + webClient.getCookieManager().clearCookies(); page = webClient.getPage("http://localhost:8081/index.html"); assertEquals("Sign in to quarkus", page.getTitleText()); @@ -321,6 +324,8 @@ public Boolean call() throws Exception { // session invalidated because it ended at the OP, should redirect to login page at the OP webClient.getOptions().setRedirectEnabled(true); + webClient.getCookieManager().clearCookies(); + page = webClient.getPage("http://localhost:8081/tenant-logout"); assertNull(getSessionCookie(webClient, "tenant-logout")); assertEquals("Sign in to logout-realm", page.getTitleText()); @@ -510,6 +515,8 @@ public void testIdTokenInjectionWithoutRestoredPathDifferentRoot() throws IOExce assertNull(getSessionCookie(webClient, "tenant-2")); webClient.getOptions().setRedirectEnabled(true); + webClient.getCookieManager().clearCookies(); + page = webClient.getPage("http://localhost:8081/web-app2/name"); assertEquals("Sign in to quarkus", page.getTitleText());