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

[Amos Jeffries]

On 21/10/2016 3:55 a.m., Christos Tsantilas wrote:
> 
> Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.
> 
> Use case: Skype groups appear to use TLS-encrypted MSNP protocol instead
> of HTTPS. This change allows Squid admins using SslBump to tunnel Skype
> groups and similar non-HTTP traffic bytes via "on_unsupported_protocol
> tunnel all". Previously, the combination resulted in encrypted HTTP 400
> (Bad Request) messages sent to the client (that does not speak HTTP).
> 
> Also this patch:
>  * fixes bug 4529: !EBIT_TEST(entry->flags, ENTRY_FWD_HDR_WAIT)
>    assertion in FwdState.cc.
> 
>  * when splicing transparent connections during SslBump step1, avoid
>    access-logging an extra record and log %ssl::bump_mode as the expected
>    "splice" not "none".
> 
>  * handles an XXX comment inside clientTunnelOnError for possible memory
>    leak of client streams related objects
> 
>  * fixes TunnelStateData logging in the case of splicing after peek.
> 
> This is a Measurement Factory project.


Are any of these additional fixes able to be easily broken out into
separate patches? It would greatly help the auditing process to get
smaller patches.

- It looks like the chunk in ConnStateData::clientParseRequests() is a
leak fix which could go in separately (eg right now if thats correct).


in src/FwdState.cc:

* Most of the changed lines are a needless de-scoping of code.
 - note that the sub-scope existed to begin with so as to silence noisy
false-positives by static analysis tools (trunk rev.14790).

* The one meaningful change is erasing
"pinned_connection->stopPinnedConnectionMonitoring()". Seemingly to move
it to tunnel.cc
 - are comm_add_close_handler() and syncWithServerConn() safe to run on
a still-monitored FD ?
 - what happens to all the pinned connection uses that do not lead to
tunnel? eg connection auth?


Please use nullptr in new code instead of NULL.
 - there is one part "if (cause != NULL && cause->method == " that
should either not have the " != NULL" bytes, or use nullptr explicitly.
 - I prefer the former, "if (cause && cause->method == ", when testing
pointers existence-vs-nil.


in tunnelPeerSelectComplete()
 - "if (!bail &&" can be replaced by "else if (".

 - please take advantage of the surrounding code being re-written to
cleanup:
    if (peer_paths == NULL || peer_paths->size() < 1) {
        debugs(26, 3, HERE << "No paths found. Aborting CONNECT");
as:
    if (!peer_paths || peer_paths->size() < 1) {
        debugs(26, 3, "No paths found. Aborting CONNECT");

Amos