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

Wed Dec 23 19:18:55 UTC 2015

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