Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Willy Tarreau]

On Wed, Jul 19, 2017 at 04:15:49PM +0200, Emmanuel Hocdet wrote:
> >> 
> >> src/ssl_sock.c: In function 'ssl_sock_load_cert_chain_file':
> >> src/ssl_sock.c:3038:20: error: 'TLSEXT_signature_anonymous' undeclared 
> >> (first use in this function)
> >> src/ssl_sock.c:3038:20: note: each undeclared identifier is reported only 
> >> once for each function it appears in
> >> src/ssl_sock.c:3063:14: error: 'TLSEXT_signature_rsa' undeclared (first 
> >> use in this function)
> >> src/ssl_sock.c:3066:14: error: 'TLSEXT_signature_ecdsa' undeclared (first 
> >> use in this function)
> >> /g/public/linux/master/x86_64-gcc47_glibc218-linux-gnu-gcc -Iinclude 
> >> -Iebtree -Wall -pg -O0 -g -fno-strict-aliasing -Wdeclaration-after-statemen
> >> 
> >> I think this is minor considering that you added an argument, probably
> >> you can simply "#ifndef x /#define x 0" on it. Could you please have a
> >> look ?
> >> 
> > 
> > Of course!
> > It's not a big problem, i will simply drop this information because is not 
> > used in this context.
> > 
> ... or set missing define:

That fixed it, so I merged it, thank you! I always forget about this
openssl-compat file, it significantly eases compatibility porting.

Willy

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emmanuel Hocdet]

Le 19 juil. 2017 à 15:37, Emmanuel Hocdet  a écrit :Le 19 juil. 2017 à 14:54, Willy Tarreau  a écrit :Hi guys,On Wed, Jul 12, 2017 at 03:36:24PM +0200, Emeric Brun wrote:Same worries, the openssl 0.9.8 is still maintained in redhat 5 so we shouldbe able to compile with this version.OK so I checked and this patch is OK with 0.9.8zh, 1.0.0t, 1.0.1u and 1.0.2k,so I merged it.Thanks!However Manu, the following patch broke 0.9.8 and 1.0.0 :commit 0594211987351eaf521577b798a3a461b043710cAuthor: Emmanuel Hocdet Date:   Mon Feb 20 16:11:50 2017 +0100  MEDIUM: boringssl: support native multi-cert selection without bundling  This patch used boringssl's callback to analyse CLientHello before any  handshake to extract key signature capabilities.  Certificat with better signature (ECDSA before RSA) is choosed  transparenty, if client can support it. RSA and ECDSA certificates can  be declare in a row (without order). This makes it possible to set  different ssl and filter parameter with crt-list.src/ssl_sock.c: In function 'ssl_sock_load_cert_chain_file':src/ssl_sock.c:3038:20: error: 'TLSEXT_signature_anonymous' undeclared (first use in this function)src/ssl_sock.c:3038:20: note: each undeclared identifier is reported only once for each function it appears insrc/ssl_sock.c:3063:14: error: 'TLSEXT_signature_rsa' undeclared (first use in this function)src/ssl_sock.c:3066:14: error: 'TLSEXT_signature_ecdsa' undeclared (first use in this function)/g/public/linux/master/x86_64-gcc47_glibc218-linux-gnu-gcc -Iinclude -Iebtree -Wall -pg -O0 -g -fno-strict-aliasing -Wdeclaration-after-statemenI think this is minor considering that you added an argument, probablyyou can simply "#ifndef x /#define x 0" on it. Could you please have alook ?Of course!It’s not a big problem, i will simply drop this information because is not used in this context.… or set missing define:

0001-BUILD-ssl-fix-compatibility-with-openssl-without-TLS.patch
Description: Binary data
++Manu

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emmanuel Hocdet]

> Le 19 juil. 2017 à 14:54, Willy Tarreau  a écrit :
> 
> Hi guys,
> 
> On Wed, Jul 12, 2017 at 03:36:24PM +0200, Emeric Brun wrote:
>> Same worries, the openssl 0.9.8 is still maintained in redhat 5 so we should
>> be able to compile with this version.
> 
> OK so I checked and this patch is OK with 0.9.8zh, 1.0.0t, 1.0.1u and 1.0.2k,
> so I merged it.
> 

Thanks!

> However Manu, the following patch broke 0.9.8 and 1.0.0 :
> 

