Cert - protobufs to serialize and deserialize CertVerifierCache.

net/extras/cert/cert_verifier_cache_persister.cc

Index: net/extras/cert/cert_verifier_cache_persister.cc
diff --git a/net/extras/cert/cert_verifier_cache_persister.cc b/net/extras/cert/cert_verifier_cache_persister.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7304a7fdfee7d219b73f5635ffdc92642dfd0060
--- /dev/null
+++ b/net/extras/cert/cert_verifier_cache_persister.cc
@@ -0,0 +1,336 @@
+// Copyright (c) 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/extras/cert/cert_verifier_cache_persister.h"
+
+#include <vector>
+
+#include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/strings/string_piece.h"
+#include "base/time/time.h"
+#include "net/base/hash_value.h"
+#include "net/cert/caching_cert_verifier.h"
+#include "net/cert/cert_type.h"
+#include "net/cert/cert_verify_result.h"
+#include "net/cert/x509_cert_types.h"
+#include "net/cert/x509_certificate.h"
+#include "net/extras/cert/proto/cert_verification.pb.h"
+
+namespace net {
+
+namespace {
+
+typedef std::vector<std::string> CertVector;
+typedef std::map<std::string, size_t> EncodedCertMap;
+
+bool SerializeCertHandle(const X509Certificate::OSCertHandle& cert_handle,
+                         CertVector* serialized_certs,
+                         EncodedCertMap* encoded_certs,

[Ryan Sleevi, 2016/06/09 22:38:41]

DESIGN: This seems like a memory-intensive serialization pattern, because you
end up keeping a copy of |encoded| in both |serialized_certs| and
|encoded_certs| (and their names aren't very helpful).

Why not simply remove |serialized_certs| entirely:

auto result = encoded_certs->insert({ encoded, encoded_certs->size() + 1});
return result.first->second;

In case it's not obvious, if |encoded| already exists in the map, then |result|
will return the iterator to that as part of the first item. If it doesn't, it'll
be added - with a value of encoded_certs->size() + 1. In either event, you'll
get an iterator back in result.first, and then you can get the number by doing
result.first->second;

[ramant (doing other things), 2016/06/10 06:13:51]

Excellent point. Fixed it.

Done.

+                         size_t* cert_number) {
+  std::string encoded;
+  if (!X509Certificate::GetDEREncoded(cert_handle, &encoded)) {
+    NOTREACHED();
+    return false;
+  }
+  EncodedCertMap::const_iterator it = encoded_certs->find(encoded);

[Ryan Sleevi, 2016/06/09 22:38:41]

Improve documentation

For example:
// Determine if |cert_handle| was already serialized. If so, simply
// return a reference to that entry. Otherwise, add a new entry to
// the set of certs to be serialized (|serialized_certs|).

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+  if (it != encoded_certs->end()) {
+    *cert_number = it->second;
+  } else {
+    serialized_certs->push_back(encoded);
+    *cert_number = serialized_certs->size() - 1;
+    encoded_certs->insert({encoded, *cert_number});
+  }
+  return true;
+}
+
+// Update |proto_certificate| with certificate number and updates
+// |serialized_certs| with DER-encoded representation of certificate if the
+// certicate is not in |serialized_certs|. Returns true if data is serialized
+// correctly.
+bool SerializeCertificate(const scoped_refptr<X509Certificate>& certificate,

[Ryan Sleevi, 2016/06/09 22:38:42]

Pass as a naked pointer here since you're not retaining references, per
https://www.chromium.org/developers/smart-pointer-guidelines

That is, X509Certificate*

[ramant (doing other things), 2016/06/10 06:13:52]

Done.

+                          CertVector* serialized_certs,
+                          EncodedCertMap* encoded_certs,
+                          CertVerificationCertificate* proto_certificate) {
+  size_t cert_number = 0;
+  if (!SerializeCertHandle(certificate->os_cert_handle(), serialized_certs,
+                           encoded_certs, &cert_number)) {
+    NOTREACHED();
+    return false;
+  }
+  proto_certificate->add_cert_numbers(cert_number);
+  const X509Certificate::X509Certificate::OSCertHandles& intermediate_ca_certs =
+      certificate->GetIntermediateCertificates();
+  for (size_t i = 0; i < intermediate_ca_certs.size(); ++i) {
+    if (!SerializeCertHandle(intermediate_ca_certs[i], serialized_certs,
+                             encoded_certs, &cert_number)) {
+      NOTREACHED();
+      return false;
+    }
+    proto_certificate->add_cert_numbers(cert_number);
+  }
+  return true;
+}
+
+// Returns deserialized certificate with the deserialized data from
+// |proto_certificate|.

[Ryan Sleevi, 2016/06/09 22:38:41]

This comment can be substantially improved

// Deserializes |proto_certificate| using the certificate database
// provided in |deserialized_der_certs|. Returns the parsed certificate
// on success, or nullptr if deserialization failed.

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+scoped_refptr<X509Certificate> DeserializeCertificate(
+    const CertVerificationCertificate& proto_certificate,
+    const CertVector& deserialized_der_certs) {
+  if (0 == proto_certificate.cert_numbers_size())
+    return nullptr;
+  std::vector<base::StringPiece> der_cert_pieces(
+      proto_certificate.cert_numbers_size());
+  for (int i = 0; i < proto_certificate.cert_numbers_size(); i++) {
+    size_t cert_number = proto_certificate.cert_numbers(i);
+    if (cert_number < deserialized_der_certs.size()) {
+      der_cert_pieces[i] =
+          base::StringPiece(deserialized_der_certs[cert_number]);
+    } else {
+      return nullptr;

[Ryan Sleevi, 2016/06/09 22:38:41]

Do error handling first

if (cert_number >= deserialized_der_certs.size())
  return nullptr;
der_cert_pieces[i] = ...

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+    }
+  }
+  return X509Certificate::CreateFromDERCertChain(der_cert_pieces);
+}
+
+// Update |proto_request_param| with RequestParams data from |params|.

[Ryan Sleevi, 2016/06/09 22:38:41]

Again, documentation can be improved.

// Serializes |params| into |proto_request_param|, updating |encoded_certs|
// with the set of raw certificates that will be needed to deserialize
// the certificate in |proto_request_param| via DeserializeCertificate().

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+bool SerializeRequestParams(
+    const CertVerifier::RequestParams& params,
+    CertVector* serialized_certs,
+    EncodedCertMap* encoded_certs,
+    CertVerificationRequestParams* proto_request_param) {
+  CertVerificationCertificate* proto_certificate =
+      proto_request_param->mutable_certificate();
+  if (!SerializeCertificate(params.certificate(), serialized_certs,
+                            encoded_certs, proto_certificate)) {
+    NOTREACHED();
+    return false;
+  }
+  proto_request_param->set_hostname(params.hostname());
+  proto_request_param->set_flags(params.flags());
+  proto_request_param->set_ocsp_response(params.ocsp_response());
+  for (const scoped_refptr<X509Certificate>& cert :
+       params.additional_trust_anchors()) {
+    proto_certificate = proto_request_param->add_additional_trust_anchors();
+    if (!SerializeCertificate(cert, serialized_certs, encoded_certs,
+                              proto_certificate)) {
+      NOTREACHED();
+      return false;
+    }
+  }
+  return true;
+}
+
+// Update |proto_cached_result| with CachedResult data from |cache_iterator|.
+// |serialized_certs| contains a list of unique DER-encoded representation of
+// certificates.

[Ryan Sleevi, 2016/06/09 22:38:42]

See above for how this wording can be improved.

[ramant (doing other things), 2016/06/10 06:13:52]

Done.

+bool SerializeCachedResult(CachingCertVerifier::Iterator& cache_iterator,
+                           CertVector* serialized_certs,
+                           EncodedCertMap* encoded_certs,
+                           CertVerificationCachedResult* proto_cached_result) {
+  proto_cached_result->set_error(cache_iterator.error());
+
+  // Serialize CertVerifyResult.
+  const CertVerifyResult& result = cache_iterator.verify_result();
+
+  CertVerificationResult* proto_result = proto_cached_result->mutable_result();
+  CertVerificationCertificate* proto_certificate =
+      proto_result->mutable_verified_cert();
+  if (!SerializeCertificate(result.verified_cert, serialized_certs,
+                            encoded_certs, proto_certificate)) {
+    NOTREACHED();
+    return false;
+  }
+  proto_result->set_cert_status(result.cert_status);
+  proto_result->set_has_md2(result.has_md2);
+  proto_result->set_has_md4(result.has_md4);
+  proto_result->set_has_md5(result.has_md5);
+  proto_result->set_has_sha1(result.has_sha1);
+  proto_result->set_has_sha1_leaf(result.has_sha1_leaf);
+  for (const HashValue& value : result.public_key_hashes)
+    proto_result->add_public_key_hashes(value.ToString());
+  proto_result->set_is_issued_by_known_root(result.is_issued_by_known_root);
+  proto_result->set_is_issued_by_additional_trust_anchor(
+      result.is_issued_by_additional_trust_anchor);
+  proto_result->set_common_name_fallback_used(result.common_name_fallback_used);
+  return true;
+}
+
+// Update |error| and |result| with deserialized CachedResult data from
+// |proto_cached_result|. Returns true if it is deserialized correctly.
+bool DeserializeCachedResult(
+    const CertVerificationCachedResult& proto_cached_result,
+    const CertVector& deserialized_der_certs,
+    int* error,
+    CertVerifyResult* result) {
+  if (!proto_cached_result.has_error() || !proto_cached_result.has_result()) {
+    return false;
+  }

[Ryan Sleevi, 2016/06/09 22:38:41]

no braces on single-line conditionals

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+
+  const CertVerificationResult& proto_result = proto_cached_result.result();
+  if (!proto_result.has_verified_cert() || !proto_result.has_cert_status()) {

[Ryan Sleevi, 2016/06/09 22:38:41]

no braces on single-line conditionals

[ramant (doing other things), 2016/06/10 06:13:52]

Done.

+    return false;
+  }
+
+  *error = proto_cached_result.error();
+
+  result->verified_cert = DeserializeCertificate(proto_result.verified_cert(),
+                                                 deserialized_der_certs);
+  if (!result->verified_cert || !result->verified_cert.get()) {

[Ryan Sleevi, 2016/06/09 22:38:41]

no braces on single-line conditionals

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+    return false;
+  }
+
+  for (int i = 0; i < proto_result.public_key_hashes_size(); ++i) {
+    const ::std::string& public_key_hash = proto_result.public_key_hashes(i);

[Ryan Sleevi, 2016/06/09 22:38:41]

Don't do ::std, just do std::

[ramant (doing other things), 2016/06/10 06:13:52]

Done.

+    HashValue hash;
+    if (!hash.FromString(public_key_hash))
+      return false;
+    result->public_key_hashes.push_back(hash);
+  }
+  result->cert_status = proto_result.cert_status();
+  result->has_md2 = proto_result.has_md2();
+  result->has_md4 = proto_result.has_md4();
+  result->has_md5 = proto_result.has_md5();
+  result->has_sha1 = proto_result.has_sha1();
+  result->has_sha1_leaf = proto_result.has_sha1_leaf();
+  result->is_issued_by_known_root = proto_result.is_issued_by_known_root();
+  result->is_issued_by_additional_trust_anchor =
+      proto_result.is_issued_by_additional_trust_anchor();
+  result->common_name_fallback_used = proto_result.common_name_fallback_used();
+  return true;
+}
+
+}  // namespace
+
+CertVerificationCache SerializeCertVerifierCache(
+    const CachingCertVerifier& verifier) {
+  base::TimeTicks start_time(base::TimeTicks::Now());
+  CertVerificationCache proto_cert_cache;
+  CertVector serialized_certs;
+  EncodedCertMap encoded_certs;
+
+  CachingCertVerifier::Iterator cache_iterator(verifier);
+  for (; cache_iterator.HasNext(); cache_iterator.Advance()) {
+    CertVerificationCacheEntry* proto_cache_entry =
+        proto_cert_cache.add_cache_entry();
+
+    CertVerificationRequestParams* proto_request_param =
+        proto_cache_entry->mutable_request_params();
+    if (!SerializeRequestParams(cache_iterator.params(), &serialized_certs,
+                                &encoded_certs, proto_request_param)) {
+      NOTREACHED();
+      continue;
+    }
+
+    CertVerificationCachedResult* proto_cached_result =
+        proto_cache_entry->mutable_cached_result();
+    if (!SerializeCachedResult(cache_iterator, &serialized_certs,
+                               &encoded_certs, proto_cached_result)) {
+      NOTREACHED();
+      continue;
+    }
+
+    proto_cache_entry->set_verification_time(
+        cache_iterator.verification_time().ToInternalValue());
+  }
+  for (const std::string& cert : serialized_certs)
+    proto_cert_cache.add_certs(cert);
+
+  UMA_HISTOGRAM_TIMES("Net.CertVerifierCachePersister.SerializeTime",
+                      base::TimeTicks::Now() - start_time);

[Ryan Sleevi, 2016/06/09 22:38:41]

Again, I think this is the wrong layer for histograming

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+  return proto_cert_cache;
+}
+
+bool DeserializeCertVerifierCache(const CertVerificationCache& proto_cert_cache,
+                                  CachingCertVerifier* verifier) {
+  base::TimeTicks load_cache_start_time = base::TimeTicks::Now();
+  CertVector deserialized_der_certs;
+
+  if (proto_cert_cache.certs_size() == 0u ||
+      proto_cert_cache.cache_entry_size() == 0u) {
+    return false;
+  }
+
+  for (int i = 0; i < proto_cert_cache.certs_size(); ++i)

[Ryan Sleevi, 2016/06/09 22:38:42]

Seems like you can improve the comments throughout this function.

[ramant (doing other things), 2016/06/10 06:13:52]

Done.

+    deserialized_der_certs.push_back(proto_cert_cache.certs(i));
+
+  bool detected_corrupted_data = false;
+  for (int i = 0; i < proto_cert_cache.cache_entry_size(); ++i) {
+    const CertVerificationCacheEntry& cache_entry =
+        proto_cert_cache.cache_entry(i);
+    if (!cache_entry.has_request_params() ||
+        !cache_entry.has_verification_time() ||
+        !cache_entry.has_cached_result()) {
+      detected_corrupted_data = true;

[Ryan Sleevi, 2016/06/09 22:38:41]

DESIGN: Why do you continue deserializing once data is corrupted?
NAMING: "detected_" seems superfluous here - that is, detection is implied by a
variable existing.

bool data_corrupted = false;

Seems similarly expressive and clear (if you decide to keep it)

[ramant (doing other things), 2016/06/10 06:13:51]

If an entry is corrupted, ignore that entry and continue to the next one.

Changed it to return (and not continue with deserialization) if an error is
detected.
Deleted this variable.

Changed the code to return if there is an error.

+      continue;
+    }
+
+    const CertVerificationRequestParams& proto_request_params =
+        cache_entry.request_params();
+
+    if (!proto_request_params.has_certificate() ||
+        !proto_request_params.has_hostname() ||
+        proto_request_params.hostname().empty() ||
+        !proto_request_params.has_flags() ||
+        !proto_request_params.has_ocsp_response()) {
+      detected_corrupted_data = true;
+      continue;
+    }
+
+    scoped_refptr<X509Certificate> certificate = DeserializeCertificate(
+        proto_request_params.certificate(), deserialized_der_certs);
+    if (!certificate || !certificate.get()) {

[Ryan Sleevi, 2016/06/09 22:38:42]

!certificate.get() is redundant with !certificate - they do the same thing

if (!certificate)
  return false;

(or update data_corrupted)

[ramant (doing other things), 2016/06/10 06:13:52]

Done.

+      detected_corrupted_data = true;
+      continue;
+    }
+
+    std::string hostname(proto_request_params.hostname());

[Ryan Sleevi, 2016/06/09 22:38:41]

Why make a copy like this? Or with line 290? Why not just pass in straight from
the Protobuf and avoid the extra copy?

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+    int flags = proto_request_params.flags();
+    std::string ocsp_response(proto_request_params.ocsp_response());
+    CertificateList additional_trust_anchors;
+    bool cert_error = false;
+    for (int i = 0; i < proto_request_params.additional_trust_anchors_size();
+         ++i) {
+      const CertVerificationCertificate& proto_certificate =
+          proto_request_params.additional_trust_anchors(i);
+      scoped_refptr<X509Certificate> cert =
+          DeserializeCertificate(proto_certificate, deserialized_der_certs);
+      if (!cert || !cert.get()) {

[Ryan Sleevi, 2016/06/09 22:38:41]

See above about .get()

[ramant (doing other things), 2016/06/10 06:13:52]

Done.

+        cert_error = true;
+        break;
+      }
+      additional_trust_anchors.push_back(cert);
+    }
+    if (cert_error) {
+      detected_corrupted_data = true;
+      continue;
+    }
+
+    CertVerifier::RequestParams params(certificate, hostname, flags,
+                                       ocsp_response, additional_trust_anchors);
+
+    base::Time verification_time =
+        base::Time::FromInternalValue(cache_entry.verification_time());
+
+    const CertVerificationCachedResult& proto_cached_result =
+        cache_entry.cached_result();
+    CertVerifyResult result;
+    int error;
+    if (!DeserializeCachedResult(proto_cached_result, deserialized_der_certs,
+                                 &error, &result)) {
+      detected_corrupted_data = true;
+      continue;
+    }
+
+    if (!verifier->AddEntry(params, error, result, verification_time)) {
+      detected_corrupted_data = true;

[Ryan Sleevi, 2016/06/09 22:38:42]

BUG: AddEntry failing does not mean data was corrupt. It means the Verifier
might be full or have a fresher entry.

[ramant (doing other things), 2016/06/10 06:13:52]

Done.

+      continue;
+    }
+  }
+  UMA_HISTOGRAM_TIMES("Net.CertVerifierCachePersister.DeserializeTime",
+                      base::TimeTicks::Now() - load_cache_start_time);

[Ryan Sleevi, 2016/06/09 22:38:41]

Ditto

[ramant (doing other things), 2016/06/10 06:13:51]

Done.

+  return !detected_corrupted_data;
+}
+
+}  // namespace net