Use user specific NSSDatabase in CertLoader.

chromeos/cert_loader_unittest.cc

Index: chromeos/cert_loader_unittest.cc
diff --git a/chromeos/cert_loader_unittest.cc b/chromeos/cert_loader_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c2c1917b4206d169a5732cc780299c5b61f04538
--- /dev/null
+++ b/chromeos/cert_loader_unittest.cc
@@ -0,0 +1,336 @@
+// 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 "chromeos/cert_loader.h"
+
+#include "base/bind.h"
+#include "base/file_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "crypto/nss_util.h"
+#include "crypto/nss_util_internal.h"
+#include "net/base/net_errors.h"
+#include "net/base/test_data_directory.h"
+#include "net/cert/nss_cert_database_chromeos.h"
+#include "net/cert/x509_certificate.h"
+#include "net/test/cert_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace {
+
+bool IsCertInCertificateList(const net::X509Certificate* cert,
+                             const net::CertificateList& cert_list) {
+  for (net::CertificateList::const_iterator it = cert_list.begin();
+       it != cert_list.end();
+       ++it) {
+    if (net::X509Certificate::IsSameOSCert((*it)->os_cert_handle(),
+                                            cert->os_cert_handle()))

[pneubeck (no reviews), 2014/01/24 13:18:02]

nit: missing {}

[tbarzic, 2014/01/25 00:26:27]

Done.

+      return true;
+  }
+  return false;
+}
+
+void NotReachedPrivateSlotCallback(crypto::ScopedPK11Slot slot) {

[pneubeck (no reviews), 2014/01/24 13:18:02]

Is "NotReached.." the right prefix here?
Maybe Fail... ?

[tbarzic, 2014/01/25 00:26:27]

yeah, FailOn... sounds betterthan NotReached

+  ASSERT_FALSE(true) << "GetPrivateSlotForChromeOSUser callback called even "

[pneubeck (no reviews), 2014/01/24 13:18:02]

EXPECT..., as it doesn't prevent a crash

[tbarzic, 2014/01/25 00:26:27]

Done.

+                     << "though the private slot had been initialized.";
+}
+
+class CertLoaderTest : public testing::Test,
+                       public CertLoader::Observer {
+ public:
+  CertLoaderTest() : cert_loader_(NULL),
+                     primary_user_("primary"),
+                     new_certificates_loaded_events_count_(0U) {
+  }
+
+  virtual ~CertLoaderTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    ASSERT_TRUE(primary_user_.constructed_successfully());
+    ASSERT_TRUE(
+        crypto::GetPublicSlotForChromeOSUser(primary_user_.username_hash()));

[pneubeck (no reviews), 2014/01/24 13:18:02]

why only checking the public slot and not the private slot?

[tbarzic, 2014/01/25 00:26:27]

the private slot does not have to be set at this point (not until
FinishInitialization).

+
+    CertLoader::Initialize();
+    cert_loader_ = CertLoader::Get();
+    cert_loader_->AddObserver(this);
+    cert_loader_->SetSlowTaskRunnerForTest(message_loop_.message_loop_proxy());
+  }
+
+  virtual void TearDown() {
+    cert_loader_->RemoveObserver(this);
+    CertLoader::Shutdown();
+  }
+
+ protected:
+  // CertLoader::Observer:
+  // The test keeps count of times the observer method was called.
+  virtual void OnCertificatesLoaded(const net::CertificateList& cert_list,
+                                    bool initial_load) OVERRIDE {
+    new_certificates_loaded_events_count_++;

[pneubeck (no reviews), 2014/01/24 13:18:02]

could EXPECT_TRUE(new_certificates_loaded_events_count == 0 || !initial_load)

[tbarzic, 2014/01/25 00:26:27]

Done.

+  }
+
+  // Checks if the number of |OnCertificatesLoaded| calls observed since the
+  // last call to this method equals |value|.
+  bool NewCertificatesLoadedEventsCountEquals(size_t value) {

[pneubeck (no reviews), 2014/01/24 13:18:02]

for better debugging should return the count. then EXPECT_EQ( , ) can be used.

[tbarzic, 2014/01/25 00:26:27]

Done.

+    bool result = value == new_certificates_loaded_events_count_;
+    new_certificates_loaded_events_count_ = 0;
+    return result;
+  }
+
+  // Finishes initialization for the |user| and returns a user's NSS database
+  // instance.
+  scoped_ptr<net::NSSCertDatabaseChromeOS> FinishUserInitAndGetDatabase(
+      crypto::ScopedTestNSSChromeOSUser* user) {
+    scoped_ptr<net::NSSCertDatabaseChromeOS> database;
+    if (!user->constructed_successfully())

[pneubeck (no reviews), 2014/01/24 13:18:02]

what happens in the early return cases? shouldn't they instead be ASSERTed ?

[tbarzic, 2014/01/25 00:26:27]

made it void and added ASSERTS

+      return database.Pass();
+
+    user->FinishInit();
+
+    crypto::ScopedPK11Slot private_slot(
+        crypto::GetPrivateSlotForChromeOSUser(
+            user->username_hash(),
+            base::Bind(&NotReachedPrivateSlotCallback)));
+    if (!private_slot)
+      return database.Pass();
+
+    database.reset(new net::NSSCertDatabaseChromeOS(
+        crypto::GetPublicSlotForChromeOSUser(user->username_hash()),
+        private_slot.Pass()));
+    return database.Pass();
+  }
+
+  int GetDbPrivateSlotId(net::NSSCertDatabase* db) {
+    return static_cast<int>(PK11_GetSlotID(db->GetPrivateSlot().get()));
+  }
+
+  bool ImportCACerts(const std::string& cert_file,

[pneubeck (no reviews), 2014/01/24 13:18:02]

only used to import a single CA cert, thus could
ASSERT_EQ(imported_certs->size(),1) at the end 

[tbarzic, 2014/01/25 00:26:27]

Done.

+                     net::NSSCertDatabase* database,
+                     net::CertificateList* imported_certs) {
+    if (!database || !imported_certs)

[pneubeck (no reviews), 2014/01/24 13:18:02]

this function can have return type void
and instead do all the ASSERTs itself (instead of at the calling site).
The error messages will be more helpful and the |success| variable below not
necessary.

[tbarzic, 2014/01/25 00:26:27]

Done.

+      return false;
+
+    // Add a certificate to the user's db.
+    *imported_certs = net::CreateCertificateListFromFile(
+        net::GetTestCertsDirectory(),
+        cert_file,
+        net::X509Certificate::FORMAT_AUTO);
+
+    net::NSSCertDatabase::ImportCertFailureList failed;
+    bool success = database->ImportCACerts(
+        *imported_certs, net::NSSCertDatabase::TRUSTED_SSL, &failed);

[pneubeck (no reviews), 2014/01/24 13:18:02]

why not use the default trust bits? CertLoader shouldn't depend on the actual
trust.

[tbarzic, 2014/01/25 00:26:27]

Done.

+    success = success && failed.empty();
+
+    if (!success)
+      imported_certs->clear();
+    return success;
+  }
+
+  bool ImportClientCertAndKey(const std::string& pkcs12_file,
+                              net::NSSCertDatabase* database,
+                              net::CertificateList* imported_certs) {
+    std::string pkcs12_data;

[pneubeck (no reviews), 2014/01/24 13:18:02]

same as for ImportCACerts. could be void(...) and check for errors directly.

[tbarzic, 2014/01/25 00:26:27]

Done.

+    base::FilePath pkcs12_file_path =
+        net::GetTestCertsDirectory().Append(pkcs12_file);
+    if (!base::ReadFileToString(pkcs12_file_path, &pkcs12_data))
+      return false;
+
+    net::CertificateList client_cert_list;
+    scoped_refptr<net::CryptoModule> module(net::CryptoModule::CreateFromHandle(
+        database->GetPrivateSlot().get()));
+    return net::OK ==
+        database->ImportFromPKCS12(module, pkcs12_data, base::string16(), false,
+                                   imported_certs);
+  }
+
+  CertLoader* cert_loader_;
+
+  // The user is primary as the one whose certificates CertLoader handles, it
+  // has nothing to do with crypto::InitializeNSSForChromeOSUser is_primary_user
+  // parameter (which is irrelevant for these tests).
+  crypto::ScopedTestNSSChromeOSUser primary_user_;
+  scoped_ptr<net::NSSCertDatabaseChromeOS> primary_db_;
+
+  base::MessageLoop message_loop_;
+
+ private:
+  size_t new_certificates_loaded_events_count_;
+};
+
+TEST_F(CertLoaderTest, Basic) {
+  EXPECT_FALSE(cert_loader_->CertificatesLoading());
+  EXPECT_FALSE(cert_loader_->certificates_loaded());
+  EXPECT_FALSE(cert_loader_->is_hardware_backed());
+
+  primary_db_ = FinishUserInitAndGetDatabase(&primary_user_);
+  ASSERT_TRUE(primary_db_);

[pneubeck (no reviews), 2014/01/24 13:18:02]

could be moved into FinishUserInitAndGetDatabase

[tbarzic, 2014/01/25 00:26:27]

Done.

+
+  cert_loader_->StartWithNSSDB(primary_db_.get());
+
+  EXPECT_FALSE(cert_loader_->certificates_loaded());
+  EXPECT_TRUE(cert_loader_->CertificatesLoading());
+  EXPECT_TRUE(cert_loader_->cert_list().empty());
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  EXPECT_TRUE(cert_loader_->certificates_loaded());
+  EXPECT_FALSE(cert_loader_->CertificatesLoading());
+  EXPECT_FALSE(cert_loader_->is_hardware_backed());
+  EXPECT_EQ(GetDbPrivateSlotId(primary_db_.get()),
+            cert_loader_->tpm_token_slot_id());
+
+  // Default CA cert roots should get loaded.
+  EXPECT_FALSE(cert_loader_->cert_list().empty());
+}
+
+TEST_F(CertLoaderTest, CertLoaderUpdatesCertListOnNewCert) {
+  primary_db_ = FinishUserInitAndGetDatabase(&primary_user_);
+  ASSERT_TRUE(primary_db_);
+
+  cert_loader_->StartWithNSSDB(primary_db_.get());
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));

