Handle port and IPv6 in forwarded headers (#3651)

* Handle port and IPv6 in forwarded headers * More IPv6... * Test proper quoted host:port for Forwarded case
open-telemetry · Jul 23, 2021 · 83b5121 · 83b5121
1 parent d7dcc70
commit 83b5121
Show file tree

Hide file tree

Showing 2 changed files with 217 additions and 25 deletions.
diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java
@@ -206,7 +206,7 @@ private String clientIp(CONNECTION connection, REQUEST request) {
     // try Forwarded
     String forwarded = requestHeader(request, "Forwarded");
     if (forwarded != null) {
-      forwarded = extractForwardedFor(forwarded);
+      forwarded = extractForwarded(forwarded);
       if (forwarded != null) {
         return forwarded;
       }
@@ -215,12 +215,8 @@ private String clientIp(CONNECTION connection, REQUEST request) {
     // try X-Forwarded-For
     forwarded = requestHeader(request, "X-Forwarded-For");
     if (forwarded != null) {
-      // may be split by ,
-      int endIndex = forwarded.indexOf(',');
-      if (endIndex > 0) {
-        forwarded = forwarded.substring(0, endIndex);
-      }
-      if (!forwarded.isEmpty()) {
+      forwarded = extractForwardedFor(forwarded);
+      if (forwarded != null) {
         return forwarded;
       }
     }
@@ -230,7 +226,7 @@ private String clientIp(CONNECTION connection, REQUEST request) {
   }
 
   // VisibleForTesting
-  static String extractForwardedFor(String forwarded) {
+  static String extractForwarded(String forwarded) {
     int start = forwarded.toLowerCase().indexOf("for=");
     if (start < 0) {
       return null;
@@ -239,9 +235,42 @@ static String extractForwardedFor(String forwarded) {
     if (start >= forwarded.length() - 1) { // the value after for= must not be empty
       return null;
     }
+    return extractIpAddress(forwarded, start);
+  }
+
+  // VisibleForTesting
+  static String extractForwardedFor(String forwarded) {
+    return extractIpAddress(forwarded, 0);
+  }
+
+  // from https://www.rfc-editor.org/rfc/rfc7239
+  //  "Note that IPv6 addresses may not be quoted in
+  //   X-Forwarded-For and may not be enclosed by square brackets, but they
+  //   are quoted and enclosed in square brackets in Forwarded"
+  // and also (applying to Forwarded but not X-Forwarded-For)
+  //  "It is important to note that an IPv6 address and any nodename with
+  //   node-port specified MUST be quoted, since ':' is not an allowed
+  //   character in 'token'."
+  private static String extractIpAddress(String forwarded, int start) {
+    if (forwarded.length() == start) {
+      return null;
+    }
+    if (forwarded.charAt(start) == '"') {
+      return extractIpAddress(forwarded, start + 1);
+    }
+    if (forwarded.charAt(start) == '[') {
+      int end = forwarded.indexOf(']', start + 1);
+      if (end == -1) {
+        return null;
+      }
+      return forwarded.substring(start + 1, end);
+    }
+    boolean inIpv4 = false;
     for (int i = start; i < forwarded.length() - 1; i++) {
       char c = forwarded.charAt(i);
-      if (c == ',' || c == ';') {
+      if (c == '.') {
+        inIpv4 = true;
+      } else if (c == ',' || c == ';' || c == '"' || (inIpv4 && c == ':')) {
         if (i == start) { // empty string
           return null;
         }

diff --git a/...n-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java b/...n-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java
@@ -11,19 +11,161 @@
 import org.junit.Test;
 
 public class HttpServerTracerTest {
+  @Test
+  public void extractForwarded() {
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=1.1.1.1"));
+  }
+
+  @Test
+  public void extractForwardedIpv6() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwarded("for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\""));
+  }
+
+  @Test
+  public void extractForwardedWithPort() {
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=\"1.1.1.1:2222\""));
+  }
+
+  @Test
+  public void extractForwardedIpv6WithPort() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwarded(
+            "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\""));
+  }
+
+  @Test
+  public void extractForwardedCaps() {
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("For=1.1.1.1"));
+  }
+
+  @Test
+  public void extractForwardedMalformed() {
+    assertNull(HttpServerTracer.extractForwarded("for=;for=1.1.1.1"));
+  }
+
+  @Test
+  public void extractForwardedEmpty() {
+    assertNull(HttpServerTracer.extractForwarded(""));
+  }
+
+  @Test
+  public void extractForwardedEmptyValue() {
+    assertNull(HttpServerTracer.extractForwarded("for="));
+  }
+
+  @Test
+  public void extractForwardedEmptyValueWithSemicolon() {
+    assertNull(HttpServerTracer.extractForwarded("for=;"));
+  }
+
+  @Test
+  public void extractForwardedNoFor() {
+    assertNull(HttpServerTracer.extractForwarded("by=1.1.1.1;test=1.1.1.1"));
+  }
+
+  @Test
+  public void extractForwardedMultiple() {
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=1.1.1.1;for=1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedMultipleIpV6() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwarded(
+            "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\";for=1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedMultipleWithPort() {
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=\"1.1.1.1:2222\";for=1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedMultipleIpV6WithPort() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwarded(
+            "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\";for=1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedMixedSplitter() {
+    assertEquals(
+        "1.1.1.1",
+        HttpServerTracer.extractForwarded("test=abcd; by=1.2.3.4, for=1.1.1.1;for=1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedMixedSplitterIpv6() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwarded(
+            "test=abcd; by=1.2.3.4, for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\";for=1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedMixedSplitterWithPort() {
+    assertEquals(
+        "1.1.1.1",
+        HttpServerTracer.extractForwarded(
+            "test=abcd; by=1.2.3.4, for=\"1.1.1.1:2222\";for=1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedMixedSplitterIpv6WithPort() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwarded(
+            "test=abcd; by=1.2.3.4, for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\";for=1.2.3.4"));
+  }
+
   @Test
   public void extractForwardedFor() {
-    assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("for=1.1.1.1"));
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("1.1.1.1"));
+  }
+
+  @Test
+  public void extractForwardedForIpv6() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor("\"[1111:1111:1111:1111:1111:1111:1111:1111]\""));
   }
 
   @Test
-  public void extractForwardedForCaps() {
-    assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("For=1.1.1.1"));
+  public void extractForwardedForIpv6Unquoted() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor("[1111:1111:1111:1111:1111:1111:1111:1111]"));
   }
 
   @Test
-  public void extractForwardedForMalformed() {
-    assertNull(HttpServerTracer.extractForwardedFor("for=;for=1.1.1.1"));
+  public void extractForwardedForIpv6Unbracketed() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor("1111:1111:1111:1111:1111:1111:1111:1111"));
+  }
+
+  @Test
+  public void extractForwardedForWithPort() {
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("1.1.1.1:2222"));
+  }
+
+  @Test
+  public void extractForwardedForIpv6WithPort() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor("\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\""));
+  }
+
+  @Test
+  public void extractForwardedForIpv6UnquotedWithPort() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor("[1111:1111:1111:1111:1111:1111:1111:1111]:2222"));
   }
 
   @Test
