Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
On Sat, Dec 21, 2019 at 01:19:30AM +0100, Lukas Tribus wrote: > You can merge the patch I posted today, as there is consensus for this > particular fix: > https://www.mail-archive.com/haproxy@formilux.org/msg35760.html > > It should be backported to 2.0 (or even 1.9 - I forgot to mention that > in the commit message). OK I've merged it now, thank you. It was my first impression but the ongoing discussions made me doubt. > Further discussion is needed for some remaining points, which I doubt > can be fully clarified for the release tomorrow. Perfect! > If your question is, which openssl releases should we support with > no-deprecated, I'd say 1.1.0 and newer. I don't believe there is any > reason to support openssl 1.0.2 with no-deprecated. I tend to agree on this. It doesn't seem very logical to decide now to use an older release and disable what was considered deprecated when it was issued. If you use an older release it's because you have ABI compatibility issues, interoperability issues, or need legacy stuff. If there were issues for users facing older vendor-provided openssl libs built with no-deprecated, I think we'd know about it by now, and given that it's not the case, I also suggest we don't waste valuable time implementing support for something nobody intends to use. We can revisit this choice later if the situation ever changes, which I strongly doubt. Thanks! Willy
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
Hello, > Guys, I must confess I'm completely lost in your discussions. I intend > to produce another round of 2.1 and 2.0 tomorrow as time permits, so if > you want me to get anything merged into it, please let me know. Lukas, > I'll count on you to summarize and suggest what's expected from me to > do at this point, I guess it will be easier :-) You can merge the patch I posted today, as there is consensus for this particular fix: https://www.mail-archive.com/haproxy@formilux.org/msg35760.html It should be backported to 2.0 (or even 1.9 - I forgot to mention that in the commit message). Further discussion is needed for some remaining points, which I doubt can be fully clarified for the release tomorrow. On Fri, 20 Dec 2019 at 19:53, Илья Шипицин wrote: >> On Wed, 27 Nov 2019 at 07:11, Илья Шипицин wrote: >> > >> > -#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) >> > +#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) || >> > defined(OPENSSL_NO_DEPRECATED) >> > [...] >> > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) >> > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) && >> > !defined(OPENSSL_NO_DEPRECATED) >> > [...] >> > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) >> > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) && >> > !defined(OPENSSL_NO_DEPRECATED) >> > [...] >> >> I'm confused. This is not required in my environment for the build to >> succeed and I don't see any reason why HA_OPENSSL_VERSION_NUMBER would >> be smaller here? Can you elaborate why the HA_OPENSSL_VERSION_NUMBER >> comparison would fail to do its job in those comparisons? > > > what is the lowest openssl we support ? > > those callbacks are required if threads are used for non-deprecated builds > and for early openssl versions like 1.0.0 Are you saying this is for openssl 1.0.2 with no-deprecated, is that what fails without this? I have troubles understanding what it is *exactly* that requires this change. If your question is, which openssl releases should we support with no-deprecated, I'd say 1.1.0 and newer. I don't believe there is any reason to support openssl 1.0.2 with no-deprecated. Sure we support old releases. But I would not waste any time/changes and especially complexity on adding support for "no-deprecated" feature for obsolete openssl releases (1.0.2 is EOL in a few days, nobody turns on no-deprecated on 1.0.2 now). What I am saying is that this change is not needed in my environment for the build to succeed, which is against openssl 1.1.1d with no-deprecated option. Maybe there is something that I don't see from my build machine here, that's why I need more information. If you see a build failure after the SSL_CTX_set_ecdh_auto() fix on top of current master, please elaborate how that build failure looks like and how openssl was compiled. Thanks, Lukas
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
Guys, I must confess I'm completely lost in your discussions. I intend to produce another round of 2.1 and 2.0 tomorrow as time permits, so if you want me to get anything merged into it, please let me know. Lukas, I'll count on you to summarize and suggest what's expected from me to do at this point, I guess it will be easier :-) Thanks! Willy
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
сб, 21 дек. 2019 г. в 01:44, Rosen Penev : > On Fri, Dec 20, 2019 at 10:54 AM Илья Шипицин > wrote: > > > > > > > > пт, 20 дек. 2019 г. в 22:39, Lukas Tribus : > >> > >> Hello Ilya, > >> > >> > >> > >> sorry about the delay ... > >> > >> > >> On Wed, 27 Nov 2019 at 07:11, Илья Шипицин > wrote: > >> > > >> > -#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) > >> > +#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) || > defined(OPENSSL_NO_DEPRECATED) > >> > [...] > >> > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) > >> > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) > && !defined(OPENSSL_NO_DEPRECATED) > no idea what patch this is, but OPENSSL_NO_DEPRECATED should not be > used anywhere. Always use OPENSSL_API_COMPAT. > >> > [...] > >> > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) > >> > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) > && !defined(OPENSSL_NO_DEPRECATED) > >> > [...] > >> > >> I'm confused. This is not required in my environment for the build to > >> succeed and I don't see any reason why HA_OPENSSL_VERSION_NUMBER would > >> be smaller here? Can you elaborate why the HA_OPENSSL_VERSION_NUMBER > >> comparison would fail to do its job in those comparisons? > > > > > > what is the lowest openssl we support ? > > > > those callbacks are required if threads are used for non-deprecated > builds and for early openssl versions like 1.0.0 > >> > >> > >> The X509_getm_ issue has been fixed by Rosen's patch [1], which is > >> committed and backported. > >> > >> SSL_CTX_set_ecdh_auto issue is fixed by your patch (additional guard > >> in ssl_sock.c) or by removing the existing guards and defining the > >> same compatibility macro openssl uses [2] (as per the input from Willy > >> and Emmanuel): > >> > >> #ifndef SSL_CTX_set_ecdh_auto > >> #define SSL_CTX_set_ecdh_auto(dummy, onoff) ((onoff) != 0) > >> #endif > >> > >> I'd prefer the latter, which is what OpenSSL uses (when not using > >> no-deprecated) and does not pollute the ssl_sock.c. > > > > > > that's just perfect > > > >> > >> > >> Everything builds just fine after that for me (both master and 2.0), > >> without any warnings. I also tried with threading disabled > >> (USE_THREAD=). > >> > >> I will be sending the single SSL_CTX_set_ecdh_auto() fix shortly. Let > >> me know what you think and if you believe something is missing for > >> no-deprecated compatibility. > >> > >> > >> FYI: to avoid rebuilding openssl each time with and without > >> no-deprecate option, the same can be achieved when building haproxy by > >> adding DEFINE="-DOPENSSL_API_COMPAT=0x1010L > >> -DOPENSSL_NO_DEPRECATED" to the make command (maybe this can be useful > >> in CI - I don't know anything about that). > I think we can take Lukas approach "not to reinvent the wheel, but take one invented by openssl itself" https://github.com/openssl/openssl/blob/bf4006a6f9be691ba6eef0e8629e63369a033ccf/include/openssl/crypto.h#L242-L246 :) thank for the hint > > > > > > yep, I'll have a look at that and will send patch for CI > > > >> > >> > >> Once we agree on a fix and commit it, we should definitely add a CI > >> build testing this (with openssl 1.1.1). I disagree to test the build > >> against openssl master, because the API may continually change during > >> development (I mentioned this point in another conversation but I > >> don't recall whether it was on ML or GH). > >> > >> > >> > >> thanks, > >> lukas > >> > >> > >> [1] > https://github.com/haproxy/haproxy/commit/b3814c2ca8a8c28a890f8f50e0a35d5247222a12 > >> [2] > https://github.com/openssl/openssl/blob/bf4006a6f9be691ba6eef0e8629e63369a033ccf/include/openssl/ssl.h#L1480 >
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
On Fri, Dec 20, 2019 at 10:54 AM Илья Шипицин wrote: > > > > пт, 20 дек. 2019 г. в 22:39, Lukas Tribus : >> >> Hello Ilya, >> >> >> >> sorry about the delay ... >> >> >> On Wed, 27 Nov 2019 at 07:11, Илья Шипицин wrote: >> > >> > -#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) >> > +#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) || >> > defined(OPENSSL_NO_DEPRECATED) >> > [...] >> > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) >> > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) && >> > !defined(OPENSSL_NO_DEPRECATED) no idea what patch this is, but OPENSSL_NO_DEPRECATED should not be used anywhere. Always use OPENSSL_API_COMPAT. >> > [...] >> > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) >> > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) && >> > !defined(OPENSSL_NO_DEPRECATED) >> > [...] >> >> I'm confused. This is not required in my environment for the build to >> succeed and I don't see any reason why HA_OPENSSL_VERSION_NUMBER would >> be smaller here? Can you elaborate why the HA_OPENSSL_VERSION_NUMBER >> comparison would fail to do its job in those comparisons? > > > what is the lowest openssl we support ? > > those callbacks are required if threads are used for non-deprecated builds > and for early openssl versions like 1.0.0 >> >> >> The X509_getm_ issue has been fixed by Rosen's patch [1], which is >> committed and backported. >> >> SSL_CTX_set_ecdh_auto issue is fixed by your patch (additional guard >> in ssl_sock.c) or by removing the existing guards and defining the >> same compatibility macro openssl uses [2] (as per the input from Willy >> and Emmanuel): >> >> #ifndef SSL_CTX_set_ecdh_auto >> #define SSL_CTX_set_ecdh_auto(dummy, onoff) ((onoff) != 0) >> #endif >> >> I'd prefer the latter, which is what OpenSSL uses (when not using >> no-deprecated) and does not pollute the ssl_sock.c. > > > that's just perfect > >> >> >> Everything builds just fine after that for me (both master and 2.0), >> without any warnings. I also tried with threading disabled >> (USE_THREAD=). >> >> I will be sending the single SSL_CTX_set_ecdh_auto() fix shortly. Let >> me know what you think and if you believe something is missing for >> no-deprecated compatibility. >> >> >> FYI: to avoid rebuilding openssl each time with and without >> no-deprecate option, the same can be achieved when building haproxy by >> adding DEFINE="-DOPENSSL_API_COMPAT=0x1010L >> -DOPENSSL_NO_DEPRECATED" to the make command (maybe this can be useful >> in CI - I don't know anything about that). > > > yep, I'll have a look at that and will send patch for CI > >> >> >> Once we agree on a fix and commit it, we should definitely add a CI >> build testing this (with openssl 1.1.1). I disagree to test the build >> against openssl master, because the API may continually change during >> development (I mentioned this point in another conversation but I >> don't recall whether it was on ML or GH). >> >> >> >> thanks, >> lukas >> >> >> [1] >> https://github.com/haproxy/haproxy/commit/b3814c2ca8a8c28a890f8f50e0a35d5247222a12 >> [2] >> https://github.com/openssl/openssl/blob/bf4006a6f9be691ba6eef0e8629e63369a033ccf/include/openssl/ssl.h#L1480
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
пт, 20 дек. 2019 г. в 22:39, Lukas Tribus : > Hello Ilya, > > > > sorry about the delay ... > > > On Wed, 27 Nov 2019 at 07:11, Илья Шипицин wrote: > > > > -#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) > > +#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) || > defined(OPENSSL_NO_DEPRECATED) > > [...] > > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) > > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) && > !defined(OPENSSL_NO_DEPRECATED) > > [...] > > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) > > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) && > !defined(OPENSSL_NO_DEPRECATED) > > [...] > > I'm confused. This is not required in my environment for the build to > succeed and I don't see any reason why HA_OPENSSL_VERSION_NUMBER would > be smaller here? Can you elaborate why the HA_OPENSSL_VERSION_NUMBER > comparison would fail to do its job in those comparisons? > what is the lowest openssl we support ? those callbacks are required if threads are used for non-deprecated builds and for early openssl versions like 1.0.0 > > The X509_getm_ issue has been fixed by Rosen's patch [1], which is > committed and backported. > > SSL_CTX_set_ecdh_auto issue is fixed by your patch (additional guard > in ssl_sock.c) or by removing the existing guards and defining the > same compatibility macro openssl uses [2] (as per the input from Willy > and Emmanuel): > > #ifndef SSL_CTX_set_ecdh_auto > #define SSL_CTX_set_ecdh_auto(dummy, onoff) ((onoff) != 0) > #endif > > I'd prefer the latter, which is what OpenSSL uses (when not using > no-deprecated) and does not pollute the ssl_sock.c. > that's just perfect > > Everything builds just fine after that for me (both master and 2.0), > without any warnings. I also tried with threading disabled > (USE_THREAD=). > > I will be sending the single SSL_CTX_set_ecdh_auto() fix shortly. Let > me know what you think and if you believe something is missing for > no-deprecated compatibility. > > > FYI: to avoid rebuilding openssl each time with and without > no-deprecate option, the same can be achieved when building haproxy by > adding DEFINE="-DOPENSSL_API_COMPAT=0x1010L > -DOPENSSL_NO_DEPRECATED" to the make command (maybe this can be useful > in CI - I don't know anything about that). > yep, I'll have a look at that and will send patch for CI > > Once we agree on a fix and commit it, we should definitely add a CI > build testing this (with openssl 1.1.1). I disagree to test the build > against openssl master, because the API may continually change during > development (I mentioned this point in another conversation but I > don't recall whether it was on ML or GH). > > > > thanks, > lukas > > > [1] > https://github.com/haproxy/haproxy/commit/b3814c2ca8a8c28a890f8f50e0a35d5247222a12 > [2] > https://github.com/openssl/openssl/blob/bf4006a6f9be691ba6eef0e8629e63369a033ccf/include/openssl/ssl.h#L1480 >
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
Hello Ilya, sorry about the delay ... On Wed, 27 Nov 2019 at 07:11, Илья Шипицин wrote: > > -#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) > +#if (HA_OPENSSL_VERSION_NUMBER >= 0x101fL) || > defined(OPENSSL_NO_DEPRECATED) > [...] > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) && > !defined(OPENSSL_NO_DEPRECATED) > [...] > -#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) > +#if defined(USE_THREAD) && (HA_OPENSSL_VERSION_NUMBER < 0x1010L) && > !defined(OPENSSL_NO_DEPRECATED) > [...] I'm confused. This is not required in my environment for the build to succeed and I don't see any reason why HA_OPENSSL_VERSION_NUMBER would be smaller here? Can you elaborate why the HA_OPENSSL_VERSION_NUMBER comparison would fail to do its job in those comparisons? The X509_getm_ issue has been fixed by Rosen's patch [1], which is committed and backported. SSL_CTX_set_ecdh_auto issue is fixed by your patch (additional guard in ssl_sock.c) or by removing the existing guards and defining the same compatibility macro openssl uses [2] (as per the input from Willy and Emmanuel): #ifndef SSL_CTX_set_ecdh_auto #define SSL_CTX_set_ecdh_auto(dummy, onoff) ((onoff) != 0) #endif I'd prefer the latter, which is what OpenSSL uses (when not using no-deprecated) and does not pollute the ssl_sock.c. Everything builds just fine after that for me (both master and 2.0), without any warnings. I also tried with threading disabled (USE_THREAD=). I will be sending the single SSL_CTX_set_ecdh_auto() fix shortly. Let me know what you think and if you believe something is missing for no-deprecated compatibility. FYI: to avoid rebuilding openssl each time with and without no-deprecate option, the same can be achieved when building haproxy by adding DEFINE="-DOPENSSL_API_COMPAT=0x1010L -DOPENSSL_NO_DEPRECATED" to the make command (maybe this can be useful in CI - I don't know anything about that). Once we agree on a fix and commit it, we should definitely add a CI build testing this (with openssl 1.1.1). I disagree to test the build against openssl master, because the API may continually change during development (I mentioned this point in another conversation but I don't recall whether it was on ML or GH). thanks, lukas [1] https://github.com/haproxy/haproxy/commit/b3814c2ca8a8c28a890f8f50e0a35d5247222a12 [2] https://github.com/openssl/openssl/blob/bf4006a6f9be691ba6eef0e8629e63369a033ccf/include/openssl/ssl.h#L1480
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
Hi,
> Le 27 nov. 2019 à 03:46, Willy Tarreau a écrit :
>
>> @@ -5046,7 +5046,9 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf,
>> struct ssl_bind_conf *ssl_
>> NULL);
>>
>> if (ecdhe == NULL) {
>> +#if defined(SSL_CTX_set_ecdh_auto)
>> (void)SSL_CTX_set_ecdh_auto(ctx, 1);
>> +#endif
>> return cfgerr;
>> }
>> #else
>
> Here, in order to avoid the pollution caused by too many ifdefs, I'd
> instead put this into openssl-compat.h:
>
> +#if !defined(SSL_CTX_set_ecdh_auto)
> +#define SSL_CTX_set_ecdh_auto(a,b) 0
> +#endif
>
> As long as we can keep all such changes limited, we could imagine
> backporting them, that's great!
>
To be suitable with deprecated version of libssl* should be:
#define SSL_CTX_set_ecdh_auto(ctx, onoff) ((onoff) != 0)
or
#define SSL_CTX_set_ecdh_auto(ctx, onoff) 1
++
Manu
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
ср, 27 нояб. 2019 г. в 05:02, Lukas Tribus : > Hello, > > On Tue, Nov 26, 2019 at 10:50 PM Илья Шипицин > wrote: > > > > Hello, > > > > I resolved `CRYPTO_set_id_callback', `ERR_remove_state', > `SSL_CTX_set_ecdh_auto' issues. > > > > > > the following two will be addressed later: `X509_get_notBefore', > `X509_get_notAfter' > > I'm not sure if matching OPENSSL_NO_DEPRECATED is a good idea, for > compatibility reasons (what's deprecated in one release isn't in > another). It is also not immediately clear to me if those old code > paths are needed by older openssl or not (or only libressl)? > > > Please allow *ample* time to review and test these patches before commit. > sure. I will send v2 after your feedback (also, I'm ok with notes from Tim and Willy (regarding changing commit message and moving 'SSL_CTX_set_ecdh_auto'' to openssl_compat.h) > > > > Thanks, > > Lukas >
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
On Wed, Nov 27, 2019 at 01:02:25AM +0100, Lukas Tribus wrote: > Hello, > > On Tue, Nov 26, 2019 at 10:50 PM ??? wrote: > > > > Hello, > > > > I resolved `CRYPTO_set_id_callback', `ERR_remove_state', > > `SSL_CTX_set_ecdh_auto' issues. > > > > > > the following two will be addressed later: `X509_get_notBefore', > > `X509_get_notAfter' > > I'm not sure if matching OPENSSL_NO_DEPRECATED is a good idea, for > compatibility reasons (what's deprecated in one release isn't in > another). It is also not immediately clear to me if those old code > paths are needed by older openssl or not (or only libressl)? It's a very good point. Somewhere else in this discussion there was a suggestion about using OPENSSL_API_VERSION (or something like this). It might be safer. > Please allow *ample* time to review and test these patches before commit. Noted, thanks! Willy
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
Hi Ilya,
On Wed, Nov 27, 2019 at 02:50:18AM +0500, ??? wrote:
> Hello,
>
> I resolved `CRYPTO_set_id_callback', `ERR_remove_state',
> `SSL_CTX_set_ecdh_auto' issues.
Great, thanks!
I'm seeing some minor cosmetic details:
> @@ -5046,7 +5046,9 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf,
> struct ssl_bind_conf *ssl_
>NULL);
>
> if (ecdhe == NULL) {
> +#if defined(SSL_CTX_set_ecdh_auto)
> (void)SSL_CTX_set_ecdh_auto(ctx, 1);
> +#endif
> return cfgerr;
> }
> #else
Here, in order to avoid the pollution caused by too many ifdefs, I'd
instead put this into openssl-compat.h:
+#if !defined(SSL_CTX_set_ecdh_auto)
+#define SSL_CTX_set_ecdh_auto(a,b) 0
+#endif
As long as we can keep all such changes limited, we could imagine
backporting them, that's great!
Thanks,
Willy
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
Hello, On Tue, Nov 26, 2019 at 10:50 PM Илья Шипицин wrote: > > Hello, > > I resolved `CRYPTO_set_id_callback', `ERR_remove_state', > `SSL_CTX_set_ecdh_auto' issues. > > > the following two will be addressed later: `X509_get_notBefore', > `X509_get_notAfter' I'm not sure if matching OPENSSL_NO_DEPRECATED is a good idea, for compatibility reasons (what's deprecated in one release isn't in another). It is also not immediately clear to me if those old code paths are needed by older openssl or not (or only libressl)? Please allow *ample* time to review and test these patches before commit. Thanks, Lukas
Re: PATCH: partially fix build if OpenSSL is built with no-deprecated option
Ilya, I can't comment about the patch itself, but wanted to comment about the commit message. Am 26.11.19 um 22:50 schrieb Илья Шипицин: > Subject: [PATCH] CLEANUP: partially resolve #367 I believe this should be the 'BUILD' tag instead of CLEANUP. And I also recommend to give a more descriptive subject line and move the issue number into the body. This makes it easier to understand what the patch is about without needing to reference the issue tracker. May I suggest: BUILD: Fix build for OpenSSL built with no-deprecated switch > if OpenSSL is built with no-deprecated mode, some functions are not available. > however, we keep those functions for LibreSSL when appropriate Best regards Tim Düsterhus
