Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Christos Tsantilas]

Patch applied to trunk as r14460.
The patches r14461, r14462, r14464 required to fix "make 
distcheck/check" builds. Sorry for this...




On 12/22/2015 10:33 PM, Christos Tsantilas wrote:

If no objections I will apply the last patch to trunk.

On 12/13/2015 10:46 PM, Christos Tsantilas wrote:

On 12/13/2015 11:16 AM, Amos Jeffries wrote:

On 11/12/2015 6:36 a.m., Christos Tsantilas wrote:

This patch adds the following formatting codes:

   %ssl::>negotiated_version
   The SSL version of the client-to-Squid connection.

   %ssl::received_hello_version
   The SSL version of the Hello message received from SSL client

   %ssl::received_supported_version
   The maximum SSL version supported by the the SSL client.

   %ssl::cipher
   The negotiated cipher of the client-to-Squid connection.

   %ssl::

Looks good. But there are some minor issues to resolve.



src/ssl/support.h:
* s/SSL/TLS/ in the new documentation


fixed in my new patch



* Can you please put this new class in libsecurity and the Security::
namespace?


The Security::NegotiationHistory class moved to
security/NegotiationHistory.[cc,h] files



in src/ssl/support.cc:
* since printTlsVersion() is used in format codes that sometimes used
for HTTP headers, please make it output the IETF protocol-version
format. ie. protocol/major.minor


OK.




in src/comm/Connection.cc:
* tlsHistory is only sometime protected by USE_OPENSSL.
  - it should be defined in the .h as a void* for non-OpenSSL builds.
Which can avoid many of the wrappers.
- if you moved the class definitino to libsecurity it should always be
available anyway, so not need the wrappers in this code.


The "#ifdef USE_OPENSSL/#endif" removed from Connection.[cc,h] code.
Maybe we need it around the new methods and members, to same some bytes
per connection object, if the squid did not compiled with SSL/TLS support



in src/comm/Connection.h:
* replace "/** SSL conenction details*/"
   with "/// TLS connection details"


fixed




in src/SquidConfig.h
* logTlsServerHelloDetails should be bool type.


Fixed. It is the first bool type in Config classes.




in src/cf.data.pre:
* s/SSL/TLS/ Squid-4 no longer performs SSL.

ok
However I must note that still accepts SSL hello messages.



* s/client-to-Squid/client/

* s/Squid-to-server/last server or peer/


OK on this.





* please make the codes shorter. We still have to work within a
relatively short line length for the entire log format.


Well, I did not fix it in the new patch. We need the other developers
opinion.

The short names does not improve the speed or something in operation.
However they confuses system admins when trying to read a logformat
strings.
I am suggesting to keep the long names for logformat codes as is.




There also seems to be a lot of confusion over the meaning of "SSL
version" in the documentation.
  - I suggest:

   %ssl::v - Negotiated TLS version on the last server or peer
connection.

   %ssl::>cv - ClientHello message version sent on the last server or
peer connection.

   %ssl::>sv - ServerHello message version received on the last
server or
peer connection.


in src/format/ByteCode.h:
* s/LFT_SSL_/LFT_TLS_/ on the new codes.


This is fixed in new patch.



The codes so far have a bit of a naming convention:

  LFT_(TLS|SSL)_(CLIENT|SERVER)_(LOCAL_)_FOO

- CLIENT for client connection
- SERVER for server connection
- LOCAL_ for Squid details on the connection
- FOO for the FOO detail name


I disagree in this too..
- The client sends a Hello message

- The hello message version may be is SSLv2, or SSLv3 or TLS1.x. Or
better this is the version of the SSL/TLS Record protocol includes the
hello handshake message.

- The client Hello message includes the (maximum) supported TLS version
by the client, which is not always the same as Hello (SSL/TLS record)
message version.

- The server sends back hello server handshake message in an SSL/TLS
record of version X, and which includes the supported TLS version Y by
server.

The CLIENT, SERVER and LOCAL names are not enough to hold the above.
I believe that the current scheme is  better.



Then sorted by tag, so related things are grouped together.

