Make PlatformKeysServiceImpl independent of BrowserContext/Profile

This change makes PlatformKeysServiceImpl independent of
BrowserContext/Profile.
All BrowserContext-specific calls have been extracted into a
PlatformKeysServiceImplDelegate.

This has the advantage that for device-wide usage of the
PlatformKeysService, a delegate can be created which does not depend on
a Profile (avoiding using the sign-in Profile for that use case).
The CertProvisioningScheduler use case has been migrated to this
pattern.

As the device-wide PlatformKeysService lives "forever" but the
underlying system token NSSCertDatabase does not, PlatformKeysService
can now explicitly "shut down" when the underlying NSSCertDatabase is
being destroyed. After it is shut down all operations on the
PlatformKeysService will fail with an error (except for removing
observers).

Note: The PlatformKeysService still works for the sign-in Profile
which is important for the sign-in screen extensions use case.

Bug: 1045895, 1073512
Test: browser_tests
Change-Id: I09b855c61caaa744bf8618073eaa2854a609df84
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2188633
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Michael Ershov <miersh@google.com>
Cr-Commit-Position: refs/heads/master@{#790397}

chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.cc

@@ -9,7 +9,9 @@
 #include "base/optional.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
+#include "chrome/browser/chromeos/platform_keys/platform_keys_service_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/cryptohome/cryptohome_client.h"
@@ -207,5 +209,17 @@ scoped_refptr<net::X509Certificate> CreateSingleCertificateFromBytes(
   return cert_list[0];
 }
 
+platform_keys::PlatformKeysService* GetPlatformKeysService(CertScope scope,
+                                                           Profile* profile) {
+  switch (scope) {
+    case CertScope::kUser:
+      return platform_keys::PlatformKeysServiceFactory::GetForBrowserContext(
+          profile);
+    case CertScope::kDevice:
+      return platform_keys::PlatformKeysServiceFactory::GetInstance()
+          ->GetDeviceWideService();
+  }
+}
+
 }  // namespace cert_provisioning
 }  // namespace chromeos

chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h

@@ -21,6 +21,11 @@ class PrefRegistrySimple;
 class Profile;
 
 namespace chromeos {
+
+namespace platform_keys {
+class PlatformKeysService;
+}  // namespace platform_keys
+
 namespace cert_provisioning {
 
 // Used for both DeleteVaKey and DeleteVaKeysByPrefix
@@ -131,6 +136,16 @@ scoped_refptr<net::X509Certificate> CreateSingleCertificateFromBytes(
     const char* data,
     size_t length);
 
+// Returns the PlatformKeysService to be used.
+// If |scope| is CertScope::kDevice, |profile| is ignored and the
+// device-wide PlatformKeysService is returned.
+// If |scope| is CertScope::kUser, returns the service for |profile|.
+// The returned object is owned by the Profile (user-specific) or globally
+// (device-wide) and may only be used until it notifies its observers that it is
+// being shut down.
+platform_keys::PlatformKeysService* GetPlatformKeysService(CertScope scope,
+                                                           Profile* profile);
+
 }  // namespace cert_provisioning
 }  // namespace chromeos
 

chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler.cc

