On Wed, Sep 28, 2016 at 07:40:13PM +0300, Maxim Dounin wrote:
On Wed, Sep 28, 2016 at 05:28:48PM +0100, Alessandro Ghedini wrote:
On Wed, Sep 28, 2016 at 07:00:00PM +0300, Maxim Dounin wrote:
On Wed, Sep 28, 2016 at 03:37:48PM +0100, Alessandro Ghedini wrote:
On Wed, Sep 28, 2016 at 05:19:02PM +0300, Maxim Dounin wrote:
On Wed, Sep 28, 2016 at 03:10:46PM +0100, Alessandro Ghedini wrote:
I don't now what your current plans for supporting BoringSSL are, but its API has been fairly stable for a while and this is the only change required to make NGINX build with it again (the other issue with error definitions was fixed in BoringSSL itself).
I don't think BoringSSL is going to change the API back, so NGINX migh want to fix this if support for BoringSSL is desired (again, don't know your opinion on this).
Please have a look and let me know what you think.
: Ok, this looks like the real reason for the patch. This looks : like an API change in BoringSSL, and should be threated : accordingly.
: Given the number of various API changes BoringSSL introduces here : and there - we probably don't want to follow, at least till some : version is actually released.
Ok, thanks, I missed that. TBH I don't think the BoringSSL team intends to release "proper" versions like OpenSSL does, so what you propose to wait for might not actually ever happen.
Sure, and I'm fine with it.
I understand your concern of wanting to target a fixed release, but as I mentioned (and Piotr as well) BoringSSL's API seems to have been fairly stable for a while (except for fixes like the one for the problem mentioned in the patch you linked, which was worked around in BoringSSL itself), and AFAIK there aren't other similar compatibility problems left except for this build warning (but maybe Piotr could prove me wrong on that), so it might make sense to start looking at supporting BoringSSL again.
Last time I looked into BoringSSL code due to ticket #993 several months ago (https://trac.nginx.org/nginx/ticket/993), and my impression wasn't that positive.
Ah, BoringSSL actually supports the new SSL_CTX_set1_curves_list() API that NGINX already uses, but it doesn't seem to define SSL_CTRL_SET_CURVES_LIST (yet) so the other API is picked. I'll make a patch for BoringSSL to fix this.
I'm afraid you are wrong here,
$ grep -r SSL_CTX_set1_curves_list boringssl/ | wc -l 0
Instead, BoringSSL introduced its own API to work with curves.
BoringSSL now provides SSL_CTX_set1_curves_list() as well: https://github.com/google/boringssl/commit/5fd1807d95f75895ae99e336ac21b422f...
Both me and Piotr verified that it works with NGINX.
David Benjamin also suggested to modify my NGINX patch to always cast to (const char *) the argument of SSL_set_tlsext_host_name(), even when using OpenSSL since it uses macros and force-cast the argument back to (char *): https://github.com/openssl/openssl/blob/master/include/openssl/ssl.h#L1231
This way there would be no need for the #ifdef, which makes the whole thing nicer. I'll send an updated patch in a bit so you can pick the one you like the most if you decide to add BoringSSL compatibility.