Call crypto::InitializeTPMToken on the IO thread (Take 2)

This is identical to https://chromiumcodereview.appspot.com/15649018
with the following addition:

Call DeviceSettingsService::EnsureReload once certs are loaded

BUG=244455
R=mnissler@chromium.org, nkostylev@chromium.org, rsleevi@chromium.org, xiyuan@chromium.org

Review URL: https://codereview.chromium.org/20130002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@213951 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 141 insertions, 59 deletions

diff --git a/chromeos/network/cert_loader.cc b/chromeos/cert_loader.cc
index f75f8bb..ff57d36 100644
--- a/chromeos/network/cert_loader.cc
+++ b/chromeos/cert_loader.cc
@@ -2,12 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chromeos/network/cert_loader.h"
+#include "chromeos/cert_loader.h"
 
 #include <algorithm>
 
 #include "base/chromeos/chromeos_version.h"
+#include "base/message_loop/message_loop_proxy.h"
 #include "base/observer_list.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/threading/worker_pool.h"
@@ -46,8 +48,45 @@ void LoadNSSCertificates(net::CertificateList* cert_list) {
     net::NSSCertDatabase::GetInstance()->ListCerts(cert_list);
 }
 
+void CallOpenPersistentNSSDB() {
+  // Called from crypto_task_runner_.
+  VLOG(1) << "CallOpenPersistentNSSDB";
+
+  // Ensure we've opened the user's key/certificate database.
+  crypto::OpenPersistentNSSDB();
+  if (base::chromeos::IsRunningOnChromeOS())
+    crypto::EnableTPMTokenForNSS();
+}
+
 }  // namespace
 
+static CertLoader* g_cert_loader = NULL;
+// static
+void CertLoader::Initialize() {
+  CHECK(!g_cert_loader);
+  g_cert_loader = new CertLoader();
+  g_cert_loader->Init();
+}
+
+// static
+void CertLoader::Shutdown() {
+  CHECK(g_cert_loader);
+  delete g_cert_loader;
+  g_cert_loader = NULL;
+}
+
+// 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_requested_(false),
       certificates_loaded_(false),
