[ABANDONED] Refactor CertVerifyProcWeakDigestTest.

net/cert/cert_verify_proc_unittest.cc

Index: net/cert/cert_verify_proc_unittest.cc
diff --git a/net/cert/cert_verify_proc_unittest.cc b/net/cert/cert_verify_proc_unittest.cc
index c6bb0e71bc5f8fc2701fdc030ef9c6f8bffd02ad..38aef6555f0abdd5d53431d4dbfed44a2ce10787 100644
--- a/net/cert/cert_verify_proc_unittest.cc
+++ b/net/cert/cert_verify_proc_unittest.cc
@@ -1662,19 +1662,9 @@ TEST_F(CertVerifyProcTest, RejectsPrivateSHA1UnlessFlag) {
   EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_SHA1_SIGNATURE_PRESENT);
 }
 
-enum ExpectedAlgorithms {
-  EXPECT_MD2 = 1 << 0,
-  EXPECT_MD4 = 1 << 1,
-  EXPECT_MD5 = 1 << 2,
-  EXPECT_SHA1 = 1 << 3,
-  EXPECT_SHA1_LEAF = 1 << 4,
-};
-
 struct WeakDigestTestData {
-  const char* root_cert_filename;
-  const char* intermediate_cert_filename;
-  const char* ee_cert_filename;
-  int expected_algorithms;
+  const char* cert_filename;
+  X509Certificate::SignatureHashAlgorithm expected_algorithm;
 };
 
 // GTest 'magic' pretty-printer, so that if/when a test fails, it knows how
@@ -1682,10 +1672,7 @@ struct WeakDigestTestData {
 // attempt to print out the first twenty bytes of the object, which depending
 // on platform and alignment, may result in an invalid read.
 void PrintTo(const WeakDigestTestData& data, std::ostream* os) {
-  *os << "root: "
-      << (data.root_cert_filename ? data.root_cert_filename : "none")
-      << "; intermediate: " << data.intermediate_cert_filename
-      << "; end-entity: " << data.ee_cert_filename;
+  *os << "cert: " << data.cert_filename;
 }
 
 class CertVerifyProcWeakDigestTest
@@ -1697,196 +1684,59 @@ class CertVerifyProcWeakDigestTest
 };
 
 // Test that the underlying cryptographic library properly surfaces the
-// algorithms used in the chain. Some libraries, like NSS, don't return
-// the failing chain on error, and thus not all tests can be run.
+// algorithm used in a certificate.
 TEST_P(CertVerifyProcWeakDigestTest, VerifyDetectsAlgorithm) {

[Ryan Sleevi, 2017/01/06 02:27:14]

So, a few behaviour changes worth calling out:
1) Not testing Verify() is actually calling this
2) Not testing intermediates (are handled) or that roots (are ignored)

In effect, this is now just testing FillCertVerifyResultWeakSignature().

I'm wondering if we can't do something like:

(Keep all of lines 1706-1732 of the original)
ASSERT_TRUE(verify_result.verified_cert);
bool has_chain =
!verify_result.verified_cert->GetIntermediateCertificates().empty();
bool expect_has_md2 =
  (data.expected_leaf_algorithm == X509Certificate::kSignatureHashAlgorithmMd2)
||
  (has_chain && data.expected_chain_algorithm ==
X509Certificate::kSignatureHashAlgorithmMd2);
...
bool expect_has_sha1_leaf =
  (data.expected_leaf_algorithm == ...);


It may be that the above change only works if we uplift to
CertVerifyProc::Verify() rather than in the VerifyInternal()s that it is today,
because there may be short-circuits on the CertVerifyResult in the case of
failure (... probably).

If that's the case though, then I guess it's still blocked on the further
cleanup, because I'm a little nervous about any gap where we lose test coverage.

   WeakDigestTestData data = GetParam();
   base::FilePath certs_dir = GetTestCertsDirectory();
 
-  ScopedTestRoot test_root;
-  if (data.root_cert_filename) {
-     scoped_refptr<X509Certificate> root_cert =
-         ImportCertFromFile(certs_dir, data.root_cert_filename);
-     ASSERT_TRUE(root_cert);
-     test_root.Reset(root_cert.get());
-  }
+  scoped_refptr<X509Certificate> cert =
+      ImportCertFromFile(certs_dir, data.cert_filename);
+  ASSERT_TRUE(cert);
 
