Automatically resolve ClientCertificatePatterns.

chromeos/network/client_cert_resolver.cc

+// Copyright (c) 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>
+#include <algorithm>
+#include <string>
+
+#include "base/stl_util.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/network/network_state_handler.h"
+#include "chromeos/network/network_ui_data.h"
+#include "chromeos/network/onc/onc_constants.h"
+#include "dbus/object_path.h"
+#include "net/cert/x509_certificate.h"
+
+namespace {
+
+template <class T>
+bool ContainsValue(const std::vector<T>& vector, const T& value) {
+  return find(vector.begin(), vector.end(), value) != vector.end();
+}
+
+bool HasPrivateKey(const net::X509Certificate& cert) {
+  PK11SlotInfo* slot = PK11_KeyForCertExists(cert.os_cert_handle(), NULL, NULL);
+  if (!slot)
+    return false;
+
+  PK11_FreeSlot(slot);
+  return true;
+}
+
+}  // namespace
+
+namespace chromeos {
+
+struct ClientCertResolver::NetworkAndMatchingCert {
+  NetworkAndMatchingCert(const std::string& network_path,
+                         client_cert::ConfigType config_type,
+                         const std::string& cert_id)
+      : service_path(network_path),
+        cert_config_type(config_type),
+        pkcs11_id(cert_id) {}
+  std::string service_path;
+  client_cert::ConfigType cert_config_type;
+  // The id of the matching certificate.
+  std::string pkcs11_id;
+};

[stevenjb, 2013/08/07 19:04:51]

This is kind of confusing up here, maybe move to just after the anon namespace?

[pneubeck (no reviews), 2013/08/08 12:41:03]

It's used below in FindCertificateMatches, thus C++ requires the full
declaration there.

I moved it to the very top, maybe that's better than in between.

[stevenjb, 2013/08/08 16:35:11]

That helps I think, thanks.

+
+namespace {
+
+struct CertAndIssuer {
+  CertAndIssuer(const scoped_refptr<net::X509Certificate>& certificate,
+                const std::string& issuer)
+      : cert(certificate), pem_encoded_issuer(issuer) {}

[stevenjb, 2013/08/07 19:04:51]

one member per line

[pneubeck (no reviews), 2013/08/08 12:41:03]

Done.

+  scoped_refptr<net::X509Certificate> cert;
+  std::string pem_encoded_issuer;
+};
+
+bool CompareCertExpiration(const CertAndIssuer& a,
+                           const CertAndIssuer& b) {
+  return (a.cert->valid_expiry() < b.cert->valid_expiry());
+}
+
+struct NetworkAndCertPattern {
+  NetworkAndCertPattern(const std::string& network_path,
+                        client_cert::ConfigType config_type,
+                        const CertificatePattern& pattern)
+      : service_path(network_path),
+        cert_config_type(config_type),
+        client_cert_pattern(pattern) {}
+  std::string service_path;
+  client_cert::ConfigType cert_config_type;
+  CertificatePattern client_cert_pattern;
+};
+
+struct MatchCertWithPattern {
+  MatchCertWithPattern(const NetworkAndCertPattern& pattern)
+      : net_and_pattern(pattern) {}
+
+  bool operator()(const CertAndIssuer& cert_and_issuer) {
+    const CertificatePattern& pattern = net_and_pattern.client_cert_pattern;
+    if (!pattern.issuer().Empty() &&
+        !client_cert::CertPrincipalMatches(pattern.issuer(),
+                                           cert_and_issuer.cert->issuer())) {
+      return false;
+    }
+    if (!pattern.issuer().Empty() &&
+        !client_cert::CertPrincipalMatches(pattern.issuer(),
+                                           cert_and_issuer.cert->subject())) {
+      return false;
+    }
+
+    const std::vector<std::string>& issuer_ca_pems = pattern.issuer_ca_pems();
+    if (!issuer_ca_pems.empty() &&
+        !ContainsValue(issuer_ca_pems, cert_and_issuer.pem_encoded_issuer)) {
+      return false;
+    }
+    return true;
+  }
+
+  NetworkAndCertPattern net_and_pattern;
+};
+
+void FindCertificateMatches(
+    const net::CertificateList& certs,
+    std::vector<NetworkAndCertPattern>* networks,
+    std::vector<ClientCertResolver::NetworkAndMatchingCert>* matches) {

[stevenjb, 2013/08/07 19:04:51]

nit: typedef these vectors for better readability here and below

[pneubeck (no reviews), 2013/08/08 12:41:03]

Done.

+  std::vector<CertAndIssuer> client_certs;
+  for (net::CertificateList::const_iterator it = certs.begin();
+       it != certs.end();
+       ++it) {
+    const net::X509Certificate& cert = **it;
+    if (cert.valid_expiry().is_null() || cert.HasExpired() ||
+        !HasPrivateKey(cert)) {
+      continue;
+    }
+    CERTCertificate* issuer = CERT_FindCertIssuer(
+        cert.os_cert_handle(), PR_Now(), certUsageAnyCA);
+    if (!issuer) {
+      LOG(ERROR) << "Couldn't find cert issuer.";
+      continue;
+    }
+    std::string pem_encoded_issuer;
+    if (!net::X509Certificate::GetPEMEncoded(issuer, &pem_encoded_issuer)) {
+      LOG(ERROR) << "Couldn't PEM-encode certificate.";
+      continue;
+    }
+    client_certs.push_back(CertAndIssuer(*it, pem_encoded_issuer));
+  }
+
+  std::sort(client_certs.begin(), client_certs.end(), &CompareCertExpiration);
+
+  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));
+    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;
+    }
+    matches->push_back(ClientCertResolver::NetworkAndMatchingCert(
+        it->service_path, it->cert_config_type, pkcs11_id));
+  }
+}
+
+bool OncToClientCertConfigurationType(
+    const base::DictionaryValue& network_config,
+    client_cert::ConfigType* config_type) {
+  using namespace ::chromeos::onc;
+
+  const base::DictionaryValue* wifi = NULL;
+  network_config.GetDictionaryWithoutPathExpansion(network_config::kWiFi,
+                                                   &wifi);
+  if (wifi) {
+    const base::DictionaryValue* eap = NULL;
+    wifi->GetDictionaryWithoutPathExpansion(wifi::kEAP, &eap);
+    if (!eap)
+      return false;
+    *config_type = client_cert::CONFIG_TYPE_EAP;
+    return true;
+  }
+
+  const base::DictionaryValue* vpn = NULL;
+  network_config.GetDictionaryWithoutPathExpansion(network_config::kVPN,
+                                                   &vpn);
+  if (vpn) {
+    const base::DictionaryValue* openvpn = NULL;
+    vpn->GetDictionaryWithoutPathExpansion(vpn::kOpenVPN, &openvpn);
+    if (openvpn) {
+      *config_type = client_cert::CONFIG_TYPE_OPENVPN;
+      return true;
+    }
+
+    const base::DictionaryValue* ipsec = NULL;
+    vpn->GetDictionaryWithoutPathExpansion(vpn::kIPsec, &ipsec);
+    if (ipsec) {
+      *config_type = client_cert::CONFIG_TYPE_IPSEC;
+      return true;
+    }
+
+    return false;
+  }
+
+  const base::DictionaryValue* ethernet = NULL;
+  network_config.GetDictionaryWithoutPathExpansion(network_config::kEthernet,
+                                                   &ethernet);
+  if (ethernet) {
+    const base::DictionaryValue* eap = NULL;
+    ethernet->GetDictionaryWithoutPathExpansion(wifi::kEAP, &eap);
+    if (!eap)
+      return false;
+    *config_type = client_cert::CONFIG_TYPE_EAP;
+    return true;
+  }
+
+  return false;
+}
+
+void LogError(const std::string& service_path,
+              const std::string& dbus_error_name,
+              const std::string& dbus_error_message) {
+  network_handler::ShillErrorCallbackFunction(
+      "ClientCertResolver.SetProperties failed",
+      service_path,
+      network_handler::ErrorCallback(),
+      dbus_error_name,
+      dbus_error_message);
+}
+
+bool CertificatesLoaded() {
+  if(!CertLoader::Get()->certificates_loaded()) {
+    VLOG(1) << "Certificates not loaded yet.";
+    return false;
+  }
+  if (!CertLoader::Get()->IsHardwareBacked()) {
+    VLOG(1) << "TPM is not available.";
+    return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+ClientCertResolver::ClientCertResolver()
+    : network_state_handler_(NULL),
+      managed_network_config_handler_(NULL),
+      weak_ptr_factory_(this) {}

[stevenjb, 2013/08/07 19:04:51]

nit: } on next line

[pneubeck (no reviews), 2013/08/08 12:41:03]

Done.

+
+ClientCertResolver::~ClientCertResolver() {
+  if (network_state_handler_)
+    network_state_handler_->RemoveObserver(this, FROM_HERE);
+  if (CertLoader::IsInitialized())
+    CertLoader::Get()->RemoveObserver(this);
+  if (managed_network_config_handler_)
+    managed_network_config_handler_->RemoveObserver(this);
+}
+
+void ClientCertResolver::Init(
+    NetworkStateHandler* network_state_handler,
+    ManagedNetworkConfigurationHandler* managed_network_config_handler) {
+  DCHECK(network_state_handler);
+  network_state_handler_ = network_state_handler;
+  network_state_handler_->AddObserver(this, FROM_HERE);
+
+  DCHECK(managed_network_config_handler);
+  managed_network_config_handler_ = managed_network_config_handler;
+  managed_network_config_handler_->AddObserver(this);
+
+  CertLoader::Get()->AddObserver(this);
+}
+
+void ClientCertResolver::SetSlowTaskRunnerForTest(
+    const scoped_refptr<base::TaskRunner>& task_runner) {
+  slow_task_runner_for_test_ = task_runner;
+}
+
+void ClientCertResolver::NetworkListChanged() {
+  if (!CertificatesLoaded())
+    return;
+  // Configure only networks that were not configured before.
+
+  // We'll drop networks from the set, which are not known anymore.
+  std::set<std::string> old_resolved_networks;
+  old_resolved_networks.swap(resolved_networks_);
+
+  NetworkStateList networks;
+  network_state_handler_->GetNetworkList(&networks);
+
+  NetworkStateList networks_to_check;
+  for (NetworkStateList::const_iterator it = networks.begin();
+       it != networks.end();
+       ++it) {
+    const std::string& service_path = (*it)->path();
+    if (ContainsKey(old_resolved_networks, service_path)) {
+      resolved_networks_.insert(service_path);
+      continue;
+    }
+    networks_to_check.push_back(*it);
+  }

[stevenjb, 2013/08/07 19:04:51]

I'm a little concerned with this. Because Shill forgets about networks after
~30s, and creates new Services on a scan, we can end up seeing thousands of
services over the course of a session. While not a huge bloat, it could become
significant.

I think we need to only do this for Favorites. With
https://codereview.chromium.org/21961003/, GetFavoriteList() should be robust.

Alternately we can ignore networks without a guid, see note below.

[pneubeck (no reviews), 2013/08/08 12:41:03]

That's not right. The new list (resolved_networks_) will not contain old, no
more existing networks (old_resolved_networks). See the swap above (line 286).
The loop iterates over the new network list.
I totally agree. Again, it's easy to fix that afterwards as well.
I think that's a good easy solution for now.

[stevenjb, 2013/08/08 16:35:11]

Ahh... right... it's a swap not a merge. OK, that's fine then, especially with
skipping empty guids.

+
+  ResolveNetworks(networks_to_check);
+}
+
+void ClientCertResolver::OnCertificatesLoaded(
+    const net::CertificateList& cert_list,
+    bool initial_load) {
+  if (!CertificatesLoaded())
+    return;
+  // Compare all networks with all certificates.
+  NetworkStateList networks;
+  network_state_handler_->GetNetworkList(&networks);
+  ResolveNetworks(networks);
+}
+
+void ClientCertResolver::PolicyApplied(const std::string& service_path) {
+  if (!CertificatesLoaded())
+    return;
+  // Compare this network with all certificates.
+  const NetworkState* network =
+      network_state_handler_->GetNetworkState(service_path);
+  if (!network) {
+    LOG(ERROR) << "service path '" << service_path << "' unknown.";
+    return;
+  }
+  NetworkStateList networks;
+  networks.push_back(network);
+  ResolveNetworks(networks);
+}
+
+void ClientCertResolver::ResolveNetworks(
+    const NetworkStateList& networks) {
+  scoped_ptr<std::vector<NetworkAndCertPattern> > networks_with_pattern(
+      new std::vector<NetworkAndCertPattern>);
+
+  // Filter networks with ClientCertPattern. As ClientCertPatterns can only be
+  // set by policy, we check there.
+  for (NetworkStateList::const_iterator it = networks.begin();
+       it != networks.end();
+       ++it) {
+    const NetworkState& network = **it;

[stevenjb, 2013/08/07 19:04:51]

Fwiw, most of the rest of the code uses a const* instead of a const& when
iterating over NetworkStateList. I don't think it really matters much, but I
have a slight preference for consistency.

[pneubeck (no reviews), 2013/08/08 12:41:03]

I strongly prefer the enforcement of not-null by types.
Done.

+
+    // In any case, don't check this network again in NetworkListChanged.
+    resolved_networks_.insert(network.path());
+
+    // If this network is not managed, thus cannot have a ClientCertPattern.

[stevenjb, 2013/08/07 19:04:51]

s/thus/it

[pneubeck (no reviews), 2013/08/08 12:41:03]

Done.

+    if (network.guid().empty())
+      continue;

[stevenjb, 2013/08/07 19:04:51]

We can also use this to filter out networks to consider and track. Actually, we
could track networks by guid which should easily avoid the bloat problem.

[pneubeck (no reviews), 2013/08/08 12:41:03]

Done.

+
+    DCHECK(!network.profile_path().empty());

[stevenjb, 2013/08/07 19:04:51]

Is this a valid assertion? Shill will never set a GUID for a Service? What if we
forget a network, with the Service guid get cleared? It might be safer just to
skip networks without a profile_path.

[pneubeck (no reviews), 2013/08/08 12:41:03]

Done.

+    const base::DictionaryValue* policy =
+        managed_network_config_handler_->FindPolicyByGuidAndProfile(
+            network.guid(), network.profile_path());
+
+    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();
+    // TODO(pneubeck): Access the ClientCertPattern without relying on
+    //   NetworkUIData, which also parses other things that we don't need here.
+    // The ONCSource is irrelevant here.
+    scoped_ptr<NetworkUIData> ui_data(
+        NetworkUIData::CreateFromONC(onc::ONC_SOURCE_NONE, *policy));
+
+    // Skip networks that don't have a ClientCertPattern.
+    if (ui_data->certificate_type() != CLIENT_CERT_TYPE_PATTERN)
+      continue;
+
+    client_cert::ConfigType config_type =
+        client_cert::CONFIG_TYPE_OPENVPN;  // Initialize to arbitrary value.
+    if (!OncToClientCertConfigurationType(*policy, &config_type)) {
+      LOG(ERROR) << "UIData contains a CertificatePattern, but the policy "
+                 << "doesn't. Network: " << network.path();
+      continue;
+    }
+
+    networks_with_pattern->push_back(NetworkAndCertPattern(
+        network.path(), config_type, ui_data->certificate_pattern()));
+  }
+  if (networks_with_pattern->empty())
+    return;
+
+  VLOG(2) << "Start task for resolving client cert patterns.";
+  base::TaskRunner* task_runner = slow_task_runner_for_test_.get();
+  if (!task_runner)
+    task_runner = base::WorkerPool::GetTaskRunner(true /* task is slow */);
+
+  std::vector<NetworkAndMatchingCert>* matches =
+      new std::vector<NetworkAndMatchingCert>;
+  task_runner->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&FindCertificateMatches,
+                 CertLoader::Get()->cert_list(),
+                 base::Owned(networks_with_pattern.release()),
+                 matches),
+      base::Bind(&ClientCertResolver::ConfigureCertificates,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(matches)));
+}
+
+void ClientCertResolver::ConfigureCertificates(
+    std::vector<NetworkAndMatchingCert>* matches) {
+  for (std::vector<NetworkAndMatchingCert>::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;
+    client_cert::SetShillProperties(it->cert_config_type,
+                                    cert_loader->tpm_token_slot(),
+                                    cert_loader->tpm_user_pin(),
+                                    it->pkcs11_id,
+                                    &shill_properties);
+    DBusThreadManager::Get()->GetShillServiceClient()
+        ->SetProperties(dbus::ObjectPath(it->service_path),

[stevenjb, 2013/08/07 19:04:51]

nit: -> on prev line

[pneubeck (no reviews), 2013/08/08 12:41:03]

as all other formatting issues, this is what clang does.
(Btw. this particular case is consistent with what we usually do in case of '.')

[stevenjb, 2013/08/08 16:35:11]

Yeah, the clang formatting came up somewhere else. Guess we'll have to get used
to -> on the second line, but I'm not the only one who prefers -> on the
previous line.

+                        shill_properties,
+                        base::Bind(&base::DoNothing),
+                        base::Bind(&LogError, it->service_path));
+    network_state_handler_->RequestUpdateForNetwork(it->service_path);
+  }
+}
+
+}  // namespace chromeos