Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process OIDC code query param only if the state cookie exists #23274

Merged
merged 1 commit into from
Feb 4, 2022
Merged

Process OIDC code query param only if the state cookie exists #23274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
View file
Open in desktop
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
130 changes: 80 additions & 50 deletions ...sions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java
View file
Open in desktop
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) {
Copy link
Contributor

@pedroigor pedroigor Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not 100% sure how reliable is this. Is it really needed?

Copy link
Member Author

@sberyozkin sberyozkin Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor Good point, I've been contemplating above about it, it is the best effort at trying to figure out how to fail the request best if the state cookie is missing.

On the main it is an immediate 401. But on the main it is also the problem that if the code is detected then it is assumed it is the redirect from KC - and only then then the state cookie is checked, but with the error and the Apple's form_post response mode to be supported next it just does not work - especially with form_post - we can't start checking the post data without asserting the state cookie exists, so this refactoring is making these checks dependent on the availability of the state cookie.

So now, if the state cookie is missing and 1) If no code is there - we fail indirectly by challenging the user with a redirect to KC - this is how it works on main 2) if code is there - we check Referer and if proved it comes from KC only then do we stop everything with 401 - this is new to this PR, instead of just failing with 401

So if the Referer check fails then instead of 401 the user will be redirected to KC to re-authenticate. And as I said above, we can follow up with adding the property configuring what to do if the state cookie is missing and the code (error) (query or form post) present - do 401 or redirect to KC. In fact I started with the property first but then thought it was unnecessary yet.

Does it clarify it ?

We have a test expecting 401 in case of a missing state cookie and this code properly gives 401 as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'm still trying to figure out if this has some security implication because the header can be easily forged into the request. But don't think there is and it should improve UX.

Makes sense?

Copy link
Member Author

@sberyozkin sberyozkin Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor Hi Pedro,

I'm still trying to figure out if this has some security implication because the header can be easily forged into the request. But don't think there is and it should improve UX. Makes sense?

I'm not sure I follow, can you clarify please ?

This header is only checked in the getChallenge method. We don't do any positive decision based on its match - we only check it to decide how to fail if state cookie is not available but 'code' is.

If it matches the Keycloak authorization URL then it is 401 (meaning the authentication has failed, no retries), if it does not match it - it is a new unauthenticated request, the user goes to Keycloak/etc. This referer won't be used to redirect the user.

We can totally drop this check, but that will mean it will always be only a redirect to Keycloak if the state cookie is absent but the code is there; the referer check gives us an option to react to this case in a more nuanced way.
It is really about this use case, how to fail when the code is available but the state cookie is not.

I'm not sure, may be I will just drop this Referer check. We have a test where a state cookie is missing but the code is, and 401 is expected. Without this check I'll have to tweak the test to expect 302 to Keycloak in this case though.

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
48 changes: 48 additions & 0 deletions ...tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantConfigResolver.java
View file
Open in desktop
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();
}
}
5 changes: 0 additions & 5 deletions ...ation-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/CustomTenantResolver.java
View file
Open in desktop
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
19 changes: 19 additions & 0 deletions ...gration-tests/oidc-code-flow/src/main/java/io/quarkus/it/keycloak/ProtectedResource3.java
View file
Open in desktop
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
Open in desktop
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;
}
}
17 changes: 2 additions & 15 deletions integration-tests/oidc-code-flow/src/main/resources/application.properties
View file
Open in desktop
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