Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1720653002:
Add new functions to handle UPN and email addresses (Closed)
|
Created:
4 years, 10 months ago by Kevin Cernekee Modified:
4 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam, pfeldman, devtools-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd new functions to handle UPN and email addresses
The existing code only handles SAN fields that are useful for verifying
website certificates (hostnames + IPs). For CrOS wifi certs
we also need to read out email addresses and Microsoft UPN fields.
BUG=549659
TEST=`./out/Debug/net_unittests --gtest_filter="X509CertificateTest*"`
Committed: https://crrev.com/fca855226530609db4ec4400a83cf5339ae5c1e6
Cr-Commit-Position: refs/heads/master@{#378799}
Patch Set 1 #Patch Set 2 : Add UPN support; split into a separate function #Patch Set 3 : Don't squash 10 other commits into the same CL #
Total comments: 14
Patch Set 4 : incorporate code review feedback #Patch Set 5 : add NOTREACHED() for impossible enum case #
Total comments: 2
Patch Set 6 : rename function; reorder NET_EXPORT #
Total comments: 14
Patch Set 7 : incorporate code review feedback #Patch Set 8 : add unit tests #Patch Set 9 : fix ios build breakage #
Messages
Total messages: 27 (12 generated)
Description was changed from ========== Extend GetSubjectAltName to return email addresses Currently this function only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out SAN email addresses. BUG=549659 ========== to ========== Add GetSubjectAltName() variant to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 ==========
Description was changed from ========== Add GetSubjectAltName() variant to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 ========== to ========== Add GetSubjectAltName() variant to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 ==========
cernekee@chromium.org changed reviewers: + rsleevi@chromium.org
I'll add test cases once we agree on the general concept. The hardcoded OID string is not so nice - there's probably a better way. nsNSSCertHelper registers this OID as ms_nt_principal_name but I don't think we can rely on it (SECOID_FindOID() still returns NULL when I try to look it up). Is there a simple way to do an OID lookup that doesn't involve the OID database? Or a central place where we inject interesting OIDs into the database?
Thanks for taking this on; I'm not a fan of this approach in general, but I want to discuss more with a few other people and see if I'm being unreasonable, before saying "no" - and especially, to discuss some design alternatives.
https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_certifica... File net/cert/x509_certificate.h (right): https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_certifica... net/cert/x509_certificate.h:143: SAN_URI, Concrete reasons I don't like this: Exposing RFC 822 name and URI are entirely unnecessary for Chrome. Exposing UPN is specific only for a very specific use case. None of these are needed for general X509Certificate's - they only are needed for an OSCertHandle (which suggests something perhaps suited for x509_util working on a CERTCertificate or OSCertHandle) And there's also the concerns about cross-platform; does this functionality need to be cross platform (believe the answer is "No"), which would further argue *against* putting it in X509Certificate and moreso one of the platform specific files. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util.h File net/cert/x509_util.h (right): https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util.h#ne... net/cert/x509_util.h:36: 0x82, 0x37, 0x14, 0x2, 0x3}; I don't like exposing the constant's value in the header; plus this initialization syntax doesn't provide external linkage but instead duplicates the constant. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... File net/cert/x509_util_nss_certs.cc (right): https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:206: SECItem alt_name; alt_name should be zero-initialized SECItem alt_name = { siBuffer, 0, nullptr }; (IIRC) https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:212: PLArenaPool* arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE); We have scopers for this. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:217: SECITEM_FreeItem(&alt_name, PR_FALSE); This early free-ing seems unnecessary (we also have scopers to do this) https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:225: (type == X509Certificate::SAN_IP_ADDRESS && Further concerns: Exposing IP addresses / RFC names as std::string's feels fundamentally wrong here as well. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:225: (type == X509Certificate::SAN_IP_ADDRESS && 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: ... ; } ...
https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_certifica... File net/cert/x509_certificate.h (right): https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_certifica... net/cert/x509_certificate.h:143: SAN_URI, On 2016/02/27 00:38:45, Ryan Sleevi wrote: > Concrete reasons I don't like this: Exposing RFC 822 name and URI are entirely > unnecessary for Chrome. > > Exposing UPN is specific only for a very specific use case. > > None of these are needed for general X509Certificate's - they only are needed > for an OSCertHandle (which suggests something perhaps suited for x509_util > working on a CERTCertificate or OSCertHandle) > > And there's also the concerns about cross-platform; does this functionality need > to be cross platform (believe the answer is "No"), which would further argue > *against* putting it in X509Certificate and moreso one of the platform specific > files. Done. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util.h File net/cert/x509_util.h (right): https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util.h#ne... net/cert/x509_util.h:36: 0x82, 0x37, 0x14, 0x2, 0x3}; On 2016/02/27 00:38:45, Ryan Sleevi wrote: > I don't like exposing the constant's value in the header; plus this > initialization syntax doesn't provide external linkage but instead duplicates > the constant. Done. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... File net/cert/x509_util_nss_certs.cc (right): https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:206: SECItem alt_name; On 2016/02/27 00:38:45, Ryan Sleevi wrote: > alt_name should be zero-initialized > > SECItem alt_name = { siBuffer, 0, nullptr }; (IIRC) Done. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:212: PLArenaPool* arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE); On 2016/02/27 00:38:45, Ryan Sleevi wrote: > We have scopers for this. Done. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:217: SECITEM_FreeItem(&alt_name, PR_FALSE); On 2016/02/27 00:38:45, Ryan Sleevi wrote: > This early free-ing seems unnecessary (we also have scopers to do this) Done. https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:225: (type == X509Certificate::SAN_IP_ADDRESS && On 2016/02/27 00:38:45, Ryan Sleevi wrote: > Further concerns: Exposing IP addresses / RFC names as std::string's feels > fundamentally wrong here as well. What do you recommend? https://codereview.chromium.org/1720653002/diff/40001/net/cert/x509_util_nss_... net/cert/x509_util_nss_certs.cc:225: (type == X509Certificate::SAN_IP_ADDRESS && On 2016/02/27 00:38:45, Ryan Sleevi wrote: > 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: > ... ; > } > ... > Done.
Description was changed from ========== Add GetSubjectAltName() variant to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 ========== to ========== Add GetSubjectAltName() variant to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=buildbots ==========
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/1720653002/diff/80001/net/cert/x509_util_nss.h File net/cert/x509_util_nss.h (right): https://codereview.chromium.org/1720653002/diff/80001/net/cert/x509_util_nss.... net/cert/x509_util_nss.h:63: std::vector<std::string>* names); Avoid having two identical function names with different signatures. Maybe GetSubjectAltNameByType. Also, for functions I believe the order should be: NET_EXPORT void
Description was changed from ========== Add GetSubjectAltName() variant to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=buildbots ========== to ========== Add GetSubjectAltNameByType() variant to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=buildbots ==========
https://codereview.chromium.org/1720653002/diff/80001/net/cert/x509_util_nss.h File net/cert/x509_util_nss.h (right): https://codereview.chromium.org/1720653002/diff/80001/net/cert/x509_util_nss.... net/cert/x509_util_nss.h:63: std::vector<std::string>* names); On 2016/02/29 22:05:03, stevenjb wrote: > Avoid having two identical function names with different signatures. Maybe > GetSubjectAltNameByType. > > Also, for functions I believe the order should be: > NET_EXPORT void Done.
lgtm fwiw (but not an owner)
https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss.h File net/cert/x509_util_nss.h (right): https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:34: // Name). 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. https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:40: SAN_UPN, 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 https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:40: SAN_UPN, 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. https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:54: // Gets the subjectAltName extension field from the certificate, if any. // 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. https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:60: // support wifi on CrOS. 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) https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:60: // support wifi on CrOS. 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(...); https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... File net/cert/x509_util_nss_certs.cc (right): https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss_certs.cc:256: !memcmp(on->oid.data, kUpnOid, sizeof(kUpnOid))) { The use of ! for values that return <0,0,>0 has been a huge source of bugs, both security and otherwise. if (on->oid.len == sizeof(kUpnOid) && memcmp(...) == 0) { } https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss_certs.cc:258: if (SEC_ASN1DecodeItem(arena.get(), &decoded, SECURITY: DO *not* use this function in any way whatsoever. It is NOT safe to use! The idiomatic NSS way is SEC_QuickDERDecodeItem(...); alternatively, we've been updating code to use //net/der (as seen in //net/cert/internal/parse_name). For now, use QuickDER
https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss.h File net/cert/x509_util_nss.h (right): https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:34: // Name). On 2016/03/01 00:28:28, Ryan Sleevi wrote: > 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. Done. https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:40: SAN_UPN, On 2016/03/01 00:28:28, Ryan Sleevi wrote: > 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 Done. https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:54: // Gets the subjectAltName extension field from the certificate, if any. On 2016/03/01 00:28:28, Ryan Sleevi wrote: > // 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. Done. https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss.h:60: // support wifi on CrOS. On 2016/03/01 00:28:28, Ryan Sleevi wrote: > 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(...); Done. https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... File net/cert/x509_util_nss_certs.cc (right): https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss_certs.cc:256: !memcmp(on->oid.data, kUpnOid, sizeof(kUpnOid))) { On 2016/03/01 00:28:29, Ryan Sleevi wrote: > The use of ! for values that return <0,0,>0 has been a huge source of bugs, both > security and otherwise. > > if (on->oid.len == sizeof(kUpnOid) && > memcmp(...) == 0) { > } Done. https://codereview.chromium.org/1720653002/diff/100001/net/cert/x509_util_nss... net/cert/x509_util_nss_certs.cc:258: if (SEC_ASN1DecodeItem(arena.get(), &decoded, On 2016/03/01 00:28:28, Ryan Sleevi wrote: > SECURITY: DO *not* use this function in any way whatsoever. It is NOT safe to > use! > > The idiomatic NSS way is SEC_QuickDERDecodeItem(...); alternatively, we've been > updating code to use //net/der (as seen in //net/cert/internal/parse_name). For > now, use QuickDER Done.
Description was changed from ========== Add GetSubjectAltNameByType() variant to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=buildbots ========== to ========== Add new functions to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=buildbots ==========
Now you just need unit tests - that you can extract rfc822Name and that you can extract the UPN name properly :)
Description was changed from ========== Add new functions to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=buildbots ========== to ========== Add new functions to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=`./out/Debug/net_unittests --gtest_filter="X509CertificateTest*"` ==========
lgtm
The CQ bit was checked by cernekee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1720653002/#ps160001 (title: "fix ios build breakage")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720653002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720653002/160001
Message was sent while issue was closed.
Description was changed from ========== Add new functions to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=`./out/Debug/net_unittests --gtest_filter="X509CertificateTest*"` ========== to ========== Add new functions to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=`./out/Debug/net_unittests --gtest_filter="X509CertificateTest*"` ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add new functions to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=`./out/Debug/net_unittests --gtest_filter="X509CertificateTest*"` ========== to ========== Add new functions to handle UPN and email addresses The existing code only handles SAN fields that are useful for verifying website certificates (hostnames + IPs). For CrOS wifi certs we also need to read out email addresses and Microsoft UPN fields. BUG=549659 TEST=`./out/Debug/net_unittests --gtest_filter="X509CertificateTest*"` Committed: https://crrev.com/fca855226530609db4ec4400a83cf5339ae5c1e6 Cr-Commit-Position: refs/heads/master@{#378799} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/fca855226530609db4ec4400a83cf5339ae5c1e6 Cr-Commit-Position: refs/heads/master@{#378799} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
