Extend the webRequest.onCompleted event details object with TLS/SSL information

extensions/browser/api/web_request/web_request_api_helpers.cc

Index: extensions/browser/api/web_request/web_request_api_helpers.cc
diff --git a/extensions/browser/api/web_request/web_request_api_helpers.cc b/extensions/browser/api/web_request/web_request_api_helpers.cc
index bdd91f1dfe04310d620abafce152939a6acd4dcb..d1c9adb7f2c12c1079633ff047f583c7aa935823 100644
--- a/extensions/browser/api/web_request/web_request_api_helpers.cc
+++ b/extensions/browser/api/web_request/web_request_api_helpers.cc
@@ -28,6 +28,7 @@
 #include "extensions/browser/runtime_data.h"
 #include "extensions/browser/warning_set.h"
 #include "extensions/common/extension_messages.h"
+#include "net/cert/x509_certificate.h"
 #include "net/cookies/cookie_util.h"
 #include "net/cookies/parsed_cookie.h"
 #include "net/http/http_util.h"
@@ -36,6 +37,7 @@
 #include "net/log/net_log_event_type.h"
 #include "net/log/net_log_parameters_callback.h"
 #include "net/log/net_log_with_source.h"
+#include "net/ssl/ssl_info.h"
 #include "net/url_request/url_request.h"
 #include "url/url_constants.h"
 
@@ -1279,4 +1281,122 @@ bool ParseResourceType(const std::string& type_str,
   return found;
 }
 
