Remove default key slot from KeygenHandler.

KeygenHandler defaulted to use the persistent key slot. However, the Handler is also used in ChromeOS where no such global persistent slot exists.

This change removes this default behavior from KeygenHandler.
Instead only the slot provided by the CryptoModuleDelegate will be used.
If no CryptoModuleDelegate is set, then an error will be thrown on keygen usage.

The unit test of KeygenHandler now explicitly provides the slot through a StubCryptoModuleDelegate.
In a follow up, the crypto::GetPersistentNSSKeySlot() call will be removed from the unit tests as well and instead the test slot will be get()'ed from the ScopedTestNSSDB.

BUG=210525

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283634 0039d316-1c4b-4281-b951-d872f2087c98

net/base/keygen_handler_nss.cc

@@ -7,7 +7,6 @@
 #include "base/logging.h"
 #include "crypto/nss_crypto_module_delegate.h"
 #include "crypto/nss_util.h"
-#include "crypto/nss_util_internal.h"
 #include "crypto/scoped_nss_types.h"
 #include "net/third_party/mozilla_security_manager/nsKeygenHandler.h"
 
@@ -17,26 +16,21 @@ namespace psm = mozilla_security_manager;
 namespace net {
 
 std::string KeygenHandler::GenKeyAndSignChallenge() {
-  // Ensure NSS is initialized.
   crypto::EnsureNSSInit();
 
   crypto::ScopedPK11Slot slot;
-  if (crypto_module_delegate_)
+  if (crypto_module_delegate_) {
     slot = crypto_module_delegate_->RequestSlot().Pass();
-  else
-    slot.reset(crypto::GetPersistentNSSKeySlot());
-  if (!slot.get()) {
-    LOG(ERROR) << "Couldn't get private key slot from NSS!";
+  } else {
+    LOG(ERROR) << "Could not get an NSS key slot.";
     return std::string();
   }
 
   // Authenticate to the token.
-  if (SECSuccess !=
-      PK11_Authenticate(
-          slot.get(),
-          PR_TRUE,
-          crypto_module_delegate_ ? crypto_module_delegate_->wincx() : NULL)) {
-    LOG(ERROR) << "Couldn't authenticate to private key slot!";
+  if (SECSuccess != PK11_Authenticate(slot.get(),
+                                      PR_TRUE,
+                                      crypto_module_delegate_->wincx())) {
+    LOG(ERROR) << "Could not authenticate to the key slot.";
     return std::string();
   }
 

net/base/keygen_handler_unittest.cc

@@ -14,24 +14,59 @@
 #include "base/threading/thread_restrictions.h"
 #include "base/synchronization/waitable_event.h"
 #include "build/build_config.h"
-#include "crypto/nss_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(USE_NSS)
 #include <private/pprthred.h>  // PR_DetachThread
+#include "crypto/nss_crypto_module_delegate.h"
+#include "crypto/nss_util.h"
+#include "crypto/nss_util_internal.h"
 #endif
 
 namespace net {
 
 namespace {
 
+#if defined(USE_NSS)
+class StubCryptoModuleDelegate : public crypto::NSSCryptoModuleDelegate {
+  public:
+   explicit StubCryptoModuleDelegate(crypto::ScopedPK11Slot slot)
+       : slot_(slot.Pass()) {}
+
+   virtual std::string RequestPassword(const std::string& slot_name,
+                                       bool retry,
+                                       bool* cancelled) OVERRIDE{
+     return std::string();
+   }
+
+   virtual crypto::ScopedPK11Slot RequestSlot() OVERRIDE {
+     return crypto::ScopedPK11Slot(PK11_ReferenceSlot(slot_.get()));
+   }
+
+  private:
+   crypto::ScopedPK11Slot slot_;
+};
+#endif
+
 class KeygenHandlerTest : public ::testing::Test {
  public:
   KeygenHandlerTest() {}
   virtual ~KeygenHandlerTest() {}
 
+  scoped_ptr<KeygenHandler> CreateKeygenHandler() {
+    scoped_ptr<KeygenHandler> handler(new KeygenHandler(
+        768, "some challenge", GURL("http://www.example.com")));
+#if defined(USE_NSS)
+    handler->set_crypto_module_delegate(
+        scoped_ptr<crypto::NSSCryptoModuleDelegate>(
+            new StubCryptoModuleDelegate(
+                crypto::ScopedPK11Slot(crypto::GetPersistentNSSKeySlot()))));
+#endif
+    return handler.Pass();
+  }
+
  private:
-#if defined(OS_CHROMEOS) && defined(USE_NSS)
+#if defined(USE_NSS)
   crypto::ScopedTestNSSDB test_nss_db_;
 #endif
 };
@@ -73,22 +108,22 @@ void AssertValidSignedPublicKeyAndChallenge(const std::string& result,
 }
 
 TEST_F(KeygenHandlerTest, SmokeTest) {
-  KeygenHandler handler(768, "some challenge", GURL("http://www.example.com"));
-  handler.set_stores_key(false);  // Don't leave the key-pair behind
-  std::string result = handler.GenKeyAndSignChallenge();
+  scoped_ptr<KeygenHandler> handler(CreateKeygenHandler());
+  handler->set_stores_key(false);  // Don't leave the key-pair behind
+  std::string result = handler->GenKeyAndSignChallenge();
   VLOG(1) << "KeygenHandler produced: " << result;
   AssertValidSignedPublicKeyAndChallenge(result, "some challenge");
 }
 
-void ConcurrencyTestCallback(base::WaitableEvent* event,
-                             const std::string& challenge,
+void ConcurrencyTestCallback(const std::string& challenge,
+                             base::WaitableEvent* event,
+                             scoped_ptr<KeygenHandler> handler,
                              std::string* result) {
   // We allow Singleton use on the worker thread here since we use a
   // WaitableEvent to synchronize, so it's safe.
   base::ThreadRestrictions::ScopedAllowSingleton scoped_allow_singleton;
-  KeygenHandler handler(768, challenge, GURL("http://www.example.com"));
-  handler.set_stores_key(false);  // Don't leave the key-pair behind.
-  *result = handler.GenKeyAndSignChallenge();
+  handler->set_stores_key(false);  // Don't leave the key-pair behind.
+  *result = handler->GenKeyAndSignChallenge();
   event->Signal();
 #if defined(USE_NSS)
   // Detach the thread from NSPR.
@@ -109,12 +144,15 @@ TEST_F(KeygenHandlerTest, ConcurrencyTest) {
   base::WaitableEvent* events[NUM_HANDLERS] = { NULL };
   std::string results[NUM_HANDLERS];
   for (int i = 0; i < NUM_HANDLERS; i++) {
+    scoped_ptr<KeygenHandler> handler(CreateKeygenHandler());
     events[i] = new base::WaitableEvent(false, false);
-    base::WorkerPool::PostTask(
-        FROM_HERE,
-        base::Bind(ConcurrencyTestCallback, events[i], "some challenge",
-                   &results[i]),
-        true);
+    base::WorkerPool::PostTask(FROM_HERE,
+                               base::Bind(ConcurrencyTestCallback,
+                                          "some challenge",
+                                          events[i],
+                                          base::Passed(&handler),
+                                          &results[i]),
+                               true);
   }
 
   for (int i = 0; i < NUM_HANDLERS; i++) {