Re-enable CertVerifyProcInternalTest.PublicKeyHashes.

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 86fce4961de1c3d301270a51722e43db87ea53a8..6d551815f601c82a4212f3aec5a269ae017cc600 100644
--- a/net/cert/cert_verify_proc_unittest.cc
+++ b/net/cert/cert_verify_proc_unittest.cc
@@ -732,15 +732,16 @@ TEST_P(CertVerifyProcInternalTest, DISABLED_TestKnownRoot) {
   int flags = 0;
   CertVerifyResult verify_result;
   // This will blow up, May 9th, 2016. Sorry! Please disable and file a bug
-  // against agl. See also PublicKeyHashes.
+  // against agl.
   int error = Verify(cert_chain.get(), "twitter.com", flags, NULL,
                      CertificateList(), &verify_result);
   EXPECT_THAT(error, IsOk());
   EXPECT_TRUE(verify_result.is_issued_by_known_root);
 }
 
-// TODO(crbug.com/610546): Fix and re-enable this test.
-TEST_P(CertVerifyProcInternalTest, DISABLED_PublicKeyHashes) {
+// This test uses a similar setup to VerifyReturnChainProperlyOrdered, however
+// verifies the public key hashes chain rather than the chain itself.

[Ryan Sleevi, 2017/02/03 23:54:21]

This isn't necessary to match that. We're really just trying to test that it
fills PublicKeyHashes with all the values.

[eroman, 2017/02/04 00:59:28]

Done.

+TEST_P(CertVerifyProcInternalTest, PublicKeyHashes) {
   if (!SupportsReturningVerifiedChain()) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
@@ -748,47 +749,57 @@ TEST_P(CertVerifyProcInternalTest, DISABLED_PublicKeyHashes) {
 
   base::FilePath certs_dir = GetTestCertsDirectory();
   CertificateList certs = CreateCertificateListFromFile(
-      certs_dir, "twitter-chain.pem", X509Certificate::FORMAT_AUTO);
+      certs_dir, "x509_verify_results.chain.pem", X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(3U, certs.size());
 
+  // Construct the chain out of order.

[Ryan Sleevi, 2017/02/03 23:54:21]

This isn't necessary for the test

[eroman, 2017/02/04 00:59:28]

Done.

   X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(certs[2]->os_cert_handle());
   intermediates.push_back(certs[1]->os_cert_handle());
 
+  ScopedTestRoot scoped_root(certs[2].get());
   scoped_refptr<X509Certificate> cert_chain = X509Certificate::CreateFromHandle(
       certs[0]->os_cert_handle(), intermediates);
+  ASSERT_TRUE(cert_chain);
+  ASSERT_EQ(2U, cert_chain->GetIntermediateCertificates().size());
+
   int flags = 0;
   CertVerifyResult verify_result;
-
-  // This will blow up, May 9th, 2016. Sorry! Please disable and file a bug
-  // against agl. See also TestKnownRoot.
-  int error = Verify(cert_chain.get(), "twitter.com", flags, NULL,
+  int error = Verify(cert_chain.get(), "127.0.0.1", flags, NULL,
                      CertificateList(), &verify_result);
   EXPECT_THAT(error, IsOk());
-  ASSERT_LE(3U, verify_result.public_key_hashes.size());
 
   HashValueVector sha1_hashes;
-  for (size_t i = 0; i < verify_result.public_key_hashes.size(); ++i) {
-    if (verify_result.public_key_hashes[i].tag != HASH_VALUE_SHA1)
-      continue;
-    sha1_hashes.push_back(verify_result.public_key_hashes[i]);
+  HashValueVector sha256_hashes;

[Ryan Sleevi, 2017/02/03 23:54:21]

concrete suggestion: vector<SHA1HashValue> and vector<SHA256HashValue> for these
will allow you to make use of vector==

[eroman, 2017/02/04 00:59:28]

no longer applicable

+  for (const auto& public_key_hash : verify_result.public_key_hashes) {
+    if (public_key_hash.tag == HASH_VALUE_SHA1) {
+      sha1_hashes.push_back(public_key_hash);
+    } else if (public_key_hash.tag == HASH_VALUE_SHA256) {
+      sha256_hashes.push_back(public_key_hash);
+    }
   }
-  ASSERT_LE(3u, sha1_hashes.size());
 
-  for (size_t i = 0; i < 3; ++i) {
-    EXPECT_EQ(HexEncode(kTwitterSPKIs[i], base::kSHA1Length),
-              HexEncode(sha1_hashes[i].data(), base::kSHA1Length));
-  }
+  const char* kExpectedSha1HashesHex[] = {
+      "7D2425F064E0A666AB93FF660CAF6ACC82067C51",
+      "EFED0CB34EE112401CEB354F3A8FAE2ED304C1F5",
+      "749C2F3B880454866FADEB40AC6C8136082396B4",
+  };
 
-  HashValueVector sha256_hashes;
-  for (size_t i = 0; i < verify_result.public_key_hashes.size(); ++i) {
-    if (verify_result.public_key_hashes[i].tag != HASH_VALUE_SHA256)
-      continue;
-    sha256_hashes.push_back(verify_result.public_key_hashes[i]);
+  const char* kExpectedSha256HashesHex[] = {
+      "E48E7EE277408700E259DD56A9F0600E4280008121B2AD0C7C0C76E47A1CF9D0",
+      "32D9EA81D4B00088048EC7BB4A9C679B22A8A3F45388BF420C85B0167CF89DAB",
+      "CFBC754B37ACF9E40EA89A7AAC12B7BBFB5032CE79158A2364750216205C8EE7",
+  };
+
+  ASSERT_EQ(arraysize(kExpectedSha1HashesHex), sha1_hashes.size());
+  for (size_t i = 0; i < sha1_hashes.size(); ++i) {
+    EXPECT_EQ(std::string(kExpectedSha1HashesHex[i]),
+              HexEncode(sha1_hashes[i].data(), base::kSHA1Length));
   }
-  ASSERT_LE(3u, sha256_hashes.size());
 
-  for (size_t i = 0; i < 3; ++i) {
-    EXPECT_EQ(HexEncode(kTwitterSPKIsSHA256[i], crypto::kSHA256Length),
+  ASSERT_EQ(arraysize(kExpectedSha256HashesHex), sha256_hashes.size());
+  for (size_t i = 0; i < sha256_hashes.size(); ++i) {
+    EXPECT_EQ(std::string(kExpectedSha256HashesHex[i]),
               HexEncode(sha256_hashes[i].data(), crypto::kSHA256Length));
   }

[Ryan Sleevi, 2017/02/03 23:54:21]

Interesting, the original test was relying on a (non-guaranteed) API behaviour,
which is that hashes are in a particular order (we don't presently guarantee
that, and every time I add a new search thing I keep wondering whether it's
worth paying the 1-time sort cost in CertVerifyProc and having the API guarantee
sort-stability

You can replace this with

EXPECT_THAT(sha256_hashes,
WhenSorted(UnorderedElementsAreArray(kExpectedSHA256Hashes)));

[eroman, 2017/02/04 00:59:28]

Done -- removed ordering requirement in test, and made some changes to the
documentation.

 }