From 6b30d634265c04a53e994628a412af5bfb99919f Mon Sep 17 00:00:00 2001 From: Idel Pivnitskiy Date: Wed, 13 Sep 2023 09:53:10 -0700 Subject: [PATCH] `CONNECT` request to a proxy must include `Host` header (#2691) Motivation: According to RFC7230 section 5.4 (https://datatracker.ietf.org/doc/html/rfc7230#section-5.4), all HTTP/1.1 requests must contain the `Host` header. If the target URI includes an authority component, then a client MUST send a field-value for Host that is identical to that authority component. On the other hand, `content-length: 0` header is not required because request payload body is not expected: https://datatracker.ietf.org/doc/html/rfc9110#section-9.3.6 Modifications: - Enhance `ProxyTunnel` to parse and validate initial line and the host header before passing data to `handler`, return 400 if it finds an error in request; - Update `ProxyConnectConnectionFactoryFilter` to remove `content-lenght: 0` and add `host` header; - Use `BuilderUtils` in `HttpsProxyTest` to allow enabling wire logging via system properties; Result: `CONNECT` request follows RFC guidelines and examples. --- .../ProxyConnectConnectionFactoryFilter.java | 11 +- .../http/netty/HttpsProxyTest.java | 19 ++- .../servicetalk/http/netty/ProxyTunnel.java | 126 +++++++++++------- 3 files changed, 102 insertions(+), 54 deletions(-) diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java index 214bbdd8ab..9a6e0322aa 100644 --- a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java +++ b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java @@ -44,8 +44,7 @@ import static io.servicetalk.concurrent.api.Single.failed; import static io.servicetalk.concurrent.api.SourceAdapters.fromSource; import static io.servicetalk.http.api.HttpContextKeys.HTTP_TARGET_ADDRESS_BEHIND_PROXY; -import static io.servicetalk.http.api.HttpHeaderNames.CONTENT_LENGTH; -import static io.servicetalk.http.api.HttpHeaderValues.ZERO; +import static io.servicetalk.http.api.HttpHeaderNames.HOST; import static io.servicetalk.http.api.HttpResponseStatus.StatusClass.SUCCESSFUL_2XX; /** @@ -86,7 +85,13 @@ public Single newConnection(final ResolvedAddress resolvedAddress, connectAddress, LOGGER); return delegate().newConnection(resolvedAddress, contextMap, observer).flatMap(c -> { try { - return c.request(c.connect(connectAddress).addHeader(CONTENT_LENGTH, ZERO)) + // Send CONNECT request: https://datatracker.ietf.org/doc/html/rfc9110#section-9.3.6 + // Host header value must be equal to CONNECT request target, see + // https://github.com/haproxy/haproxy/issues/1159 + // https://datatracker.ietf.org/doc/html/rfc7230#section-5.4: + // If the target URI includes an authority component, then a client MUST send a field-value + // for Host that is identical to that authority component + return c.request(c.connect(connectAddress).setHeader(HOST, connectAddress)) .flatMap(response -> handleConnectResponse(c, response).shareContextOnSubscribe()) // Close recently created connection in case of any error while it connects to the // proxy: diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpsProxyTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpsProxyTest.java index df5f5918ab..eda012d703 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpsProxyTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpsProxyTest.java @@ -31,10 +31,12 @@ import io.servicetalk.transport.api.ServerContext; import io.servicetalk.transport.api.ServerSslConfigBuilder; import io.servicetalk.transport.api.TransportObserver; +import io.servicetalk.transport.netty.internal.ExecutionContextExtension; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; import java.net.InetSocketAddress; import java.util.concurrent.atomic.AtomicReference; @@ -48,7 +50,6 @@ import static io.servicetalk.http.api.HttpSerializers.textSerializerUtf8; import static io.servicetalk.test.resources.DefaultTestCerts.serverPemHostname; import static io.servicetalk.transport.netty.NettyIoExecutors.createIoExecutor; -import static io.servicetalk.transport.netty.internal.AddressUtils.localAddress; import static io.servicetalk.transport.netty.internal.AddressUtils.serverHostAndPort; import static java.nio.charset.StandardCharsets.US_ASCII; import static org.hamcrest.MatcherAssert.assertThat; @@ -59,6 +60,15 @@ class HttpsProxyTest { + @RegisterExtension + static final ExecutionContextExtension SERVER_CTX = + ExecutionContextExtension.cached("server-io", "server-executor") + .setClassLevel(true); + @RegisterExtension + static final ExecutionContextExtension CLIENT_CTX = + ExecutionContextExtension.cached("client-io", "client-executor") + .setClassLevel(true); + private final ProxyTunnel proxyTunnel = new ProxyTunnel(); private final AtomicReference targetAddress = new AtomicReference<>(); @@ -104,7 +114,7 @@ static void safeClose(@Nullable AutoCloseable closeable) { } void startServer() throws Exception { - serverContext = HttpServers.forAddress(localAddress(0)) + serverContext = BuilderUtils.newServerBuilder(SERVER_CTX) .ioExecutor(serverIoExecutor = createIoExecutor("server-io-executor")) .sslConfig(new ServerSslConfigBuilder(DefaultTestCerts::loadServerPem, DefaultTestCerts::loadServerKey).build()) @@ -114,9 +124,8 @@ void startServer() throws Exception { } void createClient() { - assert serverAddress != null && proxyAddress != null; - client = HttpClients - .forSingleAddress(serverAddress) + assert serverContext != null && proxyAddress != null; + client = BuilderUtils.newClientBuilder(serverContext, CLIENT_CTX) .proxyAddress(proxyAddress) .sslConfig(new ClientSslConfigBuilder(DefaultTestCerts::loadServerCAPem) .peerHost(serverPemHostname()).build()) diff --git a/servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/ProxyTunnel.java b/servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/ProxyTunnel.java index bc27d4558e..6cf58aa344 100644 --- a/servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/ProxyTunnel.java +++ b/servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/ProxyTunnel.java @@ -28,10 +28,17 @@ import java.net.InetSocketAddress; import java.net.ServerSocket; import java.net.Socket; +import java.util.Locale; import java.util.concurrent.ExecutorService; import java.util.concurrent.atomic.AtomicInteger; import javax.annotation.Nullable; +import static io.servicetalk.http.api.HttpHeaderNames.CONTENT_LENGTH; +import static io.servicetalk.http.api.HttpHeaderNames.HOST; +import static io.servicetalk.http.api.HttpProtocolVersion.HTTP_1_1; +import static io.servicetalk.http.api.HttpRequestMethod.CONNECT; +import static io.servicetalk.http.api.HttpResponseStatus.BAD_REQUEST; +import static io.servicetalk.http.api.HttpResponseStatus.INTERNAL_SERVER_ERROR; import static java.net.InetAddress.getLoopbackAddress; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.Executors.newCachedThreadPool; @@ -42,7 +49,7 @@ */ public final class ProxyTunnel implements AutoCloseable { private static final Logger LOGGER = LoggerFactory.getLogger(ProxyTunnel.class); - private static final String CONNECT_PREFIX = "CONNECT "; + private static final String CONNECT_PREFIX = CONNECT + " "; private final ExecutorService executor = newCachedThreadPool(new DefaultThreadFactory("proxy-tunnel")); private final AtomicInteger connectCount = new AtomicInteger(); @@ -79,12 +86,41 @@ public HostAndPort startProxy() throws IOException { executor.submit(() -> { try { final InputStream in = socket.getInputStream(); - final String initialLine = readLine(in); - while (readLine(in).length() > 0) { - // Ignore headers. + final String host; + final int port; + final String protocol; + try { + final String initialLine = readLine(in); + if (!initialLine.startsWith(CONNECT_PREFIX)) { + throw new IllegalArgumentException("Expected " + CONNECT + " request, but found: " + + initialLine); + } + final int end = initialLine.indexOf(' ', CONNECT_PREFIX.length()); + final String authority = initialLine.substring(CONNECT_PREFIX.length(), end); + final int colon = authority.indexOf(':'); + host = authority.substring(0, colon); + port = Integer.parseInt(authority.substring(colon + 1)); + protocol = initialLine.substring(end + 1); + + final String hostHeader = readLine(in); + if (!hostHeader.toLowerCase(Locale.ROOT).startsWith(HOST.toString())) { + throw new IllegalArgumentException("Expected " + HOST + " header, but found: " + + hostHeader); + } + final String hostHeaderValue = hostHeader.substring(HOST.length() + 2 /* colon & space */); + if (!(host + ':' + port).equalsIgnoreCase(hostHeaderValue)) { + throw new IllegalArgumentException( + "Host header value must be identical to authority component"); + } + + while (readLine(in).length() > 0) { + // Ignore any other headers. + } + } catch (Exception e) { + badRequest(socket, e.getMessage()); + return; } - - handler.handle(socket, initialLine); + handler.handle(socket, host, port, protocol); } catch (Exception e) { LOGGER.debug("Error from proxy socket={}", socket, e); } finally { @@ -102,13 +138,21 @@ public HostAndPort startProxy() throws IOException { return HostAndPort.of(serverSocketAddress.getAddress().getHostAddress(), serverSocketAddress.getPort()); } + private static void badRequest(final Socket socket, final String cause) throws IOException { + final OutputStream os = socket.getOutputStream(); + os.write((HTTP_1_1 + " " + BAD_REQUEST + "\r\n" + + CONTENT_LENGTH + ": " + cause.length() + "\r\n" + + "\r\n" + cause).getBytes(UTF_8)); + os.flush(); + } + /** * Changes the proxy handler to return 500 instead of 200. */ public void badResponseProxy() { - handler = (socket, initialLine) -> { + handler = (socket, host, port, protocol) -> { final OutputStream os = socket.getOutputStream(); - os.write("HTTP/1.1 500 Internal Server Error\r\n\r\n".getBytes(UTF_8)); + os.write((protocol + ' ' + INTERNAL_SERVER_ERROR + "\r\n\r\n").getBytes(UTF_8)); os.flush(); }; } @@ -137,48 +181,38 @@ private static String readLine(final InputStream in) throws IOException { } } - private void handleRequest(final Socket serverSocket, final String initialLine) throws IOException { - if (initialLine.startsWith(CONNECT_PREFIX)) { - final int end = initialLine.indexOf(' ', CONNECT_PREFIX.length()); - final String authority = initialLine.substring(CONNECT_PREFIX.length(), end); - final String protocol = initialLine.substring(end + 1); - final int colon = authority.indexOf(':'); - final String host = authority.substring(0, colon); - final int port = Integer.parseInt(authority.substring(colon + 1)); - - try (Socket clientSocket = new Socket(host, port)) { - connectCount.incrementAndGet(); - final OutputStream out = serverSocket.getOutputStream(); - out.write((protocol + " 200 Connection established\r\n\r\n").getBytes(UTF_8)); - out.flush(); + private void handleRequest(final Socket serverSocket, final String host, final int port, + final String protocol) throws IOException { + try (Socket clientSocket = new Socket(host, port)) { + connectCount.incrementAndGet(); + final OutputStream out = serverSocket.getOutputStream(); + out.write((protocol + " 200 Connection established\r\n\r\n").getBytes(UTF_8)); + out.flush(); - executor.submit(() -> { + executor.submit(() -> { + try { + copyStream(out, clientSocket.getInputStream()); + } catch (IOException e) { + LOGGER.debug("Error copying clientSocket input to serverSocket output " + + "clientSocket={} serverSocket={}", clientSocket, serverSocket, e); + } finally { try { - copyStream(out, clientSocket.getInputStream()); + // We are simulating a proxy that doesn't do half closure. The proxy should close the server + // socket as soon as the server read is done. ServiceTalk's CloseHandler is expected to + // handle this gracefully (and delay FIN/RST until requests/responses complete). + // See GracefulConnectionClosureHandlingTest and ConnectionCloseHeaderHandlingTest. + serverSocket.close(); } catch (IOException e) { - LOGGER.debug("Error copying clientSocket input to serverSocket output " + - "clientSocket={} serverSocket={}", clientSocket, serverSocket, e); - } finally { - try { - // We are simulating a proxy that doesn't do half closure. The proxy should close the server - // socket as soon as the server read is done. ServiceTalk's CloseHandler is expected to - // handle this gracefully (and delay FIN/RST until requests/responses complete). - // See GracefulConnectionClosureHandlingTest and ConnectionCloseHeaderHandlingTest. - serverSocket.close(); - } catch (IOException e) { - LOGGER.debug("Error closing serverSocket={}", serverSocket, e); - } + LOGGER.debug("Error closing serverSocket={}", serverSocket, e); } - }); - copyStream(clientSocket.getOutputStream(), serverSocket.getInputStream()); + } + }); + copyStream(clientSocket.getOutputStream(), serverSocket.getInputStream()); - // Don't wait on the clientSocket input to serverSocket output copy to complete. We want to simulate a - // proxy that doesn't do half closure and that means we should close as soon as possible. ServiceTalk's - // CloseHandler should handle this gracefully (and delay FIN/RST until requests/responses complete). - // See GracefulConnectionClosureHandlingTest and ConnectionCloseHeaderHandlingTest. - } - } else { - throw new IllegalArgumentException("Unrecognized initial line: " + initialLine); + // Don't wait on the clientSocket input to serverSocket output copy to complete. We want to simulate a + // proxy that doesn't do half closure and that means we should close as soon as possible. ServiceTalk's + // CloseHandler should handle this gracefully (and delay FIN/RST until requests/responses complete). + // See GracefulConnectionClosureHandlingTest and ConnectionCloseHeaderHandlingTest. } // serverSocket is closed outside the scope of this method. } @@ -196,6 +230,6 @@ private static void copyStream(final OutputStream out, final InputStream cin) th @FunctionalInterface private interface ProxyRequestHandler { - void handle(Socket socket, String initialLine) throws IOException; + void handle(Socket socket, String host, int port, String protocol) throws IOException; } }