Split CertDatabase notifications between trust changes and client cer…

…t changes Bug: 915463 Change-Id: Ieda96f9e17ac69238c8c19f6737e5930c3acb312 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4596317 Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Chris Thompson <cthomp@chromium.org> Reviewed-by: Hidehiko Abe <hidehiko@chromium.org> Reviewed-by: Adam Rice <ricea@chromium.org> Reviewed-by: Chad Duffin <chadduffin@chromium.org> Reviewed-by: David Benjamin <davidben@chromium.org> Reviewed-by: Nate Fischer <ntfschr@chromium.org> Reviewed-by: Pavol Marko <pmarko@chromium.org> Commit-Queue: Matt Mueller <mattm@chromium.org> Cr-Commit-Position: refs/heads/main@{#1157277}
Denger-Network · Jun 14, 2023 · 70e3c42 · 70e3c42
1 parent 20ecad7
commit 70e3c42
Show file tree

Hide file tree

Showing 55 changed files with 573 additions and 149 deletions.
diff --git a/android_webview/browser/aw_contents_statics.cc b/android_webview/browser/aw_contents_statics.cc
@@ -46,7 +46,7 @@ void ClientCertificatesCleared(const JavaRef<jobject>& callback) {
 
 void NotifyClientCertificatesChanged() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+  net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
 }
 
 void SafeBrowsingAllowlistAssigned(const JavaRef<jobject>& callback,

diff --git a/chrome/browser/ash/arc/enterprise/cert_store/cert_store_service.cc b/chrome/browser/ash/arc/enterprise/cert_store/cert_store_service.cc
@@ -383,7 +383,7 @@ CertStoreService::~CertStoreService() {
     net::CertDatabase::GetInstance()->RemoveObserver(this);
 }
 
-void CertStoreService::OnCertDBChanged() {
+void CertStoreService::OnClientCertStoreChanged() {
   UpdateCertificates();
 }
 

diff --git a/chrome/browser/ash/arc/enterprise/cert_store/cert_store_service.h b/chrome/browser/ash/arc/enterprise/cert_store/cert_store_service.h
@@ -49,7 +49,7 @@ class CertStoreService : public KeyedService,
   CertStoreService& operator=(const CertStoreService&) = delete;
 
   // CertDatabase::Observer overrides.
-  void OnCertDBChanged() override;
+  void OnClientCertStoreChanged() override;
 
   std::vector<std::string> get_required_cert_names() const {
     return certificate_cache_.get_required_cert_names();

diff --git a/chrome/browser/ash/crosapi/cert_database_ash.cc b/chrome/browser/ash/crosapi/cert_database_ash.cc
@@ -176,9 +176,21 @@ void CertDatabaseAsh::LoggedInStateChanged() {
   is_cert_database_ready_.reset();
 }
 
-void CertDatabaseAsh::OnCertsChangedInLacros() {
+void CertDatabaseAsh::OnCertsChangedInLacros(
+    mojom::CertDatabaseChangeType change_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+  switch (change_type) {
+    case mojom::CertDatabaseChangeType::kUnknown:
+      net::CertDatabase::GetInstance()->NotifyObserversTrustStoreChanged();
+      net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
+      break;
+    case mojom::CertDatabaseChangeType::kTrustStore:
+      net::CertDatabase::GetInstance()->NotifyObserversTrustStoreChanged();
+      break;
+    case mojom::CertDatabaseChangeType::kClientCertStore:
+      net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
+      break;
+  }
 }
 
 void CertDatabaseAsh::AddAshCertDatabaseObserver(
@@ -209,10 +221,11 @@ void CertDatabaseAsh::SetCertsProvidedByExtension(
       extension_id, filtered_certificate_infos);
 }
 
-void CertDatabaseAsh::NotifyCertsChangedInAsh() {
+void CertDatabaseAsh::NotifyCertsChangedInAsh(
+    mojom::CertDatabaseChangeType change_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   for (const auto& observer : observers_) {
-    observer->OnCertsChangedInAsh();
+    observer->OnCertsChangedInAsh(change_type);
   }
 }
 

diff --git a/chrome/browser/ash/crosapi/cert_database_ash.h b/chrome/browser/ash/crosapi/cert_database_ash.h
@@ -42,7 +42,8 @@ class CertDatabaseAsh : public mojom::CertDatabase, ash::LoginState::Observer {
   void GetCertDatabaseInfo(GetCertDatabaseInfoCallback callback) override;
 
   // mojom::CertDatabase
-  void OnCertsChangedInLacros() override;
+  void OnCertsChangedInLacros(
+      mojom::CertDatabaseChangeType change_type) override;
   void AddAshCertDatabaseObserver(
       mojo::PendingRemote<mojom::AshCertDatabaseObserver> observer) override;
   void SetCertsProvidedByExtension(
@@ -52,7 +53,7 @@ class CertDatabaseAsh : public mojom::CertDatabase, ash::LoginState::Observer {
 
   // Notifies observers that were added with `AddAshCertDatabaseObserver` about
   // cert changes in Ash.
-  void NotifyCertsChangedInAsh();
+  void NotifyCertsChangedInAsh(mojom::CertDatabaseChangeType change_type);
 
  private:
   // ash::LoginState::Observer

diff --git a/chrome/browser/ash/policy/networking/network_policy_application_browsertest.cc b/chrome/browser/ash/policy/networking/network_policy_application_browsertest.cc
@@ -573,7 +573,7 @@ class NetworkPolicyApplicationTest : public ash::LoginManagerTest {
     // becomes available for networks. Production code does this through
     // NSSCertDatabase::ImportUserCert.
     ScopedNetworkCertLoaderRefreshWaiter network_cert_loader_refresh_waiter;
-    net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+    net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
     network_cert_loader_refresh_waiter.Wait();
   }
 

diff --git a/chrome/browser/ash/policy/networking/policy_certs_browsertest.cc b/chrome/browser/ash/policy/networking/policy_certs_browsertest.cc
@@ -136,8 +136,8 @@ class WebTrustedCertsChangedObserver
   base::RunLoop run_loop_;
 };
 
-// Allows waiting until the |CertDatabase| notifies its observers that it has
-// changd.
+// Allows waiting until the |CertDatabase| notifies its observers that a client
+// cert change has occurred.
 class CertDatabaseChangedObserver : public net::CertDatabase::Observer {
  public:
   CertDatabaseChangedObserver() {}
@@ -146,7 +146,7 @@ class CertDatabaseChangedObserver : public net::CertDatabase::Observer {
   CertDatabaseChangedObserver& operator=(const CertDatabaseChangedObserver&) =
       delete;
 
-  void OnCertDBChanged() override { run_loop_.Quit(); }
+  void OnClientCertStoreChanged() override { run_loop_.Quit(); }
 
   void Wait() { run_loop_.Run(); }
 

diff --git a/chrome/browser/ash/system_token_cert_db_initializer.cc b/chrome/browser/ash/system_token_cert_db_initializer.cc
@@ -91,12 +91,13 @@ base::TimeDelta GetNextRequestDelay(base::TimeDelta last_delay) {
   return std::min(last_delay * 2, kMaxRequestDelay);
 }
 
-void NotifyCertsChangedInAshOnUIThread() {
+void NotifyCertsChangedInAshOnUIThread(
+    crosapi::mojom::CertDatabaseChangeType change_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   crosapi::CrosapiManager::Get()
       ->crosapi_ash()
       ->cert_database_ash()
-      ->NotifyCertsChangedInAsh();
+      ->NotifyCertsChangedInAsh(change_type);
 }
 
 }  // namespace
@@ -259,9 +260,18 @@ void SystemTokenCertDBInitializer::InitializeDatabase(
   system_token_cert_db_storage->SetDatabase(system_token_cert_database_.get());
 }
 
-void SystemTokenCertDBInitializer::OnCertDBChanged() {
+void SystemTokenCertDBInitializer::OnTrustStoreChanged() {
   content::GetUIThreadTaskRunner({})->PostTask(
-      FROM_HERE, base::BindOnce(&NotifyCertsChangedInAshOnUIThread));
+      FROM_HERE,
+      base::BindOnce(&NotifyCertsChangedInAshOnUIThread,
+                     crosapi::mojom::CertDatabaseChangeType::kTrustStore));
+}
+
+void SystemTokenCertDBInitializer::OnClientCertStoreChanged() {
+  content::GetUIThreadTaskRunner({})->PostTask(
+      FROM_HERE,
+      base::BindOnce(&NotifyCertsChangedInAshOnUIThread,
+                     crosapi::mojom::CertDatabaseChangeType::kClientCertStore));
 }
 
 }  // namespace ash
diff --git a/chrome/browser/ash/system_token_cert_db_initializer.h b/chrome/browser/ash/system_token_cert_db_initializer.h
@@ -56,7 +56,8 @@ class SystemTokenCertDBInitializer
   }
 
   // net::NSSCertDatabase::Observer
-  void OnCertDBChanged() override;
+  void OnTrustStoreChanged() override;
+  void OnClientCertStoreChanged() override;
 
  private:
   // Called once the cryptohome service is available.

diff --git a/chrome/browser/certificate_manager_model.cc b/chrome/browser/certificate_manager_model.cc
@@ -208,7 +208,11 @@ class CertsSourcePlatformNSS : public CertificateManagerModel::CertsSource,
   ~CertsSourcePlatformNSS() override = default;
 
   // net::CertDatabase::Observer
-  void OnCertDBChanged() override {
+  void OnTrustStoreChanged() override {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    Refresh();
+  }
+  void OnClientCertStoreChanged() override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     Refresh();
   }

diff --git a/chrome/browser/chromeos/kcer_nss/kcer_nss_unittest.cc b/chrome/browser/chromeos/kcer_nss/kcer_nss_unittest.cc
@@ -536,14 +536,14 @@ TEST_F(KcerNssTest, ObserveExternalNotification) {
   EXPECT_EQ(observer_1.Notifications(), 0u);
 
   // Check that it receives a notification.
-  net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+  net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
   EXPECT_TRUE(observer_1.WaitUntil(/*notifications=*/1));
 
   // Add one more observer.
   auto subscription_2 = kcer->AddObserver(observer_2.GetCallback());
 
   // Check that both of them receive a notification.
-  net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+  net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
   EXPECT_TRUE(observer_1.WaitUntil(/*notifications=*/2));
   EXPECT_TRUE(observer_2.WaitUntil(/*notifications=*/1));
 
@@ -552,7 +552,7 @@ TEST_F(KcerNssTest, ObserveExternalNotification) {
   subscription_1 = base::CallbackListSubscription();
 
   // Check that only the second observer receives a notification.
-  net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+  net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
   EXPECT_TRUE(observer_2.WaitUntil(/*notifications=*/2));
   EXPECT_TRUE(observer_1.WaitUntil(/*notifications=*/2));
 }

diff --git a/chrome/browser/chromeos/kcer_nss/kcer_token_impl_nss.cc b/chrome/browser/chromeos/kcer_nss/kcer_token_impl_nss.cc
@@ -965,7 +965,7 @@ base::WeakPtr<KcerTokenImplNss> KcerTokenImplNss::GetWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }
 
-void KcerTokenImplNss::OnCertDBChanged() {
+void KcerTokenImplNss::OnClientCertStoreChanged() {
   state_ = State::kCacheOutdated;
 
   // If task queue is not blocked, trigger cache update immediately.
@@ -1398,7 +1398,7 @@ void KcerTokenImplNss::OnCertsModified(Kcer::StatusCallback callback,
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   if (did_modify) {
-    net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+    net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
   }
   // The Notify... above will post a task to invalidate the cache. Calling the
   // original callback for a request will automatically trigger updating cache

diff --git a/chrome/browser/chromeos/kcer_nss/kcer_token_impl_nss.h b/chrome/browser/chromeos/kcer_nss/kcer_token_impl_nss.h
@@ -62,7 +62,7 @@ class KcerTokenImplNss : public KcerToken, public net::CertDatabase::Observer {
   void Initialize(crypto::ScopedPK11Slot nss_slot);
 
   // Implements net::CertDatabase::Observer.
-  void OnCertDBChanged() override;
+  void OnClientCertStoreChanged() override;
 
   // Implements KcerToken.
   void GenerateRsaKey(uint32_t modulus_length_bits,

diff --git a/chrome/browser/lacros/cert/cert_db_initializer_browsertest.cc b/chrome/browser/lacros/cert/cert_db_initializer_browsertest.cc
@@ -199,7 +199,7 @@ class ScopedCertDatabaseObserver : public net::CertDatabase::Observer {
     net::CertDatabase::GetInstance()->RemoveObserver(this);
   }
 
-  void OnCertDBChanged() override {
+  void OnClientCertStoreChanged() override {
     notifications_received_++;
     run_loop_.Quit();
   }

diff --git a/chrome/browser/lacros/cert/cert_db_initializer_impl.cc b/chrome/browser/lacros/cert/cert_db_initializer_impl.cc
@@ -165,7 +165,19 @@ CertDbInitializerImpl::CreateNssCertDatabaseGetterForIOThread() {
                         base::Unretained(cert_db_initializer_io_.get()));
 }
 
-void CertDbInitializerImpl::OnCertsChangedInAsh() {
+void CertDbInitializerImpl::OnCertsChangedInAsh(
+    crosapi::mojom::CertDatabaseChangeType change_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+  switch (change_type) {
+    case crosapi::mojom::CertDatabaseChangeType::kUnknown:
+      net::CertDatabase::GetInstance()->NotifyObserversTrustStoreChanged();
+      net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
+      break;
+    case crosapi::mojom::CertDatabaseChangeType::kTrustStore:
+      net::CertDatabase::GetInstance()->NotifyObserversTrustStoreChanged();
+      break;
+    case crosapi::mojom::CertDatabaseChangeType::kClientCertStore:
+      net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
+      break;
+  }
 }
diff --git a/chrome/browser/lacros/cert/cert_db_initializer_impl.h b/chrome/browser/lacros/cert/cert_db_initializer_impl.h
@@ -74,7 +74,8 @@ class CertDbInitializerImpl : public CertDbInitializer,
 
   // Called when there's a change in certificate database in Ash.
   // Forwards the notification to the CertDatabase.
-  void OnCertsChangedInAsh() override;
+  void OnCertsChangedInAsh(
+      crosapi::mojom::CertDatabaseChangeType change_type) override;
 
  private:
   void InitializeForMainProfile();

diff --git a/chrome/browser/lacros/cert/cert_db_initializer_io_impl.cc b/chrome/browser/lacros/cert/cert_db_initializer_io_impl.cc
@@ -103,7 +103,8 @@ void LoadSlotsOnWorkerThread(
   std::move(callback).Run(std::move(private_slot), std::move(system_slot));
 }
 
-void NotifyCertsChangedInLacrosOnUIThread() {
+void NotifyCertsChangedInLacrosOnUIThread(
+    crosapi::mojom::CertDatabaseChangeType change_type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   chromeos::LacrosService* service = chromeos::LacrosService::Get();
@@ -116,7 +117,7 @@ void NotifyCertsChangedInLacrosOnUIThread() {
 
   chromeos::LacrosService::Get()
       ->GetRemote<crosapi::mojom::CertDatabase>()
-      ->OnCertsChangedInLacros();
+      ->OnCertsChangedInLacros(change_type);
 }
 
 }  // namespace
@@ -218,7 +219,16 @@ void CertDbInitializerIOImpl::InitializeReadOnlyNssCertDatabase(
   ready_callback_list_.Notify(nss_cert_database_.get());
 }
 
-void CertDbInitializerIOImpl::OnCertDBChanged() {
+void CertDbInitializerIOImpl::OnTrustStoreChanged() {
   content::GetUIThreadTaskRunner({})->PostTask(
-      FROM_HERE, base::BindOnce(NotifyCertsChangedInLacrosOnUIThread));
+      FROM_HERE,
+      base::BindOnce(NotifyCertsChangedInLacrosOnUIThread,
+                     crosapi::mojom::CertDatabaseChangeType::kTrustStore));
+}
+
+void CertDbInitializerIOImpl::OnClientCertStoreChanged() {
+  content::GetUIThreadTaskRunner({})->PostTask(
+      FROM_HERE,
+      base::BindOnce(NotifyCertsChangedInLacrosOnUIThread,
+                     crosapi::mojom::CertDatabaseChangeType::kClientCertStore));
 }
diff --git a/chrome/browser/lacros/cert/cert_db_initializer_io_impl.h b/chrome/browser/lacros/cert/cert_db_initializer_io_impl.h
@@ -76,7 +76,8 @@ class CertDbInitializerIOImpl : public net::NSSCertDatabase::Observer {
   void InitializeReadOnlyNssCertDatabase(base::OnceClosure init_callback);
 
   // net::NSSCertDatabase::Observer
-  void OnCertDBChanged() override;
+  void OnTrustStoreChanged() override;
+  void OnClientCertStoreChanged() override;
 
  private:
   void DidLoadSoftwareNssDb(base::OnceClosure load_callback,

diff --git a/chrome/browser/net/nss_service_chromeos.cc b/chrome/browser/net/nss_service_chromeos.cc
@@ -146,12 +146,13 @@ void StartNSSInitOnIOThread(const AccountId& account_id,
       &StartTPMSlotInitializationOnIOThread, account_id, username_hash));
 }
 
-void NotifyCertsChangedInAshOnUIThread() {
+void NotifyCertsChangedInAshOnUIThread(
+    crosapi::mojom::CertDatabaseChangeType change_type) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   crosapi::CrosapiManager::Get()
       ->crosapi_ash()
       ->cert_database_ash()
-      ->NotifyCertsChangedInAsh();
+      ->NotifyCertsChangedInAsh(change_type);
 }
 
 }  // namespace
@@ -205,9 +206,18 @@ class NssService::NSSCertDatabaseChromeOSManager
   }
 
   // net::NSSCertDatabase::Observer
-  void OnCertDBChanged() override {
+  void OnTrustStoreChanged() override {
     content::GetUIThreadTaskRunner({})->PostTask(
-        FROM_HERE, base::BindOnce(&NotifyCertsChangedInAshOnUIThread));
+        FROM_HERE,
+        base::BindOnce(&NotifyCertsChangedInAshOnUIThread,
+                       crosapi::mojom::CertDatabaseChangeType::kTrustStore));
+  }
+  void OnClientCertStoreChanged() override {
+    content::GetUIThreadTaskRunner({})->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &NotifyCertsChangedInAshOnUIThread,
+            crosapi::mojom::CertDatabaseChangeType::kClientCertStore));
   }
 
  private:

diff --git a/chrome/browser/policy/test/policy_certs_browsertest.cc b/chrome/browser/policy/test/policy_certs_browsertest.cc
@@ -132,24 +132,6 @@ class NetworkCertLoaderTestObserver : public ash::NetworkCertLoader::Observer {
   base::RunLoop run_loop_;
 };
 
-// Allows waiting until the |CertDatabase| notifies its observers that it has
-// changd.
-class CertDatabaseChangedObserver : public net::CertDatabase::Observer {
- public:
-  CertDatabaseChangedObserver() {}
-
-  CertDatabaseChangedObserver(const CertDatabaseChangedObserver&) = delete;
-  CertDatabaseChangedObserver& operator=(const CertDatabaseChangedObserver&) =
-      delete;
-
-  void OnCertDBChanged() override { run_loop_.Quit(); }
-
-  void Wait() { run_loop_.Run(); }
-
- private:
-  base::RunLoop run_loop_;
-};
-
 // Retrieves the path to the directory containing certificates designated for
 // testing of policy-provided certificates into *|out_test_certs_path|.
 base::FilePath GetTestCertsPath() {

diff --git a/chrome/browser/ssl/ssl_browsertest.cc b/chrome/browser/ssl/ssl_browsertest.cc
@@ -2125,8 +2125,8 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, TestCertDBChangedFlushesClientAuthCache) {
       browser(), GURL("about:blank"), 1);
   EXPECT_EQ("", tab->GetLastCommittedURL().ref());
 
-  // Send a CertDBChanged notification.
-  net::CertDatabase::GetInstance()->NotifyObserversCertDBChanged();
+  // Send an OnClientCertStoreChanged notification.
+  net::CertDatabase::GetInstance()->NotifyObserversClientCertStoreChanged();
   content::FlushNetworkServiceInstanceForTesting();
 
   // Visiting the page which requires client certs should fail, as the socket
@@ -5807,7 +5807,7 @@ class TestCertDatabaseObserver : public net::CertDatabase::Observer {
     cert_db->AddObserver(this);
   }
   ~TestCertDatabaseObserver() override = default;
-  void OnCertDBChanged() override { run_loop.Quit(); }
+  void OnTrustStoreChanged() override { run_loop.Quit(); }
   void WaitForCertDBChange() { run_loop.Run(); }
 
  private: