[squid-dev] [PATCH] splicing resumed sessions

[Amos Jeffries]

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.


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


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.


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

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

* 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/

* Ssl::Bio::sslFeatures::parseV3ServerHello()
 - s/SSL Exntension/TLS Extension/ ... spelling, and extensions is
another TLS-specific feature.
 - 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.
 - please update the comment about detecting NPN extension to indicate
its "obsoleted by ALPN".
 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.

* 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')


in src/ssl/bio.h

* comments have same issues as in .cc:
 - s/client hello/ClientHello/
 - s/server hello/ServerHello/
 - and what is a server "Hello A" message ?

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


Amos