>  commit 0594211987351eaf521577b798a3a461b043710c
>  Author: Emmanuel Hocdet 
>  Date:   Mon Feb 20 16:11:50 2017 +0100
> 
>MEDIUM: boringssl: support native multi-cert selection without bundling
> 
>This patch used boringssl's callback to analyse CLientHello before any
>handshake to extract key signature capabilities.
>Certificat with better signature (ECDSA before RSA) is choosed
>transparenty, if client can support it. RSA and ECDSA certificates can
>be declare in a row (without order). This makes it possible to set
>different ssl and filter parameter with crt-list.
> 
> src/ssl_sock.c: In function 'ssl_sock_load_cert_chain_file':
> src/ssl_sock.c:3038:20: error: 'TLSEXT_signature_anonymous' undeclared (first 
> use in this function)
> src/ssl_sock.c:3038:20: note: each undeclared identifier is reported only 
> once for each function it appears in
> src/ssl_sock.c:3063:14: error: 'TLSEXT_signature_rsa' undeclared (first use 
> in this function)
> src/ssl_sock.c:3066:14: error: 'TLSEXT_signature_ecdsa' undeclared (first use 
> in this function)
> /g/public/linux/master/x86_64-gcc47_glibc218-linux-gnu-gcc -Iinclude -Iebtree 
> -Wall -pg -O0 -g -fno-strict-aliasing -Wdeclaration-after-statemen
> 
> I think this is minor considering that you added an argument, probably
> you can simply "#ifndef x /#define x 0" on it. Could you please have a
> look ?
> 

Of course!
It’s not a big problem, i will simply drop this information because is not used 
in this context.

++
Manu

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Willy Tarreau]

Hi guys,

On Wed, Jul 12, 2017 at 03:36:24PM +0200, Emeric Brun wrote:
> Same worries, the openssl 0.9.8 is still maintained in redhat 5 so we should
> be able to compile with this version.

OK so I checked and this patch is OK with 0.9.8zh, 1.0.0t, 1.0.1u and 1.0.2k,
so I merged it.

However Manu, the following patch broke 0.9.8 and 1.0.0 :

  commit 0594211987351eaf521577b798a3a461b043710c
  Author: Emmanuel Hocdet 
  Date:   Mon Feb 20 16:11:50 2017 +0100

MEDIUM: boringssl: support native multi-cert selection without bundling

This patch used boringssl's callback to analyse CLientHello before any
handshake to extract key signature capabilities.
Certificat with better signature (ECDSA before RSA) is choosed
transparenty, if client can support it. RSA and ECDSA certificates can
be declare in a row (without order). This makes it possible to set
different ssl and filter parameter with crt-list.

src/ssl_sock.c: In function 'ssl_sock_load_cert_chain_file':
src/ssl_sock.c:3038:20: error: 'TLSEXT_signature_anonymous' undeclared (first 
use in this function)
src/ssl_sock.c:3038:20: note: each undeclared identifier is reported only once 
for each function it appears in
src/ssl_sock.c:3063:14: error: 'TLSEXT_signature_rsa' undeclared (first use in 
this function)
src/ssl_sock.c:3066:14: error: 'TLSEXT_signature_ecdsa' undeclared (first use 
in this function)
/g/public/linux/master/x86_64-gcc47_glibc218-linux-gnu-gcc -Iinclude -Iebtree 
-Wall -pg -O0 -g -fno-strict-aliasing -Wdeclaration-after-statemen

I think this is minor considering that you added an argument, probably
you can simply "#ifndef x /#define x 0" on it. Could you please have a
look ?

Thanks,
Willy

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Willy Tarreau]

On Wed, Jul 12, 2017 at 03:54:28PM +0200, Emmanuel Hocdet wrote:
> Yes i'm confident because i worked a lot to abstract tls version/api support 
> with
> older/newer openssl versions. It's what i do with haproxy's methodVersions 
> table
> for ssl-min/max-ver support.
> What i'm missing is OPENSSL_NO_SSL3 define...

OK.

> > $ grep -rF methodVersions openssl-1.0.2k/
> > $ echo $?
> > 1
> 
> methodVersions is in ssl_sock.c, it will break nothing.

Ah sorry, thank you :-) As you can see I try to be careful these days!

Willy

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emmanuel Hocdet]

