Re: [PATCH] detaching sslbump from accel mode

On 9/12/2011 3:59 a.m., Alex Rousskov wrote:
> 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?

Unless I've missed something the only remnant we have of the original
CONNECT request-URL is stored in sslHostName. It is set from
request->GetHost() which is the bare hostname with port elided.
Relatively easy to send both, I just have not had time to investigate
sslHostName usage properly.

Amos
Received on Thu Dec 08 2011 - 21:34:50 MST