[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::<negotiated_version
>   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::<received_hello_version
>   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::<received_supported_version
>   The maximum SSL version supported by the the SSL server.
> 
>   %ssl::>cipher
>   The negotiated cipher of the client-to-Squid connection.
> 
>   %ssl::<cipher
>   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 client connection.

  %ssl::<cv - ClientHello message version received on the client connection.

  %ssl::<sv - ServerHello message version sent on the client connection.


  %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:<cv)
  LFT_TLS_CLIENT_LOCAL_HELLO_VERSION (%ssl:<sv)
  LFT_TLS_CLIENT_NEGOTIATED_VERSION  (%ssl:<v)

  LFT_TLS_SERVER_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