Re: [PATCH] SSL Peek and Splice

I am attaching a new patch.
I hope this patch fixes most of the issues.

On 08/19/2014 02:57 PM, Amos Jeffries wrote:
> On 13/08/2014 11:20 p.m., Tsantilas Christos wrote:
>
> 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.
fixed
>
>
> 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".

fixed

>
>
> in src/acl/AtBumpStepData.h
> - missing gap between "" and <> include lists. This also happens elsewhere.
>
I tried to fix all of these.

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

yep

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

I hope it is ok in this patch.

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

ok.

>
> 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 the attached patch "#if [!]defined()" used in all of the cases.

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

I do not remember, but probably when the development for this project
started the SBuf was not investigated, so the std::string used instead.
It is not easy to convert all of the std::strings to SBuf (because of
missing SBuf features), however in the attached patch the SBuf used for
cases the SBuf helps or perform better
(Ssl::Bio::sslFeatures::serverName,
Ssl::Bio::sslFeatures::HelloMesssage, Ssl::ServerBio::helloMsg members)

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

ok

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

Now set to &ClientSocketContext->http->out.size if the
ClientSocketContext is available.

I hope it is OK (however probably we should use a different method to
keep statistics in TunnelStateData class, than pointers to
AccessLogEntry members....)

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

ok

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

If there is not any objection I will apply this patch to trunk.

Regards,
     Christos

>
> Amos
>