Don't use request headers for remote address/port in hosted Vespa

Control which headers are used for remote address/port in access log through config model.

5 files changed, 93 insertions, 34 deletions

diff --git a/jdisc_http_service/abi-spec.json b/jdisc_http_service/abi-spec.json
index f6bfe769997..3f68009cd42 100644
--- a/jdisc_http_service/abi-spec.json
+++ b/jdisc_http_service/abi-spec.json
@@ -682,6 +682,44 @@
     ],
     "fields": []
   },
+  "com.yahoo.jdisc.http.ServerConfig$AccessLog$Builder": {
+    "superClass": "java.lang.Object",
+    "interfaces": [
+      "com.yahoo.config.ConfigBuilder"
+    ],
+    "attributes": [
+      "public"
+    ],
+    "methods": [
+      "public void <init>()",
+      "public void <init>(com.yahoo.jdisc.http.ServerConfig$AccessLog)",
+      "public com.yahoo.jdisc.http.ServerConfig$AccessLog$Builder remoteAddressHeaders(java.lang.String)",
+      "public com.yahoo.jdisc.http.ServerConfig$AccessLog$Builder remoteAddressHeaders(java.util.Collection)",
+      "public com.yahoo.jdisc.http.ServerConfig$AccessLog$Builder remotePortHeaders(java.lang.String)",
+      "public com.yahoo.jdisc.http.ServerConfig$AccessLog$Builder remotePortHeaders(java.util.Collection)",
+      "public com.yahoo.jdisc.http.ServerConfig$AccessLog build()"
+    ],
+    "fields": [
+      "public java.util.List remoteAddressHeaders",
+      "public java.util.List remotePortHeaders"
+    ]
+  },
+  "com.yahoo.jdisc.http.ServerConfig$AccessLog": {
+    "superClass": "com.yahoo.config.InnerNode",
+    "interfaces": [],
+    "attributes": [
+      "public",
+      "final"
+    ],
+    "methods": [
+      "public void <init>(com.yahoo.jdisc.http.ServerConfig$AccessLog$Builder)",
+      "public java.util.List remoteAddressHeaders()",
+      "public java.lang.String remoteAddressHeaders(int)",
+      "public java.util.List remotePortHeaders()",
+      "public java.lang.String remotePortHeaders(int)"
+    ],
+    "fields": []
+  },
   "com.yahoo.jdisc.http.ServerConfig$Builder": {
     "superClass": "java.lang.Object",
     "interfaces": [
@@ -704,6 +742,7 @@
       "public com.yahoo.jdisc.http.ServerConfig$Builder stopTimeout(double)",
       "public com.yahoo.jdisc.http.ServerConfig$Builder jmx(com.yahoo.jdisc.http.ServerConfig$Jmx$Builder)",
       "public com.yahoo.jdisc.http.ServerConfig$Builder metric(com.yahoo.jdisc.http.ServerConfig$Metric$Builder)",
+      "public com.yahoo.jdisc.http.ServerConfig$Builder accessLog(com.yahoo.jdisc.http.ServerConfig$AccessLog$Builder)",
       "public final boolean dispatchGetConfig(com.yahoo.config.ConfigInstance$Producer)",
       "public final java.lang.String getDefMd5()",
       "public final java.lang.String getDefName()",
@@ -713,7 +752,8 @@
     "fields": [
       "public java.util.List filter",
       "public com.yahoo.jdisc.http.ServerConfig$Jmx$Builder jmx",
-      "public com.yahoo.jdisc.http.ServerConfig$Metric$Builder metric"
+      "public com.yahoo.jdisc.http.ServerConfig$Metric$Builder metric",
+      "public com.yahoo.jdisc.http.ServerConfig$AccessLog$Builder accessLog"
     ]
   },
   "com.yahoo.jdisc.http.ServerConfig$Filter$Builder": {
@@ -854,7 +894,8 @@
       "public int maxWorkerThreads()",
       "public double stopTimeout()",
       "public com.yahoo.jdisc.http.ServerConfig$Jmx jmx()",
-      "public com.yahoo.jdisc.http.ServerConfig$Metric metric()"
+      "public com.yahoo.jdisc.http.ServerConfig$Metric metric()",
+      "public com.yahoo.jdisc.http.ServerConfig$AccessLog accessLog()"
     ],
     "fields": [
       "public static final java.lang.String CONFIG_DEF_MD5",
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java
index 7085f07585a..e8fd92f8e19 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java
@@ -4,6 +4,7 @@ package com.yahoo.jdisc.http.server.jetty;
 import com.google.common.base.Objects;
 import com.yahoo.container.logging.AccessLog;
 import com.yahoo.container.logging.AccessLogEntry;
+import com.yahoo.jdisc.http.ServerConfig;
 import com.yahoo.jdisc.http.servlet.ServletRequest;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.RequestLog;
@@ -15,6 +16,7 @@ import java.security.Principal;
 import java.security.cert.X509Certificate;
 import java.util.List;
 import java.util.Optional;
+import java.util.OptionalInt;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -27,25 +29,21 @@ import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnectorLoca
  * @author Oyvind Bakksjo
  * @author bjorncs
  */
-public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog {
+class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog {
 
     private static final Logger logger = Logger.getLogger(AccessLogRequestLog.class.getName());
 
-    // TODO These hardcoded headers should be provided by config instead
-    private static final String HEADER_NAME_X_FORWARDED_FOR = "x-forwarded-for";
-    private static final String HEADER_NAME_X_FORWARDED_PORT = "X-Forwarded-Port";
-    private static final String HEADER_NAME_Y_RA = "y-ra";
-    private static final String HEADER_NAME_Y_RP = "y-rp";
-    private static final String HEADER_NAME_YAHOOREMOTEIP = "yahooremoteip";
-    private static final String HEADER_NAME_CLIENT_IP = "client-ip";
-
     // HTTP headers that are logged as extra key-value-pairs in access log entries
     private static final List<String> LOGGED_REQUEST_HEADERS = List.of("Vespa-Client-Version");
 
     private final AccessLog accessLog;
+    private final List<String> remoteAddressHeaders;
+    private final List<String> remotePortHeaders;
 
-    public AccessLogRequestLog(AccessLog accessLog) {
+    AccessLogRequestLog(AccessLog accessLog, ServerConfig.AccessLog config) {
         this.accessLog = accessLog;
+        this.remoteAddressHeaders = config.remoteAddressHeaders();
+        this.remotePortHeaders = config.remotePortHeaders();
     }
 
     @Override
@@ -121,26 +119,30 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog
         }
     }
 
-    private static String getRemoteAddress(HttpServletRequest request) {
-        return Optional.ofNullable(request.getHeader(HEADER_NAME_X_FORWARDED_FOR))
-                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_Y_RA)))
-                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_YAHOOREMOTEIP)))
-                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_CLIENT_IP)))
-                .orElseGet(request::getRemoteAddr);
+    private String getRemoteAddress(HttpServletRequest request) {
+        for (String header : remoteAddressHeaders) {
+            String value = request.getHeader(header);
+            if (value != null) return value;
+        }
+        return request.getRemoteAddr();
     }
 
