Implement Expect-Staple

net/cert/expect_staple_report_unittest.cc

Index: net/cert/expect_staple_report_unittest.cc
diff --git a/net/cert/expect_staple_report_unittest.cc b/net/cert/expect_staple_report_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..027b7fb1c1679b112fd8607598382de9cbf0b298
--- /dev/null
+++ b/net/cert/expect_staple_report_unittest.cc
@@ -0,0 +1,138 @@
+// Copyright 2016 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 "net/cert/expect_staple_report.h"
+
+#include "base/macros.h"
+#include "net/base/host_port_pair.h"
+#include "net/cert/internal/test_helpers.h"
+
+namespace net {
+
+namespace {
+
+const char kOCSPPathPrefix[] = "net/data/parse_ocsp_unittest/";
+
+const base::TimeDelta kOCSPResponseMaxAge = base::TimeDelta::FromDays(3650);

[Ryan Sleevi, 2016/06/17 16:19:56]

This also makes me uncomfortable, FWIW - having callers supply the max age as an
API parameter seems a bit like a large footgun.

[dadrian, 2016/06/17 17:26:55]

It's primarily like that for testing, although I suppose we could backdate all
of the verification times in tests, and always set a hard limit of 1 day / 1
week / whatever.

+
+struct OCSPTest {
+  std::string response;

[Ryan Sleevi, 2016/06/17 16:19:56]

include what you use (IWYU): #include <string>

+  scoped_refptr<X509Certificate> certificate;

[Ryan Sleevi, 2016/06/17 16:19:56]

IWYU:
#include "base/memory/ref_counted.h"
#include "net/cert/x509_certificate.h"

+};
+
+bool LoadOCSPFromFile(std::string file_name, OCSPTest* ocsp) {
+  std::string ca_data;
+  std::string cert_data;
+  const PemBlockMapping mappings[] = {
+      {"OCSP RESPONSE", &ocsp->response},
+      {"CA CERTIFICATE", &ca_data},

[Ryan Sleevi, 2016/06/17 16:19:56]

Why do you do this, when ca_data is unused?

[dadrian, 2016/06/17 17:26:55]

I'm not sure if the test files parse otherwise, since they have the CA
certificate in them. It's currently unused because the code doesn't check
signatures or issuers, as documented in https://crbug.com/620005.

+      {"CERTIFICATE", &cert_data},
+  };
+  std::string full_path = std::string(kOCSPPathPrefix) + file_name;
+  if (!ReadTestDataFromPemFile(full_path, mappings))

[Ryan Sleevi, 2016/06/17 16:19:56]

API: ReadTestDataFromPemFile is designed around a ::testing::AssertionResult
interface, specifically so that the failure can give sufficient diagnostics.
That is,

EXPECT_TRUE(ReadTestDataFromPemFile(...));

The way to propagate this is by having this function return a
::testing::AssertionResult, and return that.

The only place this pattern isn't followed is the OCSP parsing code, and I'm not
sure if that's a good pattern we want to mix in.

+    return false;
+
+  // Parse the server certificate
+  CertificateList server_cert_list =
+      X509Certificate::CreateCertificateListFromBytes(
+          cert_data.data(), cert_data.size(),
+          X509Certificate::FORMAT_SINGLE_CERTIFICATE);
+  ocsp->certificate = server_cert_list[0];
+  return true;
+}
+
+}  // namespace
+
+class ExpectStapleReportTest : public testing::Test {
+ public:
+  ExpectStapleReportTest() {}
+
+ protected:
+  void SetUp() override {

[Ryan Sleevi, 2016/06/17 16:19:56]

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

+    // Thu Jun 16 11:29:38 PDT 2016
+    verify_time_ = base::Time::FromDoubleT(1466101795.0);

[Ryan Sleevi, 2016/06/16 21:49:29]

Suggestion: base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(...)

(Makes it easier for people to update the value by just using a time-t
converter)

+  }
+
+  std::unique_ptr<ExpectStapleReport> MakeReport(const OCSPTest& ocsp) {
+    std::unique_ptr<ExpectStapleReport> report =
+        ExpectStapleReport::FromRawOCSPResponse(ocsp.response, verify_time_,
+                                                kOCSPResponseMaxAge,
+                                                *ocsp.certificate);
+    return report;
+  }
+
+  base::Time verify_time_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExpectStapleReportTest);
+};
+
+TEST_F(ExpectStapleReportTest, Valid) {
+  OCSPTest ocsp;
+  ASSERT_TRUE(LoadOCSPFromFile("good_response.pem", &ocsp));
+  auto report = MakeReport(ocsp);

[Ryan Sleevi, 2016/06/17 16:19:56]

This is not an allowed usage of auto

1) Return type is not clear
2) Autos for pointer-semantics of creators is *especially* opaque. That is, you
have to look on MakeReport to see are you getting a naked pointer, a unique
pointer, a ref-counted, etc.

