ClientCertResolver: Remove IsHardwareBacked check.

There were two methods: one to check whether the hardware backed crypto token is loaded at all and one to check whether an individual certificate is hardware backed. These were not consistently used by ClientCertResolver and the network configuration UI. Now always individual certificates are checked to be hardware backed and the IsHardwareBacked method is removed from CertLoader. For upcoming changes, it's also required that ClientCertResolver notifies observers even if no certificate patterns were resolved or no hardware token is present. Therefore, the check for IsHardwareBacked is removed. BUG=424036 Review URL: https://codereview.chromium.org/699073003 Cr-Commit-Position: refs/heads/master@{#303005}
hylschwy · Nov 6, 2014 · ad2f216 · ad2f216
1 parent 758abeb
commit ad2f216
Show file tree

Hide file tree

Showing 10 changed files with 30 additions and 51 deletions.
diff --git a/chrome/browser/chromeos/options/cert_library.cc b/chrome/browser/chromeos/options/cert_library.cc
@@ -145,10 +145,6 @@ bool CertLibrary::CertificatesLoaded() const {
   return CertLoader::Get()->certificates_loaded();
 }
 
-bool CertLibrary::IsHardwareBacked() const {
-  return CertLoader::Get()->IsHardwareBacked();
-}
-
 int CertLibrary::NumCertificates(CertType type) const {
   const net::CertificateList& cert_list = GetCertificateListForType(type);
   return static_cast<int>(cert_list.size());
@@ -172,7 +168,7 @@ std::string CertLibrary::GetUserCertPkcs11IdAt(int index, int* slot_id) const {
 
 bool CertLibrary::IsCertHardwareBackedAt(CertType type, int index) const {
   net::X509Certificate* cert = GetCertificateAt(type, index);
-  return CertLoader::Get()->IsCertificateHardwareBacked(cert);
+  return CertLoader::IsCertificateHardwareBacked(cert);
 }
 
 int CertLibrary::GetServerCACertIndexByPEM(

diff --git a/chrome/browser/chromeos/options/cert_library.h b/chrome/browser/chromeos/options/cert_library.h
@@ -60,9 +60,6 @@ class CertLibrary : public CertLoader::Observer {
   // Returns true when the certificate list has been initiailized.
   bool CertificatesLoaded() const;
 
-  // Returns true if the TPM is available for hardware-backed certificates.
-  bool IsHardwareBacked() const;
-
   // Retruns the number of certificates available for |type|.
   int NumCertificates(CertType type) const;
 

diff --git a/chrome/browser/chromeos/options/vpn_config_view.cc b/chrome/browser/chromeos/options/vpn_config_view.cc
@@ -1026,10 +1026,10 @@ bool VPNConfigView::IsUserCertValid() const {
   if (index < 0)
     return false;
   // Currently only hardware-backed user certificates are valid.
-  if (CertLibrary::Get()->IsHardwareBacked() &&
-      !CertLibrary::Get()->IsCertHardwareBackedAt(
-          CertLibrary::CERT_TYPE_USER, index))
+  if (!CertLibrary::Get()->IsCertHardwareBackedAt(CertLibrary::CERT_TYPE_USER,
+                                                  index)) {
     return false;
+  }
   return true;
 }
 

diff --git a/chrome/browser/chromeos/options/wifi_config_view.cc b/chrome/browser/chromeos/options/wifi_config_view.cc
@@ -441,10 +441,10 @@ bool WifiConfigView::IsUserCertValid() const {
   if (index < 0)
     return false;
   // Currently only hardware-backed user certificates are valid.
-  if (CertLibrary::Get()->IsHardwareBacked() &&
-      !CertLibrary::Get()->IsCertHardwareBackedAt(
-          CertLibrary::CERT_TYPE_USER, index))
+  if (!CertLibrary::Get()->IsCertHardwareBackedAt(CertLibrary::CERT_TYPE_USER,
+                                                  index)) {
     return false;
+  }
   return true;
 }
 

diff --git a/chromeos/cert_loader.cc b/chromeos/cert_loader.cc
@@ -21,6 +21,7 @@
 namespace chromeos {
 
 static CertLoader* g_cert_loader = NULL;
+static bool g_force_hardware_backed_for_test = false;
 
 // static
 void CertLoader::Initialize() {
@@ -51,7 +52,6 @@ CertLoader::CertLoader()
       certificates_update_required_(false),
       certificates_update_running_(false),
       database_(NULL),
-      force_hardware_backed_for_test_(false),
       cert_list_(new net::CertificateList),
       weak_factory_(this) {
 }
@@ -84,28 +84,23 @@ void CertLoader::RemoveObserver(CertLoader::Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-bool CertLoader::IsHardwareBacked() const {
-  if (force_hardware_backed_for_test_)
+// static
+bool CertLoader::IsCertificateHardwareBacked(const net::X509Certificate* cert) {
+  if (g_force_hardware_backed_for_test)
     return true;
-  if (!database_)
-    return false;
-  crypto::ScopedPK11Slot slot(database_->GetPrivateSlot());
-  if (!slot)
-    return false;
-  return PK11_IsHW(slot.get());
-}
-
-bool CertLoader::IsCertificateHardwareBacked(
-    const net::X509Certificate* cert) const {
-  if (!database_)
-    return false;
-  return database_->IsHardwareBacked(cert);
+  PK11SlotInfo* slot = cert->os_cert_handle()->slot;
+  return slot && PK11_IsHW(slot);
 }
 
 bool CertLoader::CertificatesLoading() const {
   return database_ && !certificates_loaded_;
 }
 
+// static
+void CertLoader::ForceHardwareBackedForTesting() {
+  g_force_hardware_backed_for_test = true;
+}
+
 // static
 //
 // For background see this discussion on dev-tech-crypto.lists.mozilla.org:

diff --git a/chromeos/cert_loader.h b/chromeos/cert_loader.h
@@ -74,12 +74,9 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   void AddObserver(CertLoader::Observer* observer);
   void RemoveObserver(CertLoader::Observer* observer);
 
-  bool IsHardwareBacked() const;
-
-  // Whether the certificate is hardware backed. Returns false if the CertLoader
-  // was not yet started (both |CertificatesLoading()| and
-  // |certificates_loaded()| are false).
-  bool IsCertificateHardwareBacked(const net::X509Certificate* cert) const;
+  // Returns true if |cert| is hardware backed. See also
+  // ForceHardwareBackedForTesting().
+  static bool IsCertificateHardwareBacked(const net::X509Certificate* cert);
 
   // Returns true when the certificate list has been requested but not loaded.
   bool CertificatesLoading() const;
@@ -89,9 +86,9 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   // This will be empty until certificates_loaded() is true.
   const net::CertificateList& cert_list() const { return *cert_list_; }
 
-  void force_hardware_backed_for_test() {
-    force_hardware_backed_for_test_ = true;
-  }
+  // Called in tests if |IsCertificateHardwareBacked()| should always return
+  // true.
+  static void ForceHardwareBackedForTesting();
 
  private:
   CertLoader();
@@ -122,9 +119,6 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   // should be loaded.
   net::NSSCertDatabase* database_;
 
-  // Set during tests if |IsHardwareBacked()| should always return true.
-  bool force_hardware_backed_for_test_;
-
   // Cached Certificates loaded from the database.
   scoped_ptr<net::CertificateList> cert_list_;
 

diff --git a/chromeos/cert_loader_unittest.cc b/chromeos/cert_loader_unittest.cc
@@ -178,7 +178,6 @@ class CertLoaderTest : public testing::Test,
 TEST_F(CertLoaderTest, Basic) {
   EXPECT_FALSE(cert_loader_->CertificatesLoading());
   EXPECT_FALSE(cert_loader_->certificates_loaded());
-  EXPECT_FALSE(cert_loader_->IsHardwareBacked());
 
   FinishUserInitAndGetDatabase(&primary_user_, &primary_db_);
 
@@ -217,6 +216,8 @@ TEST_F(CertLoaderTest, CertLoaderUpdatesCertListOnNewCert) {
   // The certificate list should be updated now, as the message loop's been run.
   EXPECT_TRUE(
       IsCertInCertificateList(certs[0].get(), cert_loader_->cert_list()));
+
+  EXPECT_FALSE(cert_loader_->IsCertificateHardwareBacked(certs[0].get()));
 }
 
 TEST_F(CertLoaderTest, CertLoaderNoUpdateOnSecondaryDbChanges) {

diff --git a/chromeos/network/client_cert_resolver.cc b/chromeos/network/client_cert_resolver.cc
@@ -138,7 +138,8 @@ std::vector<CertAndIssuer> CreateSortedCertAndIssuerList(
        it != certs.end(); ++it) {
     const net::X509Certificate& cert = **it;
     if (cert.valid_expiry().is_null() || cert.HasExpired() ||
-        !HasPrivateKey(cert)) {
+        !HasPrivateKey(cert) ||
+        !CertLoader::IsCertificateHardwareBacked(&cert)) {
       continue;
     }
     net::ScopedCERTCertificate issuer_handle(
@@ -221,10 +222,6 @@ bool ClientCertificatesLoaded() {
     VLOG(1) << "Certificates not loaded yet.";
     return false;
   }
-  if (!CertLoader::Get()->IsHardwareBacked()) {
-    VLOG(1) << "TPM is not available.";
-    return false;
-  }
   return true;
 }
 

diff --git a/chromeos/network/client_cert_resolver_unittest.cc b/chromeos/network/client_cert_resolver_unittest.cc
@@ -78,7 +78,7 @@ class ClientCertResolverTest : public testing::Test,
 
     CertLoader::Initialize();
     cert_loader_ = CertLoader::Get();
-    cert_loader_->force_hardware_backed_for_test();
+    CertLoader::ForceHardwareBackedForTesting();
   }
 
   void TearDown() override {

diff --git a/chromeos/network/network_connection_handler_unittest.cc b/chromeos/network/network_connection_handler_unittest.cc
@@ -74,8 +74,7 @@ class NetworkConnectionHandlerTest : public testing::Test {
     test_nssdb_->SetSlowTaskRunnerForTest(message_loop_.message_loop_proxy());
 
     CertLoader::Initialize();
-    CertLoader* cert_loader = CertLoader::Get();
-    cert_loader->force_hardware_backed_for_test();
+    CertLoader::ForceHardwareBackedForTesting();
 
     DBusThreadManager::Initialize();
     DBusThreadManager* dbus_manager = DBusThreadManager::Get();