Skip to content

Commit

Permalink
Merge pull request #23274 from sberyozkin/oidc_web_app_code_param
Browse files Browse the repository at this point in the history
Process OIDC code query param only if the state cookie exists
  • Loading branch information
sberyozkin authored Feb 4, 2022
2 parents 21a4a54 + 844b978 commit 067533c
Show file tree
Hide file tree
Showing 8 changed files with 161 additions and 76 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,6 @@ public Optional<String> getCookieSuffix() {
public void setCookieSuffix(String cookieSuffix) {
this.cookieSuffix = Optional.of(cookieSuffix);
}

}

@ConfigGroup
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public class CodeAuthenticationMechanism extends AbstractOidcAuthenticationMecha
static final String SESSION_MAX_AGE_PARAM = "session-max-age";
static final Uni<Void> 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);
Expand All @@ -63,7 +64,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 +77,52 @@ 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) {
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<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,
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<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);
}
});
}

private boolean isStateValid(RoutingContext context, String cookieState) {
List<String> 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<SecurityIdentity> reAuthenticate(Cookie sessionCookie,
Expand Down Expand Up @@ -198,6 +230,16 @@ public Uni<ChallengeData> getChallengeInternal(RoutingContext context, TenantCon

@Override
public Uni<ChallengeData> 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.
Expand Down Expand Up @@ -250,43 +292,31 @@ public Uni<ChallengeData> apply(Void t) {
});
}

private Uni<SecurityIdentity> 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<SecurityIdentity> performCodeFlow(IdentityProviderManager identityProviderManager,
RoutingContext context, TenantConfigContext configContext, String code, Cookie stateCookie,
String[] parsedStateCookieValue) {

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
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;
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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<OidcTenantConfig> resolve(RoutingContext context, OidcRequestContext<OidcTenantConfig> 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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}
Expand Down
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");
}
}
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;
}
}
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 @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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
Loading

0 comments on commit 067533c

Please sign in to comment.