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..0207a82e1a041898449d10591c1cf8de7daa4b0a 100644
--- a/net/ssl/client_cert_store_chromeos_unittest.cc
+++ b/net/ssl/client_cert_store_chromeos_unittest.cc
@@ -11,8 +11,8 @@
 #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 "net/ssl/client_cert_store_unittest-inl.h"
+#include "net/test/cert_test_util.h"
 
 namespace net {
 
@@ -20,40 +20,50 @@ 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())
       return NULL;
 
-    net::CertificateList cert_list;
+    ImportSensitiveKeyFromFile(
+        GetTestCertsDirectory(), key_filename, slot.get());
 
-    base::FilePath p12_path = GetTestCertsDirectory().AppendASCII(filename);
-    std::string p12_data;
-    if (!base::ReadFileToString(p12_path, &p12_data)) {
-      EXPECT_TRUE(false);
-      return NULL;
-    }
+    scoped_refptr<X509Certificate> cert(
+        ImportCertFromFile(GetTestCertsDirectory(), cert_filename));
 
-    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);
+    EXPECT_TRUE(cert) << "Failed to parse cert from file " << cert_filename;
+    if (!cert)
+      return NULL;
 
-    EXPECT_EQ(0, rv);
-    EXPECT_EQ(1U, cert_list.size());
-    if (rv || cert_list.size() != 1)
+    CK_OBJECT_HANDLE key;
+    crypto::ScopedPK11Slot key_slot(
+        PK11_KeyForCertExists(cert->os_cert_handle(), &key, NULL));
+    EXPECT_EQ(slot.get(), key_slot.get())
+        << "Did not find key in the right slot, for cert file "
+        << cert_filename;

[Ryan Sleevi, 2014/07/16 19:32:18]

Note: This is generally a dangerous pattern to put EXPECT clauses within a
sub-routine, since they can lead to subtle error when someone makes them ASSERT
clauses (e.g.
https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai...
)

[pneubeck (no reviews), 2014/07/16 19:49:44]

I followed the existing style.
I'd actually would prefer CHECKs, but these are also controversial in tests.

So, I'll just remove the EXPECTs and ASSERT at the call site.

[Ryan Sleevi, 2014/07/16 19:59:00]

I think it's fine, but it means that you need to wrap these calls in the
ASSERT_NO_FATAL_FAILURES macro, in order to prevent surprises.

[pneubeck (no reviews), 2014/07/17 13:26:43]

Yeah, but that would still mean that there are redundant checks of the failure
conditions (one in the EXPECT statement and one to return).
I went ahead with the simpler and still safe variant to LOG(ERROR) and return.
ASSERTion is done at the call-site.

+    if (slot.get() != key_slot.get())
       return NULL;
 
-    return cert_list[0];
+    // Use some nickname that is unique within this test.
+    std::string nickname = cert_filename;
+    {
+      crypto::AutoNSSWriteLock lock;
+      SECStatus rv = PK11_ImportCert(
+          slot.get(), cert->os_cert_handle(), key, nickname.c_str(), PR_FALSE);
+      EXPECT_EQ(SECSuccess, rv) << "Could not import cert from file "
+                                << cert_filename;
+      if (rv != SECSuccess)
+        return NULL;
+    }
+
+    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.
+// TODO(mattm): Do better testing of cert_authorities matching below.
 
 TEST_F(ClientCertStoreChromeOSTest, WaitForNSSInit) {
   crypto::ScopedTestNSSChromeOSUser user("scopeduser");
@@ -61,14 +71,15 @@ TEST_F(ClientCertStoreChromeOSTest, WaitForNSSInit) {
   ClientCertStoreChromeOS store(
       user.username_hash(), ClientCertStoreChromeOS::PasswordDelegateFactory());
   scoped_refptr<X509Certificate> cert_1(
-      ImportCertForUser(user.username_hash(), "client.p12", "12345"));
+      ImportCertForUser(user.username_hash(), "client_1.pem", "client_1.pk8"));
+  ASSERT_TRUE(cert_1);
   scoped_refptr<X509Certificate> cert_2(
-      ImportCertForUser(user.username_hash(), "websocket_client_cert.p12", ""));
+      ImportCertForUser(user.username_hash(), "client_2.pem", "client_2.pk8"));
+  ASSERT_TRUE(cert_2);
 
