Enable client certificate patterns in device ONC policy

chromeos/cert_loader.cc

 // Copyright (c) 2013 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 "chromeos/cert_loader.h"
 
 #include <algorithm>
 #include <utility>
 
 #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"
 #include "net/cert/nss_cert_database.h"
 #include "net/cert/nss_cert_database_chromeos.h"
 #include "net/cert/x509_certificate.h"
 
 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.

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

nit: My understanding was that the issue is not only with freeing the currrent
element ("the last one" as is said currently), but with all remaining elements
starting from |slot_element|? If so, then please adjust the comment accordingly.

[pmarko, 2017/04/25 16:59:57]

Actually:
- Each element in the list starts out with refcount=1 (PK11_AddSlotToList sets
it to 1 and is used by PK11_GetAllSlotsForCert)
- PK11_GetFirstSafe increases the refcount of list->head
- PK11_GetNextSafe increases the refcount of the next element (which is then
returned) and decreases the refcount of the passed element
- PK11_FreeSlotListElement indeed only decreases the refcount of the passed
element. It needs the list to acquire the list-level lock.
- When ScopedPK11SlotList goes out of scope, it calls PK11_FreeSlotList which
actually iterates the whole list and decreases the refcount of each element,
reaching 0.

So, it seems that the semantics are that:
- the existence of the list increases the refcount of each element (+1)
- the element in the current iteration has a further increased refcount (+2).
This is probably done to support "find and return" loops where a matched element
is required to survive after exiting the loop.
See https://github.com/servo/nss/blob/master/lib/pk11wrap/pk11slot.c for more
details.

So, all in all, this is pretty confusing but I think the comment is correct.

+      // 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
 void CertLoader::Initialize() {
   CHECK(!g_cert_loader);
   g_cert_loader = new CertLoader();
 }
 
 // static
 void CertLoader::Shutdown() {
   CHECK(g_cert_loader);
   delete g_cert_loader;
-  g_cert_loader = NULL;
+  g_cert_loader = nullptr;
 }
 
 // static
 CertLoader* CertLoader::Get() {
   CHECK(g_cert_loader) << "CertLoader::Get() called before Initialize()";
   return g_cert_loader;
 }
 
 // static
 bool CertLoader::IsInitialized() {
   return g_cert_loader;
 }
 
 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);
 }
 
 void CertLoader::StartWithNSSDB(net::NSSCertDatabase* database) {
   CHECK(!database_);
   database_ = database;
 
   // Start observing cert database for changes.
@@ skipping 41 matching lines @@
 // NOTE: This function relies on the convention that the same PKCS#11 ID
 // is shared between a certificate and its associated private and public
 // keys.  I tried to implement this with PK11_GetLowLevelKeyIDForCert(),
 // but that always returns NULL on Chrome OS for me.
 std::string CertLoader::GetPkcs11IdAndSlotForCert(
     const net::X509Certificate& cert,
     int* slot_id) {
   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();
 
   *slot_id = static_cast<int>(PK11_GetSlotID(priv_key->pkcs11Slot));
 
   // Get the CKA_ID attribute for a key.
   SECItem* sec_item = PK11_GetLowLevelKeyIDForPrivateKey(priv_key);
   std::string pkcs11_id;
   if (sec_item) {
     pkcs11_id = base::HexEncode(sec_item->data, sec_item->len);
@@ skipping 10 matching lines @@
 
   if (certificates_update_running_) {
     certificates_update_required_ = true;
     return;
   }
 
   certificates_update_running_ = true;
   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());
+  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(
+      FROM_HERE,
+      base::TaskTraits()
+          .WithShutdownBehavior(

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

I've used the same traits that were used in the referenced CL. Do you think we
should explicitly set the priority? It seems that the default priority would be
USER_VISIBLE which might be true if the user is waiting for the certs to get
loaded on the wifi connect dialog or for the network to start connecting (i.e.
for ClientCertResolver to do its work). On other cases, BACKGROUND would be
appropriate, but we don't know.

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

Maybe you're right; to me the difference between these task priorities is a bit
too vague.

I think you may want to involve some of the corresponding OWNERS (maybe
fdoray@).

[pmarko, 2017/04/25 16:59:57]

@fdoray: Could you give advice here? We have a task which in some circumstances
is user-visible (e.g. networks needing client certificates will only connect
after this has been done). Is the default priority fine for this?

+              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
+          .MayBlock(),
+      base::Bind(
+          &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());
   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;
   NotifyCertificatesLoaded(initial_load);
 
   certificates_update_running_ = false;
   if (certificates_update_required_)
     LoadCertificates();
 }
 
 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() {
   VLOG(1) << "OnCertDBChanged";
   LoadCertificates();
 }
 
 }  // namespace chromeos