Split cert reporter class into report building/serializing and sending

chrome/browser/ssl/certificate_error_report.cc

Index: chrome/browser/ssl/certificate_error_report.cc
diff --git a/chrome/browser/ssl/certificate_error_report.cc b/chrome/browser/ssl/certificate_error_report.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bd0b6bd5c91ab624c0a0c0fb5512f96835758f6b
--- /dev/null
+++ b/chrome/browser/ssl/certificate_error_report.cc
@@ -0,0 +1,52 @@
+// Copyright 2015 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 "chrome/browser/ssl/certificate_error_report.h"
+
+#include <vector>
+
+#include "base/stl_util.h"
+#include "base/time/time.h"
+#include "net/cert/x509_certificate.h"
+#include "net/ssl/ssl_info.h"
+
+namespace chrome_browser_ssl {
+
+CertificateErrorReport::CertificateErrorReport() {
+}
+
+CertificateErrorReport::CertificateErrorReport(const std::string& hostname,
+                                               const net::SSLInfo& ssl_info) {
+  base::Time now = base::Time::Now();
+  cert_report_.set_time_usec(now.ToInternalValue());

[eroman, 2015/05/12 00:27:51]

Is it the case that base::Time's internal value uses the same epoch on all
platforms?

I vaguely seem to remember we changed base::Time on Windows to use the unix
epoch too, but please confirm.

[estark, 2015/05/12 20:42:16]

It looks like it actually uses the Windows epoch on all platforms, IIUC. e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time_pos...

+  cert_report_.set_hostname(hostname);

[eroman, 2015/05/12 00:27:51]

is the assumption that hostname will be ASCII?

[estark, 2015/05/12 20:42:16]

I'm not sure... I think in practice only ASCII will get passed in here, but I
don't see that anything would break if it were non-ASCII. Are you suggesting
that we should document this function as taking an ASCII-only |hostname|?

+
+  std::vector<std::string> pem_encoded_chain;
+  if (!ssl_info.cert->GetPEMEncodedChain(&pem_encoded_chain))
+    LOG(ERROR) << "Could not get PEM encoded chain.";

[eroman, 2015/05/12 00:27:51]

nit: Maybe I am just paranoid, but anytime I see macros not surrounded by braces
I get scared. Feel free to do nothing if the expansion is safe, but I suggest
putting curlies around this single-line if

[estark, 2015/05/12 20:42:16]

Done.

+
+  std::string* cert_chain = cert_report_.mutable_cert_chain();
+  for (size_t i = 0; i < pem_encoded_chain.size(); ++i)
+    *cert_chain += pem_encoded_chain[i];

[eroman, 2015/05/12 00:27:51]

would std::string::append() be better than += ?

Probably won't make any real difference, but I suspect += will be allocating
more objects.

Could get all crazy with reserve() and the like, but probably not worth it. Will
leave this up to your judgement based on the expected sizes and frequency of
use.

[estark, 2015/05/12 20:42:16]

Changed to use std::string::append() but I don't think it's worth trying to
optimize any further than that. This code only runs occasionally (on SSL
interstitials or pinning violations on Google properties).

+
+  cert_report_.add_pin(ssl_info.pinning_failure_log);
+}
+
+CertificateErrorReport::~CertificateErrorReport() {
+}
+
+bool CertificateErrorReport::InitializeFromString(
+    const std::string& serialized_report) {
+  return cert_report_.ParseFromString(serialized_report);
+}
+
+void CertificateErrorReport::Serialize(std::string* output) const {
+  cert_report_.SerializeToString(output);

[eroman, 2015/05/12 00:27:51]

This can return a failure. Why is it not necessary to check?

[estark, 2015/05/12 20:42:16]

Oh I didn't realize it returned a value. Changed this method to return a bool.

+}
+
+const std::string& CertificateErrorReport::hostname() const {
+  return cert_report_.hostname();
+}
+
+}  // namespace chrome_browser_ssl