@@ -105,7 +105,7 @@ CertProvisioningSchedulerImpl::CreateUserCertProvisioningScheduler(
   policy::CloudPolicyClient* cloud_policy_client =
       GetCloudPolicyClientForUser(profile);
   platform_keys::PlatformKeysService* platform_keys_service =
-      platform_keys::PlatformKeysServiceFactory::GetForBrowserContext(profile);
+      GetPlatformKeysService(CertScope::kUser, profile);
   NetworkStateHandler* network_state_handler = GetNetworkStateHandler();
 
   if (!profile || !pref_service || !cloud_policy_client ||
@@ -130,7 +130,7 @@ CertProvisioningSchedulerImpl::CreateDeviceCertProvisioningScheduler(
   policy::CloudPolicyClient* cloud_policy_client =
       GetCloudPolicyClientForDevice();
   platform_keys::PlatformKeysService* platform_keys_service =
-      platform_keys::PlatformKeysServiceFactory::GetForBrowserContext(profile);
+      GetPlatformKeysService(CertScope::kDevice, profile);
   NetworkStateHandler* network_state_handler = GetNetworkStateHandler();
 
   if (!profile || !pref_service || !cloud_policy_client ||
@@ -173,6 +173,8 @@ CertProvisioningSchedulerImpl::CertProvisioningSchedulerImpl(
   pref_name_ = GetPrefNameForCertProfiles(cert_scope);
   CHECK(pref_name_);
 
+  scoped_platform_keys_service_observer_.Add(platform_keys_service_);
+
   network_state_handler_->AddObserver(this, FROM_HERE);
 
   ScheduleInitialUpdate();
@@ -238,6 +240,10 @@ void CertProvisioningSchedulerImpl::InitialUpdateCerts() {
 
 void CertProvisioningSchedulerImpl::DeleteCertsWithoutPolicy() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // No-op if the PlatformKeysService has already been shut down.
+  if (!platform_keys_service_) {
+    return;
+  }
 
   base::flat_set<CertProfileId> cert_profile_ids_to_keep;
   {
@@ -391,6 +397,11 @@ void CertProvisioningSchedulerImpl::UpdateCertList(
     std::vector<CertProfile> profiles) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  // No-op if the PlatformKeysService has already been shut down.
+  if (!platform_keys_service_) {
+    return;
+  }
+
   if (!MaybeWaitForInternetConnection()) {
     return;
   }
@@ -677,6 +688,23 @@ void CertProvisioningSchedulerImpl::UpdateFailedCertProfiles(
   failed_cert_profiles_[worker.GetCertProfile().profile_id] = std::move(info);
 }
 
+void CertProvisioningSchedulerImpl::OnPlatformKeysServiceShutDown() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // The |platform_keys_service_| will only return errors going forward, so
+  // stop using it. Shutdown all workers, as if this CertProvisioningScheduler
+  // was destroyed, and stop pending tasks that may depend on
+  // |platform_keys_service_|.
+  workers_.clear();
+  certs_with_ids_getter_.Cancel();
+  cert_deleter_.Cancel();
+  pref_change_registrar_.RemoveAll();
+  weak_factory_.InvalidateWeakPtrs();
+
+  scoped_platform_keys_service_observer_.RemoveAll();
+  platform_keys_service_ = nullptr;
+}
+
 void CertProvisioningSchedulerImpl::CancelWorkersWithoutPolicy(
     const std::vector<CertProfile>& profiles) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler.h

@@ -10,11 +10,13 @@
 #include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
 #include "base/sequence_checker.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_invalidator.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_platform_keys_helpers.h"
+#include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
 #include "chromeos/network/network_state_handler_observer.h"
 #include "components/prefs/pref_change_registrar.h"
 
@@ -29,10 +31,6 @@ namespace chromeos {
 
 class NetworkStateHandler;
 
-namespace platform_keys {
-class PlatformKeysService;
-}  // namespace platform_keys
-
 namespace cert_provisioning {
 
 class CertProvisioningWorker;
@@ -82,8 +80,10 @@ class CertProvisioningScheduler {
 // Should work on the UI thread because it interacts with PlatformKeysService
 // and some methods are called from the UI to populate certificate manager
 // settings page.
-class CertProvisioningSchedulerImpl : public CertProvisioningScheduler,
-                                      public NetworkStateHandlerObserver {
+class CertProvisioningSchedulerImpl
+    : public CertProvisioningScheduler,
+      public NetworkStateHandlerObserver,
+      public platform_keys::PlatformKeysServiceObserver {
  public:
   static std::unique_ptr<CertProvisioningScheduler>
   CreateUserCertProvisioningScheduler(Profile* profile);
@@ -167,11 +167,15 @@ class CertProvisioningSchedulerImpl : public CertProvisioningScheduler,
 
   void UpdateFailedCertProfiles(const CertProvisioningWorker& worker);
 
+  // PlatformKeysServiceObserver
+  void OnPlatformKeysServiceShutDown() override;
+
   CertScope cert_scope_ = CertScope::kUser;
   Profile* profile_ = nullptr;
   PrefService* pref_service_ = nullptr;
   const char* pref_name_ = nullptr;
   policy::CloudPolicyClient* cloud_policy_client_ = nullptr;
+  // |platform_keys_service_| can be nullptr if it has been shut down.
   platform_keys::PlatformKeysService* platform_keys_service_ = nullptr;
   NetworkStateHandler* network_state_handler_ = nullptr;
   PrefChangeRegistrar pref_change_registrar_;
@@ -197,6 +201,10 @@ class CertProvisioningSchedulerImpl : public CertProvisioningScheduler,
   CertDeleter cert_deleter_;
   std::unique_ptr<CertProvisioningInvalidatorFactory> invalidator_factory_;
 
+  ScopedObserver<platform_keys::PlatformKeysService,
+                 platform_keys::PlatformKeysServiceObserver>
+      scoped_platform_keys_service_observer_{this};
+
   base::WeakPtrFactory<CertProvisioningSchedulerImpl> weak_factory_{this};
 };
 

chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler_unittest.cc

@@ -14,7 +14,6 @@
 #include "chrome/browser/chromeos/cert_provisioning/mock_cert_provisioning_worker.h"
 #include "chrome/browser/chromeos/platform_keys/mock_platform_keys_service.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
-#include "chrome/browser/chromeos/platform_keys/platform_keys_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/network/network_state_test_helper.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
@@ -33,6 +32,7 @@ using testing::Invoke;
 using testing::Mock;
 using testing::Return;
 using testing::ReturnRef;
+using testing::SaveArg;
 using testing::StrictMock;
 
 namespace chromeos {
@@ -900,6 +900,56 @@ TEST_F(CertProvisioningSchedulerTest, CertRenewal) {
   ASSERT_EQ(scheduler.GetWorkers().size(), 1U);
 }
 
+TEST_F(CertProvisioningSchedulerTest, PlatformKeysServiceShutDown) {
+  CertScope kCertScope = CertScope::kDevice;
+
+  platform_keys::PlatformKeysServiceObserver* observer = nullptr;
+  EXPECT_CALL(platform_keys_service_, AddObserver(_))
+      .WillOnce(SaveArg<0>(&observer));
+  CertProvisioningSchedulerImpl scheduler(
+      kCertScope, GetProfile(), &pref_service_, &cloud_policy_client_,
+      &platform_keys_service_,
+      network_state_test_helper_.network_state_handler(),
+      MakeFakeInvalidationFactory());
+
+  ASSERT_TRUE(observer);
+
+  // Add 1 certificate profile to the policy.
+  base::Value config = ParseJson(
+      R"([{"name": "Certificate Profile 1",
+           "cert_profile_id":"cert_profile_id_1",
+           "policy_version":"cert_profile_version_1",
+           "key_algorithm":"rsa" }])");
+  pref_service_.Set(prefs::kRequiredClientCertificateForDevice, config);
+
+  // Same as in the policy.
+  const char kCertProfileId[] = "cert_profile_id_1";
+  const char kCertProfileVersion[] = "cert_profile_version_1";
+  CertProfile cert_profile{kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod};
+
+  MockCertProvisioningWorker* worker =
+      mock_factory_.ExpectCreateReturnMock(kCertScope, cert_profile);
+  worker->SetExpectations(/*do_step_times=*/AtLeast(1), /*is_waiting=*/false,
+                          cert_profile);
+  scheduler.UpdateAllCerts();
+
+  // Now 1 worker should be created.
+  EXPECT_EQ(scheduler.GetWorkers().size(), 1U);
+
+  // PlatformKeysService notifies that it is shutting down.
+  EXPECT_CALL(platform_keys_service_, RemoveObserver(observer));
+  observer->OnPlatformKeysServiceShutDown();
+
+  // The worker should be deleted.
+  EXPECT_EQ(scheduler.GetWorkers().size(), 0U);
+
+  // Check one more time that scheduler doesn't create new workers after
+  // PlatformKeysService has been shut down (the factory will fail on an attempt
+  // to do so).
+  scheduler.UpdateAllCerts();
+}
+
 }  // namespace
 }  // namespace cert_provisioning
 }  // namespace chromeos

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker.cc

@@ -187,8 +187,7 @@ CertProvisioningWorkerImpl::CertProvisioningWorkerImpl(
       cloud_policy_client_(cloud_policy_client),
       invalidator_(std::move(invalidator)) {
   CHECK(profile);
-  platform_keys_service_ =
-      platform_keys::PlatformKeysServiceFactory::GetForBrowserContext(profile);
+  platform_keys_service_ = GetPlatformKeysService(cert_scope, profile);
   CHECK(platform_keys_service_);
 
   CHECK(pref_service);

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker.h

@@ -254,6 +254,10 @@ class CertProvisioningWorkerImpl : public CertProvisioningWorker {
   // because of it).
   static constexpr int kVersion = 1;
 
+  // Unowned PlatformKeysService. Note that the CertProvisioningWorker does not
+  // observe the PlatformKeysService for shutdown events. Instead, it relies on
+  // the CertProvisioningScheduler to destroy all CertProvisioningWorker
+  // instances when the corresponding PlatformKeysService is shutting down.
   platform_keys::PlatformKeysService* platform_keys_service_ = nullptr;
   std::unique_ptr<attestation::TpmChallengeKeySubtle>
       tpm_challenge_key_subtle_impl_;

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker_unittest.cc

@@ -344,6 +344,9 @@ class CertProvisioningWorkerTest : public ::testing::Test {
                     base::BindRepeating(
                         &platform_keys::BuildMockPlatformKeysService)));
     ASSERT_TRUE(platform_keys_service_);
+    platform_keys::PlatformKeysServiceFactory::GetInstance()
+        ->SetDeviceWideServiceForTesting(platform_keys_service_);
+
     // Only explicitly expected removals are allowed.
     EXPECT_CALL(*platform_keys_service_, RemoveCertificate).Times(0);
     EXPECT_CALL(*platform_keys_service_, RemoveKey).Times(0);

chrome/browser/chromeos/platform_keys/mock_platform_keys_service.h

@@ -11,6 +11,10 @@
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
+namespace content {
+class BrowserContext;
+}
+
 namespace chromeos {
 namespace platform_keys {
 
@@ -21,6 +25,16 @@ class MockPlatformKeysService : public PlatformKeysService {
   MockPlatformKeysService& operator=(const MockPlatformKeysService&) = delete;
   ~MockPlatformKeysService() override;
 
+  MOCK_METHOD(void,
+              AddObserver,
+              (PlatformKeysServiceObserver * observer),
+              (override));
+
+  MOCK_METHOD(void,
+              RemoveObserver,
+              (PlatformKeysServiceObserver * observer),
+              (override));
+
   MOCK_METHOD(void,
               GenerateRSAKey,
               (TokenId token_id,

chrome/browser/chromeos/platform_keys/platform_keys_service.cc

@@ -5,12 +5,14 @@
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
 
 #include <map>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool.h"
+#include "content/public/browser/browser_thread.h"
 #include "net/base/hash_value.h"
 #include "net/cert/x509_certificate.h"
 
@@ -75,14 +77,65 @@ ClientCertificateRequest::ClientCertificateRequest(
 
 ClientCertificateRequest::~ClientCertificateRequest() = default;
 
+// =============== PlatformKeysServiceImplDelegate =============================
+
+PlatformKeysServiceImplDelegate::PlatformKeysServiceImplDelegate() = default;
+
+PlatformKeysServiceImplDelegate::~PlatformKeysServiceImplDelegate() {
+  ShutDown();
+}
+
+void PlatformKeysServiceImplDelegate::SetOnShutdownCallback(
+    base::OnceClosure on_shutdown_callback) {
+  DCHECK(!shut_down_);
+  DCHECK(!on_shutdown_callback_);
+  on_shutdown_callback_ = std::move(on_shutdown_callback);
+}
+
+bool PlatformKeysServiceImplDelegate::IsShutDown() const {
+  return shut_down_;
+}
+
+void PlatformKeysServiceImplDelegate::ShutDown() {
+  if (shut_down_)
+    return;
+
+  shut_down_ = true;
+  if (on_shutdown_callback_)
+    std::move(on_shutdown_callback_).Run();
+}
+
 // =================== PlatformKeysServiceImpl =================================
 
 PlatformKeysServiceImpl::PlatformKeysServiceImpl(
-    content::BrowserContext* context)
-    : browser_context_(context) {}
+    std::unique_ptr<PlatformKeysServiceImplDelegate> delegate)
+    : delegate_(std::move(delegate)) {
+  // base::Unretained is OK because |delegate_| is owned by this and can
+  // only call the callback before it is destroyed.
+  delegate_->SetOnShutdownCallback(base::BindOnce(
+      &PlatformKeysServiceImpl::OnDelegateShutDown, base::Unretained(this)));
+}
 
 PlatformKeysServiceImpl::~PlatformKeysServiceImpl() = default;
 
+void PlatformKeysServiceImpl::AddObserver(
+    PlatformKeysServiceObserver* observer) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  observers_.AddObserver(observer);
+}
+
+void PlatformKeysServiceImpl::RemoveObserver(
+    PlatformKeysServiceObserver* observer) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  observers_.RemoveObserver(observer);
+}
+
+void PlatformKeysServiceImpl::OnDelegateShutDown() {
+  for (auto& observer : observers_) {
+    observer.OnPlatformKeysServiceShutDown();
+  }
+}
+
 // The rest of the methods - the NSS-specific part of the implementation -
 // resides in the platform_keys_service_nss.cc file.
 

chrome/browser/chromeos/platform_keys/platform_keys_service.h

@@ -11,16 +11,19 @@
 #include <string>
 #include <vector>
 
-#include "base/callback_forward.h"
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
 #include "base/optional.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "net/cert/x509_certificate.h"
 
-namespace content {
-class BrowserContext;
-}
+namespace net {
+class NSSCertDatabase;
+class ClientCertStore;
+}  // namespace net
 
 namespace chromeos {
 namespace platform_keys {
@@ -178,10 +181,27 @@ using GetAttributeForKeyCallback =
     base::OnceCallback<void(const base::Optional<std::string>& attribute_value,
                             const std::string& error_message)>;
 
+// An observer that gets notified when the PlatformKeysService is being shut
+// down.
+class PlatformKeysServiceObserver : public base::CheckedObserver {
+ public:
+  // Called when the PlatformKeysService is being shut down.
+  // It may not be used after this call - any usage except for removing the
+  // observer will DCHECK.
+  virtual void OnPlatformKeysServiceShutDown() = 0;
+};
+
 // Functions of this class shouldn't be called directly from the context of
 // an extension. Instead use ExtensionPlatformKeysService which enforces
 // restrictions upon extensions.
 // All public methods of this class should be called on the UI thread.
+// When the underlying key store is not available anymore, a PlatformKeysService
+// is shut down. Any function called after that will fail with an error.
+// For a Profile-specific PlatformKeysService, this will be when the Profile is
+// being destroyed.
+// For a device-wide PlatformKeysService, this will be at some point during
+// chrome shut down.
+// Use AddObserver to get a notification when the service shuts down.
 class PlatformKeysService : public KeyedService {
  public:
   PlatformKeysService() = default;
@@ -189,6 +209,12 @@ class PlatformKeysService : public KeyedService {
   PlatformKeysService& operator=(const PlatformKeysService&) = delete;
   ~PlatformKeysService() override = default;
 
+  // Adds |observer| which will be notified when this service is being shut
+  // down.
+  virtual void AddObserver(PlatformKeysServiceObserver* observer) = 0;
+  // Removes a previously added |observer|.
+  virtual void RemoveObserver(PlatformKeysServiceObserver* observer) = 0;
+
   // Generates a RSA key pair with |modulus_length_bits|. |token_id| specifies
   // the token to store the key pair on. |callback| will be invoked with the
   // resulting public key
@@ -328,12 +354,55 @@ class PlatformKeysService : public KeyedService {
       const bool map_to_softoken_attrs_for_testing) = 0;
 };
 
+class PlatformKeysServiceImplDelegate {
+ public:
+  PlatformKeysServiceImplDelegate();
+  virtual ~PlatformKeysServiceImplDelegate();
+  PlatformKeysServiceImplDelegate(
+      const PlatformKeysServiceImplDelegate& other) = delete;
+  PlatformKeysServiceImplDelegate& operator=(
+      const PlatformKeysServiceImplDelegate& other) = delete;
+
+  // |on_shutdown_callback| will be called when the underlying key/certificate
+  // store is shut down. It is an error to call this twice, or after the
+  // delegate has been shut down.
+  void SetOnShutdownCallback(base::OnceClosure on_shutdown_callback);
+
+  // This callback is invoked by GetNSSCertDatabase.
+  using OnGotNSSCertDatabase = base::OnceCallback<void(net::NSSCertDatabase*)>;
+
+  // Retrieves the NSSCertDatabase that should be used for certificate
+  // operations. |callback| will be called on the thread that GetNSSCertDatabase
+  // has been called on.
+  virtual void GetNSSCertDatabase(OnGotNSSCertDatabase callback) = 0;
+
+  // Creates a ClientCertStore that should be used to list / operate on client
+  // certificates.
+  virtual std::unique_ptr<net::ClientCertStore> CreateClientCertStore() = 0;
+
+  bool IsShutDown() const;
+
+ protected:
+  void ShutDown();
+
+ private:
+  // A callback that should be called when the underlying key/certificate store
+  // is shut down.
+  base::OnceClosure on_shutdown_callback_;
+
+  // True if the underlying key/certificate store has already been shut down.
+  bool shut_down_ = false;
+};
+
 class PlatformKeysServiceImpl final : public PlatformKeysService {
  public:
-  explicit PlatformKeysServiceImpl(content::BrowserContext* context);
+  explicit PlatformKeysServiceImpl(
+      std::unique_ptr<PlatformKeysServiceImplDelegate> delegate);
   ~PlatformKeysServiceImpl() override;
 
   // PlatformKeysService
+  void AddObserver(PlatformKeysServiceObserver* observer) override;
+  void RemoveObserver(PlatformKeysServiceObserver* observer) override;
   void GenerateRSAKey(TokenId token_id,
                       unsigned int modulus_length_bits,
                       const GenerateKeyCallback& callback) override;
@@ -386,7 +455,11 @@ class PlatformKeysServiceImpl final : public PlatformKeysService {
   bool IsSetMapToSoftokenAttrsForTesting();
 
  private:
-  content::BrowserContext* const browser_context_;
+  void OnDelegateShutDown();
+
+  std::unique_ptr<PlatformKeysServiceImplDelegate> const delegate_;
+  // List of observers that will be notified when the service is shut down.
+  base::ObserverList<PlatformKeysServiceObserver> observers_;
   bool map_to_softoken_attrs_for_testing_ = false;
   base::WeakPtrFactory<PlatformKeysServiceImpl> weak_factory_{this};
 };

chrome/browser/chromeos/platform_keys/platform_keys_service_factory.cc

@@ -4,14 +4,129 @@
 
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service_factory.h"
 
+#include "base/callback_helpers.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/singleton.h"
+#include "base/scoped_observer.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/chromeos/certificate_provider/certificate_provider.h"
+#include "chrome/browser/chromeos/net/client_cert_store_chromeos.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/chromeos/system_token_cert_db_initializer.h"
+#include "chrome/browser/net/nss_context.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
+#include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/user_manager/user.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/resource_context.h"
+#include "net/cert/nss_cert_database.h"
 
 namespace chromeos {
 namespace platform_keys {
 
+namespace {
+
+// Invoked on the IO thread when a NSSCertDatabase is available, delegates back
+// to origin thread.
+void DidGetCertDbOnIoThread(
+    const scoped_refptr<base::SingleThreadTaskRunner>& origin_task_runner,
+    base::OnceCallback<void(net::NSSCertDatabase*)> callback,
+    net::NSSCertDatabase* cert_db) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  origin_task_runner->PostTask(FROM_HERE,
+                               base::BindOnce(std::move(callback), cert_db));
+}
+
+// Retrieves the NSSCertDatabase for |context|. Must be called on the IO thread.
+void GetCertDatabaseOnIoThread(
+    const scoped_refptr<base::SingleThreadTaskRunner>& origin_task_runner,
+    PlatformKeysServiceImplDelegate::OnGotNSSCertDatabase callback,
+    content::ResourceContext* context) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  base::RepeatingCallback<void(net::NSSCertDatabase*)> on_got_on_io_thread =
+      base::BindRepeating(&DidGetCertDbOnIoThread, origin_task_runner,
+                          base::AdaptCallbackForRepeating(std::move(callback)));
+  net::NSSCertDatabase* cert_db =
+      GetNSSCertDatabaseForResourceContext(context, on_got_on_io_thread);
+
+  if (cert_db)
+    on_got_on_io_thread.Run(cert_db);
+}
+
+class DelegateForUser : public PlatformKeysServiceImplDelegate {
+ public:
+  explicit DelegateForUser(content::BrowserContext* browser_context)
+      : browser_context_(browser_context) {}
+  ~DelegateForUser() override = default;
+
+  void GetNSSCertDatabase(OnGotNSSCertDatabase callback) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+    content::GetIOThreadTaskRunner({})->PostTask(
+        FROM_HERE,
+        base::BindOnce(&GetCertDatabaseOnIoThread,
+                       base::ThreadTaskRunnerHandle::Get(), std::move(callback),
+                       browser_context_->GetResourceContext()));
+  }
+
+  std::unique_ptr<net::ClientCertStore> CreateClientCertStore() override {
+    const user_manager::User* user =
+        chromeos::ProfileHelper::Get()->GetUserByProfile(
+            Profile::FromBrowserContext(browser_context_));
+
+    // Use the device-wide system key slot only if the user is affiliated on the
+    // device.
+    const bool use_system_key_slot = user->IsAffiliated();
+    return std::make_unique<ClientCertStoreChromeOS>(
+        nullptr,  // no additional provider
+        use_system_key_slot, user->username_hash(),
+        ClientCertStoreChromeOS::PasswordDelegateFactory());
+  }
+
+ private:
+  content::BrowserContext* browser_context_;
+};
+
+class DelegateForDevice : public PlatformKeysServiceImplDelegate,
+                          public SystemTokenCertDBObserver {
+ public:
+  DelegateForDevice() {
+    scoped_observer_.Add(SystemTokenCertDBInitializer::Get());
+  }
+
+  ~DelegateForDevice() override = default;
+
+  void GetNSSCertDatabase(OnGotNSSCertDatabase callback) override {
+    SystemTokenCertDBInitializer::Get()->GetSystemTokenCertDb(
+        std::move(callback));
+  }
+
+  std::unique_ptr<net::ClientCertStore> CreateClientCertStore() override {
+    return std::make_unique<ClientCertStoreChromeOS>(
+        nullptr,  // no additional provider
+        /*use_system_key_slot=*/true, /*username_hash=*/std::string(),
+        ClientCertStoreChromeOS::PasswordDelegateFactory());
+  }
+
+ private:
+  ScopedObserver<SystemTokenCertDBInitializer, SystemTokenCertDBObserver>
+      scoped_observer_{this};
+
+  // SystemTokenCertDBObserver:
+  void OnSystemTokenCertDBDestroyed() override {
+    scoped_observer_.RemoveAll();
+    ShutDown();
+  }
+};
+
+}  // namespace
+
 // static
 PlatformKeysService* PlatformKeysServiceFactory::GetForBrowserContext(
     content::BrowserContext* context) {
@@ -24,6 +139,23 @@ PlatformKeysServiceFactory* PlatformKeysServiceFactory::GetInstance() {
   return base::Singleton<PlatformKeysServiceFactory>::get();
 }
 
+// static
+PlatformKeysService* PlatformKeysServiceFactory::GetDeviceWideService() {
+  if (device_wide_service_for_testing_)
+    return device_wide_service_for_testing_;
+
+  if (!device_wide_service_) {
+    device_wide_service_ = std::make_unique<PlatformKeysServiceImpl>(
+        std::make_unique<DelegateForDevice>());
+  }
+  return device_wide_service_.get();
+}
+
+void PlatformKeysServiceFactory::SetDeviceWideServiceForTesting(
+    PlatformKeysService* device_wide_service_for_testing) {
+  device_wide_service_for_testing_ = device_wide_service_for_testing;
+}
+
 PlatformKeysServiceFactory::PlatformKeysServiceFactory()
     : BrowserContextKeyedServiceFactory(
           "PlatformKeysService",
@@ -33,7 +165,16 @@ PlatformKeysServiceFactory::~PlatformKeysServiceFactory() = default;
 
 KeyedService* PlatformKeysServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return new PlatformKeysServiceImpl(context);
+  std::unique_ptr<PlatformKeysServiceImplDelegate> delegate;
+  Profile* profile = Profile::FromBrowserContext(context);
+  if (ProfileHelper::IsSigninProfile(profile) ||
+      ProfileHelper::IsLockScreenAppProfile(profile)) {
+    delegate = std::make_unique<DelegateForDevice>();
+  } else {
+    delegate = std::make_unique<DelegateForUser>(context);
+  }
+
+  return new PlatformKeysServiceImpl(std::move(delegate));
 }
 
 content::BrowserContext* PlatformKeysServiceFactory::GetBrowserContextToUse(

chrome/browser/chromeos/platform_keys/platform_keys_service_factory.h

@@ -25,6 +25,21 @@ class PlatformKeysServiceFactory : public BrowserContextKeyedServiceFactory {
 
   static PlatformKeysServiceFactory* GetInstance();
 
+  // Returns an instance of PlatformKeysService that allows operations on the
+  // device-wide key store and is not tied to a user.
+  // The lifetime of the returned service is tied to the
+  // PlatformKeysServiceFactory itself.
+  PlatformKeysService* GetDeviceWideService();
+
+  // When call with a nun-nullptr |device_wide_service_for_testing|, subsequent
+  // calls to GetDeviceWideService() will return the passed pointer.
+  // When called with nullptr, subsequent calls to GetDeviceWideService() will
+  // return the default device-wide PlatformKeysService again.
+  // The caller is responsible that this is called with nullptr before an object
+  // previously passed in is destroyed.
+  void SetDeviceWideServiceForTesting(
+      PlatformKeysService* device_wide_service_for_testing);
+
  private:
   friend struct base::DefaultSingletonTraits<PlatformKeysServiceFactory>;
 
@@ -39,6 +54,15 @@ class PlatformKeysServiceFactory : public BrowserContextKeyedServiceFactory {
       content::BrowserContext* context) const override;
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
+
+  PlatformKeysService* GetOrCreateDeviceWideService();
+
+  // A PlatformKeysService that is not tied to a Profile/User and only has
+  // access to the system token.
+  // Initialized lazily.
+  std::unique_ptr<PlatformKeysService> device_wide_service_;
+
+  PlatformKeysService* device_wide_service_for_testing_ = nullptr;
 };
 }  // namespace platform_keys
 }  // namespace chromeos

chrome/browser/chromeos/system_token_cert_db_initializer.cc

@@ -108,6 +108,17 @@ void SystemTokenCertDBInitializer::ShutDown() {
   // Note that the observer could potentially not be added yet, but
   // RemoveObserver() is a no-op in that case.
   CryptohomeClient::Get()->RemoveObserver(this);
+
+  // Cancel any in-progress initialization sequence.
+  weak_ptr_factory_.InvalidateWeakPtrs();
+
+  // Notify observers that the SystemTokenCertDBInitializer and the
+  // NSSCertDatabase it provides can not be used anymore.
+  for (auto& observer : observers_)
+    observer.OnSystemTokenCertDBDestroyed();
+
+  // Now it's safe to destroy the NSSCertDatabase.
+  system_token_cert_database_.reset();
 }
 
 void SystemTokenCertDBInitializer::TpmInitStatusUpdated(
@@ -134,6 +145,18 @@ void SystemTokenCertDBInitializer::GetSystemTokenCertDb(
     get_system_token_cert_db_callback_list_.push_back(std::move(callback));
 }
 
+void SystemTokenCertDBInitializer::AddObserver(
+    SystemTokenCertDBObserver* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  observers_.AddObserver(observer);
+}
+
+void SystemTokenCertDBInitializer::RemoveObserver(
+    SystemTokenCertDBObserver* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  observers_.RemoveObserver(observer);
+}
+
 void SystemTokenCertDBInitializer::OnCryptohomeAvailable(bool available) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 

chrome/browser/chromeos/system_token_cert_db_initializer.h

@@ -11,6 +11,8 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
 #include "base/optional.h"
 #include "base/sequence_checker.h"
 #include "chromeos/dbus/cryptohome/cryptohome_client.h"
@@ -22,6 +24,16 @@ class NSSCertDatabase;
 
 namespace chromeos {
 
+// An observer that gets notified when the global NSSCertDatabase is about to be
+// destroyed.
+class SystemTokenCertDBObserver : public base::CheckedObserver {
+ public:
+  // Called when the global NSSCertDatabase is about to be destroyed.
+  // Consumers of that database should drop any reference to it and stop using
+  // it.
+  virtual void OnSystemTokenCertDBDestroyed() = 0;
+};
+
 // Initializes a global NSSCertDatabase for the system token and starts
 // NetworkCertLoader with that database.
 //
@@ -50,10 +62,17 @@ class SystemTokenCertDBInitializer final : public CryptohomeClient::Observer {
   // |callback|. If the database is already initialized, calls |callback|
   // immediately. Otherwise, |callback| will be called when the database is
   // initialized.
+  // To be notified when the returned NSSCertDatabase becomes invalid, callers
+  // should register as SystemTokenCertDBObserver.
   using GetSystemTokenCertDbCallback =
       base::OnceCallback<void(net::NSSCertDatabase*)>;
   void GetSystemTokenCertDb(GetSystemTokenCertDbCallback callback);
 
+  // Adds |observer| as SystemTokenCertDBObserver.
+  void AddObserver(SystemTokenCertDBObserver* observer);
+  // Removes |observer| as SystemTokenCertDBObserver.
+  void RemoveObserver(SystemTokenCertDBObserver* observer);
+
  private:
   // Called once the cryptohome service is available.
   void OnCryptohomeAvailable(bool available);
@@ -86,6 +105,10 @@ class SystemTokenCertDBInitializer final : public CryptohomeClient::Observer {
   std::vector<GetSystemTokenCertDbCallback>
       get_system_token_cert_db_callback_list_;
 
+  // List of observers that will be notified when the global system token
+  // NSSCertDatabase is destroyed.
+  base::ObserverList<SystemTokenCertDBObserver> observers_;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   base::WeakPtrFactory<SystemTokenCertDBInitializer> weak_ptr_factory_{this};