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..a4a495c1142c9a81c2cfbdc60c94c2665fa731c7 100644
--- a/chromeos/cert_loader.cc
+++ b/chromeos/cert_loader.cc
@@ -9,6 +9,7 @@
 
 #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/threading/worker_pool.h"
@@ -20,6 +21,47 @@
 
 namespace chromeos {
 
+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(), NULL));

[emaxx, 2017/04/24 21:23:13]

nit: Maybe use nullptr? (Sorry for proposing this only now.)

[pmarko, 2017/04/25 12:10:02]

Done. (whole file)

+  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) {
+      PK11_FreeSlotListElement(slots_for_cert.get(), slot_element);

[emaxx, 2017/04/24 21:23:13]

nit: Maybe drop an explanatory comment here? Something like that (please correct
if it's wrong and/or feel free to rephrase it as you wish):
// Free the remaining list elements. All previously visited items have
// been freed by PK11_GetNextSafe().

[pmarko, 2017/04/25 12:10:02]

Done. (please see if my comment is good or too much detail)

+      return true;
+    }
+  }
+  return false;
+}
+
+// Goes through all certificates in |cert_list| and copies those certificates
+// which are on |system_slot| to |system_cert_list|.
+void FilterSystemTokenCertificates(const net::CertificateList* cert_list,
+                                   net::CertificateList* system_cert_list,
+                                   crypto::ScopedPK11Slot system_slot) {
+  VLOG(1) << "FilterSystemTokenCertificates";
+  if (!system_slot)
+    return;
+  // Extract certificates which are in the system token into the
+  // system_cert_list_ sublist.
+  for (auto cert : *cert_list) {
+    if (IsCertificateOnSlot(cert.get(), system_slot.get())) {
+      system_cert_list->push_back(cert);
+    }
+  }
+}
+
+}  // namespace
+
 static CertLoader* g_cert_loader = NULL;
 static bool g_force_hardware_backed_for_test = false;
 
@@ -96,6 +138,11 @@ bool CertLoader::CertificatesLoading() const {
   return database_ && !certificates_loaded_;
 }
 
+void CertLoader::SetSlowTaskRunnerForTest(
+    const scoped_refptr<base::TaskRunner>& task_runner) {
+  slow_task_runner_for_test_ = task_runner;
+}
+
 // static
 void CertLoader::ForceHardwareBackedForTesting() {
   g_force_hardware_backed_for_test = true;
@@ -148,17 +195,37 @@ 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::UpdateCertificates(
+void CertLoader::CertificatesLoaded(
     std::unique_ptr<net::CertificateList> cert_list) {
   CHECK(thread_checker_.CalledOnValidThread());
+  VLOG(1) << "CertificatesLoaded: " << cert_list->size();
+
+  crypto::ScopedPK11Slot system_slot = database_->GetSystemSlot();
+  std::unique_ptr<net::CertificateList> system_cert_list =
+      base::MakeUnique<net::CertificateList>();
+  GetSlowTaskRunner()->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(
+          &FilterSystemTokenCertificates, base::Unretained(cert_list.get()),
+          base::Unretained(system_cert_list.get()), base::Passed(&system_slot)),
+      base::Bind(&CertLoader::UpdateCertificates, weak_factory_.GetWeakPtr(),
+                 base::Passed(&cert_list), base::Passed(&system_cert_list)));
+}
+
+void CertLoader::UpdateCertificates(
+    std::unique_ptr<net::CertificateList> cert_list,
+    std::unique_ptr<net::CertificateList> system_cert_list) {
+  CHECK(thread_checker_.CalledOnValidThread());
   DCHECK(certificates_update_running_);
-  VLOG(1) << "UpdateCertificates: " << cert_list->size();
+  VLOG(1) << "UpdateCertificates: " << cert_list->size() << " ("
+          << system_cert_list->size() << " on system slot)";
 
   // Ignore any existing certificates.
   cert_list_ = std::move(cert_list);
+  system_cert_list_ = std::move(system_cert_list);
 
   bool initial_load = !certificates_loaded_;
   certificates_loaded_ = true;
@@ -179,4 +246,10 @@ void CertLoader::OnCertDBChanged() {
   LoadCertificates();
 }
 
+scoped_refptr<base::TaskRunner> CertLoader::GetSlowTaskRunner() const {
+  if (slow_task_runner_for_test_.get())
+    return slow_task_runner_for_test_;
+  return base::WorkerPool::GetTaskRunner(true /*task is slow*/);

[emaxx, 2017/04/24 21:23:13]

I'm concerned about using WorkerPool. It's used only in a small number of call
sites, and looks like it's being deprecated: http://crbug.com/659191

I believe you may use the following tools (though I may be wrong here):
* PostTaskWithTraitsAndReply - with, I guess, the following traits: WithWait(),
WithPriority(BACKGROUND), WithShutdownBehavior(CONTINUE_ON_SHUTDOWN);
* ScopedTaskScheduler for tests.

[pmarko, 2017/04/25 12:10:02]

Good find, thanks! I used WorkerPool because that's what other NSS-using places
use. I've checked why and stumbled upon
https://codereview.chromium.org/2722733002 and
https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/SubA-WOEB18.

IIUC, the main concern is that if chrome calls a NSS function which requires the
user to login to a token, NSS uses the callback set by PK11_SetPasswordFunc to
perform the password request. In chrome, this invokes UI, so it might do things
asynchronously and use TaskScheduler. If TaskScheduler can't grow its task pool,
it might be starved and cause a deadlock.

I think that this is not an issue for our callsite because we don't do
operations which would require a token auth (esp. no private key access and no
access to new tokens). So switching to TaskScheduler.

WDYT?

[emaxx, 2017/04/25 15:15:58]

Yes, I think the remaining uses of WorkerPool are there because of the tasks
that may involve the user gesture and are therefore unbound in the execution
time.
I agree that it shouldn't be an issue for this code (and maybe even never for
Chrome OS specific code).

+}
+
 }  // namespace chromeos