Skip to content

Commit

Permalink
Process OIDC code query param only if the state cookie exists
Browse files Browse the repository at this point in the history
  • Loading branch information
@sberyozkin
sberyozkin committed Jan 28, 2022
1 parent 704b144 commit bb7fa6f
Show file tree
Hide file tree
Showing 6 changed files with 124 additions and 63 deletions.
23 changes: 23 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,21 @@ public static class Authentication {
@ConfigItem(defaultValueDocumentation = "false")
public Optional<Boolean> forceRedirectHttpsScheme = Optional.empty();

/**
* Fail the authentication with the HTTP 401 status if the state cookie is missing when the code flow
* has to be completed and either the 'code' or 'error' parameter is detected.
*
* If this property is enabled then an authentication challenge redirecting the user to OpenId Connect provider
* will be issued instead if no state cookie is detected.
*
* Enable this property only if you'd like to support custom 'code' or 'error' query parameters and
* avoid triggering the authorization code completion process.
*
* This property has no effect when the session is already established.
*/
@ConfigItem(defaultValue = "false")
public boolean redirectWithoutStateCookie;

/**
* List of scopes
*/
Expand Down Expand Up @@ -700,6 +715,14 @@ public void setCookieSuffix(String cookieSuffix) {
this.cookieSuffix = Optional.of(cookieSuffix);
}

public boolean isRedirectWithoutStateCookie() {
return redirectWithoutStateCookie;
}

public void setRedirectWithoutStateCookie(boolean redirectWithoutStateCookie) {
this.redirectWithoutStateCookie = redirectWithoutStateCookie;
}

}

@ConfigGroup
Expand Down
118 changes: 68 additions & 50 deletions ...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public Uni<SecurityIdentity> 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<TenantConfigContext> resolvedContext = resolver.resolveContext(context);
Expand All @@ -76,21 +76,56 @@ public Uni<SecurityIdentity> 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) {

if (!isStateCookieValid(context, stateCookie)) {
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<TenantConfigContext> resolvedContext = resolver.resolveContext(context);
return resolvedContext.onItem()
.transformToUni(new Function<TenantConfigContext, Uni<? extends SecurityIdentity>>() {
@Override
public Uni<SecurityIdentity> apply(TenantConfigContext tenantContext) {
return performCodeFlow(identityProviderManager, context, tenantContext, code, stateCookie);
}
});
} 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());
}
} else if (context.request().getParam(OidcConstants.CODE_FLOW_CODE) != null
&& !oidcTenantConfig.authentication.redirectWithoutStateCookie) {
LOG.debug(
"The state cookie is missing but either the 'code' or 'error' parameter is available, authentication has failed");
return Uni.createFrom().failure(new AuthenticationCompletionException());

}

// start a new session by starting the code flow dance
Uni<TenantConfigContext> resolvedContext = resolver.resolveContext(context);
return resolvedContext.onItem()
.transformToUni(new Function<TenantConfigContext, Uni<? extends SecurityIdentity>>() {
@Override
public Uni<SecurityIdentity> apply(TenantConfigContext tenantContext) {
return performCodeFlow(identityProviderManager, context, tenantContext, code);
}
});
// return an empty identity - this will lead to a challenge redirecting the user to OpenId Connect provider
// unless some other authentication mechanism creates the identity.
return Uni.createFrom().optional(Optional.empty());

}

private boolean isStateCookieValid(RoutingContext context, Cookie stateCookie) {
List<String> 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 false;
} else if (!stateCookie.getValue().startsWith(values.get(0))) {
LOG.debug("State cookie value does not match the state query parameter value");
return false;
}
return true;
}

private Uni<SecurityIdentity> reAuthenticate(Cookie sessionCookie,
Expand Down Expand Up @@ -251,42 +286,25 @@ public Uni<ChallengeData> apply(Void t) {
}

private Uni<SecurityIdentity> performCodeFlow(IdentityProviderManager identityProviderManager,
RoutingContext context, TenantConfigContext configContext, String code) {

Cookie stateCookie = context.getCookie(getStateCookieName(configContext));
RoutingContext context, TenantConfigContext configContext, String code, Cookie stateCookie) {

String userPath = null;
String userQuery = null;
if (stateCookie != null) {
List<String> 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
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);
}
OidcUtils.removeCookie(context, configContext.oidcConfig, stateCookie.getName());
} else {
userPath = pair[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;
Expand Down Expand Up @@ -463,13 +481,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();
}
Expand Down Expand Up @@ -639,12 +657,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) {
Expand Down
19 changes: 19 additions & 0 deletions ...gration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/ProtectedResource3.java
View file
Original file line number Diff line number Diff line change
@@ -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");
}
}
4 changes: 2 additions & 2 deletions integration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/TenantHttps.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
10 changes: 2 additions & 8 deletions integration-tests/oidc-code-flow/src/main/resources/application.properties
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -103,6 +103,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
Expand Down Expand Up @@ -144,9 +145,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

Expand All @@ -169,9 +167,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
13 changes: 10 additions & 3 deletions integration-tests/oidc-code-flow/src/test/java/io/quarkus/it/keycloak/CodeFlowTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down

0 comments on commit bb7fa6f

Please sign in to comment.