Test for system cert token loading after TPM ready

Add a browser test for the logic that triggers the system slot
initialization whenever the TPM goes into the "ready" state as
notified by cryptohomed (added in r698007).

The test exercises the following codepath (this description
skips some intermediate callers for the sake of brevity):
1. SystemTokenCertDBInitializer is notified by the
   chromeos::[Fake]CryptohomeClient that TpmInitStatusUpdated
   with |ready|==true;
2. chromeos::TPMTokenLoader::EnsureStarted() gets called;
3. crypto::InitializeTPMTokenAndSystemSlot() gets called.

This is verified by the test by opening a web page in the
Chrome OS Login screen that uses client cert authentication.
If the codepath mentioned above wouldn't work, the test would
fail due to ClientCertFilterChromeOS waiting infinitely for
the result from crypto::GetSystemNSSKeySlot().

Bug: 725500
Change-Id: I17e0fa42d3fda7d0deed5f86a2ab99b079865b36
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1815857
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705243}

chrome/browser/chromeos/login/webview_login_browsertest.cc

@@ -43,7 +43,9 @@
 #include "chrome/browser/ui/webui/signin/signin_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "chromeos/constants/chromeos_switches.h"
+#include "chromeos/dbus/cryptohome/fake_cryptohome_client.h"
 #include "chromeos/dbus/session_manager/fake_session_manager_client.h"
+#include "chromeos/tpm/tpm_token_loader.h"
 #include "components/content_settings/core/common/pref_names.h"
 #include "components/guest_view/browser/guest_view_manager.h"
 #include "components/onc/onc_constants.h"
@@ -62,7 +64,9 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
+#include "crypto/nss_util.h"
 #include "crypto/nss_util_internal.h"
+#include "crypto/scoped_test_nss_db.h"
 #include "crypto/scoped_test_system_nss_key_slot.h"
 #include "media/base/media_switches.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -424,33 +428,14 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, RequestCamera) {
   EXPECT_FALSE(getUserMediaSuccess);
 }
 