If you could follow that it would help readability.

Which would mean:
   LFT_TLS_CLIENT_HELLO_VERSION   (%ssl:sv)
   LFT_TLS_SERVER_LOCAL_HELLO_VERSION (%ssl:>cv)
   LFT_TLS_SERVER_NEGOTIATED_VERSION  (%ssl:>v)



in src/ssl/bio.cc:
* s/SSL/TLS/ - at least in the new documentation.


ok



* since you are changing the initialization list for
Ssl::Bio::sslFeatures::sslFeatures(), please make the entries one member
per line for easier reading.


ok




* Ssl::Bio::sslFeatures::parseMsgHead()
  - why is the SSL version parse being changed?
  - what was wrong with reporting the actual on-wire value given?

-sslVersion = (head[3] << 8) | head[4];
+

Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Christos Tsantilas]

If no objections I will apply the last patch to trunk.

On 12/13/2015 10:46 PM, Christos Tsantilas wrote:

On 12/13/2015 11:16 AM, Amos Jeffries wrote:

On 11/12/2015 6:36 a.m., Christos Tsantilas wrote:

This patch adds the following formatting codes:

   %ssl::>negotiated_version
   The SSL version of the client-to-Squid connection.

   %ssl::received_hello_version
   The SSL version of the Hello message received from SSL client

   %ssl::received_supported_version
   The maximum SSL version supported by the the SSL client.

   %ssl::cipher
   The negotiated cipher of the client-to-Squid connection.

   %ssl::

Looks good. But there are some minor issues to resolve.



src/ssl/support.h:
* s/SSL/TLS/ in the new documentation


fixed in my new patch



* Can you please put this new class in libsecurity and the Security::
namespace?


The Security::NegotiationHistory class moved to
security/NegotiationHistory.[cc,h] files



in src/ssl/support.cc:
* since printTlsVersion() is used in format codes that sometimes used
for HTTP headers, please make it output the IETF protocol-version
format. ie. protocol/major.minor


OK.




in src/comm/Connection.cc:
* tlsHistory is only sometime protected by USE_OPENSSL.
  - it should be defined in the .h as a void* for non-OpenSSL builds.
Which can avoid many of the wrappers.
- if you moved the class definitino to libsecurity it should always be
available anyway, so not need the wrappers in this code.


The "#ifdef USE_OPENSSL/#endif" removed from Connection.[cc,h] code.
Maybe we need it around the new methods and members, to same some bytes
per connection object, if the squid did not compiled with SSL/TLS support



in src/comm/Connection.h:
* replace "/** SSL conenction details*/"
   with "/// TLS connection details"


fixed




in src/SquidConfig.h
* logTlsServerHelloDetails should be bool type.


Fixed. It is the first bool type in Config classes.




in src/cf.data.pre:
* s/SSL/TLS/ Squid-4 no longer performs SSL.

ok
However I must note that still accepts SSL hello messages.



* s/client-to-Squid/client/

* s/Squid-to-server/last server or peer/


OK on this.





* please make the codes shorter. We still have to work within a
relatively short line length for the entire log format.


Well, I did not fix it in the new patch. We need the other developers
opinion.

The short names does not improve the speed or something in operation.
However they confuses system admins when trying to read a logformat
strings.
I am suggesting to keep the long names for logformat codes as is.




There also seems to be a lot of confusion over the meaning of "SSL
version" in the documentation.
  - I suggest:

   %ssl::v - Negotiated TLS version on the last server or peer
connection.

   %ssl::>cv - ClientHello message version sent on the last server or
peer connection.

   %ssl::>sv - ServerHello message version received on the last server or
peer connection.


in src/format/ByteCode.h:
* s/LFT_SSL_/LFT_TLS_/ on the new codes.


This is fixed in new patch.



The codes so far have a bit of a naming convention:

  LFT_(TLS|SSL)_(CLIENT|SERVER)_(LOCAL_)_FOO

- CLIENT for client connection
- SERVER for server connection
- LOCAL_ for Squid details on the connection
- FOO for the FOO detail name


