Add production Cast CRL certificate.

components/cast_certificate/cast_crl_unittest.cc

Index: components/cast_certificate/cast_crl_unittest.cc
diff --git a/components/cast_certificate/cast_crl_unittest.cc b/components/cast_certificate/cast_crl_unittest.cc
index d65b3a63b1e8d1f0ffaefd93cbebbb65e49a2623..87b39a214a4ee5de6dff4df7096025d73048bcf1 100644
--- a/components/cast_certificate/cast_crl_unittest.cc
+++ b/components/cast_certificate/cast_crl_unittest.cc
@@ -7,11 +7,24 @@
 #include "components/cast_certificate/cast_cert_validator_test_helpers.h"
 #include "components/cast_certificate/cast_crl.h"
 #include "components/cast_certificate/proto/test_suite.pb.h"
+#include "net/cert/internal/trust_store.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace cast_certificate {
 namespace {
 
+// Adds a trust anchor to the provided trust store. The input is the path of
+// the test trust anchor relative to the test directory.
+void AddTrustAnchor(net::TrustStore& trust_store, const std::string& path) {

[eroman, 2016/08/04 19:28:53]

style nit: Make the out parameter a pointer type rather than reference, and make
it the last parameter.

In fact based on a subsequent comment, I suggest prototyping this as:

std::unique_ptr<net::TrustStore> CreateTrustStoreFromFile(const std::string&
path) {
  ...
}

[ryanchung, 2016/08/04 21:56:53]

Done.

+  const auto crl_test_root =
+      cast_certificate::testing::ReadCertificateChainFromFile(path);
+  EXPECT_EQ(crl_test_root.size(), 1u);
+  scoped_refptr<net::ParsedCertificate> anchor(
+      net::ParsedCertificate::CreateFromCertificateCopy(crl_test_root[0], {}));
+  EXPECT_TRUE(anchor);
+  trust_store.AddTrustedCertificate(std::move(anchor));
+}
+
 // Converts uint64_t unix timestamp in seconds to base::Time.
 base::Time ConvertUnixTimestampSeconds(uint64_t time) {
   return base::Time::UnixEpoch() +
@@ -28,11 +41,20 @@ enum TestStepResult {
 // and chains up to a trust anchor.
 bool TestVerifyCertificate(TestStepResult expected_result,
                            const std::vector<std::string>& certificate_chain,
-                           const base::Time& time) {
+                           const base::Time& time,
+                           bool use_test_trust_anchors,

[eroman, 2016/08/04 19:28:53]

Instead of the boolean parameter, how about an optional pointer? (where nullptr
is interpreted as meaning "use the production trust anchors"):

net::TrustStore* cast_trust_store

[ryanchung, 2016/08/04 21:56:53]

Done.

+                           net::TrustStore& cast_trust_store) {

[eroman, 2016/08/04 19:28:53]

see comment above. Style-wise should use pointer rather than reference

[ryanchung, 2016/08/04 21:56:53]

Done.

   std::unique_ptr<CertVerificationContext> context;
   CastDeviceCertPolicy policy;
-  bool result = VerifyDeviceCert(certificate_chain, time, &context, &policy,
-                                 nullptr, CRLPolicy::CRL_OPTIONAL);
+  int result;
+  if (use_test_trust_anchors) {
+    result = VerifyDeviceCertForTest(certificate_chain, time, &context, &policy,
+                                     nullptr, CRLPolicy::CRL_OPTIONAL,
+                                     cast_trust_store);
+  } else {
+    result = VerifyDeviceCert(certificate_chain, time, &context, &policy,
+                              nullptr, CRLPolicy::CRL_OPTIONAL);
+  }
   if (expected_result != RESULT_SUCCESS) {
     EXPECT_FALSE(result);
     return !result;
@@ -46,8 +68,15 @@ bool TestVerifyCertificate(TestStepResult expected_result,
 // The validity of the CRL is also checked at the specified time.
 bool TestVerifyCRL(TestStepResult expected_result,
                    const std::string& crl_bundle,
-                   const base::Time& time) {
-  std::unique_ptr<CastCRL> crl = ParseAndVerifyCRL(crl_bundle, time);
+                   const base::Time& time,
+                   bool use_test_trust_anchors,

[eroman, 2016/08/04 19:28:53]

Same comment as above (how about a single parameter).

There are advantages to two parameters as you have it (can verify that don't
accidentally pass nulptr), but I think based on these tests that is a less
useful assertion than the simpler interface. WDYT?

[ryanchung, 2016/08/04 21:56:53]

Done. Sounds good.

+                   net::TrustStore& crl_trust_store) {
+  std::unique_ptr<CastCRL> crl;
+  if (use_test_trust_anchors) {
+    crl = ParseAndVerifyCRLForTest(crl_bundle, time, crl_trust_store);
+  } else {
+    crl = ParseAndVerifyCRL(crl_bundle, time);
+  }
   if (expected_result != RESULT_SUCCESS) {
     EXPECT_EQ(crl, nullptr);
     return crl == nullptr;
@@ -66,10 +95,17 @@ bool TestVerifyRevocation(TestStepResult expected_result,
                           const std::string& crl_bundle,
                           const base::Time& crl_time,
                           const base::Time& cert_time,
-                          bool crl_required) {
+                          bool crl_required,
+                          bool use_test_trust_anchors,

[eroman, 2016/08/04 19:28:53]

same comment

[ryanchung, 2016/08/04 21:56:53]

Done.

+                          net::TrustStore& cast_trust_store,
+                          net::TrustStore& crl_trust_store) {
   std::unique_ptr<CastCRL> crl;
   if (!crl_bundle.empty()) {
-    crl = ParseAndVerifyCRL(crl_bundle, crl_time);
+    if (use_test_trust_anchors) {
+      crl = ParseAndVerifyCRLForTest(crl_bundle, crl_time, crl_trust_store);
+    } else {
+      crl = ParseAndVerifyCRL(crl_bundle, crl_time);
+    }
     EXPECT_NE(crl.get(), nullptr);
   }
 
@@ -78,8 +114,15 @@ bool TestVerifyRevocation(TestStepResult expected_result,
   CRLPolicy crl_policy = CRLPolicy::CRL_REQUIRED;
   if (!crl_required)
     crl_policy = CRLPolicy::CRL_OPTIONAL;
-  int result = VerifyDeviceCert(certificate_chain, cert_time, &context, &policy,
-                                crl.get(), crl_policy);
+  int result;
+  if (use_test_trust_anchors) {
+    result =
+        VerifyDeviceCertForTest(certificate_chain, cert_time, &context, &policy,
+                                crl.get(), crl_policy, cast_trust_store);
+  } else {
+    result = VerifyDeviceCert(certificate_chain, cert_time, &context, &policy,
+                              crl.get(), crl_policy);
+  }
   if (expected_result != RESULT_SUCCESS) {
     EXPECT_FALSE(result);
     return !result;
@@ -91,18 +134,10 @@ bool TestVerifyRevocation(TestStepResult expected_result,
 // Runs a single test case.
 bool RunTest(const DeviceCertTest& test_case) {
   bool use_test_trust_anchors = test_case.use_test_trust_anchors();

[eroman, 2016/08/04 19:28:53]

If you follow my recommendation from above, then you would probably want to
change this to something like:

std::unique_ptr<net::TrustStore> crl_trust_store;
std::unique_ptr<net::TrustStore> cast_trust_store;

if (test_case.use_test_trust_anchors()) {
  crl_trust_store =
CreateTrustStoreFromFile("certificates/cast_crl_test_root_ca.pem");
  cast_trust_store =
CreateTrustStoreFromFile("certificates/cast_test_root_ca.pem");

  EXPECT_TRUE(crl_trust_store.get());
  EXPECT_TRUE(cast_trust_store.get());
}

Then the rest of the code proceeds by just passing crl_trst_store.get() and
cast_trust_store.get(), since nullptr is interpreted as "use default".

WDYT?

[ryanchung, 2016/08/04 21:56:53]

Done. Sounds good.

-  if (use_test_trust_anchors) {
-    const auto crl_test_root =
-        cast_certificate::testing::ReadCertificateChainFromFile(
-            "certificates/cast_crl_test_root_ca.pem");
-    EXPECT_EQ(crl_test_root.size(), 1u);
-    EXPECT_TRUE(SetCRLTrustAnchorForTest(crl_test_root[0]));
-    const auto cast_test_root =
-        cast_certificate::testing::ReadCertificateChainFromFile(
-            "certificates/cast_test_root_ca.pem");
-    EXPECT_EQ(cast_test_root.size(), 1u);
-    EXPECT_TRUE(SetTrustAnchorForTest(cast_test_root[0]));
-  }
+  net::TrustStore crl_trust_store;
+  net::TrustStore cast_trust_store;
+  AddTrustAnchor(crl_trust_store, "certificates/cast_crl_test_root_ca.pem");
+  AddTrustAnchor(cast_trust_store, "certificates/cast_test_root_ca.pem");
 
   VerificationResult expected_result = test_case.expected_result();
 
@@ -124,35 +159,46 @@ bool RunTest(const DeviceCertTest& test_case) {
   switch (expected_result) {
     case PATH_VERIFICATION_FAILED:
       return TestVerifyCertificate(RESULT_FAIL, certificate_chain,
-                                   cert_verification_time);
+                                   cert_verification_time,
+                                   use_test_trust_anchors, cast_trust_store);
       break;
     case CRL_VERIFICATION_FAILED:
-      return TestVerifyCRL(RESULT_FAIL, crl_bundle, crl_verification_time);
+      return TestVerifyCRL(RESULT_FAIL, crl_bundle, crl_verification_time,
+                           use_test_trust_anchors, crl_trust_store);
       break;
     case REVOCATION_CHECK_FAILED_WITHOUT_CRL:
       return TestVerifyCertificate(RESULT_SUCCESS, certificate_chain,
-                                   cert_verification_time) &&
-             TestVerifyCRL(RESULT_FAIL, crl_bundle, crl_verification_time) &&
+                                   cert_verification_time,
+                                   use_test_trust_anchors, cast_trust_store) &&
+             TestVerifyCRL(RESULT_FAIL, crl_bundle, crl_verification_time,
+                           use_test_trust_anchors, crl_trust_store) &&
              TestVerifyRevocation(RESULT_FAIL, certificate_chain, crl_bundle,
                                   crl_verification_time, cert_verification_time,
-                                  true);
+                                  true, use_test_trust_anchors,
+                                  cast_trust_store, crl_trust_store);
       break;
     case REVOCATION_CHECK_FAILED:
       return TestVerifyCertificate(RESULT_SUCCESS, certificate_chain,
-                                   cert_verification_time) &&
-             TestVerifyCRL(RESULT_SUCCESS, crl_bundle, crl_verification_time) &&
+                                   cert_verification_time,
+                                   use_test_trust_anchors, cast_trust_store) &&
+             TestVerifyCRL(RESULT_SUCCESS, crl_bundle, crl_verification_time,
+                           use_test_trust_anchors, crl_trust_store) &&
              TestVerifyRevocation(RESULT_FAIL, certificate_chain, crl_bundle,
                                   crl_verification_time, cert_verification_time,
-                                  false);
+                                  false, use_test_trust_anchors,
+                                  cast_trust_store, crl_trust_store);
       break;
     case SUCCESS:
-      return (crl_bundle.empty() || TestVerifyCRL(RESULT_SUCCESS, crl_bundle,
-                                                  crl_verification_time)) &&
+      return (crl_bundle.empty() ||
+              TestVerifyCRL(RESULT_SUCCESS, crl_bundle, crl_verification_time,
+                            use_test_trust_anchors, crl_trust_store)) &&
              TestVerifyCertificate(RESULT_SUCCESS, certificate_chain,
-                                   cert_verification_time) &&
+                                   cert_verification_time,
+                                   use_test_trust_anchors, cast_trust_store) &&
              TestVerifyRevocation(RESULT_SUCCESS, certificate_chain, crl_bundle,
                                   crl_verification_time, cert_verification_time,
-                                  !crl_bundle.empty());
+                                  !crl_bundle.empty(), use_test_trust_anchors,
+                                  cast_trust_store, crl_trust_store);
       break;
     case UNSPECIFIED:
       return false;