Skip to content

Fix logging of forwarded IPs in logs #11854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abh1sar wants to merge 5 commits into
base: 4.22
Choose a base branch
from
+69 −62
Open

Fix logging of forwarded IPs in logs #11854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f331f0f
log the forwarded ip address in access log if the setting proxy.heade…
abh1sar Oct 16, 2025
8879aa8
use static functions from ApiServlet and code refactoring
abh1sar Oct 17, 2025
e9b7ba4
Fix ApiServletTest.java
abh1sar Oct 21, 2025
5a59165
fix precommit
abh1sar Oct 21, 2025
04eb391
fix precommit
abh1sar Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions client/src/main/java/org/apache/cloudstack/ACSRequestLog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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)
Copy link
Member

@vishesh92 vishesh92 Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sb.append(remoteAddress)
sb.append(remoteAddress.getHostAddress())

Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log format has changed by appending the InetAddress object directly instead of calling getHostAddress(). This results in IP addresses being logged with a leading "/" (e.g., "/192.168.1.1" instead of "192.168.1.1") because InetAddress.toString() returns the format "hostname/ipaddress" or "/ipaddress". This format change could break existing log parsing tools. Consider calling getHostAddress() on the InetAddress object to maintain the previous log format.

Suggested change
sb.append(remoteAddress)
sb.append(remoteAddress != null ? remoteAddress.getHostAddress() : "-")

Copilot uses AI. Check for mistakes.
.append(" - - [")
.append(dateCache.format(request.getTimeStamp()))
.append("] \"")
Expand Down
19 changes: 0 additions & 19 deletions client/src/main/java/org/apache/cloudstack/ServerDaemon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -193,7 +190,6 @@ public void start() throws Exception {
httpConfig.setResponseHeaderSize(8192);
httpConfig.setSendServerVersion(false);
httpConfig.setSendDateHeader(false);
addForwardingCustomiser(httpConfig);
Copy link
Member

@vishesh92 vishesh92 Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to remove this?


// HTTP Connector
createHttpConnector(httpConfig);
Expand All @@ -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();
Expand Down
34 changes: 15 additions & 19 deletions server/src/main/java/com/cloud/api/ApiServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(",");
}

Expand Down
71 changes: 50 additions & 21 deletions server/src/test/java/com/cloud/api/ApiServletTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand All @@ -131,6 +135,7 @@ public void setup() throws SecurityException, NoSuchFieldException,
apiServerField.setAccessible(true);
apiServerField.set(servlet, apiServer);

setupConfigDepotMock();
}

/**
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading