Migrate slot ID of client certs in network configuration.

If a network was configured with a client certificate before R38, the slot ID will either be missing or not be valid in R38 and later because of the added device wide token.

This migration code will look up a configured client certificate by its PKCS#11 ID and update the slot id.
If the certificate can't be found, the client configuration will be removed in order to trigger a clean 'missing configuration' error in the UI.

This change is primarily targeted for manually configured networks and not policy-configured networks. The latter are automatically updated by the chromeos::ClientCertResolver.

BUG=403900

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

Cr-Commit-Position: refs/heads/master@{#290044}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@290044 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/options/wifi_config_view.cc

@@ -1293,7 +1293,9 @@ void WifiConfigView::InitFromProperties(
     std::string eap_cert_id;
     properties.GetStringWithoutPathExpansion(
         shill::kEapCertIdProperty, &eap_cert_id);
-    std::string pkcs11_id = client_cert::GetPkcs11IdFromEapCertId(eap_cert_id);
+    int unused_slot_id = 0;
+    std::string pkcs11_id = client_cert::GetPkcs11AndSlotIdFromEapCertId(
+        eap_cert_id, &unused_slot_id);
     if (!pkcs11_id.empty()) {
       int cert_index =
           CertLibrary::Get()->GetUserCertIndexByPkcs11Id(pkcs11_id);

chromeos/dbus/fake_shill_manager_client.cc

@@ -497,6 +497,7 @@ void FakeShillManagerClient::ServiceStateChanged(
 void FakeShillManagerClient::SortManagerServices(bool notify) {
   DVLOG(1) << "SortManagerServices";
   static const char* ordered_types[] = {shill::kTypeEthernet,
+                                        shill::kTypeEthernetEap,
                                         shill::kTypeWifi,
                                         shill::kTypeCellular,
                                         shill::kTypeWimax,
@@ -876,6 +877,8 @@ base::ListValue* FakeShillManagerClient::GetListProperty(
 bool FakeShillManagerClient::TechnologyEnabled(const std::string& type) const {
   if (type == shill::kTypeVPN)
     return true;  // VPN is always "enabled" since there is no associated device
+  if (type == shill::kTypeEthernetEap)
+    return true;
   bool enabled = false;
   const base::ListValue* technologies;
   if (stub_properties_.GetListWithoutPathExpansion(

chromeos/network/client_cert_util.cc

@@ -97,7 +97,9 @@ bool CertPrincipalMatches(const IssuerSubjectPattern& pattern,
   return true;
 }
 
-std::string GetPkcs11IdFromEapCertId(const std::string& cert_id) {
+std::string GetPkcs11AndSlotIdFromEapCertId(const std::string& cert_id,
+                                            int* slot_id) {
+  *slot_id = -1;
   if (cert_id.empty())
     return std::string();
 
@@ -110,9 +112,73 @@ std::string GetPkcs11IdFromEapCertId(const std::string& cert_id) {
     LOG(ERROR) << "Empty PKCS11 id in cert id.";
     return std::string();
   }
+  int parsed_slot_id;
+  if (base::StringToInt(cert_id.substr(0, delimiter_pos), &parsed_slot_id))
+    *slot_id = parsed_slot_id;
+  else
+    LOG(ERROR) << "Slot ID is not an integer. Cert ID is: " << cert_id << ".";
   return cert_id.substr(delimiter_pos + 1);
 }
 
+void GetClientCertFromShillProperties(
+    const base::DictionaryValue& shill_properties,
+    ConfigType* cert_config_type,
+    int* tpm_slot,
+    std::string* pkcs11_id) {
+  *cert_config_type = CONFIG_TYPE_NONE;
+  *tpm_slot = -1;
+  pkcs11_id->clear();
+
+  // Look for VPN specific client certificate properties.
+  //
+  // VPN Provider values are read from the "Provider" dictionary, not the
+  // "Provider.Type", etc keys (which are used only to set the values).
+  const base::DictionaryValue* provider_properties = NULL;
+  if (shill_properties.GetDictionaryWithoutPathExpansion(
+          shill::kProviderProperty, &provider_properties)) {
+    // Look for OpenVPN specific properties.
+    if (provider_properties->GetStringWithoutPathExpansion(
+            shill::kOpenVPNClientCertIdProperty, pkcs11_id)) {
+      *cert_config_type = CONFIG_TYPE_OPENVPN;
+      return;
+    }
+    // Look for L2TP-IPsec specific properties.
+    if (provider_properties->GetStringWithoutPathExpansion(
+                   shill::kL2tpIpsecClientCertIdProperty, pkcs11_id)) {
+      std::string cert_slot;
+      provider_properties->GetStringWithoutPathExpansion(
+          shill::kL2tpIpsecClientCertSlotProperty, &cert_slot);
+      if (!cert_slot.empty() && !base::StringToInt(cert_slot, tpm_slot)) {
+        LOG(ERROR) << "Cert slot is not an integer: " << cert_slot << ".";
+        return;
+      }
+
+      *cert_config_type = CONFIG_TYPE_IPSEC;
+    }
+    return;
+  }
+
+  // Look for EAP specific client certificate properties, which can either be
+  // part of a WiFi or EthernetEAP configuration.
+  std::string cert_id;
+  if (shill_properties.GetStringWithoutPathExpansion(shill::kEapCertIdProperty,
+                                                     &cert_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.
+    std::string key_id;
+    shill_properties.GetStringWithoutPathExpansion(shill::kEapKeyIdProperty,
+                                                   &key_id);
+    // Assume the configuration to be invalid, if the two IDs are not identical.
+    if (cert_id != key_id) {
+      LOG(ERROR) << "EAP CertID differs from KeyID";
+      return;
+    }
+    *pkcs11_id = GetPkcs11AndSlotIdFromEapCertId(cert_id, tpm_slot);
+    *cert_config_type = CONFIG_TYPE_EAP;
+  }
+}
+
 void SetShillProperties(const ConfigType cert_config_type,
                         const int tpm_slot,
                         const std::string& pkcs11_id,

chromeos/network/client_cert_util.h

@@ -55,10 +55,26 @@ struct CHROMEOS_EXPORT ClientCertConfig {
 bool CertPrincipalMatches(const IssuerSubjectPattern& pattern,
                           const net::CertPrincipal& principal);
 
-// 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);
+// Returns the PKCS11 and slot ID of |cert_id|, which is expected to be a
+// value of the Shill property kEapCertIdProperty or kEapKeyIdProperty, either
+// of format "<pkcs11_id>" or "<slot_id>:<pkcs11_id>".
+CHROMEOS_EXPORT std::string GetPkcs11AndSlotIdFromEapCertId(
+    const std::string& cert_id,
+    int* slot_id);
+
+// Reads the client certificate configuration from the Shill Service properties
+// |shill_properties|.
+// If such a configuration is found, the values |cert_config_type|, |tpm_slot|
+// and |pkcs11_id| are filled accordingly. In case of OpenVPN or because the
+// property was not set, |tpm_slot| will be set to -1.
+// If an error occurred or no client configuration is found, |cert_config_type|
+// will be set to CONFIG_TYPE_NONE, |tpm_slot| to -1 and |pkcs11_id| to the
+// empty string.
+CHROMEOS_EXPORT void GetClientCertFromShillProperties(
+    const base::DictionaryValue& shill_properties,
+    ConfigType* cert_config_type,
+    int* tpm_slot,
+    std::string* pkcs11_id);
 
 // 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/network/network_cert_migrator.cc

@@ -12,6 +12,7 @@
 #include "base/metrics/histogram.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/client_cert_util.h"
 #include "chromeos/network/network_handler_callbacks.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
@@ -44,12 +45,23 @@ std::string GetNickname(const net::X509Certificate& cert) {
 
 }  // namespace
 
-// Checks which of the given |networks| has one of the deprecated
-// CaCertNssProperties set. If such a network already has a CaCertPEM property,
-// then the NssProperty is cleared. Otherwise, the NssProperty is compared with
-// the nickname of each certificate of |certs|. If a match is found, then the
-// CaCertPemProperty is set and the NssProperty is cleared. Otherwise, the
-// network is not modified.
+// Migrates each network of |networks| with a deprecated CaCertNss property to
+// the respective CaCertPEM property and fixes an invalid or missing slot ID of
+// a client certificate configuration.
+//
+// If a network already has a CaCertPEM property, then the NssProperty is
+// cleared. Otherwise, the NssProperty is compared with
+// the nickname of each certificate of |certs|. If a match is found, the
+// CaCertPemProperty is set and the NssProperty is cleared.
+//
+// If a network with a client certificate configuration (i.e. a PKCS11 ID) is
+// found, the configured client certificate is looked up.
+// If the certificate is found, the currently configured slot ID (if any) is
+// compared with the actual slot ID of the certificate and if required updated.
+// If the certificate is not found, the client certificate configuration is
+// removed.
+//
+// Only if necessary, a network will be notified.
 class NetworkCertMigrator::MigrationTask
     : public base::RefCounted<MigrationTask> {
  public:
@@ -61,11 +73,15 @@ class NetworkCertMigrator::MigrationTask
 
   void Run(const NetworkStateHandler::NetworkStateList& networks) {
     // Request properties for each network that has a CaCertNssProperty set
-    // according to the NetworkStateHandler.
+    // or which could be configured with a client certificate.
     for (NetworkStateHandler::NetworkStateList::const_iterator it =
              networks.begin(); it != networks.end(); ++it) {
-      if (!(*it)->HasCACertNSS())
+      if (!(*it)->HasCACertNSS() &&
+          (*it)->security() != shill::kSecurity8021x &&
+          (*it)->type() != shill::kTypeVPN &&
+          (*it)->type() != shill::kTypeEthernetEap) {
         continue;
+      }
       const std::string& service_path = (*it)->path();
       DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
           dbus::ObjectPath(service_path),
@@ -83,6 +99,59 @@ class NetworkCertMigrator::MigrationTask
       return;
     }
 
+    base::DictionaryValue new_properties;
+    MigrateClientCertProperties(service_path, properties, &new_properties);
+    MigrateNssProperties(service_path, properties, &new_properties);
+
+    if (new_properties.empty())
+      return;
+    SendPropertiesToShill(service_path, new_properties);
+  }
+
+  void MigrateClientCertProperties(const std::string& service_path,
+                                   const base::DictionaryValue& properties,
+                                   base::DictionaryValue* new_properties) {
+    int configured_slot_id = -1;
+    std::string pkcs11_id;
+    chromeos::client_cert::ConfigType config_type =
+        chromeos::client_cert::CONFIG_TYPE_NONE;
+    chromeos::client_cert::GetClientCertFromShillProperties(
+        properties, &config_type, &configured_slot_id, &pkcs11_id);
+    if (config_type == chromeos::client_cert::CONFIG_TYPE_NONE ||
+        pkcs11_id.empty()) {
+      return;
+    }
+
+    // OpenVPN configuration doesn't have a slot id to migrate.
+    if (config_type == chromeos::client_cert::CONFIG_TYPE_OPENVPN)
+      return;
+
+    int real_slot_id = -1;
+    scoped_refptr<net::X509Certificate> cert =
+        FindCertificateWithPkcs11Id(pkcs11_id, &real_slot_id);
+    if (!cert) {
+      LOG(WARNING) << "No matching cert found, removing the certificate "
+                      "configuration from network " << service_path;
+      chromeos::client_cert::SetEmptyShillProperties(config_type,
+                                                     new_properties);
+      return;
+    }
+    if (real_slot_id == -1) {
+      LOG(WARNING) << "Found a certificate without slot id.";
+      return;
+    }
+
+    if (cert && real_slot_id != configured_slot_id) {
+      VLOG(1) << "Network " << service_path
+              << " is configured with no or an incorrect slot id.";
+      chromeos::client_cert::SetShillProperties(
+          config_type, real_slot_id, pkcs11_id, new_properties);
+    }
+  }
+
+  void MigrateNssProperties(const std::string& service_path,
+                            const base::DictionaryValue& properties,
+                            base::DictionaryValue* new_properties) {
     std::string nss_key, pem_key, nickname;
     const base::ListValue* pem_property = NULL;
     UMANetworkType uma_type = UMA_NETWORK_TYPE_SIZE;
@@ -99,7 +168,7 @@ class NetworkCertMigrator::MigrationTask
 
     if (pem_property && !pem_property->empty()) {
       VLOG(2) << "PEM already exists, clearing NSS property.";
-      ClearNssProperty(service_path, nss_key);
+      ClearNssProperty(nss_key, new_properties);
       return;
     }
 
@@ -117,7 +186,8 @@ class NetworkCertMigrator::MigrationTask
       return;
     }
 
-    SetNssAndPemProperties(service_path, nss_key, pem_key, pem_encoded);
+    ClearNssProperty(nss_key, new_properties);
+    SetPemProperty(pem_key, pem_encoded, new_properties);
   }
 
   void GetNssAndPemProperties(const base::DictionaryValue& shill_properties,
@@ -159,18 +229,25 @@ class NetworkCertMigrator::MigrationTask
     }
   }
 
-  void ClearNssProperty(const std::string& service_path,
-                        const std::string& nss_key) {
-    DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-        dbus::ObjectPath(service_path),
-        nss_key,
-        base::StringValue(std::string()),
-        base::Bind(
-            &MigrationTask::NotifyNetworkStateHandler, this, service_path),
-        base::Bind(&network_handler::ShillErrorCallbackFunction,
-                   "MigrationTask.SetProperty failed",
-                   service_path,
-                   network_handler::ErrorCallback()));
+  void ClearNssProperty(const std::string& nss_key,
+                        base::DictionaryValue* new_properties) {
+    new_properties->SetStringWithoutPathExpansion(nss_key, std::string());
+  }
+
+  scoped_refptr<net::X509Certificate> FindCertificateWithPkcs11Id(
+      const std::string& pkcs11_id, int* slot_id) {
+    *slot_id = -1;
+    for (net::CertificateList::iterator it = certs_.begin(); it != certs_.end();
+         ++it) {
+      int current_slot_id = -1;
+      std::string current_pkcs11_id =
+          CertLoader::GetPkcs11IdAndSlotForCert(**it, &current_slot_id);
+      if (current_pkcs11_id == pkcs11_id) {
+        *slot_id = current_slot_id;
+        return *it;
+      }
+    }
+    return NULL;
   }
 
   scoped_refptr<net::X509Certificate> FindCertificateWithNickname(
@@ -183,19 +260,19 @@ class NetworkCertMigrator::MigrationTask
     return NULL;
   }
 
-  void SetNssAndPemProperties(const std::string& service_path,
-                              const std::string& nss_key,
-                              const std::string& pem_key,
-                              const std::string& pem_encoded_cert) {
-    base::DictionaryValue new_properties;
-    new_properties.SetStringWithoutPathExpansion(nss_key, std::string());
+  void SetPemProperty(const std::string& pem_key,
+                      const std::string& pem_encoded_cert,
+                      base::DictionaryValue* new_properties) {
     scoped_ptr<base::ListValue> ca_cert_pems(new base::ListValue);
     ca_cert_pems->AppendString(pem_encoded_cert);
-    new_properties.SetWithoutPathExpansion(pem_key, ca_cert_pems.release());
+    new_properties->SetWithoutPathExpansion(pem_key, ca_cert_pems.release());
+  }
 
+  void SendPropertiesToShill(const std::string& service_path,
+                             const base::DictionaryValue& properties) {
     DBusThreadManager::Get()->GetShillServiceClient()->SetProperties(
         dbus::ObjectPath(service_path),
-        new_properties,
+        properties,
         base::Bind(
             &MigrationTask::NotifyNetworkStateHandler, this, service_path),
         base::Bind(&MigrationTask::LogErrorAndNotifyNetworkStateHandler,
@@ -258,20 +335,26 @@ void NetworkCertMigrator::NetworkListChanged() {
     VLOG(2) << "Certs not loaded yet.";
     return;
   }
-  // Run the migration process from deprecated CaCertNssProperties to CaCertPem.
-  VLOG(2) << "Start NSS nickname to PEM migration.";
+  // Run the migration process from deprecated CaCertNssProperties to CaCertPem
+  // and to fix missing or incorrect slot ids of client certificates.
+  VLOG(2) << "Start certificate migration of network configurations.";
   scoped_refptr<MigrationTask> helper(new MigrationTask(
       CertLoader::Get()->cert_list(), weak_ptr_factory_.GetWeakPtr()));
   NetworkStateHandler::NetworkStateList networks;
-  network_state_handler_->GetVisibleNetworkList(&networks);
+  network_state_handler_->GetNetworkListByType(
+      NetworkTypePattern::Default(),
+      true,  // only configured networks
+      false, // visible and not visible networks
+      0,     // no count limit
+      &networks);
   helper->Run(networks);
 }
 
 void NetworkCertMigrator::OnCertificatesLoaded(
     const net::CertificateList& cert_list,
     bool initial_load) {
-  // Maybe there are networks referring to certs (by NSS nickname) that were not
-  // loaded before but are now.
+  // Maybe there are networks referring to certs that were not loaded before but
+  // are now.
   NetworkListChanged();
 }
 

chromeos/network/network_cert_migrator.h

@@ -16,7 +16,7 @@ namespace chromeos {
 class NetworkStateHandler;
 
 // Migrates network configurations from deprecated CaCertNSS properties to
-// CaCertPEM.
+// CaCertPEM and incorrect or missing slot IDs of client certificates.
 class CHROMEOS_EXPORT NetworkCertMigrator : public NetworkStateHandlerObserver,
                                             public CertLoader::Observer {
  public: