Add new functions to handle UPN and email addresses

net/cert/x509_util_nss.h

 // Copyright (c) 2011 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.
 
 #ifndef NET_CERT_X509_UTIL_NSS_H_
 #define NET_CERT_X509_UTIL_NSS_H_
 
 #include <stddef.h>
 
 #include <string>
@@ skipping 11 matching lines @@
 typedef struct CERTNameStr CERTName;
 typedef struct PK11SlotInfoStr PK11SlotInfo;
 typedef struct PLArenaPool PLArenaPool;
 typedef struct SECItemStr SECItem;
 
 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,
                     CertPrincipal* principal);
 
 // Parses the date from |der_date| and outputs the result in |result|.
 void ParseDate(const SECItem* der_date, base::Time* result);
 
 // Parses the serial number from |certificate|.
 std::string ParseSerialNumber(const CERTCertificate* certificate);
 
 // Gets the subjectAltName extension field from the certificate, if any.

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

// Gets the dNSName and iPAddress name types from the subjectAltName
// extension of |cert_handle|, storing them in |dns_names| and
// |ip_addrs|, respectively.
// If no subjectAltName is present, or no names of that type are
// present, the relevant vectors are cleared.

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

Done.

 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(
     const char* data,
     size_t length,
     X509Certificate::Format format);
 
 // Reads a single certificate from |pickle_iter| and returns a platform-specific
 // certificate handle. Returns an invalid handle, NULL, on failure.
 X509Certificate::OSCertHandle ReadOSCertHandleFromPickle(
@@ skipping 36 matching lines @@
 std::string GetUniqueNicknameForSlot(const std::string& nickname,
                                      const SECItem* subject,
                                      PK11SlotInfo* slot);
 #endif  // defined(USE_NSS_CERTS) || defined(OS_IOS)
 
 } // namespace x509_util
 
 } // namespace net
 
 #endif  // NET_CERT_X509_UTIL_NSS_H_