From 3fc4b777bdca1c589705435d0c709c2fbd01eed3 Mon Sep 17 00:00:00 2001 From: "Mateusz \"Serafin\" Gajewski" Date: Wed, 21 Feb 2024 11:34:06 +0100 Subject: [PATCH 1/5] Migrate to a NewCookie.Builder --- .../trino/server/ui/OAuthIdTokenCookie.java | 40 ++++++++---------- .../io/trino/server/ui/OAuthWebUiCookie.java | 41 ++++++++----------- 2 files changed, 33 insertions(+), 48 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java index 3ee038b6188b..c296c4e1baca 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java @@ -21,8 +21,6 @@ import java.util.Optional; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOCATION; -import static jakarta.ws.rs.core.Cookie.DEFAULT_VERSION; -import static jakarta.ws.rs.core.NewCookie.DEFAULT_MAX_AGE; import static java.util.function.Predicate.not; public final class OAuthIdTokenCookie @@ -34,17 +32,14 @@ private OAuthIdTokenCookie() {} public static NewCookie create(String token, Instant tokenExpiration) { - return new NewCookie( - ID_TOKEN_COOKIE, - token, - UI_LOCATION, - null, - DEFAULT_VERSION, - null, - DEFAULT_MAX_AGE, - Date.from(tokenExpiration), - true, - true); + return new NewCookie.Builder(ID_TOKEN_COOKIE) + .value(token) + .path(UI_LOCATION) + .domain(null) + .expiry(Date.from(tokenExpiration)) + .secure(true) + .httpOnly(true) + .build(); } public static Optional read(Cookie cookie) @@ -56,16 +51,13 @@ public static Optional read(Cookie cookie) public static NewCookie delete() { - return new NewCookie( - ID_TOKEN_COOKIE, - "delete", - UI_LOCATION, - null, - DEFAULT_VERSION, - null, - 0, - null, - true, - true); + return new NewCookie.Builder(ID_TOKEN_COOKIE) + .value("delete") + .path(UI_LOCATION) + .domain(null) + .maxAge(0) + .secure(true) + .httpOnly(true) + .build(); } } diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java index 31a15bee269b..3a8dd84e67b9 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java @@ -21,8 +21,6 @@ import java.util.Optional; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOCATION; -import static jakarta.ws.rs.core.Cookie.DEFAULT_VERSION; -import static jakarta.ws.rs.core.NewCookie.DEFAULT_MAX_AGE; import static java.util.function.Predicate.not; public final class OAuthWebUiCookie @@ -34,17 +32,14 @@ private OAuthWebUiCookie() {} public static NewCookie create(String token, Instant tokenExpiration) { - return new NewCookie( - OAUTH2_COOKIE, - token, - UI_LOCATION, - null, - DEFAULT_VERSION, - null, - DEFAULT_MAX_AGE, - Date.from(tokenExpiration), - true, - true); + return new NewCookie.Builder(OAUTH2_COOKIE) + .path(UI_LOCATION) + .domain(null) + .value(token) + .expiry(Date.from(tokenExpiration)) + .secure(true) + .httpOnly(true) + .build(); } public static Optional read(Cookie cookie) @@ -56,16 +51,14 @@ public static Optional read(Cookie cookie) public static NewCookie delete() { - return new NewCookie( - OAUTH2_COOKIE, - "delete", - UI_LOCATION, - null, - DEFAULT_VERSION, - null, - 0, - null, - true, - true); + return new NewCookie.Builder(OAUTH2_COOKIE) + .value("delete") + .path(UI_LOCATION) + .domain(null) + .maxAge(0) + .expiry(null) + .secure(true) + .httpOnly(true) + .build(); } } From 8b8ae80d7672cca89ac78009b94d029429a6b615 Mon Sep 17 00:00:00 2001 From: "Mateusz \"Serafin\" Gajewski" Date: Wed, 21 Feb 2024 11:49:46 +0100 Subject: [PATCH 2/5] Allow passing array of cookies to responses --- .../server/security/oauth2/OAuth2Service.java | 5 ++-- .../server/ui/OAuth2WebUiLogoutResource.java | 3 ++- .../trino/server/ui/OAuthIdTokenCookie.java | 12 +++++----- .../io/trino/server/ui/OAuthWebUiCookie.java | 24 +++++++++---------- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Service.java b/core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Service.java index 86e060ac032b..2887a8efc73d 100644 --- a/core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Service.java +++ b/core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Service.java @@ -174,9 +174,8 @@ public Response finishOAuth2Challenge(String state, String code, URI callbackUri if (handlerState.isEmpty()) { Response.ResponseBuilder builder = Response .seeOther(URI.create(UI_LOCATION)) - .cookie( - OAuthWebUiCookie.create(tokenPairSerializer.serialize(fromOAuth2Response(oauth2Response)), cookieExpirationTime), - NonceCookie.delete()); + .cookie(OAuthWebUiCookie.create(tokenPairSerializer.serialize(fromOAuth2Response(oauth2Response)), cookieExpirationTime)) + .cookie(NonceCookie.delete()); if (oauth2Response.getIdToken().isPresent()) { builder.cookie(OAuthIdTokenCookie.create(oauth2Response.getIdToken().get(), cookieExpirationTime)); } diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java index 4101ded289a3..23b7ef56714a 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java @@ -60,7 +60,8 @@ public Response logout(@Context HttpHeaders httpHeaders, @Context UriInfo uriInf .build(); return Response.seeOther(auth2Client.getLogoutEndpoint(idToken, callBackUri).orElse(callBackUri)) - .cookie(delete(), OAuthIdTokenCookie.delete()) + .cookie(OAuthIdTokenCookie.delete()) + .cookie(delete()) .build(); } diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java index c296c4e1baca..d5fe44c81cc2 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java @@ -30,16 +30,16 @@ public final class OAuthIdTokenCookie private OAuthIdTokenCookie() {} - public static NewCookie create(String token, Instant tokenExpiration) + public static NewCookie[] create(String token, Instant tokenExpiration) { - return new NewCookie.Builder(ID_TOKEN_COOKIE) + return new NewCookie[] {new NewCookie.Builder(ID_TOKEN_COOKIE) .value(token) .path(UI_LOCATION) .domain(null) .expiry(Date.from(tokenExpiration)) .secure(true) .httpOnly(true) - .build(); + .build()}; } public static Optional read(Cookie cookie) @@ -49,15 +49,15 @@ public static Optional read(Cookie cookie) .filter(not(String::isBlank)); } - public static NewCookie delete() + public static NewCookie[] delete() { - return new NewCookie.Builder(ID_TOKEN_COOKIE) + return new NewCookie[] {new NewCookie.Builder(ID_TOKEN_COOKIE) .value("delete") .path(UI_LOCATION) .domain(null) .maxAge(0) .secure(true) .httpOnly(true) - .build(); + .build()}; } } diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java index 3a8dd84e67b9..3e3a89e4bc3d 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java @@ -30,16 +30,16 @@ public final class OAuthWebUiCookie private OAuthWebUiCookie() {} - public static NewCookie create(String token, Instant tokenExpiration) + public static NewCookie[] create(String token, Instant tokenExpiration) { - return new NewCookie.Builder(OAUTH2_COOKIE) - .path(UI_LOCATION) - .domain(null) - .value(token) - .expiry(Date.from(tokenExpiration)) - .secure(true) - .httpOnly(true) - .build(); + return new NewCookie[] {new NewCookie.Builder(OAUTH2_COOKIE) + .path(UI_LOCATION) + .domain(null) + .value(token) + .expiry(Date.from(tokenExpiration)) + .secure(true) + .httpOnly(true) + .build()}; } public static Optional read(Cookie cookie) @@ -49,9 +49,9 @@ public static Optional read(Cookie cookie) .filter(not(String::isBlank)); } - public static NewCookie delete() + public static NewCookie[] delete() { - return new NewCookie.Builder(OAUTH2_COOKIE) + return new NewCookie[] {new NewCookie.Builder(OAUTH2_COOKIE) .value("delete") .path(UI_LOCATION) .domain(null) @@ -59,6 +59,6 @@ public static NewCookie delete() .expiry(null) .secure(true) .httpOnly(true) - .build(); + .build()}; } } From f16d10e29568d728cd4de3e90b7f96ecfaf89cef Mon Sep 17 00:00:00 2001 From: "Mateusz \"Serafin\" Gajewski" Date: Wed, 21 Feb 2024 11:51:10 +0100 Subject: [PATCH 3/5] Pass existing cookies to delete() methods --- .../java/io/trino/server/ui/OAuth2WebUiLogoutResource.java | 4 ++-- .../src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java | 3 ++- .../src/main/java/io/trino/server/ui/OAuthWebUiCookie.java | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java index 23b7ef56714a..7a228d08c32b 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java @@ -60,8 +60,8 @@ public Response logout(@Context HttpHeaders httpHeaders, @Context UriInfo uriInf .build(); return Response.seeOther(auth2Client.getLogoutEndpoint(idToken, callBackUri).orElse(callBackUri)) - .cookie(OAuthIdTokenCookie.delete()) - .cookie(delete()) + .cookie(OAuthIdTokenCookie.delete(httpHeaders.getCookies())) + .cookie(delete(httpHeaders.getCookies())) .build(); } diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java index d5fe44c81cc2..d8dba2a14cfc 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java @@ -18,6 +18,7 @@ import java.time.Instant; import java.util.Date; +import java.util.Map; import java.util.Optional; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOCATION; @@ -49,7 +50,7 @@ public static Optional read(Cookie cookie) .filter(not(String::isBlank)); } - public static NewCookie[] delete() + public static NewCookie[] delete(Map availableCookies) { return new NewCookie[] {new NewCookie.Builder(ID_TOKEN_COOKIE) .value("delete") diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java index 3e3a89e4bc3d..035f5bd2dfaf 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java @@ -18,6 +18,7 @@ import java.time.Instant; import java.util.Date; +import java.util.Map; import java.util.Optional; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOCATION; @@ -49,7 +50,7 @@ public static Optional read(Cookie cookie) .filter(not(String::isBlank)); } - public static NewCookie[] delete() + public static NewCookie[] delete(Map availableCookies) { return new NewCookie[] {new NewCookie.Builder(OAUTH2_COOKIE) .value("delete") From d391daadd96386bc43e5d43889a614513731fc32 Mon Sep 17 00:00:00 2001 From: "Mateusz \"Serafin\" Gajewski" Date: Wed, 21 Feb 2024 11:54:31 +0100 Subject: [PATCH 4/5] Pass all cookies to read methods --- .../io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java | 6 ++---- .../java/io/trino/server/ui/OAuth2WebUiLogoutResource.java | 3 +-- .../main/java/io/trino/server/ui/OAuthIdTokenCookie.java | 4 ++-- .../src/main/java/io/trino/server/ui/OAuthWebUiCookie.java | 4 ++-- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java index a0afec3683d6..c24f7e044047 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java @@ -45,8 +45,6 @@ import static io.trino.server.ui.FormWebUiAuthenticationFilter.DISABLED_LOCATION; import static io.trino.server.ui.FormWebUiAuthenticationFilter.DISABLED_LOCATION_URI; import static io.trino.server.ui.FormWebUiAuthenticationFilter.TRINO_FORM_LOGIN; -import static io.trino.server.ui.OAuthIdTokenCookie.ID_TOKEN_COOKIE; -import static io.trino.server.ui.OAuthWebUiCookie.OAUTH2_COOKIE; import static jakarta.ws.rs.core.Response.Status.UNAUTHORIZED; import static java.util.Objects.requireNonNull; @@ -125,7 +123,7 @@ public void filter(ContainerRequestContext request) private Optional getTokenPair(ContainerRequestContext request) { try { - return OAuthWebUiCookie.read(request.getCookies().get(OAUTH2_COOKIE)) + return OAuthWebUiCookie.read(request.getCookies()) .map(tokenPairSerializer::deserialize); } catch (Exception e) { @@ -168,7 +166,7 @@ private void redirectForNewToken(ContainerRequestContext request, String refresh Response.ResponseBuilder builder = Response.temporaryRedirect(request.getUriInfo().getRequestUri()) .cookie(OAuthWebUiCookie.create(serializedToken, newExpirationTime)); - OAuthIdTokenCookie.read(request.getCookies().get(ID_TOKEN_COOKIE)) + OAuthIdTokenCookie.read(request.getCookies()) .ifPresent(idToken -> builder.cookie(OAuthIdTokenCookie.create(idToken, newExpirationTime))); request.abortWith(builder.build()); diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java index 7a228d08c32b..f73682d3ba56 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java @@ -33,7 +33,6 @@ import static io.trino.server.security.ResourceSecurity.AccessType.PUBLIC; import static io.trino.server.security.ResourceSecurity.AccessType.WEB_UI; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOGOUT; -import static io.trino.server.ui.OAuthIdTokenCookie.ID_TOKEN_COOKIE; import static io.trino.server.ui.OAuthWebUiCookie.delete; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Objects.requireNonNull; @@ -54,7 +53,7 @@ public OAuth2WebUiLogoutResource(OAuth2Client auth2Client) public Response logout(@Context HttpHeaders httpHeaders, @Context UriInfo uriInfo, @Context SecurityContext securityContext) throws IOException { - Optional idToken = OAuthIdTokenCookie.read(httpHeaders.getCookies().get(ID_TOKEN_COOKIE)); + Optional idToken = OAuthIdTokenCookie.read(httpHeaders.getCookies()); URI callBackUri = UriBuilder.fromUri(uriInfo.getAbsolutePath()) .path("logout.html") .build(); diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java index d8dba2a14cfc..efa9d5ea4ff6 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java @@ -43,9 +43,9 @@ public static NewCookie[] create(String token, Instant tokenExpiration) .build()}; } - public static Optional read(Cookie cookie) + public static Optional read(Map availableCookies) { - return Optional.ofNullable(cookie) + return Optional.ofNullable(availableCookies.get(ID_TOKEN_COOKIE)) .map(Cookie::getValue) .filter(not(String::isBlank)); } diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java index 035f5bd2dfaf..dd2b110427ab 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java @@ -43,9 +43,9 @@ public static NewCookie[] create(String token, Instant tokenExpiration) .build()}; } - public static Optional read(Cookie cookie) + public static Optional read(Map availableCookies) { - return Optional.ofNullable(cookie) + return Optional.ofNullable(availableCookies.get(OAUTH2_COOKIE)) .map(Cookie::getValue) .filter(not(String::isBlank)); } From fba3819cb3d03e0d7f96922e24fbe1ab8e2068ee Mon Sep 17 00:00:00 2001 From: "Mateusz \"Serafin\" Gajewski" Date: Wed, 21 Feb 2024 13:15:49 +0100 Subject: [PATCH 5/5] Support multipart UI token cookies When cookie name+value length exceeds 4096 bytes, it is silently rejected by most of the browsers per https://datatracker.ietf.org/doc/html/rfc6265#section-6.1. Since we don't control access & refresh token lengths and encryption scheme, we need to split value and set/remove multiple cookies in such cases. --- .../ui/FormWebUiAuthenticationFilter.java | 41 ++--- .../io/trino/server/ui/LoginResource.java | 6 +- .../io/trino/server/ui/MultipartUiCookie.java | 142 ++++++++++++++++ .../trino/server/ui/OAuthIdTokenCookie.java | 25 +-- .../io/trino/server/ui/OAuthWebUiCookie.java | 26 +-- .../server/ui/TestMultipartUiCookie.java | 152 ++++++++++++++++++ 6 files changed, 314 insertions(+), 78 deletions(-) create mode 100644 core/trino-main/src/main/java/io/trino/server/ui/MultipartUiCookie.java create mode 100644 core/trino-main/src/test/java/io/trino/server/ui/TestMultipartUiCookie.java diff --git a/core/trino-main/src/main/java/io/trino/server/ui/FormWebUiAuthenticationFilter.java b/core/trino-main/src/main/java/io/trino/server/ui/FormWebUiAuthenticationFilter.java index 20e571e72206..a990bc69fd0c 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/FormWebUiAuthenticationFilter.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/FormWebUiAuthenticationFilter.java @@ -35,6 +35,7 @@ import java.security.SecureRandom; import java.time.ZonedDateTime; import java.util.Date; +import java.util.Map; import java.util.Optional; import java.util.function.Function; @@ -70,6 +71,8 @@ public class FormWebUiAuthenticationFilter private final FormAuthenticator formAuthenticator; private final Optional authenticator; + private static final MultipartUiCookie MULTIPART_COOKIE = new MultipartUiCookie(TRINO_UI_COOKIE, "/ui"); + @Inject public FormWebUiAuthenticationFilter( FormWebUiConfig config, @@ -225,7 +228,7 @@ public static ResponseBuilder redirectFromSuccessfulLoginResponse(String redirec return Response.seeOther(redirectLocation); } - public Optional checkLoginCredentials(String username, String password, boolean secure) + public Optional checkLoginCredentials(String username, String password, boolean secure) { return formAuthenticator.isValidCredential(username, password, secure) .map(user -> createAuthenticationCookie(user, secure)); @@ -233,13 +236,8 @@ public Optional checkLoginCredentials(String username, String passwor private Optional getAuthenticatedUsername(ContainerRequestContext request) { - Cookie cookie = request.getCookies().get(TRINO_UI_COOKIE); - if (cookie == null) { - return Optional.empty(); - } - try { - return Optional.of(parseJwt(cookie.getValue())); + return MULTIPART_COOKIE.read(request.getCookies()).map(this::parseJwt); } catch (JwtException e) { return Optional.empty(); @@ -249,35 +247,14 @@ private Optional getAuthenticatedUsername(ContainerRequestContext reques } } - private NewCookie createAuthenticationCookie(String userName, boolean secure) + private NewCookie[] createAuthenticationCookie(String userName, boolean secure) { - String jwt = jwtGenerator.apply(userName); - return new NewCookie( - TRINO_UI_COOKIE, - jwt, - "/ui", - null, - Cookie.DEFAULT_VERSION, - null, - NewCookie.DEFAULT_MAX_AGE, - null, - secure, - true); + return MULTIPART_COOKIE.create(jwtGenerator.apply(userName), null, secure); } - public static NewCookie getDeleteCookie(boolean secure) + public static NewCookie[] getDeleteCookies(Map existingCookies, boolean isSecure) { - return new NewCookie( - TRINO_UI_COOKIE, - "delete", - "/ui", - null, - Cookie.DEFAULT_VERSION, - null, - 0, - null, - secure, - true); + return MULTIPART_COOKIE.delete(existingCookies, isSecure); } public boolean isPasswordAllowed(boolean secure) diff --git a/core/trino-main/src/main/java/io/trino/server/ui/LoginResource.java b/core/trino-main/src/main/java/io/trino/server/ui/LoginResource.java index 4022c69fa225..c43594fc09d5 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/LoginResource.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/LoginResource.java @@ -39,7 +39,7 @@ import static io.trino.server.ui.FormWebUiAuthenticationFilter.LOGIN_FORM_URI; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOGIN; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOGOUT; -import static io.trino.server.ui.FormWebUiAuthenticationFilter.getDeleteCookie; +import static io.trino.server.ui.FormWebUiAuthenticationFilter.getDeleteCookies; import static io.trino.server.ui.FormWebUiAuthenticationFilter.redirectFromSuccessfulLoginResponse; import static jakarta.ws.rs.core.MediaType.TEXT_HTML; import static java.nio.charset.StandardCharsets.UTF_8; @@ -89,7 +89,7 @@ public Response login( return Response.seeOther(DISABLED_LOCATION_URI).build(); } - Optional authenticationCookie = formWebUiAuthenticationManager.checkLoginCredentials(username, password, securityContext.isSecure()); + Optional authenticationCookie = formWebUiAuthenticationManager.checkLoginCredentials(username, password, securityContext.isSecure()); if (authenticationCookie.isEmpty()) { // authentication failed, redirect back to the login page return Response.seeOther(LOGIN_FORM_URI).build(); @@ -113,7 +113,7 @@ public Response logout(@Context HttpHeaders httpHeaders, @Context UriInfo uriInf redirectLocation = DISABLED_LOCATION_URI; } return Response.seeOther(redirectLocation) - .cookie(getDeleteCookie(securityContext.isSecure())) + .cookie(getDeleteCookies(httpHeaders.getCookies(), securityContext.isSecure())) .build(); } } diff --git a/core/trino-main/src/main/java/io/trino/server/ui/MultipartUiCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/MultipartUiCookie.java new file mode 100644 index 000000000000..3960f9ee6f27 --- /dev/null +++ b/core/trino-main/src/main/java/io/trino/server/ui/MultipartUiCookie.java @@ -0,0 +1,142 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.server.ui; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import jakarta.ws.rs.core.Cookie; +import jakarta.ws.rs.core.NewCookie; + +import java.time.Instant; +import java.util.Date; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.regex.Pattern; + +import static com.google.common.base.Strings.isNullOrEmpty; +import static jakarta.ws.rs.core.NewCookie.DEFAULT_MAX_AGE; +import static java.util.Objects.requireNonNull; + +public class MultipartUiCookie +{ + // https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis/#section-5.6-3.4.1 + public static final int MAXIMUM_COOKIE_SIZE = 4096; + + private final String cookieName; + private final Pattern cookiePattern; + private final String location; + + public MultipartUiCookie(String cookieName, String location) + { + this.cookieName = requireNonNull(cookieName, "cookieName is null"); + this.cookiePattern = Pattern.compile("^" + Pattern.quote(cookieName) + "(_\\d+)?$"); + this.location = requireNonNull(location, "location is null"); + } + + public NewCookie[] create(String token, Instant tokenExpiration, boolean isSecure) + { + Date expiration = Optional.ofNullable(tokenExpiration).map(Date::from).orElse(null); + ImmutableList.Builder cookiesToSet = ImmutableList.builder(); + int index = 0; + for (String part : splitValueByLength(token)) { + cookiesToSet.add(new NewCookie.Builder(cookieName(index++)) + .value(part) + .path(location) + .domain(null) + .comment(null) + .maxAge(DEFAULT_MAX_AGE) + .expiry(expiration) + .secure(isSecure) + .httpOnly(true) + .build()); + } + return cookiesToSet.build().toArray(new NewCookie[0]); + } + + public Optional read(Map existingCookies) + { + long cookiesCount = existingCookies.values().stream() + .filter(this::matchesName) + .filter(cookie -> !isNullOrEmpty(cookie.getValue())) + .count(); + + if (cookiesCount == 0) { + return Optional.empty(); + } + + StringBuilder token = new StringBuilder(); + for (int i = 0; i < cookiesCount; i++) { + Cookie cookie = existingCookies.get(cookieName(i)); + if (cookie == null || isNullOrEmpty(cookie.getValue())) { + return Optional.empty(); // non continuous + } + token.append(cookie.getValue()); + } + return Optional.of(token.toString()); + } + + public NewCookie[] delete(Map existingCookies, boolean isSecured) + { + ImmutableSet.Builder cookiesToDelete = ImmutableSet.builder(); + cookiesToDelete.add(deleteCookie(cookieName, isSecured)); // Always invalidate first cookie even if it doesn't exist + for (Cookie existingCookie : existingCookies.values()) { + if (matchesName(existingCookie)) { + cookiesToDelete.add(deleteCookie(existingCookie.getName(), isSecured)); + } + } + return cookiesToDelete.build().toArray(new NewCookie[0]); + } + + private List splitValueByLength(String value) + { + return Splitter.fixedLength(maximumCookieValueLength()).splitToList(value); + } + + private boolean matchesName(Cookie cookie) + { + return cookiePattern.matcher(cookie.getName()).matches(); + } + + @VisibleForTesting + String cookieName(int index) + { + if (index == 0) { + return cookieName; + } + + return cookieName + '_' + index; + } + + int maximumCookieValueLength() + { + // A browser should be able to accept at least 300 cookies with a maximum size of 4096 bytes, as stipulated by RFC 2109 (#6.3), RFC 2965 (#5.3), and RFC 6265 + return MAXIMUM_COOKIE_SIZE - cookieName(999).length(); + } + + private NewCookie deleteCookie(String name, boolean isSecured) + { + return new NewCookie.Builder(name) + .value("delete") + .path(location) + .domain(null) + .maxAge(0) + .expiry(null) + .secure(isSecured) + .httpOnly(true) + .build(); + } +} diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java index efa9d5ea4ff6..e5acab68f3ea 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthIdTokenCookie.java @@ -17,48 +17,31 @@ import jakarta.ws.rs.core.NewCookie; import java.time.Instant; -import java.util.Date; import java.util.Map; import java.util.Optional; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOCATION; -import static java.util.function.Predicate.not; public final class OAuthIdTokenCookie { // prefix according to: https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-4.1.3.1 public static final String ID_TOKEN_COOKIE = "__Secure-Trino-ID-Token"; + private static final MultipartUiCookie MULTIPART_COOKIE = new MultipartUiCookie(ID_TOKEN_COOKIE, UI_LOCATION); private OAuthIdTokenCookie() {} public static NewCookie[] create(String token, Instant tokenExpiration) { - return new NewCookie[] {new NewCookie.Builder(ID_TOKEN_COOKIE) - .value(token) - .path(UI_LOCATION) - .domain(null) - .expiry(Date.from(tokenExpiration)) - .secure(true) - .httpOnly(true) - .build()}; + return MULTIPART_COOKIE.create(token, tokenExpiration, true); } public static Optional read(Map availableCookies) { - return Optional.ofNullable(availableCookies.get(ID_TOKEN_COOKIE)) - .map(Cookie::getValue) - .filter(not(String::isBlank)); + return MULTIPART_COOKIE.read(availableCookies); } public static NewCookie[] delete(Map availableCookies) { - return new NewCookie[] {new NewCookie.Builder(ID_TOKEN_COOKIE) - .value("delete") - .path(UI_LOCATION) - .domain(null) - .maxAge(0) - .secure(true) - .httpOnly(true) - .build()}; + return MULTIPART_COOKIE.delete(availableCookies, true); } } diff --git a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java index dd2b110427ab..f57d8fe5eb9e 100644 --- a/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java +++ b/core/trino-main/src/main/java/io/trino/server/ui/OAuthWebUiCookie.java @@ -17,49 +17,31 @@ import jakarta.ws.rs.core.NewCookie; import java.time.Instant; -import java.util.Date; import java.util.Map; import java.util.Optional; import static io.trino.server.ui.FormWebUiAuthenticationFilter.UI_LOCATION; -import static java.util.function.Predicate.not; public final class OAuthWebUiCookie { // prefix according to: https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-4.1.3.1 public static final String OAUTH2_COOKIE = "__Secure-Trino-OAuth2-Token"; + private static final MultipartUiCookie MULTIPART_COOKIE = new MultipartUiCookie(OAUTH2_COOKIE, UI_LOCATION); private OAuthWebUiCookie() {} public static NewCookie[] create(String token, Instant tokenExpiration) { - return new NewCookie[] {new NewCookie.Builder(OAUTH2_COOKIE) - .path(UI_LOCATION) - .domain(null) - .value(token) - .expiry(Date.from(tokenExpiration)) - .secure(true) - .httpOnly(true) - .build()}; + return MULTIPART_COOKIE.create(token, tokenExpiration, true); } public static Optional read(Map availableCookies) { - return Optional.ofNullable(availableCookies.get(OAUTH2_COOKIE)) - .map(Cookie::getValue) - .filter(not(String::isBlank)); + return MULTIPART_COOKIE.read(availableCookies); } public static NewCookie[] delete(Map availableCookies) { - return new NewCookie[] {new NewCookie.Builder(OAUTH2_COOKIE) - .value("delete") - .path(UI_LOCATION) - .domain(null) - .maxAge(0) - .expiry(null) - .secure(true) - .httpOnly(true) - .build()}; + return MULTIPART_COOKIE.delete(availableCookies, true); } } diff --git a/core/trino-main/src/test/java/io/trino/server/ui/TestMultipartUiCookie.java b/core/trino-main/src/test/java/io/trino/server/ui/TestMultipartUiCookie.java new file mode 100644 index 000000000000..10d79c3ef9ee --- /dev/null +++ b/core/trino-main/src/test/java/io/trino/server/ui/TestMultipartUiCookie.java @@ -0,0 +1,152 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.server.ui; + +import com.google.common.collect.ImmutableMap; +import jakarta.ws.rs.core.Cookie; +import jakarta.ws.rs.core.NewCookie; +import org.junit.jupiter.api.Test; + +import java.time.Instant; +import java.util.Date; +import java.util.List; +import java.util.Map; + +import static io.trino.server.ui.MultipartUiCookie.MAXIMUM_COOKIE_SIZE; +import static jakarta.ws.rs.core.NewCookie.DEFAULT_MAX_AGE; +import static org.assertj.core.api.Assertions.assertThat; + +class TestMultipartUiCookie +{ + private static final String COOKIE_NAME = "__UI_Cookie"; + + private static final MultipartUiCookie COOKIE = new MultipartUiCookie(COOKIE_NAME, "/location"); + + @Test + public void testCookieNames() + { + assertThat(COOKIE.cookieName(0)).isEqualTo(COOKIE_NAME); + assertThat(COOKIE.cookieName(1)).isEqualTo(COOKIE_NAME + "_1"); + assertThat(COOKIE.cookieName(500)).isEqualTo(COOKIE_NAME + "_500"); + } + + @Test + public void testShortValueRoundTrip() + { + String longToken = "123456789".repeat(100); // < 4096 cookie value length limit + NewCookie[] newCookies = COOKIE.create(longToken, Instant.EPOCH, true); + assertThat(newCookies).hasSize(1); + assertThat(newCookies).extracting(NewCookie::getName).hasSameElementsAs(List.of(COOKIE.cookieName(0))); + assertThat(COOKIE.read(cookies(newCookies))).contains(longToken); + + NewCookie[] deleteCookies = COOKIE.delete(cookies(newCookies), true); + assertThat(deleteCookies).hasSize(1); + assertThat(deleteCookies).extracting(NewCookie::getName).hasSameElementsAs(List.of(COOKIE.cookieName(0))); + } + + @Test + public void testLongValueRoundTrip() + { + String longToken = "123456789".repeat(1000); // > 4096 cookie value length limit + NewCookie[] newCookies = COOKIE.create(longToken, Instant.EPOCH, true); + assertThat(newCookies) + .hasSize(3) + .allMatch(NewCookie::isSecure) + .allMatch(NewCookie::isHttpOnly) + .allMatch(cookie -> cookie.getMaxAge() == DEFAULT_MAX_AGE) + .allMatch(cookie -> cookie.getExpiry().equals(Date.from(Instant.EPOCH))) + .allMatch(cookie -> (cookie.getName().length() + cookie.getValue().length()) <= MAXIMUM_COOKIE_SIZE); + + assertThat(newCookies) + .extracting(NewCookie::getName) + .hasSameElementsAs(List.of(COOKIE.cookieName(0), COOKIE.cookieName(1), COOKIE.cookieName(2))); + + assertThat(COOKIE.read(cookies(newCookies))).contains(longToken); + + NewCookie[] deleteCookies = COOKIE.delete(cookies(newCookies), false); + assertThat(deleteCookies) + .hasSize(3) + .allMatch(cookie -> !cookie.isSecure()) + .extracting(NewCookie::getName) + .hasSameElementsAs(List.of(COOKIE.cookieName(0), COOKIE.cookieName(1), COOKIE.cookieName(2))); + } + + @Test + public void testMultipartValueRead() + { + assertThat(COOKIE.read(cookies(cookie(0, "a"), cookie(1, "b")))).contains("ab"); + assertThat(COOKIE.read(cookies(cookie(0, "a"), cookie(1, "b"), cookie(2, "c")))).contains("abc"); + assertThat(COOKIE.read(cookies(cookie(1, "b"), cookie(0, "a"), cookie(2, "c")))).contains("abc"); + assertThat(COOKIE.read(cookies(cookie(2, "c"), cookie(1, "b"), cookie(0, "a"), collidingCookie(0, "d")))).contains("abc"); + } + + @Test + public void testNonContinuousValueRead() + { + assertThat(COOKIE.read(cookies(cookie(0, "a"), cookie(2, "b")))).isEmpty(); + assertThat(COOKIE.read(cookies(cookie(0, "a"), cookie(1, "b"), cookie(3, "a")))).isEmpty(); + assertThat(COOKIE.read(cookies(cookie(1, "a")))).isEmpty(); + } + + @Test + public void testDelete() + { + assertThat(COOKIE.delete(cookies(cookie(0, "a"), cookie(2, "b"), collidingCookie(3, "d")), true)) + .hasSize(2) + .allMatch(cookie -> cookie.getMaxAge() == 0) + .allMatch(cookie -> cookie.getValue().equals("delete")) + .allMatch(NewCookie::isSecure) + .allMatch(NewCookie::isHttpOnly); + assertThat(COOKIE.delete(cookies(cookie(0, "a"), cookie(2, "b"), cookie("some-other-cookie", "some-other-value")), false)) + .hasSize(2) + .allMatch(cookie -> cookie.getName().startsWith(COOKIE_NAME)) + .allMatch(cookie -> cookie.getMaxAge() == 0) + .allMatch(cookie -> cookie.getValue().equals("delete")) + .allMatch(cookie -> !cookie.isSecure()) + .allMatch(NewCookie::isHttpOnly); + assertThat(COOKIE.delete(cookies(), false)) + .hasSize(1) + .allMatch(cookie -> cookie.getName().startsWith(COOKIE_NAME)) + .allMatch(cookie -> cookie.getMaxAge() == 0) + .allMatch(cookie -> cookie.getValue().equals("delete")) + .allMatch(cookie -> !cookie.isSecure()) + .allMatch(NewCookie::isHttpOnly); + } + + private static Map cookies(NewCookie... cookies) + { + ImmutableMap.Builder result = ImmutableMap.builderWithExpectedSize(cookies.length); + for (NewCookie cookie : cookies) { + result.put(cookie.getName(), cookie); + } + return result.buildOrThrow(); + } + + private static NewCookie cookie(int index, String value) + { + return cookie(COOKIE.cookieName(index), value); + } + + private static NewCookie collidingCookie(int index, String value) + { + return cookie(COOKIE.cookieName(index) + "-ignored", value); + } + + private static NewCookie cookie(String name, String value) + { + return new NewCookie.Builder(name) + .value(value) + .build(); + } +}