-  scoped_refptr<X509Certificate> intermediate_cert =
-      ImportCertFromFile(certs_dir, data.intermediate_cert_filename);
-  ASSERT_TRUE(intermediate_cert);
-  scoped_refptr<X509Certificate> ee_cert =
-      ImportCertFromFile(certs_dir, data.ee_cert_filename);
-  ASSERT_TRUE(ee_cert);
+  bool expected_has_md2 =
+      data.expected_algorithm == X509Certificate::kSignatureHashAlgorithmMd2;
+  bool expected_has_md4 =
+      data.expected_algorithm == X509Certificate::kSignatureHashAlgorithmMd4;
+  bool expected_has_md5 =
+      data.expected_algorithm == X509Certificate::kSignatureHashAlgorithmMd5;
+  bool expected_has_sha1 =
+      data.expected_algorithm == X509Certificate::kSignatureHashAlgorithmSha1;
+
+  // Try verifying with both is_leaf=true, and is_leaf=false.
+  for (bool is_leaf : {true, false}) {
+    // Fill the weak hash algorithm information using a default-initialized
+    // CertVerifyResult (each of has_XXX will be false).
+    CertVerifyResult verify_result;
+    X509Certificate::SignatureHashAlgorithm hash_algorithm =
+        FillCertVerifyResultWeakSignature(cert->os_cert_handle(), is_leaf,
+                                          &verify_result);
 
-  X509Certificate::OSCertHandles intermediates;
-  intermediates.push_back(intermediate_cert->os_cert_handle());
+    EXPECT_EQ(data.expected_algorithm, hash_algorithm);
 
-  scoped_refptr<X509Certificate> ee_chain =
-      X509Certificate::CreateFromHandle(ee_cert->os_cert_handle(),
-                                        intermediates);
-  ASSERT_TRUE(ee_chain);
+    EXPECT_EQ(expected_has_md2, verify_result.has_md2);
+    EXPECT_EQ(expected_has_md4, verify_result.has_md4);
+    EXPECT_EQ(expected_has_md5, verify_result.has_md5);
+    EXPECT_EQ(expected_has_sha1, verify_result.has_sha1);
+    EXPECT_EQ(expected_has_sha1 && is_leaf, verify_result.has_sha1_leaf);
+  }
 
-  int flags = 0;
-  CertVerifyResult verify_result;
-  Verify(ee_chain.get(), "127.0.0.1", flags, NULL, empty_cert_list_,
-         &verify_result);
-  EXPECT_EQ(!!(data.expected_algorithms & EXPECT_MD2), verify_result.has_md2);
-  EXPECT_EQ(!!(data.expected_algorithms & EXPECT_MD4), verify_result.has_md4);
-  EXPECT_EQ(!!(data.expected_algorithms & EXPECT_MD5), verify_result.has_md5);
-  EXPECT_EQ(!!(data.expected_algorithms & EXPECT_SHA1), verify_result.has_sha1);
-  EXPECT_EQ(!!(data.expected_algorithms & EXPECT_SHA1_LEAF),
-            verify_result.has_sha1_leaf);
+  // TODO(eroman): Verify that values are not re-set to false when not
+  //               applicable.

[Ryan Sleevi, 2017/01/06 02:27:14]

I'm not sure why this TODO?

 }
 
