Remove NSSCertDatabase from ClientCertStoreChromeOS unittest.

The database was only used to import a PKCS#12 file. By changing to separate key (PKCS#8 format) and cert (X509 in PEM encoding), only dependencies on the lower level RSAPrivateKey, X509Certificate and PK11_* NSS functions are required.
Note this removes at the same time a call to the deprecated NSSCertDatabase::GetInstance().

Also
- fixes multi profile cases of the unit test and the CA matching (the latter is now identical to all other platforms).
- fixes a bug in the matching of client certs from software slots, because of reused cert database names
- gets rid of the error output that occurred during the PKCS12 import because the file contained also a CA cert:
  [ERROR:nsPKCS12Blob.cpp(219)] Could not grab a handle to the certificate in the slot from the corresponding PKCS#12 DER certificate.

BUG=210525, 329735,315285

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

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

3 files changed, 175 insertions, 121 deletions

diff --git a/net/ssl/client_cert_store_chromeos.cc b/net/ssl/client_cert_store_chromeos.cc
index f357797..bd4a5c4 100644
--- a/net/ssl/client_cert_store_chromeos.cc
+++ b/net/ssl/client_cert_store_chromeos.cc
@@ -67,28 +67,4 @@ void ClientCertStoreChromeOS::DidGetPrivateSlot(
   ClientCertStoreNSS::GetClientCerts(*request, selected_certs, callback);
 }
 
-void ClientCertStoreChromeOS::InitForTesting(
-    crypto::ScopedPK11Slot public_slot,
-    crypto::ScopedPK11Slot private_slot) {
-  profile_filter_.Init(public_slot.Pass(), private_slot.Pass());
-}
-
-bool ClientCertStoreChromeOS::SelectClientCertsForTesting(
-    const CertificateList& input_certs,
-    const SSLCertRequestInfo& request,
-    CertificateList* selected_certs) {
-  CERTCertList* cert_list = CERT_NewCertList();
-  if (!cert_list)
-    return false;
-  for (size_t i = 0; i < input_certs.size(); ++i) {
-    CERT_AddCertToListTail(
-        cert_list, CERT_DupCertificate(input_certs[i]->os_cert_handle()));
-  }
-
-  GetClientCertsImpl(cert_list, request, false, selected_certs);
-  CERT_DestroyCertList(cert_list);
-  return true;
-}
-
-
 }  // namespace net
diff --git a/net/ssl/client_cert_store_chromeos.h b/net/ssl/client_cert_store_chromeos.h
index 41339f1..087190c 100644
--- a/net/ssl/client_cert_store_chromeos.h
+++ b/net/ssl/client_cert_store_chromeos.h
@@ -40,21 +40,6 @@ class NET_EXPORT ClientCertStoreChromeOS : public ClientCertStoreNSS {
                          const base::Closure& callback,
                          crypto::ScopedPK11Slot private_slot);
 
-  // Allows tests to initialize the cert store with the given slots.
-  // Must be called before SelectClientCertsForTesting.
-  void InitForTesting(crypto::ScopedPK11Slot public_slot,
-                      crypto::ScopedPK11Slot private_slot);
-
-  // A hook for testing. Filters |input_certs| using the logic being used to
-  // filter the system store when GetClientCerts() is called.
-  // Implemented by creating a list of certificates that otherwise would be
-  // extracted from the system store and filtering it using the common logic
-  // (less adequate than the approach used on Windows).
-  bool SelectClientCertsForTesting(const CertificateList& input_certs,
-                                   const SSLCertRequestInfo& cert_request_info,
-                                   CertificateList* selected_certs);
-
-
   std::string username_hash_;
   NSSProfileFilterChromeOS profile_filter_;
 
diff --git a/net/ssl/client_cert_store_chromeos_unittest.cc b/net/ssl/client_cert_store_chromeos_unittest.cc
index 65b668d..4a6a6c3 100644
--- a/net/ssl/client_cert_store_chromeos_unittest.cc
+++ b/net/ssl/client_cert_store_chromeos_unittest.cc
@@ -4,94 +4,191 @@
 
 #include "net/ssl/client_cert_store_chromeos.h"
 
+#include <string>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/run_loop.h"
-#include "base/strings/utf_string_conversions.h"
 #include "crypto/nss_util.h"
 #include "crypto/nss_util_internal.h"
-#include "net/cert/nss_cert_database.h"
+#include "crypto/rsa_private_key.h"
+#include "net/base/test_data_directory.h"
+#include "net/cert/cert_type.h"
+#include "net/cert/x509_certificate.h"
 #include "net/ssl/client_cert_store_unittest-inl.h"
