Refactor how net/data/verify_certificate_chain_unittest/*

net/cert/internal/test_helpers.cc

Index: net/cert/internal/test_helpers.cc
diff --git a/net/cert/internal/test_helpers.cc b/net/cert/internal/test_helpers.cc
index ab8ed5510949b2a4f5496c06ca4d451f8ae86521..ccd4387e6e7aef390370c6a876ed878152e67122 100644
--- a/net/cert/internal/test_helpers.cc
+++ b/net/cert/internal/test_helpers.cc
@@ -8,6 +8,7 @@
 #include "base/base_paths.h"
 #include "base/files/file_util.h"
 #include "base/path_service.h"
+#include "base/strings/string_split.h"
 #include "net/cert/internal/cert_errors.h"
 #include "net/cert/pem_tokenizer.h"
 #include "net/der/parser.h"
@@ -16,6 +17,27 @@
 
 namespace net {
 
+namespace {
+
+bool GetValue(base::StringPiece prefix,
+              base::StringPiece line,
+              std::string* value,
+              bool* has_value) {
+  if (!line.starts_with(prefix))
+    return false;
+
+  if (*has_value) {
+    ADD_FAILURE() << "Duplicated " << prefix;
+    return false;
+  }
+
+  *has_value = true;
+  *value = line.substr(prefix.size()).as_string();
+  return true;
+}
+
+}  // namespace
+
 namespace der {
 
 void PrintTo(const Input& data, ::std::ostream* os) {
@@ -56,8 +78,8 @@ der::Input SequenceValueFromString(const std::string* s) {
   // Read the full contents of the PEM file.
   std::string file_data;
   if (!base::ReadFileToString(filepath, &file_data)) {
-    return ::testing::AssertionFailure() << "Couldn't read file: "
-                                         << filepath.value();
+    return ::testing::AssertionFailure()
+           << "Couldn't read file: " << filepath.value();
   }
 
   // mappings_copy is used to keep track of which mappings have already been
@@ -94,8 +116,8 @@ der::Input SequenceValueFromString(const std::string* s) {
   // Ensure that all specified blocks were found.
   for (const auto& mapping : mappings_copy) {
     if (mapping.value && !mapping.optional) {
-      return ::testing::AssertionFailure() << "PEM block missing: "
-                                           << mapping.block_name;
+      return ::testing::AssertionFailure()
+             << "PEM block missing: " << mapping.block_name;
     }
   }
 
@@ -105,103 +127,135 @@ der::Input SequenceValueFromString(const std::string* s) {
 VerifyCertChainTest::VerifyCertChainTest() = default;
 VerifyCertChainTest::~VerifyCertChainTest() = default;
 
-void ReadVerifyCertChainTestFromFile(const std::string& file_path_ascii,
+bool ReadCertChainFromFile(const std::string& file_path_ascii,
+                           ParsedCertificateList* chain) {
+  // Reset all the out parameters to their defaults.
+  *chain = ParsedCertificateList();
+
+  std::string file_data = ReadTestFileToString(file_path_ascii);
+  if (file_data.empty())
+    return false;
+
+  std::vector<std::string> pem_headers = {"CERTIFICATE"};
+
+  PEMTokenizer pem_tokenizer(file_data, pem_headers);
+  while (pem_tokenizer.GetNext()) {
+    const std::string& block_data = pem_tokenizer.data();
+
+    CertErrors errors;
+    if (!net::ParsedCertificate::CreateAndAddToVector(
+            bssl::UniquePtr<CRYPTO_BUFFER>(CRYPTO_BUFFER_new(
+                reinterpret_cast<const uint8_t*>(block_data.data()),
+                block_data.size(), nullptr)),
+            {}, chain, &errors)) {
+      ADD_FAILURE() << errors.ToDebugString();
+      return false;
+    }
+  }
+
+  return true;
+}
+
+bool ReadVerifyCertChainTestFromFile(const std::string& file_path_ascii,
                                      VerifyCertChainTest* test) {
   // Reset all the out parameters to their defaults.
   *test = {};
 
   std::string file_data = ReadTestFileToString(file_path_ascii);
+  if (file_data.empty())
+    return false;
 
-  std::vector<std::string> pem_headers;
-
-  // For details on the file format refer to:
-  // net/data/verify_certificate_chain_unittest/README.
-  const char kCertificateHeader[] = "CERTIFICATE";
-  const char kTrustAnchorUnconstrained[] = "TRUST_ANCHOR_UNCONSTRAINED";
-  const char kTrustAnchorConstrained[] = "TRUST_ANCHOR_CONSTRAINED";
-  const char kTimeHeader[] = "TIME";
-  const char kResultHeader[] = "VERIFY_RESULT";
-  const char kErrorsHeader[] = "ERRORS";
-  const char kKeyPurpose[] = "KEY_PURPOSE";
-
-  pem_headers.push_back(kCertificateHeader);
-  pem_headers.push_back(kTrustAnchorUnconstrained);
-  pem_headers.push_back(kTrustAnchorConstrained);
-  pem_headers.push_back(kTimeHeader);
-  pem_headers.push_back(kResultHeader);
-  pem_headers.push_back(kErrorsHeader);
-  pem_headers.push_back(kKeyPurpose);
+  std::vector<std::string> lines = base::SplitString(
+      file_data, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
 
+  bool has_chain = false;
+  bool has_trust = false;
   bool has_time = false;
-  bool has_result = false;
   bool has_errors = false;
   bool has_key_purpose = false;
-  bool has_trust_anchor = false;
 
-  PEMTokenizer pem_tokenizer(file_data, pem_headers);
-  while (pem_tokenizer.GetNext()) {
-    const std::string& block_type = pem_tokenizer.block_type();
-    const std::string& block_data = pem_tokenizer.data();
+  for (const std::string& line : lines) {
+    base::StringPiece line_piece(line);
 
-    if (block_type == kCertificateHeader) {
-      ASSERT_FALSE(has_trust_anchor) << "Trust anchor must appear last";
-      CertErrors errors;
-      ASSERT_TRUE(net::ParsedCertificate::CreateAndAddToVector(
-          bssl::UniquePtr<CRYPTO_BUFFER>(CRYPTO_BUFFER_new(
-              reinterpret_cast<const uint8_t*>(block_data.data()),
-              block_data.size(), nullptr)),
-          {}, &test->chain, &errors))
-          << errors.ToDebugString();
-    } else if (block_type == kTrustAnchorUnconstrained ||
-               block_type == kTrustAnchorConstrained) {
-      ASSERT_FALSE(has_trust_anchor) << "Duplicate trust anchor";
-      CertErrors errors;
-      scoped_refptr<ParsedCertificate> root = net::ParsedCertificate::Create(
-          bssl::UniquePtr<CRYPTO_BUFFER>(CRYPTO_BUFFER_new(
-              reinterpret_cast<const uint8_t*>(block_data.data()),
-              block_data.size(), nullptr)),
-          {}, &errors);
-      ASSERT_TRUE(root) << errors.ToDebugString();
-      test->chain.push_back(std::move(root));
-      test->last_cert_trust =
-          (block_type == kTrustAnchorUnconstrained)
-              ? CertificateTrust::ForTrustAnchor()
-              : CertificateTrust::ForTrustAnchorEnforcingConstraints();
-      has_trust_anchor = true;
-    } else if (block_type == kTimeHeader) {
-      ASSERT_FALSE(has_time) << "Duplicate " << kTimeHeader;
-      has_time = true;
-      ASSERT_TRUE(der::ParseUTCTime(der::Input(&block_data), &test->time));
-    } else if (block_type == kKeyPurpose) {
-      ASSERT_FALSE(has_key_purpose) << "Duplicate " << kKeyPurpose;
-      has_key_purpose = true;
-
-      if (block_data == "anyExtendedKeyUsage") {
+    std::string value;
+
+    // For details on the file format refer to:
+    // net/data/verify_certificate_chain_unittest/README.
+    if (GetValue("chain: ", line_piece, &value, &has_chain)) {
+      std::string chain_path = file_path_ascii;

[mattm, 2017/05/02 06:43:46]

initialized value here isn't used.

[eroman, 2017/05/02 19:20:23]

Done.

+      size_t slash = file_path_ascii.rfind('/');
+      if (slash == std::string::npos) {
+        ADD_FAILURE() << "Bad path - expecting slashes";
+        return false;
+      }
+      chain_path = file_path_ascii.substr(0, slash) + "/" + value;
+
+      ReadCertChainFromFile(chain_path, &test->chain);
+    } else if (GetValue("utc_time: ", line_piece, &value, &has_time)) {
+      if (!der::ParseUTCTime(der::Input(&value), &test->time)) {
+        ADD_FAILURE() << "Failed parsing UTC time";
+        return false;
+      }
+    } else if (GetValue("key_purpose: ", line_piece, &value,
+                        &has_key_purpose)) {
+      if (value == "anyExtendedKeyUsage") {
         test->key_purpose = KeyPurpose::ANY_EKU;
-      } else if (block_data == "serverAuth") {
+      } else if (value == "serverAuth") {
         test->key_purpose = KeyPurpose::SERVER_AUTH;
-      } else if (block_data == "clientAuth") {
+      } else if (value == "clientAuth") {
         test->key_purpose = KeyPurpose::CLIENT_AUTH;
       } else {
-        ADD_FAILURE() << "Unrecognized " << block_type << ": " << block_data;
+        ADD_FAILURE() << "Unrecognized key_purpose: " << value;
+        return false;
+      }
+    } else if (GetValue("last_cert_trust: ", line_piece, &value, &has_trust)) {
+      if (value == "trustAnchor") {
+        test->last_cert_trust = CertificateTrust::ForTrustAnchor();
+      } else if (value == "trustAnchor (enforcesConstraints)") {
+        test->last_cert_trust =
+            CertificateTrust::ForTrustAnchorEnforcingConstraints();
+      } else if (value == "distrusted") {
+        test->last_cert_trust = CertificateTrust::ForDistrusted();
+      } else if (value == "unspecified") {
+        test->last_cert_trust = CertificateTrust::ForUnspecified();
+      } else {
+        ADD_FAILURE() << "Unrecognized last_cert_trust: " << value;
+        return false;
+      }
+    } else if (GetValue("expected_errors:", line_piece, &value, &has_errors)) {
+      // The errors start on the next line, and extend until the end of the
+      // file.
+      size_t errors_start = file_data.find("\nexpected_errors:\n");
+      if (errors_start == std::string::npos) {
+        ADD_FAILURE() << "expected_errors not found";
+        return false;
       }
-    } else if (block_type == kResultHeader) {
-      ASSERT_FALSE(has_result) << "Duplicate " << kResultHeader;
-      ASSERT_TRUE(block_data == "SUCCESS" || block_data == "FAIL")
-          << "Unrecognized result: " << block_data;
-      has_result = true;
-      test->expected_result = block_data == "SUCCESS";
-    } else if (block_type == kErrorsHeader) {
-      ASSERT_FALSE(has_errors) << "Duplicate " << kErrorsHeader;
-      has_errors = true;
-      test->expected_errors = block_data;
+      test->expected_errors = file_data.substr(errors_start + 18);

[mattm, 2017/05/02 06:43:46]

I guess "\nexpected_errors:\n" could be a constant so you could get the length
of that instead of hardcoding 18 here

[eroman, 2017/05/02 19:20:23]

Done.

+      break;
     }

[mattm, 2017/05/02 06:43:46]

should it error on unrecognized lines?

[eroman, 2017/05/02 19:20:23]

Done.

   }
 
-  ASSERT_TRUE(has_time);
-  ASSERT_TRUE(has_result);
-  ASSERT_TRUE(has_trust_anchor);
-  ASSERT_TRUE(has_key_purpose);
+  if (!has_chain) {
+    ADD_FAILURE() << "Missing chain: ";
+    return false;
+  }
+
+  if (!has_trust) {
+    ADD_FAILURE() << "Missing last_cert_trust: ";
+    return false;
+  }
+
+  if (!has_time) {
+    ADD_FAILURE() << "Missing time: ";
+    return false;
+  }
+
+  if (!has_key_purpose) {
+    ADD_FAILURE() << "Missing key_purpose: ";
+    return false;
+  }
+
+  return true;
 }
 
 std::string ReadTestFileToString(const std::string& file_path_ascii) {