Enable device-wide EAP-TLS networks

chromeos/cert_loader.cc

Index: chromeos/cert_loader.cc
diff --git a/chromeos/cert_loader.cc b/chromeos/cert_loader.cc
index 9d9a4a90f95903d0d792f0af14f9f027a0ca561a..35ff399b2ba83606bcece2814010e8d897eed9b0 100644
--- a/chromeos/cert_loader.cc
+++ b/chromeos/cert_loader.cc
@@ -14,12 +14,123 @@
 #include "base/task_scheduler/post_task.h"
 #include "crypto/nss_util.h"
 #include "crypto/scoped_nss_types.h"
+#include "net/cert/cert_database.h"
 #include "net/cert/nss_cert_database.h"
 #include "net/cert/nss_cert_database_chromeos.h"
 #include "net/cert/x509_certificate.h"
 
 namespace chromeos {
 
+// Caches certificates from a NSSCertDatabase. Handles reloading of certificates
+// on update notifications and provides status flags (loading / loaded).
+// CertLoader can use multiple CertCaches to combine certificates from multiple
+// sources.
+class CertLoader::CertCache : public net::CertDatabase::Observer {
+ public:
+  explicit CertCache(base::RepeatingClosure certificates_updated_callback)
+      : certificates_updated_callback_(certificates_updated_callback),
+        cert_list_(base::MakeUnique<net::CertificateList>()),
+        weak_factory_(this) {}
+
+  ~CertCache() override {
+    net::CertDatabase::GetInstance()->RemoveObserver(this);
+  }
+
+  void StartWithNSSDB(net::NSSCertDatabase* nss_database) {
+    CHECK(!nss_database_);

[emaxx, 2017/05/11 02:57:46]

nit: #include "base/logging.h"

[pmarko, 2017/05/11 11:49:18]

Done.

+    nss_database_ = nss_database;
+
+    // Start observing cert database for changes.
+    // Observing net::CertDatabase is preferred over observing |nss_database_|
+    // directly, as |nss_database_| observers receive only events generated
+    // directly by |nss_database_|, so they may miss a few relevant ones.
+    // TODO(tbarzic): Once singleton NSSCertDatabase is removed, investigate if
+    // it would be OK to observe |nss_database_| directly; or change
+    // NSSCertDatabase to send notification on all relevant changes.
+    net::CertDatabase::GetInstance()->AddObserver(this);
+
+    LoadCertificates();
+  }
+
+  net::NSSCertDatabase* nss_database() { return nss_database_; }
+
+  // Trigger a certificate load. If a certificate loading task is already in
+  // progress, will start a reload once the current task is finished.
+  void LoadCertificates() {

[emaxx, 2017/05/11 02:57:46]

nit: Make private?

[pmarko, 2017/05/11 11:49:18]

Done.

+    CHECK(thread_checker_.CalledOnValidThread());
+    VLOG(1) << "LoadCertificates: " << certificates_update_running_;
+
+    if (certificates_update_running_) {
+      certificates_update_required_ = true;
+      return;
+    }
+
+    certificates_update_running_ = true;
+    certificates_update_required_ = false;
+
+    if (nss_database_) {
+      has_system_certificates_ =
+          static_cast<bool>(nss_database_->GetSystemSlot());
+      nss_database_->ListCerts(base::Bind(&CertCache::UpdateCertificates,
+                                          weak_factory_.GetWeakPtr()));
+    }
+  }
+
+  // Called if a certificate load task is finished.
+  void UpdateCertificates(std::unique_ptr<net::CertificateList> cert_list) {

[emaxx, 2017/05/11 02:57:46]

nit: Make private?

[pmarko, 2017/05/11 11:49:18]

Done.

+    CHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(certificates_update_running_);
+    VLOG(1) << "UpdateCertificates: " << cert_list->size();
+
+    // Ignore any existing certificates.
+    cert_list_ = std::move(cert_list);
+
+    certificates_loaded_ = true;
+    certificates_updated_callback_.Run();
+
+    certificates_update_running_ = false;
+    if (certificates_update_required_)
+      LoadCertificates();
+  }
+
+  // net::CertDatabase::Observer
+  void OnCertDBChanged() override {
+    VLOG(1) << "OnCertDBChanged";
+    LoadCertificates();
+  }
+
+  const net::CertificateList& cert_list() const { return *cert_list_; }
+
+  bool CertificatesLoading() const {

[emaxx, 2017/05/11 02:57:46]

nit: Rename to lowercase, as it's a simple accessor just like the following two
ones?

[pmarko, 2017/05/11 11:49:18]

Good point. I've chosen "initial_load_running" to make the name reflect reality.
Also renamed in CertLoader:: to initial_load_of_any_database_running(). Done.

+    return nss_database_ && !certificates_loaded_;
+  }
+
+  bool certificates_loaded() const { return certificates_loaded_; }
+  bool has_system_certificates() const { return has_system_certificates_; }
+
+ private:
+  // To be called when certificates have been updated.
+  base::RepeatingClosure certificates_updated_callback_;
+
+  bool has_system_certificates_ = false;
+
+  // Flags describing current CertLoader state.

[emaxx, 2017/05/11 02:57:46]

nit: Remove the reference to the CertLoader class.
(And I'd prefer having more detailed comment for each of these flags.)

[pmarko, 2017/05/11 11:49:18]

Done. Also renamed certificates_loaded_ to initial_load_finished_.

+  bool certificates_loaded_ = false;
+  bool certificates_update_required_ = false;
+  bool certificates_update_running_ = false;
+
+  // The user-specific NSS certificate database from which the certificates

[emaxx, 2017/05/11 02:57:46]

nit: The "user-specific" characteristic is not actual anymore.

[pmarko, 2017/05/11 11:49:18]

Done.

+  // should be loaded.
+  net::NSSCertDatabase* nss_database_ = nullptr;
+
+  // Cached Certificates loaded from the database.
+  std::unique_ptr<net::CertificateList> cert_list_;

[emaxx, 2017/05/11 02:57:46]

Is the smart pointer actually needed there?

[pmarko, 2017/05/11 11:49:18]

Hrm. NSSCertDatabase->ListCerts calls its callback with
std::unique_ptr<net::CertificateList>[1], which is where this originates from.
I'm not sure if we should prefer moving from the vector pointed to there:
cert_list_ = std::move(*cert_list_ptr);
It should work but looks a bit weird. WDYT?


[1]
https://cs.chromium.org/chromium/src/net/cert/nss_cert_database.h?l=92&gs=cpp...

[emaxx, 2017/05/11 14:36:52]

I'm not insisting, but to me ridding of these smart pointers would make it all a
bit clearer.
Yes, you'd have to move from unique_ptr<CertificateList> in that single place
(using the technique that you showed - which looks fine to me), and then you'd
be able to use move semantics with CertificateList directly.

BTW, I believe they're gradually switching to the move semantics in the
net::-related code, see e.g. this recent change:
https://chromium.googlesource.com/chromium/src/+/7ed243fc89fb72230c47f5974212...

+
+  base::ThreadChecker thread_checker_;
+
+  base::WeakPtrFactory<CertCache> weak_factory_;
+};

[emaxx, 2017/05/11 02:57:46]

nit: DISALLOW_COPY_AND_ASSIGN (for consistency).

[pmarko, 2017/05/11 11:49:18]

Done.

+
 namespace {
 
 // Checks if |certificate| is on the given |slot|.
@@ -97,31 +208,27 @@ bool CertLoader::IsInitialized() {
 }
 
 CertLoader::CertLoader()
-    : certificates_loaded_(false),
-      certificates_update_required_(false),
-      certificates_update_running_(false),
-      database_(nullptr),
+    : pending_initial_load_(true),
+      system_cert_cache_(base::MakeUnique<CertCache>(
+          base::BindRepeating(&CertLoader::CacheUpdated,
+                              base::Unretained(this)))),
+      user_cert_cache_(base::MakeUnique<CertCache>(
+          base::BindRepeating(&CertLoader::CacheUpdated,
+                              base::Unretained(this)))),
       all_certs_(base::MakeUnique<net::CertificateList>()),
+      system_certs_(base::MakeUnique<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.
-  // Observing net::CertDatabase is preferred over observing |database_|
-  // directly, as |database_| observers receive only events generated directly
-  // by |database_|, so they may miss a few relevant ones.
-  // TODO(tbarzic): Once singleton NSSCertDatabase is removed, investigate if
-  // it would be OK to observe |database_| directly; or change NSSCertDatabase
-  // to send notification on all relevant changes.
-  net::CertDatabase::GetInstance()->AddObserver(this);
+void CertLoader::StartWithSystemNSSDB(
+    net::NSSCertDatabase* system_slot_database) {
+  system_cert_cache_->StartWithNSSDB(system_slot_database);
+}
 
-  LoadCertificates();
+void CertLoader::StartWithUserNSSDB(net::NSSCertDatabase* user_database) {
+  user_cert_cache_->StartWithNSSDB(user_database);
 }
 
 void CertLoader::AddObserver(CertLoader::Observer* observer) {
@@ -141,7 +248,13 @@ bool CertLoader::IsCertificateHardwareBacked(const net::X509Certificate* cert) {
 }
 
 bool CertLoader::CertificatesLoading() const {
-  return database_ && !certificates_loaded_;
+  return system_cert_cache_->CertificatesLoading() ||
+         user_cert_cache_->CertificatesLoading();
+}
+
+bool CertLoader::certificates_loaded() const {
+  return system_cert_cache_->certificates_loaded() ||
+         user_cert_cache_->certificates_loaded();
 }
 
 // static
@@ -183,56 +296,54 @@ std::string CertLoader::GetPkcs11IdAndSlotForCert(
   return pkcs11_id;
 }
 
-void CertLoader::LoadCertificates() {
+void CertLoader::CacheUpdated() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  VLOG(1) << "LoadCertificates: " << certificates_update_running_;
-
-  if (certificates_update_running_) {
-    certificates_update_required_ = true;
-    return;
+  VLOG(1) << "CacheUpdated";
+
+  if (user_cert_cache_->has_system_certificates()) {
+    // The user's cert cache also contains system certificates.
+    // Filter those.
+    crypto::ScopedPK11Slot system_slot =
+        user_cert_cache_->nss_database()->GetSystemSlot();
+    DCHECK(system_slot);
+    std::unique_ptr<net::CertificateList> all_certs =

[emaxx, 2017/05/11 02:57:46]

Can this logic lead to problems in the case when user_cert_cache_ hasn't loaded
yet? IIUC, the CertLoader will respond with an empty list of certs in this case,
despite that it could have returned non-empty list of system-slot certs before.

[pmarko, 2017/05/11 11:49:18]

Very good find! I'd say that this line can stay as it is and
has_system_certificates() should return true only if it has loaded certificates
_and_ has access to the system token.
Added a comment here and at the method and fixed the method.

The unittests didn't catch this because:
- CacheUpdated is called on the UI thread when a *_cert_cache_ has finished
loading certs
- If it originates from the user_cert_cache_, it will work
- It would only break if user_cert_cache_ is initialized and has not finished
its initial cert load, and at that time an update comes in from
system_cert_cache_.

+        base::MakeUnique<net::CertificateList>(user_cert_cache_->cert_list());
+    base::PostTaskWithTraitsAndReplyWithResult(
+        FROM_HERE,
+        {base::MayBlock(), base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+        base::BindOnce(&FilterSystemTokenCertificates,
+                       base::Unretained(all_certs.get()),
+                       std::move(system_slot)),
+        base::BindOnce(&CertLoader::UpdateCertificates,
+                       weak_factory_.GetWeakPtr(), std::move(all_certs)));
+  } else {
+    // The user's cert cache does not contain system certificates.
+    std::unique_ptr<net::CertificateList> system_certs =

[emaxx, 2017/05/11 02:57:46]

nit: BTW, wouldn't it be possible to drop the smart pointers here and from
UpdateCertificates, FilterSystemTokenCertificates, et al? Moving
std::unique_ptr's is not much better than moving std::vector's.

[pmarko, 2017/05/11 11:49:18]

The only part where this is actually useful is with the
FilterSystemTokenCertificates, UpdateCertificates PostTask.*ReplyWithResult
above:
- all_certs gets moved into the UpdateCertificates callback object and later
into UpdateCertificates.
- We can still use all_certs.get() to get a pointer to the same vector to pass
into FilterSystemTokenCertificates.

Doing this with plain std::vector would be hard (we'd have to make a copy). But
making a copy once might be more efficient than the unique_ptr indirection +
more readable. WDYT?

(BTW fixed UB above by moving the all_certs.get() to a separate statement - C++
parameter evaluation order is undefined).

+        base::MakeUnique<net::CertificateList>(system_cert_cache_->cert_list());
+    std::unique_ptr<net::CertificateList> all_certs =
+        base::MakeUnique<net::CertificateList>(user_cert_cache_->cert_list());
+    all_certs->insert(all_certs->end(), system_certs->begin(),
+                      system_certs->end());
+    UpdateCertificates(std::move(all_certs), std::move(system_certs));
   }
-
-  certificates_update_running_ = true;
-  certificates_update_required_ = false;
-
-  database_->ListCerts(
-      base::Bind(&CertLoader::CertificatesLoaded, weak_factory_.GetWeakPtr()));
-}
-
-void CertLoader::CertificatesLoaded(
-    std::unique_ptr<net::CertificateList> all_certs) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  VLOG(1) << "CertificatesLoaded: " << all_certs->size();
-
-  crypto::ScopedPK11Slot system_slot = database_->GetSystemSlot();
-  base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE,
-      {base::MayBlock(), base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
-      base::BindOnce(&FilterSystemTokenCertificates,
-                     base::Unretained(all_certs.get()), std::move(system_slot)),
-      base::BindOnce(&CertLoader::UpdateCertificates,
-                     weak_factory_.GetWeakPtr(), std::move(all_certs)));
 }
 
 void CertLoader::UpdateCertificates(
     std::unique_ptr<net::CertificateList> all_certs,
     std::unique_ptr<net::CertificateList> system_certs) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(certificates_update_running_);
+  bool initial_load = pending_initial_load_;
+  pending_initial_load_ = false;
+
   VLOG(1) << "UpdateCertificates: " << all_certs->size() << " ("
-          << system_certs->size() << " on system slot)";
+          << system_certs->size() << " on system slot)"
+          << ", initial_load=" << initial_load;
 
   // Ignore any existing certificates.
   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) {
@@ -240,9 +351,4 @@ void CertLoader::NotifyCertificatesLoaded(bool initial_load) {
     observer.OnCertificatesLoaded(*all_certs_, initial_load);
 }
 
-void CertLoader::OnCertDBChanged() {
-  VLOG(1) << "OnCertDBChanged";
-  LoadCertificates();
-}
-
 }  // namespace chromeos