Re: [PATCH] SSL Peek and Splice

On 13/08/2014 11:20 p.m., Tsantilas Christos wrote:
> Hi all,
>
> This is a first patch which implements the Peek-and-Splice feature
> described in wiki:
> http://wiki.squid-cache.org/Features/SslPeekAndSplice
>
> The goal of this patch is to make SSL bumping decision after the origin
> server name is known.
>
> Short description
> ====================
>
> Peek and Splice peeks at the SSL client Hello message and SNI info if
> any (bumping step 1), sends identical or a similar Hello message to the
> SSL server and peeks at the SSL server Hello message (bumping step 2),
> and finally decides to proceed with splicing or bumping the connection
> (bumping step 3).
>
> After the step 1 bumping step completes the SNI information is available
> and after the step 2 bumping step completes the server certificate is
> available.
>
> The ssl_bump access list evaluated on every bumping step to select the
> bumping mode to use. The new acl "at_step" can be used to match the
> current bumping step.
>
> In most cases:
> - if the user select "peek" bumping mode at step2 then at step3 can select
> one of the "splice" or "terminate" modes.
> - If the user select "stare" bumping mode at step2 then at step 3 can
> select
> one of the "bump" or "terminate" modes.
>
> If the squid built with the SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK and
> the client uses openSSL library similar to the library used by squid
> then bumping is possible after "peek" bumping mode selection and
> "splice" after "stare" bumping mode selection.
>
> The bump, terminate and splice are final decisions.
>
> Example configurations:
>
> acl step1 at_step SslBump1
> acl step2 at_step SslBump2
> acl step3 at_step SslBump3
>
> ssl_bump peek step1 all
> ssl_bump splice step2 BANKS
> ssl_bump peek step2 all
> ssl_bump terminate step3 BLACKLIST
> ssl_bump splice step3 all
>
> This is a Measurement Factory project

in configure.ac:
- please perform all the AC_CHECK_HEADERS for openssl/*.h inside the
--with-openssl block. Some of them are already being checked there.

in acl/AtBumpStep .h and .cc
 - please wrap the file contents with USE_OPENSSL internally.
 - please also rename either the files or classes according to the
coding guidelines about filename matching the class name. Classes seem
to be "AtStep" instead of "AtBumpStep".

in src/acl/AtBumpStepData.h
- missing gap between "" and <> include lists. This also happens elsewhere.

in src/acl/AtBumpStep.h:
 - this does not need to be doxygen comment, nor so big:
+ /**
+ * Not implemented to prevent copies of the instance.
+ */

in src/client_side.cc:
- "debugs(33, 5, "httpsCreate: " shuffled but not cleaned up. Should
have the function name prefix removed.

- httpsSslBumpStep2AccessCheckDone() debugs message "Bio for " please
display the whole connection detail, or at least prefix the fd with "FD ".
  - similar problems in clientPeekAndSpliceSSL() fd debugs output.
  - later on in src/ssl/PeerConnector.cc you are using lower case "fd "
to prefix the FD. The log grep pattern is "FD n" where n is the numeric
FD number.

in src/format/Token.cc:
- please add a reservation for the ssl::<cert_subject and
ssl::<cert_issuer tokens. We do that with a /* */ comment block in the
entry where they will be when implemented.

in ssl/bio.cc:
- several "#ifdef DO_SSLV23" should be "#if defined(DO_SSLV23)"
 - there are also other #ifdef uses that need fixing to defined()
 - and several #ifndef to become #if !defined()

in src/ssl/bio.h:
- <string> include does not need HAVE_STRING
- is there a particular reason you are using std::string for members of
class sslFeatures instead of SBuf ?
- please use mb_size_t for class ServerBio::helloMsgSize. It may be
capable of holding 64-bit sizes even when int is only 32-bit.

in src/tunnel.cc:
- switchToTunnel()
"+ tunnelState->server.size_ptr = NULL;//????" should probably be set
to one of the ConnStateData::al.http.clientReplySz counters.

Also,
 - please do make fd.h (not fde.h) export its
default_read_method/default_write_method functions instead of
re-defining them all over the place.

+1. Thats all cosmetic. So I think this can go in after fixing.

Amos
Received on Tue Aug 19 2014 - 11:58:08 MDT