There was discussion on chromium-dev regarding this pattern, with the conclusion
being even for testing code, we should follow the style guide's remarks on auto,
which is to eschew it when it hinders comprehensibility of the code.

[dadrian, 2016/06/17 17:26:55]

This was me being lazy when I was getting the tests to compile, and I forgot to
go back and change it.

+  ASSERT_TRUE(report);
+  EXPECT_EQ(ExpectStapleReport::StapleError::OK, report->staple_error());
+  EXPECT_EQ(verify_time_, report->verify_time());
+  const auto& stapled_responses = report->stapled_responses();

[Ryan Sleevi, 2016/06/17 16:19:56]

This is an OK auto fwiw :)

+  ASSERT_EQ(1u, stapled_responses.size());
+  EXPECT_TRUE(stapled_responses[0].is_date_valid);
+  EXPECT_TRUE(stapled_responses[0].is_correct_certificate);
+  EXPECT_EQ(OCSPCertStatus::Status::GOOD, stapled_responses[0].status);
+};
+
+TEST_F(ExpectStapleReportTest, ValidWithExtension) {
+  OCSPTest ocsp;
+  ASSERT_TRUE(LoadOCSPFromFile("has_extension.pem", &ocsp));
+  auto report = MakeReport(ocsp);
+  ASSERT_TRUE(report);
+  EXPECT_EQ(ExpectStapleReport::StapleError::OK, report->staple_error());
+  EXPECT_EQ(verify_time_, report->verify_time());
+};
+
+TEST_F(ExpectStapleReportTest, MissingSingleResponse) {
+  OCSPTest ocsp;
+  ASSERT_TRUE(LoadOCSPFromFile("missing_response.pem", &ocsp));
+  auto report = MakeReport(ocsp);
+  ASSERT_TRUE(report);
+  EXPECT_EQ(ExpectStapleReport::StapleError::NO_MATCHING_RESPONSE,
+            report->staple_error());
+  EXPECT_EQ(verify_time_, report->verify_time());
+  const auto& stapled_responses = report->stapled_responses();
+  EXPECT_EQ(0u, stapled_responses.size());
+};
+
+TEST_F(ExpectStapleReportTest, MultipleResponse) {
+  OCSPTest ocsp;
+  ASSERT_TRUE(LoadOCSPFromFile("multiple_response.pem", &ocsp));
+  auto report = MakeReport(ocsp);
+  ASSERT_TRUE(report);
+  EXPECT_EQ(ExpectStapleReport::StapleError::OK, report->staple_error());
+  EXPECT_EQ(verify_time_, report->verify_time());
+  const auto& stapled_responses = report->stapled_responses();
+  ASSERT_EQ(2u, stapled_responses.size());
+  for (const auto& staple : stapled_responses) {
+    EXPECT_TRUE(staple.is_date_valid);
+    EXPECT_TRUE(staple.is_correct_certificate);
+  }
+  EXPECT_EQ(OCSPCertStatus::Status::GOOD, stapled_responses[0].status);
+  EXPECT_EQ(OCSPCertStatus::Status::UNKNOWN, stapled_responses[1].status);
+};
+
+TEST_F(ExpectStapleReportTest, RevokeResponse) {
+  OCSPTest ocsp;
+  ASSERT_TRUE(LoadOCSPFromFile("revoke_response.pem", &ocsp));
+  auto report = MakeReport(ocsp);
+  ASSERT_TRUE(report);
+  EXPECT_EQ(ExpectStapleReport::StapleError::NO_MATCHING_RESPONSE,
+            report->staple_error());
+  EXPECT_EQ(verify_time_, report->verify_time());
+  const auto& stapled_responses = report->stapled_responses();
+  ASSERT_EQ(1u, stapled_responses.size());
+  EXPECT_TRUE(stapled_responses[0].is_date_valid);
+  EXPECT_TRUE(stapled_responses[0].is_correct_certificate);
+  EXPECT_EQ(OCSPCertStatus::Status::REVOKED, stapled_responses[0].status);
+};
+
+}  // namespace