Upgrade pac4j-oidc to 4.5.7 to address CVE-2021-44878 (apache#15522)

* Upgrade org.pac4j:pac4j-oidc to 4.5.5 to address CVE-2021-44878
* add CVE suppression and notes, since vulnerability scan still shows this CVE
* Add tests to improve coverage

extensions-core/druid-pac4j/pom.xml

@@ -34,7 +34,7 @@
   </parent>
 
   <properties>
-    <pac4j.version>3.8.3</pac4j.version>
+    <pac4j.version>4.5.7</pac4j.version>
 
     <!-- Following must be updated along with any updates to pac4j version -->
     <nimbus.lang.tag.version>1.7</nimbus.lang.tag.version>

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<String, J2EContext> NOOP_HTTP_ACTION_ADAPTER = (int code, J2EContext ctx) -> null;
+  private static final HttpActionAdapter<String, JEEContext> NOOP_HTTP_ACTION_ADAPTER = (HttpAction code, JEEContext ctx) -> null;
 
   private final Config pac4jConfig;
-  private final SecurityLogic<String, J2EContext> securityLogic;
-  private final CallbackLogic<String, J2EContext> callbackLogic;
-  private final SessionStore<J2EContext> sessionStore;
+  private final SecurityLogic<String, JEEContext> securityLogic;
+  private final CallbackLogic<String, JEEContext> callbackLogic;
+  private final SessionStore<JEEContext> 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<CommonProfile> profiles, Object... parameters) -> {
+          (JEEContext ctx, Collection<UserProfile> profiles, Object... parameters) -> {
             if (profiles.isEmpty()) {
               LOGGER.warn("No profiles found after OIDC auth.");
               return null;

extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jSessionStore.java

@@ -25,19 +25,20 @@
 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;
 import java.io.ByteArrayOutputStream;
 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,15 +79,15 @@ public String getOrCreateSessionId(WebContext context)
 
   @Nullable
   @Override
-  public Object get(WebContext context, String key)
+  public Optional<Object> get(WebContext context, String key)
   {
     final Cookie cookie = ContextHelper.getCookie(context, PAC4J_SESSION_PREFIX + key);
     Object value = null;
     if (cookie != null) {
       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<String, CommonProfile> profiles = (Map<String, CommonProfile>) 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<SessionStore<T>> 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

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<WebContext> sessionStore = new Pac4jSessionStore("test-cookie-passphrase");
+    Pac4jSessionStore<WebContext> 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<WebContext> sessionStore = new Pac4jSessionStore(COOKIE_PASSPHRASE);
+
+    WebContext webContext1 = EasyMock.mock(WebContext.class);
+    EasyMock.expect(webContext1.getScheme()).andReturn("https");
+    Capture<Cookie> 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<Object> 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<WebContext> sessionStore = new Pac4jSessionStore(COOKIE_PASSPHRASE);
+
+    WebContext webContext1 = EasyMock.mock(WebContext.class);
+    EasyMock.expect(webContext1.getScheme()).andReturn("https");
+    Capture<Cookie> 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<String, CommonProfile> 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<Object> value = sessionStore.get(webContext2, "pac4jUserProfiles");
+    Assert.assertTrue(Objects.requireNonNull(value).isPresent());
+    Assert.assertEquals(2, ((Map<String, CommonProfile>) value.get()).size());
   }
 }

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
 

owasp-dependency-check-suppressions.xml

@@ -577,9 +577,11 @@
   </suppress>
 
   <suppress>
-    <!-- pac4j-core-3.8.3 -->
+    <!-- 4.5.5 is a fixed version as per https://nvd.nist.gov/vuln/detail/CVE-2021-44878. -->
+    <!-- However, vulnerability scan still shows this CVE. Pac4j release notes mention 5.3.1 as "fully fixed" version. -->
+    <!-- Remove suppression once upgraded to 5.3.1. -->
     <notes><![CDATA[
-   file name: pac4j-core-3.8.3.jar
+   file name: pac4j-core-4.5.7.jar
    ]]></notes>
     <cve>CVE-2021-44878</cve>
   </suppress>