+#include "net/test/cert_test_util.h"
 
 namespace net {
 
+namespace {
+
+bool ImportClientCertToSlot(const scoped_refptr<X509Certificate>& cert,
+                            PK11SlotInfo* slot) {
+  std::string nickname = cert->GetDefaultNickname(USER_CERT);
+  {
+    crypto::AutoNSSWriteLock lock;
+    SECStatus rv = PK11_ImportCert(slot,
+                                   cert->os_cert_handle(),
+                                   CK_INVALID_HANDLE,
+                                   nickname.c_str(),
+                                   PR_FALSE);
+    if (rv != SECSuccess) {
+      LOG(ERROR) << "Could not import cert";
+      return false;
+    }
+  }
+  return true;
+}
+
+}  // namespace
+
+// Define a delegate to be used for instantiating the parameterized test set
+// ClientCertStoreTest.
+class ClientCertStoreChromeOSTestDelegate {
+ public:
+  ClientCertStoreChromeOSTestDelegate()
+      : user_("scopeduser"),
+        store_(user_.username_hash(),
+               ClientCertStoreChromeOS::PasswordDelegateFactory()) {
+    // Defer futher initialization and checks to SelectClientCerts, because the
+    // constructor doesn't allow us to return an initialization result. Could be
+    // cleaned up by adding an Init() function.
+  }
+
+  // Called by the ClientCertStoreTest tests.
+  // |inpurt_certs| contains certificates to select from. Because
+  // ClientCertStoreChromeOS filters also for the right slot, we have to import
+  // the certs at first.
+  // Since the certs are imported, the store can be tested by using its public
+  // interface (GetClientCerts), which will read the certs from NSS.
+  bool SelectClientCerts(const CertificateList& input_certs,
+                         const SSLCertRequestInfo& cert_request_info,
+                         CertificateList* selected_certs) {
+    if (!user_.constructed_successfully()) {
+      LOG(ERROR) << "Scoped test user DB could not be constructed.";
+      return false;
+    }
+    user_.FinishInit();
+
+    crypto::ScopedPK11Slot slot(
+        crypto::GetPublicSlotForChromeOSUser(user_.username_hash()));
+    if (!slot) {
+      LOG(ERROR) << "Could not get the user's public slot";
+      return false;
+    }
+
+    // Only user certs are considered for the cert request, which means that the
+    // private key must be known to NSS. Import all private keys for certs that
+    // are used througout the test.
+    if (!ImportSensitiveKeyFromFile(
+            GetTestCertsDirectory(), "client_1.pk8", slot.get()) ||
+        !ImportSensitiveKeyFromFile(
+            GetTestCertsDirectory(), "client_2.pk8", slot.get())) {
+      return false;
+    }
+
+    for (CertificateList::const_iterator it = input_certs.begin();
+         it != input_certs.end();
+         ++it) {
+      if (!ImportClientCertToSlot(*it, slot.get()))
+        return false;
+    }
+    base::RunLoop run_loop;
+    store_.GetClientCerts(
+        cert_request_info, selected_certs, run_loop.QuitClosure());
+    run_loop.Run();
+    return true;
+  }
+
+ private:
+  crypto::ScopedTestNSSChromeOSUser user_;
+  ClientCertStoreChromeOS store_;
+};
+
+// ClientCertStoreChromeOS derives from ClientCertStoreNSS and delegates the
+// filtering by issuer to that base class.
+// To verify that this delegation is functional, run the same filtering tests as
+// for the other implementations. These tests are defined in
+// client_cert_store_unittest-inl.h and are instantiated for each platform.
+INSTANTIATE_TYPED_TEST_CASE_P(ChromeOS,
+                              ClientCertStoreTest,
+                              ClientCertStoreChromeOSTestDelegate);
+
 class ClientCertStoreChromeOSTest : public ::testing::Test {
  public:
   scoped_refptr<X509Certificate> ImportCertForUser(
       const std::string& username_hash,
-      const std::string& filename,
-      const std::string& password) {
+      const std::string& cert_filename,
+      const std::string& key_filename) {
     crypto::ScopedPK11Slot slot(
         crypto::GetPublicSlotForChromeOSUser(username_hash));
-    EXPECT_TRUE(slot.get());
-    if (!slot.get())
+    if (!slot) {
+      LOG(ERROR) << "No slot for user " << username_hash;
       return NULL;
+    }
 
-    net::CertificateList cert_list;
-
-    base::FilePath p12_path = GetTestCertsDirectory().AppendASCII(filename);
-    std::string p12_data;
-    if (!base::ReadFileToString(p12_path, &p12_data)) {
-      EXPECT_TRUE(false);
+    if (!ImportSensitiveKeyFromFile(
+            GetTestCertsDirectory(), key_filename, slot.get())) {
+      LOG(ERROR) << "Could not import private key for user " << username_hash;
       return NULL;
     }
 
-    scoped_refptr<net::CryptoModule> module(
-        net::CryptoModule::CreateFromHandle(slot.get()));
-    int rv = NSSCertDatabase::GetInstance()->ImportFromPKCS12(
-        module.get(), p12_data, base::UTF8ToUTF16(password), false, &cert_list);
+    scoped_refptr<X509Certificate> cert(
+        ImportCertFromFile(GetTestCertsDirectory(), cert_filename));
 
-    EXPECT_EQ(0, rv);
-    EXPECT_EQ(1U, cert_list.size());
-    if (rv || cert_list.size() != 1)
+    if (!cert) {
+      LOG(ERROR) << "Failed to parse cert from file " << cert_filename;
       return NULL;
+    }
 
-    return cert_list[0];
+    if (!ImportClientCertToSlot(cert, slot.get()))
+      return NULL;
+
+    // |cert| continues to point to the original X509Certificate before the
+    // import to |slot|. However this should not make a difference for this
+    // test.
+    return cert;
   }
 };
 
-// TODO(mattm): Do better testing of cert_authorities matching below. Update
-// net/data/ssl/scripts/generate-client-certificates.sh so that it actually
-// saves the .p12 files, and regenerate them.
-
-TEST_F(ClientCertStoreChromeOSTest, WaitForNSSInit) {
+// Ensure that cert requests, that are started before the user's NSS DB is
+// initialized, will wait for the initialization and succeed afterwards.
+TEST_F(ClientCertStoreChromeOSTest, RequestWaitsForNSSInitAndSucceeds) {
   crypto::ScopedTestNSSChromeOSUser user("scopeduser");
   ASSERT_TRUE(user.constructed_successfully());
   ClientCertStoreChromeOS store(
       user.username_hash(), ClientCertStoreChromeOS::PasswordDelegateFactory());
   scoped_refptr<X509Certificate> cert_1(
-      ImportCertForUser(user.username_hash(), "client.p12", "12345"));
-  scoped_refptr<X509Certificate> cert_2(
-      ImportCertForUser(user.username_hash(), "websocket_client_cert.p12", ""));
-
-  std::vector<std::string> authority_1(
-      1,
-      std::string(reinterpret_cast<const char*>(kAuthority1DN),
-                  sizeof(kAuthority1DN)));
-  scoped_refptr<SSLCertRequestInfo> request_1(new SSLCertRequestInfo());
-  request_1->cert_authorities = authority_1;
+      ImportCertForUser(user.username_hash(), "client_1.pem", "client_1.pk8"));
+  ASSERT_TRUE(cert_1);
 
+  // Request any client certificate, which is expected to match client_1.
   scoped_refptr<SSLCertRequestInfo> request_all(new SSLCertRequestInfo());
 
-  base::RunLoop run_loop_1;
-  base::RunLoop run_loop_all;
-  store.GetClientCerts(
-      *request_1, &request_1->client_certs, run_loop_1.QuitClosure());
+  base::RunLoop run_loop;
   store.GetClientCerts(
-      *request_all, &request_all->client_certs, run_loop_all.QuitClosure());
+      *request_all, &request_all->client_certs, run_loop.QuitClosure());
 
-  // Callbacks won't be run until nss_util init finishes for the user.
+  {
+    base::RunLoop run_loop_inner;
+    run_loop_inner.RunUntilIdle();
+    // GetClientCerts should wait for the initialization of the user's DB to
+    // finish.
+    ASSERT_EQ(0u, request_all->client_certs.size());
+  }
+  // This should trigger the GetClientCerts operation to finish and to call
+  // back.
   user.FinishInit();
 
-  run_loop_1.Run();
-  run_loop_all.Run();
+  run_loop.Run();
 
-  ASSERT_EQ(0u, request_1->client_certs.size());
-  ASSERT_EQ(2u, request_all->client_certs.size());
+  ASSERT_EQ(1u, request_all->client_certs.size());
 }
 
-TEST_F(ClientCertStoreChromeOSTest, NSSAlreadyInitialized) {
+// Ensure that cert requests, that are started after the user's NSS DB was
+// initialized, will succeed.
+TEST_F(ClientCertStoreChromeOSTest, RequestsAfterNSSInitSucceed) {
   crypto::ScopedTestNSSChromeOSUser user("scopeduser");
   ASSERT_TRUE(user.constructed_successfully());
   user.FinishInit();
@@ -99,70 +196,66 @@ TEST_F(ClientCertStoreChromeOSTest, NSSAlreadyInitialized) {
   ClientCertStoreChromeOS store(
       user.username_hash(), ClientCertStoreChromeOS::PasswordDelegateFactory());
   scoped_refptr<X509Certificate> cert_1(
-      ImportCertForUser(user.username_hash(), "client.p12", "12345"));
-  scoped_refptr<X509Certificate> cert_2(
-      ImportCertForUser(user.username_hash(), "websocket_client_cert.p12", ""));
-
-  std::vector<std::string> authority_1(
-      1,
-      std::string(reinterpret_cast<const char*>(kAuthority1DN),
-                  sizeof(kAuthority1DN)));
-  scoped_refptr<SSLCertRequestInfo> request_1(new SSLCertRequestInfo());
-  request_1->cert_authorities = authority_1;
+      ImportCertForUser(user.username_hash(), "client_1.pem", "client_1.pk8"));
+  ASSERT_TRUE(cert_1);
 
   scoped_refptr<SSLCertRequestInfo> request_all(new SSLCertRequestInfo());
 
-  base::RunLoop run_loop_1;
-  base::RunLoop run_loop_all;
-  store.GetClientCerts(
-      *request_1, &request_1->client_certs, run_loop_1.QuitClosure());
+  base::RunLoop run_loop;
   store.GetClientCerts(
-      *request_all, &request_all->client_certs, run_loop_all.QuitClosure());
-
-  run_loop_1.Run();
-  run_loop_all.Run();
+      *request_all, &request_all->client_certs, run_loop.QuitClosure());
+  run_loop.Run();
 
-  ASSERT_EQ(0u, request_1->client_certs.size());
-  ASSERT_EQ(2u, request_all->client_certs.size());
+  ASSERT_EQ(1u, request_all->client_certs.size());
 }
 
-TEST_F(ClientCertStoreChromeOSTest, TwoUsers) {
+// This verifies that a request in the context of User1 doesn't see certificates
+// of User2, and the other way round. We check both directions, to ensure that
+// the behavior doesn't depend on initialization order of the DBs, for example.
+TEST_F(ClientCertStoreChromeOSTest, RequestDoesCrossReadSecondDB) {
   crypto::ScopedTestNSSChromeOSUser user1("scopeduser1");
   ASSERT_TRUE(user1.constructed_successfully());
   crypto::ScopedTestNSSChromeOSUser user2("scopeduser2");
   ASSERT_TRUE(user2.constructed_successfully());
+
+  user1.FinishInit();
+  user2.FinishInit();
+
   ClientCertStoreChromeOS store1(
       user1.username_hash(),
       ClientCertStoreChromeOS::PasswordDelegateFactory());
   ClientCertStoreChromeOS store2(
       user2.username_hash(),
       ClientCertStoreChromeOS::PasswordDelegateFactory());
+
   scoped_refptr<X509Certificate> cert_1(
-      ImportCertForUser(user1.username_hash(), "client.p12", "12345"));
-  scoped_refptr<X509Certificate> cert_2(ImportCertForUser(
-      user2.username_hash(), "websocket_client_cert.p12", ""));
+      ImportCertForUser(user1.username_hash(), "client_1.pem", "client_1.pk8"));
+  ASSERT_TRUE(cert_1);
+  scoped_refptr<X509Certificate> cert_2(
+      ImportCertForUser(user2.username_hash(), "client_2.pem", "client_2.pk8"));
+  ASSERT_TRUE(cert_2);
 
-  scoped_refptr<SSLCertRequestInfo> request_1(new SSLCertRequestInfo());
-  scoped_refptr<SSLCertRequestInfo> request_2(new SSLCertRequestInfo());
+  scoped_refptr<SSLCertRequestInfo> request_all(new SSLCertRequestInfo());
 
   base::RunLoop run_loop_1;
   base::RunLoop run_loop_2;
+
+  CertificateList selected_certs1, selected_certs2;
   store1.GetClientCerts(
-      *request_1, &request_1->client_certs, run_loop_1.QuitClosure());
+      *request_all, &selected_certs1, run_loop_1.QuitClosure());
   store2.GetClientCerts(
-      *request_2, &request_2->client_certs, run_loop_2.QuitClosure());
-
-  // Callbacks won't be run until nss_util init finishes for the user.
-  user1.FinishInit();
-  user2.FinishInit();
+      *request_all, &selected_certs2, run_loop_2.QuitClosure());
 
   run_loop_1.Run();
   run_loop_2.Run();
 
-  ASSERT_EQ(1u, request_1->client_certs.size());
-  EXPECT_TRUE(cert_1->Equals(request_1->client_certs[0]));
-  // TODO(mattm): Request for second user will have zero results due to
-  // crbug.com/315285.  Update the test once that is fixed.
+  // store1 should only return certs of user1, namely cert_1.
+  ASSERT_EQ(1u, selected_certs1.size());
+  EXPECT_TRUE(cert_1->Equals(selected_certs1[0]));
+
+  // store2 should only return certs of user2, namely cert_2.
+  ASSERT_EQ(1u, selected_certs2.size());
+  EXPECT_TRUE(cert_2->Equals(selected_certs2[0]));
 }
 
 }  // namespace net