Remove NSSCertDatabase from ClientCertStoreChromeOS unittest.

net/ssl/client_cert_store_chromeos_unittest.cc

Index: net/ssl/client_cert_store_chromeos_unittest.cc
diff --git a/net/ssl/client_cert_store_chromeos_unittest.cc b/net/ssl/client_cert_store_chromeos_unittest.cc
index 65b668dbb25d21696337db71aeef49b0d5c58ded..d25fb0939d1be9abe990a4b9317727718c1de21b 100644
--- a/net/ssl/client_cert_store_chromeos_unittest.cc
+++ b/net/ssl/client_cert_store_chromeos_unittest.cc
@@ -4,94 +4,168 @@
 
 #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(scoped_refptr<X509Certificate> cert,

[Ryan Sleevi, 2014/07/17 22:18:52]

const scoped_refptr<>&

[pneubeck (no reviews), 2014/07/18 08:42:42]

Done.

+                            PK11SlotInfo* slot) {
+  CK_OBJECT_HANDLE key;
+  crypto::ScopedPK11Slot key_slot(
+      PK11_KeyForCertExists(cert->os_cert_handle(), &key, NULL));
+  if (slot != key_slot.get()) {
+    LOG(ERROR) << "Did not find key in the right slot";
+    return false;
+  }

[Ryan Sleevi, 2014/07/17 22:18:52]

Not sure why you do this check. It's not required that a cert be in the same key
as its slot.

[pneubeck (no reviews), 2014/07/18 08:42:42]

Removed.

+
+  std::string nickname = cert->GetDefaultNickname(USER_CERT);
+  {
+    crypto::AutoNSSWriteLock lock;
+    SECStatus rv = PK11_ImportCert(
+        slot, cert->os_cert_handle(), key, nickname.c_str(), PR_FALSE);
+    if (rv != SECSuccess) {
+      LOG(ERROR) << "Could not import cert";
+      return false;
+    }
+  }
+  return true;
+}
+
+}  // namespace
+
+class ClientCertStoreChromeOSTestDelegate {
+ public:
+  ClientCertStoreChromeOSTestDelegate()
+      : user_("scopeduser"),
+        store_(user_.username_hash(),
+               ClientCertStoreChromeOS::PasswordDelegateFactory()) {
+    CHECK(user_.constructed_successfully());
+    user_.FinishInit();
+
+    slot_ = crypto::GetPublicSlotForChromeOSUser(user_.username_hash());
+    CHECK(slot_);
+
+    // Client certs can only be imported to |slot_| if the respective private
+    // key is present. Import the private keys for all client certificates that
+    // are used during the test.

[Ryan Sleevi, 2014/07/17 22:18:52]

This doesn't sound right, especially since NSS does violate this.

[pneubeck (no reviews), 2014/07/18 08:42:42]

I never get used to the spares (or rather not existent) NSS documentation...
Found finally that CK_INVALID_KEY and that the key is not required for the
import.

+    ImportSensitiveKeyFromFile(
+        GetTestCertsDirectory(), "client_1.pk8", slot_.get());
+    ImportSensitiveKeyFromFile(
+        GetTestCertsDirectory(), "client_2.pk8", slot_.get());

[Ryan Sleevi, 2014/07/17 22:18:51]

One way to reduce refactoring would be to defer all this setup until
SelectClientCerts() was called, and then return false if setup failed.

[pneubeck (no reviews), 2014/07/18 08:42:42]

Done.

+  }
+
+  bool SelectClientCerts(const CertificateList& input_certs,
+                         const SSLCertRequestInfo& cert_request_info,
+                         CertificateList* selected_certs) {
+    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_;
+  crypto::ScopedPK11Slot slot_;
+};

[Ryan Sleevi, 2014/07/17 22:18:52]

More documentation is needed - either in this class, on 72, or 94/95, to
document what's going on and what the expectations are here.

Most importantly is noting that the base test is defined somewhere else (took me
a while to figure that out)

+
+// This tests whether the filtering functionality is correctly delegated to the
+// base class ClientCertStoreNSS.

[Ryan Sleevi, 2014/07/17 22:18:52]

Eh? This doesn't make sense, considering that you're using
ClientCertStoreChromeOS everywhere.

[pneubeck (no reviews), 2014/07/18 08:42:42]

I don't see why it didn't make sense.
But I made the explanation a bit more verbose.

+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));
+
+    if (!cert) {
+      LOG(ERROR) << "Failed to parse cert from file " << cert_filename;
+      return NULL;
+    }
 
-    EXPECT_EQ(0, rv);
-    EXPECT_EQ(1U, cert_list.size());
-    if (rv || cert_list.size() != 1)
+    if (!ImportClientCertToSlot(cert, slot.get()))
       return NULL;
 
-    return cert_list[0];
+    return cert;

[Ryan Sleevi, 2014/07/17 22:18:52]

|cert| may (and almost certainly will) refer to the old/global cert, not the
cert in the slot.

Not that this should matter in practice, but it does go back to NSS not caring
about the slot.

[pneubeck (no reviews), 2014/07/18 08:42:42]

Added a comment.

   }
 };
 
-// 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) {
+TEST_F(ClientCertStoreChromeOSTest,
+       IfRequestCertsBeforeNSSDBInitializedThenRequestWaitsForInitAndSucceeds) {

[Ryan Sleevi, 2014/07/17 22:18:52]

I actually meant just include a more meaningful comment.
ReallyLongSentencesWithCamelCapsCanBeHardToReadAndEvenHarderToTypeOnCommandLines

What you did on 190-192 is good stuff and what I was asking for :)

[pneubeck (no reviews), 2014/07/18 08:42:42]

I blame TotT for that...

   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;
+  base::RunLoop run_loop;
   store.GetClientCerts(
-      *request_1, &request_1->client_certs, run_loop_1.QuitClosure());
-  store.GetClientCerts(
-      *request_all, &request_all->client_certs, run_loop_all.QuitClosure());
-
-  // Callbacks won't be run until nss_util init finishes for the user.
+      *request_all, &request_all->client_certs, run_loop.QuitClosure());
+
+  {
+    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) {
+TEST_F(ClientCertStoreChromeOSTest,
+       IfRequestCertsAfterNSSDBInitializedThenRequestSucceeds) {
   crypto::ScopedTestNSSChromeOSUser user("scopeduser");
   ASSERT_TRUE(user.constructed_successfully());
   user.FinishInit();
@@ -99,70 +173,68 @@ 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());
+      *request_all, &request_all->client_certs, run_loop.QuitClosure());
 
-  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, 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,
+       IfTwoNSSDBsExistThenCertRequestsReturnOnlyCertsOfTheGivenDB) {
   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