diff --git a/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java b/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java index 123d2761e008..1d833a000dcd 100644 --- a/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java +++ b/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java @@ -18,6 +18,7 @@ // 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; @@ -25,6 +26,7 @@ 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) .append(" - - [") .append(dateCache.format(request.getTimeStamp())) .append("] \"") diff --git a/client/src/main/java/org/apache/cloudstack/ServerDaemon.java b/client/src/main/java/org/apache/cloudstack/ServerDaemon.java index 196695e1fc6b..09bdb11a6b39 100644 --- a/client/src/main/java/org/apache/cloudstack/ServerDaemon.java +++ b/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(); diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index 21d093758127..db17daaf146b 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/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(","); } diff --git a/server/src/test/java/com/cloud/api/ApiServletTest.java b/server/src/test/java/com/cloud/api/ApiServletTest.java index 4d4f0a12098c..79fe4b86f859 100644 --- a/server/src/test/java/com/cloud/api/ApiServletTest.java +++ b/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 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