Clear the CertID of a network if no valid cert exists.

The old behavior of the ClientCertResolver was that a non-empty Cert/KeyID is never cleared. However, this lead to other code handling as if the network was correctly configured even if that Cert/KeyID didn't point to a valid cert anymore.
If a cert is available, the behavior will not change: the resolver always writes the id of the best fitting certificate.

The new behavior is, that if no certificate matching the network's cert pattern is found, then the Cert/KeyID is cleared.
Accordingly, this leads to the Enrollment Dialog correctly popping up.

BUG=390914

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

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

chromeos/network/client_cert_resolver.cc

@@ -35,7 +35,7 @@
 namespace chromeos {
 
 // Describes a network |network_path| for which a matching certificate |cert_id|
-// was found.
+// was found or for which no certificate was found (|cert_id| will be empty).
 struct ClientCertResolver::NetworkAndMatchingCert {
   NetworkAndMatchingCert(const std::string& network_path,
                          client_cert::ConfigType config_type,
@@ -46,7 +46,7 @@ struct ClientCertResolver::NetworkAndMatchingCert {
 
   std::string service_path;
   client_cert::ConfigType cert_config_type;
-  // The id of the matching certificate.
+  // The id of the matching certificate or empty if no certificate was found.
   std::string pkcs11_id;
 };
 
@@ -177,15 +177,20 @@ void FindCertificateMatches(const net::CertificateList& certs,
        it != networks->end(); ++it) {
     std::vector<CertAndIssuer>::iterator cert_it = std::find_if(
         client_certs.begin(), client_certs.end(), MatchCertWithPattern(*it));
+    std::string pkcs11_id;
     if (cert_it == client_certs.end()) {
-      LOG(WARNING) << "Couldn't find a matching client cert for network "
-                   << it->service_path;
-      continue;
-    }
-    std::string pkcs11_id = CertLoader::GetPkcs11IdForCert(*cert_it->cert);
-    if (pkcs11_id.empty()) {
-      LOG(ERROR) << "Couldn't determine PKCS#11 ID.";
-      continue;
+      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
+      // network.
+    } else {
+      pkcs11_id = CertLoader::GetPkcs11IdForCert(*cert_it->cert);
+      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.
+        continue;
+      }
     }
     matches->push_back(ClientCertResolver::NetworkAndMatchingCert(
         it->service_path, it->cert_config_type, pkcs11_id));
@@ -447,6 +452,8 @@ void ClientCertResolver::ConfigureCertificates(NetworkCertMatches* matches) {
     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()),

chromeos/network/client_cert_resolver_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/values.h"
 #include "chromeos/cert_loader.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
 #include "chromeos/network/managed_network_configuration_handler_impl.h"
@@ -82,7 +83,6 @@ class ClientCertResolverTest : public testing::Test {
     CertLoader::Initialize();
     cert_loader_ = CertLoader::Get();
     cert_loader_->force_hardware_backed_for_test();
-    cert_loader_->StartWithNSSDB(test_nssdb_.get());
   }
 
   virtual void TearDown() OVERRIDE {
@@ -98,6 +98,17 @@ class ClientCertResolverTest : public testing::Test {
   }
 
  protected:
+  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());
+    }
+  }
+
   // 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_|.
@@ -131,11 +142,7 @@ class ClientCertResolverTest : public testing::Test {
         test_nssdb_->ImportFromPKCS12(
             module, pkcs12_data, base::string16(), false, &client_cert_list));
     ASSERT_TRUE(!client_cert_list.empty());
-    test_pkcs11_id_ = base::StringPrintf(
-        "%i:%s",
-        cert_loader_->TPMTokenSlotID(),
-        CertLoader::GetPkcs11IdForCert(*client_cert_list[0]).c_str());
-    ASSERT_TRUE(!test_pkcs11_id_.empty());
+    test_client_cert_ = client_cert_list[0];
   }
 
   void SetupNetworkHandlers() {
@@ -160,16 +167,22 @@ class ClientCertResolverTest : public testing::Test {
   }
 
   void SetupWifi() {
-    const bool add_to_visible = true;
-    service_test_->AddService(kWifiStub,
-                              kWifiSSID,
-                              shill::kTypeWifi,
-                              shill::kStateOnline,
-                              add_to_visible);
+    service_test_->SetServiceProperties(kWifiStub,
+                                        kWifiStub,
+                                        kWifiSSID,
+                                        shill::kTypeWifi,
+                                        shill::kStateOnline,
+                                        true /* visible */);
+    // Set an arbitrary cert id, so that we can check afterwards whether we
+    // cleared the property or not.
     service_test_->SetServiceProperty(
-        kWifiStub, shill::kGuidProperty, base::StringValue(kWifiStub));
-
+        kWifiStub, shill::kEapCertIdProperty, base::StringValue("invalid id"));
     profile_test_->AddService(kUserProfilePath, kWifiStub);
+
+    DBusThreadManager::Get()
+        ->GetShillManagerClient()
+        ->GetTestInterface()
+        ->AddManagerService(kWifiStub, true);
   }
 
   // Setup a policy with a certificate pattern that matches any client cert that
