Re: [PATCH] BUG/MINOR: ssl: fix curve setup with LibreSSL

2019-11-24 Thread Willy Tarreau
On Sun, Nov 24, 2019 at 06:20:40PM +0100, Lukas Tribus wrote:
> Since commit 9a1ab08 ("CLEANUP: ssl-sock: use HA_OPENSSL_VERSION_NUMBER
> instead of OPENSSL_VERSION_NUMBER") we restrict LibreSSL to the OpenSSL
> 1.0.1 API, to avoid breaking LibreSSL every minute. We set
> HA_OPENSSL_VERSION_NUMBER to 0x1000107fL if LibreSSL is detected and
> only allow curves to be configured if HA_OPENSSL_VERSION_NUMBER is at
> least 0x1000200fL.
(...)

Thank you Lukas!
Willy



[PATCH] BUG/MINOR: ssl: fix curve setup with LibreSSL

2019-11-24 Thread Lukas Tribus
Since commit 9a1ab08 ("CLEANUP: ssl-sock: use HA_OPENSSL_VERSION_NUMBER
instead of OPENSSL_VERSION_NUMBER") we restrict LibreSSL to the OpenSSL
1.0.1 API, to avoid breaking LibreSSL every minute. We set
HA_OPENSSL_VERSION_NUMBER to 0x1000107fL if LibreSSL is detected and
only allow curves to be configured if HA_OPENSSL_VERSION_NUMBER is at
least 0x1000200fL.

However all relevant LibreSSL releases actually support settings curves,
which is now broken. Fix this by always allowing curve configuration when
using LibreSSL.

Reported on GitHub in issue #366.

Fixes: 9a1ab08 ("CLEANUP: ssl-sock: use HA_OPENSSL_VERSION_NUMBER instead
of OPENSSL_VERSION_NUMBER").
---
 src/ssl_sock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 6513760..4024248 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5030,7 +5030,7 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, 
struct ssl_bind_conf *ssl_
if (ssl_conf_cur)
SSL_CTX_set_alpn_select_cb(ctx, ssl_sock_advertise_alpn_protos, 
ssl_conf_cur);
 #endif
-#if HA_OPENSSL_VERSION_NUMBER >= 0x1000200fL
+#if ((HA_OPENSSL_VERSION_NUMBER >= 0x1000200fL) || 
defined(LIBRESSL_VERSION_NUMBER))
conf_curves = (ssl_conf && ssl_conf->curves) ? ssl_conf->curves : 
bind_conf->ssl_conf.curves;
if (conf_curves) {
if (!SSL_CTX_set1_curves_list(ctx, conf_curves)) {
@@ -8473,7 +8473,7 @@ static int bind_parse_crl_file(char **args, int cur_arg, 
struct proxy *px, struc
 /* parse the "curves" bind keyword keyword */
 static int ssl_bind_parse_curves(char **args, int cur_arg, struct proxy *px, 
struct ssl_bind_conf *conf, char **err)
 {
-#if HA_OPENSSL_VERSION_NUMBER >= 0x1000200fL
+#if ((HA_OPENSSL_VERSION_NUMBER >= 0x1000200fL) || 
defined(LIBRESSL_VERSION_NUMBER))
if (!*args[cur_arg + 1]) {
if (err)
memprintf(err, "'%s' : missing curve suite", 
args[cur_arg]);
-- 
2.7.4




Re: [PATCH] BUG/MINOR: ssl: fix curve setup with LibreSSL

2019-11-24 Thread Lukas Tribus
Hello,

On Sun, Nov 24, 2019 at 6:20 PM Lukas Tribus  wrote:
>
> Since commit 9a1ab08 ("CLEANUP: ssl-sock: use HA_OPENSSL_VERSION_NUMBER
> instead of OPENSSL_VERSION_NUMBER") we restrict LibreSSL to the OpenSSL
> 1.0.1 API, to avoid breaking LibreSSL every minute. We set
> HA_OPENSSL_VERSION_NUMBER to 0x1000107fL if LibreSSL is detected and
> only allow curves to be configured if HA_OPENSSL_VERSION_NUMBER is at
> least 0x1000200fL.
>
> However all relevant LibreSSL releases actually support settings curves,
> which is now broken. Fix this by always allowing curve configuration when
> using LibreSSL.
>
> Reported on GitHub in issue #366.
>
> Fixes: 9a1ab08 ("CLEANUP: ssl-sock: use HA_OPENSSL_VERSION_NUMBER instead
> of OPENSSL_VERSION_NUMBER").

Should be backported to 2.0.


Lukas



Re: [PATCH] BUG/MINOR: ssl: Stop passing dynamic strings as format arguments

2019-11-24 Thread William Lallemand
On Sat, Nov 23, 2019 at 11:52:30PM +0100, Tim Duesterhus wrote:
> gcc complains rightfully:
> 
> src/ssl_sock.c: In function ‘ssl_sock_prepare_all_ctx’:
> src/ssl_sock.c:5507:3: warning: format not a string literal and no format 
> arguments [-Wformat-security]
>ha_warning(errmsg);
>^
> src/ssl_sock.c:5509:3: warning: format not a string literal and no format 
> arguments [-Wformat-security]
>ha_alert(errmsg);
>^
> src/ssl_sock.c: In function ‘cli_io_handler_commit_cert’:
> src/ssl_sock.c:10208:3: warning: format not a string literal and no format 
> arguments [-Wformat-security]
>chunk_appendf(trash, err);
> 
> Introduced in 8b453912ce9a4e1a3b1329efb2af04d1e470852e. Must be backported
> together with that commit.
> ---
>  src/ssl_sock.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
> index bcfa3e712..53f6c3cd2 100644
> --- a/src/ssl_sock.c
> +++ b/src/ssl_sock.c
> @@ -5504,9 +5504,9 @@ int ssl_sock_prepare_all_ctx(struct bind_conf 
> *bind_conf)
>   }
>  
>   if (errcode & ERR_WARN) {
> - ha_warning(errmsg);
> + ha_warning("%s", errmsg);
>   } else if (errcode & ERR_CODE) {
> - ha_alert(errmsg);
> + ha_alert("%s", errmsg);
>   err++;
>   }
>  
> @@ -10205,7 +10205,7 @@ end:
>  
>   chunk_appendf(trash, "\n");
>   if (errcode & ERR_WARN)
> - chunk_appendf(trash, err);
> + chunk_appendf(trash, "%s", err);
>   chunk_appendf(trash, "Success!\n");
>   if (ci_putchk(si_ic(si), trash) == -1)
>   si_rx_room_blk(si);

Merged, Thanks Tim.

I removed the mention to the backport because it's in master only and mustn't
be backported.

-- 
William Lallemand



Why separate git repository for different versions?

2019-11-24 Thread flamesea12
Just wondering why not single git repository and use branches for different 
versions?
Instead:
haproxy-1.8.githaproxy-1.9.githaproxy-2.0.git
use git://git.haproxy.com/haproxy.gitanddevelop branch for current 
developmentand v2.0/v1.9 branches...



[PATCH v2] CLEANUP: ssl: check if a transaction exists once before setting it

2019-11-24 Thread William Dauchy
trivial patch to fix issue #351

Fixes: bc6ca7ccaa72 ("MINOR: ssl/cli: rework 'set ssl cert' as 'set/commit'")
Reported-by: Илья Шипицин 
Signed-off-by: William Dauchy 
---
 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 bcfa3e71..0848dc7c 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -10468,7 +10468,7 @@ static int cli_parse_set_cert(char **args, char 
*payload, struct appctx *appctx,
/* we succeed, we can save the ckchs in the transaction */
 
/* if there wasn't a transaction, update the old ckchs */
-   if (!ckchs_transaction.old_ckchs && !ckchs_transaction.old_ckchs) {
+   if (!ckchs_transaction.old_ckchs) {
ckchs_transaction.old_ckchs = appctx->ctx.ssl.old_ckchs;
ckchs_transaction.path = appctx->ctx.ssl.path;
err = memprintf(, "Transaction created for certificate 
%s!\n", ckchs_transaction.path);
-- 
2.24.0




Re: [PATCH] MINOR: ssl: check transaction path before assigning it

2019-11-24 Thread William Dauchy
On Sun, Nov 24, 2019 at 12:15 AM William Lallemand
 wrote:
> That's a remain of the previous way of doing this, which was done with an
> array of 2 old_ckch, so the previous check was something like:
>
> >  if (!old_ckchs[0] && !old_ckchs[1])
>
> When a transaction is created the old_ckchs and the path are set.
> So we can check the path OR the old_ckchs, but it's not needed to do both,
> since there are both set and free during a transaction at the same time.
>
> The goal there was to check if a transaction exists, not to check if we free'd
> the path before setting it.

ok got it, thanks for the explanation. In that case I will send a
cleanup patch shortly, but I let you decide whether it is worth
merging it or wait for another rework to integrate it.
-- 
William