NettyConnector should not copy headers from request to CONNECT message

[jimdoble]

When using the NettyConnector, in conjunction with an HTTP proxy, any headers in the request being sent are copied into the HTTP CONNECT message which is being sent to the proxy. This can cause PUT and POST requests to fail, because these requests will typically include a content-length header, and copying this header to the CONNECT message signals to the proxy that the CONNECT message will have a request body, when in fact it does not. The proxy may wait for a request body that will never arrive, and the client will finally time out the request.

If you look at the createProxyHandler method in the NettyConnector class, we have:
HttpHeaders httpHeaders = setHeaders(jerseyRequest, new DefaultHttpHeaders());

ProxyHandler proxy = userName == null ? new HttpProxyHandler(proxyAddr, httpHeaders
            : new HttpProxyHandler(proxyAddr, userName, password, httpHeaders);`

The setHeaders method returns the set of headers in the current request, and we pass that set of headers to the HttpProxyHandler constructor. This constructor saves these headers in a member variable:
this.outboundHeaders = headers;
And later HttpProxyHandler will call the newInitialMessage method to construct a CONNECT message to send to the proxy:

protected Object newInitialMessage(ChannelHandlerContext ctx) throws Exception {
    InetSocketAddress raddr = destinationAddress();

    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,
            url,
            Unpooled.EMPTY_BUFFER, false);

    req.headers().set(HttpHeaderNames.HOST, hostHeader);

    if (authorization != null) {
        req.headers().set(HttpHeaderNames.PROXY_AUTHORIZATION, authorization);
    }

    if (outboundHeaders != null) {
        req.headers().add(outboundHeaders);
    }

    return req;

Note that HttpProxyHandler is always sending a CONNECT message as the initial message, even in the HTTP proxy case. But consider this code snippet in newInitialMessage:

if (outboundHeaders != null) {
    req.headers().add(outboundHeaders);
}

Here, we are adding whatever headers had been specified for the request we are intending to be sent to the target server, and are adding them to the CONNECT message that will be sent to the proxy. This is not what we want.

A simple problem that can arise here is if we are trying to send a POST request to the target server, and our request contains a content-length header, indicating the length of the request body we are posting. As a result of the way the code is currently written, this content-length header will be added to the CONNECT message that will be sent to the proxy, but the HttpProxyHandler will not actually send a request body along with the CONNECT message. The proxy will see the content-length header in the CONNECT message, and will wait for a request body which never comes. Eventually, the proxy request times out, because the proxy does not respond.

I thing the problem here is in the first snippet of code from NettyConnector:

HttpHeaders httpHeaders = setHeaders(jerseyRequest, new DefaultHttpHeaders());

ProxyHandler proxy = userName == null ? new HttpProxyHandler(proxyAddr, httpHeaders
                : new HttpProxyHandler(proxyAddr, userName, password, httpHeaders);`

We should not be passing the request headers from the jerseyRequest in the HttpProxyHandler constructor. The only headers which should be passed should be headers that we want to be included in the CONNECT message. Generally speaking, I think we should pass null in this case.

[jansupol]

It sounds like sending null and dropping all PROXY- prefixed HTTP headers is not the intended solution here.

Instead, we can drop all content-related headers such as CONTENT-ENCODING, CONTENT-LANGUAGE, CONTENT-LOCATION, CONTENT-TYPE, CONTENT-LENGTH, CONTENT-DISPOSITION, CONTENT-ID, LAST-MODIFIED, TRANSFER-ENCODING.

@jimdoble wdyt?

[jimdoble]

When it comes to determining which headers from the actual HTTP request should be copied to the CONNECT message, I wonder if it would be better to have an include list or an exclude list. That is, is there a small set that it makes sense to copy? Or a small set that we should not copy. For example, an exclude list would certainly need to include content-related headers, because the CONNECT message has no content. We should also omit any headers which may contain sensitive information (e.g. Authorization), because the CONNECT message is sent in the clear, as opposed to subsequent messages which would be carried over TLS. Are there any specific use cases of headers which should be copied from the HTTP request to the CONNECT message? I wonder if it would make sense to study how a client like the Apache client handles this, and duplicate that behavior.

…

On Mon, Sep 18, 2023 at 10:28 AM jansupol ***@***.***> wrote: It sounds like sending null and dropping all PROXY- prefixed HTTP headers is not the intended solution here. Instead, we can drop all content-related headers such as CONTENT-ENCODING, CONTENT-LANGUAGE, CONTENT-LOCATION, CONTENT-TYPE, CONTENT-LENGTH, CONTENT-DISPOSITION, CONTENT-ID, LAST-MODIFIED, TRANSFER-ENCODING. @jimdoble <https://github.com/jimdoble> wdyt? — Reply to this email directly, view it on GitHub <#5413 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BCRZRYVS72AAQKL7E4IL4QLX3BK75ANCNFSM6AAAAAA4WXAF4M> . You are receiving this because you were mentioned.Message ID: ***@***.***>