Enable client certificate patterns in device ONC policy

chromeos/network/client_cert_resolver.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/network/client_cert_resolver.h"
 
 #include <cert.h>
 #include <certt.h>  // for (SECCertUsageEnum) certUsageAnyCA
 #include <pk11pub.h>
 
@@ skipping 170 matching lines @@
         !CertLoader::IsCertificateHardwareBacked(&cert)) {
       continue;
     }
     client_certs.push_back(CertAndIssuer(*it, GetPEMEncodedIssuer(cert)));
   }
 
   std::sort(client_certs.begin(), client_certs.end(), &CompareCertExpiration);
   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;
     std::string identity;
 
     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
       // network.
@@ skipping 228 matching lines @@
            networks.begin(); it != networks.end(); ++it) {
     const NetworkState* network = *it;
 
     // In any case, don't check this network again in NetworkListChanged.
     resolved_networks_.insert(network->path());
 
     // If this network is not configured, it cannot have a ClientCertPattern.
     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 "
               << network->guid() << " is not available yet.";
       // Skip this network for now. Once the policy is loaded, PolicyApplied()
       // will retry.
       continue;
     }
 
     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)
       continue;
 
     networks_to_resolve->push_back(
         NetworkAndCertPattern(network->path(), cert_config));
   }
 
   if (networks_to_resolve->empty()) {
     VLOG(1) << "No networks to resolve.";
     NotifyResolveRequestCompleted();
     return;
   }
 
   if (resolve_task_running_) {
     VLOG(1) << "A resolve task is already running. Queue this request.";
     for (const NetworkAndCertPattern& network_and_pattern :
          *networks_to_resolve) {
       queued_networks_to_resolve_.insert(network_and_pattern.service_path);
     }
     return;
   }
 
   VLOG(2) << "Start task for resolving client cert patterns.";
   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)));
 }
 
 void ClientCertResolver::ResolvePendingNetworks() {
   NetworkStateHandler::NetworkStateList networks;
   network_state_handler_->GetNetworkListByType(NetworkTypePattern::Default(),
                                                true /* configured_only */,
                                                false /* visible_only */,
@@ skipping 52 matching lines @@
     observer.ResolveRequestCompleted(changed);
 }
 
 base::Time ClientCertResolver::Now() const {
   if (testing_clock_)
     return testing_clock_->Now();
   return base::Time::Now();
 }
 
 }  // namespace chromeos