@@ -58,10 +97,18 @@ CertLoader::CertLoader()
           base::TimeDelta::FromMilliseconds(kInitialRequestDelayMs)),
       initialize_token_factory_(this),
       update_certificates_factory_(this) {
+}
+
+void CertLoader::Init() {
   net::CertDatabase::GetInstance()->AddObserver(this);
   if (LoginState::IsInitialized())
     LoginState::Get()->AddObserver(this);
-  RequestCertificates();
+}
+
+void CertLoader::SetCryptoTaskRunner(
+    const scoped_refptr<base::SequencedTaskRunner>& crypto_task_runner) {
+  crypto_task_runner_ = crypto_task_runner;
+  MaybeRequestCertificates();
 }
 
 CertLoader::~CertLoader() {
@@ -86,36 +133,44 @@ bool CertLoader::IsHardwareBacked() const {
   return !tpm_token_name_.empty();
 }
 
-void CertLoader::RequestCertificates() {
+void CertLoader::MaybeRequestCertificates() {
   CHECK(thread_checker_.CalledOnValidThread());
+
+  // This is the entry point to the TPM token initialization process,
+  // which we should do at most once.
+  if (certificates_requested_ || !crypto_task_runner_.get())
+    return;
+
   const bool logged_in = LoginState::IsInitialized() ?
       LoginState::Get()->IsUserLoggedIn() : false;
   VLOG(1) << "RequestCertificates: " << logged_in;
-  if (certificates_requested_ || !logged_in)
+  if (!logged_in)
     return;
 
   certificates_requested_ = true;
 
-  // Ensure we've opened the user's key/certificate database.
-  crypto::OpenPersistentNSSDB();
-  if (base::chromeos::IsRunningOnChromeOS())
-    crypto::EnableTPMTokenForNSS();
-
-  // This is the entry point to the TPM token initialization process, which we
-  // should do at most once.
-  DCHECK(!initialize_token_factory_.HasWeakPtrs());
+  // Ensure we only initialize the TPM token once.
+  DCHECK_EQ(tpm_token_state_, TPM_STATE_UNKNOWN);
   InitializeTokenAndLoadCertificates();
 }
 
 void CertLoader::InitializeTokenAndLoadCertificates() {
   CHECK(thread_checker_.CalledOnValidThread());
-  VLOG(1) << "InitializeTokenAndLoadCertificates";
+  VLOG(1) << "InitializeTokenAndLoadCertificates: " << tpm_token_state_;
 
   switch (tpm_token_state_) {
     case TPM_STATE_UNKNOWN: {
+      crypto_task_runner_->PostTaskAndReply(
+          FROM_HERE,
+          base::Bind(&CallOpenPersistentNSSDB),
+          base::Bind(&CertLoader::OnPersistentNSSDBOpened,
+                     initialize_token_factory_.GetWeakPtr()));
+      return;
+    }
+    case TPM_DB_OPENED: {
       DBusThreadManager::Get()->GetCryptohomeClient()->TpmIsEnabled(
-      base::Bind(&CertLoader::OnTpmIsEnabled,
-                 initialize_token_factory_.GetWeakPtr()));
+          base::Bind(&CertLoader::OnTpmIsEnabled,
+                     initialize_token_factory_.GetWeakPtr()));
       return;
     }
     case TPM_DISABLED: {
@@ -138,10 +193,20 @@ void CertLoader::InitializeTokenAndLoadCertificates() {
       return;
     }
     case TPM_TOKEN_INFO_RECEIVED: {
-      InitializeNSSForTPMToken();
-      return;
+      if (base::chromeos::IsRunningOnChromeOS()) {
+        base::PostTaskAndReplyWithResult(
+            crypto_task_runner_.get(),
+            FROM_HERE,
+            base::Bind(&crypto::InitializeTPMToken,
+                       tpm_token_name_, tpm_user_pin_),
+            base::Bind(&CertLoader::OnTPMTokenInitialized,
+                       initialize_token_factory_.GetWeakPtr()));
+        return;
+      }
+      tpm_token_state_ = TPM_TOKEN_INITIALIZED;
+      // FALL_THROUGH_INTENDED
     }
-    case TPM_TOKEN_NSS_INITIALIZED: {
+    case TPM_TOKEN_INITIALIZED: {
       StartLoadCertificates();
       return;
     }
@@ -149,6 +214,7 @@ void CertLoader::InitializeTokenAndLoadCertificates() {
 }
 
 void CertLoader::RetryTokenInitializationLater() {
+  CHECK(thread_checker_.CalledOnValidThread());
   LOG(WARNING) << "Re-Requesting Certificates later.";
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
@@ -158,6 +224,12 @@ void CertLoader::RetryTokenInitializationLater() {
   tpm_request_delay_ = GetNextRequestDelayMs(tpm_request_delay_);
 }
 
+void CertLoader::OnPersistentNSSDBOpened() {
+  VLOG(1) << "PersistentNSSDBOpened";
+  tpm_token_state_ = TPM_DB_OPENED;
+  InitializeTokenAndLoadCertificates();
+}
+
 // For background see this discussion on dev-tech-crypto.lists.mozilla.org:
 // http://web.archiveorange.com/archive/v/6JJW7E40sypfZGtbkzxX
 //
@@ -234,21 +306,19 @@ void CertLoader::OnPkcs11GetTpmTokenInfo(DBusMethodCallStatus call_status,
   InitializeTokenAndLoadCertificates();
 }
 
-void CertLoader::InitializeNSSForTPMToken() {
-  VLOG(1) << "InitializeNSSForTPMToken";
-
-  if (base::chromeos::IsRunningOnChromeOS() &&
-      !crypto::InitializeTPMToken(tpm_token_name_, tpm_user_pin_)) {
+void CertLoader::OnTPMTokenInitialized(bool success) {
+  VLOG(1) << "OnTPMTokenInitialized: " << success;
+  if (!success) {
     RetryTokenInitializationLater();
     return;
   }
-
-  tpm_token_state_ = TPM_TOKEN_NSS_INITIALIZED;
+  tpm_token_state_ = TPM_TOKEN_INITIALIZED;
   InitializeTokenAndLoadCertificates();
 }
 
 void CertLoader::StartLoadCertificates() {
-  VLOG(1) << "StartLoadCertificates";
+  CHECK(thread_checker_.CalledOnValidThread());
+  VLOG(1) << "StartLoadCertificates: " << certificates_update_running_;
 
   if (certificates_update_running_) {
     certificates_update_required_ = true;
@@ -303,7 +373,7 @@ void CertLoader::OnCertRemoved(const net::X509Certificate* cert) {
 
 void CertLoader::LoggedInStateChanged(LoginState::LoggedInState state) {
   VLOG(1) << "LoggedInStateChanged: " << state;
-  RequestCertificates();
+  MaybeRequestCertificates();
 }
 
 }  // namespace chromeos
diff --git a/chromeos/network/cert_loader.h b/chromeos/cert_loader.h
index a57d29a..69e3191 100644
--- a/chromeos/network/cert_loader.h
+++ b/chromeos/cert_loader.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROMEOS_NETWORK_CERT_LOADER_H_
-#define CHROMEOS_NETWORK_CERT_LOADER_H_
+#ifndef CHROMEOS_CERT_LOADER_H_
+#define CHROMEOS_CERT_LOADER_H_
 
 #include <string>
 
@@ -15,10 +15,13 @@
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/login/login_state.h"
-#include "chromeos/network/network_handler.h"
 #include "net/cert/cert_database.h"
 #include "net/cert/x509_certificate.h"
 
+namespace base {
+class SequencedTaskRunner;
+}
+
 namespace crypto {
 class SymmetricKey;
 }
@@ -50,7 +53,24 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer,
     DISALLOW_COPY_AND_ASSIGN(Observer);
   };
 
-  virtual ~CertLoader();
+  // 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();
+
+  // |crypto_task_runner| is the task runner that any synchronous crypto calls
+  // should be made from. e.g. in Chrome this is the IO thread. Must be called
+  // after the thread is started. Certificate loading will not happen unless
+  // this is set.
+  void SetCryptoTaskRunner(
+      const scoped_refptr<base::SequencedTaskRunner>& crypto_task_runner);
 
   void AddObserver(CertLoader::Observer* observer);
   void RemoveObserver(CertLoader::Observer* observer);
@@ -75,15 +95,17 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer,
   const net::CertificateList& cert_list() const { return cert_list_; }
 
  private:
-  friend class NetworkHandler;
   CertLoader();
+  virtual ~CertLoader();
 
-  void RequestCertificates();
+  void Init();
+  void MaybeRequestCertificates();
 
   // This is the cyclic chain of callbacks to initialize the TPM token and to
   // kick off the update of the certificate list.
   void InitializeTokenAndLoadCertificates();
   void RetryTokenInitializationLater();
+  void OnPersistentNSSDBOpened();
   void OnTpmIsEnabled(DBusMethodCallStatus call_status,
                       bool tpm_is_enabled);
   void OnPkcs11IsTpmTokenReady(DBusMethodCallStatus call_status,
@@ -91,7 +113,7 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer,
   void OnPkcs11GetTpmTokenInfo(DBusMethodCallStatus call_status,
                                const std::string& token_name,
                                const std::string& user_pin);
-  void InitializeNSSForTPMToken();
+  void OnTPMTokenInitialized(bool success);
 
   // These calls handle the updating of the certificate list after the TPM token
   // was initialized.
@@ -119,11 +141,12 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer,
   // be left.
   enum TPMTokenState {
     TPM_STATE_UNKNOWN,
+    TPM_DB_OPENED,
     TPM_DISABLED,
     TPM_ENABLED,
     TPM_TOKEN_READY,
     TPM_TOKEN_INFO_RECEIVED,
-    TPM_TOKEN_NSS_INITIALIZED,
+    TPM_TOKEN_INITIALIZED,
   };
   TPMTokenState tpm_token_state_;
 
@@ -141,6 +164,9 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer,
 
   base::ThreadChecker thread_checker_;
 
+  // TaskRunner for crypto calls.
+  scoped_refptr<base::SequencedTaskRunner> crypto_task_runner_;
+
   // This factory should be used only for callbacks during TPMToken
   // initialization.
   base::WeakPtrFactory<CertLoader> initialize_token_factory_;
@@ -154,4 +180,4 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer,
 
 }  // namespace chromeos
 
-#endif  // CHROMEOS_NETWORK_CERT_LOADER_H_
+#endif  // CHROMEOS_CERT_LOADER_H_
diff --git a/chromeos/chromeos.gyp b/chromeos/chromeos.gyp
index 04459d6..f387c94 100644
--- a/chromeos/chromeos.gyp
+++ b/chromeos/chromeos.gyp
@@ -46,6 +46,8 @@
         'attestation/attestation_constants.h',
         'attestation/attestation_flow.cc',
         'attestation/attestation_flow.h',
+        'cert_loader.cc',
+        'cert_loader.h',
         'chromeos_constants.cc',
         'chromeos_constants.h',
         'chromeos_export.h',
@@ -209,8 +211,6 @@
         'ime/xkeyboard.h',
         'login/login_state.cc',
         'login/login_state.h',
-        'network/cert_loader.cc',
-        'network/cert_loader.h',
         'network/certificate_pattern.cc',
         'network/certificate_pattern.h',
         'network/certificate_pattern_matcher.cc',
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 3e86d9f..3920e1d 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -11,7 +11,6 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_service_client.h"
-#include "chromeos/network/cert_loader.h"
 #include "chromeos/network/certificate_pattern_matcher.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_configuration_handler.h"
@@ -175,7 +174,6 @@ NetworkConnectionHandler::~NetworkConnectionHandler() {
 }
 
 void NetworkConnectionHandler::Init(
-    CertLoader* cert_loader,
     NetworkStateHandler* network_state_handler,
     NetworkConfigurationHandler* network_configuration_handler) {
   if (LoginState::IsInitialized()) {
@@ -183,10 +181,10 @@ void NetworkConnectionHandler::Init(
     logged_in_ =
         LoginState::Get()->GetLoggedInState() == LoginState::LOGGED_IN_ACTIVE;
   }
-  if (cert_loader) {
-    cert_loader_ = cert_loader;
+  if (CertLoader::IsInitialized()) {
+    cert_loader_ = CertLoader::Get();
     cert_loader_->AddObserver(this);
-    certificates_loaded_ = cert_loader->certificates_loaded();
+    certificates_loaded_ = cert_loader_->certificates_loaded();
   } else {
     // TODO(stevenjb): Require a mock or stub cert_loader in tests.
     certificates_loaded_ = true;
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index 420426f..894e788 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -12,10 +12,10 @@
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
+#include "chromeos/cert_loader.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/login/login_state.h"
-#include "chromeos/network/cert_loader.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_handler_callbacks.h"
 #include "chromeos/network/network_state_handler_observer.h"
@@ -118,8 +118,7 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
 
   NetworkConnectionHandler();
 
-  void Init(CertLoader* cert_loader,
-            NetworkStateHandler* network_state_handler,
+  void Init(NetworkStateHandler* network_state_handler,
             NetworkConfigurationHandler* network_configuration_handler);
 
   ConnectRequest* pending_request(const std::string& service_path);
diff --git a/chromeos/network/network_connection_handler_unittest.cc b/chromeos/network/network_connection_handler_unittest.cc
index 98002f7..8379d2f 100644
--- a/chromeos/network/network_connection_handler_unittest.cc
+++ b/chromeos/network/network_connection_handler_unittest.cc
@@ -52,8 +52,7 @@ class NetworkConnectionHandlerTest : public testing::Test {
             network_state_handler_.get()));
     network_connection_handler_.reset(new NetworkConnectionHandler);
     // TODO(stevenjb): Test integration with CertLoader using a stub or mock.
-    network_connection_handler_->Init(NULL /* cert_loader */,
-                                      network_state_handler_.get(),
+    network_connection_handler_->Init(network_state_handler_.get(),
                                       network_configuration_handler_.get());
   }
 
diff --git a/chromeos/network/network_handler.cc b/chromeos/network/network_handler.cc
index 8758843..272a1de 100644
--- a/chromeos/network/network_handler.cc
+++ b/chromeos/network/network_handler.cc
@@ -5,7 +5,6 @@
 #include "chromeos/network/network_handler.h"
 
 #include "chromeos/dbus/dbus_thread_manager.h"
-#include "chromeos/network/cert_loader.h"
 #include "chromeos/network/geolocation_handler.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_configuration_handler.h"
@@ -26,7 +25,6 @@ NetworkHandler::NetworkHandler() {
 
   network_event_log::Initialize();
 
-  cert_loader_.reset(new CertLoader);
   network_state_handler_.reset(new NetworkStateHandler());
   network_device_handler_.reset(new NetworkDeviceHandler());
   network_profile_handler_.reset(new NetworkProfileHandler());
@@ -49,8 +47,7 @@ void NetworkHandler::Init() {
       network_state_handler_.get(),
       network_profile_handler_.get(),
       network_configuration_handler_.get());
-  network_connection_handler_->Init(cert_loader_.get(),
-                                    network_state_handler_.get(),
+  network_connection_handler_->Init(network_state_handler_.get(),
                                     network_configuration_handler_.get());
   geolocation_handler_->Init();
 }
@@ -81,10 +78,6 @@ bool NetworkHandler::IsInitialized() {
   return g_network_handler;
 }
 
-CertLoader* NetworkHandler::cert_loader() {
-  return cert_loader_.get();
-}
-
 NetworkStateHandler* NetworkHandler::network_state_handler() {
   return network_state_handler_.get();
 }
diff --git a/chromeos/network/network_handler.h b/chromeos/network/network_handler.h
index 3d9aa42..b25ab1b 100644
--- a/chromeos/network/network_handler.h
+++ b/chromeos/network/network_handler.h
@@ -11,7 +11,6 @@
 
 namespace chromeos {
 
-class CertLoader;
 class GeolocationHandler;
 class ManagedNetworkConfigurationHandler;
 class NetworkConfigurationHandler;
@@ -40,7 +39,6 @@ class CHROMEOS_EXPORT NetworkHandler {
   // Do not use these accessors within this module; all dependencies should be
   // explicit so that classes can be constructed explicitly in tests without
   // NetworkHandler.
-  CertLoader* cert_loader();
   NetworkStateHandler* network_state_handler();
   NetworkDeviceHandler* network_device_handler();
   NetworkProfileHandler* network_profile_handler();
@@ -56,7 +54,6 @@ class CHROMEOS_EXPORT NetworkHandler {
   void Init();
 
   // The order of these determines the (inverse) destruction order.
-  scoped_ptr<CertLoader> cert_loader_;
   scoped_ptr<NetworkStateHandler> network_state_handler_;
   scoped_ptr<NetworkDeviceHandler> network_device_handler_;
   scoped_ptr<NetworkProfileHandler> network_profile_handler_;