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

On 24/01/2013 10:58 a.m., Alex Rousskov wrote:
> 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?

Yes that and some discussion I have been having recently about HTTP/1.x
framing convince me you are on the right track with patch #2 for now.

We will need to revisit this logic when adding HTTP/2 support. The
framing there is a LOT better, wven if it ends up being the
session-centric SPDY frame design.

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

You are overlooking that httpStart() is where ftp:// and gopher:// and
internal:// and ICY protocol are handled. Makign it clear these function
names are all out of true with what they do.

Also, ssl-bump is a special-case handling of tunnel data. So I see no
conceptual clash with tunnelStart() deciding which processing path to
use (use ssl-bump versus blind relay).

Amos
Received on Thu Jan 24 2013 - 02:24:35 MST