-class WebviewClientCertsLoginTest : public WebviewLoginTest {
+// Base class for tests of the client certificates in the sign-in frame.
+class WebviewClientCertsLoginTestBase : public WebviewLoginTest {
  public:
-  WebviewClientCertsLoginTest() {}
-
-  // Installs a testing system slot and imports a client certificate into it.
-  void SetUpClientCertInSystemSlot() {
-    {
-      bool system_slot_constructed_successfully = false;
-      base::RunLoop loop;
-      base::PostTaskAndReply(
-          FROM_HERE, {content::BrowserThread::IO},
-          base::BindOnce(&WebviewClientCertsLoginTest::SetUpTestSystemSlotOnIO,
-                         base::Unretained(this),
-                         &system_slot_constructed_successfully),
-          loop.QuitClosure());
-      loop.Run();
-      ASSERT_TRUE(system_slot_constructed_successfully);
-    }
-
-    // Import a second client cert signed by another CA than client_1 into the
-    // system wide key slot.
-    base::ScopedAllowBlockingForTesting allow_io;
-    client_cert_ = net::ImportClientCertAndKeyFromFile(
-        net::GetTestCertsDirectory(), "client_1.pem", "client_1.pk8",
-        test_system_slot_->slot());
-    ASSERT_TRUE(client_cert_.get());
-  }
+  WebviewClientCertsLoginTestBase() = default;
+  WebviewClientCertsLoginTestBase(const WebviewClientCertsLoginTestBase&) =
+      delete;
+  WebviewClientCertsLoginTestBase& operator=(
+      const WebviewClientCertsLoginTestBase&) = delete;
 
   // Sets up the DeviceLoginScreenAutoSelectCertificateForUrls policy.
   void SetAutoSelectCertificatePatterns(
@@ -567,34 +552,16 @@ class WebviewClientCertsLoginTest : public WebviewLoginTest {
     WebviewLoginTest::SetUpInProcessBrowserTestFixture();
   }
 
-  void TearDownOnMainThread() override {
-    TearDownTestSystemSlot();
-    WebviewLoginTest::TearDownOnMainThread();
+  bool ImportSystemSlotClientCert(PK11SlotInfo* system_slot) {
+    base::ScopedAllowBlockingForTesting allow_io;
+    scoped_refptr<net::X509Certificate> client_cert =
+        net::ImportClientCertAndKeyFromFile(net::GetTestCertsDirectory(),
+                                            "client_1.pem", "client_1.pk8",
+                                            system_slot);
+    return client_cert.get() != nullptr;
   }
 
  private:
-  void SetUpTestSystemSlotOnIO(bool* out_system_slot_constructed_successfully) {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    test_system_slot_ = std::make_unique<crypto::ScopedTestSystemNSSKeySlot>();
-    *out_system_slot_constructed_successfully =
-        test_system_slot_->ConstructedSuccessfully();
-  }
-
-  void TearDownTestSystemSlot() {
-    if (!test_system_slot_)
-      return;
-
-    base::RunLoop loop;
-    base::PostTaskAndReply(
-        FROM_HERE, {content::BrowserThread::IO},
-        base::BindOnce(&WebviewClientCertsLoginTest::TearDownTestSystemSlotOnIO,
-                       base::Unretained(this)),
-        loop.QuitClosure());
-    loop.Run();
-  }
-
-  void TearDownTestSystemSlotOnIO() { test_system_slot_.reset(); }
-
   // Builds a device ONC dictionary defining a single untrusted authority
   // certificate.
   base::DictionaryValue BuildDeviceOncDictForUntrustedAuthority(
@@ -619,12 +586,61 @@ class WebviewClientCertsLoginTest : public WebviewLoginTest {
   }
 
   policy::DevicePolicyBuilder device_policy_builder_;
-  std::unique_ptr<crypto::ScopedTestSystemNSSKeySlot> test_system_slot_;
-  scoped_refptr<net::X509Certificate> client_cert_;
   std::unique_ptr<net::SpawnedTestServer> https_server_;
 
   DeviceStateMixin device_state_{
       &mixin_host_, DeviceStateMixin::State::OOBE_COMPLETED_CLOUD_ENROLLED};
+};
+
+// Tests of the client certificates in the sign-in frame. The testing system
+// slot is pre-initialized with a client cert.
+class WebviewClientCertsLoginTest : public WebviewClientCertsLoginTestBase {
+ public:
+  WebviewClientCertsLoginTest() = default;
+
+  // Installs a testing system slot and imports a client certificate into it.
+  void SetUpClientCertInSystemSlot() {
+    bool system_slot_constructed_successfully = false;
+    base::RunLoop loop;
+    base::PostTaskAndReply(
+        FROM_HERE, {content::BrowserThread::IO},
+        base::BindOnce(&WebviewClientCertsLoginTest::SetUpTestSystemSlotOnIO,
+                       base::Unretained(this),
+                       &system_slot_constructed_successfully),
+        loop.QuitClosure());
+    loop.Run();
+    ASSERT_TRUE(system_slot_constructed_successfully);
+
+    ASSERT_TRUE(ImportSystemSlotClientCert(test_system_slot_->slot()));
+  }
+
+ protected:
+  void TearDownOnMainThread() override {
+    TearDownTestSystemSlot();
+    WebviewClientCertsLoginTestBase::TearDownOnMainThread();
+  }
+
+ private:
+  void SetUpTestSystemSlotOnIO(bool* out_system_slot_constructed_successfully) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    test_system_slot_ = std::make_unique<crypto::ScopedTestSystemNSSKeySlot>();
+    *out_system_slot_constructed_successfully =
+        test_system_slot_->ConstructedSuccessfully();
+  }
+
+  void TearDownTestSystemSlot() {
+    base::RunLoop loop;
+    base::PostTaskAndReply(
+        FROM_HERE, {content::BrowserThread::IO},
+        base::BindOnce(&WebviewClientCertsLoginTest::TearDownTestSystemSlotOnIO,
+                       base::Unretained(this)),
+        loop.QuitClosure());
+    loop.Run();
+  }
+
+  void TearDownTestSystemSlotOnIO() { test_system_slot_.reset(); }
+
+  std::unique_ptr<crypto::ScopedTestSystemNSSKeySlot> test_system_slot_;
 
   DISALLOW_COPY_AND_ASSIGN(WebviewClientCertsLoginTest);
 };
@@ -880,6 +896,142 @@ IN_PROC_BROWSER_TEST_F(WebviewClientCertsLoginTest,
   EXPECT_EQ("got no client cert", https_reply_content);
 }
 
+// Tests the scenario where the system token is not initialized initially (due
+// to the TPM not being ready).
+class WebviewClientCertsTokenLoadingLoginTest
+    : public WebviewClientCertsLoginTestBase {
+ public:
+  WebviewClientCertsTokenLoadingLoginTest()
+      : cryptohome_client_(new FakeCryptohomeClient) {
+    cryptohome_client_->set_tpm_is_ready(false);
+  }
+
+  WebviewClientCertsTokenLoadingLoginTest(
+      const WebviewClientCertsTokenLoadingLoginTest&) = delete;
+  WebviewClientCertsTokenLoadingLoginTest& operator=(
+      const WebviewClientCertsTokenLoadingLoginTest&) = delete;
+
+  FakeCryptohomeClient* cryptohome_client() { return cryptohome_client_; }
+
+  // Prepares a testing system slot (without injecting it as an already
+  // initialized yet) and imports a client certificate into it.
+  void PrepareSystemSlot() {
+    bool out_system_slot_prepared_successfully = false;
+    base::RunLoop loop;
+    base::PostTaskAndReply(
+        FROM_HERE, {content::BrowserThread::IO},
+        base::BindOnce(
+            &WebviewClientCertsTokenLoadingLoginTest::PrepareSystemSlotOnIO,
+            base::Unretained(this), &out_system_slot_prepared_successfully),
+        loop.QuitClosure());
+    loop.Run();
+    ASSERT_TRUE(out_system_slot_prepared_successfully);
+
+    ASSERT_TRUE(ImportSystemSlotClientCert(test_system_slot_nss_db_->slot()));
+  }
+
+ protected:
+  void SetUpOnMainThread() override {
+    TPMTokenLoader::Get()->enable_tpm_loading_for_testing(true);
+    WebviewClientCertsLoginTestBase::SetUpOnMainThread();
+  }
+
+  void TearDownOnMainThread() override {
+    TearDownTestSystemSlot();
+    WebviewClientCertsLoginTestBase::TearDownOnMainThread();
+  }
+
+ private:
+  void PrepareSystemSlotOnIO(bool* out_system_slot_prepared_successfully) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    test_system_slot_nss_db_ = std::make_unique<crypto::ScopedTestNSSDB>();
+    crypto::SetSystemKeySlotWithoutInitializingTPMForTesting(
+        crypto::ScopedPK11Slot(
+            PK11_ReferenceSlot(test_system_slot_nss_db_->slot())));
+    *out_system_slot_prepared_successfully =
+        test_system_slot_nss_db_->is_open();
+  }
+
+  void TearDownTestSystemSlot() {
+    base::RunLoop loop;
+    base::PostTaskAndReply(
+        FROM_HERE, {content::BrowserThread::IO},
+        base::BindOnce(&WebviewClientCertsTokenLoadingLoginTest::
+                           TearDownTestSystemSlotOnIO,
+                       base::Unretained(this)),
+        loop.QuitClosure());
+    loop.Run();
+  }
+
+  void TearDownTestSystemSlotOnIO() {
+    crypto::SetSystemKeySlotWithoutInitializingTPMForTesting(/*slot=*/nullptr);
+    test_system_slot_nss_db_.reset();
+  }
+
+  // Owned by the CryptohomeClient singleton.
+  FakeCryptohomeClient* cryptohome_client_;
+
+  std::unique_ptr<crypto::ScopedTestNSSDB> test_system_slot_nss_db_;
+};
+
+namespace {
+
+bool IsTpmTokenReady() {
+  base::RunLoop run_loop;
+  bool is_ready = false;
+  base::PostTaskAndReplyWithResult(
+      FROM_HERE, {content::BrowserThread::IO},
+      base::BindOnce(&crypto::IsTPMTokenReady,
+                     /*callback=*/base::OnceClosure()),
+      base::BindOnce(
+          [](base::OnceClosure run_loop_quit_closure, bool* is_ready,
+             bool is_tpm_token_ready) {
+            *is_ready = is_tpm_token_ready;
+            std::move(run_loop_quit_closure).Run();
+          },
+          run_loop.QuitClosure(), base::Unretained(&is_ready)));
+  run_loop.Run();
+  return is_ready;
+}
+
+}  // namespace
+
+// Test that the system slot becomes initialized and the client certificate
+// authentication works in the sign-in frame after the TPM gets reported as
+// ready.
+IN_PROC_BROWSER_TEST_F(WebviewClientCertsTokenLoadingLoginTest,
+                       SystemSlotInitialization) {
+  ASSERT_NO_FATAL_FAILURE(PrepareSystemSlot());
+  net::SpawnedTestServer::SSLOptions ssl_options;
+  ssl_options.request_client_certificate = true;
+  ASSERT_NO_FATAL_FAILURE(StartHttpsServer(ssl_options));
+
+  const std::vector<std::string> autoselect_patterns = {
+      R"({"pattern": "*", "filter": {"ISSUER": {"CN": "B CA"}}})"};
+  SetAutoSelectCertificatePatterns(autoselect_patterns);
+
+  WaitForGaiaPageLoadAndPropertyUpdate();
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(IsTpmTokenReady());
+
+  // Report the TPM as ready, triggering the system token initialization by
+  // SystemTokenCertDBInitializer.
+  cryptohome_client()->set_tpm_is_ready(true);
+  cryptohome_client()->NotifyTpmInitStatusUpdated(
+      /*ready=*/true, /*owned=*/true,
+      /*was_owned_this_boot=*/false);
+
+  const std::string https_reply_content =
+      RequestClientCertTestPageInFrame({"gaia-signin", gaia_frame_parent_});
+  EXPECT_EQ(
+      "got client cert with fingerprint: "
+      "c66145f49caca4d1325db96ace0f12f615ba4981",
+      https_reply_content);
+
+  EXPECT_TRUE(IsTpmTokenReady());
+}
+
 class WebviewProxyAuthLoginTest : public WebviewLoginTest {
  public:
   WebviewProxyAuthLoginTest()
@@ -1041,4 +1193,5 @@ IN_PROC_BROWSER_TEST_F(WebviewProxyAuthLoginTest, DISABLED_ProxyAuthTransfer) {
   // so the sign-in screen will not display user pods.
   ExpectIdentifierPage();
 }
+
 }  // namespace chromeos

chromeos/dbus/cryptohome/fake_cryptohome_client.cc

@@ -175,7 +175,7 @@ void FakeCryptohomeClient::GetRsuDeviceId(
 
 void FakeCryptohomeClient::TpmIsReady(DBusMethodCallback<bool> callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), true));
+      FROM_HERE, base::BindOnce(std::move(callback), tpm_is_ready_));
 }
 
 void FakeCryptohomeClient::TpmIsEnabled(DBusMethodCallback<bool> callback) {
@@ -900,6 +900,14 @@ void FakeCryptohomeClient::NotifyAsyncCallStatusWithData(
     observer.AsyncCallStatusWithData(async_id, return_status, data);
 }
 
+void FakeCryptohomeClient::NotifyTpmInitStatusUpdated(
+    bool ready,
+    bool owned,
+    bool was_owned_this_boot) {
+  for (auto& observer : observer_list_)
+    observer.TpmInitStatusUpdated(ready, owned, was_owned_this_boot);
+}
+
 void FakeCryptohomeClient::NotifyDircryptoMigrationProgress(
     cryptohome::DircryptoMigrationStatus status,
     uint64_t current,

chromeos/dbus/cryptohome/fake_cryptohome_client.h

@@ -255,6 +255,9 @@ class COMPONENT_EXPORT(CRYPTOHOME_CLIENT) FakeCryptohomeClient
   // unavailable. Expects service not to be available when called.
   void ReportServiceIsNotAvailable();
 
+  // Changes the behavior of TpmIsReady().
+  void set_tpm_is_ready(bool value) { tpm_is_ready_ = value; }
+
   // Changes the behavior of TpmIsEnabled().
   void set_tpm_is_enabled(bool value) { tpm_is_enabled_ = value; }
 
@@ -344,6 +347,11 @@ class COMPONENT_EXPORT(CRYPTOHOME_CLIENT) FakeCryptohomeClient
   void SetTpmAttestationDeviceKeyPayload(const std::string& key_name,
                                          const std::string& payload);
 
+  // Calls TpmInitStatusUpdated() on Observer instances.
+  void NotifyTpmInitStatusUpdated(bool ready,
+                                  bool owned,
+                                  bool was_owned_this_boot);
+
   // Calls DircryptoMigrationProgress() on Observer instances.
   void NotifyDircryptoMigrationProgress(
       cryptohome::DircryptoMigrationStatus status,
@@ -476,6 +484,7 @@ class COMPONENT_EXPORT(CRYPTOHOME_CLIENT) FakeCryptohomeClient
   bool supports_low_entropy_credentials_ = false;
   // Controls if CheckKeyEx actually checks the key.
   bool enable_auth_check_ = false;
+  bool tpm_is_ready_ = true;
   bool tpm_is_enabled_ = true;
 
   // Reply to GetRsuDeviceId().

chromeos/tpm/tpm_token_loader.cc

@@ -33,13 +33,13 @@ static TPMTokenLoader* g_tpm_token_loader = NULL;
 // static
 void TPMTokenLoader::Initialize() {
   CHECK(!g_tpm_token_loader);
-  g_tpm_token_loader = new TPMTokenLoader(false /*for_test*/);
+  g_tpm_token_loader = new TPMTokenLoader(/*initialized_for_test=*/false);
 }
 
 // static
 void TPMTokenLoader::InitializeForTest() {
   CHECK(!g_tpm_token_loader);
-  g_tpm_token_loader = new TPMTokenLoader(true /*for_test*/);
+  g_tpm_token_loader = new TPMTokenLoader(/*initialized_for_test=*/true);
 }
 
 // static
@@ -61,8 +61,8 @@ bool TPMTokenLoader::IsInitialized() {
   return g_tpm_token_loader;
 }
 
-TPMTokenLoader::TPMTokenLoader(bool for_test)
-    : initialized_for_test_(for_test),
+TPMTokenLoader::TPMTokenLoader(bool initialized_for_test)
+    : initialized_for_test_(initialized_for_test),
       tpm_token_state_(TPM_STATE_UNKNOWN),
       tpm_token_info_getter_(TPMTokenInfoGetter::CreateForSystemToken(
           CryptohomeClient::Get(),
@@ -112,8 +112,9 @@ bool TPMTokenLoader::IsTPMLoadingEnabled() const {
   // TPM loading is enabled on non-ChromeOS environments, e.g. when running
   // tests on Linux.
   // Treat TPM as disabled for guest users since they do not store certs.
-  return initialized_for_test_ || (base::SysInfo::IsRunningOnChromeOS() &&
-                                   !LoginState::Get()->IsGuestSessionUser());
+  return initialized_for_test_ || enable_tpm_loading_for_testing_ ||
+         (base::SysInfo::IsRunningOnChromeOS() &&
+          !LoginState::Get()->IsGuestSessionUser());
 }
 
 void TPMTokenLoader::MaybeStartTokenInitialization() {

chromeos/tpm/tpm_token_loader.h

@@ -50,7 +50,8 @@ class COMPONENT_EXPORT(CHROMEOS_TPM) TPMTokenLoader
   // The global instance will immediately start observing |LoginState|.
   static void Initialize();
 
-  // Sets the global. stubbed out, instance. To be used in tests.
+  // Sets the global, stubbed out with the already initialized token, instance.
+  // To be used in tests.
   static void InitializeForTest();
 
   // Destroys the global instance.
@@ -81,8 +82,13 @@ class COMPONENT_EXPORT(CHROMEOS_TPM) TPMTokenLoader
 
   std::string tpm_user_pin() const { return tpm_user_pin_; }
 
+  // Allows tests to enable the TPM token loading logic in this class.
+  void enable_tpm_loading_for_testing(bool enable) {
+    enable_tpm_loading_for_testing_ = enable;
+  }
+
  private:
-  explicit TPMTokenLoader(bool for_test);
+  explicit TPMTokenLoader(bool initialized_for_test);
   ~TPMTokenLoader() override;
 
   bool IsTPMLoadingEnabled() const;
@@ -104,6 +110,8 @@ class COMPONENT_EXPORT(CHROMEOS_TPM) TPMTokenLoader
   // LoginState::Observer
   void LoggedInStateChanged() override;
 
+  bool enable_tpm_loading_for_testing_ = false;
+
   bool initialized_for_test_;
 
   TPMReadyCallbackList tpm_ready_callback_list_;

crypto/nss_util.cc

@@ -580,6 +580,19 @@ class NSSInitSingleton {
     }
   }
 
+  void SetSystemKeySlotWithoutInitializingTPMForTesting(ScopedPK11Slot slot) {
+    DCHECK(thread_checker_.CalledOnValidThread());
+
+    // Ensure that a previous value of test_system_slot_ is not overwritten.
+    // Unsetting, i.e. setting a nullptr, however is allowed.
+    DCHECK(!slot || !test_system_slot_);
+    if (tpm_slot_ && tpm_slot_ == test_system_slot_) {
+      // Unset |tpm_slot_| if it was initialized from |test_system_slot_|.
+      tpm_slot_.reset();
+    }
+    test_system_slot_ = std::move(slot);
+  }
+
   void SetPrivateSoftwareSlotForChromeOSUserForTesting(ScopedPK11Slot slot) {
     DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -823,6 +836,11 @@ void SetSystemKeySlotForTesting(ScopedPK11Slot slot) {
   g_nss_singleton.Get().SetSystemKeySlotForTesting(std::move(slot));
 }
 
+void SetSystemKeySlotWithoutInitializingTPMForTesting(ScopedPK11Slot slot) {
+  g_nss_singleton.Get().SetSystemKeySlotWithoutInitializingTPMForTesting(
+      std::move(slot));
+}
+
 void EnableTPMTokenForNSS() {
   g_nss_singleton.Get().EnableTPMTokenForNSS();
 }

crypto/nss_util_internal.h

@@ -63,6 +63,14 @@ CRYPTO_EXPORT ScopedPK11Slot GetSystemNSSKeySlot(
 // |slot| is nullptr, the test system slot is unset.
 CRYPTO_EXPORT void SetSystemKeySlotForTesting(ScopedPK11Slot slot);
 
+// Injects the given |slot| as a system slot set by the future
+// |InitializeTPMTokenAndSystemSlot| call.
+// This must must not be called consecutively with a |slot| != nullptr. If
+// |slot| is nullptr and the system slot is already initialized to the
+// previously passed test value, the system slot is unset.
+CRYPTO_EXPORT void SetSystemKeySlotWithoutInitializingTPMForTesting(
+    ScopedPK11Slot slot);
+
 // Prepare per-user NSS slot mapping. It is safe to call this function multiple
 // times. Returns true if the user was added, or false if it already existed.
 CRYPTO_EXPORT bool InitializeNSSForChromeOSUser(