I disagree in this too..
- The client sends a Hello message

- The hello message version may be is SSLv2, or SSLv3 or TLS1.x. Or
better this is the version of the SSL/TLS Record protocol includes the
hello handshake message.

- The client Hello message includes the (maximum) supported TLS version
by the client, which is not always the same as Hello (SSL/TLS record)
message version.

- The server sends back hello server handshake message in an SSL/TLS
record of version X, and which includes the supported TLS version Y by
server.

The CLIENT, SERVER and LOCAL names are not enough to hold the above.
I believe that the current scheme is  better.



Then sorted by tag, so related things are grouped together.

If you could follow that it would help readability.

Which would mean:
   LFT_TLS_CLIENT_HELLO_VERSION   (%ssl:sv)
   LFT_TLS_SERVER_LOCAL_HELLO_VERSION (%ssl:>cv)
   LFT_TLS_SERVER_NEGOTIATED_VERSION  (%ssl:>v)



in src/ssl/bio.cc:
* s/SSL/TLS/ - at least in the new documentation.


ok



* since you are changing the initialization list for
Ssl::Bio::sslFeatures::sslFeatures(), please make the entries one member
per line for easier reading.


ok




* Ssl::Bio::sslFeatures::parseMsgHead()
  - why is the SSL version parse being changed?
  - what was wrong with reporting the actual on-wire value given?

-sslVersion = (head[3] << 8) | head[4];
+sslHelloVersion = 0x0002;


This is code refers to a Hello message sent into an SSLv23 SSL record.
The maximum supported version (in head[3] and head[4] bytes) will be
extracted latter in 

Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Eliezer Croitoru]

On 22/12/2015 22:33, Christos Tsantilas wrote:

If no objections I will apply the last patch to trunk.

+1 from me on long names rather then short and cryptic ones.

Eliezer
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Amos Jeffries]

On 13/12/2015 10:16 p.m., Amos Jeffries wrote:
> * please make the codes shorter. We still have to work within a
> relatively short line length for the entire log format.
> 
> There also seems to be a lot of confusion over the meaning of "SSL
> version" in the documentation.
>  - I suggest:
> 
>   %ssl:: 
>   %ssl:: 
>   %ssl:: 
> 
>   %ssl::>v - Negotiated TLS version on the last server or peer connection.
> 
>   %ssl::>cv - ClientHello message version sent on the last server or
> peer connection.
> 
>   %ssl::>sv - ServerHello message version received on the last server or
> peer connection.
> 

Oop. I got those > and < around the wrong way. > is client connection, <
is server connection.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Amos Jeffries]

On 11/12/2015 6:36 a.m., Christos Tsantilas wrote:
> This patch adds the following formatting codes:
> 
>   %ssl::>negotiated_version
>   The SSL version of the client-to-Squid connection.
> 
>   %ssl::   The SSL version of the Squid-to-server connection.
> 
>   %ssl::>received_hello_version
>   The SSL version of the Hello message received from SSL client
> 
>   %ssl::   The SSL version of the Hello message received from SSL server.
> 
>   %ssl::>received_supported_version
>   The maximum SSL version supported by the the SSL client.
> 
>   %ssl::   The maximum SSL version supported by the the SSL server.
> 
>   %ssl::>cipher
>   The negotiated cipher of the client-to-Squid connection.
> 
>   %ssl::   The negotiated cipher of the Squid-to-server connection.
> 
> These are useful for statistics collection, security reviews, and
> reviews prior to adjusting the list of the allowed SSL protocols and
> ciphers.
> 
> This is a Measurement Factory project
> 

Looks good. But there are some minor issues to resolve.



src/ssl/support.h:
* s/SSL/TLS/ in the new documentation

* Can you please put this new class in libsecurity and the Security::
namespace?

in src/ssl/support.cc:
* since printTlsVersion() is used in format codes that sometimes used
for HTTP headers, please make it output the IETF protocol-version
format. ie. protocol/major.minor


in src/comm/Connection.cc:
* tlsHistory is only sometime protected by USE_OPENSSL.
 - it should be defined in the .h as a void* for non-OpenSSL builds.
