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

On 01/18/2013 08:43 PM, Amos Jeffries wrote:
> 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.

I do not think that is a good idea for two reasons:

1) As you know, both tunnelStart() and sslBumpStart() are called from
processRequest(). A lot of things can happen _before_ we get to that
method if we allow readMore to remain true while we are getting there!
Some of those things will probably lead to bugs.

For example, Squid may read [encrypted] data from the client socket and
try to interpret that as the beginning of the next request (or the body
of the current?). This is what I may have seen in the logs from take1
tests (I did not investigate, but it sounds quite possible based on the
code).

The key here is that ClientSocketContext::keepaliveNextRequest() and/or
clientAfterReadingRequests() may start reading and interpreting again if
readMore is true while doCallouts() for the previous request are still
"waiting" for some async call to return.

Granted, it may be possible that the same kind of bad things already
happen without my patches, but I do not want to add more of them
(because I would end up being responsible for fixing all of them as it
would appear that I broke something that worked).

2) The problem with take1 of the patch was not just that we started
reading from the client while SslBump was being setup. We also started
reading from the client when responding with an error (including
authentication?) or a redirect.

I am seriously worried that if we do not disable client reading, we will
hit assertions elsewhere on that path because Squid client-side is not
very good at handling a new request while the old one is still being
processed, especially in corner/error cases like the ones I just mentioned.

I think it is much safer to remain conservative here and disable reading
until it is known to be safe.

Did the above arguments convince you that take2 version of the patch is
better than what you are proposing?

> 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.

It would be wrong to start a non-tunneling path in a function called
tunnelStart() IMO. Currently, ClientHttpRequest::processRequest() is
where various "start" methods are called and where the decision to
tunnel or bump is made for CONNECT requests. I think that is a
reasonable design (even though the processRequest() method name is too
general and should be changed). We can move CONNECT handling to a
CONNECT-dedicated method, but the current code is too simple to
_require_ such a method IMO.

Thank you,

Alex.
Received on Wed Jan 23 2013 - 21:58:44 MST