Use user specific NSSDatabase in CertLoader.

chromeos/cert_loader.cc

 // Copyright (c) 2013 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 <algorithm>
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/threading/worker_pool.h"
 #include "crypto/nss_util.h"
 #include "net/cert/nss_cert_database.h"
+#include "net/cert/nss_cert_database_chromeos.h"
 #include "net/cert/x509_certificate.h"
 
 namespace chromeos {
 
 namespace {
 
 // Loads certificates from |cert_database| into |cert_list|.
 void LoadNSSCertificates(net::NSSCertDatabase* cert_database,
                          net::CertificateList* cert_list) {
   cert_database->ListCerts(cert_list);
@@ skipping 25 matching lines @@
 // static
 bool CertLoader::IsInitialized() {
   return g_cert_loader;
 }
 
 CertLoader::CertLoader()
     : certificates_requested_(false),
       certificates_loaded_(false),
       certificates_update_required_(false),
       certificates_update_running_(false),
+      database_(NULL),
       tpm_token_slot_id_(-1),
+      is_hardware_backed_(false),
+      hardware_backed_for_test_(false),
       weak_factory_(this) {
-  if (TPMTokenLoader::IsInitialized())
-    TPMTokenLoader::Get()->AddObserver(this);
-}
-
-void CertLoader::SetSlowTaskRunnerForTest(
-    const scoped_refptr<base::TaskRunner>& task_runner) {
-  slow_task_runner_for_test_ = task_runner;
 }
 
 CertLoader::~CertLoader() {
   net::CertDatabase::GetInstance()->RemoveObserver(this);
-  if (TPMTokenLoader::IsInitialized())
-    TPMTokenLoader::Get()->RemoveObserver(this);
-}
-
-void CertLoader::AddObserver(CertLoader::Observer* observer) {
-  observers_.AddObserver(observer);
-}
-
-void CertLoader::RemoveObserver(CertLoader::Observer* observer) {
-  observers_.RemoveObserver(observer);
-}
-
-bool CertLoader::IsHardwareBacked() const {
-  return !tpm_token_name_.empty();
-}
-
-bool CertLoader::CertificatesLoading() const {
-  return certificates_requested_ && !certificates_loaded_;
 }
 
 // This is copied from chrome/common/net/x509_certificate_model_nss.cc.
 // For background see this discussion on dev-tech-crypto.lists.mozilla.org:
 // http://web.archiveorange.com/archive/v/6JJW7E40sypfZGtbkzxX
 //
 // NOTE: This function relies on the convention that the same PKCS#11 ID
 // is shared between a certificate and its associated private and public
 // keys.  I tried to implement this with PK11_GetLowLevelKeyIDForCert(),
 // but that always returns NULL on Chrome OS for me.
@@ skipping 11 matching lines @@
   std::string pkcs11_id;
   if (sec_item) {
     pkcs11_id = base::HexEncode(sec_item->data, sec_item->len);
     SECITEM_FreeItem(sec_item, PR_TRUE);
   }
   SECKEY_DestroyPrivateKey(priv_key);
 
   return pkcs11_id;
 }
 
-void CertLoader::RequestCertificates() {
+void CertLoader::SetSlowTaskRunnerForTest(

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

nit:
If you already reorder the definitions, could you match it with the declaration
order?
Although it would be preferable to save all reordering for a separate CL.

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

Done.

+    const scoped_refptr<base::TaskRunner>& task_runner) {
+  slow_task_runner_for_test_ = task_runner;
+}
+
+void CertLoader::AddObserver(CertLoader::Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void CertLoader::RemoveObserver(CertLoader::Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+bool CertLoader::IsCertificateHardwareBacked(
+    const net::X509Certificate* cert) const {
+  if (!database_ || !cert)

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

DCHECK(cert) (or let it crash in IsHardwareBacked below) instead of return false
?

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

Done.

+    return false;
+  return database_->IsHardwareBacked(cert);
+}
+
+bool CertLoader::CertificatesLoading() const {
+  return certificates_requested_ && !certificates_loaded_;
+}
+
+void CertLoader::StartWithNSSDB(net::NSSCertDatabase* database) {
   if (certificates_requested_)

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

this shouldn't be called more than once anymore, right?
then it should return silently and instead only CHECK(!database_);

all other occurrences of certificates_requested_ can be replaced by database_
and the former be removed.

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

Done.

     return;
   certificates_requested_ = true;
 
-  DCHECK(!certificates_loaded_ && !certificates_update_running_);
+  CHECK(database);

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

nit: not necessary, will crash anyways below.

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

Done.

+  CHECK(!database_);
+
+  database_ = database;
+  tpm_token_slot_id_ =

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

Has to be cached? Why not calculate every time?

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

Done.

+      static_cast<int>(PK11_GetSlotID(database_->GetPrivateSlot().get()));
+
+  is_hardware_backed_ = hardware_backed_for_test_ ||

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

hardware_backed_for_test_ can only switch is_hardware_backed_ from false to
true.
Maybe rename it to "force_hardware_backed_for_test_" ?

Does this value even have to be cached? Why not calculate every time?

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

Done.

+                        PK11_IsHW(database_->GetPrivateSlot().get());
+
+  // Start observing cert database for changes.
   net::CertDatabase::GetInstance()->AddObserver(this);
+
   LoadCertificates();
 }
 
 void CertLoader::LoadCertificates() {
   CHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "LoadCertificates: " << certificates_update_running_;
 
   if (certificates_update_running_) {
     certificates_update_required_ = true;
     return;
   }
 
   net::CertificateList* cert_list = new net::CertificateList;
   certificates_update_running_ = true;
   certificates_update_required_ = false;
 
   base::TaskRunner* task_runner = slow_task_runner_for_test_.get();
   if (!task_runner)
     task_runner = base::WorkerPool::GetTaskRunner(true /* task is slow */);
   task_runner->PostTaskAndReply(
       FROM_HERE,
       base::Bind(LoadNSSCertificates,
-                 net::NSSCertDatabase::GetInstance(),
+                 // Create a copy of the database so it can be used on the
+                 // worker pool.

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

if you plan to change this, please add a TODO

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

Done.

+                 base::Owned(new net::NSSCertDatabaseChromeOS(
+                     database_->GetPublicSlot(),
+                     database_->GetPrivateSlot())),
                  cert_list),
       base::Bind(&CertLoader::UpdateCertificates,
                  weak_factory_.GetWeakPtr(),
                  base::Owned(cert_list)));
 }
 
 void CertLoader::UpdateCertificates(net::CertificateList* cert_list) {
   CHECK(thread_checker_.CalledOnValidThread());
   DCHECK(certificates_update_running_);
   VLOG(1) << "UpdateCertificates: " << cert_list->size();
@@ skipping 25 matching lines @@
   // This is triggered when a client certificate is added.
   VLOG(1) << "OnCertAdded";
   LoadCertificates();
 }
 
 void CertLoader::OnCertRemoved(const net::X509Certificate* cert) {
   VLOG(1) << "OnCertRemoved";
   LoadCertificates();
 }
 
-void CertLoader::OnTPMTokenReady(const std::string& tpm_user_pin,
-                                 const std::string& tpm_token_name,
-                                 int tpm_token_slot_id) {
-  tpm_user_pin_ = tpm_user_pin;
-  tpm_token_name_ = tpm_token_name;
-  tpm_token_slot_id_ = tpm_token_slot_id;
-
-  VLOG(1) << "TPM token ready.";
-  RequestCertificates();
-}
-
 }  // namespace chromeos