Add new functions to handle UPN and email addresses

net/cert/x509_util_nss.h

Index: net/cert/x509_util_nss.h
diff --git a/net/cert/x509_util_nss.h b/net/cert/x509_util_nss.h
index 55e562effbab95875f4fb9cd76099c0977d8a83b..b0ad7138e317f7506d541c28d5f52e451adf1b83 100644
--- a/net/cert/x509_util_nss.h
+++ b/net/cert/x509_util_nss.h
@@ -29,6 +29,17 @@ namespace net {
 namespace x509_util {
 
 #if defined(USE_NSS_CERTS) || defined(OS_IOS)
+// Allows the caller to filter the subjectAltName list and return only
+// a specific data type (e.g. email addresses or Microsoft User Principal
+// Name).

[Ryan Sleevi, 2016/03/01 00:28:28]

While not wanting to seem too aggressive in this CL, I do want to highlight that
this isn't a good comment; it doesn't describe what it does or what the values
are, and instead provides an 'example usage' which is really just documenting
the existing code.

However, please see my comments below for greater context.

[Kevin Cernekee, 2016/03/01 19:28:01]

Done.

+enum SubjectAltNameType {
+  SAN_RFC822_NAME,
+  SAN_DNS_NAME,
+  SAN_URI,
+  SAN_IP_ADDRESS,
+  SAN_UPN,

[Ryan Sleevi, 2016/03/01 00:28:28]

I highlighted this in a previous CL, but I do want to restate:

1) I'm opposed to exposing SAN_URI
2) I'm not a fan of adding a separate, duplicate method of exposing DNS &
IPAddress SANs

[Ryan Sleevi, 2016/03/01 00:28:28]

SAN_UPN explicitly is the sort of thing that should be documented. Each of these
really should be documented, ideally with a spec-citation inline

For example

// The type of subjectAltName to return.
// RFC 5280, Section 4.2.1.6 defines subjectAltName as:
//   SubjectAltName ::= GeneralNames
//   GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName
//   GeneralName ::= CHOICE {
//        otherName                       [0]     OtherName,
//        rfc822Name                      [1]     IA5String,
//        dNSName                         [2]     IA5String,
//        x400Address                     [3]     ORAddress,
//        directoryName                   [4]     Name,
//        ediPartyName                    [5]     EDIPartyName,
//        uniformResourceIdentifier       [6]     IA5String,
//        iPAddress                       [7]     OCTET STRING,
//        registeredID                    [8]     OBJECT IDENTIFIER }
//   OtherName ::= SEQUENCE {
//        type-id    OBJECT IDENTIFIER,
//        value      [0] EXPLICIT ANY DEFINED BY type-id }
//
// This enum only supports the subset of types that are relevant
// to GetSubjectAltNameByType().
enum SubjectAltNameType {
  // rfc822Name                      [1]     IA5String,
  SAN_RFC822_NAME,
  //  dNSName                         [2]     IA5String,
  SAN_DNS_NAME,
  ...
  // An OtherName value whose type-id is equal to
  // 1.3.6.1.4.1.311.20.2.3 (known as either id-ms-san-sc-logon-upn,
  // as described in RFC 4556, or as szOID_NT_PRINCIPAL_NAME, as
  // documented in Microsoft KB287547).
  // The value field is a UTF8String literal.
  // For more information:
  //   https://www.ietf.org/mail-archive/web/pkix/current/msg03145.html
  //   https://www.ietf.org/proceedings/65/slides/pkix-4/sld1.htm
  //   https://tools.ietf.org/html/rfc4556
  SAN_UPN
}


The goal is to be explicit *and thorough* in documentation, so that everyone
following has enough context to understand what's going on.

However, please take this suggestion in broader context of this CL.

[Kevin Cernekee, 2016/03/01 19:28:01]

Done.

+};
+
 // Parses the Principal attribute from |name| and outputs the result in
 // |principal|.
 void ParsePrincipal(CERTName* name,
@@ -45,6 +56,12 @@ void GetSubjectAltName(CERTCertificate* cert_handle,
                        std::vector<std::string>* dns_names,
                        std::vector<std::string>* ip_addrs);
 
+// Filters subjectAltName list by type.  Only implemented for NSS, to
+// support wifi on CrOS.

[Ryan Sleevi, 2016/03/01 00:28:28]

1) It's obvious this is only implemented for NSS, by virtue of this being in
_nss.h
2) "to support wifi on CrOS" is the sort of bad comment trifecta
  a) Abbreviates ChromeOS
  b) Explains an implementation detail (of code far away)
  c) grammar nit (only a single space after the period, wifi -> WiFi ||
wireless)

[Ryan Sleevi, 2016/03/01 00:28:28]

DESIGN: This design really gets to the crux of the matter. Nothing uses the URI
type, nor do you have any unittests to ensure the URI is returned. It does type
erasure (collapsing all name types to std::string, but of unclear contents,
especially for types that can be ambiguous - like directoryName: Would this be
the DER or the string form? And which string form - prepped or not?)

The concrete suggestion is this:

// Stores the values of all rfc822Name subjectAltNames from |cert_handle|
// into |names|. If no names are present, clears |names|.
// WARNING: This method does not validate that the rfc822Name is
// properly encoded; it MAY contain embedded NULs or other illegal
// characters; care should be taken to validate the well-formedness
// before using.
NET_EXPORT void GetRFC822SubjectAltNames(CERTCertificate* cert_handle,
                                         std::vector<std::string>* names);

// Clear documentation goes here
NET_EXPORT void GetUPNSubjectAltNames(...);

[Kevin Cernekee, 2016/03/01 19:28:01]

Done.

+NET_EXPORT void GetSubjectAltNameByType(CERTCertificate* cert_handle,
+                                        SubjectAltNameType type,
+                                        std::vector<std::string>* names);
+
 // Creates all possible OS certificate handles from |data| encoded in a specific
 // |format|. Returns an empty collection on failure.
 X509Certificate::OSCertHandles CreateOSCertHandlesFromBytes(