From 305ac528bf609d89384c4929d99d41e774601a92 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 21 Jul 2021 20:51:22 -0700 Subject: [PATCH 1/3] Handle port and IPv6 in forwarded headers --- .../api/tracer/HttpServerTracer.java | 36 +++-- .../api/tracer/HttpServerTracerTest.java | 153 +++++++++++++++--- 2 files changed, 162 insertions(+), 27 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java index fa12937695ca..8e5e6de61c9c 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java +++ b/instrumentation-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,31 @@ 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); + } + + private static String extractIpAddress(String forwarded, int start) { + if (forwarded.length() == start) { + return null; + } + if (forwarded.charAt(start) == '[') { + // from https://www.rfc-editor.org/rfc/rfc7239 + // "note that an IPv6 address is always enclosed in square brackets" + int end = forwarded.indexOf(']', start); + if (end <= start) { + return null; + } + return forwarded.substring(start + 1, end); + } for (int i = start; i < forwarded.length() - 1; i++) { char c = forwarded.charAt(i); - if (c == ',' || c == ';') { + // ok to check ':' (for start of port) because ipv6 case is checked above + if (c == ',' || c == ';' || c == ':') { if (i == start) { // empty string return null; } diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java index fb27162afc53..62c04bbf2357 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java @@ -12,49 +12,166 @@ public class HttpServerTracerTest { @Test - public void extractForwardedFor() { - assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("for=1.1.1.1")); + public void extractForwarded() { + assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=1.1.1.1")); } @Test - public void extractForwardedForCaps() { - assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("For=1.1.1.1")); + 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 extractForwardedForMalformed() { - assertNull(HttpServerTracer.extractForwardedFor("for=;for=1.1.1.1")); + public void extractForwardedWithPort() { + assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=1.1.1.1:2222")); } @Test - public void extractForwardedForEmpty() { - assertNull(HttpServerTracer.extractForwardedFor("")); + 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 extractForwardedForEmptyValue() { - assertNull(HttpServerTracer.extractForwardedFor("for=")); + public void extractForwardedCaps() { + assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("For=1.1.1.1")); } @Test - public void extractForwardedForEmptyValueWithSemicolon() { - assertNull(HttpServerTracer.extractForwardedFor("for=;")); + public void extractForwardedMalformed() { + assertNull(HttpServerTracer.extractForwarded("for=;for=1.1.1.1")); } @Test - public void extractForwardedForNoFor() { - assertNull(HttpServerTracer.extractForwardedFor("by=1.1.1.1;test=1.1.1.1")); + public void extractForwardedEmpty() { + assertNull(HttpServerTracer.extractForwarded("")); } @Test - public void extractForwardedForMultiple() { - assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("for=1.1.1.1;for=1.2.3.4")); + public void extractForwardedEmptyValue() { + assertNull(HttpServerTracer.extractForwarded("for=")); } @Test - public void extractForwardedForMixedSplitter() { + 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.extractForwardedFor("test=abcd; by=1.2.3.4, for=1.1.1.1;for=1.2.3.4")); + 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("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 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 extractForwardedForEmpty() { + assertNull(HttpServerTracer.extractForwardedFor("")); + } + + @Test + public void extractForwardedForMultiple() { + assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("1.1.1.1,1.2.3.4")); + } + + @Test + 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 extractForwardedForMultipleWithPort() { + assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("1.1.1.1:2222,1.2.3.4")); + } + + @Test + public void extractForwardedForMultipleIpv6WithPort() { + assertEquals( + "1111:1111:1111:1111:1111:1111:1111:1111", + HttpServerTracer.extractForwardedFor( + "[1111:1111:1111:1111:1111:1111:1111:1111]:2222,1.2.3.4")); } } From 18dca64b9083937aaba4bbaba5d2709da2273b72 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 22 Jul 2021 13:54:50 -0700 Subject: [PATCH 2/3] More IPv6... --- .../api/tracer/HttpServerTracer.java | 19 ++++-- .../api/tracer/HttpServerTracerTest.java | 59 ++++++++++++++++--- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java index 8e5e6de61c9c..a62a591e66da 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java @@ -243,23 +243,30 @@ 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" 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) == '[') { - // from https://www.rfc-editor.org/rfc/rfc7239 - // "note that an IPv6 address is always enclosed in square brackets" - int end = forwarded.indexOf(']', start); - if (end <= 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); - // ok to check ':' (for start of port) because ipv6 case is checked above - if (c == ',' || c == ';' || c == ':') { + if (c == '.') { + inIpv4 = true; + } else if (c == ',' || c == ';' || c == '"' || (inIpv4 && c == ':')) { if (i == start) { // empty string return null; } diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java index 62c04bbf2357..4bbf314e6234 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java @@ -20,7 +20,7 @@ public void extractForwarded() { public void extractForwardedIpv6() { assertEquals( "1111:1111:1111:1111:1111:1111:1111:1111", - HttpServerTracer.extractForwarded("for=[1111:1111:1111:1111:1111:1111:1111:1111]")); + HttpServerTracer.extractForwarded("for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\"")); } @Test @@ -32,7 +32,8 @@ public void extractForwardedWithPort() { public void extractForwardedIpv6WithPort() { assertEquals( "1111:1111:1111:1111:1111:1111:1111:1111", - HttpServerTracer.extractForwarded("for=[1111:1111:1111:1111:1111:1111:1111:1111]:2222")); + HttpServerTracer.extractForwarded( + "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\"")); } @Test @@ -75,7 +76,7 @@ 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")); + "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\";for=1.2.3.4")); } @Test @@ -88,7 +89,7 @@ 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")); + "for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\";for=1.2.3.4")); } @Test @@ -103,7 +104,7 @@ 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=abcd; by=1.2.3.4, for=\"[1111:1111:1111:1111:1111:1111:1111:1111]\";for=1.2.3.4")); } @Test @@ -118,7 +119,7 @@ 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=abcd; by=1.2.3.4, for=\"[1111:1111:1111:1111:1111:1111:1111:1111]:2222\";for=1.2.3.4")); } @Test @@ -130,7 +131,21 @@ public void extractForwardedFor() { public void extractForwardedForIpv6() { assertEquals( "1111:1111:1111:1111:1111:1111:1111:1111", - HttpServerTracer.extractForwardedFor("[1111:1111:1111:1111:1111:1111:1111:1111]]")); + HttpServerTracer.extractForwardedFor("\"[1111:1111:1111:1111:1111:1111:1111:1111]\"")); + } + + @Test + 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 extractForwardedForIpv6Unbracketed() { + assertEquals( + "1111:1111:1111:1111:1111:1111:1111:1111", + HttpServerTracer.extractForwardedFor("1111:1111:1111:1111:1111:1111:1111:1111")); } @Test @@ -140,6 +155,13 @@ public void extractForwardedForWithPort() { @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")); @@ -157,11 +179,26 @@ public void extractForwardedForMultiple() { @Test 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 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 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")); @@ -169,6 +206,14 @@ public void extractForwardedForMultipleWithPort() { @Test public void extractForwardedForMultipleIpv6WithPort() { + assertEquals( + "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( From 589bcf4b20179a0ab84d29fa5261c229848a9d75 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 22 Jul 2021 14:27:08 -0700 Subject: [PATCH 3/3] Test proper quoted host:port for Forwarded case --- .../instrumentation/api/tracer/HttpServerTracer.java | 4 ++++ .../instrumentation/api/tracer/HttpServerTracerTest.java | 7 ++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java index a62a591e66da..98f3d298d1d8 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java @@ -247,6 +247,10 @@ static String extractForwardedFor(String forwarded) { // "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; diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java index 4bbf314e6234..a5ac1a1226c7 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracerTest.java @@ -25,7 +25,7 @@ public void extractForwardedIpv6() { @Test public void extractForwardedWithPort() { - assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=1.1.1.1:2222")); + assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=\"1.1.1.1:2222\"")); } @Test @@ -81,7 +81,7 @@ public void extractForwardedMultipleIpV6() { @Test public void extractForwardedMultipleWithPort() { - assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=1.1.1.1:2222;for=1.2.3.4")); + assertEquals("1.1.1.1", HttpServerTracer.extractForwarded("for=\"1.1.1.1:2222\";for=1.2.3.4")); } @Test @@ -111,7 +111,8 @@ public void extractForwardedMixedSplitterIpv6() { 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")); + HttpServerTracer.extractForwarded( + "test=abcd; by=1.2.3.4, for=\"1.1.1.1:2222\";for=1.2.3.4")); } @Test