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

Jump to bottom

OIDC improvement #8323

Merged
merged 1 commit into from
Feb 5, 2024
Merged

OIDC improvement #8323

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
61 changes: 58 additions & 3 deletions ...iders/oidc-common/src/main/java/io/helidon/security/providers/oidc/common/OidcConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,10 @@ public final class OidcConfig extends TenantConfigImpl {
* Default tenant cookie name.
*/
public static final String DEFAULT_TENANT_COOKIE_NAME = "HELIDON_TENANT";
/**
* Default state cookie name.
*/
public static final String DEFAULT_STATE_COOKIE_NAME = "OIDC_STATE";
static final String DEFAULT_REDIRECT_URI = "/oidc/redirect";
static final String DEFAULT_LOGOUT_URI = "/oidc/logout";
static final boolean DEFAULT_REDIRECT = true;
Expand Down Expand Up @@ -394,6 +398,7 @@ public final class OidcConfig extends TenantConfigImpl {
private final OidcCookieHandler idTokenCookieHandler;
private final OidcCookieHandler refreshTokenCookieHandler;
private final OidcCookieHandler tenantCookieHandler;
private final OidcCookieHandler stateCookieHandler;
private final boolean tokenSignatureValidation;
private final boolean idTokenSignatureValidation;

Expand Down Expand Up @@ -425,6 +430,7 @@ private OidcConfig(Builder builder) {
this.idTokenCookieHandler = builder.idTokenCookieBuilder.build();
this.tenantCookieHandler = builder.tenantCookieBuilder.build();
this.refreshTokenCookieHandler = builder.refreshTokenCookieBuilder.build();
this.stateCookieHandler = builder.stateCookieBuilder.build();
this.tokenSignatureValidation = builder.tokenSignatureValidation;
this.idTokenSignatureValidation = builder.idTokenSignatureValidation;

Expand Down Expand Up @@ -562,6 +568,15 @@ public OidcCookieHandler refreshTokenCookieHandler() {
return refreshTokenCookieHandler;
}

/**
* Cookie handler to create cookies or unset cookies for state value.
*
* @return a new cookie handler
*/
public OidcCookieHandler stateCookieHandler() {
return stateCookieHandler;
}

/**
* Redirection URI.
*
Expand Down Expand Up @@ -919,6 +934,9 @@ public static class Builder extends BaseBuilder<Builder, OidcConfig> {
private final OidcCookieHandler.Builder refreshTokenCookieBuilder = OidcCookieHandler.builder()
.encryptionEnabled(true)
.cookieName(DEFAULT_REFRESH_COOKIE_NAME);
private final OidcCookieHandler.Builder stateCookieBuilder = OidcCookieHandler.builder()
.encryptionEnabled(true)
.cookieName(DEFAULT_STATE_COOKIE_NAME);
private TokenHandler headerHandler = TokenHandler.builder()
.tokenHeader("Authorization")
.tokenPrefix("bearer ")
Expand Down Expand Up @@ -1016,6 +1034,7 @@ public Builder config(Config config) {
config.get("cookie-name-id-token").asString().ifPresent(this::cookieNameIdToken);
config.get("cookie-name-tenant").asString().ifPresent(this::cookieTenantName);
config.get("cookie-name-refresh-token").asString().ifPresent(this::cookieNameRefreshToken);
config.get("cookie-name-state").asString().ifPresent(this::cookieNameState);
config.get("cookie-domain").asString().ifPresent(this::cookieDomain);
config.get("cookie-path").asString().ifPresent(this::cookiePath);
config.get("cookie-max-age-seconds").asLong().ifPresent(this::cookieMaxAgeSeconds);
Expand All @@ -1027,6 +1046,7 @@ public Builder config(Config config) {
config.get("cookie-encryption-id-enabled").asBoolean().ifPresent(this::cookieEncryptionEnabledIdToken);
config.get("cookie-encryption-tenant-enabled").asBoolean().ifPresent(this::cookieEncryptionEnabledTenantName);
config.get("cookie-encryption-refresh-enabled").asBoolean().ifPresent(this::cookieEncryptionEnabledRefreshToken);
config.get("cookie-encryption-state-enabled").asBoolean().ifPresent(this::cookieEncryptionEnabledState);
config.get("cookie-encryption-password").as(String.class)
.map(String::toCharArray)
.ifPresent(this::cookieEncryptionPassword);
Expand Down Expand Up @@ -1374,6 +1394,7 @@ public Builder cookieEncryptionName(String cookieEncryptionName) {
this.idTokenCookieBuilder.encryptionName(cookieEncryptionName);
this.tenantCookieBuilder.encryptionName(cookieEncryptionName);
this.refreshTokenCookieBuilder.encryptionName(cookieEncryptionName);
this.stateCookieBuilder.encryptionName(cookieEncryptionName);
return this;
}

Expand All @@ -1390,6 +1411,7 @@ public Builder cookieEncryptionPassword(char[] cookieEncryptionPassword) {
this.idTokenCookieBuilder.encryptionPassword(cookieEncryptionPassword);
this.tenantCookieBuilder.encryptionPassword(cookieEncryptionPassword);
this.refreshTokenCookieBuilder.encryptionPassword(cookieEncryptionPassword);
this.stateCookieBuilder.encryptionPassword(cookieEncryptionPassword);
return this;
}

Expand All @@ -1415,7 +1437,7 @@ public Builder cookieEncryptionEnabled(boolean cookieEncryptionEnabled) {
* OIDC server {@code false}
* @return updated builder instance
*/
@ConfiguredOption(value = "true")
@ConfiguredOption(key = "cookie-encryption-id-enabled", value = "true")
public Builder cookieEncryptionEnabledIdToken(boolean cookieEncryptionEnabled) {
this.idTokenCookieBuilder.encryptionEnabled(cookieEncryptionEnabled);
return this;
Expand All @@ -1428,7 +1450,7 @@ public Builder cookieEncryptionEnabledIdToken(boolean cookieEncryptionEnabled) {
* @param cookieEncryptionEnabled whether cookie should be encrypted {@code true}, or as plain text name {@code false}
* @return updated builder instance
*/
@ConfiguredOption(value = "true")
@ConfiguredOption(key = "cookie-encryption-tenant-enabled", value = "true")
public Builder cookieEncryptionEnabledTenantName(boolean cookieEncryptionEnabled) {
this.tenantCookieBuilder.encryptionEnabled(cookieEncryptionEnabled);
return this;
Expand All @@ -1442,12 +1464,26 @@ public Builder cookieEncryptionEnabledTenantName(boolean cookieEncryptionEnabled
* OIDC server {@code false}
* @return updated builder instance
*/
@ConfiguredOption(value = "true")
@ConfiguredOption(key = "cookie-encryption-refresh-enabled", value = "true")
public Builder cookieEncryptionEnabledRefreshToken(boolean cookieEncryptionEnabled) {
this.refreshTokenCookieBuilder.encryptionEnabled(cookieEncryptionEnabled);
return this;
}

/**
* Whether to encrypt state cookie created by this microservice.
* Defaults to {@code true}.
*
* @param cookieEncryptionEnabled whether cookie should be encrypted {@code true}, or as sent to
* OIDC server {@code false}
* @return updated builder instance
*/
@ConfiguredOption(key = "cookie-encryption-state-enabled", value = "true")
public Builder cookieEncryptionEnabledState(boolean cookieEncryptionEnabled) {
this.stateCookieBuilder.encryptionEnabled(cookieEncryptionEnabled);
return this;
}

/**
* When using cookie, used to set the SameSite cookie value. Can be
* "Strict" or "Lax"
Expand All @@ -1472,6 +1508,7 @@ public Builder cookieSameSite(SetCookie.SameSite sameSite) {
this.idTokenCookieBuilder.sameSite(sameSite);
this.tenantCookieBuilder.sameSite(sameSite);
this.refreshTokenCookieBuilder.sameSite(sameSite);
this.stateCookieBuilder.sameSite(sameSite);
this.cookieSameSiteDefault = false;
return this;
}
Expand All @@ -1489,6 +1526,7 @@ public Builder cookieSecure(Boolean secure) {
this.idTokenCookieBuilder.secure(secure);
this.tenantCookieBuilder.secure(secure);
this.refreshTokenCookieBuilder.secure(secure);
this.stateCookieBuilder.secure(secure);
return this;
}

Expand All @@ -1505,6 +1543,7 @@ public Builder cookieHttpOnly(Boolean httpOnly) {
this.idTokenCookieBuilder.httpOnly(httpOnly);
this.tenantCookieBuilder.httpOnly(httpOnly);
this.refreshTokenCookieBuilder.httpOnly(httpOnly);
this.stateCookieBuilder.httpOnly(httpOnly);
return this;
}

Expand All @@ -1522,6 +1561,7 @@ public Builder cookieMaxAgeSeconds(long age) {
this.idTokenCookieBuilder.maxAge(age);
this.tenantCookieBuilder.maxAge(age);
this.refreshTokenCookieBuilder.maxAge(age);
this.stateCookieBuilder.maxAge(age);
return this;
}

Expand All @@ -1538,6 +1578,7 @@ public Builder cookiePath(String path) {
this.idTokenCookieBuilder.path(path);
this.tenantCookieBuilder.path(path);
this.refreshTokenCookieBuilder.path(path);
this.stateCookieBuilder.path(path);
return this;
}

Expand All @@ -1554,6 +1595,7 @@ public Builder cookieDomain(String domain) {
this.idTokenCookieBuilder.domain(domain);
this.tenantCookieBuilder.domain(domain);
this.refreshTokenCookieBuilder.domain(domain);
this.stateCookieBuilder.domain(domain);
return this;
}

Expand Down Expand Up @@ -1612,6 +1654,19 @@ public Builder cookieNameRefreshToken(String cookieName) {
return this;
}

/**
* The name of the cookie to use for the state storage.
* Defaults to {@value #DEFAULT_STATE_COOKIE_NAME}.
*
* @param cookieName name of a cookie
* @return updated builder instance
*/
@ConfiguredOption(DEFAULT_REFRESH_COOKIE_NAME)
public Builder cookieNameState(String cookieName) {
this.stateCookieBuilder.cookieName(cookieName);
return this;
}

/**
* Whether to use cookie to store JWT between requests.
* Defaults to {@value #DEFAULT_COOKIE_USE}.
Expand Down
59 changes: 48 additions & 11 deletions security/providers/oidc/src/main/java/io/helidon/security/providers/oidc/OidcFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@

package io.helidon.security.providers.oidc;

import java.io.StringReader;
import java.lang.System.Logger.Level;
import java.net.URI;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.Base64;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
Expand All @@ -46,6 +48,8 @@
import io.helidon.http.Status;
import io.helidon.security.Security;
import io.helidon.security.SecurityException;
import io.helidon.security.jwt.Jwt;
import io.helidon.security.jwt.SignedJwt;
import io.helidon.security.providers.oidc.common.OidcConfig;
import io.helidon.security.providers.oidc.common.OidcCookieHandler;
import io.helidon.security.providers.oidc.common.Tenant;
Expand All @@ -62,7 +66,9 @@
import io.helidon.webserver.http.ServerResponse;
import io.helidon.webserver.security.SecurityHttpFeature;

import jakarta.json.Json;
import jakarta.json.JsonObject;
import jakarta.json.JsonReaderFactory;

import static io.helidon.http.HeaderNames.HOST;
import static io.helidon.security.providers.oidc.common.spi.TenantConfigFinder.DEFAULT_TENANT_ID;
Expand Down Expand Up @@ -145,6 +151,7 @@ public final class OidcFeature implements HttpFeature {
private static final String CODE_PARAM_NAME = "code";
private static final String STATE_PARAM_NAME = "state";
private static final String DEFAULT_REDIRECT = "/index.html";
private static final JsonReaderFactory JSON = Json.createReaderFactory(Map.of());

private final List<TenantConfigFinder> oidcConfigFinders;
private final LruCache<String, Tenant> tenants = LruCache.create();
Expand All @@ -153,6 +160,7 @@ public final class OidcFeature implements HttpFeature {
private final OidcCookieHandler idTokenCookieHandler;
private final OidcCookieHandler refreshTokenCookieHandler;
private final OidcCookieHandler tenantCookieHandler;
private final OidcCookieHandler stateCookieHandler;
private final boolean enabled;
private final CorsSupport corsSupport;

Expand All @@ -163,6 +171,7 @@ private OidcFeature(Builder builder) {
this.idTokenCookieHandler = oidcConfig.idTokenCookieHandler();
this.refreshTokenCookieHandler = oidcConfig.refreshTokenCookieHandler();
this.tenantCookieHandler = oidcConfig.tenantCookieHandler();
this.stateCookieHandler = oidcConfig.stateCookieHandler();
this.corsSupport = prepareCrossOriginSupport(oidcConfig.redirectUri(), oidcConfig.crossOriginConfig());
this.oidcConfigFinders = List.copyOf(builder.tenantConfigFinders);

Expand Down Expand Up @@ -374,6 +383,26 @@ private void processCode(String code, ServerRequest req, ServerResponse res) {
}

private void processCodeWithTenant(String code, ServerRequest req, ServerResponse res, String tenantName, Tenant tenant) {
Optional<String> maybeStateCookie = stateCookieHandler.findCookie(req.headers().toMap());
if (maybeStateCookie.isEmpty()) {
processError(res,
Status.UNAUTHORIZED_401,
"State cookie needs to be provided upon redirect");
return;
}
String stateBase64 = new String(Base64.getDecoder().decode(maybeStateCookie.get()), StandardCharsets.UTF_8);
JsonObject stateCookie = JSON.createReader(new StringReader(stateBase64)).readObject();
//Remove state cookie
res.headers().addCookie(stateCookieHandler.removeCookie().build());
String state = stateCookie.getString("state");
String queryState = req.query().get("state");
if (!state.equals(queryState)) {
processError(res,
Status.UNAUTHORIZED_401,
"State of the original request and obtained from identity server does not match");
return;
}

TenantConfig tenantConfig = tenant.tenantConfig();

WebClient webClient = tenant.appWebClient();
Expand All @@ -393,7 +422,7 @@ private void processCodeWithTenant(String code, ServerRequest req, ServerRespons
if (response.status().family() == Status.Family.SUCCESSFUL) {
try {
JsonObject jsonObject = response.as(JsonObject.class);
processJsonResponse(req, res, jsonObject, tenantName);
processJsonResponse(res, jsonObject, tenantName, stateCookie);
} catch (Exception e) {
processError(res, e, "Failed to read JSON from response");
}
Expand Down Expand Up @@ -449,29 +478,37 @@ private String redirectUri(ServerRequest req, String tenantName) {
return uri;
}

private String processJsonResponse(ServerRequest req,
ServerResponse res,
private String processJsonResponse(ServerResponse res,
JsonObject json,
String tenantName) {
String tenantName,
JsonObject stateCookie) {
String accessToken = json.getString("access_token");
String idToken = json.getString("id_token", null);
String refreshToken = json.getString("refresh_token", null);

//redirect to "state"
String state = req.query().first(STATE_PARAM_NAME).orElse(DEFAULT_REDIRECT);
Jwt accessTokenJwt = SignedJwt.parseToken(accessToken).getJwt();
String nonceOriginal = stateCookie.getString("nonce");
String nonceAccess = accessTokenJwt.nonce()
.orElseThrow(() -> new IllegalStateException("Nonce is required to be present in the access token"));
if (!nonceAccess.equals(nonceOriginal)) {
throw new IllegalStateException("Original nonce and the one obtained from access token does not match");
}

//redirect to "originalUri"
String originalUri = stateCookie.getString("originalUri", DEFAULT_REDIRECT);
res.status(Status.TEMPORARY_REDIRECT_307);
if (oidcConfig.useParam()) {
state += (state.contains("?") ? "&" : "?") + encode(oidcConfig.paramName()) + "=" + accessToken;
originalUri += (originalUri.contains("?") ? "&" : "?") + encode(oidcConfig.paramName()) + "=" + accessToken;
if (idToken != null) {
state += "&" + encode(oidcConfig.idTokenParamName()) + "=" + idToken;
originalUri += "&" + encode(oidcConfig.idTokenParamName()) + "=" + idToken;
}
if (!DEFAULT_TENANT_ID.equals(tenantName)) {
state += "&" + encode(oidcConfig.tenantParamName()) + "=" + encode(tenantName);
originalUri += "&" + encode(oidcConfig.tenantParamName()) + "=" + encode(tenantName);
}
}

state = increaseRedirectCounter(state);
res.headers().add(HeaderNames.LOCATION, state);
originalUri = increaseRedirectCounter(originalUri);
res.headers().add(HeaderNames.LOCATION, originalUri);

if (oidcConfig.useCookie()) {
try {
Expand Down
Loading
Loading