Add new functions to handle UPN and email addresses

net/cert/x509_util_nss_certs.cc

Index: net/cert/x509_util_nss_certs.cc
diff --git a/net/cert/x509_util_nss_certs.cc b/net/cert/x509_util_nss_certs.cc
index 1ec360d31e6d81d013a6ae56154826cd72840160..6de7a63f241a89eef46b8c8357fae7963c7fc5b3 100644
--- a/net/cert/x509_util_nss_certs.cc
+++ b/net/cert/x509_util_nss_certs.cc
@@ -200,6 +200,54 @@ void GetSubjectAltName(CERTCertificate* cert_handle,
   PORT_FreeArena(arena, PR_FALSE);
 }
 
+void GetSubjectAltName(CERTCertificate* cert_handle,
+                       X509Certificate::SubjectAltNameType type,
+                       std::vector<std::string>* names) {
+  SECItem alt_name;

[Ryan Sleevi, 2016/02/27 00:38:45]

alt_name should be zero-initialized

SECItem alt_name = { siBuffer, 0, nullptr }; (IIRC)

[Kevin Cernekee, 2016/02/27 19:06:24]

Done.

+  SECStatus rv = CERT_FindCertExtension(
+      cert_handle, SEC_OID_X509_SUBJECT_ALT_NAME, &alt_name);
+  if (rv != SECSuccess)
+    return;
+
+  PLArenaPool* arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);

[Ryan Sleevi, 2016/02/27 00:38:45]

We have scopers for this.

[Kevin Cernekee, 2016/02/27 19:06:24]

Done.

+  DCHECK(arena != NULL);
+
+  CERTGeneralName* alt_name_list;
+  alt_name_list = CERT_DecodeAltNameExtension(arena, &alt_name);
+  SECITEM_FreeItem(&alt_name, PR_FALSE);

[Ryan Sleevi, 2016/02/27 00:38:45]

This early free-ing seems unnecessary (we also have scopers to do this)

[Kevin Cernekee, 2016/02/27 19:06:24]

Done.

+
+  CERTGeneralName* name = alt_name_list;
+  while (name) {
+    if ((type == X509Certificate::SAN_RFC822_NAME &&
+         name->type == certRFC822Name) ||
+        (type == X509Certificate::SAN_DNS_NAME && name->type == certDNSName) ||
+        (type == X509Certificate::SAN_URI && name->type == certURI) ||
+        (type == X509Certificate::SAN_IP_ADDRESS &&

[Ryan Sleevi, 2016/02/27 00:38:45]

Further concerns: Exposing IP addresses / RFC names as std::string's feels
fundamentally wrong here as well.

[Ryan Sleevi, 2016/02/27 00:38:45]

Design concerns: This seems better accomplished as a switch & helper, to make
sure we're not missing cases

if (name->type != GetTypeForEnumButWithABetterNamedFunction(type))
  continue;  // OK, something better than continue

switch (name->type) {
  case foo:
  case bar:
  case baz:
    ...;
   break;
  case bat:
    ... ;
}
...
    

[Kevin Cernekee, 2016/02/27 19:06:24]

Done.

[Kevin Cernekee, 2016/02/27 19:06:24]

What do you recommend?

+         name->type == certIPAddress)) {
+      names->push_back(
+          std::string(reinterpret_cast<char*>(name->name.other.data),
+                      name->name.other.len));
+    } else if (type == X509Certificate::SAN_UPN &&
+               name->type == certOtherName) {
+      OtherName* on = &name->name.OthName;
+      if (on->oid.len == sizeof(kUpnOid) &&
+          !memcmp(on->oid.data, kUpnOid, sizeof(kUpnOid))) {
+        SECItem decoded;
+        if (SEC_ASN1DecodeItem(arena, &decoded,
+                               SEC_ASN1_GET(SEC_UTF8StringTemplate),
+                               &name->name.OthName.name) == SECSuccess) {
+          names->push_back(
+              std::string(reinterpret_cast<char*>(decoded.data), decoded.len));
+        }
+      }
+    }
+    name = CERT_GetNextGeneralName(name);
+    if (name == alt_name_list)
+      break;
+  }
+  PORT_FreeArena(arena, PR_FALSE);
+}
+
 X509Certificate::OSCertHandles CreateOSCertHandlesFromBytes(
     const char* data,
     size_t length,