[pneubeck (no reviews), 2014/01/24 13:18:02]

already tested in "Basic". same for the other tests.

[tbarzic, 2014/01/25 00:26:27]

Done.

+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  ASSERT_TRUE(cert_loader_->certificates_loaded());

[pneubeck (no reviews), 2014/01/24 13:18:02]

these two are covered in "Basic" as well.

[tbarzic, 2014/01/25 00:26:27]

Done.

+  ASSERT_FALSE(cert_loader_->cert_list().empty());
+
+  net::CertificateList certs;
+  ASSERT_TRUE(ImportCACerts("root_ca_cert.pem", primary_db_.get(), &certs));
+  ASSERT_EQ(1U, certs.size());
+
+  // Certs are loaded asynchronously, so the new cert should not yet be in the
+  // cert list.
+  EXPECT_FALSE(IsCertInCertificateList(certs[0], cert_loader_->cert_list()));
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  // The certificate list should be updated now, as the message loop's been run.
+  EXPECT_TRUE(IsCertInCertificateList(certs[0], cert_loader_->cert_list()));
+}
+
+TEST_F(CertLoaderTest, CertLoaderNoUpdateOnSecondaryDbChanges) {
+  primary_db_ = FinishUserInitAndGetDatabase(&primary_user_);
+  ASSERT_TRUE(primary_db_);
+  cert_loader_->StartWithNSSDB(primary_db_.get());
+
+  crypto::ScopedTestNSSChromeOSUser secondary_user("secondary");
+  scoped_ptr<net::NSSCertDatabaseChromeOS> secondary_db =
+      FinishUserInitAndGetDatabase(&secondary_user);
+  ASSERT_TRUE(secondary_db);
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  ASSERT_TRUE(cert_loader_->certificates_loaded());
+  ASSERT_FALSE(cert_loader_->cert_list().empty());
+
+  net::CertificateList certs;
+  ASSERT_TRUE(ImportCACerts("root_ca_cert.pem", secondary_db.get(), &certs));
+  ASSERT_EQ(1U, certs.size());
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));

