Add histogram for key exchange strength

net/socket/ssl_client_socket_pool.cc

Index: net/socket/ssl_client_socket_pool.cc
diff --git a/net/socket/ssl_client_socket_pool.cc b/net/socket/ssl_client_socket_pool.cc
index a80df57d0be4a8d084e62c5d09f1040ee458c962..d06d96d9d54394d619d27a3e245179ee43f86c15 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -21,6 +21,7 @@
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/transport_client_socket_pool.h"
 #include "net/ssl/ssl_cert_request_info.h"
+#include "net/ssl/ssl_cipher_suite_names.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "net/ssl/ssl_info.h"
 
@@ -371,9 +372,29 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
                                                     ssl_info.connection_status),
                               SSL_CONNECTION_VERSION_MAX);
 
-    UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_CipherSuite",
-                                SSLConnectionStatusToCipherSuite(
-                                    ssl_info.connection_status));
+    uint16 cipher_suite =
+        SSLConnectionStatusToCipherSuite(ssl_info.connection_status);
+    UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_CipherSuite", cipher_suite);
+
+    const char *str, *cipher_str, *mac_str;
+    bool is_aead;
+    SSLCipherSuiteToStrings(&str, &cipher_str, &mac_str, &is_aead,
+                            cipher_suite);
+    // UMA_HISTOGRAM_... macros cache the Histogram instance and thus only work
+    // if the histogram name is constant, so don't generate it dynamically.
+    if (strncmp(str, "RSA", 3) == 0) {

[davidben, 2015/09/15 14:35:35]

I think you an exact match (strcmp) here.

[sigbjorn, 2015/09/15 15:35:05]

Right now all comparisons use strncmp against the first part of the name only. I
find this easy to relate to, without needing to look up the details of the
names.

I can use strcmp here, and include the underscores below, the code might be more
accurate, but presumably also more confusing. E.g. "RSA_EXPORT" is still a valid
cipher name, so one would have to do quite some digging to find out that all RSA
ciphers are included if doing strcmp. Anyone hunting down a bug (from elsewhere)
might suspect the discrepancies between the ciphers here instead.

I have no strong opinion though, will change both here and below if you prefer
:)

[davidben, 2015/09/15 15:41:54]

We do not support RSA_EXPORT and I can quite confidently say we never will. :-P
This logic is already making assumptions about what cipher suites we support due
to it omitting some of the crazier things like ECDH_RSA and ECDH_anon.

In so far as parsing the names is sound at all, I think it's better to include
the separator and/or end-of-string in the comparison, so that you're actually
looking at an individual component.

For instance, what if ECDHE happened to be called "DHECE" for Diffie-Hellman,
Elliptic Curve, Ephemeral (kind of a weird acronym, but we did already end up
with one modifier at the end)? Then this code would be broken.

[sigbjorn, 2015/09/15 16:30:25]

Done.

+      UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_KeyExchange.RSA",
+                                  ssl_info.key_exchange_info);
+    } else if (strncmp(str, "DHE", 3) == 0) {

[davidben, 2015/09/15 14:35:35]

Nit: I'd do DHE_ and ECDHE_ to make sure you get the separator.

[sigbjorn, 2015/09/15 16:30:25]

Done.

+      UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_KeyExchange.DHE",
+                                  ssl_info.key_exchange_info);
+    } else if (strncmp(str, "ECDHE", 5) == 0) {
+      UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_KeyExchange.ECDHE",
+                                  ssl_info.key_exchange_info);
+    } else {
+      UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_KeyExchange.Unknown",
+                                  ssl_info.key_exchange_info);
+    }
 
     if (ssl_info.handshake_type == SSLInfo::HANDSHAKE_RESUME) {
       UMA_HISTOGRAM_CUSTOM_TIMES("Net.SSL_Connection_Latency_Resume_Handshake",