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.
