Re: [PATCH] detaching sslbump from accel mode

On 9/12/2011 12:44 p.m., Alex Rousskov wrote:
> On 12/08/2011 02:34 PM, Amos Jeffries wrote:
>> 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.
> FWIW, we are fiddling with sslHostName-related code as a part of the
> bump-server-first project. I even tried passing the request itself
> (instead of request->GetHost) but that did not work well. Our current
> code reads:
>
>> void
>> ConnStateData::switchToHttps(const char *host, const int port)
>> {
>> assert(!switchedToHttps_);
>>
>> sslHostName = host;
> so both host and port are now available where sslHostName is set (either
> from CONNECT or from intercepted connections). We can change sslHostName
> to something that holds both the host name and the port OR we can
> convert available information to full URI before calling switchToHttps().

:) I was looking at it today and reached the same conclusion. I added a
sslBumpedPort field to ConnStateData to hold the port, but did not quite
get around to the bits you have done to get the port there where the
member needs to get set.

Essentially we keep the authority-URL (only) but as two pieces instead
of one. The URL refactoring/cleanup work I have ticking away in the
background can fix that up again later in a nice way.

PS. Keep a record of the problems about that request passing. The bug
about doing re-CONNECT will likely hit the same issues again, since it
ideally sends the original request headers out again. Can be avoided if
it was very problematic, but forwarding things like unknown headers and
U-A details would be good in that bugs fix.

Amos
Received on Fri Dec 09 2011 - 11:48:56 MST