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