Conversation
The previous implementation had grown organically over time, as OpenSSL's API evolved.
|
@alex, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @tiran and @giampaolo to be potential reviewers. |
|
Windows failures don't appear to be relevant. |
| certificate, NID_subject_alt_name, i)) >= 0) { | ||
|
|
||
| names = (GENERAL_NAMES *)X509_get_ext_d2i( | ||
| certificate, NID_subject_alt_name, NULL, NULL); |
There was a problem hiding this comment.
Do we care about handling the case where there is > 1 SAN extension? Right now this will return the first one found and use it, but if there is > 1 we should really just error out.
There was a problem hiding this comment.
None of the other X.509 extension handlers attempt to handle this; we just defer to OpenSSL's behavior.
There was a problem hiding this comment.
I think this actually Does The Right Thing, the docs are just profoundly awful:
"If idx is NULL then only one occurrence of an extension is permissible otherwise the first extension after index *idx is returned and *idx updated to the location of the extension. If crit is not NULL then *crit is set to a status value: -2 if the extension occurs multiple times (this is only returned if idx is NULL), -1 if the extension could not be found, 0 if the extension is found and is not critical and 1 if critical. A pointer to an extension specific structure or NULL is returned."
|
@alex is your patch compatible with historic versions of OpenSSL? Or should we wait until we officially drop support for 1.0.1 and 0.9.8? |
|
@tiran yes, this works with all supported versions of OpenSSL -- we use this same API in the other extension parsing code. |
The previous implementation had grown organically over time, as OpenSSL's API evolved.