Add OCSPVerifyResult for tracking stapled OCSP responses cross-platform.

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 cd41ebba004bd5a806f7ee4d2799f6c29f36fc2d..d6fd7817e26a6b63437c2df20d083d2c1e0aa391 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -9247,26 +9247,49 @@ class HTTPSOCSPTest : public HTTPSRequestTest {
 #endif
   }
 
-  void DoConnection(const SpawnedTestServer::SSLOptions& ssl_options,
-                    CertStatus* out_cert_status) {
-    // We always overwrite out_cert_status.
-    *out_cert_status = 0;
+  ::testing::AssertionResult DoConnection(

[svaldez, 2016/06/29 14:41:23]

Still unclear whether you need the return change.

[dadrian, 2016/06/30 21:52:43]

I got flak for not making this change on another review.

[Ryan Sleevi, 2016/06/30 22:14:51]

It may not have been clear why the change.

It matters if you're doing ASSERTs (which abort the method), as you need to have
a return value that allows them to escape.

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
covers this in more detail, but judging by it, you don't need the
::testing::AssertionResult for any of these; it only becomes an issue if you're
trying to abort early.

[dadrian, 2016/07/08 22:17:30]

Reverted, but not sure I follow. You can't use an ASSERT in a function that
returns a ::testing::AssertionResult, just EXCEPT.

+      const SpawnedTestServer::SSLOptions& ssl_options,
+      TestDelegate* delegate,
+      SSLInfo* out_ssl_info) {
+    // Always overwrite |out_ssl_info|.
+    out_ssl_info->Reset();
+
     SpawnedTestServer test_server(
         SpawnedTestServer::TYPE_HTTPS,
         ssl_options,
         base::FilePath(FILE_PATH_LITERAL("net/data/ssl")));
-    ASSERT_TRUE(test_server.Start());
+    EXPECT_TRUE(test_server.Start());
 
-    TestDelegate d;
-    d.set_allow_certificate_errors(true);
-    std::unique_ptr<URLRequest> r(
-        context_.CreateRequest(test_server.GetURL("/"), DEFAULT_PRIORITY, &d));
+    delegate->set_allow_certificate_errors(true);
+    std::unique_ptr<URLRequest> r(context_.CreateRequest(
+        test_server.GetURL("/"), DEFAULT_PRIORITY, delegate));
     r->Start();
 
     base::RunLoop().Run();
+    EXPECT_EQ(1, delegate->response_started_count());
 
-    EXPECT_EQ(1, d.response_started_count());
-    *out_cert_status = r->ssl_info().cert_status;
+    *out_ssl_info = r->ssl_info();
+    return ::testing::AssertionSuccess();
+  }
+
+  ::testing::AssertionResult DoConnection(
+      const SpawnedTestServer::SSLOptions& ssl_options,
+      SSLInfo* out_ssl_info) {
+    TestDelegate d;
+    return DoConnection(ssl_options, &d, out_ssl_info);
+  }
+
+  ::testing::AssertionResult DoConnection(
+      const SpawnedTestServer::SSLOptions& ssl_options,
+      CertStatus* out_cert_status) {
+    // Always overwrite |out_cert_status|.
+    *out_cert_status = 0;
+
+    SSLInfo ssl_info;
+    EXPECT_TRUE(DoConnection(ssl_options, &ssl_info));
+
+    *out_cert_status = ssl_info.cert_status;
+    return ::testing::AssertionSuccess();
   }
 
   ~HTTPSOCSPTest() override {
@@ -9485,6 +9508,182 @@ TEST_F(HTTPSOCSPTest, MAYBE_RevokedStapled) {
   EXPECT_TRUE(cert_status & CERT_STATUS_REV_CHECKING_ENABLED);
 }
 
+struct OCSPVerifyTestData {
+  SpawnedTestServer::SSLOptions::OCSPStatus ocsp_status;
+  SpawnedTestServer::SSLOptions::OCSPDate ocsp_date;
+  OCSPVerifyResult::ResponseStatus response_status;
+  bool has_cert_status;
+  OCSPCertStatus::Status cert_status;
+};
+
+static const OCSPVerifyTestData kOCSPVerifyData[] = {

[Ryan Sleevi, 2016/06/30 22:14:51]

We tend to just combine these definitions

static const struct OCSPVerifyTestData {
  ...
} kOCSPVerifyData[] = {
  ...
};

[dadrian, 2016/07/08 22:17:30]

I had kept them separate since the type was used in more than one location, but
this is no longer the case.

+    {
+        SpawnedTestServer::SSLOptions::OCSP_OK,
+        SpawnedTestServer::SSLOptions::OCSP_VALID, OCSPVerifyResult::PROVIDED,
+        true, OCSPCertStatus::Status::GOOD,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_OK,
+        SpawnedTestServer::SSLOptions::OCSP_OLD, OCSPVerifyResult::INVALID_DATE,
+        false, OCSPCertStatus::Status::GOOD,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_OK,
+        SpawnedTestServer::SSLOptions::OCSP_EARLY,
+        OCSPVerifyResult::INVALID_DATE, false, OCSPCertStatus::Status::GOOD,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_OK,
+        SpawnedTestServer::SSLOptions::OCSP_LONG,
+        OCSPVerifyResult::INVALID_DATE, false, OCSPCertStatus::Status::GOOD,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_INVALID,
+        SpawnedTestServer::SSLOptions::OCSP_VALID,
+        OCSPVerifyResult::PARSE_RESPONSE, false,
+        OCSPCertStatus::Status::UNKNOWN,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_REVOKED,
+        SpawnedTestServer::SSLOptions::OCSP_EARLY,
+        OCSPVerifyResult::INVALID_DATE, false, OCSPCertStatus::Status::UNKNOWN,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_UNKNOWN,
+        SpawnedTestServer::SSLOptions::OCSP_VALID, OCSPVerifyResult::PROVIDED,
+        true, OCSPCertStatus::Status::UNKNOWN,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_UNKNOWN,
+        SpawnedTestServer::SSLOptions::OCSP_OLD, OCSPVerifyResult::INVALID_DATE,
+        false, OCSPCertStatus::Status::UNKNOWN,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_UNKNOWN,
+        SpawnedTestServer::SSLOptions::OCSP_EARLY,
+        OCSPVerifyResult::INVALID_DATE, false, OCSPCertStatus::Status::UNKNOWN,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_UNKNOWN,
+        SpawnedTestServer::SSLOptions::OCSP_LONG,
+        OCSPVerifyResult::INVALID_DATE, false, OCSPCertStatus::Status::UNKNOWN,
+    },
+};
+
+class HTTPSOCSPVerifyTest
+    : public HTTPSOCSPTest,
+      public testing::WithParamInterface<OCSPVerifyTestData> {
+ public:
+  HTTPSOCSPVerifyTest() = default;
+  virtual ~HTTPSOCSPVerifyTest() {}
+};
+
+TEST_P(HTTPSOCSPVerifyTest, SingleResponse) {
+  SpawnedTestServer::SSLOptions ssl_options(
+      SpawnedTestServer::SSLOptions::CERT_AUTO);
+  OCSPVerifyTestData test = GetParam();
+  ssl_options.ocsp_status = test.ocsp_status;
+  ssl_options.ocsp_date = test.ocsp_date;
+  ssl_options.staple_ocsp_response = true;
+
+  SSLInfo ssl_info;
+  ASSERT_TRUE(DoConnection(ssl_options, &ssl_info));
+
+  EXPECT_EQ(0u, ssl_info.cert_status & CERT_STATUS_ALL_ERRORS);
+  EXPECT_EQ(test.response_status, ssl_info.ocsp.response_status);
+
+  if (test.has_cert_status) {
+    ASSERT_TRUE(ssl_info.ocsp.cert_status);
+    EXPECT_EQ(test.cert_status, *ssl_info.ocsp.cert_status);
+  } else {
+    EXPECT_FALSE(ssl_info.ocsp.cert_status);
+  }
+};
+
+INSTANTIATE_TEST_CASE_P(OCSPVerify,
+                        HTTPSOCSPVerifyTest,
+                        testing::ValuesIn(kOCSPVerifyData));
+
+// OCSPErrorTestDelegate caches the SSLInfo passed to OnSSLCertificateError.
+// This is needed because after the certificate failure, the URLRequest will
+// retry the connection, and return a partial SSLInfo with a cached cert status.
+// The partial SSLInfo does not have the OCSP information filled out.
+class OCSPErrorTestDelegate : public TestDelegate {

[dadrian, 2016/06/27 22:43:03]

This approach is definitely less than ideal, and needlessly splits up the
OCSPVerify tests into two classes. One class for "successful" responses, and one
class for "failure" responses. This was the first solution svaldez and I came up
with, but I'm curious if anyone else has ideas, especially given the further
issues described below.

[svaldez, 2016/06/29 14:41:23]

Arguably, we might actually want a flag on the cert verifier that doesn't pass
OCSP information into the system verifier, which would prevent certificate
errors as a result of OCSP, though I don't know whether we actually want that
much plumbing.

Alternatively, should OCSP stapling mean we don't cache results, or make sure
the lifetime is only valid within the OCSP lifetime?

[Ryan Sleevi, 2016/06/30 22:14:50]

I'm not sure I understand what you're proposing?
For disk cache? I would argue we shouldn't cache the OCSP response; we don't
re-verify certs as they come out of the disk cache, so the OCSP status is
irrelevant. Nor do we need to limit the disk cache response (doing so gets to
the more fundamental discussion about whether to re-validate certs as they come
out of the cache, which we don't, but limiting cache lifetime to OCSP lifetime
implies that)

[Ryan Sleevi, 2016/06/30 22:14:51]

Have you traced through with gdb to figure out who is calling this?
https://cs.chromium.org/chromium/src/net/url_request/url_request.h?rcl=146730...
is the magic point on the delegate,
https://cs.chromium.org/chromium/src/net/url_request/url_request.h?rcl=146730...
on the URLRequest

[dadrian, 2016/07/08 22:17:30]

I worked around it, but it was getting modified by
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc....

+ public:
+  void OnSSLCertificateError(URLRequest* request,
+                             const SSLInfo& ssl_info,
+                             bool fatal) override {
+    ssl_info_ = ssl_info;
+    on_ssl_certificate_error_called_ = true;
+    TestDelegate::OnSSLCertificateError(request, ssl_info, fatal);
+  }
+
+  bool on_ssl_certificate_error_called() {
+    return on_ssl_certificate_error_called_;
+  }
+
+  SSLInfo ssl_info() { return ssl_info_; }
+
+ private:
+  bool on_ssl_certificate_error_called_ = false;
+  SSLInfo ssl_info_;
+};
+
+static const OCSPVerifyTestData kOCSPFailData[] = {
+    {
+        SpawnedTestServer::SSLOptions::OCSP_REVOKED,
+        SpawnedTestServer::SSLOptions::OCSP_VALID, OCSPVerifyResult::PROVIDED,
+        true, OCSPCertStatus::Status::REVOKED,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_REVOKED,
+        SpawnedTestServer::SSLOptions::OCSP_OLD, OCSPVerifyResult::INVALID_DATE,

[dadrian, 2016/06/27 22:43:03]

Arguably, this test case should not cause a failure, since the response is old,
and therefore invalid. However, it does fail (at least with NSS). I'm not sure
how other platforms will behave, but I wouldn't be surprised if some will treat
this as GOOD, and some as REVOKED. I can run the CL against the try bots once
the dependent CL lands, but I'm anticipating issues from this.

+        false, OCSPCertStatus::Status::UNKNOWN,
+    },
+    {
+        SpawnedTestServer::SSLOptions::OCSP_REVOKED,
+        SpawnedTestServer::SSLOptions::OCSP_LONG,

[dadrian, 2016/06/27 22:43:03]

Similar comment about this test case.

+        OCSPVerifyResult::INVALID_DATE, false, OCSPCertStatus::Status::UNKNOWN,
+    },
+};
+
+class HTTPSOCSPFailTest
+    : public HTTPSOCSPTest,
+      public testing::WithParamInterface<OCSPVerifyTestData> {
+ public:
+  HTTPSOCSPFailTest() = default;
+  virtual ~HTTPSOCSPFailTest() {}
+};
+
+TEST_P(HTTPSOCSPFailTest, SingleResponse) {
+  SpawnedTestServer::SSLOptions ssl_options(
+      SpawnedTestServer::SSLOptions::CERT_AUTO);
+  OCSPVerifyTestData test = GetParam();
+  ssl_options.ocsp_status = test.ocsp_status;
+  ssl_options.ocsp_date = test.ocsp_date;
+  ssl_options.staple_ocsp_response = true;
+
+  SSLInfo unused;
+  OCSPErrorTestDelegate d;
+  ASSERT_TRUE(DoConnection(ssl_options, &d, &unused));
+  ASSERT_TRUE(d.on_ssl_certificate_error_called());
+  SSLInfo ssl_info = d.ssl_info();
+
+  EXPECT_EQ(CERT_STATUS_REVOKED, ssl_info.cert_status & CERT_STATUS_REVOKED);
+  EXPECT_EQ(test.response_status, ssl_info.ocsp.response_status);
+
+  if (test.has_cert_status) {
+    ASSERT_TRUE(ssl_info.ocsp.cert_status);
+    EXPECT_EQ(test.cert_status, *ssl_info.ocsp.cert_status);
+  } else {
+    EXPECT_FALSE(ssl_info.ocsp.cert_status);
+  }
+};
+
+INSTANTIATE_TEST_CASE_P(OCSPVerify,
+                        HTTPSOCSPFailTest,
+                        testing::ValuesIn(kOCSPFailData));
+
 class HTTPSHardFailTest : public HTTPSOCSPTest {
  protected:
   void SetupContext() override {