Which can avoid many of the wrappers.
- if you moved the class definitino to libsecurity it should always be
available anyway, so not need the wrappers in this code.

in src/comm/Connection.h:
* replace "/** SSL conenction details*/"
  with "/// TLS connection details"


in src/SquidConfig.h
* logTlsServerHelloDetails should be bool type.


in src/cf.data.pre:
* s/SSL/TLS/ Squid-4 no longer performs SSL.

* s/client-to-Squid/client/

* s/Squid-to-server/last server or peer/

* please make the codes shorter. We still have to work within a
relatively short line length for the entire log format.

There also seems to be a lot of confusion over the meaning of "SSL
version" in the documentation.
 - I suggest:

  %ssl::v - Negotiated TLS version on the last server or peer connection.

  %ssl::>cv - ClientHello message version sent on the last server or
peer connection.

  %ssl::>sv - ServerHello message version received on the last server or
peer connection.


in src/format/ByteCode.h:
* s/LFT_SSL_/LFT_TLS_/ on the new codes.

The codes so far have a bit of a naming convention:

 LFT_(TLS|SSL)_(CLIENT|SERVER)_(LOCAL_)_FOO

- CLIENT for client connection
- SERVER for server connection
- LOCAL_ for Squid details on the connection
- FOO for the FOO detail name

Then sorted by tag, so related things are grouped together.

If you could follow that it would help readability.

Which would mean:
  LFT_TLS_CLIENT_HELLO_VERSION   (%ssl:sv)
  LFT_TLS_SERVER_LOCAL_HELLO_VERSION (%ssl:>cv)
  LFT_TLS_SERVER_NEGOTIATED_VERSION  (%ssl:>v)



in src/ssl/bio.cc:
* s/SSL/TLS/ - at least in the new documentation.

* since you are changing the initialization list for
Ssl::Bio::sslFeatures::sslFeatures(), please make the entries one member
per line for easier reading.


* Ssl::Bio::sslFeatures::parseMsgHead()
 - why is the SSL version parse being changed?
 - what was wrong with reporting the actual on-wire value given?

-sslVersion = (head[3] << 8) | head[4];
+sslHelloVersion = 0x0002;


in src/ssl/bio.h:
* s/SSL/TLS/ - at least in the new documentation.

* receivedHelloFeatures_ says "The futures received".
 - typo of "features" or are you actually documenting C++ futures
functionality usage?


Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Kinkie]

I would prefer more telling (aka long) codes, especially for less common tokens.

On Sun, Dec 13, 2015 at 10:27 PM, Alex Rousskov
 wrote:
> On 12/13/2015 01:46 PM, Christos Tsantilas wrote:
>> On 12/13/2015 11:16 AM, Amos Jeffries wrote:
>>> * please make the codes shorter. We still have to work within a
>>> relatively short line length for the entire log format.
>
>
>> Well, I did not fix it in the new patch. We need the other developers
>> opinion.
>>
>> The short names does not improve the speed or something in operation.
>> However they confuses system admins when trying to read a logformat
>> strings.
>> I am suggesting to keep the long names for logformat codes as is.
>
>
> I agree with Christos: Short codes are best for computers and, arguably,
> frequently-used command-line options typed by humans. Long names offer
> better UX in most other cases and should be the default approach for new
> logformat %codes.
>
>
> If Squid cannot support long [wrapped] directives, let's discuss and
> remove that limitation instead of trying harder and harder to cramp a
> growing list of various options into a too-short internal buffer. We
> ought to eventually support something similar to this example:
>
>   logformat myFormat \
> %ts.%03tu \ # record timestamp [seconds.milliseconds]
> %6tr \ # response time; TODO: Ask George about units!
> %>a \ # XXX: our script does not handle IPv6 addresses yet
> %ssl::>cert_subject \ # TODO: Should we log the issuer?
> ... many more %codes, including the newer long ones ...
>
> However, improving handling of long [wrapped] lines is a separate issue,
> with its own discussion points (like supporting the combination of a
> comment and line continuation). The proposed feature will work fine
> without those improvements, even if it makes those improvements even
> more desirable.
>
>
> Thank you,
>
> Alex.
>
> ___
> squid-dev mailing list
> squid-dev@lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev



