Fix logging of forwarded IPs in logs (apache#11854)

Author: abh1sar

client/src/main/java/org/apache/cloudstack/ACSRequestLog.java

@@ -18,13 +18,15 @@
 //
 package org.apache.cloudstack;
 
+import com.cloud.api.ApiServlet;
 import com.cloud.utils.StringUtils;
 import org.eclipse.jetty.server.NCSARequestLog;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.util.DateCache;
 import org.eclipse.jetty.util.component.LifeCycle;
 
+import java.net.InetAddress;
 import java.util.Locale;
 import java.util.TimeZone;
 
@@ -51,9 +53,8 @@ public void log(Request request, Response response) {
             StringBuilder sb = buffers.get();
             sb.setLength(0);
 
-            sb.append(request.getHttpChannel().getEndPoint()
-                            .getRemoteAddress().getAddress()
-                            .getHostAddress())
+            InetAddress remoteAddress = ApiServlet.getClientAddress(request);
+            sb.append(remoteAddress.getHostAddress())
                     .append(" - - [")
                     .append(dateCache.format(request.getTimeStamp()))
                     .append("] \"")

client/src/main/java/org/apache/cloudstack/ServerDaemon.java

@@ -24,15 +24,12 @@
 import java.io.InputStream;
 import java.lang.management.ManagementFactory;
 import java.net.URL;
-import java.util.Arrays;
 import java.util.Properties;
 
-import com.cloud.api.ApiServer;
 import org.apache.commons.daemon.Daemon;
 import org.apache.commons.daemon.DaemonContext;
 import org.apache.commons.lang3.StringUtils;
 import org.eclipse.jetty.jmx.MBeanContainer;
-import org.eclipse.jetty.server.ForwardedRequestCustomizer;
 import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.HttpConnectionFactory;
 import org.eclipse.jetty.server.RequestLog;
@@ -193,7 +190,6 @@ public void start() throws Exception {
         httpConfig.setResponseHeaderSize(8192);
         httpConfig.setSendServerVersion(false);
         httpConfig.setSendDateHeader(false);
-        addForwardingCustomiser(httpConfig);
 
         // HTTP Connector
         createHttpConnector(httpConfig);
@@ -216,21 +212,6 @@ public void start() throws Exception {
         server.join();
     }
 
-    /**
-     * Adds a ForwardedRequestCustomizer to the HTTP configuration to handle forwarded headers.
-     * The header used for forwarding is determined by the ApiServer.listOfForwardHeaders property.
-     * Only non empty headers are considered and only the first of the comma-separated list is used.
-     * @param httpConfig the HTTP configuration to which the customizer will be added
-     */
-    private static void addForwardingCustomiser(HttpConfiguration httpConfig) {
-        ForwardedRequestCustomizer customiser = new ForwardedRequestCustomizer();
-        String header = Arrays.stream(ApiServer.listOfForwardHeaders.value().split(",")).findFirst().orElse(null);
-        if (com.cloud.utils.StringUtils.isNotEmpty(header)) {
-            customiser.setForwardedForHeader(header);
-        }
-        httpConfig.addCustomizer(customiser);
-    }
-
     @Override
     public void stop() throws Exception {
         server.stop();

server/src/main/java/com/cloud/api/ApiServlet.java

@@ -679,38 +679,34 @@ private boolean isSpecificAPI(String commandName) {
         }
         return false;
     }
-    boolean doUseForwardHeaders() {
+    static boolean doUseForwardHeaders() {
         return Boolean.TRUE.equals(ApiServer.useForwardHeader.value());
     }
 
-    String[] proxyNets() {
+    static String[] proxyNets() {
         return ApiServer.proxyForwardList.value().split(",");
     }
     //This method will try to get login IP of user even if servlet is behind reverseProxy or loadBalancer
-    public InetAddress getClientAddress(final HttpServletRequest request) throws UnknownHostException {
-        String ip = null;
-        InetAddress pretender = InetAddress.getByName(request.getRemoteAddr());
-        if(doUseForwardHeaders()) {
-            if (NetUtils.isIpInCidrList(pretender, proxyNets())) {
+    public static InetAddress getClientAddress(final HttpServletRequest request) throws UnknownHostException {
+        final String remote = request.getRemoteAddr();
+        if (doUseForwardHeaders()) {
+            final InetAddress remoteAddr = InetAddress.getByName(remote);
+            if (NetUtils.isIpInCidrList(remoteAddr, proxyNets())) {
                 for (String header : getClientAddressHeaders()) {
                     header = header.trim();
-                    ip = getCorrectIPAddress(request.getHeader(header));
+                    final String ip = getCorrectIPAddress(request.getHeader(header));
                     if (StringUtils.isNotBlank(ip)) {
-                        LOGGER.debug(String.format("found ip %s in header %s ", ip, header));
-                        break;
+                        LOGGER.debug("found ip {} in header {}", ip, header);
+                        return InetAddress.getByName(ip);
                     }
-                } // no address found in header so ip is blank and use remote addr
-            } // else not an allowed proxy address, ip is blank and use remote addr
-        }
-        if (StringUtils.isBlank(ip)) {
-            LOGGER.trace(String.format("no ip found in headers, returning remote address %s.", pretender.getHostAddress()));
-            return pretender;
+                }
+            }
         }
-
-        return InetAddress.getByName(ip);
+        LOGGER.trace("no ip found in headers, returning remote address {}.", remote);
+        return InetAddress.getByName(remote);
     }
 
-    private String[] getClientAddressHeaders() {
+    private static String[] getClientAddressHeaders() {
         return ApiServer.listOfForwardHeaders.value().split(",");
     }
 

server/src/test/java/com/cloud/api/ApiServletTest.java

@@ -33,6 +33,7 @@
 import org.apache.cloudstack.api.auth.APIAuthenticator;
 import org.apache.cloudstack.api.command.admin.config.ListCfgsByCmd;
 import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
@@ -97,17 +98,20 @@ public class ApiServletTest {
     @Mock
     AccountService accountMgr;
 
-    @Mock ConfigKey<Boolean> useForwardHeader;
+    @Mock
+    ConfigDepotImpl mockConfigDepot;
+
     StringWriter responseWriter;
 
     ApiServlet servlet;
-    ApiServlet spyServlet;
+
+    private ConfigDepotImpl originalConfigDepot;
+
     @SuppressWarnings("unchecked")
     @Before
     public void setup() throws SecurityException, NoSuchFieldException,
-    IllegalArgumentException, IllegalAccessException, IOException, UnknownHostException {
+            IllegalArgumentException, IllegalAccessException, IOException {
         servlet = new ApiServlet();
-        spyServlet = Mockito.spy(servlet);
         responseWriter = new StringWriter();
         Mockito.when(response.getWriter()).thenReturn(
                 new PrintWriter(responseWriter));
@@ -131,6 +135,7 @@ public void setup() throws SecurityException, NoSuchFieldException,
         apiServerField.setAccessible(true);
         apiServerField.set(servlet, apiServer);
 
+        setupConfigDepotMock();
     }
 
     /**
@@ -151,6 +156,33 @@ public void cleanupEnvironmentHacks() throws Exception {
         Field smsField = ApiDBUtils.class.getDeclaredField("s_ms");
         smsField.setAccessible(true);
         smsField.set(null, null);
+        restoreConfigDepot();
+    }
+
+    private void setupConfigDepotMock() throws NoSuchFieldException, IllegalAccessException {
+        Field depotField = ConfigKey.class.getDeclaredField("s_depot");
+        depotField.setAccessible(true);
+        originalConfigDepot = (ConfigDepotImpl) depotField.get(null);
+        depotField.set(null, mockConfigDepot);
+        Mockito.when(mockConfigDepot.getConfigStringValue(
+            Mockito.anyString(),
+            Mockito.any(ConfigKey.Scope.class),
+            Mockito.any()
+        )).thenReturn(null);
+    }
+
+    private void restoreConfigDepot() throws Exception {
+        Field depotField = ConfigKey.class.getDeclaredField("s_depot");
+        depotField.setAccessible(true);
+        depotField.set(null, originalConfigDepot);
+    }
+
+    private void setConfigValue(String configName, String value) {
+        Mockito.when(mockConfigDepot.getConfigStringValue(
+            Mockito.eq(configName),
+            Mockito.eq(ConfigKey.Scope.Global),
+            Mockito.isNull()
+        )).thenReturn(value);
     }
 
     @Test
@@ -261,43 +293,40 @@ public void processRequestInContextLogin() throws UnknownHostException {
 
     @Test
     public void getClientAddressWithXForwardedFor() throws UnknownHostException {
-        String[] proxynet = {"127.0.0.0/8"};
-        Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
-        Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
+        setConfigValue("proxy.header.verify", "true");
+        setConfigValue("proxy.cidr", "127.0.0.0/8");
+        Mockito.when(request.getRemoteAddr()).thenReturn("127.0.0.1");
         Mockito.when(request.getHeader(Mockito.eq("X-Forwarded-For"))).thenReturn("192.168.1.1");
-        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
+        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
     }
 
     @Test
     public void getClientAddressWithHttpXForwardedFor() throws UnknownHostException {
-        String[] proxynet = {"127.0.0.0/8"};
-        Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
-        Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
+        setConfigValue("proxy.header.verify", "true");
+        setConfigValue("proxy.cidr", "127.0.0.0/8");
         Mockito.when(request.getHeader(Mockito.eq("HTTP_X_FORWARDED_FOR"))).thenReturn("192.168.1.1");
-        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
+        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
     }
 
     @Test
     public void getClientAddressWithRemoteAddr() throws UnknownHostException {
-        String[] proxynet = {"127.0.0.0/8"};
-        Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
-        Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
-        Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
+        setConfigValue("proxy.header.verify", "true");
+        setConfigValue("proxy.cidr", "127.0.0.0/8");
+        Assert.assertEquals(InetAddress.getByName("127.0.0.1"), ApiServlet.getClientAddress(request));
     }
 
     @Test
     public void getClientAddressWithHttpClientIp() throws UnknownHostException {
-        String[] proxynet = {"127.0.0.0/8"};
-        Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
-        Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
+        setConfigValue("proxy.header.verify", "true");
+        setConfigValue("proxy.cidr", "127.0.0.0/8");
         Mockito.when(request.getHeader(Mockito.eq("HTTP_CLIENT_IP"))).thenReturn("192.168.1.1");
-        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
+        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
     }
 
     @Test
     public void getClientAddressDefault() throws UnknownHostException {
         Mockito.when(request.getRemoteAddr()).thenReturn("127.0.0.1");
-        Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
+        Assert.assertEquals(InetAddress.getByName("127.0.0.1"), ApiServlet.getClientAddress(request));
     }
 
     @Test