Allow ${CERT_SAN_EMAIL} and ${CERT_SAN_UPN} in the ONC Identity field

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 3d438d8843c018caac5ec1a9018a0ce0ec639532..1fa103e0f1d4324e6da82f7c255692d47915e123 100644
--- a/chromeos/network/client_cert_resolver.cc
+++ b/chromeos/network/client_cert_resolver.cc
@@ -14,6 +14,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/strings/string_util.h"
 #include "base/task_runner.h"
 #include "base/threading/worker_pool.h"
 #include "base/time/clock.h"
@@ -25,6 +26,8 @@
 #include "dbus/object_path.h"
 #include "net/cert/scoped_nss_types.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util_nss.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
@@ -34,11 +37,13 @@ struct ClientCertResolver::NetworkAndMatchingCert {
   NetworkAndMatchingCert(const std::string& network_path,
                          client_cert::ConfigType config_type,
                          const std::string& cert_id,
-                         int slot_id)
+                         int slot_id,
+                         const std::string& configured_identity)
       : service_path(network_path),
         cert_config_type(config_type),
         pkcs11_id(cert_id),
-        key_slot_id(slot_id) {}
+        key_slot_id(slot_id),
+        identity(configured_identity) {}
 
   std::string service_path;
   client_cert::ConfigType cert_config_type;
@@ -48,6 +53,10 @@ struct ClientCertResolver::NetworkAndMatchingCert {
 
   // The id of the slot containing the certificate and the private key.
   int key_slot_id;
+
+  // The EAP-TLS Identity field, if it is configured through a policy
+  // substitution.

[Ryan Sleevi, 2016/02/29 23:51:51]

What does this mean?

[Kevin Cernekee, 2016/03/02 21:38:03]

Done.

+  std::string identity;
 };
 
 typedef std::vector<ClientCertResolver::NetworkAndMatchingCert>
@@ -178,6 +187,21 @@ std::vector<CertAndIssuer> CreateSortedCertAndIssuerList(
   return client_certs;
 }
 
+// Looks up subjectAlternativeName fields in |cert| that match |type|,
+// then searches for |var_name| in |identity| and replaces any occurrences
+// with the SAN.

[Ryan Sleevi, 2016/02/29 23:51:51]

Comment: This comment is somewhat grammatically confusing. It also leaves
ambiguous a variety of things - such as that a cert may have multiple
subjectAltName extensions (not supposed to, but can)

// Replaces the token |var_name| in |identity| with the first value
// of a subjectAltName whose type is |type| from the cert |cert|.
// Note: If |cert| does not contain a subjectAltName of type |type|,
// no substitution will be performed. If |cert| contains multiple
// subjectAltName extensions, only the first extension will be considered.

[Kevin Cernekee, 2016/03/02 21:38:03]

Done.

+void ReplaceSAN(const std::string& var_name,
+                const net::X509Certificate& cert,
+                net::x509_util::SubjectAltNameType type,
+                std::string* identity) {
+  if (identity->find(var_name, 0) == std::string::npos)
+    return;
+  std::vector<std::string> names;
+  net::x509_util::GetSubjectAltNameByType(cert.os_cert_handle(), type, &names);

[Ryan Sleevi, 2016/02/29 23:51:51]

API DESIGN: Why do you return all names, if you're clearly only interested in
the first?

[Kevin Cernekee, 2016/03/02 21:38:03]

Someday I might care about other names.  The callee already has to iterate
through the SAN list to find the one that it cares about, so it's not a big deal
to have it populate a vector rather than a string.

[Ryan Sleevi, 2016/03/02 21:57:28]

But today you don't.
No, the callee wouldn't - you could short-circuit at the first SAN.

I'm a strong believer in "write what you need, not what you might need" - for
the case of dNS/IP, we need all names, so we return all names. For the case of
rfc822Name/UPN, you only need the first, so returning the first saves memory and
time.

+  if (!names.empty())
+    base::ReplaceSubstringsAfterOffset(identity, 0, var_name, names.at(0));

[Ryan Sleevi, 2016/02/29 23:51:51]

DESIGN: Why do you use this method of substitution?

The API contract is a little weird for this method, so I'm trying to grok what
the logic was.

For example, in line 197, you already found |var_name| within identity, so you
know the exact offset and length of the substring to replace, so you could just
using string->replace - but that'd only replace the first instance. But you've
also coupled checking whether or not |var_name| exists within |identity| to this
method, but that naturally feels like it should be a precondition of the caller
that checks before they call this method (that is; your API allows |identity| to
contain |var_name| or not, and that seems weird)

[Kevin Cernekee, 2016/03/02 21:38:03]

It mirrors what was done for ${LOGIN_ID} and ${LOGIN_EMAIL}.

+}
+
 // 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.
