Re: [PATCH] Avoid abandoned client connections after url_rewriter redirects CONNECT

On 19/01/2013 11:53 a.m., Alex Rousskov wrote:
> On 01/18/2013 05:32 AM, Amos Jeffries wrote:
>> On 17/01/2013 6:47 a.m., Alex Rousskov wrote:
>>> Hello,
>>>
>>> clientProcessRequest() assumes that a CONNECT request is always
>>> tunneled and sets flags.readMore to false. However, if url_rewriter
>>> redirects the CONNECTing user, Squid responds with a redirect message
>>> and does not tunnel. In that case, we do want to read more. Otherwise,
>>> keepaliveNextRequest() will declare the connection abandoned and the
>>> connection descriptor will "leak" until the connection lifetime expires,
>>> even if the client disconnects immediately after receiving the redirect
>>> response.
>>>
>>> The fix delays setting flags.readMore to false until we are about to
>>> call tunnelStart().
>>>
>>> The effect on CONNECT authentication (another case where CONNECT is not
>>> tunneled) is untested, but I hope that code continues to work because it
>>> should be OK with reading more requests on the [being] authenticated
>>> connection.
>>>
>>> These changes may also fix other similar not-tunneled CONNECT cases.
>>> They are not related to SslBump.
>>>
>>>
>>> One of the attached patches is for trunk. The other one is for v3.2. I
>>> did not check whether the problem exists in v3.1.
>>>
>>>
>>> HTH,
>>>
>>> Alex.
>> +1. It looks okay, although I would like confirmation from somebody
>> using NTLM authentication that things still work afterwards.
>
> My patches broke SslBump -- Squid started reading from the client
> connection before switching to SSL :-(.
>
> The attached patches were tested with SslBump (and without). They are
> more conservative/less invasive: I now leave readMore set to false for
> CONNECT until it is clear that Squid is not going to tunnel or bump.
> This should break fewer cases.

I think the correct way will be to take your first patch, but shuffle
the use of stopReading() above tunnelStart() to above both tunnelStart()
and sslBumpStart(), then add a readMore=true when sslBump setup is
completed.

If this works it confirms a suspicion of mine that sslBumpStart() should
be moved inside tunnelStart() and that function should be the place to
select between handling CONNECT as ssl-bump / Upgrade: / or blind tunnel.

Amos
Received on Sat Jan 19 2013 - 03:43:50 MST