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

[Christos Tsantilas]

On 10/28/2016 01:11 PM, Amos Jeffries wrote:
> 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).

If you refer to Http::StreamPointer use instead of a raw Http::Stream, 
this is because after the latest changes the context (the Http::Stream 
object), can be deleted somewhere inside processParsedRequest().

It is not trying to fix a leak. This code required because 
clientTunnelOnError() may destroy the Http::Stream object.
But my sense is that the doCallouts (called somewhere inside 
processParsedRequest/clientProcessRequest) may result to Http::Stream 
object, so probably this part of patch fixes other bugs too.

Personally I prefer to keep this patch as one patch, it is not easy to 
split it. However if you insist I will  apply as separate patch the 
ConnStateData::clientParseRequests() changes plus the other Http::Stream 
raw pointers to Http:streamPointer conversion..


>
>
> 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?

This method is removed because it is needles. It has already called from 
inside pinned_connection->borrowPinnedConnection() call, some lines before.
The patch does not actually change anything important in these lines, 
just cleanup the code.

>
>
> 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
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>