Enable device-wide EAP-TLS networks

chromeos/cert_loader.h

Index: chromeos/cert_loader.h
diff --git a/chromeos/cert_loader.h b/chromeos/cert_loader.h
index 7faedbf7b609b3c8479851be850371866ec53ead..443f037e997cbb6cd474b08961cdb430269fd388 100644
--- a/chromeos/cert_loader.h
+++ b/chromeos/cert_loader.h
@@ -31,13 +31,19 @@ namespace chromeos {
 // When certificates have been loaded (after login completes and tpm token is
 // initialized), or the cert database changes, observers are called with
 // OnCertificatesLoaded().
-class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
+// This class supports using one or two cert databases. The expected usage is
+// that CertLoader is used with a NSSCertDatabase backed by the system token
+// before user sign-in, and additionally with a user-specific NSSCertDatabase
+// after user sign-in. When both NSSCertDatabase are used, CertLoader combines
+// certificates from both into |all_certs()|.
+class CHROMEOS_EXPORT CertLoader {
  public:
   class Observer {
    public:
     // Called when the certificates, passed for convenience as |all_certs|,
     // have completed loading. |initial_load| is true the first time this
-    // is called.
+    // is called. It will be false if this is called because another slot has
+    // been added to CertLoader's data sources.
     virtual void OnCertificatesLoaded(const net::CertificateList& all_certs,
                                       bool initial_load) = 0;
 
@@ -64,12 +70,21 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   static std::string GetPkcs11IdAndSlotForCert(const net::X509Certificate& cert,
                                                int* slot_id);
 
-  // Starts the CertLoader with the NSS cert database.
+  // Starts the CertLoader with the passed system NSS cert database.
+  // The CertLoader will _not_ take ownership of the database - see comment on
+  // StartWithUserNSSDB.
+  // CertLoader supports working with only one database or with both (system and
+  // user) databases.
+  void StartWithSystemNSSDB(net::NSSCertDatabase* system_slot_database);

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

nit: I think the "StartWith..." naming scheme becomes a bit misleading, as it
suggests that only a single call of one of them will happen. Maybe
"StartObservingSystemNSSDB"/"StartObservingUserNSSDB", or simple
"SetSystemNSSDB"/"SetUserNSSDB"?

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

You're absolutely right. Let's go with Set<type>NSSDB.
Done.

+
+  // Starts the CertLoader with the passed user NSS cert database.
   // The CertLoader will _not_ take the ownership of the database, but it
   // expects it to stay alive at least until the shutdown starts on the main
-  // thread. This assumes that |StartWithNSSDB| and other methods directly
+  // thread. This assumes that |StartWithUserNSSDB| and other methods directly
   // using |database_| are not called during shutdown.
-  void StartWithNSSDB(net::NSSCertDatabase* database);
+  // CertLoader supports working with only one database or with both (system and
+  // user) databases.
+  void StartWithUserNSSDB(net::NSSCertDatabase* user_database);
 
   void AddObserver(CertLoader::Observer* observer);
   void RemoveObserver(CertLoader::Observer* observer);
@@ -81,7 +96,10 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   // Returns true when the certificate list has been requested but not loaded.

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

nit: Please update the comment to also explain the semantics of this method when
there are two databases.

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

Also renamed to initial_load_of_any_database_running() as discussed below. Done.

   bool CertificatesLoading() const;
 
-  bool certificates_loaded() const { return certificates_loaded_; }
+  // Returns true if any certificates have been loaded. If CertLoader uses a
+  // system and a user nss database, this returns true after the certificates

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

nit: s/nss/NSS/

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

Done.

+  // from the first (usually system) database have been loaded.
+  bool certificates_loaded() const;

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

nit: This name also becomes slightly contradicting with its actual meaning.
What about "initial_certificates_load_finished"? (is this option technically
correct?)

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

Done.
Technically correct. I've decided to use "initial_load_finished" for brevity -
WDYT?

 
   // Returns all certificates. This will be empty until certificates_loaded() is
   // true.
@@ -102,15 +120,14 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   static void ForceHardwareBackedForTesting();
 
  private:
-  CertLoader();
-  ~CertLoader() override;
+  class CertCache;
 
-  // 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();
+  CertLoader();
+  ~CertLoader();
 
-  // Called when the underlying NSS database finished loading certificates.
-  void CertificatesLoaded(std::unique_ptr<net::CertificateList> all_certs);
+  // Called by |system_cert_cache_| or |user_cert_cache| when these had an
+  // update.
+  void CacheUpdated();
 
   // Called if a certificate load task is finished.
   void UpdateCertificates(std::unique_ptr<net::CertificateList> all_certs,
@@ -118,25 +135,20 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
 
   void NotifyCertificatesLoaded(bool initial_load);
 
-  // net::CertDatabase::Observer
-  void OnCertDBChanged() override;
+  // True if the initial load of CertLoader is still pending.

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

Seems that this member is only used for the debug output? I'd suggest removing
it and keeping the class simpler, unless there's a good reason to have this bit
of information here.

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

When CertLoader updates its Observers, it passes a bool initial_load parameter,
suggesting if this is the initial load of certificates.
The initial load is defined as the first time any certificates are loaded (see
Observer interface definition above), so it should be false when loading
additional certs due to adding another slot.

(this can be justified by two reasons:
(1) It should be transparent to CertLoader's clients that CertLoader is using
two databases internally. So the additional database load should be treated in
the same way as an update to the first database
(2) The only client using this parameter is NetworkCertMigrator, which updates
slot numbers and removes non-loaded cert references in shill. This should only
be done on the initial load. There can be no reference to certs from the
additional database when it is loaded, because those references would have been
removed on the first pass.
Instead, ClientCertResolver re-estabilishes that reference)

This boolean tracks that property. It can't be in the CertCache itself because
it is global for the CertLoader, not local per used NSSCertDatabase.

I've updated the comment to make that more clear.

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

Thanks, I missed that this flag is actually used.

+  bool pending_initial_load_;
 
   base::ObserverList<Observer> observers_;
 
-  // Flags describing current CertLoader state.
-  bool certificates_loaded_;
-  bool certificates_update_required_;
-  bool certificates_update_running_;
-
-  // The user-specific NSS certificate database from which the certificates
-  // should be loaded.
-  net::NSSCertDatabase* database_;
+  // Cache for certificates from the system-token NSSCertDatabase.
+  std::unique_ptr<CertCache> system_cert_cache_;
+  // Cache for certificates from the user-specific NSSCertDatabase.
+  std::unique_ptr<CertCache> user_cert_cache_;
 
-  // Cached certificates loaded from the database.
+  // Cached certificates loaded from the database(s).
   std::unique_ptr<net::CertificateList> all_certs_;
 
-  // Cached certificates from system token. Currently this is a sublist of
-  // |all_certs_|.
+  // Cached certificates from system token.
   std::unique_ptr<net::CertificateList> system_certs_;
 
   base::ThreadChecker thread_checker_;