@@ -242,6 +255,7 @@ class ClientCertResolverTest : public testing::Test {
   }
 
   CertLoader* cert_loader_;
+  scoped_refptr<net::X509Certificate> test_client_cert_;
   scoped_ptr<NetworkStateHandler> network_state_handler_;
   scoped_ptr<NetworkProfileHandler> network_profile_handler_;
   scoped_ptr<NetworkConfigurationHandler> network_config_handler_;
@@ -256,25 +270,25 @@ class ClientCertResolverTest : public testing::Test {
 
 TEST_F(ClientCertResolverTest, NoMatchingCertificates) {
   SetupNetworkHandlers();
-  SetupPolicy();
-  base::RunLoop().RunUntilIdle();
-
   SetupWifi();
+  StartCertLoader();
+  SetupPolicy();
   base::RunLoop().RunUntilIdle();
 
   // Verify that no client certificate was configured.
   std::string pkcs11_id;
   GetClientCertProperties(&pkcs11_id);
-  EXPECT_TRUE(pkcs11_id.empty());
+  EXPECT_EQ(std::string(), pkcs11_id);
 }
 
-TEST_F(ClientCertResolverTest, ResolveOnInitialization) {
-  SetupTestCerts();
+TEST_F(ClientCertResolverTest, ResolveOnCertificatesLoaded) {
   SetupNetworkHandlers();
+  SetupWifi();
+  SetupTestCerts();
   SetupPolicy();
   base::RunLoop().RunUntilIdle();
 
-  SetupWifi();
+  StartCertLoader();
   base::RunLoop().RunUntilIdle();
 
   // Verify that the resolver positively matched the pattern in the policy with
@@ -286,10 +300,12 @@ TEST_F(ClientCertResolverTest, ResolveOnInitialization) {
 
 TEST_F(ClientCertResolverTest, ResolveAfterPolicyApplication) {
   SetupTestCerts();
+  StartCertLoader();
   SetupNetworkHandlers();
+  SetupWifi();
   base::RunLoop().RunUntilIdle();
 
-  // The policy will trigger the creation of a new wifi service.
+  // Policy application will trigger the ClientCertResolver.
   SetupPolicy();
   base::RunLoop().RunUntilIdle();
 

chromeos/network/client_cert_util.cc

@@ -233,11 +233,15 @@ void SetShillProperties(const client_cert::ConfigType cert_config_type,
       tpm_pin_property = shill::kEapPinProperty;
       if (pkcs11_id) {
         std::string key_id;
-        if (tpm_slot.empty())
-          NET_LOG_ERROR("Missing TPM slot id", "");
-        else
-          key_id = tpm_slot + ":";
-        key_id.append(*pkcs11_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.