-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Alex Rousskov]

On 12/13/2015 01:46 PM, Christos Tsantilas wrote:
> On 12/13/2015 11:16 AM, Amos Jeffries wrote:
>> * please make the codes shorter. We still have to work within a
>> relatively short line length for the entire log format.


> Well, I did not fix it in the new patch. We need the other developers
> opinion.
> 
> The short names does not improve the speed or something in operation.
> However they confuses system admins when trying to read a logformat
> strings.
> I am suggesting to keep the long names for logformat codes as is.


I agree with Christos: Short codes are best for computers and, arguably,
frequently-used command-line options typed by humans. Long names offer
better UX in most other cases and should be the default approach for new
logformat %codes.


If Squid cannot support long [wrapped] directives, let's discuss and
remove that limitation instead of trying harder and harder to cramp a
growing list of various options into a too-short internal buffer. We
ought to eventually support something similar to this example:

  logformat myFormat \
%ts.%03tu \ # record timestamp [seconds.milliseconds]
%6tr \ # response time; TODO: Ask George about units!
%>a \ # XXX: our script does not handle IPv6 addresses yet
%ssl::>cert_subject \ # TODO: Should we log the issuer?
... many more %codes, including the newer long ones ...

However, improving handling of long [wrapped] lines is a separate issue,
with its own discussion points (like supporting the combination of a
comment and line continuation). The proposed feature will work fine
without those improvements, even if it makes those improvements even
more desirable.


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Christos Tsantilas]

On 12/13/2015 11:16 AM, Amos Jeffries wrote:

On 11/12/2015 6:36 a.m., Christos Tsantilas wrote:

This patch adds the following formatting codes:

   %ssl::>negotiated_version
   The SSL version of the client-to-Squid connection.

   %ssl::received_hello_version
   The SSL version of the Hello message received from SSL client

   %ssl::received_supported_version
   The maximum SSL version supported by the the SSL client.

   %ssl::cipher
   The negotiated cipher of the client-to-Squid connection.

   %ssl::

Looks good. But there are some minor issues to resolve.



src/ssl/support.h:
* s/SSL/TLS/ in the new documentation


fixed in my new patch



* Can you please put this new class in libsecurity and the Security::
namespace?


The Security::NegotiationHistory class moved to 
security/NegotiationHistory.[cc,h] files




in src/ssl/support.cc:
* since printTlsVersion() is used in format codes that sometimes used
for HTTP headers, please make it output the IETF protocol-version
format. ie. protocol/major.minor


OK.




in src/comm/Connection.cc:
* tlsHistory is only sometime protected by USE_OPENSSL.
  - it should be defined in the .h as a void* for non-OpenSSL builds.
Which can avoid many of the wrappers.
- if you moved the class definitino to libsecurity it should always be
available anyway, so not need the wrappers in this code.


The "#ifdef USE_OPENSSL/#endif" removed from Connection.[cc,h] code.
Maybe we need it around the new methods and members, to same some bytes 
per connection object, if the squid did not compiled with SSL/TLS support




in src/comm/Connection.h:
* replace "/** SSL conenction details*/"
   with "/// TLS connection details"


fixed




in src/SquidConfig.h
* logTlsServerHelloDetails should be bool type.


Fixed. It is the first bool type in Config classes.




in src/cf.data.pre:
* s/SSL/TLS/ Squid-4 no longer performs SSL.

ok
However I must note that still accepts SSL hello messages.



* s/client-to-Squid/client/

* s/Squid-to-server/last server or peer/


OK on this.





* please make the codes shorter. We still have to work within a
relatively short line length for the entire log format.


Well, I did not fix it in the new patch. We need the other developers 
opinion.


The short names does not improve the speed or something in operation.
However they confuses system admins when trying to read a logformat strings.
I am suggesting to keep the long names for logformat codes as is.




