[squid-dev] [PATCH] splicing resumed sessions

Fri Apr 10 13:49:11 UTC 2015

I am attaching patch for trunk and squid-3.5

On 04/09/2015 04:13 PM, Amos Jeffries wrote:
> On 9/04/2015 10:53 p.m., Tsantilas Christos wrote:
>> A new version of the patch.
>>
>> This is removes the ssl_bump_resuming_sessions directive, includes many
>> fixes over the previous patch.
>> Also include support for NPN and ALPN tls extensions, required to
>> correctly bump SSL connections.
>> Please read carefully the patch preamble , specially the technical note
>> part.
>>
>> The resumed sessions and the NPN/ALPN extensions problem appeared in
>> squid after our decision to not allow splicing of connections for which
>> we do not have access on the server certificates. The resumed sessions
>> does not include server certificates, and the NPN/ALPN extensions causes
>> openSSL to abort before retrieve and verify server certificates.
>
> Regarding NPN in Chrome:
>    https://www.imperialviolet.org/2013/03/20/alpn.html
>
> So for now this patch is okay, but we/you should already be thinking
> about how to auto-translate NPN from clients into ALPN to servers.

Probably we can do it for bump-server first or bump-client first.
In this cases we are not using at all NPN or ALPN now because we are 
only supporting the http/1.1 protocol.
We may have consider it in the case we are going to support SPDY or 
similar protocols in the future.

>
>
>>
>> The problem affects the ssl bumping and make it unusable for many cases.
>> Many of the problems which reported by the users for squid-3.5 should be
>> related to this.
>> So probably this patch should applied to squid-3.5 too. If yes I will
>> post the patch for squid-3.5 too.
>
> Yes on that.
I am including a patch for squid-3.5 too.

>
>
> As for the audit of this patch. All I've spotted is cosmetic debugs and
> comments needing fixes.
>
> +1 from me on the logic changes. Pelae apply when the below are resolved.

I did most of the changes you are requested, but maybe not all. Please 
look bellow.

>
>
> in src/ssl/bio.cc:
>
> * please thread the parser code in this file with references to relevant
> sections of RFC 5246 where the message syntax is defined.
>   - you can see the http/one/*Parser.cc comments for examples

I tried to add some code inside SSL parsing code. In the future we can 
improve them as the parser improved and extended.

>
> * Ssl::ServerBio::resumingSession() has unnecessary empty line and
> if-condition brackets {} at the start

yep

>
> * Ssl::Bio::sslFeatures::checkForCcsOrNst()
>   - comment on the second if-condition (looking for ticket) is wrong.
>   - "New Session Ticket" is a TLS feature. So in the debugs
>    s/SSL New Session Ticket/TLS New Session Ticket/

done

>
> * Ssl::Bio::sslFeatures::parseV3ServerHello()
>   - s/SSL Exntension/TLS Extension/ ... spelling, and extensions is
> another TLS-specific feature.

done

>   - there is inconsistent case on "Server Hello" all over this function.
> The RFC use "ServerHello" - CamelCase without whitespace.
>
> * Ssl::Bio::sslFeatures::parseV3Hello()
>   - similar issues with s/Client Hello/ClientHello/ and "SSL Extension"
> as above.

I did it only for the comments added or modified by this patch to avoid 
increasing the size of this patch.
If required we can do it as separate patch.

>   - please update the comment about detecting NPN extension to indicate
> its "obsoleted by ALPN".

I did not add this comment. A such comment implies that there is 
something we have to fix or change something here because it is obsoleted.
Still we have to handle NPN if client and server use it.

>   Is it possible to take the protocol list from NPN and treat it as if
> ALPN was received? if yes please do, otherwise this is fine.

No we can not do it. We are only bumping, we are not using NPN or ALPN 
for selecting protocol to cuminicate with server and client.

>
> * Ssl::Bio::sslFeatures::print()
>   - seems to be lacking display of ALPN received
>   - missing the format details for sslVersion used elsewhere:
>       std::hex << std::setw(8) << std::setfill('0')

I did not fix this. The ALPN is in an encoded form and requires some 
development to correctly print it. We do not have to gain something 
implementing this.
Also the Ssl::Bio::sslFeatures::print currently is not used. It is here 
for using it for debugging if required.

>
>
> in src/ssl/bio.h
>
> * comments have same issues as in .cc:
>   - s/client hello/ClientHello/
>   - s/server hello/ServerHello/

OK for comments added by this patch.

>   - and what is a server "Hello A" message ?
It is used in openSSL source code. I removed the "A".

>
> * parseMsgHead() documentation about return result >0 is wrong.
>    - it does not return <0 when the contents of the buffer are a TLS
> (non-SSL) message.

This is what it does!

>
>
> Amos
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: splicing-resumed-sessions-Squid-3.5-t12.patch
Type: text/x-patch
Size: 57944 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150410/b29fa730/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: splicing-resumed-sessions-t12.patch
Type: text/x-patch
Size: 58383 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150410/b29fa730/attachment-0003.bin>