Skip to content

Commit

Permalink
CONNECT request to a proxy must include Host header (#2691)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@idelpivnitskiy
idelpivnitskiy authored Sep 13, 2023
1 parent 2d1c8ca commit 6b30d63
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 54 deletions.
11 changes: 8 additions & 3 deletions ...tp-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectConnectionFactoryFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -86,7 +85,13 @@ public Single<C> 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:
Expand Down
19 changes: 14 additions & 5 deletions servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpsProxyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<Object> targetAddress = new AtomicReference<>();

Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand Down
126 changes: 80 additions & 46 deletions servicetalk-http-netty/src/testFixtures/java/io/servicetalk/http/netty/ProxyTunnel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down Expand Up @@ -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 {
Expand All @@ -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();
};
}
Expand Down Expand Up @@ -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.
}
Expand All @@ -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;
}
}

0 comments on commit 6b30d63

Please sign in to comment.