[squid-dev] [PATCH] Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.

[Christos Tsantilas]

On 10/19/2016 07:13 PM, Alex Rousskov wrote:
> On 10/19/2016 08:49 AM, Christos Tsantilas wrote:
>> I am attaching a new patch.
>
> I would like to discuss two issues:
>
>
> * Logging of scheme-less URLs
>
>> This is defines a new proto the PROTO_TCP, and for this prints the url
>> in the form host:port.
>
> The PROTO_TCP name sounds bad because we may want to log
> tcp://host:port/ URLs in the future as discussed earlier.
>
> PROTO_NONE and PROTO_UNKNOWN are already utilized for different _valid_
> use cases. I suggest adding PROTO_AUTHORITY or PROTO_AUTHORITY_FORM
> instead of adding PROTO_TCP.
>
>
> * flags.tunneling
>
> AFAIK, you added flags.tunneling specifically to work around the
> tunneling request removal from the ConnStateData pipeline (trunk commit
> r14418 "Cleanup: Refactor ConnStateData pipeline handling"). The
> justification for that removal is given in the corresponding commit
> message (thank you, Amos!):
>
>> Initial testing revealed CONNECT tunnels always being logged as ABORTED.
>> [..] I have now made the context be finished() just prior to the
>> TunnelStateData being destroyed. That way normal closure should show up
>> only as TUNNEL, but timeouts and I/O errors should still be recorded as
>> abnormal.
>
> That explanation makes sense to me -- when we know that the request has
> finished successfully, we should call its finished() method.
>
> AFAICT, the flags.tunneling is currently needed specifically because the
> following ConnStateData::checkLogging() condition is [sometimes] false
> when the pipeline is empty and we are done tunneling:
>
>>     // do not log connections that closed after a transaction (it is normal)
>>     if (receivedFirstByte_ && inBuf.isEmpty())
>>         return;
>
> Instead of adding flags.tunneling, can we fix/adjust that condition
> and/or the code that affects its components to make the condition true?
>
> receivedFirstByte_: This member remains false if we splice and tunnel
> intercepted connections during step1 because we read no bytes at that
> time, right? However, it feels like this part of the condition should be
> not receivedFirstByte_ but something like thereWasAtLeastOne (i.e., we
> pushed at least one request into ConnStateData pipeline). Can we adjust
> the pipeline code to remember that at least one request has been queued
> and use that instead of receivedFirstByte_ here.
>
> inBuf.isEmpty(): That part of the condition looks correct to me. If it
> is false in the tunneling case being discussed, then we need to fix the
> code to clean inBuf when tunneling starts.
>
> In summary, can we do something like this instead:
>
> // do not log connections that closed after a transaction (it is normal)
> if (pipeline.used() && inBuf.isEmpty())
>     return;

I think this is should work. We can use existing Pipeline::nrequests member:

if (pipeline.nrequests && inBuf.isEmpty())
     return;

Give me some time to check if there are any hidden issues


>
>
> Thank you,
>
> Alex.
>
>
>>  const SBuf &
>>  HttpRequest::effectiveRequestUri() const
>>  {
>> -    if (method.id() == Http::METHOD_CONNECT)
>> +    if (method.id() == Http::METHOD_CONNECT || url.getScheme() == AnyP::PROTO_TCP)
>>          return url.authority(true); // host:port
>>      return url.absolute();
>>  }
>