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

extensions/browser/api/web_request/web_request_api_helpers.cc

 // Copyright (c) 2012 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 "extensions/browser/api/web_request/web_request_api_helpers.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 10 matching lines @@
 #include "components/web_cache/browser/web_cache_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
 #include "extensions/browser/api/web_request/web_request_api_constants.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extensions_browser_client.h"
 #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"
 #include "net/log/net_log.h"
 #include "net/log/net_log_capture_mode.h"
 #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"
 
 // TODO(battre): move all static functions into an anonymous namespace at the
 // top of this file.
 
 using base::Time;
 using content::ResourceType;
 using net::cookie_util::ParsedRequestCookie;
 using net::cookie_util::ParsedRequestCookies;
@@ skipping 1223 matching lines @@
   bool found = false;
   for (size_t i = 0; i < kResourceTypeStringsLength; ++i) {
     if (type_str == kResourceTypeStrings[i]) {
       found = true;
       types->push_back(kResourceTypeValues[i]);
     }
   }
   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