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

[Alex Rousskov]

On 06/19/2015 06:46 PM, Amos Jeffries wrote:
> 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.


How is that problem related to the proposed fix?

The posted patch does not change or add new parsing code AFAICT. Whoever
deprecated HttpReply::parse() could update tunnel.cc but had apparently
chosen not to do it. The patch does not change that old tunnel.cc
parsing code. Does the patch add some new parsing code that I missed?


Thank you,

Alex.