Do not do Expect-Staple when OCSPVerifyResult has not been populated

net/url_request/url_request_unittest.cc

Index: net/url_request/url_request_unittest.cc
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 60920ef3de116317c49efeddd53b1a18670379a7..9ce5185f4eebd161ff6390de1943cd4bb5682956 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -9619,6 +9619,63 @@ TEST_F(HTTPSOCSPTest, ExpectStapleReportSentOnMissing) {
             mock_report_sender.latest_report_uri());
 }
 
+// Tests that Expect-Staple reports are sent for connections even when there is
+// a certificate error on the connection.
+TEST_F(HTTPSOCSPTest, ExpectStapleReportSentOnMissingWithCertError) {
+  EmbeddedTestServer https_test_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  https_test_server.SetSSLConfig(
+      net::EmbeddedTestServer::CERT_COMMON_NAME_IS_DOMAIN);
+  https_test_server.ServeFilesFromSourceDirectory(
+      base::FilePath(kTestFilePath));
+  ASSERT_TRUE(https_test_server.Start());
+
+  // Set up a MockCertVerifier to accept the certificate that the server sends,
+  // but not provide any OCSP information.

[Ryan Sleevi, 2016/12/21 01:35:29]

The MockCertVerifier doesn't accept this certificate - at least, not per line
9639. The comment seems to conflict with 9638.

// Set up a MockCertVerifier to report an error for the certificate and indicate
no OCSP information was provided.

(Or something better worded, I'm awful here.)

You could also likely delete 9638 with that.

[estark, 2016/12/21 17:53:12]

Done.

+  scoped_refptr<X509Certificate> cert = https_test_server.GetCertificate();
+  ASSERT_TRUE(cert);
+  MockCertVerifier cert_verifier;
+  CertVerifyResult verify_result;
+  // Simulate a certificate verification error.
+  verify_result.cert_status = CERT_STATUS_DATE_INVALID;
+  verify_result.verified_cert = cert;
+  verify_result.is_issued_by_known_root = true;
+  verify_result.ocsp_result.response_status = OCSPVerifyResult::MISSING;

[Ryan Sleevi, 2016/12/21 01:35:29]

Is this really an accurate simulation? Your description suggests that an error
code from a CertVerifier would result in the SSLSocket not checking the
OCSPVerifyResult, rather than saying its missing (at least, judging by the
comment in response_status)

[estark, 2016/12/21 17:53:12]

This test is for the first connection that hits a cert error, not the second
connection that bypasses it. On the first connection everything is populated as
normal (OCSPVerifyResult gets populated in CertVerifyProc regardless of whether
there's an error). It's the second connection where we populate CertVerifyStatus
with just the cached cert status from allowed_bad_certs when OCSPVerifyResult
isn't populated.
(https://cs.chromium.org/chromium/src/net/socket/ssl_client_socket_impl.cc?sq=...)

+  cert_verifier.AddResultForCert(cert.get(), verify_result,
+                                 ERR_CERT_DATE_INVALID);
+
+  // Catch the Expect-Staple report.

[Ryan Sleevi, 2016/12/21 01:35:29]

This comment reads a little weird, because this code block isn't catching
anything, it's just setting up the code to listen for the report.

[estark, 2016/12/21 17:53:12]

Changed to "Set up a mock report sender to..."

+  TransportSecurityState transport_security_state;
+  MockCertificateReportSender mock_report_sender;
+  transport_security_state.SetReportSender(&mock_report_sender);
+
+  // Use a MockHostResolver (which by default maps all hosts to 127.0.0.1) so
+  // that the request can be sent to a site on the Expect-Staple preload list.
+  MockHostResolver host_resolver;

[Ryan Sleevi, 2016/12/21 01:35:29]

Logically, it seems like there should be a newline here between 9653 and 9654 -
because 9654+ has no relation to what the comment describes.

Comment wise, it seems less ideal to describe what MockHostResolver does
(implementation wise) than it is to describe what you're doing.

// Force |kExpectStapleStaticHostname| to resolve to |https_test_server|.

[estark, 2016/12/21 17:53:12]

Done.

+  TestNetworkDelegate network_delegate;
+  TestURLRequestContext context(true);
+  context.set_host_resolver(&host_resolver);
+  context.set_transport_security_state(&transport_security_state);
+  context.set_network_delegate(&network_delegate);
+  context.set_cert_verifier(&cert_verifier);
+  context.Init();
+
+  // Now send a request to trigger the violation.

[Ryan Sleevi, 2016/12/21 01:35:29]

What violation? What is it violating? That's the first usage in this context,
and it's not clear.

// Make a connection to |kExpectStapleStaticHostname|. Because the
|verify_result| used with
// the |cert_verifier| will report a stapled OCSP response was missing, an
Expect-Staple report
// should be sent using |mock_report_sender|.

[estark, 2016/12/21 17:53:12]

Done.

+  TestDelegate d;
+  GURL url = https_test_server.GetURL("/");
+  GURL::Replacements replace_host;
+  replace_host.SetHostStr(kExpectStapleStaticHostname);
+  url = url.ReplaceComponents(replace_host);
+  std::unique_ptr<URLRequest> violating_request(
+      context.CreateRequest(url, DEFAULT_PRIORITY, &d));
+  violating_request->Start();
+  base::RunLoop().Run();
+
+  // Confirm a report was sent.
+  EXPECT_FALSE(mock_report_sender.latest_report().empty());
+  EXPECT_EQ(GURL(kExpectStapleReportURI),
+            mock_report_sender.latest_report_uri());
+}
+
 TEST_F(HTTPSOCSPTest, ExpectStapleReportNotSentOnValid) {
   EmbeddedTestServer https_test_server(net::EmbeddedTestServer::TYPE_HTTPS);
   https_test_server.SetSSLConfig(
@@ -9671,6 +9728,59 @@ TEST_F(HTTPSOCSPTest, ExpectStapleReportNotSentOnValid) {
   EXPECT_EQ(GURL(), mock_report_sender.latest_report_uri());
 }
 
+// Tests that an Expect-Staple report is not sent when OCSP details are not
+// checked on the connection.
+TEST_F(HTTPSOCSPTest, ExpectStapleReportNotSentOnUnknown) {
+  EmbeddedTestServer https_test_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  https_test_server.SetSSLConfig(
+      net::EmbeddedTestServer::CERT_COMMON_NAME_IS_DOMAIN);
+  https_test_server.ServeFilesFromSourceDirectory(
+      base::FilePath(kTestFilePath));
+  ASSERT_TRUE(https_test_server.Start());
+
+  // Set up a MockCertVerifier to accept the certificate that the server sends,
+  // and provide UNKNOWN response status.
+  scoped_refptr<X509Certificate> cert = https_test_server.GetCertificate();
+  ASSERT_TRUE(cert);
+  MockCertVerifier cert_verifier;
+  CertVerifyResult verify_result;
+  verify_result.verified_cert = cert;
+  verify_result.is_issued_by_known_root = true;
+  verify_result.ocsp_result.response_status = OCSPVerifyResult::UNKNOWN;
+  cert_verifier.AddResultForCert(cert.get(), verify_result, OK);
+
+  // Catch the Expect-Staple report.
+  TransportSecurityState transport_security_state;
+  MockCertificateReportSender mock_report_sender;
+  transport_security_state.SetReportSender(&mock_report_sender);
+
+  // Use a MockHostResolver (which by default maps all hosts to 127.0.0.1) so
+  // that the request can be sent to a site on the Expect-Staple preload list.
+  MockHostResolver host_resolver;
+  TestNetworkDelegate network_delegate;
+  TestURLRequestContext context(true);
+  context.set_host_resolver(&host_resolver);
+  context.set_transport_security_state(&transport_security_state);
+  context.set_network_delegate(&network_delegate);
+  context.set_cert_verifier(&cert_verifier);
+  context.Init();
+
+  // This request should not not trigger an Expect-Staple violation.
+  TestDelegate d;
+  GURL url = https_test_server.GetURL("/");
+  GURL::Replacements replace_host;
+  replace_host.SetHostStr(kExpectStapleStaticHostname);
+  url = url.ReplaceComponents(replace_host);
+  std::unique_ptr<URLRequest> ok_request(
+      context.CreateRequest(url, DEFAULT_PRIORITY, &d));
+  ok_request->Start();
+  base::RunLoop().Run();
+
+  // Check that no report was sent.
+  EXPECT_TRUE(mock_report_sender.latest_report().empty());
+  EXPECT_EQ(GURL(), mock_report_sender.latest_report_uri());
+}
+
 static const struct OCSPVerifyTestData {
   std::vector<SpawnedTestServer::SSLOptions::OCSPSingleResponse> ocsp_responses;
   SpawnedTestServer::SSLOptions::OCSPProduced ocsp_produced;