+static base::DictionaryValue* ExtractDN(const net::CertPrincipal& dn) {
+  base::DictionaryValue* dn_dict = new base::DictionaryValue();

[palmer, 2017/01/21 01:16:30]

Nit/Note: In unambiguous situations like this, you can use auto:

  auto* dn_dict = new base::DictionaryValue();

but it's a matter of personal taste.

+  if (!dn.common_name.empty()) {
+    dn_dict->SetString(keys::kCommonNameKey, dn.common_name);
+  }
+  if (!dn.locality_name.empty()) {
+    dn_dict->SetString(keys::kLocalityNameKey, dn.locality_name);
+  }
+  if (!dn.state_or_province_name.empty()) {
+    dn_dict->SetString(keys::kStateOrProvinceNameKey,
+                       dn.state_or_province_name);
+  }
+  if (!dn.country_name.empty()) {
+    dn_dict->SetString(keys::kCountryNameKey, dn.country_name);
+  }
+  if (dn.street_addresses.size() > 0) {
+    base::ListValue* addrs = new base::ListValue();
+    addrs->AppendStrings(dn.street_addresses);
+    dn_dict->Set(keys::kStreetAddressesKey, addrs);
+  }
+  if (dn.organization_names.size() > 0) {
+    base::ListValue* names = new base::ListValue();
+    names->AppendStrings(dn.organization_names);
+    dn_dict->Set(keys::kOrganizationNamesKey, names);
+  }
+  if (dn.organization_unit_names.size() > 0) {
+    base::ListValue* names = new base::ListValue();
+    names->AppendStrings(dn.organization_unit_names);
+    dn_dict->Set(keys::kOrganizationUnitNamesKey, names);
+  }
+  return dn_dict;
+}
+
+std::unique_ptr<base::DictionaryValue> ExtractCertificateInfo(
+    scoped_refptr<net::X509Certificate> cert) {
+  std::unique_ptr<base::DictionaryValue> info(new base::DictionaryValue);
+  info->SetString(keys::kSerialNumberKey,
+                  base::HexEncode(cert->serial_number().data(),
+                                  cert->serial_number().size()));
+  info->Set("subject", ExtractDN(cert->subject()));

[palmer, 2017/01/21 01:16:29]

Is it better to use your keys::kFooKey constants for these?

+  info->Set("issuer", ExtractDN(cert->issuer()));
+
+  std::vector<std::string>* dns_names = new std::vector<std::string>;
+  std::vector<std::string>* ip_addrs = new std::vector<std::string>;

[palmer, 2017/01/21 01:16:29]

You leak these pointers — there's no "delete dns_names;" at the end of the
function.

The norm is:

  std::vector foo;
  // stuff
  return ...;

When |foo| goes out of scope, its destructor is called, and that involves
releasing any internally-managed storage.

Since you need pointers for cert->GetSubjectAltName, use the unary & operator:

  std::vector dns_names;
  std::vector ip_addresses;
  cert->GetSubjectAltName(&dns_names, &ip_addresses);

+  cert->GetSubjectAltName(dns_names, ip_addrs);
+  if (dns_names->size() > 0) {
+    base::ListValue* names = new base::ListValue();
+    names->AppendStrings(*dns_names);
+    info->Set("DNSNames", names);
+  }
+  if (ip_addrs->size() > 0) {
+    base::ListValue* addrs = new base::ListValue();
+    addrs->AppendStrings(*ip_addrs);
+    info->Set("IPAddresses", addrs);
+  }
+
+  info->SetBoolean("expired", cert->HasExpired());
+  info->SetDouble("notBefore", cert->valid_start().ToJsTime());
+  info->SetDouble("notAfter", cert->valid_expiry().ToJsTime());
+
+  std::string der_holder;
+  if (!cert->GetDEREncoded(cert->os_cert_handle(), &der_holder))
+    return info;
+  info->Set("raw", base::BinaryValue::CreateWithCopiedBuffer(
+                       der_holder.c_str(), der_holder.size()));
+
+  return info;
+}
+
+base::ListValue* ExtractCertificateChain(
+    scoped_refptr<net::X509Certificate> cert) {
+  base::ListValue* chain = new base::ListValue();

[palmer, 2017/01/21 01:16:29]

auto here too, if you like.

+  if (cert) {
+    chain->Append(ExtractCertificateInfo(cert));
+    const net::X509Certificate::OSCertHandles cert_handles =
+        cert->GetIntermediateCertificates();
+    const net::X509Certificate::OSCertHandles empty_handle;
+    for (size_t i = 0; i < cert_handles.size(); i++) {
+      scoped_refptr<net::X509Certificate> interCert;
+      interCert =
+          net::X509Certificate::CreateFromHandle(cert_handles[i], empty_handle);
+      chain->Append(ExtractCertificateInfo(interCert));
+    }
+  }
+  return chain;
+}
+
+static std::unordered_map<net::CertStatus, int> status_to_error_map = {

[palmer, 2017/01/21 01:16:29]

This creates a static initializer, which we don't like:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables.

A common idiom you'll see in Chromium code for things like this is:

BarEnum FooEnumToBarEnum(FooEnum foo) {
  switch (foo) {
    case BLAH:
      return BLORG;
    // ...
    default:
      NOTREACHED();
      return BAD;
  }
}

+    {net::CERT_STATUS_REVOKED, net::ERR_CERT_REVOKED},
+    {net::CERT_STATUS_INVALID, net::ERR_CERT_INVALID},
+    {net::CERT_STATUS_PINNED_KEY_MISSING,
+     net::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN},
+    {net::CERT_STATUS_AUTHORITY_INVALID, net::ERR_CERT_AUTHORITY_INVALID},
+    {net::CERT_STATUS_COMMON_NAME_INVALID, net::ERR_CERT_COMMON_NAME_INVALID},
+    {net::CERT_STATUS_NAME_CONSTRAINT_VIOLATION,
+     net::ERR_CERT_NAME_CONSTRAINT_VIOLATION},
+    {net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM,
+     net::ERR_CERT_WEAK_SIGNATURE_ALGORITHM},
+    {net::CERT_STATUS_WEAK_KEY, net::ERR_CERT_WEAK_KEY},
+    {net::CERT_STATUS_DATE_INVALID, net::ERR_CERT_DATE_INVALID},
+    {net::CERT_STATUS_VALIDITY_TOO_LONG, net::ERR_CERT_VALIDITY_TOO_LONG},
+    {net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION,
+     net::ERR_CERT_UNABLE_TO_CHECK_REVOCATION},
+    {net::CERT_STATUS_NO_REVOCATION_MECHANISM,
+     net::ERR_CERT_NO_REVOCATION_MECHANISM},
+    {net::CERT_STATUS_CERTIFICATE_TRANSPARENCY_REQUIRED,
+     net::ERR_CERTIFICATE_TRANSPARENCY_REQUIRED},
+};
+
+base::ListValue* ParseCertificateStatusErrors(net::CertStatus status) {
+  base::ListValue* errors = new base::ListValue();
+  for (auto const& error : status_to_error_map) {

[palmer, 2017/01/21 01:16:29]

I think the style guide calls for "const auto& error". Search other code to be
sure.

+    if (status & error.first)
+      errors->AppendString(net::ErrorToShortString(error.second));
+  }
+  return errors;
+}
+
 }  // namespace extension_web_request_api_helpers