From d46f9cccc75558b05ff4de185d56d395edaf9d54 Mon Sep 17 00:00:00 2001 From: Pankaj kumar Date: Mon, 15 Jan 2024 15:54:57 +0530 Subject: [PATCH] Revert "Revert "Upgrade pac4j-oidc to 4.5.7 to address CVE-2021-44878 (#15522)"" This reverts commit 285b1d2043af00698af5f9ecbc747242d3a97cca. --- extensions-core/druid-pac4j/pom.xml | 2 +- .../druid/security/pac4j/Pac4jFilter.java | 17 ++-- .../security/pac4j/Pac4jSessionStore.java | 21 ++--- .../security/pac4j/Pac4jSessionStoreTest.java | 78 ++++++++++++++++++- licenses.yaml | 6 +- owasp-dependency-check-suppressions.xml | 6 +- 6 files changed, 104 insertions(+), 26 deletions(-) diff --git a/extensions-core/druid-pac4j/pom.xml b/extensions-core/druid-pac4j/pom.xml index a8cb8b3a08bf..a330f34c71f6 100644 --- a/extensions-core/druid-pac4j/pom.xml +++ b/extensions-core/druid-pac4j/pom.xml @@ -34,7 +34,7 @@ - 3.8.3 + 4.5.7 1.7 diff --git a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java index 4463e43ca29d..452a22609460 100644 --- a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java +++ b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java @@ -23,14 +23,15 @@ import org.apache.druid.server.security.AuthConfig; import org.apache.druid.server.security.AuthenticationResult; import org.pac4j.core.config.Config; -import org.pac4j.core.context.J2EContext; +import org.pac4j.core.context.JEEContext; import org.pac4j.core.context.session.SessionStore; import org.pac4j.core.engine.CallbackLogic; import org.pac4j.core.engine.DefaultCallbackLogic; import org.pac4j.core.engine.DefaultSecurityLogic; import org.pac4j.core.engine.SecurityLogic; +import org.pac4j.core.exception.http.HttpAction; import org.pac4j.core.http.adapter.HttpActionAdapter; -import org.pac4j.core.profile.CommonProfile; +import org.pac4j.core.profile.UserProfile; import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -47,12 +48,12 @@ public class Pac4jFilter implements Filter { private static final Logger LOGGER = new Logger(Pac4jFilter.class); - private static final HttpActionAdapter NOOP_HTTP_ACTION_ADAPTER = (int code, J2EContext ctx) -> null; + private static final HttpActionAdapter NOOP_HTTP_ACTION_ADAPTER = (HttpAction code, JEEContext ctx) -> null; private final Config pac4jConfig; - private final SecurityLogic securityLogic; - private final CallbackLogic callbackLogic; - private final SessionStore sessionStore; + private final SecurityLogic securityLogic; + private final CallbackLogic callbackLogic; + private final SessionStore sessionStore; private final String name; private final String authorizerName; @@ -88,7 +89,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest; HttpServletResponse httpServletResponse = (HttpServletResponse) servletResponse; - J2EContext context = new J2EContext(httpServletRequest, httpServletResponse, sessionStore); + JEEContext context = new JEEContext(httpServletRequest, httpServletResponse, sessionStore); if (Pac4jCallbackResource.SELF_URL.equals(httpServletRequest.getRequestURI())) { callbackLogic.perform( @@ -101,7 +102,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo String uid = securityLogic.perform( context, pac4jConfig, - (J2EContext ctx, Collection profiles, Object... parameters) -> { + (JEEContext ctx, Collection profiles, Object... parameters) -> { if (profiles.isEmpty()) { LOGGER.warn("No profiles found after OIDC auth."); return null; diff --git a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jSessionStore.java b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jSessionStore.java index 02612819d7a8..b0187d5e7293 100644 --- a/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jSessionStore.java +++ b/extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jSessionStore.java @@ -25,12 +25,12 @@ import org.apache.druid.java.util.common.logger.Logger; import org.pac4j.core.context.ContextHelper; import org.pac4j.core.context.Cookie; -import org.pac4j.core.context.Pac4jConstants; import org.pac4j.core.context.WebContext; import org.pac4j.core.context.session.SessionStore; import org.pac4j.core.exception.TechnicalException; import org.pac4j.core.profile.CommonProfile; import org.pac4j.core.util.JavaSerializationHelper; +import org.pac4j.core.util.Pac4jConstants; import javax.annotation.Nullable; import java.io.ByteArrayInputStream; @@ -38,6 +38,7 @@ import java.io.IOException; import java.io.Serializable; import java.util.Map; +import java.util.Optional; import java.util.zip.GZIPInputStream; import java.util.zip.GZIPOutputStream; @@ -78,7 +79,7 @@ public String getOrCreateSessionId(WebContext context) @Nullable @Override - public Object get(WebContext context, String key) + public Optional get(WebContext context, String key) { final Cookie cookie = ContextHelper.getCookie(context, PAC4J_SESSION_PREFIX + key); Object value = null; @@ -86,7 +87,7 @@ public Object get(WebContext context, String key) value = uncompressDecryptBase64(cookie.getValue()); } LOGGER.debug("Get from session: [%s] = [%s]", key, value); - return value; + return Optional.ofNullable(value); } @Override @@ -142,7 +143,7 @@ private Serializable uncompressDecryptBase64(final String v) if (v != null && !v.isEmpty()) { byte[] bytes = StringUtils.decodeBase64String(v); if (bytes != null) { - return javaSerializationHelper.unserializeFromBytes(unCompress(cryptoService.decrypt(bytes))); + return javaSerializationHelper.deserializeFromBytes(unCompress(cryptoService.decrypt(bytes))); } } return null; @@ -176,19 +177,19 @@ private Object clearUserProfile(final Object value) { if (value instanceof Map) { final Map profiles = (Map) value; - profiles.forEach((name, profile) -> profile.clearSensitiveData()); + profiles.forEach((name, profile) -> profile.removeLoginData()); return profiles; } else { final CommonProfile profile = (CommonProfile) value; - profile.clearSensitiveData(); + profile.removeLoginData(); return profile; } } @Override - public SessionStore buildFromTrackableSession(WebContext arg0, Object arg1) + public Optional> buildFromTrackableSession(WebContext arg0, Object arg1) { - return null; + return Optional.empty(); } @Override @@ -198,9 +199,9 @@ public boolean destroySession(WebContext arg0) } @Override - public Object getTrackableSession(WebContext arg0) + public Optional getTrackableSession(WebContext arg0) { - return null; + return Optional.empty(); } @Override diff --git a/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jSessionStoreTest.java b/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jSessionStoreTest.java index 0349a98a7ccd..772bef7ef6c3 100644 --- a/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jSessionStoreTest.java +++ b/extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jSessionStoreTest.java @@ -25,15 +25,23 @@ import org.junit.Test; import org.pac4j.core.context.Cookie; import org.pac4j.core.context.WebContext; +import org.pac4j.core.profile.CommonProfile; +import org.pac4j.core.profile.definition.CommonProfileDefinition; import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; public class Pac4jSessionStoreTest { + private static final String COOKIE_PASSPHRASE = "test-cookie-passphrase"; + @Test public void testSetAndGet() { - Pac4jSessionStore sessionStore = new Pac4jSessionStore("test-cookie-passphrase"); + Pac4jSessionStore sessionStore = new Pac4jSessionStore(COOKIE_PASSPHRASE); WebContext webContext1 = EasyMock.mock(WebContext.class); EasyMock.expect(webContext1.getScheme()).andReturn("https"); @@ -54,7 +62,73 @@ public void testSetAndGet() WebContext webContext2 = EasyMock.mock(WebContext.class); EasyMock.expect(webContext2.getRequestCookies()).andReturn(Collections.singletonList(cookie)); EasyMock.replay(webContext2); + Assert.assertEquals("value", Objects.requireNonNull(sessionStore.get(webContext2, "key")).orElse(null)); + } + + @Test + public void testSetAndGetClearUserProfile() + { + Pac4jSessionStore sessionStore = new Pac4jSessionStore(COOKIE_PASSPHRASE); + + WebContext webContext1 = EasyMock.mock(WebContext.class); + EasyMock.expect(webContext1.getScheme()).andReturn("https"); + Capture cookieCapture = EasyMock.newCapture(); + + webContext1.addResponseCookie(EasyMock.capture(cookieCapture)); + EasyMock.replay(webContext1); + + CommonProfile profile = new CommonProfile(); + profile.addAttribute(CommonProfileDefinition.DISPLAY_NAME, "name"); + sessionStore.set(webContext1, "pac4jUserProfiles", profile); + + Cookie cookie = cookieCapture.getValue(); + Assert.assertTrue(cookie.isSecure()); + Assert.assertTrue(cookie.isHttpOnly()); + Assert.assertTrue(cookie.isSecure()); + Assert.assertEquals(900, cookie.getMaxAge()); + + + WebContext webContext2 = EasyMock.mock(WebContext.class); + EasyMock.expect(webContext2.getRequestCookies()).andReturn(Collections.singletonList(cookie)); + EasyMock.replay(webContext2); + Optional value = sessionStore.get(webContext2, "pac4jUserProfiles"); + Assert.assertTrue(Objects.requireNonNull(value).isPresent()); + Assert.assertEquals("name", ((CommonProfile) value.get()).getAttribute(CommonProfileDefinition.DISPLAY_NAME)); + } + + @Test + public void testSetAndGetClearUserMultipleProfile() + { + Pac4jSessionStore sessionStore = new Pac4jSessionStore(COOKIE_PASSPHRASE); + + WebContext webContext1 = EasyMock.mock(WebContext.class); + EasyMock.expect(webContext1.getScheme()).andReturn("https"); + Capture cookieCapture = EasyMock.newCapture(); + + webContext1.addResponseCookie(EasyMock.capture(cookieCapture)); + EasyMock.replay(webContext1); - Assert.assertEquals("value", sessionStore.get(webContext2, "key")); + CommonProfile profile1 = new CommonProfile(); + profile1.addAttribute(CommonProfileDefinition.DISPLAY_NAME, "name1"); + CommonProfile profile2 = new CommonProfile(); + profile2.addAttribute(CommonProfileDefinition.DISPLAY_NAME, "name2"); + Map profiles = new HashMap<>(); + profiles.put("profile1", profile1); + profiles.put("profile2", profile2); + sessionStore.set(webContext1, "pac4jUserProfiles", profiles); + + Cookie cookie = cookieCapture.getValue(); + Assert.assertTrue(cookie.isSecure()); + Assert.assertTrue(cookie.isHttpOnly()); + Assert.assertTrue(cookie.isSecure()); + Assert.assertEquals(900, cookie.getMaxAge()); + + + WebContext webContext2 = EasyMock.mock(WebContext.class); + EasyMock.expect(webContext2.getRequestCookies()).andReturn(Collections.singletonList(cookie)); + EasyMock.replay(webContext2); + Optional value = sessionStore.get(webContext2, "pac4jUserProfiles"); + Assert.assertTrue(Objects.requireNonNull(value).isPresent()); + Assert.assertEquals(2, ((Map) value.get()).size()); } } diff --git a/licenses.yaml b/licenses.yaml index 1d5fe8c0d0d4..2d9fd869edaa 100644 --- a/licenses.yaml +++ b/licenses.yaml @@ -776,7 +776,7 @@ name: pac4j-oidc java security library license_category: binary module: extensions/druid-pac4j license_name: Apache License version 2.0 -version: 3.8.3 +version: 4.5.7 libraries: - org.pac4j: pac4j-oidc @@ -786,7 +786,7 @@ name: pac4j-core java security library license_category: binary module: extensions/druid-pac4j license_name: Apache License version 2.0 -version: 3.8.3 +version: 4.5.7 libraries: - org.pac4j: pac4j-core @@ -837,7 +837,7 @@ name: com.sun.mail javax.mail license_category: binary module: extensions/druid-pac4j license_name: CDDL 1.1 -version: 1.6.1 +version: 1.6.2 libraries: - com.sun.mail: javax.mail diff --git a/owasp-dependency-check-suppressions.xml b/owasp-dependency-check-suppressions.xml index 688baaddfcb7..73369dc62c33 100644 --- a/owasp-dependency-check-suppressions.xml +++ b/owasp-dependency-check-suppressions.xml @@ -588,9 +588,11 @@ - + + + CVE-2021-44878