[Lacros] Suspend web requests until client certs are ready

This CL makes web requests that require client certificates
wait until the client certificate storage is loaded. Without
that they proceed as if there is no suitable certificate and
the browser doesn't retry to find a certificate for the same
connection (including on the page refresh).

Add a method to CertDbInitializer that allows to wait for the
initialization to finish. Wrap the base ClientCertStoreNSS
class into a new class and wait in it for the initialization to
finish before continuing the retrieval of client certificates.

Bug: 1145946
Change-Id: I1f5ab87d93c3eedea75f86a9769dcaddb0cd8404
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2574757Reviewed-by: Michael Ershov <miersh@google.com>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Michael Ershov <miersh@google.com>
Cr-Commit-Position: refs/heads/master@{#836434}

chrome/browser/BUILD.gn

@@ -4580,6 +4580,8 @@ static_library("browser") {
       "lacros/cert_db_initializer_factory.h",
       "lacros/cert_db_initializer_impl.cc",
       "lacros/cert_db_initializer_impl.h",
+      "lacros/client_cert_store_lacros.cc",
+      "lacros/client_cert_store_lacros.h",
       "lacros/feedback_util.cc",
       "lacros/feedback_util.h",
       "lacros/immersive_context_lacros.cc",

chrome/browser/lacros/cert_db_initializer.h

@@ -5,10 +5,22 @@
 #ifndef CHROME_BROWSER_LACROS_CERT_DB_INITIALIZER_H_
 #define CHROME_BROWSER_LACROS_CERT_DB_INITIALIZER_H_
 
+#include "base/callback.h"
+#include "base/callback_list.h"
+
 class CertDbInitializer {
  public:
-  // TODO(miersh): implement `WaitUntilReady`.
-  // virtual void WaitUntilReady() = 0;
+  using ReadyCallback = base::OnceCallback<void(bool is_success)>;
+
+  virtual ~CertDbInitializer() = default;
+
+  // Registers `callback` to be notified once initialization is complete.
+  // If initialization has already been completed, `callback` will be
+  // synchronously invoked and an empty subscription returned; otherwise,
+  // `callback` will be invoked when initialization is completed, as long
+  // as the subscription is still live.
+  virtual base::CallbackListSubscription WaitUntilReady(
+      ReadyCallback callback) = 0;
 };
 
 #endif  // CHROME_BROWSER_LACROS_CERT_DB_INITIALIZER_H_

chrome/browser/lacros/cert_db_initializer_factory.cc

@@ -10,6 +10,7 @@
 #include "chromeos/lacros/lacros_chrome_service_impl.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 
+class CertDbInitializer;
 class Profile;
 
 // static
@@ -18,6 +19,13 @@ CertDbInitializerFactory* CertDbInitializerFactory::GetInstance() {
   return factory.get();
 }
 
+// static
+CertDbInitializer* CertDbInitializerFactory::GetForProfileIfExists(
+    Profile* profile) {
+  return static_cast<CertDbInitializerImpl*>(
+      GetInstance()->GetServiceForBrowserContext(profile, /*create=*/false));
+}
+
 CertDbInitializerFactory::CertDbInitializerFactory()
     : BrowserContextKeyedServiceFactory(
           "CertDbInitializerFactory",

chrome/browser/lacros/cert_db_initializer_factory.h

@@ -8,9 +8,13 @@
 #include "base/no_destructor.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 
+class CertDbInitializer;
+class Profile;
+
 class CertDbInitializerFactory : public BrowserContextKeyedServiceFactory {
  public:
   static CertDbInitializerFactory* GetInstance();
+  static CertDbInitializer* GetForProfileIfExists(Profile* profile);
 
  private:
   friend class base::NoDestructor<CertDbInitializerFactory>;

chrome/browser/lacros/cert_db_initializer_impl.cc

@@ -15,6 +15,7 @@
 #include "chromeos/crosapi/cpp/crosapi_constants.h"
 #include "chromeos/crosapi/mojom/cert_database.mojom.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
+#include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/chaps_support.h"
 #include "crypto/nss_util.h"
@@ -23,7 +24,7 @@
 
 namespace {
 
-void InitializeCertDbOnWorkerThread(bool should_load_chaps,
+bool InitializeCertDbOnWorkerThread(bool should_load_chaps,
                                     base::FilePath software_nss_db_path) {
   crypto::EnsureNSSInit();
 
@@ -41,7 +42,7 @@ void InitializeCertDbOnWorkerThread(bool should_load_chaps,
     // success/failure.
     if (!crypto::LoadChaps()) {
       LOG(ERROR) << "Failed to load chaps.";
-      return;
+      return false;
     }
   }
 
@@ -52,10 +53,10 @@ void InitializeCertDbOnWorkerThread(bool should_load_chaps,
                                         /*description=*/"cert_db");
   if (!slot) {
     LOG(ERROR) << "Failed to open user certificate database";
-    return;
+    return false;
   }
 
-  return;
+  return true;
 }
 
 }  // namespace
@@ -101,10 +102,13 @@ CertDbInitializerImpl::CertDbInitializerImpl(Profile* profile)
 
 CertDbInitializerImpl::~CertDbInitializerImpl() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // In case the initialization didn't finish, notify waiting observers.
+  OnCertDbInitializationFinished(false);
 }
 
 void CertDbInitializerImpl::Start(signin::IdentityManager* identity_manager) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   DCHECK(identity_manager);
   // TODO(crbug.com/1148300): This is temporary. Until ~2021
   // `Profile::IsMainProfile()` method can return a false negative response if
@@ -122,6 +126,18 @@ void CertDbInitializerImpl::Start(signin::IdentityManager* identity_manager) {
   WaitForCertDbReady();
 }
 
+base::CallbackListSubscription CertDbInitializerImpl::WaitUntilReady(
+    ReadyCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (is_ready_.has_value()) {
+    std::move(callback).Run(is_ready_.value());
+    return {};
+  }
+
+  return callbacks_.Add(std::move(callback));
+}
+
 void CertDbInitializerImpl::OnRefreshTokensLoaded() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   identity_manager_observer_.reset();
@@ -132,6 +148,7 @@ void CertDbInitializerImpl::WaitForCertDbReady() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (!profile_->IsMainProfile()) {
+    OnCertDbInitializationFinished(false);
     return;
   }
 
@@ -148,14 +165,23 @@ void CertDbInitializerImpl::OnCertDbInfoReceived(
 
   if (!cert_db_info) {
     LOG(WARNING) << "Certificate database is not accesible";
+    OnCertDbInitializationFinished(false);
     return;
   }
 
-  base::ThreadPool::PostTask(
+  base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
       {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
        base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
       base::BindOnce(&InitializeCertDbOnWorkerThread,
                      cert_db_info->should_load_chaps,
-                     base::FilePath(cert_db_info->software_nss_db_path)));
+                     base::FilePath(cert_db_info->software_nss_db_path)),
+      base::BindOnce(&CertDbInitializerImpl::OnCertDbInitializationFinished,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void CertDbInitializerImpl::OnCertDbInitializationFinished(bool is_success) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  callbacks_.Notify(is_success);
+  is_ready_ = is_success;
 }

chrome/browser/lacros/cert_db_initializer_impl.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_LACROS_CERT_DB_INITIALIZER_IMPL_H_
 #define CHROME_BROWSER_LACROS_CERT_DB_INITIALIZER_IMPL_H_
 
+#include "base/callback_list.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/lacros/cert_db_initializer.h"
 #include "chromeos/crosapi/mojom/cert_database.mojom.h"
@@ -28,6 +29,10 @@ class CertDbInitializerImpl : public CertDbInitializer, public KeyedService {
   // IdentityManager.
   void Start(signin::IdentityManager* identity_manager);
 
+  // CertDbInitializer
+  base::CallbackListSubscription WaitUntilReady(
+      ReadyCallback callback) override;
+
  private:
   // It is called when IdentityManager is ready.
   void OnRefreshTokensLoaded();
@@ -41,10 +46,17 @@ class CertDbInitializerImpl : public CertDbInitializer, public KeyedService {
   void OnCertDbInfoReceived(
       crosapi::mojom::GetCertDatabaseInfoResultPtr result);
 
-  // This class is `KeyedService` based on the `Profile`. An instance is
+  // Recieves initialization result from a worker thread and notifies observers
+  // about the result.
+  void OnCertDbInitializationFinished(bool is_success);
+
+  // This class is a `KeyedService` based on the `Profile`. An instance is
   // created together with a new profile and never outlives it.`
   Profile* profile_ = nullptr;
   std::unique_ptr<IdentityManagerObserver> identity_manager_observer_;
+  base::Optional<bool> is_ready_;
+  base::OnceCallbackList<ReadyCallback::RunType> callbacks_;
+
   base::WeakPtrFactory<CertDbInitializerImpl> weak_factory_{this};
 };
 

chrome/browser/lacros/client_cert_store_lacros.cc

+// Copyright 2020 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.
+
+#include "chrome/browser/lacros/client_cert_store_lacros.h"
+
+#include "base/memory/scoped_refptr.h"
+#include "chrome/browser/lacros/cert_db_initializer.h"
+#include "net/ssl/client_cert_store_nss.h"
+#include "net/ssl/ssl_cert_request_info.h"
+
+ClientCertStoreLacros::ClientCertStoreLacros(
+    CertDbInitializer* cert_db_initializer,
+    std::unique_ptr<net::ClientCertStore> underlying_store)
+    : cert_db_initializer_(cert_db_initializer),
+      underlying_store_(std::move(underlying_store)) {
+  DCHECK(underlying_store_);
+  DCHECK(cert_db_initializer_);
+
+  WaitForCertDb();
+}
+
+ClientCertStoreLacros::~ClientCertStoreLacros() = default;
+
+void ClientCertStoreLacros::GetClientCerts(
+    const net::SSLCertRequestInfo& cert_request_info,
+    ClientCertListCallback callback) {
+  if (!are_certs_loaded_) {
+    pending_requests_.push_back(std::make_pair(
+        WrapRefCounted(&cert_request_info), std::move(callback)));
+    return;
+  }
+
+  underlying_store_->GetClientCerts(cert_request_info, std::move(callback));
+}
+
+void ClientCertStoreLacros::WaitForCertDb() {
+  wait_subscription_ = cert_db_initializer_->WaitUntilReady(base::BindOnce(
+      &ClientCertStoreLacros::OnCertDbReady, weak_factory_.GetWeakPtr()));
+}
+
+void ClientCertStoreLacros::OnCertDbReady(bool /*is_cert_db_ready*/) {
+  // Ignore the initialization result. Even if it failed, some certificates
+  // could be accessible.
+
+  // Ensure any new requests (e.g. that result from invoking the
+  // callbacks) aren't queued.
+  are_certs_loaded_ = true;
+
+  // Move the pending requests to the stack, since `this` may
+  // be deleted by the last request callback.
+  decltype(pending_requests_) local_requests;
+  std::swap(pending_requests_, local_requests);
+
+  // Dispatch all the queued requests.
+  for (auto& request : local_requests) {
+    underlying_store_->GetClientCerts(*request.first,
+                                      std::move(request.second));
+  }
+}

chrome/browser/lacros/client_cert_store_lacros.h

+// Copyright 2020 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 CHROME_BROWSER_LACROS_CLIENT_CERT_STORE_LACROS_H_
+#define CHROME_BROWSER_LACROS_CLIENT_CERT_STORE_LACROS_H_
+
+#include "base/callback_list.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "net/ssl/client_cert_store.h"
+#include "net/ssl/client_cert_store_nss.h"
+
+class CertDbInitializer;
+
+// Provides client certs for Lacros-Chrome.
+// Client certificates may not be available during initialization, as
+// Lacros-Chrome needs to get configuration data from Ash-Chrome for the
+// user. ClientCertStoreLacros will queue requests for client certificates
+// until CertDbInitializer has completed initializing, and then dispatch
+// requests to the underlying ClientCertStore.
+class ClientCertStoreLacros final : public net::ClientCertStore {
+ public:
+  ClientCertStoreLacros(CertDbInitializer* cert_db_initializer,
+                        std::unique_ptr<net::ClientCertStore> underlying_store);
+  ClientCertStoreLacros(const ClientCertStoreLacros&) = delete;
+  ClientCertStoreLacros& operator=(ClientCertStoreLacros&) = delete;
+  ~ClientCertStoreLacros() override;
+
+  // net::ClientCertStore
+  void GetClientCerts(const net::SSLCertRequestInfo& cert_request_info,
+                      ClientCertListCallback callback) override;
+
+ private:
+  using RequestQueue =
+      std::vector<std::pair<scoped_refptr<const net::SSLCertRequestInfo>,
+                            ClientCertListCallback>>;
+
+  void WaitForCertDb();
+  void OnCertDbReady(bool is_cert_db_ready);
+
+  bool are_certs_loaded_ = false;
+  CertDbInitializer* cert_db_initializer_ = nullptr;
+  base::CallbackListSubscription wait_subscription_;
+  RequestQueue pending_requests_;
+
+  std::unique_ptr<net::ClientCertStore> underlying_store_;
+
+  base::WeakPtrFactory<ClientCertStoreLacros> weak_factory_{this};
+};
+
+#endif  // CHROME_BROWSER_LACROS_CLIENT_CERT_STORE_LACROS_H_

chrome/browser/net/profile_network_context_service.cc

@@ -97,6 +97,11 @@
 #include "extensions/common/constants.h"
 #endif
 
+#if BUILDFLAG(IS_LACROS)
+#include "chrome/browser/lacros/cert_db_initializer_factory.h"
+#include "chrome/browser/lacros/client_cert_store_lacros.h"
+#endif
+
 namespace {
 
 bool* g_discard_domain_reliability_uploads_for_testing = nullptr;
@@ -575,18 +580,24 @@ ProfileNetworkContextService::CreateClientCertStore() {
       base::BindRepeating(&CreateCryptoModuleBlockingPasswordDelegate,
                           kCryptoModulePasswordClientAuth));
 #elif defined(USE_NSS_CERTS)
-
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-  if (!profile_->IsMainProfile()) {
+  std::unique_ptr<net::ClientCertStore> store =
+      std::make_unique<net::ClientCertStoreNSS>(
+          base::BindRepeating(&CreateCryptoModuleBlockingPasswordDelegate,
+                              kCryptoModulePasswordClientAuth));
+#if BUILDFLAG(IS_LACROS)
+  CertDbInitializer* cert_db_initializer =
+      CertDbInitializerFactory::GetForProfileIfExists(profile_);
+  if (!cert_db_initializer || !profile_->IsMainProfile()) {
     // TODO(crbug.com/1148298): return some cert store for secondary profiles in
     // Lacros-Chrome.
     return nullptr;
   }
-#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
 
-  return std::make_unique<net::ClientCertStoreNSS>(
-      base::BindRepeating(&CreateCryptoModuleBlockingPasswordDelegate,
-                          kCryptoModulePasswordClientAuth));
+  store = std::make_unique<ClientCertStoreLacros>(cert_db_initializer,
+                                                  std::move(store));
+#endif  // BUILDFLAG(IS_LACROS)
+
+  return store;
 #elif defined(OS_WIN)
   return std::make_unique<net::ClientCertStoreWin>();
 #elif defined(OS_MAC)

chrome/test/BUILD.gn

@@ -4909,6 +4909,7 @@ test("unit_tests") {
     assert(enable_native_notifications)
     sources += [
       "../browser/lacros/account_manager_facade_lacros_unittest.cc",
+      "../browser/lacros/client_cert_store_lacros_unittest.cc",
       "../browser/lacros/lacros_chrome_service_delegate_impl_unittest.cc",
       "../browser/lacros/metrics_reporting_observer_unittest.cc",
       "../browser/notifications/notification_platform_bridge_lacros_unittest.cc",