> Le 12 juil. 2017 à 15:23, Willy Tarreau  a écrit :
> 
> Hi Manu!
> 
> Please don't forget to CC Emeric and keep in mind that I still don't
> understand anything about openssl, so for me it's always a huge pain
> each time to try to have an opinion on openssl related changes.
> 

oops indeed

> On Wed, Jul 12, 2017 at 02:54:16PM +0200, Emmanuel Hocdet wrote:
>> 
>> Hi Willy,
>> 
>> I would like you consider this patches because Christopher's patch is false 
>> and
>> doesn't support other ssl libs and openssl >= 1.1.0.
> 
> OK so I guess we need to take it. Are you confident that it doesn't break
> older versions ? I'm asking because since we started to add support for
> openssl derivatives, we've probably had as many patches to fix build with
> them as patches needed to fix the build with openssl due to these patches,
> to the point that sometimes I'm wondering why we still make so many efforts
> supporting these libs given the amount of incompatibilities they cause :-(
> 

Yes i’m confident because i worked a lot to abstract tls version/api support 
with
older/newer openssl versions. It’s what i do with haproxy’s methodVersions table
for ssl-min/max-ver support.
What i’m missing is OPENSSL_NO_SSL3 define...


>> I sent my original patch with more comments and another with a little 
>> cleanup:
> 
> This one will definitely break :
> 
> Subject: [PATCH 2/2] MINOR: ssl: remove an unecessary SSL_OP_NO_* dependancy
> 
> Use methodVersions table to display "OpenSSL library supports".
> (...)
> - memprintf(&ptr, "%s\nOpenSSL library supports : "
> -#if SSL_OP_NO_SSLv3
> -   "SSLv3 "
> -#endif
> -#if SSL_OP_NO_TLSv1
> -   "TLSv1.0 "
> -#endif
> -#if SSL_OP_NO_TLSv1_1
> -   "TLSv1.1 "
> -#endif
> -#if SSL_OP_NO_TLSv1_2
> -   "TLSv1.2 "
> -#endif
> -#if SSL_OP_NO_TLSv1_3
> -   "TLSv1.3"
> -#endif
> -"", ptr);
> + memprintf(&ptr, "%s\nOpenSSL library supports :", ptr);
> + for (i = CONF_TLSV_MIN; i <= CONF_TLSV_MAX; i++)
> + if (methodVersions[i].option)
> + memprintf(&ptr, "%s %s", ptr, methodVersions[i].name);
> 
> $ grep -rF methodVersions openssl-1.0.2k/
> $ echo $?
> 1

methodVersions is in ssl_sock.c, it will break nothing.

++
Manu

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emeric Brun]

Hi Manu,

On 07/12/2017 03:23 PM, Willy Tarreau wrote:
> Hi Manu!
> 
> Please don't forget to CC Emeric and keep in mind that I still don't
> understand anything about openssl, so for me it's always a huge pain
> each time to try to have an opinion on openssl related changes.
> 
> On Wed, Jul 12, 2017 at 02:54:16PM +0200, Emmanuel Hocdet wrote:
>>
>> Hi Willy,
>>
>> I would like you consider this patches because Christopher's patch is false 
>> and
>> doesn't support other ssl libs and openssl >= 1.1.0.
> 
> OK so I guess we need to take it. Are you confident that it doesn't break
> older versions ? I'm asking because since we started to add support for
> openssl derivatives, we've probably had as many patches to fix build with
> them as patches needed to fix the build with openssl due to these patches,
> to the point that sometimes I'm wondering why we still make so many efforts
> supporting these libs given the amount of incompatibilities they cause :-(
> 
>> I sent my original patch with more comments and another with a little 
>> cleanup:

Same worries, the openssl 0.9.8 is still maintained in redhat 5 so we should be 
able to compile with this version.


> This one will definitely break :
> 
> Subject: [PATCH 2/2] MINOR: ssl: remove an unecessary SSL_OP_NO_* dependancy
> 
> Use methodVersions table to display "OpenSSL library supports".
> (...)
> - memprintf(&ptr, "%s\nOpenSSL library supports : "
> -#if SSL_OP_NO_SSLv3
> -   "SSLv3 "
> -#endif
> -#if SSL_OP_NO_TLSv1
> -   "TLSv1.0 "
> -#endif
> -#if SSL_OP_NO_TLSv1_1
> -   "TLSv1.1 "
> -#endif
> -#if SSL_OP_NO_TLSv1_2
> -   "TLSv1.2 "
> -#endif
> -#if SSL_OP_NO_TLSv1_3
> -   "TLSv1.3"
> -#endif
> -"", ptr);
> + memprintf(&ptr, "%s\nOpenSSL library supports :", ptr);
> + for (i = CONF_TLSV_MIN; i <= CONF_TLSV_MAX; i++)
> + if (methodVersions[i].option)
> + memprintf(&ptr, "%s %s", ptr, methodVersions[i].name);
> 
> $ grep -rF methodVersions openssl-1.0.2k/
> $ echo $?
> 1
> 
> Thanks,
> Willy
> 

R,
Emeric

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Willy Tarreau]

