Enable client certificate patterns in device ONC policy

chromeos/cert_loader.cc

Index: chromeos/cert_loader.cc
diff --git a/chromeos/cert_loader.cc b/chromeos/cert_loader.cc
index 4e4131bad953b82db4e1cc39368f2b6c38b2a9ed..e5429a987f5af7e27c6dbc3153a46cacfc9ae6d6 100644
--- a/chromeos/cert_loader.cc
+++ b/chromeos/cert_loader.cc
@@ -9,8 +9,9 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/task_runner_util.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/threading/worker_pool.h"
 #include "crypto/nss_util.h"
 #include "crypto/scoped_nss_types.h"
@@ -20,7 +21,52 @@
 
 namespace chromeos {
 
-static CertLoader* g_cert_loader = NULL;
+namespace {
+
+// Checks if |certificate| is on the given |slot|.
+bool IsCertificateOnSlot(const net::X509Certificate* certificate,
+                         PK11SlotInfo* slot) {
+  crypto::ScopedPK11SlotList slots_for_cert(
+      PK11_GetAllSlotsForCert(certificate->os_cert_handle(), nullptr));
+  if (!slots_for_cert)
+    return false;
+
+  for (PK11SlotListElement* slot_element =
+           PK11_GetFirstSafe(slots_for_cert.get());
+       slot_element; slot_element = PK11_GetNextSafe(slots_for_cert.get(),
+                                                     slot_element, PR_FALSE)) {
+    if (slot_element->slot == slot) {
+      // All previously visited elements have been freed by PK11_GetNextSafe,
+      // but we're not calling that for the last one, so free it explicitly.
+      // The slots_for_cert list itself will be freed because ScopedPK11SlotList
+      // is a unique_ptr.
+      PK11_FreeSlotListElement(slots_for_cert.get(), slot_element);
+      return true;
+    }
+  }
+  return false;
+}
+
+// Goes through all certificates in |all_certs| and copies those certificates
+// which are on |system_slot| to |system_certs|.
+void FilterSystemTokenCertificates(const net::CertificateList* all_certs,
+                                   net::CertificateList* system_certs,
+                                   crypto::ScopedPK11Slot system_slot) {
+  VLOG(1) << "FilterSystemTokenCertificates";
+  if (!system_slot)
+    return;
+  // Extract certificates which are in the system token into the
+  // system_certs_ sublist.
+  for (auto cert : *all_certs) {
+    if (IsCertificateOnSlot(cert.get(), system_slot.get())) {
+      system_certs->push_back(cert);
+    }
+  }
+}
+
+}  // namespace
+
+static CertLoader* g_cert_loader = nullptr;
 static bool g_force_hardware_backed_for_test = false;
 
 // static
@@ -33,7 +79,7 @@ void CertLoader::Initialize() {
 void CertLoader::Shutdown() {
   CHECK(g_cert_loader);
   delete g_cert_loader;
-  g_cert_loader = NULL;
+  g_cert_loader = nullptr;
 }
 
 // static
@@ -51,10 +97,9 @@ CertLoader::CertLoader()
     : certificates_loaded_(false),
       certificates_update_required_(false),
       certificates_update_running_(false),
-      database_(NULL),
-      cert_list_(new net::CertificateList),
-      weak_factory_(this) {
-}
+      database_(nullptr),
+      all_certs_(new net::CertificateList),
+      weak_factory_(this) {}
 
 CertLoader::~CertLoader() {
   net::CertDatabase::GetInstance()->RemoveObserver(this);
@@ -116,8 +161,8 @@ std::string CertLoader::GetPkcs11IdAndSlotForCert(
   DCHECK(slot_id);
 
   CERTCertificateStr* cert_handle = cert.os_cert_handle();
-  SECKEYPrivateKey *priv_key =
-      PK11_FindKeyByAnyCert(cert_handle, NULL /* wincx */);
+  SECKEYPrivateKey* priv_key =
+      PK11_FindKeyByAnyCert(cert_handle, nullptr /* wincx */);
   if (!priv_key)
     return std::string();
 
@@ -148,17 +193,41 @@ void CertLoader::LoadCertificates() {
   certificates_update_required_ = false;
 
   database_->ListCerts(
-      base::Bind(&CertLoader::UpdateCertificates, weak_factory_.GetWeakPtr()));
+      base::Bind(&CertLoader::CertificatesLoaded, weak_factory_.GetWeakPtr()));
+}
+
+void CertLoader::CertificatesLoaded(
+    std::unique_ptr<net::CertificateList> all_certs) {
+  CHECK(thread_checker_.CalledOnValidThread());

[fdoray, 2017/04/25 17:40:24]

DCHECK?

[pmarko, 2017/04/26 08:01:29]

Good question. The rest of cert_loader.cc used CHECK for CalledOnValidThread
before this CL so I tended to stick to that, but:
- codesearch for [^D]CHECK\(.*\.CalledOnValidThread\(\)\) gives 10 results 
- codesearch for DCHECK\(.*\.CalledOnValidThread\(\)\) gives 730 results
I'll switch cert_loader.cc to go with DCHECK.
Done.

+  VLOG(1) << "CertificatesLoaded: " << all_certs->size();
+
+  crypto::ScopedPK11Slot system_slot = database_->GetSystemSlot();
+  std::unique_ptr<net::CertificateList> system_certs =
+      base::MakeUnique<net::CertificateList>();
+  base::PostTaskWithTraitsAndReply(

[fdoray, 2017/04/25 17:40:24]

You could return a std::unique_ptr<net::CertificateList> in
FilterSystemTokenCertificates and use base::PostTaskWithTraitsAndReplyWithResult
to automatically bind the return value of the first callback to the second
callback.

std::unique_ptr<net::CertificateList>
    FilterSystemTokenCertificates(
        const net::CertificateList* all_certs,
        crypto::ScopedPK11Slot system_slot) {
  auto system_certs = base::MakeUnique<net::CertificateList>();
  ...
  return system_certs;
}

base::PostTaskWithTraitsAndReplyWithResult(
    FROM_HERE,
    base::TaskTraits()...,
    base::BindOnce(
        &FilterSystemTokenCertificates,
        base::Unretained(all_certs.get()),
        base::Passed(&system_slot)),
    base::BindOnce(
        &CertLoader::UpdateCertificates,
        weak_factory_.GetWeakPtr(),
        base::Passed(&all_certs)));

[pmarko, 2017/04/26 08:01:29]

Done. - Good idea, I've also changed base::Passed to std::move because IIUC that
seems to be the preferred way to do it with BindOnce [1] and it looks like
standard C++11. 

[1]
https://groups.google.com/a/chromium.org/forum/#!msg/cxx/KXWZtoP5HJ4/5TRPU81Z...

[pmarko, 2017/04/26 11:34:17]

Also, could you give me a recommendation about using the default priority vs.
specifying a priority explicitly? I would tend to use USER_VISIBLE if specifying
explicitly. This is called on the UI thread in real chrome (mentioning this
because the default priority seems to be thread-local). What is the usecase for
"default priority"?

[fdoray, 2017/04/26 13:11:35]

The default priority (inherited) should be used most of the time. It allows code
to be reused in multiple contexts. 

The USER_BLOCKING and USER_VISIBLE priority should be set explicitly in code
that is explicitly responding to a user action or updating the UI. It doesn't
seem to be the case here.

The BACKGROUND priority should be set explicitly when you know that you are
doing something that the user is not waiting on. E.g. Reporting metrics, writing
data on disk, downloading an update.

+      FROM_HERE,
+      base::TaskTraits()
+          .WithShutdownBehavior(
+              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
+          .MayBlock(),
+      base::Bind(

[fdoray, 2017/04/25 17:40:24]

base::Bind -> base::BindOnce
https://chromium.googlesource.com/chromium/src/+/87356e8607d5c60651e22ab2ed40...

[pmarko, 2017/04/26 08:01:29]

Done.

+          &FilterSystemTokenCertificates, base::Unretained(all_certs.get()),
+          base::Unretained(system_certs.get()), base::Passed(&system_slot)),
+      base::Bind(&CertLoader::UpdateCertificates, weak_factory_.GetWeakPtr(),
+                 base::Passed(&all_certs), base::Passed(&system_certs)));
 }
 
 void CertLoader::UpdateCertificates(
-    std::unique_ptr<net::CertificateList> cert_list) {
+    std::unique_ptr<net::CertificateList> all_certs,
+    std::unique_ptr<net::CertificateList> system_certs) {
   CHECK(thread_checker_.CalledOnValidThread());

[fdoray, 2017/04/25 17:40:24]

DCHECK?

[pmarko, 2017/04/26 08:01:29]

Done.

   DCHECK(certificates_update_running_);
-  VLOG(1) << "UpdateCertificates: " << cert_list->size();
+  VLOG(1) << "UpdateCertificates: " << all_certs->size() << " ("
+          << system_certs->size() << " on system slot)";
 
   // Ignore any existing certificates.
-  cert_list_ = std::move(cert_list);
+  all_certs_ = std::move(all_certs);
+  system_certs_ = std::move(system_certs);
 
   bool initial_load = !certificates_loaded_;
   certificates_loaded_ = true;
@@ -171,7 +240,7 @@ void CertLoader::UpdateCertificates(
 
 void CertLoader::NotifyCertificatesLoaded(bool initial_load) {
   for (auto& observer : observers_)
-    observer.OnCertificatesLoaded(*cert_list_, initial_load);
+    observer.OnCertificatesLoaded(*all_certs_, initial_load);
 }
 
 void CertLoader::OnCertDBChanged() {