@@ -32,29 +174,50 @@ public void extractForwardedForEmpty() {
   }
 
   @Test
-  public void extractForwardedForEmptyValue() {
-    assertNull(HttpServerTracer.extractForwardedFor("for="));
+  public void extractForwardedForMultiple() {
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("1.1.1.1,1.2.3.4"));
   }
 
   @Test
-  public void extractForwardedForEmptyValueWithSemicolon() {
-    assertNull(HttpServerTracer.extractForwardedFor("for=;"));
+  public void extractForwardedForMultipleIpv6() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor(
+            "\"[1111:1111:1111:1111:1111:1111:1111:1111]\",1.2.3.4"));
   }
 
   @Test
-  public void extractForwardedForNoFor() {
-    assertNull(HttpServerTracer.extractForwardedFor("by=1.1.1.1;test=1.1.1.1"));
+  public void extractForwardedForMultipleIpv6Unquoted() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor("[1111:1111:1111:1111:1111:1111:1111:1111],1.2.3.4"));
   }
 
   @Test
-  public void extractForwardedForMultiple() {
-    assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("for=1.1.1.1;for=1.2.3.4"));
+  public void extractForwardedForMultipleIpv6Unbracketed() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor("1111:1111:1111:1111:1111:1111:1111:1111,1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedForMultipleWithPort() {
+    assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("1.1.1.1:2222,1.2.3.4"));
   }
 
   @Test
-  public void extractForwardedForMixedSplitter() {
+  public void extractForwardedForMultipleIpv6WithPort() {
     assertEquals(
-        "1.1.1.1",
-        HttpServerTracer.extractForwardedFor("test=abcd; by=1.2.3.4, for=1.1.1.1;for=1.2.3.4"));
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor(
+            "\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\",1.2.3.4"));
+  }
+
+  @Test
+  public void extractForwardedForMultipleIpv6UnquotedWithPort() {
+    assertEquals(
+        "1111:1111:1111:1111:1111:1111:1111:1111",
+        HttpServerTracer.extractForwardedFor(
+            "[1111:1111:1111:1111:1111:1111:1111:1111]:2222,1.2.3.4"));
   }
 }