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

Reply via email to