[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.
>
> 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).

I make FwdState related fixes a separate patch and I remove it from main 
patch.
Do you believe that the SQUID-211-FwdState_connectStart_cleanup-t1.patch 
will cause problems to Squid?
We can just remove the line :
   pinned_connection->stopPinnedConnectionMonitoring();

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

OK. I made this and some other similar fixes.


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

ok for this.

>
>  - 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");


I did not this fix Amos.
I will make a separate commit after this patch committed if you want it.

>
> Amos
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-211-FwdState_connectStart_cleanup-t1.patch
Type: text/x-patch
Size: 4005 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161101/88ee123a/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-211-Skype_groups_and_msnp_bypass-t10.patch
Type: text/x-patch
Size: 91425 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161101/88ee123a/attachment-0003.bin>