-// Unlike TEST/TEST_F, which are macros that expand to further macros,
-// INSTANTIATE_TEST_CASE_P is a macro that expands directly to code that
-// stringizes the arguments. As a result, macros passed as parameters (such as
-// prefix or test_case_name) will not be expanded by the preprocessor. To work
-// around this, indirect the macro for INSTANTIATE_TEST_CASE_P, so that the
-// pre-processor will expand macros such as MAYBE_test_name before
-// instantiating the test.
-#define WRAPPED_INSTANTIATE_TEST_CASE_P(prefix, test_case_name, generator) \
-    INSTANTIATE_TEST_CASE_P(prefix, test_case_name, generator)
-
-// The signature algorithm of the root CA should not matter.
-const WeakDigestTestData kVerifyRootCATestData[] = {
-    {"weak_digest_md5_root.pem", "weak_digest_sha1_intermediate.pem",
-     "weak_digest_sha1_ee.pem", EXPECT_SHA1 | EXPECT_SHA1_LEAF},
-#if defined(USE_OPENSSL_CERTS) || defined(OS_WIN)
-    // MD4 is not supported by OS X / NSS
-    {"weak_digest_md4_root.pem", "weak_digest_sha1_intermediate.pem",
-     "weak_digest_sha1_ee.pem", EXPECT_SHA1 | EXPECT_SHA1_LEAF},
-#endif
-    {"weak_digest_md2_root.pem", "weak_digest_sha1_intermediate.pem",
-     "weak_digest_sha1_ee.pem", EXPECT_SHA1 | EXPECT_SHA1_LEAF},
+const WeakDigestTestData kVerifyWeakSignatureData[] = {
+    {"weak_digest_md5_intermediate.pem",
+     X509Certificate::kSignatureHashAlgorithmMd5},
+    {"weak_digest_md4_intermediate.pem",
+     X509Certificate::kSignatureHashAlgorithmMd4},
+    {"weak_digest_md2_intermediate.pem",
+     X509Certificate::kSignatureHashAlgorithmMd2},
+    {"weak_digest_sha1_ee.pem", X509Certificate::kSignatureHashAlgorithmSha1},
 };
-INSTANTIATE_TEST_CASE_P(VerifyRoot,
+
+INSTANTIATE_TEST_CASE_P(,
                         CertVerifyProcWeakDigestTest,
-                        testing::ValuesIn(kVerifyRootCATestData));
-
-// The signature algorithm of intermediates should be properly detected.
-const WeakDigestTestData kVerifyIntermediateCATestData[] = {
-    {"weak_digest_sha1_root.pem", "weak_digest_md5_intermediate.pem",
-     "weak_digest_sha1_ee.pem", EXPECT_MD5 | EXPECT_SHA1 | EXPECT_SHA1_LEAF},
-#if defined(USE_OPENSSL_CERTS) || defined(OS_WIN)
-    // MD4 is not supported by OS X / NSS
-    {"weak_digest_sha1_root.pem", "weak_digest_md4_intermediate.pem",
-     "weak_digest_sha1_ee.pem", EXPECT_MD4 | EXPECT_SHA1 | EXPECT_SHA1_LEAF},
-#endif
-    {"weak_digest_sha1_root.pem", "weak_digest_md2_intermediate.pem",
-     "weak_digest_sha1_ee.pem", EXPECT_MD2 | EXPECT_SHA1 | EXPECT_SHA1_LEAF},
-};
-// Disabled on NSS - MD4 is not supported, and MD2 and MD5 are disabled.
-#if defined(USE_NSS_CERTS) || defined(OS_IOS)
-#define MAYBE_VerifyIntermediate DISABLED_VerifyIntermediate
-#else
-#define MAYBE_VerifyIntermediate VerifyIntermediate
-#endif
-WRAPPED_INSTANTIATE_TEST_CASE_P(
-    MAYBE_VerifyIntermediate,
-    CertVerifyProcWeakDigestTest,
-    testing::ValuesIn(kVerifyIntermediateCATestData));
-
-// The signature algorithm of end-entity should be properly detected.
-const WeakDigestTestData kVerifyEndEntityTestData[] = {
-  { "weak_digest_sha1_root.pem", "weak_digest_sha1_intermediate.pem",
-    "weak_digest_md5_ee.pem", EXPECT_MD5 | EXPECT_SHA1 },
-#if defined(USE_OPENSSL_CERTS) || defined(OS_WIN)
-  // MD4 is not supported by OS X / NSS
-  { "weak_digest_sha1_root.pem", "weak_digest_sha1_intermediate.pem",
-    "weak_digest_md4_ee.pem", EXPECT_MD4 | EXPECT_SHA1 },
-#endif
-  { "weak_digest_sha1_root.pem", "weak_digest_sha1_intermediate.pem",
-    "weak_digest_md2_ee.pem", EXPECT_MD2 | EXPECT_SHA1 },
-};
-// Disabled on NSS - NSS caches chains/signatures in such a way that cannot
-// be cleared until NSS is cleanly shutdown, which is not presently supported
-// in Chromium.
-// OSX 10.12+ stops building the chain at the first weak digest.
-#if defined(USE_NSS_CERTS) || defined(OS_IOS) || defined(OS_MACOSX)
-#define MAYBE_VerifyEndEntity DISABLED_VerifyEndEntity
-#else
-#define MAYBE_VerifyEndEntity VerifyEndEntity
-#endif
-WRAPPED_INSTANTIATE_TEST_CASE_P(MAYBE_VerifyEndEntity,
-                                CertVerifyProcWeakDigestTest,
-                                testing::ValuesIn(kVerifyEndEntityTestData));
-
-// Incomplete chains should still report the status of the intermediate.
-const WeakDigestTestData kVerifyIncompleteIntermediateTestData[] = {
-    {NULL, "weak_digest_md5_intermediate.pem", "weak_digest_sha1_ee.pem",
-     EXPECT_MD5 | EXPECT_SHA1 | EXPECT_SHA1_LEAF},
-#if defined(USE_OPENSSL_CERTS) || defined(OS_WIN)
-    // MD4 is not supported by OS X / NSS
-    {NULL, "weak_digest_md4_intermediate.pem", "weak_digest_sha1_ee.pem",
-     EXPECT_MD4 | EXPECT_SHA1 | EXPECT_SHA1_LEAF},
-#endif
-    {NULL, "weak_digest_md2_intermediate.pem", "weak_digest_sha1_ee.pem",
-     EXPECT_MD2 | EXPECT_SHA1 | EXPECT_SHA1_LEAF},
-};
-// Disabled on NSS - libpkix does not return constructed chains on error,
-// preventing us from detecting/inspecting the verified chain.
-#if defined(USE_NSS_CERTS) || defined(OS_IOS)
-#define MAYBE_VerifyIncompleteIntermediate \
-    DISABLED_VerifyIncompleteIntermediate
-#else
-#define MAYBE_VerifyIncompleteIntermediate VerifyIncompleteIntermediate
-#endif
-WRAPPED_INSTANTIATE_TEST_CASE_P(
-    MAYBE_VerifyIncompleteIntermediate,
-    CertVerifyProcWeakDigestTest,
-    testing::ValuesIn(kVerifyIncompleteIntermediateTestData));
-
-// Incomplete chains should still report the status of the end-entity.
-const WeakDigestTestData kVerifyIncompleteEETestData[] = {
-  { NULL, "weak_digest_sha1_intermediate.pem", "weak_digest_md5_ee.pem",
-    EXPECT_MD5 | EXPECT_SHA1 },
-#if defined(USE_OPENSSL_CERTS) || defined(OS_WIN)
-  // MD4 is not supported by OS X / NSS
-  { NULL, "weak_digest_sha1_intermediate.pem", "weak_digest_md4_ee.pem",
-    EXPECT_MD4 | EXPECT_SHA1 },
-#endif
-  { NULL, "weak_digest_sha1_intermediate.pem", "weak_digest_md2_ee.pem",
-    EXPECT_MD2 | EXPECT_SHA1 },
-};
-// Disabled on NSS - libpkix does not return constructed chains on error,
-// preventing us from detecting/inspecting the verified chain.
-// OSX 10.12+ stops building the chain at the first weak digest.
-#if defined(USE_NSS_CERTS) || defined(OS_IOS) || defined(OS_MACOSX)
-#define MAYBE_VerifyIncompleteEndEntity DISABLED_VerifyIncompleteEndEntity
-#else
-#define MAYBE_VerifyIncompleteEndEntity VerifyIncompleteEndEntity
-#endif
-WRAPPED_INSTANTIATE_TEST_CASE_P(
-    MAYBE_VerifyIncompleteEndEntity,
-    CertVerifyProcWeakDigestTest,
-    testing::ValuesIn(kVerifyIncompleteEETestData));
-
-// Differing algorithms between the intermediate and the EE should still be
-// reported.
-const WeakDigestTestData kVerifyMixedTestData[] = {
-  { "weak_digest_sha1_root.pem", "weak_digest_md5_intermediate.pem",
-    "weak_digest_md2_ee.pem", EXPECT_MD2 | EXPECT_MD5 },
-  { "weak_digest_sha1_root.pem", "weak_digest_md2_intermediate.pem",
-    "weak_digest_md5_ee.pem", EXPECT_MD2 | EXPECT_MD5 },
-#if defined(USE_OPENSSL_CERTS) || defined(OS_WIN)
-  // MD4 is not supported by OS X / NSS
-  { "weak_digest_sha1_root.pem", "weak_digest_md4_intermediate.pem",
-    "weak_digest_md2_ee.pem", EXPECT_MD2 | EXPECT_MD4 },
-#endif
-};
-// NSS does not support MD4 and does not enable MD2 by default, making all
-// permutations invalid.
-// OSX 10.12+ stops building the chain at the first weak digest.
-#if defined(USE_NSS_CERTS) || defined(OS_IOS) || defined(OS_MACOSX)
-#define MAYBE_VerifyMixed DISABLED_VerifyMixed
-#else
-#define MAYBE_VerifyMixed VerifyMixed
-#endif
-WRAPPED_INSTANTIATE_TEST_CASE_P(
-    MAYBE_VerifyMixed,
-    CertVerifyProcWeakDigestTest,
-    testing::ValuesIn(kVerifyMixedTestData));
+                        testing::ValuesIn(kVerifyWeakSignatureData));
 
 // For the list of valid hostnames, see
 // net/cert/data/ssl/certificates/subjectAltName_sanity_check.pem
@@ -1948,10 +1798,9 @@ TEST_P(CertVerifyProcNameTest, VerifyCertName) {
   }
 }
 
-WRAPPED_INSTANTIATE_TEST_CASE_P(
-    VerifyName,
-    CertVerifyProcNameTest,
-    testing::ValuesIn(kVerifyNameData));
+INSTANTIATE_TEST_CASE_P(VerifyName,
+                        CertVerifyProcNameTest,
+                        testing::ValuesIn(kVerifyNameData));
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
 // Test that CertVerifyProcMac reacts appropriately when Apple's certificate