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

crypto/nss_util.cc

@@ -57,7 +57,7 @@ namespace crypto {
 namespace {
 
 #if defined(OS_CHROMEOS)
-const char kNSSDatabaseName[] = "Real NSS database";
+const char kUserNSSDatabaseName[] = "UserNSSDB";
 
 // Constants for loading the Chrome OS TPM-backed PKCS #11 library.
 const char kChapsModuleName[] = "Chaps";
@@ -287,7 +287,8 @@ class NSSInitSingleton {
     PK11SlotInfo* tpm_slot;
   };
 
-  PK11SlotInfo* OpenPersistentNSSDBForPath(const base::FilePath& path) {
+  PK11SlotInfo* OpenPersistentNSSDBForPath(const std::string& db_name,
+                                           const base::FilePath& path) {
     DCHECK(thread_checker_.CalledOnValidThread());
     // NSS is allowed to do IO on the current thread since dispatching
     // to a dedicated thread would still have the affect of blocking
@@ -299,7 +300,7 @@ class NSSInitSingleton {
       LOG(ERROR) << "Failed to create " << nssdb_path.value() << " directory.";
       return NULL;
     }
-    return OpenUserDB(nssdb_path, kNSSDatabaseName);
+    return OpenUserDB(nssdb_path, db_name);
   }
 
   void EnableTPMTokenForNSS() {
@@ -469,7 +470,9 @@ class NSSInitSingleton {
       return false;
 
     DVLOG(2) << "Opening NSS DB " << path.value();
-    ScopedPK11Slot public_slot(OpenPersistentNSSDBForPath(path));
+    std::string db_name = base::StringPrintf(
+        "%s %s", kUserNSSDatabaseName, username_hash.c_str());
+    ScopedPK11Slot public_slot(OpenPersistentNSSDBForPath(db_name, path));
     chromeos_user_map_[username_hash] =
         new ChromeOSUserData(public_slot.Pass());
     return true;
@@ -861,10 +864,11 @@ class NSSInitSingleton {
 #endif
 
   static PK11SlotInfo* OpenUserDB(const base::FilePath& path,
-                                  const char* description) {
+                                  const std::string& description) {
     const std::string modspec =
         base::StringPrintf("configDir='sql:%s' tokenDescription='%s'",
-                           path.value().c_str(), description);
+                           path.value().c_str(),
+                           description.c_str());
     PK11SlotInfo* db_slot = SECMOD_OpenUserDB(modspec.c_str());
     if (db_slot) {
       if (PK11_NeedUserInit(db_slot))

net/BUILD.gn

@@ -639,6 +639,7 @@ source_set("test_support") {
     "socket/socket_test_util.h",
     "test/cert_test_util.cc",
     "test/cert_test_util.h",
+    "test/cert_test_util_nss.cc",
     "test/ct_test_util.cc",
     "test/ct_test_util.h",
     "test/embedded_test_server/embedded_test_server.cc",
@@ -707,6 +708,12 @@ source_set("test_support") {
     ]
   }
 
+  if (!use_nss_certs) {
+    sources -= [
+      "test/cert_test_util_nss.cc",
+    ]
+  }
+
   forward_dependent_configs_from = deps
 }
 

net/net.gyp

@@ -938,6 +938,7 @@
         'socket/socket_test_util.h',
         'test/cert_test_util.cc',
         'test/cert_test_util.h',
+        'test/cert_test_util_nss.cc',
         'test/ct_test_util.cc',
         'test/ct_test_util.h',
         'test/embedded_test_server/embedded_test_server.cc',
@@ -1021,6 +1022,11 @@
               'dns/mock_mdns_socket_factory.h'
             ]
         }],
+        [ 'use_nss != 1', {
+            'sources!': [
+              'test/cert_test_util_nss.cc',
+            ],
+        }],
       ],
       # TODO(jschuh): crbug.com/167187 fix size_t to int truncations.
       'msvs_disabled_warnings': [4267, ],

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

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_;
 

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

net/test/cert_test_util.h

@@ -11,16 +11,37 @@
 #include "net/cert/x509_cert_types.h"
 #include "net/cert/x509_certificate.h"
 
+#if defined(USE_NSS)
+#include "base/memory/scoped_ptr.h"
+
+// From <pk11pub.h>
+typedef struct PK11SlotInfoStr PK11SlotInfo;
+#endif
+
 namespace base {
 class FilePath;
 }
 
+namespace crypto {
+class RSAPrivateKey;
+}
+
 namespace net {
 
 class EVRootCAMetadata;
 
-// Imports all of the certificates in |cert_file|, a file in |certs_dir|,
-// // into a CertificateList.
+#if defined(USE_NSS)
+// Imports a private key from file |key_filename| in |dir|. The file must
+// contain a PKCS#8 PrivateKeyInfo in DER encoding. The key is imported to
+// |slot|.
+scoped_ptr<crypto::RSAPrivateKey> ImportSensitiveKeyFromFile(
+    const base::FilePath& dir,
+    const std::string& key_filename,
+    PK11SlotInfo* slot);
+#endif
+
+// Imports all of the certificates in |cert_file|, a file in |certs_dir|, into a
+// CertificateList.
 CertificateList CreateCertificateListFromFile(const base::FilePath& certs_dir,
                                               const std::string& cert_file,
                                               int format);

net/test/cert_test_util_nss.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 "net/test/cert_test_util.h"
+
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/path_service.h"
+#include "crypto/rsa_private_key.h"
+
+namespace net {
+
+scoped_ptr<crypto::RSAPrivateKey> ImportSensitiveKeyFromFile(
+    const base::FilePath& dir,
+    const std::string& key_filename,
+    PK11SlotInfo* slot) {
+  base::FilePath key_path = dir.AppendASCII(key_filename);
+  std::string key_pkcs8;
+  bool success = base::ReadFileToString(key_path, &key_pkcs8);
+  if (!success) {
+    LOG(ERROR) << "Failed to read file " << key_path.value();
+    return scoped_ptr<crypto::RSAPrivateKey>();
+  }
+
+  const uint8* key_pkcs8_begin =
+      reinterpret_cast<const uint8*>(key_pkcs8.data());
+  std::vector<uint8> key_vector(key_pkcs8_begin,
+                                key_pkcs8_begin + key_pkcs8.length());
+
+  scoped_ptr<crypto::RSAPrivateKey> private_key(
+      crypto::RSAPrivateKey::CreateSensitiveFromPrivateKeyInfo(slot,
+                                                               key_vector));
+  LOG_IF(ERROR, !private_key) << "Could not create key from file "
+                              << key_path.value();
+  return private_key.Pass();
+}
+
+}  // namespace net