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

[Alex Rousskov]

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;


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();
>  }