Delay creating easy signin TPM keys until TPM is initialized

EasyUnlockTpmKeyManager should not start poking TPM on worker thread
before TPM initialization is finished on IO thread in order to
prevent IO thread being blocked during startup waiting for easy
signin key pair being created.

BUG=464524
TEST=Easy signin works after initializing easy unlock and signin in
once.

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

Cr-Commit-Position: refs/heads/master@{#329475}

chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc

@@ -33,7 +33,7 @@ namespace {
 const int kKeyModulusLength = 2048;
 
 // Relays |GetSystemSlotOnIOThread| callback to |response_task_runner|.
-void RunCallbackOnThreadRunner(
+void RunCallbackOnTaskRunner(
     const scoped_refptr<base::SingleThreadTaskRunner>& response_task_runner,
     const base::Callback<void(crypto::ScopedPK11Slot)>& callback,
     crypto::ScopedPK11Slot slot) {
@@ -47,7 +47,7 @@ void GetSystemSlotOnIOThread(
     const scoped_refptr<base::SingleThreadTaskRunner>& response_task_runner,
     const base::Callback<void(crypto::ScopedPK11Slot)>& callback) {
   base::Callback<void(crypto::ScopedPK11Slot)> callback_on_origin_thread =
-      base::Bind(&RunCallbackOnThreadRunner, response_task_runner, callback);
+      base::Bind(&RunCallbackOnTaskRunner, response_task_runner, callback);
 
   crypto::ScopedPK11Slot system_slot =
       crypto::GetSystemNSSKeySlot(callback_on_origin_thread);
@@ -55,6 +55,29 @@ void GetSystemSlotOnIOThread(
     callback_on_origin_thread.Run(system_slot.Pass());
 }
 
+// Relays |EnsureUserTpmInitializedOnIOThread| callback to
+// |response_task_runner|, ignoring |slot|.
+void RunCallbackWithoutSlotOnTaskRunner(
+    const scoped_refptr<base::SingleThreadTaskRunner>& response_task_runner,
+    const base::Closure& callback,
+    crypto::ScopedPK11Slot slot) {
+  response_task_runner->PostTask(FROM_HERE, callback);
+}
+
+void EnsureUserTPMInitializedOnIOThread(
+    const std::string& username_hash,
+    const scoped_refptr<base::SingleThreadTaskRunner>& response_task_runner,
+    const base::Closure& callback) {
+  base::Callback<void(crypto::ScopedPK11Slot)> callback_on_origin_thread =
+      base::Bind(&RunCallbackWithoutSlotOnTaskRunner, response_task_runner,
+                 callback);
+
+  crypto::ScopedPK11Slot private_slot = crypto::GetPrivateSlotForChromeOSUser(
+      username_hash, callback_on_origin_thread);
+  if (private_slot)
+    callback_on_origin_thread.Run(private_slot.Pass());
+}
+
 // Checks if a private RSA key associated with |public_key| can be found in
 // |slot|.
 // Must be called on a worker thread.
@@ -174,9 +197,12 @@ void EasyUnlockTpmKeyManager::ResetLocalStateForUser(
   update->RemoveWithoutPathExpansion(user_id, NULL);
 }
 
-EasyUnlockTpmKeyManager::EasyUnlockTpmKeyManager(const std::string& user_id,
-                                                 PrefService* local_state)
+EasyUnlockTpmKeyManager::EasyUnlockTpmKeyManager(
+    const std::string& user_id,
+    const std::string& username_hash,
+    PrefService* local_state)
     : user_id_(user_id),
+      username_hash_(username_hash),
       local_state_(local_state),
       create_tpm_key_state_(CREATE_TPM_KEY_NOT_STARTED),
       get_tpm_slot_weak_ptr_factory_(this),
@@ -190,6 +216,7 @@ bool EasyUnlockTpmKeyManager::PrepareTpmKey(
     bool check_private_key,
     const base::Closure& callback) {
   CHECK(!user_id_.empty());
+  CHECK(!username_hash_.empty());
 
   if (create_tpm_key_state_ == CREATE_TPM_KEY_DONE)
     return true;
@@ -203,29 +230,24 @@ bool EasyUnlockTpmKeyManager::PrepareTpmKey(
   prepare_tpm_key_callbacks_.push_back(callback);
 
   if (create_tpm_key_state_ == CREATE_TPM_KEY_NOT_STARTED) {
-    create_tpm_key_state_ = CREATE_TPM_KEY_WAITING_FOR_SYSTEM_SLOT;
+    create_tpm_key_state_ = CREATE_TPM_KEY_WAITING_FOR_USER_SLOT;
 
-    base::Callback<void(crypto::ScopedPK11Slot)> create_key_with_system_slot =
-        base::Bind(&EasyUnlockTpmKeyManager::CreateKeyInSystemSlot,
-                   get_tpm_slot_weak_ptr_factory_.GetWeakPtr(),
-                   key);
+    base::Closure on_user_tpm_ready =
+        base::Bind(&EasyUnlockTpmKeyManager::OnUserTPMInitialized,
+                   get_tpm_slot_weak_ptr_factory_.GetWeakPtr(), key);
 
     content::BrowserThread::PostTask(
-        content::BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&GetSystemSlotOnIOThread,
-                   base::ThreadTaskRunnerHandle::Get(),
-                   create_key_with_system_slot));
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&EnsureUserTPMInitializedOnIOThread, username_hash_,
+                   base::ThreadTaskRunnerHandle::Get(), on_user_tpm_ready));
   }
 
   return false;
 }
 
 bool EasyUnlockTpmKeyManager::StartGetSystemSlotTimeoutMs(size_t timeout_ms) {
-  if (create_tpm_key_state_ == CREATE_TPM_KEY_DONE ||
-      create_tpm_key_state_ == CREATE_TPM_KEY_GOT_SYSTEM_SLOT) {
+  if (StartedCreatingTpmKeys())
     return false;
-  }
 
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
@@ -273,6 +295,11 @@ void EasyUnlockTpmKeyManager::SignUsingTpmKey(
                  sign_with_system_slot));
 }
 
+bool EasyUnlockTpmKeyManager::StartedCreatingTpmKeys() const {
+  return create_tpm_key_state_ == CREATE_TPM_KEY_GOT_SYSTEM_SLOT ||
+         create_tpm_key_state_ == CREATE_TPM_KEY_DONE;
+}
+
 void EasyUnlockTpmKeyManager::SetKeyInLocalState(const std::string& user_id,
                                                  const std::string& value) {
   if (!local_state_)
@@ -285,11 +312,24 @@ void EasyUnlockTpmKeyManager::SetKeyInLocalState(const std::string& user_id,
   update->SetStringWithoutPathExpansion(user_id, encoded);
 }
 
+void EasyUnlockTpmKeyManager::OnUserTPMInitialized(
+    const std::string& public_key) {
+  create_tpm_key_state_ = CREATE_TPM_KEY_WAITING_FOR_SYSTEM_SLOT;
+
+  base::Callback<void(crypto::ScopedPK11Slot)> create_key_with_system_slot =
+      base::Bind(&EasyUnlockTpmKeyManager::CreateKeyInSystemSlot,
+                 get_tpm_slot_weak_ptr_factory_.GetWeakPtr(), public_key);
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&GetSystemSlotOnIOThread, base::ThreadTaskRunnerHandle::Get(),
+                 create_key_with_system_slot));
+}
+
 void EasyUnlockTpmKeyManager::CreateKeyInSystemSlot(
     const std::string& public_key,
     crypto::ScopedPK11Slot system_slot) {
   CHECK(system_slot);
-
   create_tpm_key_state_ = CREATE_TPM_KEY_GOT_SYSTEM_SLOT;
 
   // If there are any delayed tasks posted using |StartGetSystemSlotTimeoutMs|,

chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h

@@ -27,10 +27,14 @@ class EasyUnlockTpmKeyManager : public KeyedService {
   // Clears local state for user. Should be called when a user is removed.
   static void ResetLocalStateForUser(const std::string& user_id);
 
-  // |user_id|: Id for the user associated with the service. Empty for signin
+  // |user_id|: Id for the user associated with the service. Empty for sign-in
   //     service.
+  // |username_hash|: Username hash for the user associated with the service.
+  //     Empty for sign-in service.
   // |local_state|: The local state prefs.
-  EasyUnlockTpmKeyManager(const std::string& user_id, PrefService* local_state);
+  EasyUnlockTpmKeyManager(const std::string& user_id,
+                          const std::string& username_hash,
+                          PrefService* local_state);
   ~EasyUnlockTpmKeyManager() override;
 
   // Checks if the RSA public key is set in the local state. If not, creates
@@ -69,9 +73,12 @@ class EasyUnlockTpmKeyManager : public KeyedService {
       const std::string& data,
       const base::Callback<void(const std::string& data)> callback);
 
+  bool StartedCreatingTpmKeys() const;
+
  private:
   enum CreateTpmKeyState {
     CREATE_TPM_KEY_NOT_STARTED,
+    CREATE_TPM_KEY_WAITING_FOR_USER_SLOT,
     CREATE_TPM_KEY_WAITING_FOR_SYSTEM_SLOT,
     CREATE_TPM_KEY_GOT_SYSTEM_SLOT,
     CREATE_TPM_KEY_DONE
@@ -92,6 +99,12 @@ class EasyUnlockTpmKeyManager : public KeyedService {
   void CreateKeyInSystemSlot(const std::string& public_key,
                              crypto::ScopedPK11Slot system_slot);
 
+  // Called when user TPM token initialization is done. After this happens,
+  // |this| may proceed with creating a user-specific TPM key for easy sign-in.
+  // Note that this is done solely to ensure user TPM initialization, which is
+  // done on IO thread, is not blocked by creating TPM keys in system slot.
+  void OnUserTPMInitialized(const std::string& public_key);
+
   // Called when TPM system slot is initialized and ready to be used.
   // It schedules data signing operation on a worker thread. The data is signed
   // by a private key stored in |system_slot| and identified by |public_key|
@@ -116,6 +129,7 @@ class EasyUnlockTpmKeyManager : public KeyedService {
       const std::string& signature);
 
   std::string user_id_;
+  std::string username_hash_;
 
   PrefService* local_state_;
 

chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_factory.cc

@@ -61,8 +61,9 @@ KeyedService* EasyUnlockTpmKeyManagerFactory::BuildServiceInstanceFor(
   const user_manager::User* user = NULL;
   if (!chromeos::ProfileHelper::IsSigninProfile(profile))
     user = chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
-  return new EasyUnlockTpmKeyManager(user ? user->email() : std::string(),
-                                     GetLocalState());
+  return new EasyUnlockTpmKeyManager(
+      user ? user->email() : std::string(),
+      user ? user->username_hash() : std::string(), GetLocalState());
 }
 
 content::BrowserContext* EasyUnlockTpmKeyManagerFactory::GetBrowserContextToUse(

chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc

@@ -23,7 +23,9 @@
 #include "chrome/test/base/testing_pref_service_syncable.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "crypto/scoped_test_nss_chromeos_user.h"
 #include "crypto/scoped_test_system_nss_key_slot.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -200,7 +202,8 @@ class EasyUnlockTpmKeyManagerTest : public testing::Test {
 
   void SetUp() override {
     ASSERT_TRUE(profile_manager_.SetUp());
-    user_manager_->AddUser(kTestUserId);
+    const user_manager::User* user = user_manager_->AddUser(kTestUserId);
+    username_hash_ = user->username_hash();
 
     signin_profile_ = profile_manager_.CreateTestingProfile(
         chrome::kInitialProfile,
@@ -220,10 +223,63 @@ class EasyUnlockTpmKeyManagerTest : public testing::Test {
   }
 
   void TearDown() override {
+    if (test_nss_user_)
+      ResetTestNssUser();
     profile_manager_.DeleteTestingProfile(kTestUserId);
     profile_manager_.DeleteTestingProfile(chrome::kInitialProfile);
   }
 
+  bool InitTestNssUser() {
+    bool success = false;
+    base::RunLoop run_loop;
+    // Has to be done on IO thread due to thread assertions in nss code.
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&EasyUnlockTpmKeyManagerTest::InitTestNssUserOnIOThread,
+                   base::Unretained(this), base::Unretained(&success)),
+        run_loop.QuitClosure());
+    run_loop.Run();
+    return success;
+  }
+
+  void InitTestNssUserOnIOThread(bool* success) {
+    test_nss_user_.reset(new crypto::ScopedTestNSSChromeOSUser(username_hash_));
+    *success = test_nss_user_->constructed_successfully();
+  }
+
+  // Verifies that easy sign-in TPM key generation does not start before user
+  // TPM is completely done, then finalizes user TPM initialization.
+  // Note that easy sign-in key generation should not start before TPM is
+  // initialized in order to prevent TPM initialization from blocking IO thread
+  // while waiting for TPM lock (taken for key creation) to be released.
+  void VerifyKeyGenerationNotStartedAndFinalizeTestNssUser() {
+    EXPECT_FALSE(user_key_manager()->StartedCreatingTpmKeys());
+
+    base::RunLoop run_loop;
+    // Has to be done on IO thread due to thread assertions in nss code.
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&EasyUnlockTpmKeyManagerTest::FinalizeTestNssUserOnIOThread,
+                   base::Unretained(this)),
+        run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  void FinalizeTestNssUserOnIOThread() { test_nss_user_->FinishInit(); }
+
+  void ResetTestNssUser() {
+    base::RunLoop run_loop;
+    // Has to be done on IO thread due to thread assertions in nss code.
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&EasyUnlockTpmKeyManagerTest::ResetTestNssUserOnIOThread,
+                   base::Unretained(this)),
+        run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  void ResetTestNssUserOnIOThread() { test_nss_user_.reset(); }
+
   // Creates and sets test system NSS key slot.
   bool SetUpTestSystemSlot() {
     test_system_slot_.reset(new crypto::ScopedTestSystemNSSKeySlot());
@@ -283,6 +339,7 @@ class EasyUnlockTpmKeyManagerTest : public testing::Test {
 
   // The NSS system slot used by EasyUnlockTPMKeyManagers in tests.
   scoped_ptr<crypto::ScopedTestSystemNSSKeySlot> test_system_slot_;
+  scoped_ptr<crypto::ScopedTestNSSChromeOSUser> test_nss_user_;
 
   // Needed to properly set up signin and user profiles for test.
   user_manager::FakeUserManager* user_manager_;
@@ -294,10 +351,15 @@ class EasyUnlockTpmKeyManagerTest : public testing::Test {
   TestingProfile* user_profile_;
   TestingProfile* signin_profile_;
 
+  // The test user's username hash.
+  std::string username_hash_;
+
   DISALLOW_COPY_AND_ASSIGN(EasyUnlockTpmKeyManagerTest);
 };
 
 TEST_F(EasyUnlockTpmKeyManagerTest, CreateKeyPair) {
+  ASSERT_TRUE(InitTestNssUser());
+
   base::RunLoop run_loop;
   EXPECT_TRUE(user_key_manager()->GetPublicTpmKey(kTestUserId).empty());
   EXPECT_TRUE(signin_key_manager()->GetPublicTpmKey(kTestUserId).empty());
@@ -307,6 +369,7 @@ TEST_F(EasyUnlockTpmKeyManagerTest, CreateKeyPair) {
   EXPECT_TRUE(user_key_manager()->GetPublicTpmKey(kTestUserId).empty());
 
   ASSERT_TRUE(SetUpTestSystemSlot());
+  VerifyKeyGenerationNotStartedAndFinalizeTestNssUser();
   run_loop.Run();
 
   EXPECT_FALSE(user_key_manager()->GetPublicTpmKey(kTestUserId).empty());
@@ -319,6 +382,8 @@ TEST_F(EasyUnlockTpmKeyManagerTest, CreateKeyPair) {
 }
 
 TEST_F(EasyUnlockTpmKeyManagerTest, CreateKeyPairMultipleCallbacks) {
+  ASSERT_TRUE(InitTestNssUser());
+
   int callback_count = 0;
   base::RunLoop run_loop;
 
@@ -336,6 +401,7 @@ TEST_F(EasyUnlockTpmKeyManagerTest, CreateKeyPairMultipleCallbacks) {
       false /* check_private_key */, base::Closure()));
 
   ASSERT_TRUE(SetUpTestSystemSlot());
+  VerifyKeyGenerationNotStartedAndFinalizeTestNssUser();
   EXPECT_EQ(0, callback_count);
 
   run_loop.Run();
@@ -350,7 +416,7 @@ TEST_F(EasyUnlockTpmKeyManagerTest, CreateKeyPairMultipleCallbacks) {
       base::Bind(&ExpectNotCalledCallback)));
 }
 
-TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInUserPrefs) {
+TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInPrefs) {
   SetLocalStatePublicKey(
       kTestUserId, std::string(kTestPublicKey, arraysize(kTestPublicKey)));
 
@@ -365,7 +431,9 @@ TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInUserPrefs) {
             signin_key_manager()->GetPublicTpmKey(kTestUserId));
 }
 
-TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInUserPrefsCheckPrivateKey) {
+TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInPrefsCheckPrivateKey) {
+  ASSERT_TRUE(InitTestNssUser());
+
   SetLocalStatePublicKey(
       kTestUserId, std::string(kTestPublicKey, arraysize(kTestPublicKey)));
 
@@ -375,6 +443,7 @@ TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInUserPrefsCheckPrivateKey) {
       run_loop.QuitClosure()));
 
   ASSERT_TRUE(SetUpTestSystemSlot());
+  VerifyKeyGenerationNotStartedAndFinalizeTestNssUser();
   run_loop.Run();
 
   EXPECT_FALSE(user_key_manager()->GetPublicTpmKey(kTestUserId).empty());
@@ -384,8 +453,10 @@ TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInUserPrefsCheckPrivateKey) {
             signin_key_manager()->GetPublicTpmKey(kTestUserId));
 }
 
-TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInUserPrefsCheckPrivateKey_OK) {
+TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInPrefsCheckPrivateKey_OK) {
+  ASSERT_TRUE(InitTestNssUser());
   ASSERT_TRUE(SetUpTestSystemSlot());
+  VerifyKeyGenerationNotStartedAndFinalizeTestNssUser();
   ASSERT_TRUE(ImportPrivateKey(kTestPrivateKey,  arraysize(kTestPrivateKey)));
   SetLocalStatePublicKey(
       kTestUserId, std::string(kTestPublicKey, arraysize(kTestPublicKey)));
@@ -415,6 +486,8 @@ TEST_F(EasyUnlockTpmKeyManagerTest, PublicKeySetInUserPrefsCheckPrivateKey_OK) {
 }
 
 TEST_F(EasyUnlockTpmKeyManagerTest, GetSystemSlotTimeoutTriggers) {
+  ASSERT_TRUE(InitTestNssUser());
+
   base::RunLoop run_loop;
   ASSERT_FALSE(user_key_manager()->PrepareTpmKey(
       false /* check_private_key */,
@@ -425,6 +498,7 @@ TEST_F(EasyUnlockTpmKeyManagerTest, GetSystemSlotTimeoutTriggers) {
   run_loop_get_slot_timeout.RunUntilIdle();
 
   ASSERT_TRUE(SetUpTestSystemSlot());
+  VerifyKeyGenerationNotStartedAndFinalizeTestNssUser();
 
   run_loop.Run();
 
@@ -432,12 +506,14 @@ TEST_F(EasyUnlockTpmKeyManagerTest, GetSystemSlotTimeoutTriggers) {
 }
 
 TEST_F(EasyUnlockTpmKeyManagerTest, GetSystemSlotTimeoutAfterSlotFetched) {
+  ASSERT_TRUE(InitTestNssUser());
   base::RunLoop run_loop;
   ASSERT_FALSE(user_key_manager()->PrepareTpmKey(
       false /* check_private_key */,
       run_loop.QuitClosure()));
 
   base::RunLoop run_loop_slot;
+  VerifyKeyGenerationNotStartedAndFinalizeTestNssUser();
   ASSERT_TRUE(SetUpTestSystemSlot());
   run_loop_slot.RunUntilIdle();
 
@@ -449,6 +525,7 @@ TEST_F(EasyUnlockTpmKeyManagerTest, GetSystemSlotTimeoutAfterSlotFetched) {
 }
 
 TEST_F(EasyUnlockTpmKeyManagerTest, GetSystemSlotRetryAfterFailure) {
+  ASSERT_TRUE(InitTestNssUser());
   base::RunLoop run_loop;
   ASSERT_FALSE(user_key_manager()->PrepareTpmKey(
       false /* check_private_key */,
@@ -469,6 +546,7 @@ TEST_F(EasyUnlockTpmKeyManagerTest, GetSystemSlotRetryAfterFailure) {
       run_loop_retry.QuitClosure()));
 
   ASSERT_TRUE(SetUpTestSystemSlot());
+  VerifyKeyGenerationNotStartedAndFinalizeTestNssUser();
 
   run_loop_retry.Run();