-  std::vector<std::string> authority_1(
-      1,
-      std::string(reinterpret_cast<const char*>(kAuthority1DN),
-                  sizeof(kAuthority1DN)));
+  std::vector<std::string> authority_1;
+  authority_1.push_back(std::string(
+      reinterpret_cast<const char*>(kAuthority1DN), sizeof(kAuthority1DN)));
   scoped_refptr<SSLCertRequestInfo> request_1(new SSLCertRequestInfo());
   request_1->cert_authorities = authority_1;
 
@@ -87,7 +98,7 @@ TEST_F(ClientCertStoreChromeOSTest, WaitForNSSInit) {
   run_loop_1.Run();
   run_loop_all.Run();
 
-  ASSERT_EQ(0u, request_1->client_certs.size());
+  ASSERT_EQ(1u, request_1->client_certs.size());

[Ryan Sleevi, 2014/07/16 19:32:18]

Why... is this? Seems wrong to update the test expectation here.

[pneubeck (no reviews), 2014/07/16 19:49:44]

Because they were wrong? :-)
See Matt's comment above.

Note that I didn't change it to 2, i.e. one of the certs is not matched.
Previously no cert or both certs were matched which is not proving that the
matching is actually working.

[Ryan Sleevi, 2014/07/16 19:59:00]

I don't know what comment you're referring to.

[pneubeck (no reviews), 2014/07/16 20:01:39]

Line 54 of the old file:
  // TODO(mattm): Do better testing of cert_authorities matching below. [...]

[pneubeck (no reviews), 2014/07/16 20:15:23]

That's fair.

My reasoning was:
Matt commented that something about the CA matching is imperfect.
The naming of the variable is following the patter *_1, *_2 and *_all where * is
either a client cert, an authority, or a request.
This indicates that Matt's original intention was that request_all should match
both client certs and request_1 should match only cert_1.

Independent from the author's intent, the question is what this unit test should
test.
IMO, it should verify that ClientCertStoreChromeOS is correctly delegating the
CA matching to the base class ClientCertStoreNSS. One 'heuristic' could be to
ensure that neither simply all certs are accepted or rejected. This can be
ensured by testing a request that matches exactly 1 out of 2 client certs.

And that would actually match the intent that I read from the existing test.
The only thing that doesn't match is the old test assertion in line 90:
  ASSERT_EQ(0u, request_1->client_certs.size());

[Ryan Sleevi, 2014/07/16 20:46:07]

I guess this highlights poor documentation.

I don't reach your same conclusions, though I understand how you got them. That
is, I agree that kAuthority1DN is meant to correlate to the authority requested
in cert_1.

What's not clear to me is that the cert is intended to be returned.

In particular, the only difference I see between the two tests is
user.FinishInit() (line 108)

The better explanation (it seems) from this is that it's an artifact of you
changing the certs being tested themselves. In the old code, neither client.p12
nor websocket_client_cert.p12 matched the request. Is this true?

[pneubeck (no reviews), 2014/07/16 21:29:58]

Ok. Let me explain that part then too.
Another thing that should be tested in this unit test is the asynchronous
loading of the private slot (using crypto::GetPrivateSlotForChromeOSUser).
Two possible control flows can occur:
 a) the slot is obtained synchronously
 b) the slot is obtained asynchronously via callback
(note: this difference wouldn't have to be tested here, if
GetPrivateSlotForChromeOSUser would only expose one way of returning the result)

The first test seems to test b), as indicated by the test name WaitForNSSInit,
the call order GetClientCerts() then user.FinishInit(), and the comment in line
95:
  // Callbacks won't be run until nss_util init finishes for the user.