There also seems to be a lot of confusion over the meaning of "SSL
version" in the documentation.
  - I suggest:

   %ssl::v - Negotiated TLS version on the last server or peer connection.

   %ssl::>cv - ClientHello message version sent on the last server or
peer connection.

   %ssl::>sv - ServerHello message version received on the last server or
peer connection.


in src/format/ByteCode.h:
* s/LFT_SSL_/LFT_TLS_/ on the new codes.


This is fixed in new patch.



The codes so far have a bit of a naming convention:

  LFT_(TLS|SSL)_(CLIENT|SERVER)_(LOCAL_)_FOO

- CLIENT for client connection
- SERVER for server connection
- LOCAL_ for Squid details on the connection
- FOO for the FOO detail name


I disagree in this too..
- The client sends a Hello message

- The hello message version may be is SSLv2, or SSLv3 or TLS1.x. Or 
better this is the version of the SSL/TLS Record protocol includes the 
hello handshake message.


- The client Hello message includes the (maximum) supported TLS version 
by the client, which is not always the same as Hello (SSL/TLS record) 
message version.


- The server sends back hello server handshake message in an SSL/TLS 
record of version X, and which includes the supported TLS version Y by 
server.


The CLIENT, SERVER and LOCAL names are not enough to hold the above.
I believe that the current scheme is  better.



Then sorted by tag, so related things are grouped together.

If you could follow that it would help readability.

Which would mean:
   LFT_TLS_CLIENT_HELLO_VERSION   (%ssl:sv)
   LFT_TLS_SERVER_LOCAL_HELLO_VERSION (%ssl:>cv)
   LFT_TLS_SERVER_NEGOTIATED_VERSION  (%ssl:>v)



in src/ssl/bio.cc:
* s/SSL/TLS/ - at least in the new documentation.


ok



* since you are changing the initialization list for
Ssl::Bio::sslFeatures::sslFeatures(), please make the entries one member
per line for easier reading.


ok




* Ssl::Bio::sslFeatures::parseMsgHead()
  - why is the SSL version parse being changed?
  - what was wrong with reporting the actual on-wire value given?

-sslVersion = (head[3] << 8) | head[4];
+sslHelloVersion = 0x0002;


This is code refers to a Hello message sent into an SSLv23 SSL record.
The maximum supported version (in head[3] and head[4] bytes) will be 
extracted latter in parseV23message.





in src/ssl/bio.h:
* s/SSL/TLS/ - at least in the new documentation.


ok

Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Francesco Chemolli]

On 10 Dec 2015, at 18:36, Christos Tsantilas  wrote:

> This patch adds the following formatting codes:

LGTM. +1.

Kinkie
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] [PATCH] Log SSL Cryptography Parameters

[Christos Tsantilas]

This patch adds the following formatting codes:

  %ssl::>negotiated_version
  The SSL version of the client-to-Squid connection.

  %ssl::received_hello_version
  The SSL version of the Hello message received from SSL client

  %ssl::received_supported_version
  The maximum SSL version supported by the the SSL client.

  %ssl::cipher
  The negotiated cipher of the client-to-Squid connection.

  %ssl::These are useful for statistics collection, security reviews, and 
reviews prior to adjusting the list of the allowed SSL protocols and 
ciphers.


This is a Measurement Factory project
Log SSL Cryptography Parameters

This patch adds the following formatting codes:
  %ssl::>negotiated_version  The SSL version of the client-to-Squid connection.
  %ssl::received_hello_version The SSL version of the Hello message received
from SSL client
  %ssl::received_supported_version The maximum SSL version supported by the
the SSL client.
  %ssl::cipher   The negotiated cipher of the client-to-Squid connection.
  %ssl::cert_issuer
 The Issuer field of the received client
 SSL certificate or a dash ('-') if Squid has
 received an invalid/malformed certificate or
 no certificate at all. Consider encoding the
 logged value because Issuer often has spaces.
 
 		ssl::negotiated_version The negotiated SSL version of the