Hi Manu!

Please don't forget to CC Emeric and keep in mind that I still don't
understand anything about openssl, so for me it's always a huge pain
each time to try to have an opinion on openssl related changes.

On Wed, Jul 12, 2017 at 02:54:16PM +0200, Emmanuel Hocdet wrote:
> 
> Hi Willy,
> 
> I would like you consider this patches because Christopher's patch is false 
> and
> doesn't support other ssl libs and openssl >= 1.1.0.

OK so I guess we need to take it. Are you confident that it doesn't break
older versions ? I'm asking because since we started to add support for
openssl derivatives, we've probably had as many patches to fix build with
them as patches needed to fix the build with openssl due to these patches,
to the point that sometimes I'm wondering why we still make so many efforts
supporting these libs given the amount of incompatibilities they cause :-(

> I sent my original patch with more comments and another with a little cleanup:

This one will definitely break :

Subject: [PATCH 2/2] MINOR: ssl: remove an unecessary SSL_OP_NO_* dependancy

Use methodVersions table to display "OpenSSL library supports".
(...)
-   memprintf(&ptr, "%s\nOpenSSL library supports : "
-#if SSL_OP_NO_SSLv3
- "SSLv3 "
-#endif
-#if SSL_OP_NO_TLSv1
- "TLSv1.0 "
-#endif
-#if SSL_OP_NO_TLSv1_1
- "TLSv1.1 "
-#endif
-#if SSL_OP_NO_TLSv1_2
- "TLSv1.2 "
-#endif
-#if SSL_OP_NO_TLSv1_3
- "TLSv1.3"
-#endif
-  "", ptr);
+   memprintf(&ptr, "%s\nOpenSSL library supports :", ptr);
+   for (i = CONF_TLSV_MIN; i <= CONF_TLSV_MAX; i++)
+   if (methodVersions[i].option)
+   memprintf(&ptr, "%s %s", ptr, methodVersions[i].name);

$ grep -rF methodVersions openssl-1.0.2k/
$ echo $?
1

Thanks,
Willy

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emmanuel Hocdet]

Hi Willy,

I would like you consider this patches because Christopher’s patch is false and
doesn’t support other ssl libs and openssl >= 1.1.0.

I sent my original patch with more comments and another with a little cleanup:

++
Manu




0001-BUG-MINOR-ssl-remove-haproxy-SSLv3-support-when-ssl-.patch
Description: Binary data


0002-MINOR-ssl-remove-an-unecessary-SSL_OP_NO_-dependancy.patch
Description: Binary data

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emmanuel Hocdet]

