Update CertVerifier::Verify to use RequestParams instead

net/cert/cert_verifier.cc

Index: net/cert/cert_verifier.cc
diff --git a/net/cert/cert_verifier.cc b/net/cert/cert_verifier.cc
index c308e93ebbdd61bd7d7c9f17c26302e06a706f62..3077588f45ac31d87bcfd39758a0a96077ba76ba 100644
--- a/net/cert/cert_verifier.cc
+++ b/net/cert/cert_verifier.cc
@@ -4,11 +4,13 @@
 
 #include "net/cert/cert_verifier.h"
 
+#include <openssl/sha.h>
+
 #include <algorithm>
 #include <memory>
 
 #include "base/memory/ptr_util.h"
-#include "base/sha1.h"
+#include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "net/cert/cert_verify_proc.h"
 
@@ -21,25 +23,39 @@
 namespace net {
 
 CertVerifier::RequestParams::RequestParams(
-    X509Certificate* certificate,
+    scoped_refptr<X509Certificate> certificate,
     const std::string& hostname,
     int flags,
     const std::string& ocsp_response,
-    const CertificateList& additional_trust_anchors)
-    : hostname_(hostname), flags_(flags) {
-  // Rather than store all of the original data, create a fingerprint based
-  // on the hash of the request data.
-  SHA1HashValue ocsp_hash;
-  base::SHA1HashBytes(
-      reinterpret_cast<const unsigned char*>(ocsp_response.data()),
-      ocsp_response.size(), ocsp_hash.data);
-
-  request_data_.reserve(additional_trust_anchors.size() + 3);
-  request_data_.push_back(ocsp_hash);
-  request_data_.push_back(certificate->fingerprint());
-  request_data_.push_back(certificate->ca_fingerprint());
-  for (const auto& trust_anchor : additional_trust_anchors)
-    request_data_.push_back(trust_anchor->fingerprint());
+    CertificateList additional_trust_anchors)
+    : certificate_(std::move(certificate)),
+      hostname_(hostname),
+      flags_(flags),
+      ocsp_response_(ocsp_response),
+      additional_trust_anchors_(std::move(additional_trust_anchors)) {
+  // For efficiency sake, rather than compare all of the fields for each

[eroman, 2016/05/20 00:41:18]

Have you confirmed this is worth doing with benchmarks?

Given that all the data is being kept alive now, would read more naturally to
compare fields directly.

[Ryan Sleevi, 2016/05/20 02:39:43]

It should be obviously beneficial; computing the DER involves additional copies,
and comparing the certs requires memcmp'ing the entire certificate - meaning
that, say, comparing two certificate chains of 3certs each would be a 12K vs 12K
comparison (with no cache locality) vs the 32-byte comparison here.
We don't have lexicographical comparison for certificates, intentionally.

[Ryan Sleevi, 2016/05/20 06:27:36]

On 2016/05/20 02:39:43, Ryan Sleevi wrote:
 > > Given that all the data is being kept alive now, would read more naturally
to
Turns out we do (X509Certificate::LessThan), and beyond being in tests, it's
being used in the memoizing cert store.

I'd like to get rid of it, but in the fingerprint code, I tightened it up.
However, it's still expensive (full comparison, not hash comparison), and it
seems like comparing on the hashes of the parameters here will still be a
net-win for perf.

But I'm really, really torn on this still. To gain the simplicity here, that is:

if (flags != other.flags)
  return flags < other.flags;
if (hostname != other.hostname)
  return hostname < other.hostname;
if (ocsp_response != other.ocsp_response)
  return ocsp_response < other.ocsp_response;
if (certificate != other.certificate)
  return X509Certificate::LessThan()(certificate, other.certificate);
return std::lexicographical_compare(additional_trust_anchors_.begin(),
additional_trust_anchors_.end(), other.additional_trust_anchors_.begin(),
other.additional_trust_anchors_.end(), X509Certificate::LessThan());

We'd more than likely need to reintroduce the fingerprint_ and ca_fingerprint_
members to X509Certificate, and rewrite X509Certificate::LessThan to be

if (cert.fingerprint_ != other.cert.fingerprint_)
  return cert.fingerprint_ < other.cert.fingerprint_;
if (cert.ca_fingerprint_ != other.cert.ca_fingerprint_)
  return cert.ca_fingerprint_ < other.cert.ca_fingerprint_;
if (cert_der != other.cert_der)
  return cert_der < other.cert_der;
return ca_der < other.ca_der;

And that seems.... bleh.

But I can be swayed, now that you can see the other CL for context.

+  // comparison, compute a hash of their values. This is done directly in
+  // this class, rather than as an overloaded hash operator, for efficiency's
+  // sake.
+  SHA256_CTX ctx;
+  SHA256_Init(&ctx);
+  std::string cert_der;
+  X509Certificate::GetDEREncoded(certificate_->os_cert_handle(), &cert_der);
+  SHA256_Update(&ctx, cert_der.data(), cert_der.size());
+  for (const auto& cert_handle : certificate_->GetIntermediateCertificates()) {

[eroman, 2016/05/20 00:41:18]

Do you expect to separately change the fingerprint and ca_fingerprint() in
X509Certificate to use SHA256?

[Ryan Sleevi, 2016/05/20 02:39:43]

No, I intend to remove those.

+    X509Certificate::GetDEREncoded(cert_handle, &cert_der);
+    SHA256_Update(&ctx, cert_der.data(), cert_der.size());
+  }
+  SHA256_Update(&ctx, hostname_.data(), hostname.size());
+  SHA256_Update(&ctx, &flags, sizeof(flags));
+  SHA256_Update(&ctx, ocsp_response.data(), ocsp_response.size());
+  for (const auto& trust_anchor : additional_trust_anchors_) {
+    X509Certificate::GetDEREncoded(trust_anchor->os_cert_handle(), &cert_der);
+    SHA256_Update(&ctx, cert_der.data(), cert_der.size());
+  }
+  SHA256_Final(reinterpret_cast<uint8_t*>(
+                   base::WriteInto(&key_, SHA256_DIGEST_LENGTH + 1)),
+               &ctx);
 }
 
 CertVerifier::RequestParams::RequestParams(const RequestParams& other) =
@@ -48,13 +64,7 @@ CertVerifier::RequestParams::~RequestParams() {}
 
 bool CertVerifier::RequestParams::operator<(
     const CertVerifier::RequestParams& other) const {
-  if (flags_ != other.flags_)
-    return flags_ < other.flags_;
-  if (hostname_ != other.hostname_)
-    return hostname_ < other.hostname_;
-  return std::lexicographical_compare(
-      request_data_.begin(), request_data_.end(), other.request_data_.begin(),
-      other.request_data_.end(), SHA1HashValueLessThan());
+  return key_ < other.key_;
 }
 
 bool CertVerifier::SupportsOCSPStapling() {