Copilot commented on code in PR #847: URL: https://github.com/apache/ranger/pull/847#discussion_r2897517904
########## ranger-audit-server/ranger-audit-consumer-hdfs/src/main/java/org/apache/ranger/audit/server/HdfsConsumerConfig.java: ########## @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.ranger.audit.server; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Loads HDFS consumer-specific configuration files including Hadoop configs. + */ +public class HdfsConsumerConfig extends AuditConfig { + private static final Logger LOG = LoggerFactory.getLogger(HdfsConsumerConfig.class); + private static final String CONFIG_FILE_PATH = "conf/ranger-audit-consumer-hdfs-site.xml"; + private static final String CORE_SITE_FILE_PATH = "conf/core-site.xml"; + private static final String HDFS_SITE_FILE_PATH = "conf/hdfs-site.xml"; + private static volatile HdfsConsumerConfig sInstance; + + private HdfsConsumerConfig() { + super(); + addHdfsConsumerResources(); + } + + public static HdfsConsumerConfig getInstance() { + HdfsConsumerConfig ret = HdfsConsumerConfig.sInstance; + + if (ret == null) { + synchronized (HdfsConsumerConfig.class) { + ret = HdfsConsumerConfig.sInstance; + + if (ret == null) { + ret = new HdfsConsumerConfig(); + HdfsConsumerConfig.sInstance = ret; + } + } + } + + return ret; + } + + private boolean addHdfsConsumerResources() { + LOG.debug("==> HdfsConsumerConfig.addHdfsConsumerResources()"); + + boolean ret = true; + + if (!addAuditResource(CORE_SITE_FILE_PATH, false)) { + LOG.warn("Could not load required configuration: {}", CORE_SITE_FILE_PATH); + ret = false; + } + + if (!addAuditResource(HDFS_SITE_FILE_PATH, true)) { + LOG.error("Could not load required configuration: {}", HDFS_SITE_FILE_PATH); + ret = false; + } + + if (!addAuditResource(CONFIG_FILE_PATH, true)) { + LOG.error("Could not load required configuration: {}", CONFIG_FILE_PATH); + ret = false; + } Review Comment: `addAuditResource(..., required=false)` returns `true` even when the optional file is missing (see `AuditConfig.addAuditResource()`), so `if (!addAuditResource(CORE_SITE_FILE_PATH, false)) { ... }` will never execute. This makes the `ret=false` branch dead code and obscures the intended behavior. Consider either (a) calling `addResourceIfReadable()` directly when you need the boolean result, or (b) removing the conditional block and relying on `addAuditResource()`'s internal logging for optional resources. ########## ranger-audit-server/ranger-audit-server-service/src/main/java/javax/ws/rs/core/NoContentException.java: ########## @@ -0,0 +1,43 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package javax.ws.rs.core; + +import java.io.IOException; + +/* Workaround: use of JacksonJaxbJsonProvider in RangerJsonProvider along with jersey-bundle results in following initialization failure: + * SEVERE: Allocate exception for servlet [REST Service] + * java.lang.ClassNotFoundException: javax.ws.rs.core.NoContentException + * at org.apache.catalina.loader.WebappClassLoaderBase.loadClass(WebappClassLoaderBase.java:1338) + * at org.apache.catalina.loader.WebappClassLoaderBase.loadClass(WebappClassLoaderBase.java:1150) + */ +public class NoContentException extends IOException { + private static final long serialVersionUID = -3082577759787473245L; + + public NoContentException(String message) { + super(message); + } + + public NoContentException(String message, Throwable cause) { + super(message, cause); + } + + public NoContentException(Throwable cause) { + super(cause); + } +} Review Comment: This introduces a new class under the `javax.ws.rs.core` namespace. Shipping application classes in `javax.*` can lead to classloading conflicts (split packages) with the JAX-RS implementation provided by the container/dependencies, and can break in environments where the package is sealed. Prefer fixing the underlying dependency mismatch (ensure the JAX-RS API jar providing `NoContentException` is present/compatible) rather than adding a `javax.*` shim class to the application. ########## ranger-audit-server/ranger-audit-server-service/src/main/java/org/apache/ranger/audit/security/FilterChainWrapper.java: ########## @@ -0,0 +1,159 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.ranger.audit.security; + +import org.apache.commons.lang3.StringUtils; +import org.apache.hadoop.security.authentication.client.AuthenticatedURL; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.security.authentication.AbstractAuthenticationToken; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.core.userdetails.User; +import org.springframework.security.core.userdetails.UserDetails; +import org.springframework.security.web.authentication.WebAuthenticationDetails; + +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; + +/** + * Ranger Audit specific {@link javax.servlet.FilterChain filter-chain} wrapper. + * This wrapper class will be used to perform additional activities after + * {@link javax.servlet.Filter filter} based authentication. + */ +public class FilterChainWrapper implements FilterChain { + private static final Logger LOG = LoggerFactory.getLogger(FilterChainWrapper.class); + + private static final String DEFAULT_AUDIT_ROLE = "ROLE_USER"; + private static final String COOKIE_PARAM = "Set-Cookie"; + private FilterChain filterChain; + + // Constructor + public FilterChainWrapper(final FilterChain filterChain) { + this.filterChain = filterChain; + } + + /** + * Read user from cookie. + * + * @param httpResponse + * @return + */ + private static String readUserFromCookie(HttpServletResponse httpResponse) { + String userName = null; + boolean isCookieSet = httpResponse.containsHeader(COOKIE_PARAM); + + if (isCookieSet) { + Collection<String> cookies = httpResponse.getHeaders(COOKIE_PARAM); + + if (cookies != null) { + for (String cookie : cookies) { + if (!StringUtils.startsWithIgnoreCase(cookie, AuthenticatedURL.AUTH_COOKIE) + || !cookie.contains("u=")) { + continue; + } + + String[] split = cookie.split(";"); + + for (String s : split) { + if (StringUtils.startsWithIgnoreCase(s, AuthenticatedURL.AUTH_COOKIE)) { + int idxUEq = s.indexOf("u="); + + if (idxUEq == -1) { + continue; + } + + int idxAmp = s.indexOf("&", idxUEq); + + if (idxAmp == -1) { + continue; + } + + try { + userName = s.substring(idxUEq + 2, idxAmp); + break; + } catch (Exception e) { + userName = null; + } + } + } + } + } + } + + return userName; + } + + /** + * This method sets spring security {@link org.springframework.security.core.context.SecurityContextHolder context} + * with {@link org.springframework.security.core.Authentication authentication}, + * if not already authenticated by other filter. And carried on with {@link javax.servlet.FilterChain filter-chain}. + * This method will be invoked by {@link org.apache.hadoop.security.authentication.server.AuthenticationFilter AuthenticationFilter}, + * only when user is authenticated. + */ + @Override + public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse) throws IOException, ServletException { + final HttpServletRequest httpRequest = (HttpServletRequest) servletRequest; + final HttpServletResponse httpResponse = (HttpServletResponse) servletResponse; + final Authentication existingAuth = SecurityContextHolder.getContext().getAuthentication(); + String userName = readUserFromCookie(httpResponse); + + if (StringUtils.isEmpty(userName) && StringUtils.isNotEmpty(httpRequest.getRemoteUser())) { + userName = httpRequest.getRemoteUser(); + } + + if ((existingAuth == null || !existingAuth.isAuthenticated()) && StringUtils.isNotEmpty(userName)) { + String doAsUser = httpRequest.getParameter("doAs"); + if (StringUtils.isNotEmpty(doAsUser)) { + userName = doAsUser; + } Review Comment: `FilterChainWrapper` honors a `doAs` request parameter by overwriting the authenticated username without any additional checks. In Ranger's admin KRB filters, `doAs` is typically gated behind an explicit `allowTrustedProxy` config and an already-authenticated proxy user. Please add equivalent trusted-proxy validation (or remove `doAs` support) to avoid allowing arbitrary impersonation via `?doAs=`. ########## ranger-audit-server/ranger-audit-server-service/src/main/java/org/apache/ranger/audit/producer/AuditDestinationMgr.java: ########## @@ -0,0 +1,137 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.ranger.audit.producer; + +import org.apache.ranger.audit.model.AuditEventBase; +import org.apache.ranger.audit.model.AuthzAuditEvent; +import org.apache.ranger.audit.producer.kafka.AuditMessageQueue; +import org.apache.ranger.audit.provider.AuditHandler; +import org.apache.ranger.audit.provider.AuditProviderFactory; +import org.apache.ranger.audit.provider.MiscUtil; +import org.apache.ranger.audit.server.AuditServerConfig; +import org.apache.ranger.audit.server.AuditServerConstants; +import org.apache.ranger.audit.utils.AuditServerUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Component; + +import javax.annotation.PostConstruct; +import javax.annotation.PreDestroy; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Properties; + +@Component +public class AuditDestinationMgr { + private static final Logger LOG = LoggerFactory.getLogger(AuditDestinationMgr.class); + + public AuditHandler auditHandler; + public AuditServerUtils auditServerUtils; + + @PostConstruct + public void configure() { + init(); + } + + public void init() { + LOG.info("==> AuditDestinationMgr.init()"); + + auditServerUtils = new AuditServerUtils(); + AuditServerConfig auditConfig = AuditServerConfig.getInstance(); + Properties properties = auditConfig.getProperties(); + if (properties != null) { + auditServerUtils.setAuditConfig(properties); + } + + String kafkaDestPrefix = AuditProviderFactory.AUDIT_DEST_BASE + "." + AuditServerConstants.DEFAULT_SERVICE_NAME; + boolean isAuditToKafkaDestinationEnabled = MiscUtil.getBooleanProperty(properties, kafkaDestPrefix, false); + if (isAuditToKafkaDestinationEnabled) { + auditHandler = new AuditMessageQueue(); + auditHandler.init(properties, kafkaDestPrefix); + auditHandler.start(); + LOG.info("Kafka producer initialized and started"); + } else { + LOG.warn("Kafka audit destination is not enabled. Producer service will not function."); + } + + LOG.info("<== AuditDestinationMgr.init() AuditDestination: {} ", kafkaDestPrefix); + } + + public boolean log(AuthzAuditEvent authzAuditEvent) throws Exception { + boolean ret = false; + + if (auditHandler == null) { + init(); + } + ret = auditHandler.log(authzAuditEvent); + Review Comment: If Kafka audit destination is disabled, `init()` logs a warning and leaves `auditHandler` as null. `log()`/`logBatch()` then call `auditHandler.log(...)` unconditionally, which will throw a NullPointerException. Please handle the disabled/unconfigured case explicitly (e.g., return false/throw a clear exception) after calling `init()`, or ensure `auditHandler` is always initialized to a no-op handler. ########## ranger-audit-server/ranger-audit-server-service/src/main/java/org/apache/ranger/audit/security/NullServletContext.java: ########## @@ -0,0 +1,240 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.ranger.audit.security; + +import javax.servlet.Filter; +import javax.servlet.FilterRegistration; +import javax.servlet.FilterRegistration.Dynamic; +import javax.servlet.RequestDispatcher; +import javax.servlet.Servlet; +import javax.servlet.ServletContext; +import javax.servlet.ServletException; +import javax.servlet.SessionCookieConfig; +import javax.servlet.SessionTrackingMode; +import javax.servlet.descriptor.JspConfigDescriptor; + +import java.io.InputStream; +import java.net.MalformedURLException; +import java.net.URL; +import java.util.Enumeration; +import java.util.EventListener; +import java.util.Map; +import java.util.Set; + +public class NullServletContext implements ServletContext { + public void setSessionTrackingModes(Set<SessionTrackingMode> sessionTrackingModes) { + } + + public boolean setInitParameter(String name, String value) { + return false; + } + + public void setAttribute(String name, Object object) { + } + + public void removeAttribute(String name) { + } + + public void log(String message, Throwable throwable) { + } + + public void log(Exception exception, String msg) { + } + + public void log(String msg) { + } + + public String getVirtualServerName() { + return null; + } + + public SessionCookieConfig getSessionCookieConfig() { + return null; + } + + public Enumeration<Servlet> getServlets() { + return null; + } + + public Map<String, ? extends javax.servlet.ServletRegistration> getServletRegistrations() { + return null; + } + + public javax.servlet.ServletRegistration getServletRegistration(String servletName) { + return null; + } + + public Enumeration<String> getServletNames() { + return null; + } + + public String getServletContextName() { + return null; + } + + public Servlet getServlet(String name) throws ServletException { + return null; + } + + public String getServerInfo() { + return null; + } + + public Set<String> getResourcePaths(String path) { + return null; + } + + public InputStream getResourceAsStream(String path) { + return null; + } + + public URL getResource(String path) throws MalformedURLException { + return null; + } + + public RequestDispatcher getRequestDispatcher(String path) { + return null; + } + + public String getRealPath(String path) { + return null; + } + + public RequestDispatcher getNamedDispatcher(String name) { + return null; + } + + public int getMinorVersion() { + return 0; + } + + public String getMimeType(String file) { + return null; + } + + public int getMajorVersion() { + return 0; + } + + public JspConfigDescriptor getJspConfigDescriptor() { + return null; + } + + public Enumeration<String> getInitParameterNames() { + return null; + } + + public String getInitParameter(String name) { + return null; + } + + public Map<String, ? extends FilterRegistration> getFilterRegistrations() { + return null; + } + + public FilterRegistration getFilterRegistration(String filterName) { + return null; + } + + public Set<SessionTrackingMode> getEffectiveSessionTrackingModes() { + return null; + } + + public int getEffectiveMinorVersion() { + return 0; + } + + public int getEffectiveMajorVersion() { + return 0; + } + + public Set<SessionTrackingMode> getDefaultSessionTrackingModes() { + return null; + } + + public String getContextPath() { + return null; + } + + public ServletContext getContext(String uripath) { + return null; + } + + public ClassLoader getClassLoader() { + return null; + } + + public Enumeration<String> getAttributeNames() { + return null; + } + + public Object getAttribute(String name) { + return null; + } + + public void declareRoles(String... roleNames) { + } + + public <T extends Servlet> T createServlet(Class<T> clazz) throws ServletException { + return null; + } + + public <T extends EventListener> T createListener(Class<T> clazz) throws ServletException { + return null; + } + + public <T extends Filter> T createFilter(Class<T> clazz) throws ServletException { + return null; + } + + public javax.servlet.ServletRegistration.Dynamic addServlet(String servletName, + Class<? extends Servlet> servletClass) { + return null; + } + + public javax.servlet.ServletRegistration.Dynamic addServlet(String servletName, Servlet servlet) { + return null; + } + + public javax.servlet.ServletRegistration.Dynamic addServlet(String servletName, String className) { + return null; + } + + public void addListener(Class<? extends EventListener> listenerClass) { + } + + public <T extends EventListener> void addListener(T t) { + } + + public void addListener(String className) { + } + + public Dynamic addFilter(String filterName, Class<? extends Filter> filterClass) { + return null; + } + + public Dynamic addFilter(String filterName, Filter filter) { + return null; + } + + public Dynamic addFilter(String filterName, String className) { + return null; + } +} Review Comment: `NullServletContext` implements `javax.servlet.ServletContext` (servlet-api 3.1.0), but it doesn't implement all required methods (e.g., `addJspFile(...)` and others added in Servlet 3.x). As a concrete class, this will fail compilation. Consider extending a minimal adapter (if available in the codebase) or implement the missing `ServletContext` methods (returning null/empty/false as appropriate) so the class compiles against Servlet 3.1.0. -- 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]
