Skip to content

Commit

Permalink
OIDC improvements
Browse files Browse the repository at this point in the history 
Signed-off-by: David Kral <david.k.kral@oracle.com>
  • Loading branch information
@Verdent
Verdent committed Feb 1, 2024
1 parent b3e0709 commit f4483c7
Show file tree
Hide file tree
Showing 9 changed files with 393 additions and 39 deletions.
61 changes: 58 additions & 3 deletions ...iders/oidc-common/src/main/java/io/helidon/security/providers/oidc/common/OidcConfig.java
View file
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
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

0 comments on commit f4483c7

Please sign in to comment.