From 83b5121f75e4b54c869d390706e9086f634f811a Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 23 Jul 2021 00:59:55 -0700 Subject: [PATCH] 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 --- .../api/tracer/HttpServerTracer.java | 47 +++- .../api/tracer/HttpServerTracerTest.java | 211 ++++++++++++++++-- 2 files changed, 225 insertions(+), 33 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 fa12937695..98f3d298d1 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 @@ public abstract class HttpServerTracer e // 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 @@ public abstract class HttpServerTracer e // 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 @@ public abstract class HttpServerTracer e } // 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 @@ public abstract class HttpServerTracer e 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/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 fb27162afc..a5ac1a1226 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 @@ -11,19 +11,161 @@ import static org.junit.Assert.assertNull; 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 extractForwardedForCaps() { - assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("For=1.1.1.1")); + 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 extractForwardedForMalformed() { - assertNull(HttpServerTracer.extractForwardedFor("for=;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 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 @@ -31,30 +173,51 @@ public class HttpServerTracerTest { assertNull(HttpServerTracer.extractForwardedFor("")); } - @Test - public void extractForwardedForEmptyValue() { - assertNull(HttpServerTracer.extractForwardedFor("for=")); - } - - @Test - public void extractForwardedForEmptyValueWithSemicolon() { - assertNull(HttpServerTracer.extractForwardedFor("for=;")); - } - - @Test - public void extractForwardedForNoFor() { - assertNull(HttpServerTracer.extractForwardedFor("by=1.1.1.1;test=1.1.1.1")); - } - @Test public void extractForwardedForMultiple() { - assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("for=1.1.1.1;for=1.2.3.4")); + assertEquals("1.1.1.1", HttpServerTracer.extractForwardedFor("1.1.1.1,1.2.3.4")); } @Test - public void extractForwardedForMixedSplitter() { + public void extractForwardedForMultipleIpv6() { 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]\",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")); + } + + @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( + "[1111:1111:1111:1111:1111:1111:1111:1111]:2222,1.2.3.4")); } }