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

[Amos Jeffries]

On 2/11/2016 4:31 a.m., Christos Tsantilas wrote:
> 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();
> 

It is not problems in Squid itself, but in the analysis tools being
confused. We can try this one and see how it goes.


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

I'm okay with that. Please do it the way Alex suggested though that is
slightly better.

+1.

Amos