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/haproxy@formilux.org/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
