cros: Call GetPrivateSlotForChromeOSUser properly

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc

 // Copyright 2014 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/chromeos/ownership/owner_settings_service_chromeos.h"
 
 #include <keyhi.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 35 matching lines @@
 
 using content::BrowserThread;
 using ownership::OwnerKeyUtil;
 using ownership::PrivateKey;
 using ownership::PublicKey;
 
 namespace chromeos {
 
 namespace {
 
+using ReloadKeyCallback =
+    base::Callback<void(const scoped_refptr<PublicKey>& public_key,
+                        const scoped_refptr<PrivateKey>& private_key)>;
+
 bool IsOwnerInTests(const std::string& user_id) {
   if (user_id.empty() ||
       !base::CommandLine::ForCurrentProcess()->HasSwitch(
           ::switches::kTestType) ||
       !CrosSettings::IsInitialized()) {
     return false;
   }
   const base::Value* value = CrosSettings::Get()->GetPref(kDeviceOwner);
   if (!value || value->GetType() != base::Value::Type::STRING)
     return false;
   return static_cast<const base::Value*>(value)->GetString() == user_id;
 }
 
 void LoadPrivateKeyByPublicKeyOnWorkerThread(
     const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     crypto::ScopedPK11Slot public_slot,
     crypto::ScopedPK11Slot private_slot,
-    const base::Callback<void(const scoped_refptr<PublicKey>& public_key,
-                              const scoped_refptr<PrivateKey>& private_key)>&
-        callback) {
+    const ReloadKeyCallback& callback) {
   DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
 
   std::vector<uint8_t> public_key_data;
   scoped_refptr<PublicKey> public_key;
   if (!owner_key_util->ImportPublicKey(&public_key_data)) {
     scoped_refptr<PrivateKey> private_key;
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             base::Bind(callback, public_key, private_key));
     return;
   }
@@ skipping 13 matching lines @@
                                                           private_slot.get())));
   if (!private_key->key()) {
     private_key = new PrivateKey(owner_key_util->FindPrivateKeyInSlot(
         public_key->data(), public_slot.get()));
   }
   BrowserThread::PostTask(BrowserThread::UI,
                           FROM_HERE,
                           base::Bind(callback, public_key, private_key));
 }
 
-void LoadPrivateKeyOnIOThread(
+void ContinueLoadPrivateKeyOnIOThread(
     const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     const std::string username_hash,
-    const base::Callback<void(const scoped_refptr<PublicKey>& public_key,
-                              const scoped_refptr<PrivateKey>& private_key)>&
-        callback) {
+    const ReloadKeyCallback& callback,
+    crypto::ScopedPK11Slot private_slot) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  crypto::EnsureNSSInit();
-  crypto::ScopedPK11Slot public_slot =
-      crypto::GetPublicSlotForChromeOSUser(username_hash);
-  crypto::ScopedPK11Slot private_slot = crypto::GetPrivateSlotForChromeOSUser(
-      username_hash, base::Callback<void(crypto::ScopedPK11Slot)>());
-
   scoped_refptr<base::TaskRunner> task_runner =
       BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
           base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   task_runner->PostTask(
       FROM_HERE,
-      base::Bind(&LoadPrivateKeyByPublicKeyOnWorkerThread, owner_key_util,
-                 base::Passed(std::move(public_slot)),
-                 base::Passed(std::move(private_slot)), callback));
+      base::Bind(
+          &LoadPrivateKeyByPublicKeyOnWorkerThread, owner_key_util,
+          base::Passed(crypto::GetPublicSlotForChromeOSUser(username_hash)),
+          base::Passed(std::move(private_slot)), callback));
+}
+
+void LoadPrivateKeyOnIOThread(const scoped_refptr<OwnerKeyUtil>& owner_key_util,

[mattm, 2017/04/06 18:00:52]

Is it possible for this function to get called multiple times? It seems like
there might be the possibility of multiple callbacks waiting on the private slot
at the same time. I don't know how likely that is or if it would cause problems.

[xiyuan, 2017/04/06 18:14:25]

Yes, it is possible. I have seen two overlapping calls during ownership
establishment. One triggered from OnTPMTokenReady and one from OwnerKeySet. This
happens on the existing code. And I am not aware problems with that.

If you want to change that, do you think we should protect against this? Should
we ignore subsequent loads where there is a load going on? Or should we cancel
previous loads when new load request comes in?

[mattm, 2017/04/06 18:23:27]

If it's already happening and the code is fine with it, I don't think there's
any need to change it for this CL.

+                              const std::string username_hash,
+                              const ReloadKeyCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  crypto::EnsureNSSInit();
+
+  auto continue_load_private_key_callback =
+      base::Bind(&ContinueLoadPrivateKeyOnIOThread, owner_key_util,
+                 username_hash, callback);
+
+  crypto::ScopedPK11Slot private_slot = crypto::GetPrivateSlotForChromeOSUser(
+      username_hash, continue_load_private_key_callback);
+  if (private_slot)
+    continue_load_private_key_callback.Run(std::move(private_slot));
 }
 
 bool DoesPrivateKeyExistAsyncHelper(
     const scoped_refptr<OwnerKeyUtil>& owner_key_util) {
   std::vector<uint8_t> public_key;
   if (!owner_key_util->ImportPublicKey(&public_key))
     return false;
   crypto::ScopedSECKEYPrivateKey key =
       crypto::FindNSSKeyFromPublicKeyInfo(public_key);
   return key && SECKEY_GetPrivateKeyType(key.get()) == rsaKey;
@@ skipping 591 matching lines @@
 
 void OwnerSettingsServiceChromeOS::ReportStatusAndContinueStoring(
     bool success) {
   store_settings_factory_.InvalidateWeakPtrs();
   for (auto& observer : observers_)
     observer.OnSignedPolicyStored(success);
   StorePendingChanges();
 }
 
 }  // namespace chromeos