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

Tue Dec 22 20:33:55 UTC 2015

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::<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....
>
>
>>
>
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev