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..c6ab7e5af97a0e0b01bfb90129983b84457a515e 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,91 @@ bool ParseResourceType(const std::string& type_str,
   return found;
 }
 
+static base::DictionaryValue* ExtractDN(const net::CertPrincipal& dn) {

[palmer, 2017/01/31 05:42:31]

Are |ExtractDN| and |ExtractCertificateInfo| part of the public API? (Judging
from the .h file, it seems like not?) If not, put them in the anonymous
namespace block at the top of this file. That's the equivalent of declaring them
static in C, allowing the compiler to easily see the functions are private to
this file. Various optimizations may ensue.

[Ryan Sleevi, 2017/01/31 21:37:56]

I'm not supportive of exposing this information at all

1) It's accessible via JS for those implementations that need (e.g. asn1js),
thus indicating its polyfillability does not require a specific platform
exposure
2) It introduces a dependency on net::CertPrincipal, which we (//net) folks
don't want.
3) It exposes names 'as if' theres only one possible value for a given field.

So this would nuke the entire field

+  auto* dn_dict = new base::DictionaryValue();
+  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();

[palmer, 2017/01/31 05:42:31]

I'd say it's best to be consistent about using/not using auto for declarations
where it's appropriate. Since you use it on line 1285 now, I'd suggest using it
here and on line 1305 and wherever else.

+    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(

[palmer, 2017/01/31 05:42:31]

I'm not sure if it's correct to use smart pointers here? I see the 2 call sites
of this function are used to put a value in a ListValue. At that point, the
ListValue ('s owner) owns the pointers, and will free the memory when the
ListValue goes away. 

+    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(keys::kSubjectKey, ExtractDN(cert->subject()));
+  info->Set(keys::kIssuerKey, ExtractDN(cert->issuer()));
+
+  std::vector<std::string> dns_names;
+  std::vector<std::string> ip_addrs;
+  cert->GetSubjectAltName(&dns_names, &ip_addrs);
+  if (dns_names.size() > 0) {
+    base::ListValue* names = new base::ListValue();

[palmer, 2017/01/31 05:42:31]

Could use auto here, too, and elsewhere.

+    names->AppendStrings(dns_names);
+    info->Set(keys::kDNSNamesKey, names);
+  }
+  if (ip_addrs.size() > 0) {
+    base::ListValue* addrs = new base::ListValue();
+    addrs->AppendStrings(ip_addrs);
+    info->Set(keys::kIPAddressesKey, addrs);
+  }
+
+  info->SetBoolean(keys::kExpiredKey, cert->HasExpired());
+  info->SetDouble(keys::kNotBeforeKey, cert->valid_start().ToJsTime());
+  info->SetDouble(keys::kNotAfterKey, cert->valid_expiry().ToJsTime());
+
+  std::string der_holder;
+  if (!cert->GetDEREncoded(cert->os_cert_handle(), &der_holder))
+    return info;
+  info->Set(keys::kRawKey, base::BinaryValue::CreateWithCopiedBuffer(
+                               der_holder.c_str(), der_holder.size()));

[palmer, 2017/01/31 05:42:31]

Is this formatting the result of `git cl format`? (Maybe it is, I dunno.) Make
sure to run that before submitting the CL, in any case.

[Ryan Sleevi, 2017/01/31 21:37:56]

The only field I'm supportive of exposing is the raw DER. Everything else
can/should be left up to JS/extensions handling this as appropriate, but we
shouldn't impose a particular format or parsing cost on them.

+
+  return info;
+}
+
+base::ListValue* ExtractCertificateChain(
+    scoped_refptr<net::X509Certificate> cert) {
+  auto* chain = new base::ListValue();
+  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));

[Ryan Sleevi, 2017/01/31 21:37:56]

This is a pattern that we've explicitly tried to discourage //net consumers from
- creating net::X509Certificates from intermediates. In light of the above
comments, this is hopefully entirely unnecessary.

+    }
+  }
+  return chain;
+}
+
 }  // namespace extension_web_request_api_helpers