[pneubeck (no reviews), 2014/01/24 13:18:02]

please comment why Importing a cert to the second db triggers an update of
CertLoader that is only initialized with the first db.
As this behavior doesn't seem to be relevant for the CertLoader's interface,
maybe just don't check the number of events here.

[tbarzic, 2014/01/25 00:26:27]

Removed.

+
+  EXPECT_FALSE(IsCertInCertificateList(certs[0], cert_loader_->cert_list()));
+}
+
+TEST_F(CertLoaderTest, ClientLoaderUpdateOnNewClientCert) {
+  primary_db_ = FinishUserInitAndGetDatabase(&primary_user_);
+  ASSERT_TRUE(primary_db_);
+  cert_loader_->StartWithNSSDB(primary_db_.get());
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  ASSERT_TRUE(cert_loader_->certificates_loaded());
+  ASSERT_FALSE(cert_loader_->cert_list().empty());

[pneubeck (no reviews), 2014/01/24 13:18:02]

I think it's unlucky that these initial part (lines 248-256) are just a
repetition of the basic test. Could you put these into a init function which is
called in each test case or derive a TestFixture that does just that?

[tbarzic, 2014/01/25 00:26:27]

Done.

+
+  net::CertificateList client_certs;
+  ASSERT_TRUE(ImportClientCertAndKey("websocket_client_cert.p12",
+                                     primary_db_.get(),
+                                     &client_certs));
+  ASSERT_EQ(1U, client_certs.size());
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  EXPECT_TRUE(IsCertInCertificateList(client_certs[0],
+                                      cert_loader_->cert_list()));
+}
+
+TEST_F(CertLoaderTest, CertLoaderNoUpdateOnNewClientCertInSecondaryDb) {
+  primary_db_ = FinishUserInitAndGetDatabase(&primary_user_);
+  ASSERT_TRUE(primary_db_);
+  cert_loader_->StartWithNSSDB(primary_db_.get());
+
+  crypto::ScopedTestNSSChromeOSUser secondary_user("secondary");
+  scoped_ptr<net::NSSCertDatabaseChromeOS> secondary_db =
+      FinishUserInitAndGetDatabase(&secondary_user);
+  ASSERT_TRUE(secondary_db);
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  ASSERT_TRUE(cert_loader_->certificates_loaded());
+  ASSERT_FALSE(cert_loader_->cert_list().empty());
+
+  net::CertificateList client_certs;
+  ASSERT_TRUE(ImportClientCertAndKey("websocket_client_cert.p12",
+                                     secondary_db.get(),
+                                     &client_certs));
+  ASSERT_EQ(1U, client_certs.size());
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));