> Le 15 juin 2017 à 16:18, Emmanuel Hocdet  a écrit :
> 
> 
>> Le 15 juin 2017 à 14:37, Willy Tarreau mailto:w...@1wt.eu>> a 
>> écrit :
>> 
>> Hi Manu,
>> 
>> On Thu, Jun 15, 2017 at 02:17:01PM +0200, Emmanuel Hocdet wrote:
>>> The mistake is from commit 5db33cbd "MEDIUM: ssl: ssl_methods 
>>> implementation is
>>> reworked and factored for min/max tlsxx ». I lost the correct #define when 
>>> i rework my
>>> initials patches. This patch will fix that (for all ssl lib without SSLv3):
>> 
>> From 3a013e94bbf93a83a37a73424afbc9916c9a2868 Mon Sep 17 00:00:00 2001
>> From: Emmanuel Hocdet mailto:m...@gandi.net>>
>> Date: Thu, 15 Jun 2017 12:45:28 +0200
>> Subject: [PATCH] BUG/MINOR: ssl: remove haproxy SSLv3 support when ssl lib
>> have no SSLv3
>> 
>> The commit 5db33cbd "MEDIUM: ssl: ssl_methods implementation is reworked and
>> factored for min/max tlsxx" drop this case. OPENSSL_NO_SSL3 is define when
>> ssl lib have no SSLv3 support, set SSL_OP_NO_SSLv3 to 0 makes sure that
>> haproxy is aware of this.
>> ---
>> src/ssl_sock.c | 6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
>> index c3778b7..8940f09 100644
>> --- a/src/ssl_sock.c
>> +++ b/src/ssl_sock.c
>> @@ -1808,6 +1808,10 @@ ssl_sock_generate_certificate(const char *servername, 
>> struct bind_conf *bind_con
>> #ifndef SSL_OP_NO_COMPRESSION   /* needs OpenSSL >= 
>> 0.9.9 */
>> #define SSL_OP_NO_COMPRESSION 0
>> #endif
>> +#ifdef OPENSSL_NO_SSL3  /* SSLv3 support 
>> removed */
>> +#undef  SSL_OP_NO_SSLv3
>> +#define SSL_OP_NO_SSLv3 0
>> +#endif
>> #ifndef SSL_OP_NO_TLSv1_1   /* needs OpenSSL >= 
>> 1.0.1 */
>> #define SSL_OP_NO_TLSv1_1 0
>> #endif
>> @@ -1835,7 +1839,7 @@ typedef enum { SET_CLIENT, SET_SERVER } 
>> set_context_func;
>> 
>> static void ctx_set_SSLv3_func(SSL_CTX *ctx, set_context_func c)
>> {
>> -#if SSL_OP_NO_SSLv3 && !defined(OPENSSL_NO_SSL3_METHOD)
>> +#if SSL_OP_NO_SSLv3
>>  c == SET_SERVER ? SSL_CTX_set_ssl_version(ctx, SSLv3_server_method())
>>  : SSL_CTX_set_ssl_version(ctx, SSLv3_client_method());
>> #endif
>> 
>> 
>> Hmmm, one checks OPENSSL_NO_SSL3 and the other one used to check
>> OPENSSL_NO_SSL3_METHOD, are you certain there's strict equivalence ? Also
>> do you feel sufficiently confident in doing #undef SSL_OP_NO_SSLv3 ? In
>> general I prefer to avoid unsetting what's defined by a lib when it might
>> also condition the way certain macros work.
>> 
>> Willy
>> 
> 
> Long version :) :
> 
> What i see for OPENSSL_NO_SSL3 / OPENSSL_NO_SSL3_METHOD
> . both are defined when ssl lib are build without SSL3 (or not support it)
> . only OPENSSL_NO_SSL3 is present in haproxy before commit 5db33cbd
> what is important in my patch is to know if ssl lib support SSLv3.
> 
openssl 1.0.2k on my debian have only OPENSSL_NO_SSL3, not 
OPENSSL_NO_SSL3_METHOD
(and SSLv3 is really disable)

> SSL_OP_NO_SSLv3 (and other SSL_OP_NO_TLSvX) is option used in ssl_options,
> not used for macro and the unsetting is in haproxy.c after  #includes.
> When ssl lib want to select a method version, it check (in last) ssl_options. 
> If
> SSLv3 is available (via SSL_OP_NO_SSLv3 mask) ssl lib will use SSL3_METHOD.
> (and will fail when ssl lib is build without SSLv3)  -> I think is safe to 
> alter it.
> Perhaps i can used a INTERNAL_OP_NO_SSLv3 set to SSL_OP_NO_SSLv3 or 0 
> if OPENSSL_NO_SSL3 is set. You want it?
> 
> Manu
> 

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emmanuel Hocdet]

