Use correct slot id for client certs in network config.

- Now all client cert related Shill properties are set through client_cert_util.*
- Slot id is obtained from the slot in which the private key is stored
- All client cert pattern resolution now happens in ClientCertResolver

BUG=358366

Review URL: https://codereview.chromium.org/421113002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@286583 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 225 insertions, 301 deletions

diff --git a/chromeos/cert_loader.cc b/chromeos/cert_loader.cc
index 8c222795..337c70e 100644
--- a/chromeos/cert_loader.cc
+++ b/chromeos/cert_loader.cc
@@ -84,14 +84,6 @@ void CertLoader::RemoveObserver(CertLoader::Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-int CertLoader::TPMTokenSlotID() const {
-  if (!database_)
-    return -1;
-  crypto::ScopedPK11Slot slot(database_->GetPrivateSlot());
-  DCHECK(slot);
-  return static_cast<int>(PK11_GetSlotID(slot.get()));
-}
-
 bool CertLoader::IsHardwareBacked() const {
   if (force_hardware_backed_for_test_)
     return true;
@@ -122,13 +114,19 @@ bool CertLoader::CertificatesLoading() const {
 // 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.
-std::string CertLoader::GetPkcs11IdForCert(const net::X509Certificate& cert) {
+std::string CertLoader::GetPkcs11IdAndSlotForCert(
+    const net::X509Certificate& cert,
+    int* slot_id) {
+  DCHECK(slot_id);
+
   CERTCertificateStr* cert_handle = cert.os_cert_handle();
   SECKEYPrivateKey *priv_key =
       PK11_FindKeyByAnyCert(cert_handle, NULL /* wincx */);
   if (!priv_key)
     return std::string();
 
+  *slot_id = static_cast<int>(PK11_GetSlotID(priv_key->pkcs11Slot));
+
   // Get the CKA_ID attribute for a key.
   SECItem* sec_item = PK11_GetLowLevelKeyIDForPrivateKey(priv_key);
   std::string pkcs11_id;
diff --git a/chromeos/cert_loader.h b/chromeos/cert_loader.h
index 8968b3c..59342bc 100644
--- a/chromeos/cert_loader.h
+++ b/chromeos/cert_loader.h
@@ -58,13 +58,11 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   static bool IsInitialized();
 
   // Returns the PKCS#11 attribute CKA_ID for a certificate as an upper-case
-  // hex string, or the empty string if none is found. Note that the returned ID
-  // should be used only to identify the cert in its slot.
-  // This should be used only for user certificates, assuming that only one
-  // private slot is loaded for a user.
-  // TODO(tbarzic): Make this check cert slot id if we start loading
-  // certificates for secondary users.
-  static std::string GetPkcs11IdForCert(const net::X509Certificate& cert);
+  // hex string and sets |slot_id| to the id of the containing slot, or returns
+  // an empty string and doesn't modify |slot_id| if the PKCS#11 id could not be
+  // determined.
+  static std::string GetPkcs11IdAndSlotForCert(const net::X509Certificate& cert,
+                                               int* slot_id);
 
   // Starts the CertLoader with the NSS cert database.
   // The CertLoader will _not_ take the ownership of the database, but it
@@ -76,7 +74,6 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   void AddObserver(CertLoader::Observer* observer);
   void RemoveObserver(CertLoader::Observer* observer);
 
-  int TPMTokenSlotID() const;
   bool IsHardwareBacked() const;
 
   // Whether the certificate is hardware backed. Returns false if the CertLoader
diff --git a/chromeos/cert_loader_unittest.cc b/chromeos/cert_loader_unittest.cc
index a7efa21..19765af 100644
--- a/chromeos/cert_loader_unittest.cc
+++ b/chromeos/cert_loader_unittest.cc
@@ -189,8 +189,6 @@ TEST_F(CertLoaderTest, Basic) {
 
   EXPECT_TRUE(cert_loader_->certificates_loaded());
   EXPECT_FALSE(cert_loader_->CertificatesLoading());
-  EXPECT_EQ(GetDbPrivateSlotId(primary_db_.get()),
-            cert_loader_->TPMTokenSlotID());
 
   // Default CA cert roots should get loaded.
   EXPECT_FALSE(cert_loader_->cert_list().empty());
diff --git a/chromeos/network/client_cert_resolver.cc b/chromeos/network/client_cert_resolver.cc
index 20de5f82..d345317 100644
--- a/chromeos/network/client_cert_resolver.cc
+++ b/chromeos/network/client_cert_resolver.cc
@@ -14,18 +14,14 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/stl_util.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/task_runner.h"
 #include "base/threading/worker_pool.h"
 #include "base/time/time.h"
 #include "chromeos/cert_loader.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_service_client.h"
-#include "chromeos/network/certificate_pattern.h"
-#include "chromeos/network/client_cert_util.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_state.h"
-#include "chromeos/tpm_token_loader.h"
 #include "components/onc/onc_constants.h"
 #include "dbus/object_path.h"
 #include "net/cert/scoped_nss_types.h"
@@ -38,16 +34,21 @@ namespace chromeos {
 struct ClientCertResolver::NetworkAndMatchingCert {
   NetworkAndMatchingCert(const std::string& network_path,
                          client_cert::ConfigType config_type,
-                         const std::string& cert_id)
+                         const std::string& cert_id,
+                         int slot_id)
       : service_path(network_path),
         cert_config_type(config_type),
-        pkcs11_id(cert_id) {}
+        pkcs11_id(cert_id),
+        key_slot_id(slot_id) {}
 
   std::string service_path;
   client_cert::ConfigType cert_config_type;
 
   // The id of the matching certificate or empty if no certificate was found.
   std::string pkcs11_id;
+
+  // The id of the slot containing the certificate and the private key.
+  int key_slot_id;
 };
 
 typedef std::vector<ClientCertResolver::NetworkAndMatchingCert>
@@ -100,13 +101,12 @@ struct NetworkAndCertPattern {
 };
 
 // A unary predicate that returns true if the given CertAndIssuer matches the
-// certificate pattern of the NetworkAndCertPattern.
+// given certificate pattern.
 struct MatchCertWithPattern {
-  explicit MatchCertWithPattern(const NetworkAndCertPattern& pattern)
-      : net_and_pattern(pattern) {}
+  explicit MatchCertWithPattern(const CertificatePattern& cert_pattern)
+      : pattern(cert_pattern) {}
 
   bool operator()(const CertAndIssuer& cert_and_issuer) {
-    const CertificatePattern& pattern = net_and_pattern.cert_config.pattern;
     if (!pattern.issuer().Empty() &&
         !client_cert::CertPrincipalMatches(pattern.issuer(),
                                            cert_and_issuer.cert->issuer())) {
@@ -126,15 +126,11 @@ struct MatchCertWithPattern {
     return true;
   }
 
-  NetworkAndCertPattern net_and_pattern;
+  const CertificatePattern pattern;
 };
 
-// Searches for matches between |networks| and |certs| and writes matches to
-// |matches|. Because this calls NSS functions and is potentially slow, it must
-// be run on a worker thread.
-void FindCertificateMatches(const net::CertificateList& certs,
-                            std::vector<NetworkAndCertPattern>* networks,
-                            NetworkCertMatches* matches) {
+std::vector<CertAndIssuer> CreateSortedCertAndIssuerList(
+    const net::CertificateList& certs) {
   // Filter all client certs and determines each certificate's issuer, which is
   // required for the pattern matching.
   std::vector<CertAndIssuer> client_certs;
@@ -169,20 +165,34 @@ void FindCertificateMatches(const net::CertificateList& certs,
   }
 
   std::sort(client_certs.begin(), client_certs.end(), &CompareCertExpiration);
+  return client_certs;
+}
+
+// Searches for matches between |networks| and |certs| and writes matches to
+// |matches|. Because this calls NSS functions and is potentially slow, it must
+// be run on a worker thread.
+void FindCertificateMatches(const net::CertificateList& certs,
+                            std::vector<NetworkAndCertPattern>* networks,
+                            NetworkCertMatches* matches) {
+  std::vector<CertAndIssuer> client_certs(CreateSortedCertAndIssuerList(certs));
 
   for (std::vector<NetworkAndCertPattern>::const_iterator it =
            networks->begin();
        it != networks->end(); ++it) {
-    std::vector<CertAndIssuer>::iterator cert_it = std::find_if(
-        client_certs.begin(), client_certs.end(), MatchCertWithPattern(*it));
+    std::vector<CertAndIssuer>::iterator cert_it =
+        std::find_if(client_certs.begin(),
+                     client_certs.end(),
+                     MatchCertWithPattern(it->cert_config.pattern));
     std::string pkcs11_id;
+    int slot_id = -1;
     if (cert_it == client_certs.end()) {
       VLOG(1) << "Couldn't find a matching client cert for network "
               << it->service_path;
-      // Leave |pkcs11_id empty| to indicate that no cert was found for this
+      // Leave |pkcs11_id| empty to indicate that no cert was found for this
       // network.
     } else {
-      pkcs11_id = CertLoader::GetPkcs11IdForCert(*cert_it->cert);
+      pkcs11_id =
+          CertLoader::GetPkcs11IdAndSlotForCert(*cert_it->cert, &slot_id);
       if (pkcs11_id.empty()) {
         LOG(ERROR) << "Couldn't determine PKCS#11 ID.";
         // So far this error is not expected to happen. We can just continue, in
@@ -191,7 +201,7 @@ void FindCertificateMatches(const net::CertificateList& certs,
       }
     }
     matches->push_back(ClientCertResolver::NetworkAndMatchingCert(
-        it->service_path, it->cert_config.location, pkcs11_id));
+        it->service_path, it->cert_config.location, pkcs11_id, slot_id));
   }
 }
 
@@ -254,6 +264,39 @@ void ClientCertResolver::SetSlowTaskRunnerForTest(
   slow_task_runner_for_test_ = task_runner;
 }
 
+// static
+bool ClientCertResolver::ResolveCertificatePatternSync(
+    const client_cert::ConfigType client_cert_type,
+    const CertificatePattern& pattern,
+    base::DictionaryValue* shill_properties) {
+  // Prepare and sort the list of known client certs.
+  std::vector<CertAndIssuer> client_certs(
+      CreateSortedCertAndIssuerList(CertLoader::Get()->cert_list()));
+
+  // Search for a certificate matching the pattern.
+  std::vector<CertAndIssuer>::iterator cert_it = std::find_if(
+      client_certs.begin(), client_certs.end(), MatchCertWithPattern(pattern));
+
+  if (cert_it == client_certs.end()) {
+    VLOG(1) << "Couldn't find a matching client cert";
+    client_cert::SetEmptyShillProperties(client_cert_type, shill_properties);
+    return false;
+  }
+
+  int slot_id = -1;
+  std::string pkcs11_id =
+      CertLoader::GetPkcs11IdAndSlotForCert(*cert_it->cert, &slot_id);
+  if (pkcs11_id.empty()) {
+    LOG(ERROR) << "Couldn't determine PKCS#11 ID.";
+    // So far this error is not expected to happen. We can just continue, in
+    // the worst case the user can remove the problematic cert.
+    return false;
+  }
+  client_cert::SetShillProperties(
+      client_cert_type, slot_id, pkcs11_id, shill_properties);
+  return true;
+}
+
 void ClientCertResolver::NetworkListChanged() {
   VLOG(2) << "NetworkListChanged.";
   if (!ClientCertificatesLoaded())
@@ -385,16 +428,16 @@ void ClientCertResolver::ConfigureCertificates(NetworkCertMatches* matches) {
   for (NetworkCertMatches::const_iterator it = matches->begin();
        it != matches->end(); ++it) {
     VLOG(1) << "Configuring certificate of network " << it->service_path;
-    CertLoader* cert_loader = CertLoader::Get();
     base::DictionaryValue shill_properties;
-    // If pkcs11_id is empty, this will clear the key/cert properties of this
-    // network.
-    client_cert::SetShillProperties(
-        it->cert_config_type,
-        base::IntToString(cert_loader->TPMTokenSlotID()),
-        TPMTokenLoader::Get()->tpm_user_pin(),
-        &it->pkcs11_id,
-        &shill_properties);
+    if (it->pkcs11_id.empty()) {
+      client_cert::SetEmptyShillProperties(it->cert_config_type,
+                                           &shill_properties);
+    } else {
+      client_cert::SetShillProperties(it->cert_config_type,
+                                      it->key_slot_id,
+                                      it->pkcs11_id,
+                                      &shill_properties);
+    }
     DBusThreadManager::Get()->GetShillServiceClient()->
         SetProperties(dbus::ObjectPath(it->service_path),
                         shill_properties,
diff --git a/chromeos/network/client_cert_resolver.h b/chromeos/network/client_cert_resolver.h
index 9e70a57..b8ded78 100644
--- a/chromeos/network/client_cert_resolver.h
+++ b/chromeos/network/client_cert_resolver.h
@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "chromeos/cert_loader.h"
 #include "chromeos/chromeos_export.h"
+#include "chromeos/network/client_cert_util.h"
 #include "chromeos/network/network_policy_observer.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_state_handler_observer.h"
@@ -47,6 +48,15 @@ class CHROMEOS_EXPORT ClientCertResolver : public NetworkStateHandlerObserver,
   void SetSlowTaskRunnerForTest(
       const scoped_refptr<base::TaskRunner>& task_runner);
 
+  // Returns true and sets the Shill properties that have to be configured in
+  // |shill_properties| if the certificate pattern |pattern| could be resolved.
+  // Returns false otherwise and sets empty Shill properties to clear the
+  // certificate configuration.
+  static bool ResolveCertificatePatternSync(
+      const client_cert::ConfigType client_cert_type,
+      const CertificatePattern& pattern,
+      base::DictionaryValue* shill_properties);
+
  private:
    // NetworkStateHandlerObserver overrides
   virtual void NetworkListChanged() OVERRIDE;
diff --git a/chromeos/network/client_cert_resolver_unittest.cc b/chromeos/network/client_cert_resolver_unittest.cc
index 86f45ac..f2dc452 100644
--- a/chromeos/network/client_cert_resolver_unittest.cc
+++ b/chromeos/network/client_cert_resolver_unittest.cc
@@ -101,17 +101,16 @@ class ClientCertResolverTest : public testing::Test {
   void StartCertLoader() {
     cert_loader_->StartWithNSSDB(test_nssdb_.get());
     if (test_client_cert_) {
-      test_pkcs11_id_ = base::StringPrintf(
-          "%i:%s",
-          cert_loader_->TPMTokenSlotID(),
-          CertLoader::GetPkcs11IdForCert(*test_client_cert_).c_str());
-      ASSERT_TRUE(!test_pkcs11_id_.empty());
+      int slot_id = 0;
+      const std::string pkcs11_id =
+          CertLoader::GetPkcs11IdAndSlotForCert(*test_client_cert_, &slot_id);
+      test_cert_id_ = base::StringPrintf("%i:%s", slot_id, pkcs11_id.c_str());
     }
   }
 
   // Imports a CA cert (stored as PEM in test_ca_cert_pem_) and a client
   // certificate signed by that CA. Its PKCS#11 ID is stored in
-  // |test_pkcs11_id_|.
+  // |test_cert_id_|.
   void SetupTestCerts() {
     // Import a CA cert.
     net::CertificateList ca_cert_list =
@@ -235,7 +234,7 @@ class ClientCertResolverTest : public testing::Test {
 
   ShillServiceClient::TestInterface* service_test_;
   ShillProfileClient::TestInterface* profile_test_;
-  std::string test_pkcs11_id_;
+  std::string test_cert_id_;
   scoped_refptr<net::X509Certificate> test_ca_cert_;
   std::string test_ca_cert_pem_;
   base::MessageLoop message_loop_;
@@ -295,7 +294,7 @@ TEST_F(ClientCertResolverTest, ResolveOnCertificatesLoaded) {
   // the test client cert and configured the network.
   std::string pkcs11_id;
   GetClientCertProperties(&pkcs11_id);
-  EXPECT_EQ(test_pkcs11_id_, pkcs11_id);
+  EXPECT_EQ(test_cert_id_, pkcs11_id);
 }
 
 TEST_F(ClientCertResolverTest, ResolveAfterPolicyApplication) {
@@ -313,7 +312,7 @@ TEST_F(ClientCertResolverTest, ResolveAfterPolicyApplication) {
   // the test client cert and configured the network.
   std::string pkcs11_id;
   GetClientCertProperties(&pkcs11_id);
-  EXPECT_EQ(test_pkcs11_id_, pkcs11_id);
+  EXPECT_EQ(test_cert_id_, pkcs11_id);
 }
 
 }  // namespace chromeos
diff --git a/chromeos/network/client_cert_util.cc b/chromeos/network/client_cert_util.cc
index 4957e0e..b831bea 100644
--- a/chromeos/network/client_cert_util.cc
+++ b/chromeos/network/client_cert_util.cc
@@ -11,6 +11,8 @@
 #include <string>
 #include <vector>
 
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chromeos/network/certificate_pattern.h"
 #include "chromeos/network/network_event_log.h"
@@ -29,71 +31,7 @@ namespace client_cert {
 
 namespace {
 
-// Functor to filter out non-matching issuers.
-class IssuerFilter {
- public:
-  explicit IssuerFilter(const IssuerSubjectPattern& issuer)
-    : issuer_(issuer) {}
-  bool operator()(const scoped_refptr<net::X509Certificate>& cert) const {
-    return !CertPrincipalMatches(issuer_, cert.get()->issuer());
-  }
- private:
-  const IssuerSubjectPattern& issuer_;
-};
-
-// Functor to filter out non-matching subjects.
-class SubjectFilter {
- public:
-  explicit SubjectFilter(const IssuerSubjectPattern& subject)
-    : subject_(subject) {}
-  bool operator()(const scoped_refptr<net::X509Certificate>& cert) const {
-    return !CertPrincipalMatches(subject_, cert.get()->subject());
-  }
- private:
-  const IssuerSubjectPattern& subject_;
-};
-
-// Functor to filter out certs that don't have private keys, or are invalid.
-class PrivateKeyFilter {
- public:
-  explicit PrivateKeyFilter(net::CertDatabase* cert_db) : cert_db_(cert_db) {}
-  bool operator()(const scoped_refptr<net::X509Certificate>& cert) const {
-    return cert_db_->CheckUserCert(cert.get()) != net::OK;
-  }
- private:
-  net::CertDatabase* cert_db_;
-};
-
-// Functor to filter out certs that don't have an issuer in the associated
-// IssuerCAPEMs list.
-class IssuerCaFilter {
- public:
-  explicit IssuerCaFilter(const std::vector<std::string>& issuer_ca_pems)
-    : issuer_ca_pems_(issuer_ca_pems) {}
-  bool operator()(const scoped_refptr<net::X509Certificate>& cert) const {
-    // Find the certificate issuer for each certificate.
-    // TODO(gspencer): this functionality should be available from
-    // X509Certificate or NSSCertDatabase.
-    net::ScopedCERTCertificate issuer_cert(CERT_FindCertIssuer(
-        cert.get()->os_cert_handle(), PR_Now(), certUsageAnyCA));
-
-    if (!issuer_cert)
-      return true;
-
-    std::string pem_encoded;
-    if (!net::X509Certificate::GetPEMEncoded(issuer_cert.get(),
-                                             &pem_encoded)) {
-      LOG(ERROR) << "Couldn't PEM-encode certificate.";
-      return true;
-    }
-
-    return (std::find(issuer_ca_pems_.begin(), issuer_ca_pems_.end(),
-                      pem_encoded) ==
-            issuer_ca_pems_.end());
-  }
- private:
-  const std::vector<std::string>& issuer_ca_pems_;
-};
+const char kDefaultTPMPin[] = "111111";
 
 std::string GetStringFromDictionary(const base::DictionaryValue& dict,
                                     const std::string& key) {
@@ -159,65 +97,6 @@ bool CertPrincipalMatches(const IssuerSubjectPattern& pattern,
   return true;
 }
 
-scoped_refptr<net::X509Certificate> GetCertificateMatch(
-    const CertificatePattern& pattern,
-    const net::CertificateList& all_certs) {
-  typedef std::list<scoped_refptr<net::X509Certificate> > CertificateStlList;
-
-  // Start with all the certs, and narrow it down from there.
-  CertificateStlList matching_certs;
-
-  if (all_certs.empty())
-    return NULL;
-
-  for (net::CertificateList::const_iterator iter = all_certs.begin();
-       iter != all_certs.end(); ++iter) {
-    matching_certs.push_back(*iter);
-  }
-
-  // Strip off any certs that don't have the right issuer and/or subject.
-  if (!pattern.issuer().Empty()) {
-    matching_certs.remove_if(IssuerFilter(pattern.issuer()));
-    if (matching_certs.empty())
-      return NULL;
-  }
-
-  if (!pattern.subject().Empty()) {
-    matching_certs.remove_if(SubjectFilter(pattern.subject()));
-    if (matching_certs.empty())
-      return NULL;
-  }
-
-  if (!pattern.issuer_ca_pems().empty()) {
-    matching_certs.remove_if(IssuerCaFilter(pattern.issuer_ca_pems()));
-    if (matching_certs.empty())
-      return NULL;
-  }
-
-  // Eliminate any certs that don't have private keys associated with
-  // them.  The CheckUserCert call in the filter is a little slow (because of
-  // underlying PKCS11 calls), so we do this last to reduce the number of times
-  // we have to call it.
-  PrivateKeyFilter private_filter(net::CertDatabase::GetInstance());
-  matching_certs.remove_if(private_filter);
-
-  if (matching_certs.empty())
-    return NULL;
-
-  // We now have a list of certificates that match the pattern we're
-  // looking for.  Now we find the one with the latest start date.
-  scoped_refptr<net::X509Certificate> latest(NULL);
-
-  // Iterate over the rest looking for the one that was issued latest.
-  for (CertificateStlList::iterator iter = matching_certs.begin();
-       iter != matching_certs.end(); ++iter) {
-    if (!latest.get() || (*iter)->valid_start() > latest->valid_start())
-      latest = *iter;
-  }
-
-  return latest;
-}
-
 std::string GetPkcs11IdFromEapCertId(const std::string& cert_id) {
   if (cert_id.empty())
     return std::string();
@@ -235,62 +114,82 @@ std::string GetPkcs11IdFromEapCertId(const std::string& cert_id) {
 }
 
 void SetShillProperties(const ConfigType cert_config_type,
-                        const std::string& tpm_slot,
-                        const std::string& tpm_pin,
-                        const std::string* pkcs11_id,
+                        const int tpm_slot,
+                        const std::string& pkcs11_id,
                         base::DictionaryValue* properties) {
-  const char* tpm_pin_property = NULL;
   switch (cert_config_type) {
     case CONFIG_TYPE_NONE: {
       return;
     }
     case CONFIG_TYPE_OPENVPN: {
-      tpm_pin_property = shill::kOpenVPNPinProperty;
-      if (pkcs11_id) {
-        properties->SetStringWithoutPathExpansion(
-            shill::kOpenVPNClientCertIdProperty, *pkcs11_id);
-      }
+      properties->SetStringWithoutPathExpansion(shill::kOpenVPNPinProperty,
+                                                kDefaultTPMPin);
+      properties->SetStringWithoutPathExpansion(
+          shill::kOpenVPNClientCertIdProperty, pkcs11_id);
+      break;
+    }
+    case CONFIG_TYPE_IPSEC: {
+      properties->SetStringWithoutPathExpansion(shill::kL2tpIpsecPinProperty,
+                                                kDefaultTPMPin);
+      properties->SetStringWithoutPathExpansion(
+          shill::kL2tpIpsecClientCertSlotProperty, base::IntToString(tpm_slot));
+      properties->SetStringWithoutPathExpansion(
+          shill::kL2tpIpsecClientCertIdProperty, pkcs11_id);
+      break;
+    }
+    case CONFIG_TYPE_EAP: {
+      properties->SetStringWithoutPathExpansion(shill::kEapPinProperty,
+                                                kDefaultTPMPin);
+      std::string key_id =
+          base::StringPrintf("%i:%s", tpm_slot, pkcs11_id.c_str());
+
+      // Shill requires both CertID and KeyID for TLS connections, despite the
+      // fact that by convention they are the same ID, because one identifies
+      // the certificate and the other the private key.
+      properties->SetStringWithoutPathExpansion(shill::kEapCertIdProperty,
+                                                key_id);
+      properties->SetStringWithoutPathExpansion(shill::kEapKeyIdProperty,
+                                                key_id);
+      break;
+    }
+  }
+}
+
+void SetEmptyShillProperties(const ConfigType cert_config_type,
+                             base::DictionaryValue* properties) {
+  switch (cert_config_type) {
+    case CONFIG_TYPE_NONE: {
+      return;
+    }
+    case CONFIG_TYPE_OPENVPN: {
+      properties->SetStringWithoutPathExpansion(shill::kOpenVPNPinProperty,
+                                                std::string());
+      properties->SetStringWithoutPathExpansion(
+          shill::kOpenVPNClientCertIdProperty, std::string());
       break;
     }
     case CONFIG_TYPE_IPSEC: {
-      tpm_pin_property = shill::kL2tpIpsecPinProperty;
-      if (!tpm_slot.empty()) {
-        properties->SetStringWithoutPathExpansion(
-            shill::kL2tpIpsecClientCertSlotProperty, tpm_slot);
-      }
-      if (pkcs11_id) {
-        properties->SetStringWithoutPathExpansion(
-            shill::kL2tpIpsecClientCertIdProperty, *pkcs11_id);
-      }
+      properties->SetStringWithoutPathExpansion(shill::kL2tpIpsecPinProperty,
+                                                std::string());
+      properties->SetStringWithoutPathExpansion(
+          shill::kL2tpIpsecClientCertSlotProperty, std::string());
+      properties->SetStringWithoutPathExpansion(
+          shill::kL2tpIpsecClientCertIdProperty, std::string());
       break;
     }
     case CONFIG_TYPE_EAP: {
-      tpm_pin_property = shill::kEapPinProperty;
-      if (pkcs11_id) {
-        std::string key_id;
-        if (pkcs11_id->empty()) {
-          // An empty pkcs11_id means that we should clear the properties.
-        } else {
-          if (tpm_slot.empty())
-            NET_LOG_ERROR("Missing TPM slot id", "");
-          else
-            key_id = tpm_slot + ":";
-          key_id.append(*pkcs11_id);
-        }
-        // Shill requires both CertID and KeyID for TLS connections, despite the
-        // fact that by convention they are the same ID, because one identifies
-        // the certificate and the other the private key.
-        properties->SetStringWithoutPathExpansion(shill::kEapCertIdProperty,
-                                                  key_id);
-        properties->SetStringWithoutPathExpansion(shill::kEapKeyIdProperty,
-                                                  key_id);
-      }
+      properties->SetStringWithoutPathExpansion(shill::kEapPinProperty,
+                                                std::string());
+      // Shill requires both CertID and KeyID for TLS connections, despite the
+      // fact that by convention they are the same ID, because one identifies
+      // the certificate and the other the private key.
+      properties->SetStringWithoutPathExpansion(shill::kEapCertIdProperty,
+                                                std::string());
+      properties->SetStringWithoutPathExpansion(shill::kEapKeyIdProperty,
+                                                std::string());
       break;
     }
   }
-  DCHECK(tpm_pin_property);
-  if (!tpm_pin.empty())
-    properties->SetStringWithoutPathExpansion(tpm_pin_property, tpm_pin);
 }
 
 ClientCertConfig::ClientCertConfig()
diff --git a/chromeos/network/client_cert_util.h b/chromeos/network/client_cert_util.h
index 9e3b0a2..68f07a7 100644
--- a/chromeos/network/client_cert_util.h
+++ b/chromeos/network/client_cert_util.h
@@ -24,8 +24,6 @@ typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 
 namespace chromeos {
 
-class IssuerSubjectPattern;
-
 namespace client_cert {
 
 enum ConfigType {
@@ -57,26 +55,23 @@ struct CHROMEOS_EXPORT ClientCertConfig {
 bool CertPrincipalMatches(const IssuerSubjectPattern& pattern,
                           const net::CertPrincipal& principal);
 
-// Fetches the matching certificate that has the latest valid start date.
-// Returns a NULL refptr if there is no such match.
-CHROMEOS_EXPORT scoped_refptr<net::X509Certificate> GetCertificateMatch(
-    const CertificatePattern& pattern,
-    const net::CertificateList& all_certs);
-
 // Returns the PKCS11 id part of |cert_id|, which is expected to be the value of
 // the Shill property kEapCertIdProperty or kEapKeyIdProperty.
 CHROMEOS_EXPORT std::string GetPkcs11IdFromEapCertId(
     const std::string& cert_id);
 
-// If not empty, sets the TPM properties in |properties|. If |pkcs11_id| is not
-// NULL, also sets the ClientCertID. |cert_config_type| determines which
-// dictionary entries to set.
+// Sets the properties of a client cert and the TPM slot that it's contained in.
+// |cert_config_type| determines which dictionary entries to set.
 CHROMEOS_EXPORT void SetShillProperties(const ConfigType cert_config_type,
-                                        const std::string& tpm_slot,
-                                        const std::string& tpm_pin,
-                                        const std::string* pkcs11_id,
+                                        const int tpm_slot,
+                                        const std::string& pkcs11_id,
                                         base::DictionaryValue* properties);
 
+// Like SetShillProperties but instead sets the properties to empty strings.
+// This should be used to clear previously set client certificate properties.
+CHROMEOS_EXPORT void SetEmptyShillProperties(const ConfigType cert_config_type,
+                                             base::DictionaryValue* properties);
+
 // Returns true if all required configuration properties are set and not empty.
 bool IsCertificateConfigured(const client_cert::ConfigType cert_config_type,
                              const base::DictionaryValue& service_properties);
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 33473e8..99c3051 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -14,6 +14,7 @@
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_service_client.h"
 #include "chromeos/network/certificate_pattern.h"
+#include "chromeos/network/client_cert_resolver.h"
 #include "chromeos/network/client_cert_util.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_configuration_handler.h"
@@ -23,7 +24,6 @@
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/shill_property_util.h"
-#include "chromeos/tpm_token_loader.h"
 #include "dbus/object_path.h"
 #include "net/cert/x509_certificate.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
@@ -473,18 +473,13 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
       return;
     }
 
-    // If the client certificate must be configured, this will be set to a
-    // non-empty string.
-    std::string pkcs11_id;
-
     // Check certificate properties from policy.
-    // Note: Wifi/VPNConfigView set the KeyID and CertID properties directly,
-    // in which case only the TPM must be configured.
     if (cert_config_from_policy.client_cert_type ==
         onc::client_cert::kPattern) {
-      pkcs11_id = CertificateIsConfigured(cert_config_from_policy.pattern);
-      // Ensure the certificate is available and configured.
-      if (!cert_loader_->IsHardwareBacked() || pkcs11_id.empty()) {
+      if (!ClientCertResolver::ResolveCertificatePatternSync(
+              client_cert_type,
+              cert_config_from_policy.pattern,
+              &config_properties)) {
         ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
         return;
       }
@@ -495,19 +490,6 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
       ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
       return;
     }
-
-    // The network may not be 'Connectable' because the TPM properties are not
-    // set up, so configure tpm slot/pin before connecting.
-    if (cert_loader_ && cert_loader_->IsHardwareBacked()) {
-      // Pass NULL if pkcs11_id is empty, so that it doesn't clear any
-      // previously configured client cert.
-      client_cert::SetShillProperties(
-          client_cert_type,
-          base::IntToString(cert_loader_->TPMTokenSlotID()),
-          TPMTokenLoader::Get()->tpm_user_pin(),
-          pkcs11_id.empty() ? NULL : &pkcs11_id,
-          &config_properties);
-    }
   }
 
   if (type == shill::kTypeVPN) {
@@ -745,18 +727,6 @@ void NetworkConnectionHandler::CheckAllPendingRequests() {
   }
 }
 
-std::string NetworkConnectionHandler::CertificateIsConfigured(
-    const CertificatePattern& pattern) {
-  if (pattern.Empty())
-    return std::string();
-  // Find the matching certificate.
-  scoped_refptr<net::X509Certificate> matching_cert =
-      client_cert::GetCertificateMatch(pattern, cert_loader_->cert_list());
-  if (!matching_cert.get())
-    return std::string();
-  return CertLoader::GetPkcs11IdForCert(*matching_cert.get());
-}
-
 void NetworkConnectionHandler::ErrorCallbackForPendingRequest(
     const std::string& service_path,
     const std::string& error_name) {
diff --git a/chromeos/network/network_connection_handler.h b/chromeos/network/network_connection_handler.h
index cc38dc7b..80e0376 100644
--- a/chromeos/network/network_connection_handler.h
+++ b/chromeos/network/network_connection_handler.h
@@ -24,7 +24,6 @@
 
 namespace chromeos {
 
-class CertificatePattern;
 class NetworkState;
 
 // The NetworkConnectionHandler class is used to manage network connection
@@ -194,9 +193,6 @@ class CHROMEOS_EXPORT NetworkConnectionHandler
   void CheckPendingRequest(const std::string service_path);
   void CheckAllPendingRequests();
 
-  // Returns the PKCS#11 ID of a cert matching the certificate pattern. Returns
-  // empty string otherwise.
-  std::string CertificateIsConfigured(const CertificatePattern& pattern);
   void ErrorCallbackForPendingRequest(const std::string& service_path,
                                       const std::string& error_name);
 
diff --git a/chromeos/network/network_connection_handler_unittest.cc b/chromeos/network/network_connection_handler_unittest.cc
index 5725432..ef69213 100644
--- a/chromeos/network/network_connection_handler_unittest.cc
+++ b/chromeos/network/network_connection_handler_unittest.cc
@@ -205,21 +205,45 @@ class NetworkConnectionHandlerTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
-  void ImportClientCertAndKey(const std::string& pkcs12_file,
-                              net::NSSCertDatabase* nssdb,
-                              net::CertificateList* loaded_certs) {
+  scoped_refptr<net::X509Certificate> ImportTestClientCert() {
+    net::CertificateList ca_cert_list =
+        net::CreateCertificateListFromFile(net::GetTestCertsDirectory(),
+                                           "websocket_cacert.pem",
+                                           net::X509Certificate::FORMAT_AUTO);
+    if (ca_cert_list.empty()) {
+      LOG(ERROR) << "No CA cert loaded.";
+      return NULL;
+    }
+    net::NSSCertDatabase::ImportCertFailureList failures;
+    EXPECT_TRUE(test_nssdb_->ImportCACerts(
+        ca_cert_list, net::NSSCertDatabase::TRUST_DEFAULT, &failures));
+    if (!failures.empty()) {
+      LOG(ERROR) << net::ErrorToString(failures[0].net_error);
+      return NULL;
+    }
+
     std::string pkcs12_data;
     base::FilePath pkcs12_path =
-        net::GetTestCertsDirectory().Append(pkcs12_file);
-    ASSERT_TRUE(base::ReadFileToString(pkcs12_path, &pkcs12_data));
-
-    scoped_refptr<net::CryptoModule> module(
-        net::CryptoModule::CreateFromHandle(nssdb->GetPrivateSlot().get()));
-    ASSERT_EQ(
-        net::OK,
-        nssdb->ImportFromPKCS12(module, pkcs12_data, base::string16(), false,
-                                loaded_certs));
-    ASSERT_EQ(1U, loaded_certs->size());
+        net::GetTestCertsDirectory().Append("websocket_client_cert.p12");
+    if (!base::ReadFileToString(pkcs12_path, &pkcs12_data))
+      return NULL;
+
+    net::CertificateList loaded_certs;
+    scoped_refptr<net::CryptoModule> module(net::CryptoModule::CreateFromHandle(
+        test_nssdb_->GetPrivateSlot().get()));
+    if (test_nssdb_->ImportFromPKCS12(
+            module, pkcs12_data, base::string16(), false, &loaded_certs) !=
+        net::OK) {
+      LOG(ERROR) << "Error while importing to NSSDB.";
+      return NULL;
+    }
+
+    // File contains two certs, the client cert first and the CA cert second.
+    if (loaded_certs.size() != 2U) {
+      LOG(ERROR) << "Expected two certs in file, found " << loaded_certs.size();
+      return NULL;
+    }
+    return loaded_certs[0];
   }
 
   void SetupPolicy(const std::string& network_configs_json,
@@ -355,14 +379,11 @@ TEST_F(NetworkConnectionHandlerTest, ConnectCertificateMissing) {
 
 TEST_F(NetworkConnectionHandlerTest, ConnectWithCertificateSuccess) {
   StartCertLoader();
-
-  net::CertificateList certs;
-  ImportClientCertAndKey("websocket_client_cert.p12",
-                         test_nssdb_.get(),
-                         &certs);
+  scoped_refptr<net::X509Certificate> cert = ImportTestClientCert();
+  ASSERT_TRUE(cert);
 
   SetupPolicy(base::StringPrintf(kPolicyWithCertPatternTemplate,
-                                 certs[0]->subject().common_name.c_str()),
+                                 cert->subject().common_name.c_str()),
               base::DictionaryValue(),  // no global config
               true);                    // load as user policy
 
@@ -373,13 +394,11 @@ TEST_F(NetworkConnectionHandlerTest, ConnectWithCertificateSuccess) {
 // Disabled, see http://crbug.com/396729.
 TEST_F(NetworkConnectionHandlerTest,
        DISABLED_ConnectWithCertificateRequestedBeforeCertsAreLoaded) {
-  net::CertificateList certs;
-  ImportClientCertAndKey("websocket_client_cert.p12",
-                         test_nssdb_.get(),
-                         &certs);
+  scoped_refptr<net::X509Certificate> cert = ImportTestClientCert();
+  ASSERT_TRUE(cert);
 
   SetupPolicy(base::StringPrintf(kPolicyWithCertPatternTemplate,
-                                 certs[0]->subject().common_name.c_str()),
+                                 cert->subject().common_name.c_str()),
               base::DictionaryValue(),  // no global config
               true);                    // load as user policy