[pneubeck (no reviews), 2014/01/24 13:18:02]

same comments as for CertLoaderNoUpdateOnSecondaryDbChanges

[tbarzic, 2014/01/25 00:26:27]

Done.

+
+  EXPECT_FALSE(
+      IsCertInCertificateList(client_certs[0], cert_loader_->cert_list()));
+}
+
+TEST_F(CertLoaderTest, UpdatedOnCertRemoval) {
+  primary_db_ = FinishUserInitAndGetDatabase(&primary_user_);
+  ASSERT_TRUE(primary_db_);
+  cert_loader_->StartWithNSSDB(primary_db_.get());
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  ASSERT_TRUE(cert_loader_->certificates_loaded());
+  ASSERT_FALSE(cert_loader_->cert_list().empty());
+
+  net::CertificateList client_certs;
+  ASSERT_TRUE(ImportClientCertAndKey("websocket_client_cert.p12",
+                                     primary_db_.get(),
+                                     &client_certs));
+  ASSERT_EQ(1U, client_certs.size());
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+  ASSERT_TRUE(IsCertInCertificateList(client_certs[0],
+                                      cert_loader_->cert_list()));
+
+  primary_db_->DeleteCertAndKey(client_certs[0]);
+
+  ASSERT_TRUE(NewCertificatesLoadedEventsCountEquals(0U));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(NewCertificatesLoadedEventsCountEquals(1U));
+
+  ASSERT_FALSE(IsCertInCertificateList(client_certs[0],
+                                       cert_loader_->cert_list()));
+}
+

[pneubeck (no reviews), 2014/01/24 13:18:02]

please consider adding one more test for trust change of a CA cert.

[tbarzic, 2014/01/25 00:26:27]

Done.

+}  // namespace
+}  // namespace chromeos