> Le 15 juin 2017 à 14:37, Willy Tarreau  a écrit :
> 
> Hi Manu,
> 
> On Thu, Jun 15, 2017 at 02:17:01PM +0200, Emmanuel Hocdet wrote:
>> The mistake is from commit 5db33cbd "MEDIUM: ssl: ssl_methods implementation 
>> is
>> reworked and factored for min/max tlsxx ». I lost the correct #define when i 
>> rework my
>> initials patches. This patch will fix that (for all ssl lib without SSLv3):
> 
> From 3a013e94bbf93a83a37a73424afbc9916c9a2868 Mon Sep 17 00:00:00 2001
> From: Emmanuel Hocdet 
> Date: Thu, 15 Jun 2017 12:45:28 +0200
> Subject: [PATCH] BUG/MINOR: ssl: remove haproxy SSLv3 support when ssl lib
> have no SSLv3
> 
> The commit 5db33cbd "MEDIUM: ssl: ssl_methods implementation is reworked and
> factored for min/max tlsxx" drop this case. OPENSSL_NO_SSL3 is define when
> ssl lib have no SSLv3 support, set SSL_OP_NO_SSLv3 to 0 makes sure that
> haproxy is aware of this.
> ---
> src/ssl_sock.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
> index c3778b7..8940f09 100644
> --- a/src/ssl_sock.c
> +++ b/src/ssl_sock.c
> @@ -1808,6 +1808,10 @@ ssl_sock_generate_certificate(const char *servername, 
> struct bind_conf *bind_con
> #ifndef SSL_OP_NO_COMPRESSION   /* needs OpenSSL >= 
> 0.9.9 */
> #define SSL_OP_NO_COMPRESSION 0
> #endif
> +#ifdef OPENSSL_NO_SSL3  /* SSLv3 support 
> removed */
> +#undef  SSL_OP_NO_SSLv3
> +#define SSL_OP_NO_SSLv3 0
> +#endif
> #ifndef SSL_OP_NO_TLSv1_1   /* needs OpenSSL >= 
> 1.0.1 */
> #define SSL_OP_NO_TLSv1_1 0
> #endif
> @@ -1835,7 +1839,7 @@ typedef enum { SET_CLIENT, SET_SERVER } 
> set_context_func;
> 
> static void ctx_set_SSLv3_func(SSL_CTX *ctx, set_context_func c)
> {
> -#if SSL_OP_NO_SSLv3 && !defined(OPENSSL_NO_SSL3_METHOD)
> +#if SSL_OP_NO_SSLv3
>   c == SET_SERVER ? SSL_CTX_set_ssl_version(ctx, SSLv3_server_method())
>   : SSL_CTX_set_ssl_version(ctx, SSLv3_client_method());
> #endif
> 
> 
> Hmmm, one checks OPENSSL_NO_SSL3 and the other one used to check
> OPENSSL_NO_SSL3_METHOD, are you certain there's strict equivalence ? Also
> do you feel sufficiently confident in doing #undef SSL_OP_NO_SSLv3 ? In
> general I prefer to avoid unsetting what's defined by a lib when it might
> also condition the way certain macros work.
> 
> Willy
> 

Long version :) :

What i see for OPENSSL_NO_SSL3 / OPENSSL_NO_SSL3_METHOD
. both are defined when ssl lib are build without SSL3 (or not support it)
. only OPENSSL_NO_SSL3 is present in haproxy before commit 5db33cbd
what is important in my patch is to know if ssl lib support SSLv3.

SSL_OP_NO_SSLv3 (and other SSL_OP_NO_TLSvX) is option used in ssl_options,
not used for macro and the unsetting is in haproxy.c after  #includes.
When ssl lib want to select a method version, it check (in last) ssl_options. If
SSLv3 is available (via SSL_OP_NO_SSLv3 mask) ssl lib will use SSL3_METHOD.
(and will fail when ssl lib is build without SSLv3)  -> I think is safe to 
alter it.
Perhaps i can used a INTERNAL_OP_NO_SSLv3 set to SSL_OP_NO_SSLv3 or 0 
if OPENSSL_NO_SSL3 is set. You want it?

Manu

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Willy Tarreau]

Hi Manu,

On Thu, Jun 15, 2017 at 02:17:01PM +0200, Emmanuel Hocdet wrote:
> The mistake is from commit 5db33cbd "MEDIUM: ssl: ssl_methods implementation 
> is
> reworked and factored for min/max tlsxx ». I lost the correct #define when i 
> rework my
> initials patches. This patch will fix that (for all ssl lib without SSLv3):

>From 3a013e94bbf93a83a37a73424afbc9916c9a2868 Mon Sep 17 00:00:00 2001
From: Emmanuel Hocdet 
Date: Thu, 15 Jun 2017 12:45:28 +0200
Subject: [PATCH] BUG/MINOR: ssl: remove haproxy SSLv3 support when ssl lib
 have no SSLv3

