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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ssl/chrome_security_state_model_client.h"
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/strings/string_split.h"
@@ skipping 135 matching lines @@
         secure_explanations[0].description);
     int cert_id = browser->tab_strip_model()
                       ->GetActiveWebContents()
                       ->GetController()
                       .GetActiveEntry()
                       ->GetSSL()
                       .cert_id;
     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(
     content::WebContents* contents,
     SecurityStateModel::SecurityLevel expect_security_level,
     SecurityStateModel::SHA1DeprecationStatus expect_sha1_status,
     SecurityStateModel::MixedContentStatus expect_mixed_content_status,
     bool expect_cert_error) {
   ASSERT_TRUE(contents);
 
@@ skipping 682 matching lines @@
   CheckSecureExplanations(observer.latest_explanations().secure_explanations,
                           VALID_CERTIFICATE, browser());
   EXPECT_TRUE(observer.latest_explanations().scheme_is_cryptographic);
   EXPECT_FALSE(observer.latest_explanations().displayed_insecure_content);
   EXPECT_FALSE(observer.latest_explanations().ran_insecure_content);
 }
 
 // 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,
                            net::NetworkDelegate* network_delegate,
                            const base::FilePath& file_path,
                            scoped_refptr<net::X509Certificate> cert,
                            scoped_refptr<base::TaskRunner> task_runner)
       : net::URLRequestMockHTTPJob(request,
                                    network_delegate,
                                    file_path,
                                    task_runner),
         cert_(std::move(cert)) {}
 
   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_;
   }
 
  protected:
   ~URLRequestObsoleteTLSJob() override {}
 
  private:
   const scoped_refptr<net::X509Certificate> cert_;
 
@@ skipping 87 matching lines @@
   // The security style of the page doesn't get downgraded for obsolete
   // TLS settings, so it should remain at SECURITY_STYLE_AUTHENTICATED.
   EXPECT_EQ(content::SECURITY_STYLE_AUTHENTICATED,
             observer.latest_security_style());
 
   // The messages explaining the security style do, however, get
   // downgraded: SECURE_PROTOCOL_AND_CIPHERSUITE should not show up when
   // 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