Expose TLS settings in the Security panel overview, and call out individual obsolete settings.

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index b9d1732cd8684228650c2df23a39ddfc535cf2d2..1f95e634a389a5be60f3d80b8eea2485720981e7 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -9,6 +9,7 @@
 #include <algorithm>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/base_paths.h"
 #include "base/bind.h"
@@ -20,6 +21,7 @@
 #include "base/process/process_info.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -209,6 +211,8 @@
 #include "net/base/filename_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/cookie_monster.h"
+#include "net/ssl/ssl_cipher_suite_names.h"
+#include "net/ssl/ssl_connection_status_flags.h"
 #include "net/url_request/url_request_context.h"
 #include "third_party/WebKit/public/web/WebWindowFeatures.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -1402,12 +1406,96 @@ content::SecurityStyle Browser::GetSecurityStyle(
     }
   }
 
-  if (security_info.is_secure_protocol_and_ciphersuite) {
+  int ssl_version =
+      net::SSLConnectionStatusToVersion(security_info.connection_status);
+  const char* protocol;
+  net::SSLVersionToString(&protocol, ssl_version);
+
+  const char* key_exchange;
+  const char* cipher;
+  const char* mac;
+  bool is_aead;
+  uint16_t cipher_suite =
+      net::SSLConnectionStatusToCipherSuite(security_info.connection_status);
+  net::SSLCipherSuiteToStrings(&key_exchange, &cipher, &mac, &is_aead,
+                               cipher_suite);
+
+  base::string16 protocol_name = base::ASCIIToUTF16(protocol);
+  base::string16 key_exchange_name = base::ASCIIToUTF16(key_exchange);
+  const base::string16 cipher_suite_name =

[estark, 2016/04/18 11:46:44]

Why do these all need to be string16s?

[lgarron, 2016/04/25 23:56:53]

GetStringFUTF8() as well as GetStringFUTF16() take `string16`s. In particular, I
have so many arguments that I have to pass a vector of replacements rather than
positional arguments, and we only have the GetStringFUTF16() variant of the
function that takes a vector [1] Ergo, I have to construct a
std::vector<base::string16>.
I didn't feel that this warranted a comment; do you think it would worth adding
one?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_...

+      (mac == NULL) ? base::ASCIIToUTF16(cipher)
+                    : l10n_util::GetStringFUTF16(IDS_CIPHERSUITE_WITH_MAC,
+                                                 base::ASCIIToUTF16(cipher),
+                                                 base::ASCIIToUTF16(mac));
+
+  if (security_info.obsolete_ssl_status == net::OBSOLETE_SSL_NONE) {
     security_style_explanations->secure_explanations.push_back(
         content::SecurityStyleExplanation(
             l10n_util::GetStringUTF8(IDS_SECURE_PROTOCOL_AND_CIPHERSUITE),
-            l10n_util::GetStringUTF8(
-                IDS_SECURE_PROTOCOL_AND_CIPHERSUITE_DESCRIPTION)));
+            l10n_util::GetStringFUTF8(
+                IDS_SECURE_PROTOCOL_AND_CIPHERSUITE_DESCRIPTION, protocol_name,
+                key_exchange_name, cipher_suite_name)));
+  } else if (security_info.cert_id != 0) {

[estark, 2016/04/18 11:46:44]

Can you please explain this check in a comment? It doesn't look like we check
for a 0 cert_id elsewhere, so I'm wondering why we need to here.

[lgarron, 2016/04/25 23:56:53]

The reason is net errors. If we don't include this check, visiting
dh480.badssl.com will result in:
"The connection to this site uses an obsolete protocol (???), an obsolete key
exchange (NULL), and an obsolete cipher suite (NULL with NULL)."

I've added a comment:

+    std::vector<base::string16> summary_replacements;
+    std::vector<base::string16> description_replacements;
+

[estark, 2016/04/18 11:46:44]

Hmmmm, reading this constructing-the-string code hurts my brain. It would
probably help if you separated it out into a helper function. Other optional
suggestions for possible simplifications:

1.) Are you sure we need the summary string to be so descriptive? Could it just
be "Obsolete TLS settings" or "Secure TLS connection", rather than actually
specifying the obsolete settings in the summary?

2.) Maybe factor out yet another helper function for the repetitive code in
lines 1442-1451, 1453-1463, and 1465-1474. Or, this totally might be personal
preference, but I think I might find this easier to read if you defined more
strings, and had OBSOLETE_PROTOCOL, MODERN_PROTOCOL, OBSOLETE_KEY_EXCHANGE,
MODERN_KEY_EXCHANGE, OBSOLETE_CIPHER, MODERN_CIPHER, instead of combining
obsolete/modern with protocol/key exchange/cipher for each one.

[lgarron, 2016/04/25 23:56:53]

I've simplified things considerably, taking your string change suggestions from
1. and 2.
I could still factor it out, but I think it's not bad right now.

+    if (security_info.obsolete_ssl_status & net::OBSOLETE_SSL_MASK_PROTOCOL) {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_AN_OBSOLETE));
+      summary_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_PROTOCOL));
+    } else {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_A_MODERN));
+    }
+    description_replacements.push_back(protocol_name);
+
+    if (security_info.obsolete_ssl_status &
+        net::OBSOLETE_SSL_MASK_KEY_EXCHANGE) {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_AN_OBSOLETE));
+      summary_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_KEY_EXCHANGE));
+    } else {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_A_MODERN));
+    }
+    description_replacements.push_back(key_exchange_name);
+
+    if (security_info.obsolete_ssl_status & net::OBSOLETE_SSL_MASK_CIPHER) {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_AN_OBSOLETE));
+      summary_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_CIPHER_SUITE));
+    } else {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_A_MODERN));
+    }
+    description_replacements.push_back(cipher_suite_name);
+
+    int summary_message_id;
+    switch (summary_replacements.size()) {
+      case 1:
+        summary_message_id = IDS_SINGLE_OBSOLETE_TLS_SETTING;
+        break;
+      case 2:
+        summary_message_id = IDS_TWO_OBSOLETE_TLS_SETTINGS;
+        break;
+      case 3:
+        summary_message_id = IDS_THREE_OBSOLETE_TLS_SETTINGS;
+        break;
+      default:
+        summary_message_id = 0;
+        NOTREACHED();
+    }
+
+    security_style_explanations->info_explanations.push_back(
+        content::SecurityStyleExplanation(
+            base::UTF16ToUTF8(l10n_util::GetStringFUTF16(
+                summary_message_id, summary_replacements, nullptr)),
+            base::UTF16ToUTF8(l10n_util::GetStringFUTF16(
+                IDS_OBSOLETE_SSL_DESCRIPTION, description_replacements,
+                nullptr))));
   }
 
   return security_style;