Re: [PATCH] detaching sslbump from accel mode

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

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?

Amos