Re: [PATCH] detaching sslbump from accel mode

On 12/08/2011 06:34 AM, Amos Jeffries wrote:
> On 8/12/2011 11:58 a.m., Amos Jeffries wrote:
>> On Wed, 07 Dec 2011 18:59:15 +0200, Tsantilas Christos wrote:
>>> On 12/07/2011 01:19 PM, Amos Jeffries wrote:
>>>> So, in theory ssl-bump is a form of intercept not a form of
>>>> accelerator. Its use of prepareAcceleratorURL() to convert the partial
>>>> to absolute URL unconditionally sets the accel flag with a mix of side
>>>> effects. Some bad ones have been identified already.
>>>>
>>>> This patch changes the flag setting, to allow ssl-bump to use the URL
>>>> preparation function without the side effects. I'm in half a mind to
>>>> make a ssl-bump specific URL preparation function, but only after this
>>>> is proven workable.
>>>>
>>>> Christos: as the person who appears to have the best testing ability
>>>> for
>>>> ssl-bump can you run your tests over the resulting Squid and check that
>>>> the expected behaviours have not changed for the worse? I am fully
>>>> expecting there to be several as yet unknown places needing to add a
>>>> test of the sslBumped flag alongside testing accel flag.
>>>
>>> Looks OK.
>>> Also because accel and related flags are used in http authentication and
>>> esi, I think too it is better to detach it from sslbumped requests,
>>> where we should handle authentication with a different way...
>>>
>>> The only objection is for the Config.onoff.ie_refresh parameter.
>>> Look in the client_side_request.cc line 1027 inside block:
>>> if (Config.onoff.ie_refresh) {
>>> }
>>>
>>> But does not look important....
>>
>> Hmm, maybe. The docs around that code are incorrect "or transparent
>> proxy" is not one of the cases it kicks in. Only (accel && ims).
>> We can either drop it or need to add all of sslbump, intercept,
>> tproxy, and accel flags.
>>
>> Since it is so screwed I think we can leave it for now and fix
>> separately.
>>
>>
>> Thank you for the check. I have moved on to splitting the prepare*()
>> function now.
>>
>> On the SSL intercept case we don't have any backup host:port that I
>> can see easily. If it is saved somewhere please let me know.
>>
>> For now the order of hot:port locating is:
>> - use bumped requests Host: header, else
>> - use CONNECT authority-URL, (http->sslHostName contains the host:port
>> from the CONNECT right?)
>> - reject with "400 Invalid Request" (RFC 2616 mandated response on
>> missing Host:)
>>
>> I have also added a test case to the transparent intercept case to
>> default port-443 received traffic through this new https:// URL
>> emitting function. port-80 and unknown ports through the old function
>> emitting http:// URLs.

> Here is the mk2 patch. With better re-assembly of the absolute URL for
> ssl-bump and SSL interception case as well.
>
> The only thing I'm worried about now is that strange ports on CONNECT
> request being ssl-bumped might be lost with sslHostName. The same is
> assumed for intercepted SSL, so its not a huge blocker, but could be a
> problem for some. Ideas?

If the changes result in Squid losing port information, it is a blocker.
I think it is OK to use 80/443 in comments (HTTP and HTTPS OR http_port
and https_port may be better though), but the actual port should be
passed to wherever it is needed. Can you clarify why passing the actual
port is problematic/difficult?

Thank you,

Alex.
Received on Thu Dec 08 2011 - 14:59:46 MST