From 0ebddf732615cca5bc8855aa2b874abb3d654fb1 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Thu, 16 Oct 2025 19:14:07 +0530 Subject: [PATCH 1/5] log the forwarded ip address in access log if the setting proxy.header.verify is enabled --- .../org/apache/cloudstack/ACSRequestLog.java | 12 +++++++++--- .../org/apache/cloudstack/ServerDaemon.java | 19 ------------------- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java b/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java index 123d2761e008..856cbb696757 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,12 +26,18 @@ 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; import static org.apache.commons.configuration.DataConfiguration.DEFAULT_DATE_FORMAT; +import javax.inject.Inject; + public class ACSRequestLog extends NCSARequestLog { + @Inject + ApiServlet apiServlet; + private static final ThreadLocal buffers = ThreadLocal.withInitial(() -> new StringBuilder(256)); @@ -51,9 +58,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(); From 183482b5b22c13270f288382848e370ddd46fa79 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Fri, 17 Oct 2025 19:14:39 +0530 Subject: [PATCH 2/5] use static functions from ApiServlet and code refactoring --- .../org/apache/cloudstack/ACSRequestLog.java | 7 +--- .../main/java/com/cloud/api/ApiServlet.java | 34 ++++++++----------- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java b/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java index 856cbb696757..1d833a000dcd 100644 --- a/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java +++ b/client/src/main/java/org/apache/cloudstack/ACSRequestLog.java @@ -32,12 +32,7 @@ import static org.apache.commons.configuration.DataConfiguration.DEFAULT_DATE_FORMAT; -import javax.inject.Inject; - public class ACSRequestLog extends NCSARequestLog { - @Inject - ApiServlet apiServlet; - private static final ThreadLocal buffers = ThreadLocal.withInitial(() -> new StringBuilder(256)); @@ -58,7 +53,7 @@ public void log(Request request, Response response) { StringBuilder sb = buffers.get(); sb.setLength(0); - InetAddress remoteAddress = apiServlet.getClientAddress(request); + InetAddress remoteAddress = ApiServlet.getClientAddress(request); sb.append(remoteAddress) .append(" - - [") .append(dateCache.format(request.getTimeStamp())) 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(","); } From 2ca1e10ab2feff3f84464f8770bd9d1aa0fa4a79 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Tue, 21 Oct 2025 19:42:56 +0530 Subject: [PATCH 3/5] Fix ApiServletTest.java --- .../java/com/cloud/api/ApiServletTest.java | 73 +++++++++++++------ 1 file changed, 51 insertions(+), 22 deletions(-) diff --git a/server/src/test/java/com/cloud/api/ApiServletTest.java b/server/src/test/java/com/cloud/api/ApiServletTest.java index 4d4f0a12098c..e5ce908f9b9a 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)); @@ -130,7 +134,8 @@ public void setup() throws SecurityException, NoSuchFieldException, Field apiServerField = ApiServlet.class.getDeclaredField("apiServer"); 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 From cab742899a17d412e31fb7f625d7a07c730f44fc Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Tue, 21 Oct 2025 19:47:19 +0530 Subject: [PATCH 4/5] fix precommit --- server/src/test/java/com/cloud/api/ApiServletTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/com/cloud/api/ApiServletTest.java b/server/src/test/java/com/cloud/api/ApiServletTest.java index e5ce908f9b9a..005847d5ba57 100644 --- a/server/src/test/java/com/cloud/api/ApiServletTest.java +++ b/server/src/test/java/com/cloud/api/ApiServletTest.java @@ -134,7 +134,7 @@ public void setup() throws SecurityException, NoSuchFieldException, Field apiServerField = ApiServlet.class.getDeclaredField("apiServer"); apiServerField.setAccessible(true); apiServerField.set(servlet, apiServer); - + setupConfigDepotMock(); } From cad2aa4a91da8bf1eea77d09961f2ab09ed92473 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Tue, 21 Oct 2025 19:53:45 +0530 Subject: [PATCH 5/5] fix precommit --- .../src/test/java/com/cloud/api/ApiServletTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/com/cloud/api/ApiServletTest.java b/server/src/test/java/com/cloud/api/ApiServletTest.java index 005847d5ba57..79fe4b86f859 100644 --- a/server/src/test/java/com/cloud/api/ApiServletTest.java +++ b/server/src/test/java/com/cloud/api/ApiServletTest.java @@ -104,7 +104,7 @@ public class ApiServletTest { StringWriter responseWriter; ApiServlet servlet; - + private ConfigDepotImpl originalConfigDepot; @SuppressWarnings("unchecked") @@ -158,7 +158,7 @@ public void cleanupEnvironmentHacks() throws Exception { smsField.set(null, null); restoreConfigDepot(); } - + private void setupConfigDepotMock() throws NoSuchFieldException, IllegalAccessException { Field depotField = ConfigKey.class.getDeclaredField("s_depot"); depotField.setAccessible(true); @@ -170,17 +170,17 @@ private void setupConfigDepotMock() throws NoSuchFieldException, IllegalAccessEx 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.eq(configName), + Mockito.eq(ConfigKey.Scope.Global), Mockito.isNull() )).thenReturn(value); }