capistrant commented on code in PR #18259:
URL: https://github.com/apache/druid/pull/18259#discussion_r2303911906
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jSessionStore.java:
##########
@@ -96,37 +102,103 @@ public void set(WebContext context, String key, @Nullable
Object value)
Object profile = value;
Cookie cookie;
- if (value == null) {
- cookie = new Cookie(PAC4J_SESSION_PREFIX + key, null);
+ // Check if value is null, empty string, or empty collection
+ boolean isEmpty = value == null ||
Review Comment:
would it be an exception case if the value doesn't match any of the types
checked here?
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jSessionStore.java:
##########
@@ -19,71 +19,77 @@
package org.apache.druid.security.pac4j;
-import org.apache.commons.io.IOUtils;
+import com.google.common.base.Preconditions;
+import com.google.common.io.ByteStreams;
import org.apache.druid.crypto.CryptoService;
+import org.apache.druid.error.InvalidInput;
import org.apache.druid.java.util.common.StringUtils;
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.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 org.pac4j.jee.context.JEEContext;
+import org.pac4j.jee.context.session.JEESessionStore;
import javax.annotation.Nullable;
+import javax.servlet.http.Cookie;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
import java.io.Serializable;
+import java.util.Collection;
import java.util.Map;
import java.util.Optional;
import java.util.zip.GZIPInputStream;
import java.util.zip.GZIPOutputStream;
/**
- * Code here is slight adaptation from <a
href="https://github.com/apache/knox/blob/master/gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/session/KnoxSessionStore.java";>KnoxSessionStore</a>
- * for storing oauth session information in cookies.
+ * Code here is slight adaptation from Apache Knox KnoxSessionStore
+ * for storing oauth session information in encrypted cookies.
*/
-public class Pac4jSessionStore<T extends WebContext> implements SessionStore<T>
+public class Pac4jSessionStore implements SessionStore
{
-
private static final Logger LOGGER = new Logger(Pac4jSessionStore.class);
public static final String PAC4J_SESSION_PREFIX = "pac4j.session.";
- private final JavaSerializationHelper javaSerializationHelper;
+ private final JEESessionStore delegate = JEESessionStore.INSTANCE;
private final CryptoService cryptoService;
public Pac4jSessionStore(String cookiePassphrase)
{
- javaSerializationHelper = new JavaSerializationHelper();
- cryptoService = new CryptoService(
- cookiePassphrase,
- "AES",
- "CBC",
- "PKCS5Padding",
- "PBKDF2WithHmacSHA256",
- 128,
- 65536,
- 128
+ this.cryptoService = new CryptoService(
+ cookiePassphrase,
+ "AES",
+ "CBC",
+ "PKCS5Padding",
+ "PBKDF2WithHmacSHA256",
+ 128,
+ 65536,
+ 128
);
}
@Override
- public String getOrCreateSessionId(WebContext context)
+ public Optional<String> getSessionId(WebContext context, boolean
createSession)
{
- return null;
+ if (context instanceof JEEContext) {
+ return delegate.getSessionId(context, createSession);
+ }
+ return Optional.empty();
}
- @Nullable
@Override
public Optional<Object> get(WebContext context, String key)
{
- final Cookie cookie = ContextHelper.getCookie(context,
PAC4J_SESSION_PREFIX + key);
+ final Cookie cookie = getCookie(context, PAC4J_SESSION_PREFIX + key);
Object value = null;
- if (cookie != null) {
+ if (cookie != null && cookie.getValue() != null) {
value = uncompressDecryptBase64(cookie.getValue());
}
LOGGER.debug("Get from session: [%s] = [%s]", key, value);
Review Comment:
I'm a little nervous about us logging a decrypted cookie in plaintext, even
if it is just for debugging. A misconfig by an operator is all it takes from
writing a bunch of these in plaintext. Is it really necessary to ever log this
information? Could we put it behind a config even, like
`druid.pac4j.insecure.debugLogCookies` (I made up the config name structure
here, but just want to make the point of it being some kind of runtime
property)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
