Enable client certificate patterns in device ONC policy

chromeos/network/client_cert_resolver.cc

Index: chromeos/network/client_cert_resolver.cc
diff --git a/chromeos/network/client_cert_resolver.cc b/chromeos/network/client_cert_resolver.cc
index 788e5ee9a5ea503119ed1f308210ebad9cd9c0f8..d26174e04792e715bf0bfa85d23fc8ad261b09e0 100644
--- a/chromeos/network/client_cert_resolver.cc
+++ b/chromeos/network/client_cert_resolver.cc
@@ -188,22 +188,41 @@ std::vector<CertAndIssuer> CreateSortedCertAndIssuerList(
   return client_certs;
 }
 
+// Chooses which cert list should be used to find certificate matches for a
+// client cert config based on ONC policy source. Device policy certificate

[emaxx, 2017/04/20 20:10:39]

nit: I'd prefer to move the second sentence inside the function, as it talks
already about the implementation detail, but not the interface of the function.

[pmarko, 2017/04/24 14:49:56]

Done. / Moved logic into the call site.

+// patterns may only match certificates which exist on the system token for
+// privacy reasons.
+const std::vector<CertAndIssuer>& ChooseCertList(

[emaxx, 2017/04/20 20:10:39]

Returning a reference that follows the reference received by an argument - that
looks a little bit unsafe to me. IIUC, this is correct only as long as the
function parameters are not temporary objects, so that it's becoming easy to
break this by an unrelated change.

When using pointers, it's becoming harder to break it and don't notice.
Or maybe find some other way to structure the code here.

[pmarko, 2017/04/24 14:49:56]

Done.
Very good point - yes, it would compile and crash if a temporary object was
passed. It is fragile because it relies on the caller keeping the instances
alive after the method returns. I've moved the logic into the call site and
removed this function.

+    const client_cert::ClientCertConfig& client_cert_config,
+    const std::vector<CertAndIssuer>& all_certs,
+    const std::vector<CertAndIssuer>& system_certs) {
+  if (client_cert_config.source_is_device_policy_)
+    return system_certs;
+  else

[emaxx, 2017/04/20 20:10:39]

nit: Remove else. See here:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[pmarko, 2017/04/24 14:49:56]

Done. / Moved logic into the call site.

+    return all_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,
+void FindCertificateMatches(const net::CertificateList& all_certs,
+                            const net::CertificateList& system_certs,
                             std::vector<NetworkAndCertPattern>* networks,
                             base::Time now,
                             NetworkCertMatches* matches) {
-  std::vector<CertAndIssuer> client_certs(
-      CreateSortedCertAndIssuerList(certs, now));
+  std::vector<CertAndIssuer> all_client_certs(
+      CreateSortedCertAndIssuerList(all_certs, now));
+
+  std::vector<CertAndIssuer> system_client_certs(
+      CreateSortedCertAndIssuerList(system_certs, now));
 
   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(),
+    const std::vector<CertAndIssuer>& client_certs =
+        ChooseCertList(it->cert_config, all_client_certs, system_client_certs);
+    std::vector<CertAndIssuer>::const_iterator cert_it =

[emaxx, 2017/04/20 20:10:39]

nit: I think "auto" instead of this long iterator type should be fine to use
here.

[pmarko, 2017/04/24 14:49:56]

Done.

+        std::find_if(client_certs.begin(), client_certs.end(),
                      MatchCertWithPattern(it->cert_config.pattern));
     std::string pkcs11_id;
     int slot_id = -1;
@@ -452,9 +471,10 @@ void ClientCertResolver::ResolveNetworks(
     if (network->profile_path().empty())
       continue;
 
+    onc::ONCSource onc_source;
     const base::DictionaryValue* policy =
         managed_network_config_handler_->FindPolicyByGuidAndProfile(
-            network->guid(), network->profile_path());
+            network->guid(), network->profile_path(), &onc_source);
 
     if (!policy) {
       VLOG(1) << "The policy for network " << network->path() << " with GUID "
@@ -466,7 +486,7 @@ void ClientCertResolver::ResolveNetworks(
 
     VLOG(2) << "Inspecting network " << network->path();
     client_cert::ClientCertConfig cert_config;
-    OncToClientCertConfig(*policy, &cert_config);
+    OncToClientCertConfig(onc_source, *policy, &cert_config);
 
     // Skip networks that don't have a ClientCertPattern.
     if (cert_config.client_cert_type != ::onc::client_cert::kPattern)
@@ -495,11 +515,13 @@ void ClientCertResolver::ResolveNetworks(
   resolve_task_running_ = true;
   NetworkCertMatches* matches = new NetworkCertMatches;
   base::PostTaskWithTraitsAndReply(
-      FROM_HERE, base::TaskTraits()
-                     .WithShutdownBehavior(
-                         base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
-                     .MayBlock(),
+      FROM_HERE,
+      base::TaskTraits()
+          .WithShutdownBehavior(
+              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
+          .MayBlock(),
       base::Bind(&FindCertificateMatches, CertLoader::Get()->cert_list(),
+                 CertLoader::Get()->system_cert_list(),
                  base::Owned(networks_to_resolve.release()), Now(), matches),
       base::Bind(&ClientCertResolver::ConfigureCertificates,
                  weak_ptr_factory_.GetWeakPtr(), base::Owned(matches)));