The commit 5db33cbd "MEDIUM: ssl: ssl_methods implementation is reworked and
factored for min/max tlsxx" drop this case. OPENSSL_NO_SSL3 is define when
ssl lib have no SSLv3 support, set SSL_OP_NO_SSLv3 to 0 makes sure that
haproxy is aware of this.
---
 src/ssl_sock.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index c3778b7..8940f09 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -1808,6 +1808,10 @@ ssl_sock_generate_certificate(const char *servername, 
struct bind_conf *bind_con
 #ifndef SSL_OP_NO_COMPRESSION   /* needs OpenSSL >= 
0.9.9 */
 #define SSL_OP_NO_COMPRESSION 0
 #endif
+#ifdef OPENSSL_NO_SSL3  /* SSLv3 support 
removed */
+#undef  SSL_OP_NO_SSLv3
+#define SSL_OP_NO_SSLv3 0
+#endif
 #ifndef SSL_OP_NO_TLSv1_1   /* needs OpenSSL >= 
1.0.1 */
 #define SSL_OP_NO_TLSv1_1 0
 #endif
@@ -1835,7 +1839,7 @@ typedef enum { SET_CLIENT, SET_SERVER } set_context_func;
 
 static void ctx_set_SSLv3_func(SSL_CTX *ctx, set_context_func c)
 {
-#if SSL_OP_NO_SSLv3 && !defined(OPENSSL_NO_SSL3_METHOD)
+#if SSL_OP_NO_SSLv3
c == SET_SERVER ? SSL_CTX_set_ssl_version(ctx, SSLv3_server_method())
: SSL_CTX_set_ssl_version(ctx, SSLv3_client_method());
 #endif


Hmmm, one checks OPENSSL_NO_SSL3 and the other one used to check
OPENSSL_NO_SSL3_METHOD, are you certain there's strict equivalence ? Also
do you feel sufficiently confident in doing #undef SSL_OP_NO_SSLv3 ? In
general I prefer to avoid unsetting what's defined by a lib when it might
also condition the way certain macros work.

Willy

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emmanuel Hocdet]

> Le 14 juin 2017 à 18:09, Emmanuel Hocdet  a écrit :
> 
> 
>> Le 14 juin 2017 à 16:43, Willy Tarreau  a écrit :
>> 
>> On Wed, Jun 14, 2017 at 03:11:28PM +0200, Christopher Faulet wrote:
>>> Hi,
>>> 
>>> HAProxy compilation fails if OpenSSL 1.0.2 is compiled without the support
>>> of SSLv3 methods (SSL3_server_method and SSL3_client_method). The manpage
>>> SSL_CTX_new(3) specifies that these functions are available if
>>> OPENSSL_NO_SSL3_METHOD is undefined. Here is a fix.
>> 
>> These days I feel like every build fix for one version of openssl breaks
>> another one. We'll quickly need to have something to validate the build
>> on the various configurations, or it'll become a real mess. I already
>> hate it that all openssl forks have significantly diverged to the point
>> of having to cheat on the #ifdefs. I think in the future we'll have to
>> default to reverting patches for non-legacy openssl when they break the
>> legacy one. I'm not claiming it was the case here, just that we really
>> need to be very careful.
>> 
>> Applied, thanks.
>> Willy
>> 
> 
> I agree but it’s really possible to do that with all ssl implementations, 
> versions
> and build with special options like this case?
> 
> In this case, with openssl 1.0.2 build without SSLv3 the  #define 
> SSL_OP_NO_SSLv3
> is not set to 0 (or undef), otherwise it will not break haproxy build.
> Same mistake  in two minor version of LibreSSL (extract from my patch in the 
> mailinglist):
> "SSL_OP_NO_SSLv3 to 0 made that haproxy compilation is aware that SSLv3
> is unsupported by the library. LibreSSL 2.3.0 removes SSlv3 support but
> SSL_OP_NO_SSLv3 is not set to 0 until version 2.3.2 »
> 
> This patch fix the build, but haproxy will continue to supose that SSLv3 is 
> supported.
> 
> I stop here for today, otherwise the headache will soon happen with this heat.
> 

Hi Willy,

The mistake is from commit 5db33cbd "MEDIUM: ssl: ssl_methods implementation is
reworked and factored for min/max tlsxx ». I lost the correct #define when i 
rework my
initials patches. This patch will fix that (for all ssl lib without SSLv3):



0001-BUG-MINOR-ssl-remove-haproxy-SSLv3-support-when-ssl.patch
Description: Binary data

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Emmanuel Hocdet]