@@ -197,6 +221,8 @@ void FindCertificateMatches(const net::CertificateList& certs,
                      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;
@@ -211,9 +237,20 @@ void FindCertificateMatches(const net::CertificateList& certs,
         // the worst case the user can remove the problematic cert.
         continue;
       }
+
+      // If the policy specifies an identity containing ${CERT_SAN_xxx},
+      // see if the cert contains a suitable subjectAltName that we can
+      // stuff into the shill properties.

[Ryan Sleevi, 2016/02/29 23:51:51]

Comment nit: Avoid "we" in comments; it's ambiguous. (Line 236-237 make me sad,
but let's leave them alone)

// If the policy specifies an identity containing either
// ${CERT_SAN_EMAIL} or ${CERT_SAN_UPN}, then replace that with the
// equivalent subjectAltName.

[Kevin Cernekee, 2016/03/02 21:38:03]

Done.

+      identity = it->cert_config.policy_identity;
+      ReplaceSAN(::onc::substitutes::kCertSANEmail, *cert_it->cert,
+                 net::x509_util::SAN_RFC822_NAME, &identity);

[Ryan Sleevi, 2016/02/29 23:51:51]

API DESIGN: When the SAN is absent and it just contains the email within the
subject, do customers expect this to work? That is, it's entirely reasonable to
say "That's not supported", but it's unclear from the bug and design doc whether
that's been requested.

[Kevin Cernekee, 2016/03/02 21:38:03]

Documented this in the bug.

+      ReplaceSAN(::onc::substitutes::kCertSANUPN, *cert_it->cert,
+                 net::x509_util::SAN_UPN, &identity);

[Ryan Sleevi, 2016/02/29 23:51:51]

So when there's no match, you leave the |identity| string as simply containing
"${CERT_SAN_EMAIL}" (due to line 201). Is that a reasonable API contract?
Seems... less than stellar.

Concrete design questions:
1) Should the caller be allowed to specify an identity of
"${CERT_SAN_UPN}${CERT_SAN_EMAIL}" ?
2) Should replacement be mutually exclusive
3) What should the behaviour be if a field isn't present.

[Kevin Cernekee, 2016/03/02 21:38:03]

I like it.  If a user can't get on the network and the admin sees
"${CERT_SAN_EMAIL}" in the RADIUS logs, he'll google it and figure out that it
was supposed to substitute the email address from the cert.
Yes, this should work if the cert has both fields.
This is a trivial substitution.  Let's not overcomplicate things.

     }
+
     matches->push_back(ClientCertResolver::NetworkAndMatchingCert(
-        it->service_path, it->cert_config.location, pkcs11_id, slot_id));
+        it->service_path, it->cert_config.location, pkcs11_id, slot_id,
+        identity));
   }
 }
 
@@ -502,6 +539,10 @@ void ClientCertResolver::ConfigureCertificates(NetworkCertMatches* matches) {
                                       it->key_slot_id,
                                       it->pkcs11_id,
                                       &shill_properties);
+      if (!it->identity.empty()) {
+        shill_properties.SetStringWithoutPathExpansion(
+            shill::kEapIdentityProperty, it->identity);
+      }
     }
     network_properties_changed_ = true;
     DBusThreadManager::Get()->GetShillServiceClient()->