ashibhardwaj commented on code in PR #18259: URL: https://github.com/apache/druid/pull/18259#discussion_r2304041362
########## 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: You're absolutely right. I was trying to keep the behavior similar to how it was before but logging decrypted session data is definitely a security risk even if it's just for debugging. I'll remove the debug logging from both the get and set methods entirely. If we need debugging for session issues in the future, we can always add safer metadata logging (like just logging the key and whether a value was found) or use debugger breakpoints in development environments. Thanks for catching this! -- 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]