+client-to-Squid connection.
+
+		%ssl::received_hello_version The SSL version of the Hello
+message received from SSL client.
+
+		%ssl::received_supported_version The maximum SSL version
+supported by the SSL client.
+
+		%ssl::negotiated_cipher The negotiated cipher of the
+client-to-Squid connection.
+
+		%ssl::clientConnection->tlsNegotiations()->fillWith(ssl);
 
 client_cert = SSL_get_peer_certificate(ssl);
 
 if (client_cert != NULL) {
 debugs(83, 3, "clientNegotiateSSL: FD " << fd <<
" client certificate: subject: " <<
X509_NAME_oneline(X509_get_subject_name(client_cert), 0, 0));
 
 debugs(83, 3, "clientNegotiateSSL: FD " << fd <<
" client certificate: issuer: " <<
X509_NAME_oneline(X509_get_issuer_name(client_cert), 0, 0));
 
 X509_free(client_cert);
 } else {
 debugs(83, 5, "clientNegotiateSSL: FD " << fd <<
" has no certificate.");
 }
 
 #if defined(TLSEXT_NAMETYPE_host_name)
 if (!conn->serverBump()) {
@@ -3874,41 +3874,41 @@
 
 int ret = 0;
 if ((ret = Squid_SSL_accept(conn, clientPeekAndSpliceSSL)) < 0)
 debugs(83, 2, "SSL_accept failed.");
 
 BIO *b = SSL_get_rbio(ssl);
 assert(b);
 Ssl::ClientBio *bio = static_cast(b->ptr);
 if (ret < 0) {
 const err_type err = bio->noSslClient() ? ERR_PROTOCOL_UNKNOWN : ERR_SECURE_ACCEPT_FAIL;
 if (!conn->spliceOnError(err))
 conn->clientConnection->close();
 return;
 }
 
 if (bio->rBufData().contentSize() > 0)
 conn->receivedFirstByte();
 
 if (bio->gotHello()) {
 if (conn->serverBump()) {
-Ssl::Bio::sslFeatures const  = bio->getFeatures();
+Ssl::Bio::sslFeatures const  = bio->receivedHelloFeatures();
 if (!features.serverName.isEmpty()) {
 conn->serverBump()->clientSni = features.serverName;
 conn->resetSslCommonName(features.serverName.c_str());
 }
 }
 
 debugs(83, 5, "I got hello. Start forwarding the request!!! ");
 Comm::SetSelect(fd, COMM_SELECT_READ, NULL, NULL, 0);
 Comm::SetSelect(fd, COMM_SELECT_WRITE, NULL, NULL, 0);
 conn->startPeekAndSpliceDone();
 return;
 }
 }
 
 void ConnStateData::startPeekAndSplice()
 {
 // will call httpsPeeked() with certificate and connection, eventually
 auto unConfiguredCTX = Ssl::createSSLContext(port->signingCert, port->signPkey, *port);
 fd_table[clientConnection->fd].dynamicSslContext = unConfiguredCTX;
 
@@ -3951,40 +3951,44 @@
 bumpAction = (Ssl::BumpMode)answer.kind;
 } else
 bumpAction = Ssl::bumpSplice;
 
 connState->serverBump()->act.step2 = bumpAction;
 connState->sslBumpMode = bumpAction;
 
 if (bumpAction == Ssl::bumpTerminate) {
 connState->clientConnection->close();
 } else if (bumpAction != Ssl::bumpSplice) {
 connState->startPeekAndSpliceDone();
 } else
 connState->splice();
 }
 
 void
 ConnStateData::splice()
 {
 //Normally we can splice here, because we just got client hello message
 auto ssl = fd_table[clientConnection->fd].ssl;
+
+//retrieve received SSL client information
+clientConnection->tlsNegotiations()->fillWith(ssl);
+
 BIO *b = SSL_get_rbio(ssl);
 Ssl::ClientBio *bio = static_cast(b->ptr);
 MemBuf const  = bio->rBufData();
 debugs(83,5, "Bio for  " << clientConnection << " read " << rbuf.contentSize() << " helo bytes");
 //