Remove usage of singleton software_slot_ in nss on ChromeOS

crypto/nss_util.cc

Index: crypto/nss_util.cc
diff --git a/crypto/nss_util.cc b/crypto/nss_util.cc
index 5958ad972daf74db958b9c50b88badfef4169b5b..7c6ad912576dc05a866debb19f9963d7d9994eb3 100644
--- a/crypto/nss_util.cc
+++ b/crypto/nss_util.cc
@@ -81,6 +81,7 @@ std::string GetNSSErrorMessage() {
 }
 
 #if defined(USE_NSS)
+#if !defined(OS_CHROMEOS)
 base::FilePath GetDefaultConfigDirectory() {
   base::FilePath dir;
   PathService::Get(base::DIR_HOME, &dir);
@@ -96,6 +97,7 @@ base::FilePath GetDefaultConfigDirectory() {
   DVLOG(2) << "DefaultConfigDirectory: " << dir.value();
   return dir;
 }
+#endif  // !defined(IS_CHROMEOS)
 
 // On non-Chrome OS platforms, return the default config directory. On Chrome OS
 // test images, return a read-only directory with fake root CA certs (which are
@@ -216,11 +218,11 @@ void CrashOnNSSInitFailure() {
 #if defined(OS_CHROMEOS)
 class ChromeOSUserData {
  public:
-  ChromeOSUserData(ScopedPK11Slot public_slot, bool is_primary_user)
+  ChromeOSUserData(ScopedPK11Slot public_slot, bool provisional)
       : public_slot_(public_slot.Pass()),
-        is_primary_user_(is_primary_user) {}
+        provisional_(provisional) {}
   ~ChromeOSUserData() {
-    if (public_slot_ && !is_primary_user_) {
+    if (public_slot_) {
       SECStatus status = SECMOD_CloseUserDB(public_slot_.get());
       if (status != SECSuccess)
         PLOG(ERROR) << "SECMOD_CloseUserDB failed: " << PORT_GetError();
@@ -229,12 +231,14 @@ class ChromeOSUserData {
 
   ScopedPK11Slot GetPublicSlot() {
     return ScopedPK11Slot(
-        public_slot_ ? PK11_ReferenceSlot(public_slot_.get()) : NULL);
+        !provisional_ && public_slot_ ?
+            PK11_ReferenceSlot(public_slot_.get()) :
+            NULL);
   }
 
   ScopedPK11Slot GetPrivateSlot(
       const base::Callback<void(ScopedPK11Slot)>& callback) {
-    if (private_slot_)
+    if (private_slot_ && !provisional_)
       return ScopedPK11Slot(PK11_ReferenceSlot(private_slot_.get()));
     if (!callback.is_null())
       tpm_ready_callback_list_.push_back(callback);
@@ -243,6 +247,7 @@ class ChromeOSUserData {
 
   void SetPrivateSlot(ScopedPK11Slot private_slot) {
     DCHECK(!private_slot_);
+    DCHECK(!provisional_);
     private_slot_ = private_slot.Pass();
 
     SlotReadyCallbackList callback_list;
@@ -254,10 +259,15 @@ class ChromeOSUserData {
     }
   }
 
+  bool provisional() { return provisional_; }
+
+  void set_provisional(bool value) { provisional_ = value; }
+
  private:
   ScopedPK11Slot public_slot_;
   ScopedPK11Slot private_slot_;
-  bool is_primary_user_;
+
+  bool provisional_;
 
   typedef std::vector<base::Callback<void(ScopedPK11Slot)> >
       SlotReadyCallbackList;
@@ -276,24 +286,6 @@ class NSSInitSingleton {
     PK11SlotInfo* tpm_slot;
   };
 
-  void OpenPersistentNSSDB() {
-    DCHECK(thread_checker_.CalledOnValidThread());
-
-    if (!chromeos_user_logged_in_) {
-      // GetDefaultConfigDirectory causes us to do blocking IO on UI thread.
-      // Temporarily allow it until we fix http://crbug.com/70119
-      base::ThreadRestrictions::ScopedAllowIO allow_io;
-      chromeos_user_logged_in_ = true;
-
-      // This creates another DB slot in NSS that is read/write, unlike
-      // the fake root CA cert DB and the "default" crypto key
-      // provider, which are still read-only (because we initialized
-      // NSS before we had a cryptohome mounted).
-      software_slot_ = OpenUserDB(GetDefaultConfigDirectory(),
-                                  kNSSDatabaseName);
-    }
-  }
-
   PK11SlotInfo* OpenPersistentNSSDBForPath(const base::FilePath& path) {
     DCHECK(thread_checker_.CalledOnValidThread());
     // NSS is allowed to do IO on the current thread since dispatching
@@ -459,24 +451,29 @@ class NSSInitSingleton {
   bool InitializeNSSForChromeOSUser(
       const std::string& email,
       const std::string& username_hash,
-      bool is_primary_user,
-      const base::FilePath& path) {
+      const base::FilePath& path,
+      bool provisional) {
     DCHECK(thread_checker_.CalledOnValidThread());
     if (chromeos_user_map_.find(username_hash) != chromeos_user_map_.end()) {
       // This user already exists in our mapping.
       DVLOG(2) << username_hash << " already initialized.";
-      return false;
-    }
-    ScopedPK11Slot public_slot;
-    if (is_primary_user) {
-      DVLOG(2) << "Primary user, using GetPublicNSSKeySlot()";
-      public_slot.reset(GetPublicNSSKeySlot());
-    } else {
-      DVLOG(2) << "Opening NSS DB " << path.value();
-      public_slot.reset(OpenPersistentNSSDBForPath(path));
+      if (!chromeos_user_map_[username_hash]->provisional())
+        return false;
+
+      chromeos_user_map_[username_hash]->set_provisional(provisional);
+      return true;
     }

[Ryan Sleevi, 2014/07/01 18:51:33]

This logic strikes me as a little weird, as the comment doesn't fully explain
the possible conditions.

If I set provisional to false, and load, it will return true. If I then attempt
to load again, this time with provisional=true, it will return false (even
though it's loaded and usable)

If I set provisional to true, and load, it will return true. If I then attempt
to load again, this time with provisional=false, it will true, but not actually
do any work (it just does set_provisional)

Regardless of the provisional flag, these methods all have side-effects on the
overall NSS state.

[tbarzic, 2014/07/01 19:25:39]

Yeah, I'm not to content with semantics here.. I need to come up with better
naming, and improve the comment (I've planned to do that today).
Stay tuned.

(Basically, the purpose for this is to let profile_io_data.cc know whether it
should continue with initializing private slot for the user or not)

+
+    // If test slot is set, slot getter methods will short circuit
+    // checking |chromeos_user_map_|, so there is nothing left to be
+    // initialized.
+    if (test_slot_)
+      return false;
+
+    DVLOG(2) << "Opening NSS DB " << path.value();
+    ScopedPK11Slot public_slot(OpenPersistentNSSDBForPath(path));
     chromeos_user_map_[username_hash] =
-        new ChromeOSUserData(public_slot.Pass(), is_primary_user);
+        new ChromeOSUserData(public_slot.Pass(), provisional);
     return true;
   }
 
@@ -484,6 +481,7 @@ class NSSInitSingleton {
                                     CK_SLOT_ID slot_id) {
     DCHECK(thread_checker_.CalledOnValidThread());
     DCHECK(chromeos_user_map_.find(username_hash) != chromeos_user_map_.end());
+    DCHECK(!chromeos_user_map_[username_hash]->provisional());
 
     if (!chaps_module_)
       return;
@@ -519,6 +517,7 @@ class NSSInitSingleton {
     DCHECK(thread_checker_.CalledOnValidThread());
     VLOG(1) << "using software private slot for " << username_hash;
     DCHECK(chromeos_user_map_.find(username_hash) != chromeos_user_map_.end());
+    DCHECK(!chromeos_user_map_[username_hash]->provisional());
     chromeos_user_map_[username_hash]->SetPrivateSlot(
         chromeos_user_map_[username_hash]->GetPublicSlot());
   }
@@ -619,8 +618,6 @@ class NSSInitSingleton {
 
     if (test_slot_)
       return PK11_ReferenceSlot(test_slot_);
-    if (software_slot_)
-      return PK11_ReferenceSlot(software_slot_);
     return PK11_GetInternalKeySlot();
   }
 
@@ -645,10 +642,6 @@ class NSSInitSingleton {
       }
     }
 #endif
-    // If we weren't supposed to enable the TPM for NSS, then return
-    // the software slot.
-    if (software_slot_)
-      return PK11_ReferenceSlot(software_slot_);
     return PK11_GetInternalKeySlot();
   }
 
@@ -671,11 +664,9 @@ class NSSInitSingleton {
       : tpm_token_enabled_for_nss_(false),
         initializing_tpm_token_(false),
         chaps_module_(NULL),
-        software_slot_(NULL),
         test_slot_(NULL),
         tpm_slot_(NULL),
-        root_(NULL),
-        chromeos_user_logged_in_(false) {
+        root_(NULL) {
     base::TimeTicks start_time = base::TimeTicks::Now();
 
     // It's safe to construct on any thread, since LazyInstance will prevent any
@@ -795,11 +786,6 @@ class NSSInitSingleton {
       PK11_FreeSlot(tpm_slot_);
       tpm_slot_ = NULL;
     }
-    if (software_slot_) {
-      SECMOD_CloseUserDB(software_slot_);
-      PK11_FreeSlot(software_slot_);
-      software_slot_ = NULL;
-    }
     CloseTestNSSDB();
     if (root_) {
       SECMOD_UnloadUserModule(root_);
@@ -902,11 +888,9 @@ class NSSInitSingleton {
   typedef std::vector<base::Closure> TPMReadyCallbackList;
   TPMReadyCallbackList tpm_ready_callback_list_;
   SECMODModule* chaps_module_;
-  PK11SlotInfo* software_slot_;
   PK11SlotInfo* test_slot_;
   PK11SlotInfo* tpm_slot_;
   SECMODModule* root_;
-  bool chromeos_user_logged_in_;
 #if defined(OS_CHROMEOS)
   typedef std::map<std::string, ChromeOSUserData*> ChromeOSUserMap;
   ChromeOSUserMap chromeos_user_map_;
@@ -1070,10 +1054,6 @@ AutoSECMODListReadLock::~AutoSECMODListReadLock() {
 #endif  // defined(USE_NSS)
 
 #if defined(OS_CHROMEOS)
-void OpenPersistentNSSDB() {
-  g_nss_singleton.Get().OpenPersistentNSSDB();
-}
-
 void EnableTPMTokenForNSS() {
   g_nss_singleton.Get().EnableTPMTokenForNSS();
 }
@@ -1099,8 +1079,8 @@ ScopedTestNSSChromeOSUser::ScopedTestNSSChromeOSUser(
   constructed_successfully_ =
       InitializeNSSForChromeOSUser(username_hash,
                                    username_hash,
-                                   false /* is_primary_user */,
-                                   temp_dir_.path());
+                                   temp_dir_.path(),
+                                   false /* not provisional */);
 }
 
 ScopedTestNSSChromeOSUser::~ScopedTestNSSChromeOSUser() {
@@ -1115,10 +1095,10 @@ void ScopedTestNSSChromeOSUser::FinishInit() {
 bool InitializeNSSForChromeOSUser(
     const std::string& email,
     const std::string& username_hash,
-    bool is_primary_user,
-    const base::FilePath& path) {
+    const base::FilePath& path,
+    bool provisional) {
   return g_nss_singleton.Get().InitializeNSSForChromeOSUser(
-      email, username_hash, is_primary_user, path);
+      email, username_hash, path, provisional);
 }
 void InitializeTPMForChromeOSUser(
     const std::string& username_hash,