The second test seems to test a), as indicate by the test name
NSSAlreadyInitialized and the call order  user.FinishInit() then
GetClientCerts().

If this were the only purpose of the these two tests (i.e. assuming my previous
reasoning would be wrong), then it would be sufficient to have a single request
per test.
But there are two requests, which again support my previous reasoning that it
was intended to test the delegation to ClientCertStoreNSS.
My explanation was not about why the value of request_1->client_certs.size()
changed, but about the reason why I changed the expectation and the used
certificates.
Yes, neither matched.
The changed result of request_1->client_certs.size() is because of the change
from client.p12 to client_1.pem . The former does not have the same issuer CA as
client_1.pem. The documentation of kAuthority1DN explicitly says that this DN is
the issuer of the client_1 cert.

Looking at the second sentence from Matt's comment (old line 55-56): 
      // TODO(mattm): [...] Update
net/data/ssl/scripts/generate-client-certificates.sh so that it actually	
     // saves the .p12 files, and regenerate them.

That script currently generates client_1.* and client_2.* with different CAs.
I thus assumed, that Matt had planned to generate the client.p12 with the same
client_1 or client_2 cert.

From our previous discussion, we came to the conclusion that PKCS12 is
unnecessarily complex and PKCS8 + a single X509 cert in PEM format are
preferable.
I updated the script accordingly.


And yet another argument to support the change to the new ASSERT_EQ(1u,
....client_certs.size()):
Other CertStore implementations are tested according to the tests defined in
client_cert_store_unittest-inl.h . There client_1 and client_2 certs are used
and it's verified that requesting for Authority1DN only returns client_1
(similary for client_2/Authority2DN).

I just checked the original commit of this test, but there was no discussion or
additional documentation of this test.
https://codereview.chromium.org/112533002

   ASSERT_EQ(2u, request_all->client_certs.size());
 }
 
@@ -99,9 +110,11 @@ TEST_F(ClientCertStoreChromeOSTest, NSSAlreadyInitialized) {
   ClientCertStoreChromeOS store(
       user.username_hash(), ClientCertStoreChromeOS::PasswordDelegateFactory());
   scoped_refptr<X509Certificate> cert_1(
-      ImportCertForUser(user.username_hash(), "client.p12", "12345"));
+      ImportCertForUser(user.username_hash(), "client_1.pem", "client_1.pk8"));
+  ASSERT_TRUE(cert_1);
   scoped_refptr<X509Certificate> cert_2(
-      ImportCertForUser(user.username_hash(), "websocket_client_cert.p12", ""));
+      ImportCertForUser(user.username_hash(), "client_2.pem", "client_2.pk8"));
+  ASSERT_TRUE(cert_2);
 
   std::vector<std::string> authority_1(
       1,
@@ -122,7 +135,7 @@ TEST_F(ClientCertStoreChromeOSTest, NSSAlreadyInitialized) {
   run_loop_1.Run();
   run_loop_all.Run();
 
-  ASSERT_EQ(0u, request_1->client_certs.size());
+  ASSERT_EQ(1u, request_1->client_certs.size());
   ASSERT_EQ(2u, request_all->client_certs.size());
 }
 
@@ -138,15 +151,18 @@ TEST_F(ClientCertStoreChromeOSTest, TwoUsers) {
       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());
 
   base::RunLoop run_loop_1;
   base::RunLoop run_loop_2;
+
   store1.GetClientCerts(
       *request_1, &request_1->client_certs, run_loop_1.QuitClosure());
   store2.GetClientCerts(
@@ -161,8 +177,8 @@ TEST_F(ClientCertStoreChromeOSTest, TwoUsers) {
 
   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.
+  ASSERT_EQ(1u, request_2->client_certs.size());
+  EXPECT_TRUE(cert_2->Equals(request_2->client_certs[0]));
 }
 
 }  // namespace net