From cbfdae5e754bce26311bd9adc392aa495c092235 Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Thu, 25 Jan 2024 06:58:42 +0100 Subject: [PATCH] Remove support for multiple AUTH_SESSION_ID cookies (#26462) Closes #26457 Signed-off-by: stianst --- .../AuthenticationSessionManager.java | 98 +++++++------------ .../managers/UserSessionCrossDCManager.java | 30 +++--- .../keycloak/services/util/CookieHelper.java | 55 +---------- .../testsuite/cookies/CookieHelperTest.java | 13 +-- 4 files changed, 54 insertions(+), 142 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java index 6f4db0e5ca1b..a7801e382319 100644 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java @@ -48,8 +48,6 @@ public class AuthenticationSessionManager { public static final String AUTH_SESSION_ID = "AUTH_SESSION_ID"; - public static final int AUTH_SESSION_COOKIE_LIMIT = 3; - private static final Logger log = Logger.getLogger(AuthenticationSessionManager.class); private final KeycloakSession session; @@ -78,64 +76,46 @@ public RootAuthenticationSessionModel createAuthenticationSession(RealmModel rea public RootAuthenticationSessionModel getCurrentRootAuthenticationSession(RealmModel realm) { - List authSessionCookies = getAuthSessionCookies(realm); - - return authSessionCookies.stream().map(oldEncodedId -> { - AuthSessionId authSessionId = decodeAuthSessionId(oldEncodedId); - String sessionId = authSessionId.getDecodedId(); - - RootAuthenticationSessionModel rootAuthSession = session.authenticationSessions().getRootAuthenticationSession(realm, sessionId); - - if (rootAuthSession != null) { - reencodeAuthSessionCookie(oldEncodedId, authSessionId, realm); - return rootAuthSession; - } - + String oldEncodedId = getAuthSessionCookies(realm); + if (oldEncodedId == null) { return null; - }).filter(authSession -> Objects.nonNull(authSession)).findFirst().orElse(null); - } - - - public UserSessionModel getUserSessionFromAuthCookie(RealmModel realm) { - List authSessionCookies = getAuthSessionCookies(realm); - - return authSessionCookies.stream().map(oldEncodedId -> { - AuthSessionId authSessionId = decodeAuthSessionId(oldEncodedId); - String sessionId = authSessionId.getDecodedId(); + } - UserSessionModel userSession = session.sessions().getUserSession(realm, sessionId); + AuthSessionId authSessionId = decodeAuthSessionId(oldEncodedId); + String sessionId = authSessionId.getDecodedId(); - if (userSession != null) { - reencodeAuthSessionCookie(oldEncodedId, authSessionId, realm); - return userSession; - } + RootAuthenticationSessionModel rootAuthSession = session.authenticationSessions().getRootAuthenticationSession(realm, sessionId); + if (rootAuthSession != null) { + reencodeAuthSessionCookie(oldEncodedId, authSessionId, realm); + return rootAuthSession; + } else { return null; - }).filter(authSession -> Objects.nonNull(authSession)).findFirst().orElse(null); + } } - /** * Returns current authentication session if it exists, otherwise returns {@code null}. * @param realm * @return */ public AuthenticationSessionModel getCurrentAuthenticationSession(RealmModel realm, ClientModel client, String tabId) { - List authSessionCookies = getAuthSessionCookies(realm); - - return authSessionCookies.stream().map(oldEncodedId -> { - AuthSessionId authSessionId = decodeAuthSessionId(oldEncodedId); - String sessionId = authSessionId.getDecodedId(); + String oldEncodedId = getAuthSessionCookies(realm); + if (oldEncodedId == null) { + return null; + } - AuthenticationSessionModel authSession = getAuthenticationSessionByIdAndClient(realm, sessionId, client, tabId); + AuthSessionId authSessionId = decodeAuthSessionId(oldEncodedId); + String sessionId = authSessionId.getDecodedId(); - if (authSession != null) { - reencodeAuthSessionCookie(oldEncodedId, authSessionId, realm); - return authSession; - } + AuthenticationSessionModel authSession = getAuthenticationSessionByIdAndClient(realm, sessionId, client, tabId); + if (authSession != null) { + reencodeAuthSessionCookie(oldEncodedId, authSessionId, realm); + return authSession; + } else { return null; - }).filter(authSession -> Objects.nonNull(authSession)).findFirst().orElse(null); + } } @@ -184,28 +164,22 @@ void reencodeAuthSessionCookie(String oldEncodedAuthSessionId, AuthSessionId new /** * @param realm - * @return list of the values of AUTH_SESSION_ID cookies. It is assumed that values could be encoded with route added (EG. "5e161e00-d426-4ea6-98e9-52eb9844e2d7.node1" ) + * @return the value of the AUTH_SESSION_ID cookie. It is assumed that values could be encoded with route added (EG. "5e161e00-d426-4ea6-98e9-52eb9844e2d7.node1" ) */ - List getAuthSessionCookies(RealmModel realm) { - Set cookiesVal = CookieHelper.getCookieValues(session, AUTH_SESSION_ID); - List authSessionIds = cookiesVal.stream().limit(AUTH_SESSION_COOKIE_LIMIT).collect(Collectors.toList()); - - if (authSessionIds.isEmpty()) { - log.debugf("Not found AUTH_SESSION_ID cookie"); + String getAuthSessionCookies(RealmModel realm) { + String oldEncodedId = CookieHelper.getCookieValue(session, AUTH_SESSION_ID); + if (oldEncodedId == null || oldEncodedId.isEmpty()) { + return null; } - return authSessionIds.stream().filter(new Predicate() { - @Override - public boolean test(String id) { - StickySessionEncoderProvider encoder = session.getProvider(StickySessionEncoderProvider.class); - // in case the id is encoded with a route when running in a cluster - String decodedId = encoder.decodeSessionId(id); - // we can't blindly trust the cookie and assume it is valid and referencing a valid root auth session - // but make sure the root authentication session actually exists - // without this check there is a risk of resolving user sessions from invalid root authentication sessions as they share the same id - return session.authenticationSessions().getRootAuthenticationSession(realm, decodedId) != null; - } - }).collect(Collectors.toList()); + StickySessionEncoderProvider encoder = session.getProvider(StickySessionEncoderProvider.class); + // in case the id is encoded with a route when running in a cluster + String decodedId = encoder.decodeSessionId(oldEncodedId); + // we can't blindly trust the cookie and assume it is valid and referencing a valid root auth session + // but make sure the root authentication session actually exists + // without this check there is a risk of resolving user sessions from invalid root authentication sessions as they share the same id + RootAuthenticationSessionModel rootAuthenticationSession = session.authenticationSessions().getRootAuthenticationSession(realm, decodedId); + return rootAuthenticationSession != null ? oldEncodedId : null; } diff --git a/services/src/main/java/org/keycloak/services/managers/UserSessionCrossDCManager.java b/services/src/main/java/org/keycloak/services/managers/UserSessionCrossDCManager.java index 9ef67ab8d5dc..811a22381716 100644 --- a/services/src/main/java/org/keycloak/services/managers/UserSessionCrossDCManager.java +++ b/services/src/main/java/org/keycloak/services/managers/UserSessionCrossDCManager.java @@ -64,9 +64,9 @@ public UserSessionModel getUserSessionWithClient(RealmModel realm, String id, St // Just check if userSession also exists on remoteCache. It can happen that logout happened on 2nd DC and userSession is already removed on remoteCache and this DC wasn't yet notified public UserSessionModel getUserSessionIfExistsRemotely(AuthenticationSessionManager asm, RealmModel realm) { - List sessionCookies = asm.getAuthSessionCookies(realm); + String oldEncodedId = asm.getAuthSessionCookies(realm); - if (sessionCookies.isEmpty()) { + if (oldEncodedId == null) { // ideally, we should not rely on auth session id to retrieve user sessions // in case the auth session was removed, we fall back to the identity cookie // we are here doing the user session lookup twice, however the second lookup is going to make sure the @@ -74,25 +74,25 @@ public UserSessionModel getUserSessionIfExistsRemotely(AuthenticationSessionMana AuthenticationManager.AuthResult authResult = authenticateIdentityCookie(kcSession, realm, true); if (authResult != null && authResult.getSession() != null) { - sessionCookies = Collections.singletonList(authResult.getSession().getId()); + oldEncodedId = authResult.getSession().getId(); + } else { + return null; } } - return sessionCookies.stream().map(oldEncodedId -> { - AuthSessionId authSessionId = asm.decodeAuthSessionId(oldEncodedId); - String sessionId = authSessionId.getDecodedId(); + AuthSessionId authSessionId = asm.decodeAuthSessionId(oldEncodedId); + String sessionId = authSessionId.getDecodedId(); - // This will remove userSession "locally" if it doesn't exist on remoteCache - kcSession.sessions().getUserSessionWithPredicate(realm, sessionId, false, (UserSessionModel userSession2) -> userSession2 == null); + // This will remove userSession "locally" if it doesn't exist on remoteCache + kcSession.sessions().getUserSessionWithPredicate(realm, sessionId, false, (UserSessionModel userSession2) -> userSession2 == null); - UserSessionModel userSession = kcSession.sessions().getUserSession(realm, sessionId); - - if (userSession != null) { - asm.reencodeAuthSessionCookie(oldEncodedId, authSessionId, realm); - return userSession; - } + UserSessionModel userSession = kcSession.sessions().getUserSession(realm, sessionId); + if (userSession != null) { + asm.reencodeAuthSessionCookie(oldEncodedId, authSessionId, realm); + return userSession; + } else { return null; - }).filter(userSession -> Objects.nonNull(userSession)).findFirst().orElse(null); + } } } diff --git a/services/src/main/java/org/keycloak/services/util/CookieHelper.java b/services/src/main/java/org/keycloak/services/util/CookieHelper.java index cc679faf4c00..68d036fa5597 100755 --- a/services/src/main/java/org/keycloak/services/util/CookieHelper.java +++ b/services/src/main/java/org/keycloak/services/util/CookieHelper.java @@ -17,18 +17,12 @@ package org.keycloak.services.util; -import org.jboss.logging.Logger; +import jakarta.ws.rs.core.Cookie; import org.keycloak.http.HttpCookie; import org.keycloak.http.HttpResponse; -import org.jboss.resteasy.util.CookieParser; import org.keycloak.models.KeycloakSession; -import jakarta.ws.rs.core.Cookie; -import jakarta.ws.rs.core.HttpHeaders; -import java.util.Collections; -import java.util.HashSet; import java.util.Map; -import java.util.Set; import static org.keycloak.common.util.ServerCookie.SameSiteAttributeValue; @@ -41,8 +35,6 @@ public class CookieHelper { public static final String LEGACY_COOKIE = "_LEGACY"; - private static final Logger logger = Logger.getLogger(CookieHelper.class); - /** * Set a response cookie. This solely exists because JAX-RS 1.1 does not support setting HttpOnly cookies * @param name @@ -101,49 +93,4 @@ public static String getCookieValue(KeycloakSession session, String name) { return cookie != null ? cookie.getValue() : null; } - public static Set getCookieValues(KeycloakSession session, String name) { - Set ret = getInternalCookieValue(session, name); - if (ret.size() == 0) { - String legacy = name + LEGACY_COOKIE; - logger.debugv("Could not find any cookies with name {0}, trying {1}", name, legacy); - ret = getInternalCookieValue(session, legacy); - } - return ret; - } - - private static Set getInternalCookieValue(KeycloakSession session, String name) { - HttpHeaders headers = session.getContext().getHttpRequest().getHttpHeaders(); - Set cookiesVal = new HashSet<>(); - - // check for cookies in the request headers - cookiesVal.addAll(parseCookie(headers.getRequestHeaders().getFirst(HttpHeaders.COOKIE), name)); - - // get cookies from the cookie field - Cookie cookie = headers.getCookies().get(name); - if (cookie != null) { - logger.debugv("{0} cookie found in the cookie field", name); - cookiesVal.add(cookie.getValue()); - } - - - return cookiesVal; - } - - private static Set parseCookie(String header, String name) { - if (header == null || name == null) { - return Collections.emptySet(); - } - - Set values = new HashSet<>(); - - for (Cookie cookie : CookieParser.parseCookies(header)) { - if (name.equals(cookie.getName())) { - logger.debugv("{0} cookie found in the request header", name); - values.add(cookie.getValue()); - } - } - - return values; - } - } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/CookieHelperTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/CookieHelperTest.java index fbe2a47c134a..8ba1fceb4c24 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/CookieHelperTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/CookieHelperTest.java @@ -15,7 +15,6 @@ import java.io.IOException; import java.util.List; -import java.util.Set; public class CookieHelperTest extends AbstractKeycloakTest { @@ -42,8 +41,8 @@ public void testCookieHeaderWithSpaces() { filter.setHeader("Cookie", "terms_user=; KC_RESTART=eyJhbGciOiJIUzI1NiIsInR5cCIgOiAiSldUIiwia2lkIiA6ICJhZDUyMjdhMy1iY2ZkLTRjZjAtYTdiNi0zOTk4MzVhMDg1NjYifQ.eyJjaWQiOiJodHRwczovL3Nzby5qYm9zcy5vcmciLCJwdHkiOiJzYW1sIiwicnVyaSI6Imh0dHBzOi8vc3NvLmpib3NzLm9yZy9sb2dpbj9wcm92aWRlcj1SZWRIYXRFeHRlcm5hbFByb3ZpZGVyIiwiYWN0IjoiQVVUSEVOVElDQVRFIiwibm90ZXMiOnsiU0FNTF9SRVFVRVNUX0lEIjoibXBmbXBhYWxkampqa2ZmcG5oYmJoYWdmZmJwam1rbGFqbWVlb2lsaiIsInNhbWxfYmluZGluZyI6InBvc3QifX0.d0QJSOQ6pJGzqcjqDTRwkRpU6fwYeICedL6R9Gqs8CQ; AUTH_SESSION_ID=451ec4be-a0c8-430e-b489-6580f195ccf0; AUTH_SESSION_ID=55000981-8b5e-4c8d-853f-ee4c582c1d0d;AUTH_SESSION_ID=451ec4be-a0c8-430e-b489-6580f195ccf0; AUTH_SESSION_ID=55000981-8b5e-4c8d-853f-ee4c582c1d0d;AUTH_SESSION_ID=451ec4be-a0c8-430e-b489-6580f195ccf0; AUTH_SESSION_ID=55000981-8b5e-4c8d-853f-ee4c582c1d0d4;"); testing.server().run(session -> { - Set authSessionIds = CookieHelper.getCookieValues(session, "AUTH_SESSION_ID"); - Assert.assertEquals(3, authSessionIds.size()); + String authSessionId = CookieHelper.getCookieValue(session, "AUTH_SESSION_ID"); + Assert.assertEquals("55000981-8b5e-4c8d-853f-ee4c582c1d0d4", authSessionId); }); } @@ -53,20 +52,12 @@ public void testLegacyCookie() { testing.server().run(session -> { Assert.assertEquals("new", CookieHelper.getCookieValue(session, "MYCOOKIE")); - - Set cookieValues = CookieHelper.getCookieValues(session, "MYCOOKIE"); - Assert.assertEquals(1, cookieValues.size()); - Assert.assertEquals("new", cookieValues.iterator().next()); }); filter.setHeader("Cookie", "MYCOOKIE_LEGACY=legacy"); testing.server().run(session -> { Assert.assertEquals("legacy", CookieHelper.getCookieValue(session, "MYCOOKIE")); - - Set cookieValues = CookieHelper.getCookieValues(session, "MYCOOKIE"); - Assert.assertEquals(1, cookieValues.size()); - Assert.assertEquals("legacy", cookieValues.iterator().next()); }); }