Enable device-wide EAP-TLS networks

chromeos/cert_loader.h

 // 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.
 
 #ifndef CHROMEOS_CERT_LOADER_H_
 #define CHROMEOS_CERT_LOADER_H_
 
 #include <string>
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/threading/thread_checker.h"
 #include "chromeos/chromeos_export.h"
 #include "net/cert/cert_database.h"

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

nit: Unnecessary include.

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

Done.
We do need to include something which defines X509Certificate so
scoped_refptr<X509Certificate> used through the CertificateList typedef works -
included it directly.

 
 namespace net {
 class NSSCertDatabase;
 class X509Certificate;
 typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 }
 
 namespace chromeos {
 
 // This class is responsible for loading certificates once the TPM is
 // initialized. It is expected to be constructed on the UI thread and public
 // methods should all be called from the UI thread.
 // 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;
 
    protected:
     virtual ~Observer() {}
   };
 
   // Sets the global instance. Must be called before any calls to Get().
   static void Initialize();
 
   // Destroys the global instance.
   static void Shutdown();
 
   // Gets the global instance. Initialize() must be called first.
   static CertLoader* Get();
 
   // Returns true if the global instance has been initialized.
   static bool IsInitialized();
 
   // Returns the PKCS#11 attribute CKA_ID for a certificate as an upper-case
   // hex string and sets |slot_id| to the id of the containing slot, or returns
   // an empty string and doesn't modify |slot_id| if the PKCS#11 id could not be
   // determined.
   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);
 
   // Returns true if |cert| is hardware backed. See also
   // ForceHardwareBackedForTesting().
   static bool IsCertificateHardwareBacked(const net::X509Certificate* cert);
 
   // 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.
   const net::CertificateList& all_certs() const {
     DCHECK(thread_checker_.CalledOnValidThread());
     return *all_certs_;
   }
 
   // Returns certificates from the system token. This will be empty until
   // certificates_loaded() is true.
   const net::CertificateList& system_certs() const {
     DCHECK(thread_checker_.CalledOnValidThread());
     return *system_certs_;
   }
 
   // Called in tests if |IsCertificateHardwareBacked()| should always return
   // true.
   static void ForceHardwareBackedForTesting();
 
  private:
+  class CertCache;
+
   CertLoader();
-  ~CertLoader() override;
+  ~CertLoader();
 
-  // 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();
-
-  // 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,
                           std::unique_ptr<net::CertificateList> system_certs);
 
   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_;
+  // 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_;
 
-  // The user-specific NSS certificate database from which the certificates
-  // should be loaded.
-  net::NSSCertDatabase* database_;
-
-  // 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_;
 
   base::WeakPtrFactory<CertLoader> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CertLoader);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_CERT_LOADER_H_