[squid-dev] [PATCH] Do not blindly forward cache peer CONNECT responses

[Amos Jeffries]

On 20/06/2015 4:54 a.m., Alex Rousskov wrote:
> Hello,
> 
>     The attached trunk patch fixes a rare but nasty problem by removing
> a very old hack which shielded Squid from parsing most CONNECT responses.
> 
> Currently, Squid blindly forwards cache peer CONNECT responses to
> clients when possible. This may break things if the peer responds with
> something like HTTP 403 (Forbidden) and keeps the connection with Squid
> open:
> 
>   -  The client application issues a CONNECT request.
>   -  Squid forwards this request to a cache peer.
>   -  Cache peer correctly responds back with a "403 Forbidden".
>   -  Squid does not parse cache peer response and
>      just forwards it as if it was a Squid response to the client.
>   -  The TCP connections are not closed.
> 
> At this stage, Squid is unaware that the CONNECT request has failed. All
> subsequent requests on the user agent TCP connection are treated as
> tunnelled traffic. Squid is forwarding these requests to the peer on the
> TCP connection previously used for the 403-ed CONNECT request, without
> proper processing. The additional headers which should have been applied
> by Squid to these requests are not applied, and the requests are being
> forwarded to the cache peer even though the Squid configuration may
> state that these requests must go directly to the origin server.
> 
> This patch fixes Squid to parse cache peer responses, and if an error
> response found, respond with "502 Bad Gateway" to the client and close
> the connections.
> 

Thank you. I've had this on my todo list for a long time.

One problem though...

The parse operations documented as being copied from http.cc
processReplyHeader() do not match what trunk currently does for HTTP
replies in that function.

Specifically HttpReply::parse is deprecated and not to be used in new
code. We now have an Http1::ReplyParser instead that processes the
status-line and grabs the mime headers into a SBuf blob for writing out
if necessary. That is all tunnel.cc needs right now for this patches
parse step.

Amos