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

net/url_request/url_request_unittest.cc

 // Copyright (c) 2012 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 <memory>
 #include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "build/build_config.h"
@@ skipping 9601 matching lines @@
       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());
 }
 
+// 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(
       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 GOOD revocation status.
@@ skipping 32 matching lines @@
   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());
 }
 
+// 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;
   OCSPVerifyResult::ResponseStatus response_status;
   bool has_revocation_status;
   OCSPRevocationStatus cert_status;
 } kOCSPVerifyData[] = {
 
     {{{SpawnedTestServer::SSLOptions::OCSP_OK,
        SpawnedTestServer::SSLOptions::OCSP_DATE_VALID}},
@@ skipping 950 matching lines @@
   AddTestInterceptor()->set_main_intercept_job(std::move(job));
 
   req->Start();
   req->Cancel();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(ERR_ABORTED, d.request_status());
   EXPECT_EQ(0, d.received_redirect_count());
 }
 
 }  // namespace net