[squid-dev] [PATCH] Fast SNI peek

[Amos Jeffries]

On 14/05/2016 5:07 a.m., Christos Tsantilas wrote:
> Currently, bumping peek mode at step2 and splice at step2, after the SNI
> is  received is very slow.
> 
> The most of the performance overhead comes from openSSL. However Squid
> does not need openSSL to peek at SNI. It needs only to get client TLS
> Hello message, analyse it to retrieve SNI and then splice at step2.
> 
> This patch:
>  - Postpone creation of the OpenSSL connection (i.e. SSL) object for the
> accepted TCP connection until after we peek at SNI (after step2).
> 
>  - Implements the Parser::BinaryTokenizer parser for extracting
> byte-oriented fields from raw input
> 
>  - Reimplement a new SSL/TLS handshake messages parser using the
> BinaryTokenizer, and remove old buggy parsing code from ssl/bio.cc
> 
>  - Adjust ConnStateData, Ssl::Bio, Ssl::PeerConnector classes to use the
> new parsers and parsing results.
> 

Thank you.

Below is all polish and documentation fixes :-)

+1. If nobody has anything else I think this can go in when you have
attended to these bits of polishing.
 (I will hold off on my PeerConnector related changes until this is in).



in src/anyp/ProtocolType.h:

* these protocol names should be orered from most to least common.
 - expect TLS to be more common than SSL in future now that all SSL
versions are formally prohibited.


in src/client_side.h:

* the cpmment block starting with:
  "// The following block does not needed,"

 can all be replaced with one line:
  // Setting bio->rBufData() is not needed. inBuf and rbuf have the same
content.

  - I am a little doubtful about the accuracy of that. Both places are
using SBuf to hold the buffer. If they are not kept in sync they might
end up pointing at different MemBlob offsets or different MemBlob
entirely. Did I might have missed the sync'ing ?


in src/client_side.cc:

* files in src/security/ should not need wrapping in USE_OPENSSL
 - referring to the #includes pulling in security/Handshake.h
 - maybe others

* tlsParser has no doxygen comment


in src/parser/BinaryTokenizer.cc:

* #include "parser/BinaryTokenizer.h" instead.

* delegating constructors is a C++11 feature that will definitely fail
on GCC 4.8.
 - I have put the question to Alex in how recent patch about dropping
that support. If we go ahead with that, the BinaryTokenizer construction
is fine. Otherwise it will have to be expanded to non-delegating.

* please make BinaryTokenizer_tail a function not a macro.
 - doing that will also fix the missing (void*) before use of 'this'

* please add to the BinaryTokenizer intN() methods that they assume
network-endian input.


in src/parser/BinaryTokenizer.h:

* please make the wrapper macro match the file name + path.
 - only inject '_' to replace '/' and '.'
  so: SQUID_SRC_PARSER_BINARYTOKENIZER_H
 - similar in src/security/Handshake.h

* are users of BinaryTokenizerContext allowed to change the name parameter?
 - if not it should be a "const char * const"
 - same for the parent

* please move the BinaryTokenizerContext inline method definitions
inside the class {} block.
 - then you wont need to even use "inline".


in src/security/Handshake.cc:

* Security::HandshakeParser::skipMessage
 - the comment seems to be mentioning some serious breakage of the
debugging output. That should probably get an XXX marker.


in src/security/Handshake.h:

* don't need base/RefCount.h or sbuf/SBuf.h here.
 - they get pulled by the other dependencies

* class HandshakeParser documentation "TLS/SSL" instead of just "SSL".

* put '{' on the line after "class ..."

* s/ressumingSession/resumingSession/
 - as a member this also affects ssl/bio.cc, maybe elsewhere

* tkRecords, tkMessages, and expectingModernRecords missing doxygen comments

 NP: just a note on class method definitions. Try to avoid naming the
parameters if you can. It seems to hep with the recent doxygen versions,
and the STUBS creation. We might have to if the type is basic like an
int or bool. But for types where the type-name describes the parameter
object you can omit.
 For example:  "foo(const tlsDetails &details)" the variable name is
redundant.


in src/security/NegotiationHistory.cc:

* s/NULL/nullptr/ when touching lines
 - the constructor list at least
 - bio.cc has some more in adjustSSL(), maybe elsewhere

* in toProtocolVersion() are the case values all guaranteed to exist?
 - specifically the older ones like SSL2_VERSION.
 - if not, now might be a good time to wrap the cases in #if defined()
   which might also solve issues with TLSv1.1/1.2 version in library ports


in src/ssl/PeekingPeerConnector.cc:

* the debugs dropped from Ssl::PeekingPeerConnector::noteWantWrite() was
about the srvBio->holdWrite() in the line above which still remains
wasn't it?


in src/ssl/PeerConnector.cc:

* "Log connection details, if any"
 - in the function above Ssl::PeerConnector::noteWantRead()

* change in Ssl::PeerConnector::noteWantRead() is not necessary.
 - we leave local variable definitions until the point they are needed.
 - which for that 'fd' is still where it was before.


in src/ssl/bio.cc:

* you dont need to put /* Ssl:Bio */ or /* ServerBio */ markers between
definitions at the file global level.
 - if its not obvious from the function/method definition what they are
something has gone wrong in the design.

* in applyTlsDetailsToSSL() replace:
"
 The SSL_set_ssl_method is not the correct method because it will strict
 SSL version which can be used to the SSL version used for client hello
message.
"
   with:
"
The SSL_set_ssl_method is wrong here because it will restrict the
permitted transport version to be identical to the version used in the
ClientHello message.
"


in src/ssl/bio.h:

* security/Handshake.h makes #include sbuf/SBuf.h no longer needed.

* Bio::rBufData() missing doxygen comment

* ClientBio::setReadBufData() missing doxygen comment

* ClientBio::wrongProtocol is removed
 - erase it from the constructor list too.

* ServerBio::receivedHelloDetails() use \return instead of "Return"

* ServerBio::rbufConsumePos() wrong "///<"
 - doxygen comments placed before the symbol definition use "///"
without the '<'

* "SSL messages" -> "TLS/SSL messages"



Amos