[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::<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

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

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

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
>
> * receivedHelloFeatures_ says "The futures received".
>   - typo of "features" or are you actually documenting C++ futures
> functionality usage?

..the future which will never come....


>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Log-SSL-Cryptography-Parameters-t7.patch
Type: text/x-patch
Size: 58015 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20151213/c0e5f5ff/attachment-0001.bin>