> Le 14 juin 2017 à 16:43, Willy Tarreau  a écrit :
> 
> On Wed, Jun 14, 2017 at 03:11:28PM +0200, Christopher Faulet wrote:
>> Hi,
>> 
>> HAProxy compilation fails if OpenSSL 1.0.2 is compiled without the support
>> of SSLv3 methods (SSL3_server_method and SSL3_client_method). The manpage
>> SSL_CTX_new(3) specifies that these functions are available if
>> OPENSSL_NO_SSL3_METHOD is undefined. Here is a fix.
> 
> These days I feel like every build fix for one version of openssl breaks
> another one. We'll quickly need to have something to validate the build
> on the various configurations, or it'll become a real mess. I already
> hate it that all openssl forks have significantly diverged to the point
> of having to cheat on the #ifdefs. I think in the future we'll have to
> default to reverting patches for non-legacy openssl when they break the
> legacy one. I'm not claiming it was the case here, just that we really
> need to be very careful.
> 
> Applied, thanks.
> Willy
> 

I agree but it’s really possible to do that with all ssl implementations, 
versions
and build with special options like this case?

In this case, with openssl 1.0.2 build without SSLv3 the  #define 
SSL_OP_NO_SSLv3
is not set to 0 (or undef), otherwise it will not break haproxy build.
Same mistake  in two minor version of LibreSSL (extract from my patch in the 
mailinglist):
"SSL_OP_NO_SSLv3 to 0 made that haproxy compilation is aware that SSLv3
is unsupported by the library. LibreSSL 2.3.0 removes SSlv3 support but
SSL_OP_NO_SSLv3 is not set to 0 until version 2.3.2 »

This patch fix the build, but haproxy will continue to supose that SSLv3 is 
supported.

I stop here for today, otherwise the headache will soon happen with this heat.

Manu

Re: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Willy Tarreau]

On Wed, Jun 14, 2017 at 03:11:28PM +0200, Christopher Faulet wrote:
> Hi,
> 
> HAProxy compilation fails if OpenSSL 1.0.2 is compiled without the support
> of SSLv3 methods (SSL3_server_method and SSL3_client_method). The manpage
> SSL_CTX_new(3) specifies that these functions are available if
> OPENSSL_NO_SSL3_METHOD is undefined. Here is a fix.

These days I feel like every build fix for one version of openssl breaks
another one. We'll quickly need to have something to validate the build
on the various configurations, or it'll become a real mess. I already
hate it that all openssl forks have significantly diverged to the point
of having to cheat on the #ifdefs. I think in the future we'll have to
default to reverting patches for non-legacy openssl when they break the
legacy one. I'm not claiming it was the case here, just that we really
need to be very careful.

Applied, thanks.
Willy

[PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist for openssl < 1.1.0

[Christopher Faulet]

Hi,

HAProxy compilation fails if OpenSSL 1.0.2 is compiled without the 
support of SSLv3 methods (SSL3_server_method and SSL3_client_method). 
The manpage SSL_CTX_new(3) specifies that these functions are available 
if OPENSSL_NO_SSL3_METHOD is undefined. Here is a fix.


Thanks,
--
Christopher Faulet
>From f8d90c49944a64b153091a6f524dd22db26b8c80 Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Thu, 8 Jun 2017 22:18:52 +0200
Subject: [PATCH] BUG/MINOR: ssl: Be sure that SSLv3 connection methods exist
 for openssl < 1.1.0

For openssl 1.0.2, SSLv3_server_method and SSLv3_client_method are undefined if
OPENSSL_NO_SSL3_METHOD is set. So we must add a check on this macro before using
these functions.
---
 src/ssl_sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index af09cfb..3680515 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -1835,7 +1835,7 @@ typedef enum { SET_CLIENT, SET_SERVER } set_context_func;
 
 static void ctx_set_SSLv3_func(SSL_CTX *ctx, set_context_func c)
 {
-#if SSL_OP_NO_SSLv3
+#if SSL_OP_NO_SSLv3 && !defined(OPENSSL_NO_SSL3_METHOD)
 	c == SET_SERVER ? SSL_CTX_set_ssl_version(ctx, SSLv3_server_method())
 		: SSL_CTX_set_ssl_version(ctx, SSLv3_client_method());
 #endif
-- 
2.9.4