Separate cert loading code from CertLibrary and move to src/chromeos

chromeos/network/cert_loader.cc

Index: chromeos/network/cert_loader.cc
diff --git a/chromeos/network/cert_loader.cc b/chromeos/network/cert_loader.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3613c2913b2d5d2c2f394297f3def8d325c6a2a2
--- /dev/null
+++ b/chromeos/network/cert_loader.cc
@@ -0,0 +1,253 @@
+// 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/network/cert_loader.h"
+
+#include <algorithm>
+
+#include "base/chromeos/chromeos_version.h"
+#include "base/observer_list_threadsafe.h"
+#include "base/task_runner_util.h"
+#include "base/threading/worker_pool.h"
+#include "chromeos/dbus/cryptohome_client.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "crypto/encryptor.h"
+#include "crypto/nss_util.h"
+#include "crypto/sha2.h"
+#include "crypto/symmetric_key.h"
+#include "net/cert/nss_cert_database.h"
+
+namespace chromeos {
+
+namespace {
+
+// Delay between certificate requests while waiting for TPM/PKCS#11 init.
+const int kRequestDelayMs = 500;
+
+net::CertificateList* LoadNSSCertificates() {
+  net::CertificateList* cert_list(new net::CertificateList());
+  net::NSSCertDatabase::GetInstance()->ListCerts(cert_list);
+  return cert_list;
+}
+
+}  // namespace
+
+static CertLoader* g_cert_loader = NULL;
+
+// static
+void CertLoader::Initialize() {
+  CHECK(!g_cert_loader);
+  g_cert_loader = new CertLoader();
+}
+
+// static
+void CertLoader::Shutdown() {
+  CHECK(g_cert_loader);
+  delete g_cert_loader;
+  g_cert_loader = NULL;
+}
+
+// static
+CertLoader* CertLoader::Get() {
+  CHECK(g_cert_loader) << "CertLoader::Get() called before Initialize()";
+  return g_cert_loader;
+}
+
+CertLoader::CertLoader()
+    : observer_list_(new ObserverList),
+      tpm_token_ready_(false),
+      certificates_requested_(false),
+      certificates_loaded_(false),
+      key_store_loaded_(false),
+      weak_ptr_factory_(this) {
+  net::CertDatabase::GetInstance()->AddObserver(this);
+  LoginState::Get()->AddObserver(this);
+  if (LoginState::Get()->IsUserLoggedIn())
+    RequestCertificates();
+}
+
+CertLoader::~CertLoader() {
+  DCHECK(request_task_.is_null());
+  net::CertDatabase::GetInstance()->RemoveObserver(this);
+  LoginState::Get()->RemoveObserver(this);
+}
+
+void CertLoader::AddObserver(CertLoader::Observer* observer) {
+  observer_list_->AddObserver(observer);
+}
+
+void CertLoader::RemoveObserver(CertLoader::Observer* observer) {
+  observer_list_->RemoveObserver(observer);
+}
+
+bool CertLoader::RequestCertificates() {
+  CHECK(thread_checker_.CalledOnValidThread());
+  VLOG(1) << "RequestCertificates: " << LoginState::Get()->IsUserLoggedIn();

[pneubeck (no reviews), 2013/05/02 14:58:35]

nit: clarify VLOG to explain the printed value.
E.g.
here:  VLOG << "RequestCertificates"
within the following if:
  VLOG << "Not requesting certificates as no user is logged in."
remove the other VLOG below.

[stevenjb, 2013/05/03 01:00:31]

I don't feel that would really be more helpful for debugging.

+  if (!LoginState::Get()->IsUserLoggedIn())
+    return false;
+
+  if (!key_store_loaded_) {
+    // Ensure we've opened the real user's key/certificate database.
+    crypto::OpenPersistentNSSDB();
+
+    if (base::chromeos::IsRunningOnChromeOS())
+      crypto::EnableTPMTokenForNSS();
+
+    key_store_loaded_ = true;
+  }
+
+  certificates_requested_ = true;
+
+  VLOG(1) << "Requesting Certificates.";
+  DBusThreadManager::Get()->GetCryptohomeClient()->TpmIsEnabled(
+      base::Bind(&CertLoader::OnTpmIsEnabled,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  return true;
+}
+
+bool CertLoader::CertificatesLoading() const {
+  return certificates_requested_ && !certificates_loaded_;
+}
+
+bool CertLoader::IsHardwareBacked() const {
+  return !tpm_token_name_.empty();
+}
+
+// private methods

[pneubeck (no reviews), 2013/05/02 14:58:35]

nit: not helpful?

[stevenjb, 2013/05/03 01:00:31]

Done.

+
+void CertLoader::OnTpmIsEnabled(DBusMethodCallStatus call_status,
+                                bool tpm_is_enabled) {
+  VLOG(1) << "OnTpmIsEnabled: " << tpm_is_enabled;
+  if (call_status != DBUS_METHOD_CALL_SUCCESS || !tpm_is_enabled) {
+    // TPM is not enabled, so proceed with empty tpm token name.
+    VLOG(1) << "TPM not available.";
+    StartLoadCertificates();
+  } else if (tpm_token_ready_) {

[pneubeck (no reviews), 2013/05/02 14:58:35]

verbally this doesn't make any sense
.. if it's ready, initialize it ??

better rename the variable to "tpm_token_info_loaded_" and add a comment that
loading is skipped.

[stevenjb, 2013/05/03 01:00:31]

Um, yes? The TPM needs to be initialized, once it is ready. Added a comment.

[pneubeck (no reviews), 2013/05/03 09:42:54]

"ready" sounds to me more like the initialization is finished. If "ready" is
before initialization, how would you call the state afterwards.
But it's a nit...

+    InitializeTPMToken();
+  } else {
+    DBusThreadManager::Get()->GetCryptohomeClient()->Pkcs11IsTpmTokenReady(
+        base::Bind(&CertLoader::OnPkcs11IsTpmTokenReady,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void CertLoader::OnPkcs11IsTpmTokenReady(DBusMethodCallStatus call_status,
+                                         bool is_tpm_token_ready) {
+  VLOG(1) << "OnPkcs11IsTpmTokenReady: " << is_tpm_token_ready;
+  if (call_status != DBUS_METHOD_CALL_SUCCESS || !is_tpm_token_ready) {
+    MaybeRetryRequestCertificates();
+    return;
+  }
+
+  // Retrieve token_name_ and user_pin_ here since they will never change
+  // and CryptohomeClient calls are not thread safe.
+  DBusThreadManager::Get()->GetCryptohomeClient()->Pkcs11GetTpmTokenInfo(
+      base::Bind(&CertLoader::OnPkcs11GetTpmTokenInfo,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void CertLoader::OnPkcs11GetTpmTokenInfo(DBusMethodCallStatus call_status,
+                                         const std::string& token_name,
+                                         const std::string& user_pin) {
+  VLOG(1) << "OnPkcs11GetTpmTokenInfo: " << token_name;
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    MaybeRetryRequestCertificates();
+    return;
+  }
+  tpm_token_name_ = token_name;
+  tpm_user_pin_ = user_pin;
+  tpm_token_ready_ = true;
+
+  InitializeTPMToken();
+}
+
+void CertLoader::InitializeTPMToken() {

[pneubeck (no reviews), 2013/05/02 14:58:35]

maybe rename to "InitializeNSSForTPM".

+  VLOG(1) << "InitializeTPMToken";
+  if (!crypto::InitializeTPMToken(tpm_token_name_, tpm_user_pin_)) {
+    MaybeRetryRequestCertificates();
+    return;
+  }
+  StartLoadCertificates();
+}
+
+void CertLoader::StartLoadCertificates() {
+  VLOG(1) << "Start Load Certificates";
+  base::PostTaskAndReplyWithResult(

[pneubeck (no reviews), 2013/05/02 14:58:35]

Not that it's really problematic, but I'd guess that the certificate list can
leak during shut-down.

The pattern shown in task_runner_util.h is different, it returns a value (not a
pointer).

I would instead let the second closure own the CertList (like the implementation
of PostTaskAndReplyWithResult):

CertList* result = new CertList();
...GetTaskRunner(true)->PostTaskAndReply(
   FROM_HERE,
   base::Bind(LoadNssCerts, result),
   base::Bind(&CertLoader::UpdateCerts, weak_ptr_..., base::Owned(result)));

[stevenjb, 2013/05/03 01:00:31]

Not sure I follow right now, but seems like a change that will require a careful
re-factor and testing.

+      base::WorkerPool::GetTaskRunner(true /* task_is_slow */),
+      FROM_HERE,
+      base::Bind(LoadNSSCertificates),
+      base::Bind(&CertLoader::UpdateCertificates,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void CertLoader::UpdateCertificates(net::CertificateList* cert_list) {
+  CHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(cert_list);
+  VLOG(1) << "Update Certificates: " << cert_list->size();

[pneubeck (no reviews), 2013/05/02 14:58:35]

nit: make vlog self explanatory. "Number of certs: "

[stevenjb, 2013/05/03 01:00:31]

Again, doesn't seem helpful to me, will leave for the next owner of this code...

+
+  // Clear any existing certificates.
+  cert_list_ = *cert_list;  // Copy vector

[pneubeck (no reviews), 2013/05/02 14:58:35]

nit: why not swap?

[stevenjb, 2013/05/03 01:00:31]

Done.

+
+  // cert_list is constructed in LoadCertificates.
+  delete cert_list;
+
+  // Set loaded state and notify observers.
+  if (!certificates_loaded_) {
+    certificates_loaded_ = true;
+    NotifyCertificatesLoaded(true);
+  } else {
+    NotifyCertificatesLoaded(false);
+  }
+}
+
+void CertLoader::MaybeRetryRequestCertificates() {

[pneubeck (no reviews), 2013/05/02 14:58:35]

Rename to RetryRequestCertificates

+  if (!request_task_.is_null())

[pneubeck (no reviews), 2013/05/02 14:58:35]

I don't see the gain of storing the closure request_task_ instead of a simple
boolean, like for the other "local state".
On the other hand, it's rather disturbing.

If I didn't miss something, this conditional is never fulfilled.

[stevenjb, 2013/05/03 01:00:31]

TODO: Refactor

+    return;
+
+  // Cryptohome does not notify us when the token is ready, so call
+  // this again after a delay.
+  request_task_ = base::Bind(&CertLoader::RequestCertificatesTask,
+                             weak_ptr_factory_.GetWeakPtr());
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      request_task_,
+      base::TimeDelta::FromMilliseconds(kRequestDelayMs));
+}
+
+void CertLoader::RequestCertificatesTask() {
+  // Reset the task to the initial state so is_null() returns true.
+  request_task_.Reset();
+  RequestCertificates();
+}
+
+void CertLoader::NotifyCertificatesLoaded(bool initial_load) {
+  observer_list_->Notify(
+      &CertLoader::Observer::OnCertificatesLoaded, cert_list_, initial_load);
+}
+
+void CertLoader::OnCertTrustChanged(const net::X509Certificate* cert) {
+}
+
+void CertLoader::OnCertAdded(const net::X509Certificate* cert) {
+  // Only load certificates if we have completed an initial request.
+  if (!certificates_loaded_)
+    return;
+  StartLoadCertificates();

[pneubeck (no reviews), 2013/05/02 14:58:35]

There is a race:

if LoadNSSCertificates() is currently running while a new cert is added
(removed), this call will be ignored and we miss the new cert (or the removal).

I'd either use a boolean certificate_reloading_required_ to queue another call
to StartLoadCertificates()
and use either the proposed WeakPtrFactory or yet another boolean
certificate_loading_ to check if  LoadNSSCertificates() is currenlty running.

[stevenjb, 2013/05/03 01:00:31]

TODO: Refactor

+}
+
+void CertLoader::OnCertRemoved(const net::X509Certificate* cert) {
+  // Only load certificates if we have completed an initial request.
+  if (!certificates_loaded_)
+    return;
+  StartLoadCertificates();
+}
+
+void CertLoader::LoggedInStateChanged(LoginState::LoggedInState state) {
+  VLOG(1) << "LoggedInStateChanged: " << state;
+  if (LoginState::Get()->IsUserLoggedIn() && !certificates_requested_)

[pneubeck (no reviews), 2013/05/02 14:58:35]

the conditional !certificates_requested_ shouldn't be here, but in a more
generally named function like MaybeRequestCertificates which wraps the
RequestCertificates function.

[stevenjb, 2013/05/03 01:00:31]

TODO:Refactor

+    RequestCertificates();
+}
+
+}  // namespace chromeos