On 03.08.2017 10:07, Willy Tarreau wrote:
Hi Bernard,
I'm CCing Emeric since this affects SSL. I have some comments below.
On Tue, Jul 25, 2017 at 05:03:10PM +0200, Bernard Spil wrote:
On 2017-07-04 10:18, Willy Tarreau wrote:
> On Tue, Jul 04, 2017 at 11:12:20AM +0300, Dmitry Sivachenko wrote:
> > >> https://www.mail-archive.com/[email protected]/msg25819.html
> > >
> > >
> > > Do you know if the patch applies to 1.8 (it was mangled so I didn't try).
> >
> >
> > Sorry, hit reply too fast: no, one chunk fails against 1.8-dev2
> > (the one
> > dealing with #ifdef SSL_CTX_get_tlsext_status_arg, it requires
> > analysis
> > because it is not simple surrounding context change).
>
> OK thanks. Bernard, care to have a look and ensure it works for you ?
>
> Thanks,
> Willy
I've just committed a patch to FreeBSD's ports tree for haproxy-devel
(1.8-dev2). This would be a good candidate to include.
Not sure if attachments work for the mailing-list...
https://svnweb.freebsd.org/ports/head/net/haproxy-devel/files/patch-src_ssl__sock.c
The attachment works, though we only apply git patches (with proper
commit messages explaining the nature of the change and its impact,
please check CONTRIBUTING for this). However for the discussion, your
patch is fine.
--- src/ssl_sock.c.orig 2017-06-02 13:59:51 UTC
+++ src/ssl_sock.c
@@ -56,7 +56,7 @@
#include <openssl/engine.h>
#endif
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL) &&
!defined(LIBRESSL_VERSION_NUMBER)
#include <openssl/async.h>
#endif
(...)
OK so all the ones related to LibreSSL not supporting async should go
into one patch.
@@ -1034,10 +1034,13 @@ static int ssl_sock_load_ocsp(SSL_CTX *c
ocsp = NULL;
#ifndef SSL_CTX_get_tlsext_status_cb
-# define SSL_CTX_get_tlsext_status_cb(ctx, cb) \
- *cb = (void (*) (void))ctx->tlsext_status_cb;
+#ifndef SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB
+#define SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB 128
#endif
+ callback = SSL_CTX_ctrl(ctx, SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB, 0,
callback);
+#else
SSL_CTX_get_tlsext_status_cb(ctx, &callback);
+#endif
if (!callback) {
struct ocsp_cbk_arg *cb_arg = calloc(1, sizeof(*cb_arg));
Here I'm having a problem with this change, but maybe Emeric will tell
me I'm wrong. From what I'm seeing, by hard-coding the callback number,
it will simply be ignored by older openssl versions (look at
SSL_CTX_ctrl()
in ssl/ssl_lib.c). So it will indeed build but break them. In my
opinion
we should avoid doing this and instead proceed like this :
#ifdef SSL_CTX_get_tlsext_status_cb
/* what versions are covered here */
SSL_CTX_get_tlsext_status_cb(ctx, &callback);
#elif defined(SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB)
/* what versions are covered here */
callback = SSL_CTX_ctrl(ctx, SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB, 0,
callback);
#else
/* what versions are covered here */
callback = (void (*) (void))ctx->tlsext_status_cb;
#endif
It presents the benefit of *not* changing what currently works, and of
allowing us to get rid of some of these parts in a future release when
we drop support for older openssl versions.
@@ -1063,7 +1066,10 @@ static int ssl_sock_load_ocsp(SSL_CTX *c
int key_type;
EVP_PKEY *pkey;
-#ifdef SSL_CTX_get_tlsext_status_arg
+#if defined(SSL_CTX_get_tlsext_status_arg) ||
(LIBRESSL_VERSION_NUMBER >= 0x2050100fL)
+#ifndef SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB_ARG
+#define SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB_ARG 129
+#endif
SSL_CTX_ctrl(ctx, SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB_ARG, 0,
&cb_arg);
#else
Here are you sure that libressl on this version supports this specific
arg value even if it's not defined ? It's very possible but I'm a bit
worried about the fact that recently, for the sake of supporting some
of the openssl derivatives, we've started to hide the dust under the
carpet by adding missing defines.
cb_arg = ctx->tlsext_status_arg;
@@ -3403,7 +3409,7 @@ int ssl_sock_load_cert_list_file(char *f
#define SSL_MODE_SMALL_BUFFERS 0
#endif
-#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) &&
!defined(OPENSSL_IS_BORINGSSL)
+#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) &&
!defined(OPENSSL_IS_BORINGSSL) || defined(LIBRESSL_VERSION_NUMBER)
static void ssl_set_SSLv3_func(SSL_CTX *ctx, int is_server)
{
#if SSL_OP_NO_SSLv3
This one seems more related to the drop of SSLv3 support on this lib,
please use a separate patch for it.
I think everything here is reasonable. Regarding the one you have for
1.7 in your tree, I don't know if this is valid or not :
- empty_handshake = !((SSL
*)conn->xprt_ctx)->packet_length;
+ empty_handshake = SSL_state((SSL *)conn->xprt_ctx) ==
SSL_ST_BEFORE;
Emeric tends to spend a lot of time picking the appropriate openssl
functions
so he might have had a good reason for checking the packet length
instead of
checking the state (and the lack of comment is not necessarily an
indication
that there's nothing tricky). So I'll let him double check when he's
back from
holiday in ~10 days.
Thanks!
Willy
Hi Willy, Emeric,
Pitty I missed meeting up with the HAproxy people at Paris EuroBSDcon, I
heard there were people from the project there.
I've attached a patch as per the coding-style.txt requirements. Will
wait for Emeric's response.
Cheers,
Bernard.
From f517a5e26cefc211aef78dcf68d82932553b4fa1 Mon Sep 17 00:00:00 2001
From: Bernard Spil <[email protected]>
Date: Tue, 24 Oct 2017 13:18:44 +0200
Subject: [PATCH] BUG/MEDIUM: ssl: fix build with LibreSSL
LibreSSL has forked from OpenSSL 1.0.1f and has bumped the
OPENSSL_VERSION_NUMBER as defined in openssl/opensslv.h to 2.0.
Tests for later versions of OpenSSL will therefor result in TRUE.
Later versions of LibreSSL added a LIBRESSL_VERSION_NUMBER to the
openssl/include.h header which allows testing for the use of
LibreSSL from the code.
This patch disables parts of the code that are OpenSSL 1.1
specific and are not available in supported LibreSSL versions.
---
src/ssl_sock.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 7b8570c7..1fa864a6 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -56,7 +56,7 @@
#include <openssl/engine.h>
#endif
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined LIBRESSL_VERSION_NUMBER)
#include <openssl/async.h>
#endif
@@ -362,7 +362,7 @@ fail_get:
}
#endif
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined LIBRESSL_VERSION_NUMBER)
/*
* openssl async fd handler
*/
@@ -1044,8 +1044,11 @@ static int ssl_sock_load_ocsp(SSL_CTX *ctx, const char *cert_path)
ocsp = NULL;
#ifndef SSL_CTX_get_tlsext_status_cb
+#ifndef SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB
+#define SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB 128
+#endif
# define SSL_CTX_get_tlsext_status_cb(ctx, cb) \
- *cb = (void (*) (void))ctx->tlsext_status_cb;
+ *cb = SSL_CTX_ctrl(ctx,SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB,0, (void (**)(void))cb)
#endif
SSL_CTX_get_tlsext_status_cb(ctx, &callback);
@@ -1073,7 +1076,10 @@ static int ssl_sock_load_ocsp(SSL_CTX *ctx, const char *cert_path)
int key_type;
EVP_PKEY *pkey;
-#ifdef SSL_CTX_get_tlsext_status_arg
+#if defined(SSL_CTX_get_tlsext_status_arg) || defined(LIBRESSL_VERSION_NUMBER)
+#ifndef SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB_ARG
+#define SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB_ARG 129
+#endif
SSL_CTX_ctrl(ctx, SSL_CTRL_GET_TLSEXT_STATUS_REQ_CB_ARG, 0, &cb_arg);
#else
cb_arg = ctx->tlsext_status_arg;
@@ -3627,7 +3633,7 @@ ssl_sock_initial_ctx(struct bind_conf *bind_conf)
options &= ~SSL_OP_CIPHER_SERVER_PREFERENCE;
SSL_CTX_set_options(ctx, options);
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined LIBRESSL_VERSION_NUMBER)
if (global_ssl.async)
mode |= SSL_MODE_ASYNC;
#endif
@@ -4125,7 +4131,7 @@ int ssl_sock_prepare_srv_ctx(struct server *srv)
options |= SSL_OP_NO_TICKET;
SSL_CTX_set_options(ctx, options);
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined LIBRESSL_VERSION_NUMBER)
if (global_ssl.async)
mode |= SSL_MODE_ASYNC;
#endif
@@ -4638,7 +4644,7 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag)
fd_cant_recv(conn->handle.fd);
return 0;
}
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined LIBRESSL_VERSION_NUMBER)
else if (ret == SSL_ERROR_WANT_ASYNC) {
ssl_async_process_fds(conn, conn->xprt_ctx);
return 0;
@@ -4722,7 +4728,7 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag)
fd_cant_recv(conn->handle.fd);
return 0;
}
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined LIBRESSL_VERSION_NUMBER)
else if (ret == SSL_ERROR_WANT_ASYNC) {
ssl_async_process_fds(conn, conn->xprt_ctx);
return 0;
@@ -4784,7 +4790,7 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag)
reneg_ok:
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if (OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined LIBRESSL_VERSION_NUMBER)
/* ASYNC engine API doesn't support moving read/write
* buffers. So we disable ASYNC mode right after
* the handshake to avoid buffer oveflows.
@@ -4908,7 +4914,7 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun
/* handshake is running, and it needs to enable write */
conn->flags |= CO_FL_SSL_WAIT_HS;
__conn_sock_want_send(conn);
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER)
/* Async mode can be re-enabled, because we're leaving data state.*/
if (global_ssl.async)
SSL_set_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
@@ -4920,7 +4926,7 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun
/* handshake is running, and it may need to re-enable read */
conn->flags |= CO_FL_SSL_WAIT_HS;
__conn_sock_want_recv(conn);
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER)
/* Async mode can be re-enabled, because we're leaving data state.*/
if (global_ssl.async)
SSL_set_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
@@ -5021,7 +5027,7 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl
/* handshake is running, and it may need to re-enable write */
conn->flags |= CO_FL_SSL_WAIT_HS;
__conn_sock_want_send(conn);
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER)
/* Async mode can be re-enabled, because we're leaving data state.*/
if (global_ssl.async)
SSL_set_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
@@ -5036,7 +5042,7 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl
/* handshake is running, and it needs to enable read */
conn->flags |= CO_FL_SSL_WAIT_HS;
__conn_sock_want_recv(conn);
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER)
/* Async mode can be re-enabled, because we're leaving data state.*/
if (global_ssl.async)
SSL_set_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
@@ -5060,7 +5066,7 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl
static void ssl_sock_close(struct connection *conn) {
if (conn->xprt_ctx) {
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL && !defined(LIBRESSL_VERSION_NUMBER)
if (global_ssl.async) {
OSSL_ASYNC_FD all_fd[32], afd;
size_t num_all_fds = 0;
--
2.14.2