-    private static int getRemotePort(HttpServletRequest request) {
-        return Optional.ofNullable(request.getHeader(HEADER_NAME_X_FORWARDED_PORT))
-                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_Y_RP)))
-                .flatMap(AccessLogRequestLog::parsePort)
-                .orElseGet(request::getRemotePort);
+    private int getRemotePort(HttpServletRequest request) {
+        for (String header : remotePortHeaders) {
+            String value = request.getHeader(header);
+            if (value != null) {
+                OptionalInt maybePort = parsePort(value);
+                if (maybePort.isPresent()) return maybePort.getAsInt();
+            }
+        }
+        return request.getRemotePort();
     }
 
-    private static Optional<Integer> parsePort(String port) {
+    private static OptionalInt parsePort(String port) {
         try {
-            return Optional.of(Integer.valueOf(port));
+            return OptionalInt.of(Integer.parseInt(port));
         } catch (IllegalArgumentException e) {
-            return Optional.empty();
+            return OptionalInt.empty();
         }
     }
 
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
index b050a9a6d1c..d881fae8e26 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
@@ -147,7 +147,7 @@ public class JettyHttpServer extends AbstractServerProvider {
 
         server = new Server();
         server.setStopTimeout((long)(serverConfig.stopTimeout() * 1000.0));
-        server.setRequestLog(new AccessLogRequestLog(accessLog));
+        server.setRequestLog(new AccessLogRequestLog(accessLog, serverConfig.accessLog()));
         setupJmx(server, serverConfig);
         ((QueuedThreadPool)server.getThreadPool()).setMaxThreads(serverConfig.maxWorkerThreads());
 
diff --git a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def
index 0cb5b89b20c..3118a7dea64 100644
--- a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def
+++ b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def
@@ -43,4 +43,10 @@ jmx.listenPort                        int    default = 1099
 metric.monitoringHandlerPaths[]       string
 
 # Paths that should be reported with search dimensions where applicable
-metric.searchHandlerPaths[]           string
\ No newline at end of file
+metric.searchHandlerPaths[]           string
+
+# HTTP request headers that contain remote address
+accessLog.remoteAddressHeaders[]      string
+
+# HTTP request headers that contain remote port
+accessLog.remotePortHeaders[]         string
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
index 69535be034c..a4fd7c9bc5f 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
@@ -3,6 +3,7 @@ package com.yahoo.jdisc.http.server.jetty;
 
 import com.yahoo.container.logging.AccessLog;
 import com.yahoo.container.logging.AccessLogEntry;
+import com.yahoo.jdisc.http.ServerConfig;
 import org.eclipse.jetty.http.MetaData;
 import org.eclipse.jetty.server.HttpChannel;
 import org.eclipse.jetty.server.HttpConnection;
@@ -11,6 +12,7 @@ import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.server.ServerConnector;
 import org.junit.Test;
 
+import java.util.List;
 import java.util.Optional;
 
 import static org.hamcrest.CoreMatchers.is;
@@ -33,7 +35,7 @@ public class AccessLogRequestLogTest {
         when(jettyRequest.getRequestURI()).thenReturn("/search/");
         when(jettyRequest.getQueryString()).thenReturn("query=year:>2010");
 
-        new AccessLogRequestLog(mock(AccessLog.class)).log(jettyRequest, createResponseMock());
+        doAccessLoggingOfRequest(jettyRequest);
 
         assertThat(accessLogEntry.getRawPath(), is(not(nullValue())));
         assertTrue(accessLogEntry.getRawQuery().isPresent());
@@ -48,7 +50,7 @@ public class AccessLogRequestLogTest {
         final String query = "query=year%252010+%3B&customParameter=something";
         when(jettyRequest.getQueryString()).thenReturn(query);
 
-        new AccessLogRequestLog(mock(AccessLog.class)).log(jettyRequest, createResponseMock());
+        doAccessLoggingOfRequest(jettyRequest);
 
         assertThat(accessLogEntry.getRawPath(), is(path));
         assertThat(accessLogEntry.getRawQuery().get(), is(query));
@@ -64,7 +66,7 @@ public class AccessLogRequestLogTest {
         String rawQuery = "q=%%2";
         when(jettyRequest.getQueryString()).thenReturn(rawQuery);
 
-        new AccessLogRequestLog(mock(AccessLog.class)).log(jettyRequest, createResponseMock());
+        doAccessLoggingOfRequest(jettyRequest);
         assertThat(accessLogEntry.getRawPath(), is(rawPath));
         Optional<String> actualRawQuery = accessLogEntry.getRawQuery();
         assertThat(actualRawQuery.isPresent(), is(true));
@@ -80,7 +82,7 @@ public class AccessLogRequestLogTest {
         when(jettyRequest.getHeader("x-forwarded-for")).thenReturn("1.2.3.4");
         when(jettyRequest.getHeader("y-ra")).thenReturn("2.3.4.5");
 
-        new AccessLogRequestLog(mock(AccessLog.class)).log(jettyRequest, createResponseMock());
+        doAccessLoggingOfRequest(jettyRequest);
         assertThat(accessLogEntry.getRemoteAddress(), is("1.2.3.4"));
     }
 
@@ -93,7 +95,7 @@ public class AccessLogRequestLogTest {
         when(jettyRequest.getHeader("X-Forwarded-Port")).thenReturn("80");
         when(jettyRequest.getHeader("y-rp")).thenReturn("8080");
 
-        new AccessLogRequestLog(mock(AccessLog.class)).log(jettyRequest, createResponseMock());
+        doAccessLoggingOfRequest(jettyRequest);
         assertThat(accessLogEntry.getRemotePort(), is(80));
     }
 
@@ -105,11 +107,19 @@ public class AccessLogRequestLogTest {
         when(jettyRequest.getHeader("X-Forwarded-Port")).thenReturn("8o8o");
         when(jettyRequest.getRemotePort()).thenReturn(80);
 
-        new AccessLogRequestLog(mock(AccessLog.class)).log(jettyRequest, createResponseMock());
+        doAccessLoggingOfRequest(jettyRequest);
         assertThat(accessLogEntry.getRemotePort(), is(0));
         assertThat(accessLogEntry.getPeerPort(), is(80));
     }
 
+    private void doAccessLoggingOfRequest(Request jettyRequest) {
+        ServerConfig.AccessLog config = new ServerConfig.AccessLog(
+                new ServerConfig.AccessLog.Builder()
+                        .remoteAddressHeaders(List.of("x-forwarded-for", "y-ra"))
+                        .remotePortHeaders(List.of("X-Forwarded-Port", "y-rp")));
+        new AccessLogRequestLog(mock(AccessLog.class), config).log(jettyRequest, createResponseMock());
+    }
+
     private static Request createRequestMock(AccessLogEntry entry) {
         ServerConnector serverConnector = mock(ServerConnector.class);
         when(serverConnector.getLocalPort()).thenReturn(1234);