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