Copilot commented on code in PR #11191: URL: https://github.com/apache/gravitino/pull/11191#discussion_r3286827159
########## server/src/main/java/org/apache/gravitino/server/web/filter/RequestContextFilter.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.gravitino.server.web.filter; + +import java.io.IOException; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import org.apache.gravitino.utils.RequestContext; + +/** + * A servlet filter that captures the client remote address from each HTTP request and stores it in + * {@link RequestContext} so that audit event constructors can read it on the same thread. + * + * <p>When a reverse proxy is in use, the real client IP is taken from the first entry of the {@code + * X-Forwarded-For} header (RFC 7239 §7.1). If the header is absent, {@link Review Comment: The Javadoc claims `X-Forwarded-For` is defined by “RFC 7239 §7.1”, but RFC 7239 defines the `Forwarded` header, not `X-Forwarded-For`. Please correct the reference (or add `Forwarded` parsing if that was the intent) to avoid misleading documentation. ########## server/src/main/java/org/apache/gravitino/server/web/filter/RequestContextFilter.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.gravitino.server.web.filter; + +import java.io.IOException; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import org.apache.gravitino.utils.RequestContext; + +/** + * A servlet filter that captures the client remote address from each HTTP request and stores it in + * {@link RequestContext} so that audit event constructors can read it on the same thread. + * + * <p>When a reverse proxy is in use, the real client IP is taken from the first entry of the {@code + * X-Forwarded-For} header (RFC 7239 §7.1). If the header is absent, {@link + * HttpServletRequest#getRemoteAddr()} is used instead. + * + * <p>The stored value is always cleared in a {@code finally} block to prevent thread-pool leaks. + */ +public class RequestContextFilter implements Filter { + + private static final String X_FORWARDED_FOR = "X-Forwarded-For"; + + @Override + public void init(FilterConfig filterConfig) {} + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + try { + if (request instanceof HttpServletRequest) { + RequestContext.setRemoteAddress(resolveClientAddress((HttpServletRequest) request)); + } + chain.doFilter(request, response); + } finally { + RequestContext.clear(); + } + } + + @Override + public void destroy() {} + + private String resolveClientAddress(HttpServletRequest request) { + String xff = request.getHeader(X_FORWARDED_FOR); + if (xff != null && !xff.isEmpty()) { + return xff.split(",")[0].trim(); + } + return request.getRemoteAddr(); Review Comment: `resolveClientAddress` always trusts the `X-Forwarded-For` header when present. If the server can be reached directly (not only via a trusted reverse proxy), clients can spoof their IP in audit logs by setting this header. Consider only honoring `X-Forwarded-For` when the immediate peer (`getRemoteAddr()`) is in a configured trusted-proxy list, or prefer the RFC 7239 `Forwarded` header with similar trust gating. ########## core/src/test/java/org/apache/gravitino/audit/TestFileAuditWriter.java: ########## @@ -0,0 +1,184 @@ +/* + * 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.gravitino.audit; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.gravitino.audit.v2.SimpleFormatterV2; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.apache.logging.log4j.core.config.AbstractConfiguration; +import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.config.LoggerConfig; +import org.apache.logging.log4j.core.layout.PatternLayout; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class TestFileAuditWriter { + + /** Minimal in-memory appender for capturing log events in tests. */ + static class CaptureAppender extends AbstractAppender { + private final List<LogEvent> events = new ArrayList<>(); + + CaptureAppender(String name) { + super(name, null, PatternLayout.createDefaultLayout(), true, null); + } + + @Override + public void append(LogEvent event) { + events.add(event.toImmutable()); + } + + List<LogEvent> getEvents() { + return events; + } + } + + private CaptureAppender auditCapture; + private CaptureAppender warnCapture; + private LoggerContext loggerContext; + + @BeforeEach + public void setup() { + loggerContext = + (LoggerContext) + org.apache.logging.log4j.LogManager.getContext( + FileAuditWriter.class.getClassLoader(), false); + Configuration config = loggerContext.getConfiguration(); + + auditCapture = new CaptureAppender("auditCapture"); + auditCapture.start(); + config.addAppender(auditCapture); + + // Wire a dedicated logger for gravitino.audit so writes are captured in tests. + LoggerConfig auditLoggerConfig = + new LoggerConfig(FileAuditWriter.AUDIT_LOGGER_NAME, Level.INFO, false); + auditLoggerConfig.addAppender(auditCapture, Level.INFO, null); + config.addLogger(FileAuditWriter.AUDIT_LOGGER_NAME, auditLoggerConfig); + + // Capture WARN logs from FileAuditWriter itself (for deprecation warning tests). + warnCapture = new CaptureAppender("warnCapture"); + warnCapture.start(); + config.addAppender(warnCapture); + String writerLoggerName = FileAuditWriter.class.getName(); + LoggerConfig writerLoggerConfig = new LoggerConfig(writerLoggerName, Level.WARN, false); + writerLoggerConfig.addAppender(warnCapture, Level.WARN, null); + config.addLogger(writerLoggerName, writerLoggerConfig); + + loggerContext.updateLoggers(); + } + + @AfterEach + public void teardown() { + AbstractConfiguration config = (AbstractConfiguration) loggerContext.getConfiguration(); + config.removeLogger(FileAuditWriter.AUDIT_LOGGER_NAME); + config.removeLogger(FileAuditWriter.class.getName()); Review Comment: `setup()` adds two appenders to the Log4j2 configuration, but `teardown()` only removes the logger configs. The appenders remain registered/running across tests, which can leak configuration state and interfere with other Log4j2-based tests. Please remove the appenders from the configuration and stop them in `@AfterEach` as well. ########## iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java: ########## @@ -58,13 +58,21 @@ public IcebergRequestContext(HttpServletRequest httpRequest, String catalogName) public IcebergRequestContext( HttpServletRequest httpRequest, String catalogName, boolean requestCredentialVending) { this.httpServletRequest = httpRequest; - this.remoteHostName = httpRequest.getRemoteHost(); + this.remoteHostName = resolveClientAddress(httpRequest); this.httpHeaders = IcebergRESTUtils.getHttpHeaders(httpRequest); this.catalogName = catalogName; this.userName = PrincipalUtils.getCurrentUserName(); this.requestCredentialVending = requestCredentialVending; } + private static String resolveClientAddress(HttpServletRequest request) { + String xff = request.getHeader("X-Forwarded-For"); + if (xff != null && !xff.isEmpty()) { + return xff.split(",")[0].trim(); + } + return request.getRemoteHost(); Review Comment: This method also unconditionally trusts `X-Forwarded-For`. Without restricting it to requests coming from trusted proxies, callers can spoof the recorded client address by setting the header. Consider gating `X-Forwarded-For` usage on a configured trusted-proxy list (or similar). ########## iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java: ########## @@ -58,13 +58,21 @@ public IcebergRequestContext(HttpServletRequest httpRequest, String catalogName) public IcebergRequestContext( HttpServletRequest httpRequest, String catalogName, boolean requestCredentialVending) { this.httpServletRequest = httpRequest; - this.remoteHostName = httpRequest.getRemoteHost(); + this.remoteHostName = resolveClientAddress(httpRequest); this.httpHeaders = IcebergRESTUtils.getHttpHeaders(httpRequest); this.catalogName = catalogName; this.userName = PrincipalUtils.getCurrentUserName(); this.requestCredentialVending = requestCredentialVending; } + private static String resolveClientAddress(HttpServletRequest request) { + String xff = request.getHeader("X-Forwarded-For"); + if (xff != null && !xff.isEmpty()) { + return xff.split(",")[0].trim(); + } + return request.getRemoteHost(); Review Comment: `resolveClientAddress` falls back to `request.getRemoteHost()`, which may trigger reverse-DNS lookups and can return a hostname rather than the client IP. For accurate/cheap client IP capture (and consistency with the servlet filter), the fallback should use `getRemoteAddr()` instead. ########## core/src/test/java/org/apache/gravitino/audit/TestFileAuditWriter.java: ########## @@ -0,0 +1,184 @@ +/* + * 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.gravitino.audit; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.gravitino.audit.v2.SimpleFormatterV2; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.apache.logging.log4j.core.config.AbstractConfiguration; +import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.config.LoggerConfig; +import org.apache.logging.log4j.core.layout.PatternLayout; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class TestFileAuditWriter { + + /** Minimal in-memory appender for capturing log events in tests. */ + static class CaptureAppender extends AbstractAppender { + private final List<LogEvent> events = new ArrayList<>(); + + CaptureAppender(String name) { + super(name, null, PatternLayout.createDefaultLayout(), true, null); + } + + @Override + public void append(LogEvent event) { + events.add(event.toImmutable()); + } + + List<LogEvent> getEvents() { + return events; + } + } + + private CaptureAppender auditCapture; + private CaptureAppender warnCapture; + private LoggerContext loggerContext; + + @BeforeEach + public void setup() { + loggerContext = + (LoggerContext) + org.apache.logging.log4j.LogManager.getContext( + FileAuditWriter.class.getClassLoader(), false); + Configuration config = loggerContext.getConfiguration(); Review Comment: Avoid using a fully-qualified class name in code (`org.apache.logging.log4j.LogManager...`). Import `LogManager` instead to follow the project’s Java import hygiene rules and keep the code consistent/readable. ########## conf/log4j2.properties.template: ########## @@ -72,6 +72,33 @@ logger.lineage.level = info logger.lineage.appenderRef.lineage_file.ref = lineage_file logger.lineage.additivity = false +## use separate file for audit log +appender.audit_file.type = RollingFile +appender.audit_file.name = audit_file +appender.audit_file.fileName = ${basePath}/gravitino_audit.log +appender.audit_file.filePattern = ${basePath}/gravitino_audit_%d{yyyyMMdd}.%i.log.gz +appender.audit_file.layout.type = PatternLayout +appender.audit_file.layout.pattern = %msg%n +appender.audit_file.policies.type = Policies +appender.audit_file.policies.size.type = SizeBasedTriggeringPolicy +appender.audit_file.policies.size.size = 256MB +appender.audit_file.policies.time.type = TimeBasedTriggeringPolicy +appender.audit_file.policies.time.interval = 1 +appender.audit_file.policies.time.modulate = true +appender.audit_file.strategy.type = DefaultRolloverStrategy +appender.audit_file.strategy.delete.type = Delete +appender.audit_file.strategy.delete.basePath = ${basePath} +appender.audit_file.strategy.delete.maxDepth = 10 +appender.audit_file.strategy.delete.ifLastModified.type = IfLastModified + +# Delete all audit log files older than 30 days +appender.audit_file.strategy.delete.ifLastModified.age = 30d Review Comment: The `Delete` action under `appender.audit_file.strategy` is only constrained by `IfLastModified`, so it can delete *any* file under `${basePath}` older than the age threshold, not just audit logs (despite the comment). Consider adding an `IfFileName` (e.g., `gravitino_audit*`) or similar path condition so retention applies only to the audit rolling files. -- 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]
