Skip to content

Commit 6d8999c

Browse files
abh1sardhslove
abh1sar
authored and
dhslove
committed
Fix logging of forwarded IPs in logs (apache#11854)
1 parent 276160a commit 6d8999c

4 files changed

Lines changed: 94 additions & 62 deletions

File tree

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,15 @@
1818
//
1919
package org.apache.cloudstack;
2020

21+
import com.cloud.api.ApiServlet;
2122
import com.cloud.utils.StringUtils;
2223
import org.eclipse.jetty.server.NCSARequestLog;
2324
import org.eclipse.jetty.server.Request;
2425
import org.eclipse.jetty.server.Response;
2526
import org.eclipse.jetty.util.DateCache;
2627
import org.eclipse.jetty.util.component.LifeCycle;
2728

29+
import java.net.InetAddress;
2830
import java.util.Locale;
2931
import java.util.TimeZone;
3032

@@ -51,9 +53,8 @@ public void log(Request request, Response response) {
5153
StringBuilder sb = buffers.get();
5254
sb.setLength(0);
5355

54-
sb.append(request.getHttpChannel().getEndPoint()
55-
.getRemoteAddress().getAddress()
56-
.getHostAddress())
56+
InetAddress remoteAddress = ApiServlet.getClientAddress(request);
57+
sb.append(remoteAddress.getHostAddress())
5758
.append(" - - [")
5859
.append(dateCache.format(request.getTimeStamp()))
5960
.append("] \"")

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

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,12 @@
2424
import java.io.InputStream;
2525
import java.lang.management.ManagementFactory;
2626
import java.net.URL;
27-
import java.util.Arrays;
2827
import java.util.Properties;
2928

30-
import com.cloud.api.ApiServer;
3129
import org.apache.commons.daemon.Daemon;
3230
import org.apache.commons.daemon.DaemonContext;
3331
import org.apache.commons.lang3.StringUtils;
3432
import org.eclipse.jetty.jmx.MBeanContainer;
35-
import org.eclipse.jetty.server.ForwardedRequestCustomizer;
3633
import org.eclipse.jetty.server.HttpConfiguration;
3734
import org.eclipse.jetty.server.HttpConnectionFactory;
3835
import org.eclipse.jetty.server.RequestLog;
@@ -193,7 +190,6 @@ public void start() throws Exception {
193190
httpConfig.setResponseHeaderSize(8192);
194191
httpConfig.setSendServerVersion(false);
195192
httpConfig.setSendDateHeader(false);
196-
addForwardingCustomiser(httpConfig);
197193

198194
// HTTP Connector
199195
createHttpConnector(httpConfig);
@@ -216,21 +212,6 @@ public void start() throws Exception {
216212
server.join();
217213
}
218214

219-
/**
220-
* Adds a ForwardedRequestCustomizer to the HTTP configuration to handle forwarded headers.
221-
* The header used for forwarding is determined by the ApiServer.listOfForwardHeaders property.
222-
* Only non empty headers are considered and only the first of the comma-separated list is used.
223-
* @param httpConfig the HTTP configuration to which the customizer will be added
224-
*/
225-
private static void addForwardingCustomiser(HttpConfiguration httpConfig) {
226-
ForwardedRequestCustomizer customiser = new ForwardedRequestCustomizer();
227-
String header = Arrays.stream(ApiServer.listOfForwardHeaders.value().split(",")).findFirst().orElse(null);
228-
if (com.cloud.utils.StringUtils.isNotEmpty(header)) {
229-
customiser.setForwardedForHeader(header);
230-
}
231-
httpConfig.addCustomizer(customiser);
232-
}
233-
234215
@Override
235216
public void stop() throws Exception {
236217
server.stop();

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

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -698,38 +698,34 @@ private boolean isSpecificAPI(String commandName) {
698698
}
699699
return false;
700700
}
701-
boolean doUseForwardHeaders() {
701+
static boolean doUseForwardHeaders() {
702702
return Boolean.TRUE.equals(ApiServer.useForwardHeader.value());
703703
}
704704

705-
String[] proxyNets() {
705+
static String[] proxyNets() {
706706
return ApiServer.proxyForwardList.value().split(",");
707707
}
708708
//This method will try to get login IP of user even if servlet is behind reverseProxy or loadBalancer
709-
public InetAddress getClientAddress(final HttpServletRequest request) throws UnknownHostException {
710-
String ip = null;
711-
InetAddress pretender = InetAddress.getByName(request.getRemoteAddr());
712-
if(doUseForwardHeaders()) {
713-
if (NetUtils.isIpInCidrList(pretender, proxyNets())) {
709+
public static InetAddress getClientAddress(final HttpServletRequest request) throws UnknownHostException {
710+
final String remote = request.getRemoteAddr();
711+
if (doUseForwardHeaders()) {
712+
final InetAddress remoteAddr = InetAddress.getByName(remote);
713+
if (NetUtils.isIpInCidrList(remoteAddr, proxyNets())) {
714714
for (String header : getClientAddressHeaders()) {
715715
header = header.trim();
716-
ip = getCorrectIPAddress(request.getHeader(header));
716+
final String ip = getCorrectIPAddress(request.getHeader(header));
717717
if (StringUtils.isNotBlank(ip)) {
718-
LOGGER.debug(String.format("found ip %s in header %s ", ip, header));
719-
break;
718+
LOGGER.debug("found ip {} in header {}", ip, header);
719+
return InetAddress.getByName(ip);
720720
}
721-
} // no address found in header so ip is blank and use remote addr
722-
} // else not an allowed proxy address, ip is blank and use remote addr
723-
}
724-
if (StringUtils.isBlank(ip)) {
725-
LOGGER.trace(String.format("no ip found in headers, returning remote address %s.", pretender.getHostAddress()));
726-
return pretender;
721+
}
722+
}
727723
}
728-
729-
return InetAddress.getByName(ip);
724+
LOGGER.trace("no ip found in headers, returning remote address {}.", remote);
725+
return InetAddress.getByName(remote);
730726
}
731727

732-
private String[] getClientAddressHeaders() {
728+
private static String[] getClientAddressHeaders() {
733729
return ApiServer.listOfForwardHeaders.value().split(",");
734730
}
735731

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

Lines changed: 75 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,32 @@
1717
package com.cloud.api;
1818

1919
import static org.mockito.ArgumentMatchers.nullable;
20+
import com.cloud.api.auth.ListUserTwoFactorAuthenticatorProvidersCmd;
21+
import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd;
22+
import com.cloud.api.auth.ValidateUserTwoFactorAuthenticationCodeCmd;
23+
import com.cloud.server.ManagementServer;
24+
import com.cloud.user.Account;
25+
import com.cloud.user.AccountManagerImpl;
26+
import com.cloud.user.AccountService;
27+
import com.cloud.user.User;
28+
import com.cloud.user.UserAccount;
29+
import com.cloud.utils.HttpUtils;
30+
import com.cloud.vm.UserVmManager;
31+
import org.apache.cloudstack.api.ApiConstants;
32+
import org.apache.cloudstack.api.auth.APIAuthenticationManager;
33+
import org.apache.cloudstack.api.auth.APIAuthenticationType;
34+
import org.apache.cloudstack.api.auth.APIAuthenticator;
35+
import org.apache.cloudstack.api.command.admin.config.ListCfgsByCmd;
36+
import org.apache.cloudstack.framework.config.ConfigKey;
37+
import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
38+
import org.junit.After;
39+
import org.junit.Assert;
40+
import org.junit.Before;
41+
import org.junit.Test;
42+
import org.junit.runner.RunWith;
43+
import org.mockito.Mock;
44+
import org.mockito.Mockito;
45+
import org.mockito.junit.MockitoJUnitRunner;
2046

2147
import java.io.IOException;
2248
import java.io.PrintWriter;
@@ -105,17 +131,20 @@ public class ApiServletTest {
105131
@Mock
106132
AccountService accountMgr;
107133

108-
@Mock ConfigKey<Boolean> useForwardHeader;
134+
@Mock
135+
ConfigDepotImpl mockConfigDepot;
136+
109137
StringWriter responseWriter;
110138

111139
ApiServlet servlet;
112-
ApiServlet spyServlet;
140+
141+
private ConfigDepotImpl originalConfigDepot;
142+
113143
@SuppressWarnings("unchecked")
114144
@Before
115145
public void setup() throws SecurityException, NoSuchFieldException,
116-
IllegalArgumentException, IllegalAccessException, IOException, UnknownHostException {
146+
IllegalArgumentException, IllegalAccessException, IOException {
117147
servlet = new ApiServlet();
118-
spyServlet = Mockito.spy(servlet);
119148
responseWriter = new StringWriter();
120149
Mockito.when(response.getWriter()).thenReturn(
121150
new PrintWriter(responseWriter));
@@ -139,6 +168,7 @@ public void setup() throws SecurityException, NoSuchFieldException,
139168
apiServerField.setAccessible(true);
140169
apiServerField.set(servlet, apiServer);
141170

171+
setupConfigDepotMock();
142172
}
143173

144174
/**
@@ -159,6 +189,33 @@ public void cleanupEnvironmentHacks() throws Exception {
159189
Field smsField = ApiDBUtils.class.getDeclaredField("s_ms");
160190
smsField.setAccessible(true);
161191
smsField.set(null, null);
192+
restoreConfigDepot();
193+
}
194+
195+
private void setupConfigDepotMock() throws NoSuchFieldException, IllegalAccessException {
196+
Field depotField = ConfigKey.class.getDeclaredField("s_depot");
197+
depotField.setAccessible(true);
198+
originalConfigDepot = (ConfigDepotImpl) depotField.get(null);
199+
depotField.set(null, mockConfigDepot);
200+
Mockito.when(mockConfigDepot.getConfigStringValue(
201+
Mockito.anyString(),
202+
Mockito.any(ConfigKey.Scope.class),
203+
Mockito.any()
204+
)).thenReturn(null);
205+
}
206+
207+
private void restoreConfigDepot() throws Exception {
208+
Field depotField = ConfigKey.class.getDeclaredField("s_depot");
209+
depotField.setAccessible(true);
210+
depotField.set(null, originalConfigDepot);
211+
}
212+
213+
private void setConfigValue(String configName, String value) {
214+
Mockito.when(mockConfigDepot.getConfigStringValue(
215+
Mockito.eq(configName),
216+
Mockito.eq(ConfigKey.Scope.Global),
217+
Mockito.isNull()
218+
)).thenReturn(value);
162219
}
163220

164221
@Test
@@ -269,43 +326,40 @@ public void processRequestInContextLogin() throws UnknownHostException {
269326

270327
@Test
271328
public void getClientAddressWithXForwardedFor() throws UnknownHostException {
272-
String[] proxynet = {"127.0.0.0/8"};
273-
Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
274-
Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
329+
setConfigValue("proxy.header.verify", "true");
330+
setConfigValue("proxy.cidr", "127.0.0.0/8");
331+
Mockito.when(request.getRemoteAddr()).thenReturn("127.0.0.1");
275332
Mockito.when(request.getHeader(Mockito.eq("X-Forwarded-For"))).thenReturn("192.168.1.1");
276-
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
333+
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
277334
}
278335

279336
@Test
280337
public void getClientAddressWithHttpXForwardedFor() throws UnknownHostException {
281-
String[] proxynet = {"127.0.0.0/8"};
282-
Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
283-
Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
338+
setConfigValue("proxy.header.verify", "true");
339+
setConfigValue("proxy.cidr", "127.0.0.0/8");
284340
Mockito.when(request.getHeader(Mockito.eq("HTTP_X_FORWARDED_FOR"))).thenReturn("192.168.1.1");
285-
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
341+
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
286342
}
287343

288344
@Test
289345
public void getClientAddressWithRemoteAddr() throws UnknownHostException {
290-
String[] proxynet = {"127.0.0.0/8"};
291-
Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
292-
Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
293-
Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
346+
setConfigValue("proxy.header.verify", "true");
347+
setConfigValue("proxy.cidr", "127.0.0.0/8");
348+
Assert.assertEquals(InetAddress.getByName("127.0.0.1"), ApiServlet.getClientAddress(request));
294349
}
295350

296351
@Test
297352
public void getClientAddressWithHttpClientIp() throws UnknownHostException {
298-
String[] proxynet = {"127.0.0.0/8"};
299-
Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
300-
Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
353+
setConfigValue("proxy.header.verify", "true");
354+
setConfigValue("proxy.cidr", "127.0.0.0/8");
301355
Mockito.when(request.getHeader(Mockito.eq("HTTP_CLIENT_IP"))).thenReturn("192.168.1.1");
302-
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
356+
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
303357
}
304358

305359
@Test
306360
public void getClientAddressDefault() throws UnknownHostException {
307361
Mockito.when(request.getRemoteAddr()).thenReturn("127.0.0.1");
308-
Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
362+
Assert.assertEquals(InetAddress.getByName("127.0.0.1"), ApiServlet.getClientAddress(request));
309363
}
310364

311365
@Test

0 commit comments

Comments
 (0)