Re: [PATCH] openssl-compat: Fix getm_ defines
On Tue, 17 Dec 2019 at 21:18, Rosen Penev wrote: > > That's why I'm suggesting: > > > > #ifndef X509_getm_notBefore > > #define X509_getm_notBefore X509_get_notBefore > > #define X509_getm_notAfter X509_get_notAfter > > #endif > > > > > > Am I missing something? > Yes you are. A macro is not the same as a declaration. > > Just to be sure I tried your change and it failed. > > That's twice I've had to explain basic C... I guess it's #if (HA_OPENSSL_VERSION_NUMBER < 0x101fL) && (LIBRESSL_VERSION_NUMBER < 0x207fL) || defined(OPENSSL_IS_BORINGSSL) then.
Re: [PATCH] openssl-compat: Fix getm_ defines
On Tue, Dec 17, 2019 at 11:34 AM Lukas Tribus wrote: > > Hello, > > On Mon, 16 Dec 2019 at 20:53, Rosen Penev wrote: > > > Any reason why would not just #ifndef X509_getm_notBefore, testing for > > > what we actually want instead of those backbreaking version > > > assumptions? > > X509_getm_notBefore is a function, not a define. > > A function which needs to be defined somewhere: > https://github.com/openssl/openssl/blob/25d7cd1d69e5d5df9c9f346922a48797baca03b7/include/openssl/x509.h#L658 > > We do exactly the same with X509_get_X509_PUBKEY: > https://github.com/haproxy/haproxy/blob/e9a100e982cd1fac201e6e2862cc666887e202e9/include/common/openssl-compat.h#L101 > > > We do need this code path for boringssl. We previously entered it when > boringssl was used (without being aware of it, because of the very > issue we are trying to fix here), so the build was fine. But if we fix > the issue we have to take care of boringssl also. > > > That's why I'm suggesting: > > #ifndef X509_getm_notBefore > #define X509_getm_notBefore X509_get_notBefore > #define X509_getm_notAfter X509_get_notAfter > #endif > > > Am I missing something? Yes you are. A macro is not the same as a declaration. Just to be sure I tried your change and it failed. That's twice I've had to explain basic C... > > > cheers, > lukas
Re: [PATCH] openssl-compat: Fix getm_ defines
Hello, On Mon, 16 Dec 2019 at 20:53, Rosen Penev wrote: > > Any reason why would not just #ifndef X509_getm_notBefore, testing for > > what we actually want instead of those backbreaking version > > assumptions? > X509_getm_notBefore is a function, not a define. A function which needs to be defined somewhere: https://github.com/openssl/openssl/blob/25d7cd1d69e5d5df9c9f346922a48797baca03b7/include/openssl/x509.h#L658 We do exactly the same with X509_get_X509_PUBKEY: https://github.com/haproxy/haproxy/blob/e9a100e982cd1fac201e6e2862cc666887e202e9/include/common/openssl-compat.h#L101 We do need this code path for boringssl. We previously entered it when boringssl was used (without being aware of it, because of the very issue we are trying to fix here), so the build was fine. But if we fix the issue we have to take care of boringssl also. That's why I'm suggesting: #ifndef X509_getm_notBefore #define X509_getm_notBefore X509_get_notBefore #define X509_getm_notAfter X509_get_notAfter #endif Am I missing something? cheers, lukas
Re: [PATCH] openssl-compat: Fix getm_ defines
вт, 17 дек. 2019 г. в 00:55, Rosen Penev : > On Mon, Dec 16, 2019 at 10:21 AM Илья Шипицин > wrote: > > > > > > > > пн, 16 дек. 2019 г. в 22:40, Rosen Penev : > >> > >> On Mon, Dec 16, 2019 at 4:49 AM Lukas Tribus wrote: > >> > > >> > Hello Rosen, > >> > > >> > > пн, 16 дек. 2019 г. в 12:07, Rosen Penev : > >> > >> > >> > >> LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the > condition > >> > >> always true. Check for the define before checking it. > >> > > >> > I cannot find this in the openssl sources, not in master and not in > >> > the 1.1.1 branch. Please clarify where this is defined. > >> Compile with -Wundef. Missing macros evaluate to 0. > > > > > > I checked haproxy source, it does not use such compiler flag. Any reason > for introducing it ? > > > > if we want to make it first class citizen, maybe we should add it to > proper Makefile ? or to our CI ? > > > > assuming "undefined macros may ACCIDENTLY become equal to 0" scares me > You serious? This is basic C. Undefined macros always evaluate to 0. > > indeed, you're right. I checked both gcc and clang. unfortunately, I neither learned nor used that area of C preprocessor specification before. > -Wundef only warns about it. > > > >> > >> > > >> > The SSL compatibility layer is already complex enough and needs > >> > continuous adjustments, we need to understand the reason for changes > >> > very well. Fast fixes are continually coming back to hunt us. > >> > > >> > > >> > On Mon, 16 Dec 2019 at 08:19, Илья Шипицин > wrote: > >> > > please have a look at https://github.com/haproxy/haproxy/issues/367 > (it still misses germ part, I tried things like you send, but reg-tests > fail. do you have travis-ci passed ?) > >> > > also, there's a patch already sent, Lukas Tribus promised to review > it > >> > > >> > Yeah, this one fell through the cracks. Give me a few days to catch > up. > >> > > >> > Thanks, > >> > Lukas >
Re: [PATCH] openssl-compat: Fix getm_ defines
On Mon, Dec 16, 2019 at 10:21 AM Илья Шипицин wrote: > > > > пн, 16 дек. 2019 г. в 22:40, Rosen Penev : >> >> On Mon, Dec 16, 2019 at 4:49 AM Lukas Tribus wrote: >> > >> > Hello Rosen, >> > >> > > пн, 16 дек. 2019 г. в 12:07, Rosen Penev : >> > >> >> > >> LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the >> > >> condition >> > >> always true. Check for the define before checking it. >> > >> > I cannot find this in the openssl sources, not in master and not in >> > the 1.1.1 branch. Please clarify where this is defined. >> Compile with -Wundef. Missing macros evaluate to 0. > > > I checked haproxy source, it does not use such compiler flag. Any reason for > introducing it ? > > if we want to make it first class citizen, maybe we should add it to proper > Makefile ? or to our CI ? > > assuming "undefined macros may ACCIDENTLY become equal to 0" scares me You serious? This is basic C. Undefined macros always evaluate to 0. -Wundef only warns about it. > >> >> > >> > The SSL compatibility layer is already complex enough and needs >> > continuous adjustments, we need to understand the reason for changes >> > very well. Fast fixes are continually coming back to hunt us. >> > >> > >> > On Mon, 16 Dec 2019 at 08:19, Илья Шипицин wrote: >> > > please have a look at https://github.com/haproxy/haproxy/issues/367 (it >> > > still misses germ part, I tried things like you send, but reg-tests >> > > fail. do you have travis-ci passed ?) >> > > also, there's a patch already sent, Lukas Tribus promised to review it >> > >> > Yeah, this one fell through the cracks. Give me a few days to catch up. >> > >> > Thanks, >> > Lukas
Re: [PATCH] openssl-compat: Fix getm_ defines
On Mon, Dec 16, 2019 at 10:09 AM Lukas Tribus wrote: > > On Mon, 16 Dec 2019 at 19:00, Илья Шипицин wrote: > > > > > > > > пн, 16 дек. 2019 г. в 22:42, Rosen Penev : > >> > >> LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition > >> always true. Check for the define before checking it. > >> > >> Signed-off-by: Rosen Penev > >> --- > >> include/common/openssl-compat.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/include/common/openssl-compat.h > >> b/include/common/openssl-compat.h > >> index 25102fbe3..c5029d133 100644 > >> --- a/include/common/openssl-compat.h > >> +++ b/include/common/openssl-compat.h > >> @@ -278,7 +278,7 @@ static inline void EVP_PKEY_up_ref(EVP_PKEY *pkey) > >> #define TLSEXT_signature_ecdsa 3 > >> #endif > >> > >> -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < > >> 0x2070L) > >> +#if (HA_OPENSSL_VERSION_NUMBER < 0x101fL) && (LIBRESSL_VERSION_NUMBER > >> < 0x207fL) > > > > > > assuming "&& (LIBRESSL_VERSION_NUMBER < 0x207fL)" part ... it is only > > relevant for LibreSSL, right ? > > if so, should we leave just second part and omit first ? > > Any reason why would not just #ifndef X509_getm_notBefore, testing for > what we actually want instead of those backbreaking version > assumptions? X509_getm_notBefore is a function, not a define. > > > Lukas
Re: [PATCH] openssl-compat: Fix getm_ defines
пн, 16 дек. 2019 г. в 22:40, Rosen Penev : > On Mon, Dec 16, 2019 at 4:49 AM Lukas Tribus wrote: > > > > Hello Rosen, > > > > > пн, 16 дек. 2019 г. в 12:07, Rosen Penev : > > >> > > >> LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the > condition > > >> always true. Check for the define before checking it. > > > > I cannot find this in the openssl sources, not in master and not in > > the 1.1.1 branch. Please clarify where this is defined. > Compile with -Wundef. Missing macros evaluate to 0. > I checked haproxy source, it does not use such compiler flag. Any reason for introducing it ? if we want to make it first class citizen, maybe we should add it to proper Makefile ? or to our CI ? assuming "undefined macros may ACCIDENTLY become equal to 0" scares me > > > > The SSL compatibility layer is already complex enough and needs > > continuous adjustments, we need to understand the reason for changes > > very well. Fast fixes are continually coming back to hunt us. > > > > > > On Mon, 16 Dec 2019 at 08:19, Илья Шипицин wrote: > > > please have a look at https://github.com/haproxy/haproxy/issues/367 > (it still misses germ part, I tried things like you send, but reg-tests > fail. do you have travis-ci passed ?) > > > also, there's a patch already sent, Lukas Tribus promised to review it > > > > Yeah, this one fell through the cracks. Give me a few days to catch up. > > > > Thanks, > > Lukas >
Re: [PATCH] openssl-compat: Fix getm_ defines
On Mon, 16 Dec 2019 at 19:00, Илья Шипицин wrote: > > > > пн, 16 дек. 2019 г. в 22:42, Rosen Penev : >> >> LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition >> always true. Check for the define before checking it. >> >> Signed-off-by: Rosen Penev >> --- >> include/common/openssl-compat.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/common/openssl-compat.h >> b/include/common/openssl-compat.h >> index 25102fbe3..c5029d133 100644 >> --- a/include/common/openssl-compat.h >> +++ b/include/common/openssl-compat.h >> @@ -278,7 +278,7 @@ static inline void EVP_PKEY_up_ref(EVP_PKEY *pkey) >> #define TLSEXT_signature_ecdsa 3 >> #endif >> >> -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < >> 0x2070L) >> +#if (HA_OPENSSL_VERSION_NUMBER < 0x101fL) && (LIBRESSL_VERSION_NUMBER < >> 0x207fL) > > > assuming "&& (LIBRESSL_VERSION_NUMBER < 0x207fL)" part ... it is only > relevant for LibreSSL, right ? > if so, should we leave just second part and omit first ? Any reason why would not just #ifndef X509_getm_notBefore, testing for what we actually want instead of those backbreaking version assumptions? Lukas
Re: [PATCH] openssl-compat: Fix getm_ defines
On Mon, Dec 16, 2019 at 10:00 AM Илья Шипицин wrote: > > > > пн, 16 дек. 2019 г. в 22:42, Rosen Penev : >> >> LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition >> always true. Check for the define before checking it. >> >> Signed-off-by: Rosen Penev >> --- >> include/common/openssl-compat.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/common/openssl-compat.h >> b/include/common/openssl-compat.h >> index 25102fbe3..c5029d133 100644 >> --- a/include/common/openssl-compat.h >> +++ b/include/common/openssl-compat.h >> @@ -278,7 +278,7 @@ static inline void EVP_PKEY_up_ref(EVP_PKEY *pkey) >> #define TLSEXT_signature_ecdsa 3 >> #endif >> >> -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < >> 0x2070L) >> +#if (HA_OPENSSL_VERSION_NUMBER < 0x101fL) && (LIBRESSL_VERSION_NUMBER < >> 0x207fL) > > > assuming "&& (LIBRESSL_VERSION_NUMBER < 0x207fL)" part ... it is only > relevant for LibreSSL, right ? > if so, should we leave just second part and omit first ? No. As I said previously, undefined macros evaluate to 0. OpenSSL does not define LIBRESSL_VERSION_NUMBER. > > >> >> #define X509_getm_notBefore X509_get_notBefore >> #define X509_getm_notAfter X509_get_notAfter >> #endif >> -- >> 2.23.0 >> >>
Re: [PATCH] openssl-compat: Fix getm_ defines
пн, 16 дек. 2019 г. в 22:42, Rosen Penev : > LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition > always true. Check for the define before checking it. > > Signed-off-by: Rosen Penev > --- > include/common/openssl-compat.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/common/openssl-compat.h > b/include/common/openssl-compat.h > index 25102fbe3..c5029d133 100644 > --- a/include/common/openssl-compat.h > +++ b/include/common/openssl-compat.h > @@ -278,7 +278,7 @@ static inline void EVP_PKEY_up_ref(EVP_PKEY *pkey) > #define TLSEXT_signature_ecdsa 3 > #endif > > -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < > 0x2070L) > +#if (HA_OPENSSL_VERSION_NUMBER < 0x101fL) && (LIBRESSL_VERSION_NUMBER > < 0x207fL) > assuming "&& (LIBRESSL_VERSION_NUMBER < 0x207fL)" part ... it is only relevant for LibreSSL, right ? if so, should we leave just second part and omit first ? > #define X509_getm_notBefore X509_get_notBefore > #define X509_getm_notAfter X509_get_notAfter > #endif > -- > 2.23.0 > > >
Re: [PATCH] openssl-compat: Fix getm_ defines
On Mon, Dec 16, 2019 at 4:49 AM Lukas Tribus wrote: > > Hello Rosen, > > > пн, 16 дек. 2019 г. в 12:07, Rosen Penev : > >> > >> LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition > >> always true. Check for the define before checking it. > > I cannot find this in the openssl sources, not in master and not in > the 1.1.1 branch. Please clarify where this is defined. Compile with -Wundef. Missing macros evaluate to 0. > > The SSL compatibility layer is already complex enough and needs > continuous adjustments, we need to understand the reason for changes > very well. Fast fixes are continually coming back to hunt us. > > > On Mon, 16 Dec 2019 at 08:19, Илья Шипицин wrote: > > please have a look at https://github.com/haproxy/haproxy/issues/367 (it > > still misses germ part, I tried things like you send, but reg-tests fail. > > do you have travis-ci passed ?) > > also, there's a patch already sent, Lukas Tribus promised to review it > > Yeah, this one fell through the cracks. Give me a few days to catch up. > > Thanks, > Lukas
[PATCH] openssl-compat: Fix getm_ defines
LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition always true. Check for the define before checking it. Signed-off-by: Rosen Penev --- include/common/openssl-compat.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/common/openssl-compat.h b/include/common/openssl-compat.h index 25102fbe3..c5029d133 100644 --- a/include/common/openssl-compat.h +++ b/include/common/openssl-compat.h @@ -278,7 +278,7 @@ static inline void EVP_PKEY_up_ref(EVP_PKEY *pkey) #define TLSEXT_signature_ecdsa 3 #endif -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < 0x2070L) +#if (HA_OPENSSL_VERSION_NUMBER < 0x101fL) && (LIBRESSL_VERSION_NUMBER < 0x207fL) #define X509_getm_notBefore X509_get_notBefore #define X509_getm_notAfter X509_get_notAfter #endif -- 2.23.0
Re: [PATCH] openssl-compat: Fix getm_ defines
Hello Rosen, > пн, 16 дек. 2019 г. в 12:07, Rosen Penev : >> >> LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition >> always true. Check for the define before checking it. I cannot find this in the openssl sources, not in master and not in the 1.1.1 branch. Please clarify where this is defined. The SSL compatibility layer is already complex enough and needs continuous adjustments, we need to understand the reason for changes very well. Fast fixes are continually coming back to hunt us. On Mon, 16 Dec 2019 at 08:19, Илья Шипицин wrote: > please have a look at https://github.com/haproxy/haproxy/issues/367 (it still > misses germ part, I tried things like you send, but reg-tests fail. do you > have travis-ci passed ?) > also, there's a patch already sent, Lukas Tribus promised to review it Yeah, this one fell through the cracks. Give me a few days to catch up. Thanks, Lukas
Re: [PATCH] openssl-compat: Fix getm_ defines
also, BoringSSL fails after applying your patch https://travis-ci.com/chipitsine/haproxy/jobs/267601286 пн, 16 дек. 2019 г. в 12:07, Rosen Penev : > LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition > always true. Check for the define before checking it. > > Signed-off-by: Rosen Penev > --- > include/common/openssl-compat.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/common/openssl-compat.h > b/include/common/openssl-compat.h > index 25102fbe3..8b1630110 100644 > --- a/include/common/openssl-compat.h > +++ b/include/common/openssl-compat.h > @@ -278,7 +278,8 @@ static inline void EVP_PKEY_up_ref(EVP_PKEY *pkey) > #define TLSEXT_signature_ecdsa 3 > #endif > > -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < > 0x2070L) > +#if (OPENSSL_VERSION_NUMBER < 0x1010L) || \ > + (defined(LIBRESSL_VERSION_NUMBER) && (LIBRESSL_VERSION_NUMBER < > 0x2070L)) > #define X509_getm_notBefore X509_get_notBefore > #define X509_getm_notAfter X509_get_notAfter > #endif > -- > 2.23.0 > > >
Re: [PATCH] openssl-compat: Fix getm_ defines
пн, 16 дек. 2019 г. в 12:47, William Lallemand : > Hello Rosen, > > On Sun, Dec 15, 2019 at 11:04:37PM -0800, Rosen Penev wrote: > > -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER > < 0x2070L) > > +#if (OPENSSL_VERSION_NUMBER < 0x1010L) || \ > > + (defined(LIBRESSL_VERSION_NUMBER) && (LIBRESSL_VERSION_NUMBER < > 0x2070L)) > > > > It's probably cleaner to use the HA_OPENSSL_VERSION_NUMBER function and > something like this: > @Rosen Penev there was some discussion whether to trust OPENSSL_VERSION_NUMBER or not. LibreSSL pollutes that macro, it sets it to 2.0.0 so ... new macro was introduced instead: HA_OPENSSL_VERSION_NUMBER https://github.com/haproxy/haproxy/blob/master/include/common/openssl-compat.h#L27-L36 > >#if (HA_OPENSSL_VERSION_NUMBER < 0x101fL) && > (LIBRESSL_VERSION_NUMBER < 0x207fL) > > -- > William Lallemand > >
Re: [PATCH] openssl-compat: Fix getm_ defines
Hello Rosen, On Sun, Dec 15, 2019 at 11:04:37PM -0800, Rosen Penev wrote: > -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < > 0x2070L) > +#if (OPENSSL_VERSION_NUMBER < 0x1010L) || \ > + (defined(LIBRESSL_VERSION_NUMBER) && (LIBRESSL_VERSION_NUMBER < > 0x2070L)) > It's probably cleaner to use the HA_OPENSSL_VERSION_NUMBER function and something like this: #if (HA_OPENSSL_VERSION_NUMBER < 0x101fL) && (LIBRESSL_VERSION_NUMBER < 0x207fL) -- William Lallemand
Re: [PATCH] openssl-compat: Fix getm_ defines
hello, seems OpenWRT guys here :-) please have a look at https://github.com/haproxy/haproxy/issues/367 (it still misses germ part, I tried things like you send, but reg-tests fail. do you have travis-ci passed ?) also, there's a patch already sent, Lukas Tribus promised to review it пн, 16 дек. 2019 г. в 12:07, Rosen Penev : > LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition > always true. Check for the define before checking it. > > Signed-off-by: Rosen Penev > --- > include/common/openssl-compat.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/common/openssl-compat.h > b/include/common/openssl-compat.h > index 25102fbe3..8b1630110 100644 > --- a/include/common/openssl-compat.h > +++ b/include/common/openssl-compat.h > @@ -278,7 +278,8 @@ static inline void EVP_PKEY_up_ref(EVP_PKEY *pkey) > #define TLSEXT_signature_ecdsa 3 > #endif > > -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < > 0x2070L) > +#if (OPENSSL_VERSION_NUMBER < 0x1010L) || \ > + (defined(LIBRESSL_VERSION_NUMBER) && (LIBRESSL_VERSION_NUMBER < > 0x2070L)) > #define X509_getm_notBefore X509_get_notBefore > #define X509_getm_notAfter X509_get_notAfter > #endif > -- > 2.23.0 > > >
[PATCH] openssl-compat: Fix getm_ defines
LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition always true. Check for the define before checking it. Signed-off-by: Rosen Penev --- include/common/openssl-compat.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/common/openssl-compat.h b/include/common/openssl-compat.h index 25102fbe3..8b1630110 100644 --- a/include/common/openssl-compat.h +++ b/include/common/openssl-compat.h @@ -278,7 +278,8 @@ static inline void EVP_PKEY_up_ref(EVP_PKEY *pkey) #define TLSEXT_signature_ecdsa 3 #endif -#if (OPENSSL_VERSION_NUMBER < 0x1010L) || (LIBRESSL_VERSION_NUMBER < 0x2070L) +#if (OPENSSL_VERSION_NUMBER < 0x1010L) || \ + (defined(LIBRESSL_VERSION_NUMBER) && (LIBRESSL_VERSION_NUMBER < 0x2070L)) #define X509_getm_notBefore X509_get_notBefore #define X509_getm_notAfter X509_get_notAfter #endif -- 2.23.0
