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

chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc

Index: chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
diff --git a/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc b/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
index c32738337731383b6506a0c5cbd0bfaf945c8ffc..d494a0a3d4e555dd1f125f7d2f20515f0835efa9 100644
--- a/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
+++ b/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
@@ -153,11 +153,14 @@ void CheckSecureExplanations(
     EXPECT_EQ(cert_id, secure_explanations[0].cert_id);
   }
 
-  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_SECURE_PROTOCOL_AND_CIPHERSUITE),
+  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_STRONG_SSL_SUMMARY),
             secure_explanations.back().summary);
-  EXPECT_EQ(
-      l10n_util::GetStringUTF8(IDS_SECURE_PROTOCOL_AND_CIPHERSUITE_DESCRIPTION),
-      secure_explanations.back().description);
+
+  const std::string secureDescription =
+      "The connection to this site is encrypted and authenticated using a "

[lgarron, 2016/06/14 00:59:42]

estark@, do you know if it's okay to hardcode these parameter assumptions into a
test? I don't want to drop this EXPECT_EQ unnecessarily, but it seems like the
embedded test server isn't really set up for us to specify/sanity check SSL
settings.

An alternative would be to rewrite BrowserTestNonsecureURLRequest to allow
specifying arbitrary TLS settings instead of hardcoding obsolete ones.

[estark, 2016/06/15 04:46:08]

Why hardcode the string instead of instantiating it with GetStringFUTF8?

Also, instead of hardcoding the parameters, perhaps you could grab them from
ChromeSecurityStateModelClient::FromWebContents(web_contents)->GetSecurityInfo()?

[lgarron, 2016/08/05 23:22:58]

Thanks for the tip. I've switched to instantiating the string based on the
web_contents's security info.

+      "strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and a "
+      "strong cipher (AES_128_GCM).";
+  EXPECT_EQ(secureDescription, secure_explanations.back().description);
 }
 
 void CheckSecurityInfoForSecure(
@@ -860,9 +863,13 @@ IN_PROC_BROWSER_TEST_F(SecurityStyleChangedTest,
 // After AddNonsecureUrlHandler() is called, requests to this hostname
 // will use obsolete TLS settings.
 const char kMockNonsecureHostname[] = "example-nonsecure.test";
+const int obsoleteTLSVersion = net::SSL_CONNECTION_VERSION_TLS1_1;

[estark, 2016/06/15 04:46:08]

should be named kObsoleteTLSVersion, I think (and line 868 as well)

[lgarron, 2016/08/05 23:22:58]

Done.

+// ECDHE_RSA + AES_128_CBC with HMAC-SHA1
+const uint16_t obsoleteTLSJobCipherSuite = 0xc013;

[estark, 2016/06/15 04:46:08]

Why the "Job" in the name?

[lgarron, 2016/08/05 23:22:58]

Because it's the cipher suite used by the [URLRequest]ObsoleteTLSJob class.
I've changed it to kObsoleteCipherSuite.

 
-// A URLRequestMockHTTPJob that mocks a TLS connection with an obsolete
-// protocol version.
+// A URLRequestMockHTTPJob that mocks a TLS connection with the obsolete
+// TLS settings specified in obsoleteTLSVersion and
+// obsoleteTLSJobCipherSuite.
 class URLRequestObsoleteTLSJob : public net::URLRequestMockHTTPJob {
  public:
   URLRequestObsoleteTLSJob(net::URLRequest* request,
@@ -878,10 +885,9 @@ class URLRequestObsoleteTLSJob : public net::URLRequestMockHTTPJob {
 
   void GetResponseInfo(net::HttpResponseInfo* info) override {
     net::URLRequestMockHTTPJob::GetResponseInfo(info);
-    net::SSLConnectionStatusSetVersion(net::SSL_CONNECTION_VERSION_TLS1_1,
+    net::SSLConnectionStatusSetVersion(obsoleteTLSVersion,
                                        &info->ssl_info.connection_status);
-    const uint16_t kTlsEcdheRsaWithAes128CbcSha = 0xc013;
-    net::SSLConnectionStatusSetCipherSuite(kTlsEcdheRsaWithAes128CbcSha,
+    net::SSLConnectionStatusSetCipherSuite(obsoleteTLSJobCipherSuite,
                                            &info->ssl_info.connection_status);
     info->ssl_info.cert = cert_;
   }
@@ -989,9 +995,18 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
   // the TLS settings are obsolete.
   for (const auto& explanation :
        observer.latest_explanations().secure_explanations) {
-    EXPECT_NE(l10n_util::GetStringUTF8(IDS_SECURE_PROTOCOL_AND_CIPHERSUITE),
+    EXPECT_NE(l10n_util::GetStringUTF8(IDS_STRONG_SSL_SUMMARY),
               explanation.summary);
   }
+
+  // Sanity check that the test values match what we expect.
+  ASSERT_EQ(net::SSL_CONNECTION_VERSION_TLS1_1, obsoleteTLSVersion);

[estark, 2016/06/15 04:46:08]

These should both be EXPECT_EQ, unless I'm missing something. (ASSERT is for
when the test would crash if it continued after failing an expectation, for
example to check that a pointer is non-null before dereferencing it later on in
the test.)

[lgarron, 2016/08/05 23:22:58]

Hmm, I had the impression that EXPECT was for stuff we actually want to test,
and ASSERT is to sanity-check stuff that would invalidate what we're actually
trying to test.
https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#as...
seems to support that.

I guess we might add more to the end of the test some day, and in that case it
might be good to report multiple failures, so I've switched to EXPECT_EQ.

+  ASSERT_EQ(0xc013, obsoleteTLSJobCipherSuite);

[estark, 2016/06/15 04:46:08]

Huh. This strikes me as a little weird (asserting that the constants have
particular values). Any reason not to instantiate the parameterized string with
the constants?

[lgarron, 2016/08/05 23:22:58]

Okay, okay. :-)

+
+  EXPECT_EQ(observer.latest_explanations().info_explanations[0].description,
+            "The connection to this site uses an obsolete protocol (TLS 1.1), "

[estark, 2016/06/15 04:46:08]

ditto about instantiating the parameterized string

+            "a strong key exchange (ECDHE_RSA), and an obsolete cipher "
+            "(AES_128_CBC with HMAC-SHA1).");
 }
 
 }  // namespace