Hi Grant, On 05/16/2017 12:05 PM, Emeric Brun wrote: > Hi Grant, > > On 05/15/2017 08:11 PM, Grant Zhang wrote: >> >>> On May 15, 2017, at 03:14, Emeric Brun <[email protected]> wrote: >>> >>> What does it look like? >> New patches attached. >> >>> >>> The issue is very similar: >>> https://mta.openssl.org/pipermail/openssl-dev/2016-March/005909.html >> Interesting. yeah, it looks similar. >> >> Regards, >> >> Grant >> > > I've re-based your patches on latest master and also adds two fixes (in > attachment). > > All my tests are good for now, so i think Willy could merge them. > > R, > Emeric > >
More fixes, it appears stable now, even if session are closed during handshake. I also added the support of multiple async engines (latest patch: it is limited to 32 engines) I did some tests using my home-made engine (algo EC,DH) + QuickAssist (algo RSA) and I checked it handles 2 async fds on the same session. Thanks for your job! Willy: could you merge them? R, Emeric
>From 3d5a8bbc96e1e6d6f275c29f416cac9e356389fa Mon Sep 17 00:00:00 2001 From: Grant Zhang <[email protected]> Date: Sat, 21 Jan 2017 01:10:18 +0000 Subject: [PATCH 1/7] MEDIUM: ssl: add basic support for OpenSSL crypto engine This patch adds the global 'ssl-engine' keyword. First arg is an engine identifier followed by a list of default_algorithms the engine will operate. If the openssl version is too old, an error is reported when the option is used. --- doc/configuration.txt | 16 ++++++ include/proto/ssl_sock.h | 2 + src/haproxy.c | 4 ++ src/ssl_sock.c | 147 ++++++++++++++++++++++++++++++++++++++++------- 4 files changed, 148 insertions(+), 21 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index edd9e79..ffe9f76 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -591,6 +591,7 @@ The following keywords are supported in the "global" section : - spread-checks - server-state-base - server-state-file + - ssl-engine - tune.buffers.limit - tune.buffers.reserve - tune.bufsize @@ -1265,6 +1266,21 @@ spread-checks <0..50, in percent> and +/- 50%. A value between 2 and 5 seems to show good results. The default value remains at 0. +ssl-engine <name> [algo <comma-seperated list of algorithms>] + Sets the OpenSSL engine to <name>. List of valid values for <name> may be + obtained using the command "openssl engine". This statement may be used + multiple times, it will simply enable multiple crypto engines. Referencing an + unsupported engine will prevent haproxy from starting. Note that many engines + will lead to lower HTTPS performance than pure software with recent + processors. The optional command "algo" sets the default algorithms an ENGINE + will supply using the OPENSSL function ENGINE_set_default_string(). A value + of "ALL" uses the engine for all cryptographic operations. If no list of + algo is specified then the value of "ALL" is used. A comma-seperated list + of different algorithms may be specified, including: RSA, DSA, DH, EC, RAND, + CIPHERS, DIGESTS, PKEY, PKEY_CRYPTO, PKEY_ASN1. This is the same format that + openssl configuration file uses: + https://www.openssl.org/docs/man1.0.2/apps/config.html + tune.buffers.limit <number> Sets a hard limit on the number of buffers which may be allocated per process. The default value is zero which means unlimited. The minimum non-zero value diff --git a/include/proto/ssl_sock.h b/include/proto/ssl_sock.h index 6f779fa..8779770 100644 --- a/include/proto/ssl_sock.h +++ b/include/proto/ssl_sock.h @@ -68,7 +68,9 @@ struct tls_keys_ref *tlskeys_ref_lookupid(int unique_id); #endif #ifndef OPENSSL_NO_DH int ssl_sock_load_global_dh_param_from_file(const char *filename); +void ssl_free_dh(void); #endif +void ssl_free_engines(void); SSL_CTX *ssl_sock_create_cert(struct connection *conn, const char *servername, unsigned int key); SSL_CTX *ssl_sock_get_generated_cert(unsigned int key, struct bind_conf *bind_conf); diff --git a/src/haproxy.c b/src/haproxy.c index 261b213..be1e11d 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -109,6 +109,7 @@ #include <proto/task.h> #include <proto/dns.h> #include <proto/vars.h> +#include <proto/ssl_sock.h> /* list of config files */ static struct list cfg_cfgfiles = LIST_HEAD_INIT(cfg_cfgfiles); @@ -2162,6 +2163,9 @@ int main(int argc, char **argv) for (proc = 0; proc < global.nbproc; proc++) while (waitpid(-1, NULL, 0) == -1 && errno == EINTR); } +#ifndef OPENSSL_NO_DH + ssl_free_dh(); +#endif exit(0); /* parent must leave */ } diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 21d50c7..bad9b99 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -52,6 +52,7 @@ #ifndef OPENSSL_NO_DH #include <openssl/dh.h> #endif +#include <openssl/engine.h> #include <import/lru.h> #include <import/xxhash.h> @@ -207,6 +208,13 @@ static int ssl_capture_ptr_index = -1; struct list tlskeys_reference = LIST_HEAD_INIT(tlskeys_reference); #endif +static unsigned int openssl_engines_initialized; +struct list openssl_engines = LIST_HEAD_INIT(openssl_engines); +struct ssl_engine_list { + struct list list; + ENGINE *e; +}; + #ifndef OPENSSL_NO_DH static int ssl_dh_ptr_index = -1; static DH *global_dh = NULL; @@ -302,6 +310,47 @@ struct ocsp_cbk_arg { }; }; +static int ssl_init_single_engine(const char *engine_id, const char *def_algorithms) +{ + int err_code = ERR_ABORT; + ENGINE *engine; + struct ssl_engine_list *el; + + /* grab the structural reference to the engine */ + engine = ENGINE_by_id(engine_id); + if (engine == NULL) { + Alert("ssl-engine %s: failed to get structural reference\n", engine_id); + goto fail_get; + } + + if (!ENGINE_init(engine)) { + /* the engine couldn't initialise, release it */ + Alert("ssl-engine %s: failed to initialize\n", engine_id); + goto fail_init; + } + + if (ENGINE_set_default_string(engine, def_algorithms) == 0) { + Alert("ssl-engine %s: failed on ENGINE_set_default_string\n", engine_id); + goto fail_set_method; + } + + el = calloc(1, sizeof(*el)); + el->e = engine; + LIST_ADD(&openssl_engines, &el->list); + return 0; + +fail_set_method: + /* release the functional reference from ENGINE_init() */ + ENGINE_finish(engine); + +fail_init: + /* release the structural reference from ENGINE_by_id() */ + ENGINE_free(engine); + +fail_get: + return err_code; +} + /* * This function returns the number of seconds elapsed * since the Epoch, 1970-01-01 00:00:00 +0000 (UTC) and the @@ -6929,6 +6978,48 @@ static int ssl_parse_global_ca_crt_base(char **args, int section_type, struct pr return 0; } +/* parse the "ssl-engine" keyword in global section. + * Returns <0 on alert, >0 on warning, 0 on success. + */ +static int ssl_parse_global_ssl_engine(char **args, int section_type, struct proxy *curpx, + struct proxy *defpx, const char *file, int line, + char **err) +{ + char *algo; + int ret = -1; + + if (*(args[1]) == 0) { + memprintf(err, "global statement '%s' expects a valid engine name as an argument.", args[0]); + return ret; + } + + if (*(args[2]) == 0) { + /* if no list of algorithms is given, it defaults to ALL */ + algo = strdup("ALL"); + goto add_engine; + } + + /* otherwise the expected format is ssl-engine <engine_name> algo <list of algo> */ + if (strcmp(args[2], "algo") != 0) { + memprintf(err, "global statement '%s' expects to have algo keyword.", args[0]); + return ret; + } + + if (*(args[3]) == 0) { + memprintf(err, "global statement '%s' expects algorithm names as an argument.", args[0]); + return ret; + } + algo = strdup(args[3]); + +add_engine: + if (ssl_init_single_engine(args[1], algo)==0) { + openssl_engines_initialized++; + ret = 0; + } + free(algo); + return ret; +} + /* parse the "ssl-default-bind-ciphers" / "ssl-default-server-ciphers" keywords * in global section. Returns <0 on alert, >0 on warning, 0 on success. */ @@ -7537,6 +7628,7 @@ static struct cfg_kw_list cfg_kws = {ILH, { #ifndef OPENSSL_NO_DH { CFG_GLOBAL, "ssl-dh-param-file", ssl_parse_global_dh_param_file }, #endif + { CFG_GLOBAL, "ssl-engine", ssl_parse_global_ssl_engine }, { CFG_GLOBAL, "tune.ssl.cachesize", ssl_parse_global_int }, #ifndef OPENSSL_NO_DH { CFG_GLOBAL, "tune.ssl.default-dh-param", ssl_parse_global_default_dh }, @@ -7610,6 +7702,7 @@ static void __ssl_sock_init(void) srv_register_keywords(&srv_kws); cfg_register_keywords(&cfg_kws); cli_register_kw(&cli_kws); + ENGINE_load_builtin_engines(); #if (defined SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB && TLS_TICKETS_NO > 0) hap_register_post_check(tlskeys_finalize_config); #endif @@ -7671,39 +7764,51 @@ static void __ssl_sock_init(void) #ifndef OPENSSL_NO_DH ssl_dh_ptr_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, NULL); + hap_register_post_deinit(ssl_free_dh); #endif + hap_register_post_deinit(ssl_free_engines); /* Load SSL string for the verbose & debug mode. */ ERR_load_SSL_strings(); } -__attribute__((destructor)) -static void __ssl_sock_deinit(void) -{ -#if (defined SSL_CTRL_SET_TLSEXT_HOSTNAME && !defined SSL_NO_GENERATE_CERTIFICATES) - lru64_destroy(ssl_ctx_lru_tree); -#endif +void ssl_free_engines(void) { + struct ssl_engine_list *wl, *wlb; + /* free up engine list */ + list_for_each_entry_safe(wl, wlb, &openssl_engines, list) { + ENGINE_finish(wl->e); + ENGINE_free(wl->e); + LIST_DEL(&wl->list); + free(wl); + } +} #ifndef OPENSSL_NO_DH - if (local_dh_1024) { - DH_free(local_dh_1024); - local_dh_1024 = NULL; - } - - if (local_dh_2048) { - DH_free(local_dh_2048); - local_dh_2048 = NULL; - } - - if (local_dh_4096) { - DH_free(local_dh_4096); - local_dh_4096 = NULL; - } - +void ssl_free_dh(void) { + if (local_dh_1024) { + DH_free(local_dh_1024); + local_dh_1024 = NULL; + } + if (local_dh_2048) { + DH_free(local_dh_2048); + local_dh_2048 = NULL; + } + if (local_dh_4096) { + DH_free(local_dh_4096); + local_dh_4096 = NULL; + } if (global_dh) { DH_free(global_dh); global_dh = NULL; } +} +#endif + +__attribute__((destructor)) +static void __ssl_sock_deinit(void) +{ +#if (defined SSL_CTRL_SET_TLSEXT_HOSTNAME && !defined SSL_NO_GENERATE_CERTIFICATES) + lru64_destroy(ssl_ctx_lru_tree); #endif ERR_remove_state(0); -- 1.8.3.1
>From 1e88acf0afefaac5e8a20187241db917cb77b84c Mon Sep 17 00:00:00 2001 From: Grant Zhang <[email protected]> Date: Sat, 14 Jan 2017 01:42:15 +0000 Subject: [PATCH 2/7] MAJOR: ssl: add openssl async mode support ssl-mode-async is a global configuration parameter which enables asynchronous processing in OPENSSL for all SSL connections haproxy handles. With SSL_MODE_ASYNC set, TLS I/O operations may indicate a retry with SSL_ERROR_WANT_ASYNC with this mode set if an asynchronous capable engine is used to perform cryptographic operations. Currently async mode only supports one async-capable engine. --- doc/configuration.txt | 5 ++ include/proto/connection.h | 4 ++ include/types/connection.h | 4 ++ include/types/fd.h | 1 + src/ssl_sock.c | 131 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 145 insertions(+) diff --git a/doc/configuration.txt b/doc/configuration.txt index ffe9f76..eebe02f 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -592,6 +592,7 @@ The following keywords are supported in the "global" section : - server-state-base - server-state-file - ssl-engine + - ssl-mode-async - tune.buffers.limit - tune.buffers.reserve - tune.bufsize @@ -1281,6 +1282,10 @@ ssl-engine <name> [algo <comma-seperated list of algorithms>] openssl configuration file uses: https://www.openssl.org/docs/man1.0.2/apps/config.html +ssl-mode-async + Adds SSL_MODE_ASYNC mode to the SSL context. This enables asynchronous TLS + I/O operations if an asynchronous capable SSL engine is used. + tune.buffers.limit <number> Sets a hard limit on the number of buffers which may be allocated per process. The default value is zero which means unlimited. The minimum non-zero value diff --git a/include/proto/connection.h b/include/proto/connection.h index 2380bb8..235671e 100644 --- a/include/proto/connection.h +++ b/include/proto/connection.h @@ -502,6 +502,10 @@ static inline void conn_init(struct connection *conn) conn->target = NULL; conn->proxy_netns = NULL; LIST_INIT(&conn->list); + +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + conn->async_fd = -1; +#endif } /* Tries to allocate a new connection and initialized its main fields. The diff --git a/include/types/connection.h b/include/types/connection.h index 9d1b51a..e19f883 100644 --- a/include/types/connection.h +++ b/include/types/connection.h @@ -303,6 +303,10 @@ struct connection { struct sockaddr_storage from; /* client address, or address to spoof when connecting to the server */ struct sockaddr_storage to; /* address reached by the client, or address to connect to */ } addr; /* addresses of the remote side, client for producer and server for consumer */ + +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + OSSL_ASYNC_FD async_fd; +#endif }; /* proxy protocol v2 definitions */ diff --git a/include/types/fd.h b/include/types/fd.h index 2bd7c07..42808eb 100644 --- a/include/types/fd.h +++ b/include/types/fd.h @@ -100,6 +100,7 @@ struct fdtab { unsigned char updated:1; /* 1 if this fd is already in the update list */ unsigned char linger_risk:1; /* 1 if we must kill lingering before closing */ unsigned char cloned:1; /* 1 if a cloned socket, requires EPOLL_CTL_DEL on close */ + unsigned char async:1; /* 1 if this fd is async ssl fd */ }; /* less often used information */ diff --git a/src/ssl_sock.c b/src/ssl_sock.c index bad9b99..50b3cfe 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -54,6 +54,10 @@ #endif #include <openssl/engine.h> +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL +#include <openssl/async.h> +#endif + #include <import/lru.h> #include <import/xxhash.h> @@ -156,6 +160,7 @@ static struct xprt_ops ssl_sock; static struct { char *crt_base; /* base directory path for certificates */ char *ca_base; /* base directory path for CAs and CRLs */ + int async; /* whether we use ssl async mode */ char *listen_default_ciphers; char *connect_default_ciphers; @@ -351,6 +356,57 @@ fail_get: return err_code; } +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL +void ssl_async_fd_handler(int fd) +{ + struct connection *conn = fdtab[fd].owner; + int conn_fd = conn->t.sock.fd; + + /* crypto engine is available, let's notify the associated + * connection that it can pursue its processing. + */ + conn_fd_handler(conn_fd); +} + +/* + * openssl async fd handler + */ +void ssl_async_process_fds(struct connection *conn) +{ + OSSL_ASYNC_FD add_fd; + OSSL_ASYNC_FD del_fd; + size_t num_add_fds = 0; + size_t num_del_fds = 0; + SSL *ssl = conn->xprt_ctx; + + SSL_get_changed_async_fds(ssl, NULL, &num_add_fds, NULL, + &num_del_fds); + + if (num_add_fds == 0 && num_del_fds == 0) + return; + + /* we don't support more than 1 async fd */ + if (num_add_fds > 1 || num_del_fds > 1) + return; + + SSL_get_changed_async_fds(ssl, &add_fd, &num_add_fds, &del_fd, + &num_del_fds); + + if (num_del_fds) + fd_stop_both(del_fd); + + if (num_add_fds) { + conn->async_fd = add_fd; + fdtab[add_fd].async = 1; + fdtab[add_fd].state = 0; + fdtab[add_fd].owner = conn; + fdtab[add_fd].iocb = ssl_async_fd_handler; + fd_insert(add_fd); + fd_want_recv(add_fd); + } +} +#endif + /* * This function returns the number of seconds elapsed * since the Epoch, 1970-01-01 00:00:00 +0000 (UTC) and the @@ -3404,6 +3460,11 @@ ssl_sock_initial_ctx(struct bind_conf *bind_conf) if (bind_conf->ssl_options & BC_SSL_O_PREF_CLIE_CIPH) options &= ~SSL_OP_CIPHER_SERVER_PREFERENCE; SSL_CTX_set_options(ctx, options); + +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + if (global_ssl.async) + mode |= SSL_MODE_ASYNC; +#endif SSL_CTX_set_mode(ctx, mode); if (global_ssl.life_time) SSL_CTX_set_timeout(ctx, global_ssl.life_time); @@ -3849,6 +3910,11 @@ int ssl_sock_prepare_srv_ctx(struct server *srv) if (srv->ssl_ctx.options & SRV_SSL_O_NO_TLS_TICKETS) options |= SSL_OP_NO_TICKET; SSL_CTX_set_options(ctx, options); + +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + if (global_ssl.async) + mode |= SSL_MODE_ASYNC; +#endif SSL_CTX_set_mode(ctx, mode); srv->ssl_ctx.ctx = ctx; @@ -4337,6 +4403,7 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag) if (ret <= 0) { /* handshake may have not been completed, let's find why */ ret = SSL_get_error(conn->xprt_ctx, ret); + if (ret == SSL_ERROR_WANT_WRITE) { /* SSL handshake needs to write, L4 connection may not be ready */ __conn_sock_stop_recv(conn); @@ -4360,6 +4427,12 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag) fd_cant_recv(conn->t.sock.fd); return 0; } +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + else if (ret == SSL_ERROR_WANT_ASYNC) { + ssl_async_process_fds(conn); + return 0; + } +#endif else if (ret == SSL_ERROR_SYSCALL) { /* if errno is null, then connection was successfully established */ if (!errno && conn->flags & CO_FL_WAIT_L4_CONN) @@ -4438,6 +4511,12 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag) fd_cant_recv(conn->t.sock.fd); return 0; } +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + else if (ret == SSL_ERROR_WANT_ASYNC) { + ssl_async_process_fds(conn); + return 0; + } +#endif else if (ret == SSL_ERROR_SYSCALL) { /* if errno is null, then connection was successfully established */ if (!errno && conn->flags & CO_FL_WAIT_L4_CONN) @@ -4624,6 +4703,12 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun fd_cant_recv(conn->t.sock.fd); break; } +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + else if (ret == SSL_ERROR_WANT_ASYNC) { + ssl_async_process_fds(conn); + break; + } +#endif /* otherwise it's a real error */ goto out_error; } @@ -4708,6 +4793,7 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl } else { ret = SSL_get_error(conn->xprt_ctx, ret); + if (ret == SSL_ERROR_WANT_WRITE) { if (SSL_renegotiate_pending(conn->xprt_ctx)) { /* handshake is running, and it may need to re-enable write */ @@ -4725,6 +4811,12 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl __conn_sock_want_recv(conn); break; } +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + else if (ret == SSL_ERROR_WANT_ASYNC) { + ssl_async_process_fds(conn); + break; + } +#endif goto out_error; } } @@ -4742,6 +4834,17 @@ 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_engines_initialized && conn->async_fd != -1) { + /* the async fd is created and owned by the SSL engine, which is + * responsible for fd closure. Here we are done with the async fd + * thus disable the polling on it, as well as clean up fdtab entry. + */ + fd_stop_both(conn->async_fd); + fdtab[conn->async_fd].async = 0; + fdtab[conn->async_fd].state = 0; + } +#endif SSL_free(conn->xprt_ctx); conn->xprt_ctx = NULL; sslconns--; @@ -6978,6 +7081,32 @@ static int ssl_parse_global_ca_crt_base(char **args, int section_type, struct pr return 0; } +/* parse the "ssl-mode-async" keyword in global section. + * Returns <0 on alert, >0 on warning, 0 on success. + */ +static int ssl_parse_global_ssl_async(char **args, int section_type, struct proxy *curpx, + struct proxy *defpx, const char *file, int line, + char **err) +{ +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + global_ssl.async = 1; + return 0; +#else + memprintf(err, "'%s': openssl library does not support async mode", args[0]); + return -1; +#endif +} + +static int ssl_check_async_engine_count(void) { + int err_code = 0; + + if (global_ssl.async && openssl_engines_initialized!=1) { + Alert("ssl-mode-async only supports one engine\n"); + err_code = ERR_ABORT; + } + return err_code; +} + /* parse the "ssl-engine" keyword in global section. * Returns <0 on alert, >0 on warning, 0 on success. */ @@ -7628,6 +7757,7 @@ static struct cfg_kw_list cfg_kws = {ILH, { #ifndef OPENSSL_NO_DH { CFG_GLOBAL, "ssl-dh-param-file", ssl_parse_global_dh_param_file }, #endif + { CFG_GLOBAL, "ssl-mode-async", ssl_parse_global_ssl_async }, { CFG_GLOBAL, "ssl-engine", ssl_parse_global_ssl_engine }, { CFG_GLOBAL, "tune.ssl.cachesize", ssl_parse_global_int }, #ifndef OPENSSL_NO_DH @@ -7703,6 +7833,7 @@ static void __ssl_sock_init(void) cfg_register_keywords(&cfg_kws); cli_register_kw(&cli_kws); ENGINE_load_builtin_engines(); + hap_register_post_check(ssl_check_async_engine_count); #if (defined SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB && TLS_TICKETS_NO > 0) hap_register_post_check(tlskeys_finalize_config); #endif -- 1.8.3.1
>From bd921b4316c556509385966d91c6a40fbc067786 Mon Sep 17 00:00:00 2001 From: Emeric Brun <[email protected]> Date: Tue, 16 May 2017 11:13:44 +0200 Subject: [PATCH 3/7] BUG/MAJOR: ssl: fix uninit async fd if an engine is used. Bug was introduced with engines async mode support and can cause a buffer overflow on fdtab or a stop of random fd. This patch also adds some checks to clean the async fd correctly if the openssl API reports an fd to delete. --- src/ssl_sock.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 50b3cfe..3c656c4 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -392,8 +392,17 @@ void ssl_async_process_fds(struct connection *conn) SSL_get_changed_async_fds(ssl, &add_fd, &num_add_fds, &del_fd, &num_del_fds); - if (num_del_fds) - fd_stop_both(del_fd); + if (num_del_fds) { + /* In practive, we never face this case + (using openssl 1.1.0e), the same async fd + is always kept until SSL_free */ + if (conn->async_fd == del_fd) { + fd_stop_both(del_fd); + fdtab[del_fd].async = 0; + fdtab[del_fd].state = 0; + conn->async_fd = -1; + } + } if (num_add_fds) { conn->async_fd = add_fd; @@ -4264,6 +4273,11 @@ static int ssl_sock_init(struct connection *conn) return -1; } +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL + /* Init async_fd to -1 (no async fd available yet) */ + conn->async_fd = -1; +#endif + /* If it is in client mode initiate SSL session in connect state otherwise accept state */ if (objt_server(conn->target)) { @@ -4835,7 +4849,7 @@ static void ssl_sock_close(struct connection *conn) { if (conn->xprt_ctx) { #if OPENSSL_VERSION_NUMBER >= 0x1010000fL - if (openssl_engines_initialized && conn->async_fd != -1) { + if (conn->async_fd != -1) { /* the async fd is created and owned by the SSL engine, which is * responsible for fd closure. Here we are done with the async fd * thus disable the polling on it, as well as clean up fdtab entry. @@ -4843,6 +4857,7 @@ static void ssl_sock_close(struct connection *conn) { fd_stop_both(conn->async_fd); fdtab[conn->async_fd].async = 0; fdtab[conn->async_fd].state = 0; + conn->async_fd = -1; } #endif SSL_free(conn->xprt_ctx); -- 1.8.3.1
>From 126065daaa1f7f05f70c3f412e4c17002a18b43a Mon Sep 17 00:00:00 2001 From: Emeric Brun <[email protected]> Date: Mon, 15 May 2017 17:50:05 +0200 Subject: [PATCH 4/7] BUG/MAJOR: ssl: prevent to read on async fd until an event READ was polled. Crypto operations are very slow, after receiving a WANT_ASYNC we must prevent the conn_fd_handler to immediatly re-call the SSL_read/write/handshake function and we must wait for an event READ polled on the async file descriptor. On current QuickAssist engine, the async fd is in blocking mode and haproxy was stalled on blocking read syscalls. --- src/ssl_sock.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 3c656c4..c94a1a7 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -362,6 +362,11 @@ void ssl_async_fd_handler(int fd) struct connection *conn = fdtab[fd].owner; int conn_fd = conn->t.sock.fd; + /* fd is the async_fd, we must stop + * to poll this fd until it is requested + */ + fd_cant_recv(fd); + /* crypto engine is available, let's notify the associated * connection that it can pursue its processing. */ @@ -382,8 +387,18 @@ void ssl_async_process_fds(struct connection *conn) SSL_get_changed_async_fds(ssl, NULL, &num_add_fds, NULL, &num_del_fds); - if (num_add_fds == 0 && num_del_fds == 0) + if (num_add_fds == 0 && num_del_fds == 0) { + /* We must re-enable the poll on async_fd + because openssl returns a WANT_ASYNC. + We must also prevent the conn_handler + to be called until a read event was + polled on async fd */ + if (conn->async_fd != -1) { + fd_want_recv(conn->async_fd); + __conn_sock_stop_both(conn); + } return; + } /* we don't support more than 1 async fd */ if (num_add_fds > 1 || num_del_fds > 1) @@ -411,7 +426,13 @@ void ssl_async_process_fds(struct connection *conn) fdtab[add_fd].owner = conn; fdtab[add_fd].iocb = ssl_async_fd_handler; fd_insert(add_fd); + /* We must enable the poll of async_fd + because openssl returns a WANT_ASYNC. + We must also prevent the conn_handler + to be called until a read event was + polled on async fd */ fd_want_recv(add_fd); + __conn_sock_stop_both(conn); } } #endif -- 1.8.3.1
>From 4ba8fa78bd94b5b38cd52e4bbe5b3ab9fe09b93b Mon Sep 17 00:00:00 2001 From: Emeric Brun <[email protected]> Date: Wed, 17 May 2017 17:46:45 +0200 Subject: [PATCH 5/7] BUG/MAJOR: ssl: fix segfault in ssl async engines. If a connection was closed before the engine's job end, SSL_free was called but the engine rely on some internal. The fix postpones the SSL_free on a polled read event on the async fd if openssl is waiting for an async job. --- src/ssl_sock.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index c94a1a7..cb202e2 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -373,6 +373,18 @@ void ssl_async_fd_handler(int fd) conn_fd_handler(conn_fd); } +void ssl_async_fd_free(int fd) +{ + SSL *ssl = fdtab[fd].owner; + + fd_stop_both(fd); + fdtab[fd].async = 0; + fdtab[fd].state = 0; + SSL_free(ssl); + sslconns--; + jobs--; +} + /* * openssl async fd handler */ @@ -4875,6 +4887,22 @@ static void ssl_sock_close(struct connection *conn) { * responsible for fd closure. Here we are done with the async fd * thus disable the polling on it, as well as clean up fdtab entry. */ + if (SSL_waiting_for_async(conn->xprt_ctx)) { + /* If we still waiting for the engine + * on an async job, we must delay + * the SSL_free until the job is finished + * on the engine side. So we must + * wait for a read event on the + * file descriptor */ + fdtab[conn->async_fd].async = 1; + fdtab[conn->async_fd].owner = conn->xprt_ctx; + fdtab[conn->async_fd].iocb = ssl_async_fd_free; + fd_want_recv(conn->async_fd); + conn->xprt_ctx = NULL; + conn->async_fd = -1; + jobs++; + return; + } fd_stop_both(conn->async_fd); fdtab[conn->async_fd].async = 0; fdtab[conn->async_fd].state = 0; -- 1.8.3.1
>From fcce6c5933cb0f5011ab1c036a1943f572a8f878 Mon Sep 17 00:00:00 2001 From: Emeric Brun <[email protected]> Date: Wed, 17 May 2017 21:05:04 +0200 Subject: [PATCH 6/7] BUG/MEDIUM: ssl: async engine fds are never removed from the fdtab. This patch also cleans the unused 'async' flag on fdtab. --- include/types/fd.h | 1 - src/ssl_sock.c | 15 +++------------ 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/include/types/fd.h b/include/types/fd.h index 42808eb..2bd7c07 100644 --- a/include/types/fd.h +++ b/include/types/fd.h @@ -100,7 +100,6 @@ struct fdtab { unsigned char updated:1; /* 1 if this fd is already in the update list */ unsigned char linger_risk:1; /* 1 if we must kill lingering before closing */ unsigned char cloned:1; /* 1 if a cloned socket, requires EPOLL_CTL_DEL on close */ - unsigned char async:1; /* 1 if this fd is async ssl fd */ }; /* less often used information */ diff --git a/src/ssl_sock.c b/src/ssl_sock.c index cb202e2..8d7a885 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -377,9 +377,7 @@ void ssl_async_fd_free(int fd) { SSL *ssl = fdtab[fd].owner; - fd_stop_both(fd); - fdtab[fd].async = 0; - fdtab[fd].state = 0; + fd_remove(fd); SSL_free(ssl); sslconns--; jobs--; @@ -424,17 +422,13 @@ void ssl_async_process_fds(struct connection *conn) (using openssl 1.1.0e), the same async fd is always kept until SSL_free */ if (conn->async_fd == del_fd) { - fd_stop_both(del_fd); - fdtab[del_fd].async = 0; - fdtab[del_fd].state = 0; + fd_remove(del_fd); conn->async_fd = -1; } } if (num_add_fds) { conn->async_fd = add_fd; - fdtab[add_fd].async = 1; - fdtab[add_fd].state = 0; fdtab[add_fd].owner = conn; fdtab[add_fd].iocb = ssl_async_fd_handler; fd_insert(add_fd); @@ -4894,7 +4888,6 @@ static void ssl_sock_close(struct connection *conn) { * on the engine side. So we must * wait for a read event on the * file descriptor */ - fdtab[conn->async_fd].async = 1; fdtab[conn->async_fd].owner = conn->xprt_ctx; fdtab[conn->async_fd].iocb = ssl_async_fd_free; fd_want_recv(conn->async_fd); @@ -4903,9 +4896,7 @@ static void ssl_sock_close(struct connection *conn) { jobs++; return; } - fd_stop_both(conn->async_fd); - fdtab[conn->async_fd].async = 0; - fdtab[conn->async_fd].state = 0; + fd_remove(conn->async_fd); conn->async_fd = -1; } #endif -- 1.8.3.1
>From f3d36a8d81a39733b9a1896d7bc16b37b514f20c Mon Sep 17 00:00:00 2001 From: Emeric Brun <[email protected]> Date: Wed, 17 May 2017 20:42:48 +0200 Subject: [PATCH 7/7] MEDIUM: ssl: handle multiple async engines This patch adds the support of a maximum of 32 engines in async mode. Some tests have been done using 2 engines simultaneously. This patch also remove specific 'async' attribute from the connection structure. All the code rely only on Openssl functions. --- doc/configuration.txt | 3 +- include/proto/connection.h | 4 - include/types/connection.h | 4 - src/ssl_sock.c | 177 ++++++++++++++++++++++++++------------------- 4 files changed, 104 insertions(+), 84 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index eebe02f..75f9961 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -1284,7 +1284,8 @@ ssl-engine <name> [algo <comma-seperated list of algorithms>] ssl-mode-async Adds SSL_MODE_ASYNC mode to the SSL context. This enables asynchronous TLS - I/O operations if an asynchronous capable SSL engine is used. + I/O operations if asynchronous capable SSL engines are used. The current + implementation supports a maximum of 32 engines. tune.buffers.limit <number> Sets a hard limit on the number of buffers which may be allocated per process. diff --git a/include/proto/connection.h b/include/proto/connection.h index 235671e..2380bb8 100644 --- a/include/proto/connection.h +++ b/include/proto/connection.h @@ -502,10 +502,6 @@ static inline void conn_init(struct connection *conn) conn->target = NULL; conn->proxy_netns = NULL; LIST_INIT(&conn->list); - -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL - conn->async_fd = -1; -#endif } /* Tries to allocate a new connection and initialized its main fields. The diff --git a/include/types/connection.h b/include/types/connection.h index e19f883..9d1b51a 100644 --- a/include/types/connection.h +++ b/include/types/connection.h @@ -303,10 +303,6 @@ struct connection { struct sockaddr_storage from; /* client address, or address to spoof when connecting to the server */ struct sockaddr_storage to; /* address reached by the client, or address to connect to */ } addr; /* addresses of the remote side, client for producer and server for consumer */ - -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL - OSSL_ASYNC_FD async_fd; -#endif }; /* proxy protocol v2 definitions */ diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 8d7a885..dd63c19 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -357,12 +357,15 @@ fail_get: } #if OPENSSL_VERSION_NUMBER >= 0x1010000fL -void ssl_async_fd_handler(int fd) +/* + * openssl async fd handler + */ +static void ssl_async_fd_handler(int fd) { struct connection *conn = fdtab[fd].owner; int conn_fd = conn->t.sock.fd; - /* fd is the async_fd, we must stop + /* fd is an async enfine fd, we must stop * to poll this fd until it is requested */ fd_cant_recv(fd); @@ -373,73 +376,86 @@ void ssl_async_fd_handler(int fd) conn_fd_handler(conn_fd); } -void ssl_async_fd_free(int fd) +/* + * openssl async delayed SSL_free handler + */ +static void ssl_async_fd_free(int fd) { SSL *ssl = fdtab[fd].owner; + OSSL_ASYNC_FD all_fd[32]; + size_t num_all_fds = 0; + int i; + + /* We suppose that the async job for a same SSL * + * are serialized. So if we are awake it is + * because the running job has just finished + * and we can remove all async fds safely + */ + SSL_get_all_async_fds(ssl, NULL, &num_all_fds); + if (num_all_fds > 32) { + send_log(NULL, LOG_EMERG, "haproxy: openssl returns too many async fds. It seems a bug. Process may crash\n"); + return; + } - fd_remove(fd); + SSL_get_all_async_fds(ssl, all_fd, &num_all_fds); + for (i=0 ; i < num_all_fds ; i++) + fd_remove(all_fd[i]); + + /* Now we can safely call SSL_free, no more pending job in engines */ SSL_free(ssl); sslconns--; jobs--; } - /* - * openssl async fd handler + * function used to manage a returned SSL_ERROR_WANT_ASYNC + * and enable/disable polling for async fds */ -void ssl_async_process_fds(struct connection *conn) +static void inline ssl_async_process_fds(struct connection *conn, SSL *ssl) { - OSSL_ASYNC_FD add_fd; - OSSL_ASYNC_FD del_fd; + OSSL_ASYNC_FD add_fd[32], afd; + OSSL_ASYNC_FD del_fd[32]; size_t num_add_fds = 0; size_t num_del_fds = 0; - SSL *ssl = conn->xprt_ctx; + int i; SSL_get_changed_async_fds(ssl, NULL, &num_add_fds, NULL, &num_del_fds); - - if (num_add_fds == 0 && num_del_fds == 0) { - /* We must re-enable the poll on async_fd - because openssl returns a WANT_ASYNC. - We must also prevent the conn_handler - to be called until a read event was - polled on async fd */ - if (conn->async_fd != -1) { - fd_want_recv(conn->async_fd); - __conn_sock_stop_both(conn); - } + if (num_add_fds > 32 || num_del_fds > 32) { + send_log(NULL, LOG_EMERG, "haproxy: openssl returns too many async fds. It seems a bug. Process may crash\n"); return; } - /* we don't support more than 1 async fd */ - if (num_add_fds > 1 || num_del_fds > 1) - return; + SSL_get_changed_async_fds(ssl, add_fd, &num_add_fds, del_fd, &num_del_fds); - SSL_get_changed_async_fds(ssl, &add_fd, &num_add_fds, &del_fd, - &num_del_fds); + /* We remove unused fds from the fdtab */ + for (i=0 ; i < num_del_fds ; i++) + fd_remove(del_fd[i]); - if (num_del_fds) { - /* In practive, we never face this case - (using openssl 1.1.0e), the same async fd - is always kept until SSL_free */ - if (conn->async_fd == del_fd) { - fd_remove(del_fd); - conn->async_fd = -1; - } + /* We add new fds to the fdtab */ + for (i=0 ; i < num_add_fds ; i++) { + afd = add_fd[i]; + fdtab[afd].owner = conn; + fdtab[afd].iocb = ssl_async_fd_handler; + fd_insert(afd); } - if (num_add_fds) { - conn->async_fd = add_fd; - fdtab[add_fd].owner = conn; - fdtab[add_fd].iocb = ssl_async_fd_handler; - fd_insert(add_fd); - /* We must enable the poll of async_fd - because openssl returns a WANT_ASYNC. - We must also prevent the conn_handler - to be called until a read event was - polled on async fd */ - fd_want_recv(add_fd); - __conn_sock_stop_both(conn); + num_add_fds = 0; + SSL_get_all_async_fds(ssl, NULL, &num_add_fds); + if (num_add_fds > 32) { + send_log(NULL, LOG_EMERG, "haproxy: openssl returns too many async fds. It seems a bug. Process may crash\n"); + return; } + + /* We activate the polling for all known async fds */ + SSL_get_all_async_fds(ssl, add_fd, &num_add_fds); + for (i=0 ; i < num_add_fds ; i++) + fd_want_recv(add_fd[i]); + + /* We must also prevent the conn_handler + * to be called until a read event was + * polled on an async fd + */ + __conn_sock_stop_both(conn); } #endif @@ -4300,11 +4316,6 @@ static int ssl_sock_init(struct connection *conn) return -1; } -#if OPENSSL_VERSION_NUMBER >= 0x1010000fL - /* Init async_fd to -1 (no async fd available yet) */ - conn->async_fd = -1; -#endif - /* If it is in client mode initiate SSL session in connect state otherwise accept state */ if (objt_server(conn->target)) { @@ -4470,7 +4481,7 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag) } #if OPENSSL_VERSION_NUMBER >= 0x1010000fL else if (ret == SSL_ERROR_WANT_ASYNC) { - ssl_async_process_fds(conn); + ssl_async_process_fds(conn, conn->xprt_ctx); return 0; } #endif @@ -4554,7 +4565,7 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag) } #if OPENSSL_VERSION_NUMBER >= 0x1010000fL else if (ret == SSL_ERROR_WANT_ASYNC) { - ssl_async_process_fds(conn); + ssl_async_process_fds(conn, conn->xprt_ctx); return 0; } #endif @@ -4746,7 +4757,7 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun } #if OPENSSL_VERSION_NUMBER >= 0x1010000fL else if (ret == SSL_ERROR_WANT_ASYNC) { - ssl_async_process_fds(conn); + ssl_async_process_fds(conn, conn->xprt_ctx); break; } #endif @@ -4854,7 +4865,7 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl } #if OPENSSL_VERSION_NUMBER >= 0x1010000fL else if (ret == SSL_ERROR_WANT_ASYNC) { - ssl_async_process_fds(conn); + ssl_async_process_fds(conn, conn->xprt_ctx); break; } #endif @@ -4876,28 +4887,44 @@ static void ssl_sock_close(struct connection *conn) { if (conn->xprt_ctx) { #if OPENSSL_VERSION_NUMBER >= 0x1010000fL - if (conn->async_fd != -1) { - /* the async fd is created and owned by the SSL engine, which is - * responsible for fd closure. Here we are done with the async fd - * thus disable the polling on it, as well as clean up fdtab entry. - */ - if (SSL_waiting_for_async(conn->xprt_ctx)) { - /* If we still waiting for the engine - * on an async job, we must delay - * the SSL_free until the job is finished - * on the engine side. So we must - * wait for a read event on the - * file descriptor */ - fdtab[conn->async_fd].owner = conn->xprt_ctx; - fdtab[conn->async_fd].iocb = ssl_async_fd_free; - fd_want_recv(conn->async_fd); + if (global_ssl.async) { + OSSL_ASYNC_FD all_fd[32], afd; + size_t num_all_fds = 0; + int i; + + SSL_get_all_async_fds(conn->xprt_ctx, NULL, &num_all_fds); + if (num_all_fds > 32) { + send_log(NULL, LOG_EMERG, "haproxy: openssl returns too many async fds. It seems a bug. Process may crash\n"); + return; + } + + SSL_get_all_async_fds(conn->xprt_ctx, all_fd, &num_all_fds); + + /* If an async job is pending, we must try to + to catch the end using polling before calling + SSL_free */ + if (num_all_fds && SSL_waiting_for_async(conn->xprt_ctx)) { + for (i=0 ; i < num_all_fds ; i++) { + /* switch on an handler designed to + * handle the SSL_free + */ + afd = all_fd[i]; + fdtab[afd].iocb = ssl_async_fd_free; + fdtab[afd].owner = conn->xprt_ctx; + fd_want_recv(afd); + } conn->xprt_ctx = NULL; - conn->async_fd = -1; jobs++; return; } - fd_remove(conn->async_fd); - conn->async_fd = -1; + /* Else we can remove the fds from the fdtab + * and call SSL_free. + * note: we do a fd_remove and not a delete + * because the fd is owned by the engine. + * the engine is responsible to close + */ + for (i=0 ; i < num_all_fds ; i++) + fd_remove(all_fd[i]); } #endif SSL_free(conn->xprt_ctx); @@ -7155,8 +7182,8 @@ static int ssl_parse_global_ssl_async(char **args, int section_type, struct prox static int ssl_check_async_engine_count(void) { int err_code = 0; - if (global_ssl.async && openssl_engines_initialized!=1) { - Alert("ssl-mode-async only supports one engine\n"); + if (global_ssl.async && (openssl_engines_initialized > 32)) { + Alert("ssl-mode-async only supports a maximum of 32 engines.\n"); err_code = ERR_ABORT; } return err_code; -- 1.8.3.1
