From d60cd0231d4a7e67cc38f00bfc5c13856e937fcb Mon Sep 17 00:00:00 2001 From: Stephane Landelle Date: Fri, 23 Mar 2018 11:58:43 +0100 Subject: [PATCH] HttpProxyHandler generates invalid CONNECT url and Host header when address is resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motivation: HttpProxyHandler uses `NetUtil#toSocketAddressString` to compute CONNECT url and Host header. The url is correct when the address is unresolved, as `NetUtil#toSocketAddressString` will then use `getHoststring`/`getHostname`. If the address is already resolved, the url will be based on the IP instead of the hostname. There’s an additional minor issue with the Host header: default port 443 should be omitted. Modifications: * Introduce NetUtil#getHostname * Introduce HttpUtil#formatHostnameForHttp to format an InetSocketAddress to HTTP format * Change url computation to favor hostname instead of IP * Introduce HttpProxyHandler ignoreDefaultPortsInConnectHostHeader parameter to ignore 80 and 443 ports in Host header Result: HttpProxyHandler performs properly when connecting to a resolved address --- .../io/netty/handler/codec/http/HttpUtil.java | 19 +++ .../handler/codec/http/HttpUtilTest.java | 38 +++++- .../src/main/java/io/netty/util/NetUtil.java | 14 +- .../netty/handler/proxy/HttpProxyHandler.java | 32 ++++- .../handler/proxy/HttpProxyHandlerTest.java | 126 +++++++++++++++--- 5 files changed, 206 insertions(+), 23 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java index 6893293499..5b6e90e9ab 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java @@ -17,7 +17,9 @@ package io.netty.handler.codec.http; import io.netty.util.AsciiString; import io.netty.util.CharsetUtil; +import io.netty.util.NetUtil; +import java.net.InetSocketAddress; import java.net.URI; import java.util.ArrayList; import java.nio.charset.Charset; @@ -511,4 +513,21 @@ public final class HttpUtil { return contentTypeValue.length() > 0 ? contentTypeValue : null; } } + + /** + * Formats the host string of an address so it can be used for computing an HTTP component + * such as an URL or a Host header + * @param addr the address + * @return the formatted String + */ + public static String formatHostnameForHttp(InetSocketAddress addr) { + String hostString = NetUtil.getHostname(addr); + if (NetUtil.isValidIpV6Address(hostString)) { + if (!addr.isUnresolved()) { + hostString = NetUtil.toAddressString(addr.getAddress()); + } + return "[" + hostString + "]"; + } + return hostString; + } } diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java index 0a2de48795..6ad08e6b4c 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java @@ -19,13 +19,14 @@ import io.netty.util.CharsetUtil; import io.netty.util.ReferenceCountUtil; import org.junit.Test; +import java.net.InetAddress; +import java.net.InetSocketAddress; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.List; import static io.netty.handler.codec.http.HttpHeadersTestUtils.of; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasToString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -256,4 +257,39 @@ public class HttpUtilTest { ReferenceCountUtil.release(message); } + @Test + public void testFormatHostnameForHttpFromResolvedAddressWithHostname() throws Exception { + InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("localhost"), 8080); + assertEquals("localhost", HttpUtil.formatHostnameForHttp(socketAddress)); + } + + @Test + public void testFormatHostnameForHttpFromUnesolvedAddressWithHostname() { + InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("localhost", 80); + assertEquals("localhost", HttpUtil.formatHostnameForHttp(socketAddress)); + } + + @Test + public void testIpv6() throws Exception { + InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("::1"), 8080); + assertEquals("[::1]", HttpUtil.formatHostnameForHttp(socketAddress)); + } + + @Test + public void testIpv6Unresolved() { + InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("::1", 8080); + assertEquals("[::1]", HttpUtil.formatHostnameForHttp(socketAddress)); + } + + @Test + public void testIpv4() throws Exception { + InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("10.0.0.1"), 8080); + assertEquals("10.0.0.1", HttpUtil.formatHostnameForHttp(socketAddress)); + } + + @Test + public void testIpv4Unresolved() { + InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("10.0.0.1", 8080); + assertEquals("10.0.0.1", HttpUtil.formatHostnameForHttp(socketAddress)); + } } diff --git a/common/src/main/java/io/netty/util/NetUtil.java b/common/src/main/java/io/netty/util/NetUtil.java index 3bc8a0b4ca..99d739c943 100644 --- a/common/src/main/java/io/netty/util/NetUtil.java +++ b/common/src/main/java/io/netty/util/NetUtil.java @@ -897,8 +897,8 @@ public final class NetUtil { final StringBuilder sb; if (addr.isUnresolved()) { - String hostString = PlatformDependent.javaVersion() >= 7 ? addr.getHostString() : addr.getHostName(); - sb = newSocketAddressStringBuilder(hostString, port, !isValidIpV6Address(hostString)); + String hostname = getHostname(addr); + sb = newSocketAddressStringBuilder(hostname, port, !isValidIpV6Address(hostname)); } else { InetAddress address = addr.getAddress(); String hostString = toAddressString(address); @@ -1068,6 +1068,16 @@ public final class NetUtil { return b.toString(); } + /** + * Returns {@link InetSocketAddress#getHostString()} if Java >= 7, + * or {@link InetSocketAddress#getHostName()} otherwise. + * @param addr The address + * @return the host string + */ + public static String getHostname(InetSocketAddress addr) { + return PlatformDependent.javaVersion() >= 7 ? addr.getHostString() : addr.getHostName(); + } + /** * Does a range check on {@code value} if is within {@code start} (inclusive) and {@code end} (exclusive). * @param value The value to checked if is within {@code start} (inclusive) and {@code end} (exclusive) diff --git a/handler-proxy/src/main/java/io/netty/handler/proxy/HttpProxyHandler.java b/handler-proxy/src/main/java/io/netty/handler/proxy/HttpProxyHandler.java index 04315cf111..2c41faba0f 100644 --- a/handler-proxy/src/main/java/io/netty/handler/proxy/HttpProxyHandler.java +++ b/handler-proxy/src/main/java/io/netty/handler/proxy/HttpProxyHandler.java @@ -29,11 +29,11 @@ import io.netty.handler.codec.http.HttpHeaders; import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpResponse; import io.netty.handler.codec.http.HttpResponseStatus; +import io.netty.handler.codec.http.HttpUtil; import io.netty.handler.codec.http.HttpVersion; import io.netty.handler.codec.http.LastHttpContent; import io.netty.util.AsciiString; import io.netty.util.CharsetUtil; -import io.netty.util.NetUtil; import java.net.InetSocketAddress; import java.net.SocketAddress; @@ -47,6 +47,7 @@ public final class HttpProxyHandler extends ProxyHandler { private final String username; private final String password; private final CharSequence authorization; + private final boolean ignoreDefaultPortsInConnectHostHeader; private HttpResponseStatus status; private HttpHeaders headers; @@ -55,11 +56,18 @@ public final class HttpProxyHandler extends ProxyHandler { } public HttpProxyHandler(SocketAddress proxyAddress, HttpHeaders headers) { + this(proxyAddress, headers, false); + } + + public HttpProxyHandler(SocketAddress proxyAddress, + HttpHeaders headers, + boolean ignoreDefaultPortsInConnectHostHeader) { super(proxyAddress); username = null; password = null; authorization = null; this.headers = headers; + this.ignoreDefaultPortsInConnectHostHeader = ignoreDefaultPortsInConnectHostHeader; } public HttpProxyHandler(SocketAddress proxyAddress, String username, String password) { @@ -68,6 +76,14 @@ public final class HttpProxyHandler extends ProxyHandler { public HttpProxyHandler(SocketAddress proxyAddress, String username, String password, HttpHeaders headers) { + this(proxyAddress, username, password, headers, false); + } + + public HttpProxyHandler(SocketAddress proxyAddress, + String username, + String password, + HttpHeaders headers, + boolean ignoreDefaultPortsInConnectHostHeader) { super(proxyAddress); if (username == null) { throw new NullPointerException("username"); @@ -87,6 +103,7 @@ public final class HttpProxyHandler extends ProxyHandler { authzBase64.release(); this.headers = headers; + this.ignoreDefaultPortsInConnectHostHeader = ignoreDefaultPortsInConnectHostHeader; } @Override @@ -127,13 +144,20 @@ public final class HttpProxyHandler extends ProxyHandler { @Override protected Object newInitialMessage(ChannelHandlerContext ctx) throws Exception { InetSocketAddress raddr = destinationAddress(); - final String host = NetUtil.toSocketAddressString(raddr); + + String hostString = HttpUtil.formatHostnameForHttp(raddr); + int port = raddr.getPort(); + String url = hostString + ":" + port; + String hostHeader = (ignoreDefaultPortsInConnectHostHeader && (port == 80 || port == 443)) ? + hostString : + url; + FullHttpRequest req = new DefaultFullHttpRequest( HttpVersion.HTTP_1_1, HttpMethod.CONNECT, - host, + url, Unpooled.EMPTY_BUFFER, false); - req.headers().set(HttpHeaderNames.HOST, host); + req.headers().set(HttpHeaderNames.HOST, hostHeader); if (authorization != null) { req.headers().set(HttpHeaderNames.PROXY_AUTHORIZATION, authorization); diff --git a/handler-proxy/src/test/java/io/netty/handler/proxy/HttpProxyHandlerTest.java b/handler-proxy/src/test/java/io/netty/handler/proxy/HttpProxyHandlerTest.java index 5703371252..04747e44c0 100644 --- a/handler-proxy/src/test/java/io/netty/handler/proxy/HttpProxyHandlerTest.java +++ b/handler-proxy/src/test/java/io/netty/handler/proxy/HttpProxyHandlerTest.java @@ -34,39 +34,130 @@ import static org.mockito.Mockito.*; public class HttpProxyHandlerTest { @Test - public void testIpv6() throws Exception { + public void testHostname() throws Exception { + InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("localhost"), 8080); + testInitialMessage( + socketAddress, + "localhost:8080", + "localhost:8080", + null, + true); + } + + @Test + public void testHostnameUnresolved() throws Exception { + InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("localhost", 8080); + testInitialMessage( + socketAddress, + "localhost:8080", + "localhost:8080", + null, + true); + } + + @Test + public void testHostHeaderWithHttpDefaultPort() throws Exception { + InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("localhost"), 80); + testInitialMessage(socketAddress, + "localhost:80", + "localhost:80", null, + false); + } + + @Test + public void testHostHeaderWithHttpDefaultPortIgnored() throws Exception { + InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("localhost", 80); + testInitialMessage( + socketAddress, + "localhost:80", + "localhost", + null, + true); + } + + @Test + public void testHostHeaderWithHttpsDefaultPort() throws Exception { + InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("localhost"), 443); + testInitialMessage( + socketAddress, + "localhost:443", + "localhost:443", + null, + false); + } + + @Test + public void testHostHeaderWithHttpsDefaultPortIgnored() throws Exception { + InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("localhost", 443); + testInitialMessage( + socketAddress, + "localhost:443", + "localhost", + null, + true); + } + + @Test + public void testIpv6() throws Exception { InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("::1"), 8080); - testInitialMessage(socketAddress, "[::1]:8080", null); + testInitialMessage( + socketAddress, + "[::1]:8080", + "[::1]:8080", + null, + true); } @Test - public void testIpv6Unresolved() throws Exception { + public void testIpv6Unresolved() throws Exception { InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("::1", 8080); - testInitialMessage(socketAddress, "[::1]:8080", null); + testInitialMessage( + socketAddress, + "[::1]:8080", + "[::1]:8080", + null, + true); } @Test - public void testIpv4() throws Exception { + public void testIpv4() throws Exception { InetSocketAddress socketAddress = new InetSocketAddress(InetAddress.getByName("10.0.0.1"), 8080); - testInitialMessage(socketAddress, "10.0.0.1:8080", null); + testInitialMessage(socketAddress, + "10.0.0.1:8080", + "10.0.0.1:8080", + null, + true); } @Test - public void testIpv4Unresolved() throws Exception { + public void testIpv4Unresolved() throws Exception { InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("10.0.0.1", 8080); - testInitialMessage(socketAddress, "10.0.0.1:8080", null); + testInitialMessage( + socketAddress, + "10.0.0.1:8080", + "10.0.0.1:8080", + null, + true); } @Test public void testCustomHeaders() throws Exception { InetSocketAddress socketAddress = InetSocketAddress.createUnresolved("10.0.0.1", 8080); - testInitialMessage(socketAddress, "10.0.0.1:8080", - new DefaultHttpHeaders().add("CUSTOM_HEADER", "CUSTOM_VALUE1") - .add("CUSTOM_HEADER", "CUSTOM_VALUE2")); + testInitialMessage( + socketAddress, + "10.0.0.1:8080", + "10.0.0.1:8080", + new DefaultHttpHeaders() + .add("CUSTOM_HEADER", "CUSTOM_VALUE1") + .add("CUSTOM_HEADER", "CUSTOM_VALUE2"), + true); } - private static void testInitialMessage(InetSocketAddress socketAddress, String hostPort, - HttpHeaders headers) throws Exception { + private static void testInitialMessage(InetSocketAddress socketAddress, + String expectedUrl, + String expectedHostHeader, + HttpHeaders headers, + boolean ignoreDefaultPortsInConnectHostHeader) throws Exception { InetSocketAddress proxyAddress = new InetSocketAddress(NetUtil.LOCALHOST, 8080); ChannelPromise promise = mock(ChannelPromise.class); @@ -75,15 +166,18 @@ public class HttpProxyHandlerTest { ChannelHandlerContext ctx = mock(ChannelHandlerContext.class); when(ctx.connect(same(proxyAddress), isNull(InetSocketAddress.class), same(promise))).thenReturn(promise); - HttpProxyHandler handler = new HttpProxyHandler(new InetSocketAddress(NetUtil.LOCALHOST, 8080), headers); + HttpProxyHandler handler = new HttpProxyHandler( + new InetSocketAddress(NetUtil.LOCALHOST, 8080), + headers, + ignoreDefaultPortsInConnectHostHeader); handler.connect(ctx, socketAddress, null, promise); FullHttpRequest request = (FullHttpRequest) handler.newInitialMessage(ctx); try { assertEquals(HttpVersion.HTTP_1_1, request.protocolVersion()); - assertEquals(hostPort, request.uri()); + assertEquals(expectedUrl, request.uri()); HttpHeaders actualHeaders = request.headers(); - assertEquals(hostPort, actualHeaders.get(HttpHeaderNames.HOST)); + assertEquals(expectedHostHeader, actualHeaders.get(HttpHeaderNames.HOST)); if (headers != null) { // The actual request header is a strict superset of the custom header