Hi Grant, sorry for the longer delay than what I expected.
I rebased your work on top of the latest development tree so that it's easier to review since it's where it's ultimately going to get merged. First I have a question regarding the engines configuration. I still have a pretty similar patch lying around that I never merged because I was not happy with having to enable all algorithms or none on the engine. I had tested some low performance crypto cards in the past and they were speeding up some operations and slowing down others. Even today, if I test on my 4-core home PC with a Skylake CPU, aes-128-gcm gives me 218 Gbps of bandwidth which no hardware accelerator will be able to do given that this requires a huge amount of PCI bandwidth. Thus ideally I'd like to be able to delegate the heavy stuff to an engine and let the CPU deal with the cheap stuff. That's mostly symmetric vs asymmetric, but I had cut it around all the ENGINE_register_*() functions that openssl exposes. I don't know the level of granularity we have with the engines (which is also why I never merged this patch). I remember for example a crypto card where operations were not faster but they would at least offload the CPU, and in the end it was faster to let the card sign with an RSA key and run the DH parts on the CPU so that both could work more or less in parallel. Also it's unclear to me whether or not it is possible to use multiple engines. Eg, I remember that some engines can provide random, which can be convenient if a card doesn't, or if some algos are not supported by the card and only the randoms are desired. Thus whatever information you can find in this area would be useful. I'm appending the two patches I used to work on by then (it was 4 years ago likely for version 1.5 but you'll see how similar they are to yours so you'll easily understand). Now let's go to the patches below :-) > From 0961d88889807d9af10c841824bc1a5a4d72b02f Mon Sep 17 00:00:00 2001 > From: Grant Zhang <[email protected]> > Date: Sat, 14 Jan 2017 01:42:14 +0000 > Subject: RFC: add openssl engine support > > Global configuration parameter "ssl_engine" may be used to specify > openssl engine. > --- For this one you can happily steal the doc part from my initial patch, it should match. (...) > diff --git a/src/ssl_sock.c b/src/ssl_sock.c > index 95f979c..9d07f9c 100644 > --- a/src/ssl_sock.c > +++ b/src/ssl_sock.c > @@ -56,6 +56,8 @@ > #include <import/lru.h> > #include <import/xxhash.h> > > +#include <openssl/engine.h> > + > #include <common/buffer.h> > #include <common/compat.h> > #include <common/config.h> > @@ -135,6 +137,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 */ > + char *engine; /* openssl engine to use */ > > char *listen_default_ciphers; > char *connect_default_ciphers; > @@ -263,6 +266,42 @@ static forceinline void ssl_sock_dump_errors(struct > connection *conn) > } > } > > +static ENGINE *engine; > + > +void ssl_init_engine(const char *engine_id) > +{ > + OpenSSL_add_all_algorithms(); > + ENGINE_load_builtin_engines(); > + RAND_set_rand_method(NULL); > + > + /* grab the structural reference to the engine */ > + engine = ENGINE_by_id(engine_id); > + if (engine == NULL) { > + Alert("Engine %s: failed to get structural reference\n", > engine_id); Please mention "SSL-Engine" so that there's absolutely no ambiguity when a user meets such an error. > + exit(-1); Please don't exit() directly from functions, it makes it impossible to run validity checks on the configs. Instead simply return an error code that the caller will check. Specifically we're used to have return codes having some flags like ERR_FATAL, ERR_ABORT or ERR_WARN depending on the criticity of the error, that's very convenient. > + } > + > + if (!ENGINE_init(engine)) { > + /* the engine couldn't initialise, release it */ > + Alert("Engine %s: failed to initialize\n", engine_id); > + ENGINE_free(engine); Here on the error unrolling, better add labels and goto the end of the function. That makes it easier to validate that nothing is missing (eg if we later insert something in the middle) of these calls. > + return; > + } > + > + if (ENGINE_set_default(engine, ENGINE_METHOD_ALL) == 0) { > + Alert("Engine %s: ENGINE_set_default failed\n", engine_id); > + ENGINE_finish(engine); > + ENGINE_free(engine); Same here. > + return; > + } > + > + /* release the functional reference from ENGINE_init() */ > + ENGINE_finish(engine); > + > + /* release the structural reference from ENGINE_by_id() */ > + ENGINE_free(engine); > +} > + > /* > * This function returns the number of seconds elapsed > * since the Epoch, 1970-01-01 00:00:00 +0000 (UTC) and the > @@ -6365,6 +6404,26 @@ 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) > +{ > + if (global_ssl.engine != NULL) { > + memprintf(err, "'%s' already specified.", args[0]); > + return -1; > + } > + if (*(args[1]) == 0) { > + memprintf(err, "global statement '%s' expects a valid engine > name as an argument.", args[0]); > + return -1; > + } > + global_ssl.engine = strdup(args[1]); > + ssl_init_engine(global_ssl.engine); It will be problematic to initialize so early. Here it's initialized when the keyword is being parsed, so if the statement happens multiple times, I don't know what it causes, and it also means it will call the initialization stuff when just validating that the config is OK. Instead you should just set global_ssl.engine as you did, and have a late init function that initializes the engine. You can do that using hap_register_post_checks(). There's already a function doing this so you have an example. (...) > From d5453e689a0bb76dc9cc0f3b60d205c35f5cb705 Mon Sep 17 00:00:00 2001 > From: Grant Zhang <[email protected]> > Date: Sat, 14 Jan 2017 01:42:15 +0000 > Subject: RFC: add openssl async support > (...) > diff --git a/include/common/epoll.h b/include/common/epoll.h > index cc395cd..dd1c7d6 100644 > --- a/include/common/epoll.h > +++ b/include/common/epoll.h > @@ -70,6 +70,8 @@ struct epoll_event { > } data; > }; > > +int remove_fd_from_epoll(int fd); > + > #if defined(CONFIG_HAP_LINUX_VSYSCALL) && defined(__linux__) && > defined(__i386__) > /* Those are our self-defined functions */ > extern int epoll_create(int size); This part is problematic for several reasons. The first reason is that SSL must absolutely remain poller-agnostic so we can't decide to use epoll there. The second reason is that we're using an event cache to ensure that we don't call epoll_ctl(ADD/DEL) all the time and that everything passes through this event cache. And if/when we later support EPOLL_ET there will be nothing to change at all. The good thing is that adapting to it should not be a big change. > diff --git a/include/proto/fd.h b/include/proto/fd.h > index 87309bf..922fee5 100644 > --- a/include/proto/fd.h > +++ b/include/proto/fd.h > @@ -40,6 +40,7 @@ extern int fd_nbupdt; // number of updates in > the list > * The file descriptor is also closed. > */ > void fd_delete(int fd); > +void fd_async_delete(int fd); > > /* disable the specified poller */ > void disable_poller(const char *poller_name); > @@ -339,6 +340,13 @@ static inline void fd_insert(int fd) > maxfd = fd + 1; > } > > +/* Prepares async <fd> for being polled */ > +static inline void fd_async_insert(int fd) > +{ > + fdtab[fd].state = 0; You don't need to reset the state as it's already zero if it was free (guaranteed by fd_delete() and by calloc()). > + fdtab[fd].async = 1; > + fd_insert(fd); > +} > > #endif /* _PROTO_FD_H */ > > diff --git a/include/types/connection.h b/include/types/connection.h > index c644dd5..1c8e1a8 100644 > --- a/include/types/connection.h > +++ b/include/types/connection.h > @@ -303,6 +303,8 @@ 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 */ > + > + OSSL_ASYNC_FD async_fd; Just out of curiosity, do you know the size of this element ? Is it an array, just an FD or anything else ? We tend to be very careful in the connection struct because we can have many of them. But we're also working on trying to make the struct a bit more dynamic, so unless this entry is huge, don't worry too much about it for now. However it will not build without openssl so some #ifdef are needed. That makes me remember that there is a way to store some transport-specific data in the xprt_ctx but I'm not sure how anymore. It would be much cleaner since the SSL stuff would be stored into the SSL part. After reviewing the code, I *think* it's done using SSL_CTX_set_ex_data() but I could be wrong. (...) > diff --git a/src/ev_epoll.c b/src/ev_epoll.c > index ccb7c33..27d630e 100644 > --- a/src/ev_epoll.c > +++ b/src/ev_epoll.c > @@ -41,6 +41,17 @@ static struct epoll_event ev; > #define EPOLLRDHUP 0x2000 > #endif > > +int remove_fd_from_epoll(int fd) > +{ > + struct epoll_event ee; > + int ret; > + > + ee.events = 0; > + ee.data.ptr = NULL; > + ret = epoll_ctl(epoll_fd, EPOLL_CTL_DEL, fd, &ee); > + return ret; > +} So this is the part causing me some trouble. Normally just doing fd_delete(fd) will automatically take care of all this via the event cache. > diff --git a/src/fd.c b/src/fd.c > index aeee602..483f3eb 100644 > --- a/src/fd.c > +++ b/src/fd.c > @@ -198,6 +198,19 @@ void fd_delete(int fd) > maxfd--; > } > > +void fd_async_delete(int fd) > +{ > + fd_release_cache_entry(fd); > + > + fdtab[fd].state = 0; > + fdtab[fd].async = 0; > + fdtab[fd].owner = NULL; > + fdtab[fd].new = 0; > + > + while ((maxfd-1 >= 0) && !fdtab[maxfd-1].owner) > + maxfd--; > +} Here in my opinion there's no need for a different function as fd_delete() should properly take care of all this. Also, the fd is not closed here, I don't know if it's intentional or just a miss, because at least in our terminology the delete() should also close the fd. If you don't want to close it, better simply disable the polling on it via the cache using fd_stop_both(). > diff --git a/src/ssl_sock.c b/src/ssl_sock.c > index 9d07f9c..f054744 100644 > --- a/src/ssl_sock.c > +++ b/src/ssl_sock.c > @@ -57,6 +57,7 @@ > #include <import/xxhash.h> > > #include <openssl/engine.h> > +#include <openssl/async.h> > > #include <common/buffer.h> > #include <common/compat.h> > @@ -68,6 +69,7 @@ > #include <common/time.h> > #include <common/cfgparse.h> > #include <common/base64.h> > +#include <common/epoll.h> > > #include <ebsttree.h> > > @@ -138,6 +140,7 @@ static struct { > char *crt_base; /* base directory path for certificates */ > char *ca_base; /* base directory path for CAs and CRLs */ > char *engine; /* openssl engine to use */ > + int async; /* whether we use ssl async mode */ > > char *listen_default_ciphers; > char *connect_default_ciphers; > @@ -303,6 +306,74 @@ void ssl_init_engine(const char *engine_id) > } > Just below this I'd enclose the block in an #if OPENSSL_VERSION >= 0x1010000fL so that it doesn't break older versions. > /* > + * openssl async fd handler > + */ > +int ssl_async_process_fds(struct connection *conn) > +{ > + OSSL_ASYNC_FD *add_fds = NULL; > + OSSL_ASYNC_FD *del_fds = NULL; > + size_t num_add_fds = 0; > + size_t num_del_fds = 0; > + unsigned int loop = 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 0; > + > + if (num_add_fds) { > + add_fds = OPENSSL_malloc(num_add_fds * sizeof(OSSL_ASYNC_FD)); Do you know if we're forced to pass through this malloc all the time or if there's a way to keep some FDs in pools for example ? I suspect that most of the time we'll have a single FD and thinking that we have to call malloc() just for this is a bit sad :-/ But I also know that we don't always decide with openssl's internals. > + if (add_fds == NULL) { > + Alert("Memory Allocation Error"); It's pointless to emit an alert here, it will be ignored as we're supposed to be already daemonized. However it can be useful to add a flag to pass to conn->xprt_st to keep track of the error. Look for SSL_SOCK_RECV_HEARTBEAT for example. I'm not saying it's the best one, it's just that I happened to notice it while searching :-) > + return 0; Hmmm here you return zero just as if there's nothing to do in the case above, I don't know if that's really intended. > + } > + } > + > + if (num_del_fds) { > + del_fds = OPENSSL_malloc(num_del_fds * sizeof(OSSL_ASYNC_FD)); > + if (del_fds == NULL) { > + Alert("Memory Allocation Error"); > + if (add_fds) > + OPENSSL_free(add_fds); > + return 0; > + } > + } > + > + SSL_get_changed_async_fds(ssl, add_fds, &num_add_fds, del_fds, > + &num_del_fds); OK so from what I'm seeing, indeed add_fds and del_fds could be pre-allocated since they're used only during the operation to retrieve the list of FDs. Thus I'd prefer that we preallocate them with a "large enough" value and keep them static in ssl_sock.c (with a comment mentionning this will have to be thread-local later). The way it's implemented in openssl just to actually count them is a linear linked list, so due to the time it takes it will never be huge. So it seems reasonable to allocate a few tens of them to be large. And even if you have a 1024-fd array for each (which is the default fd limit), it will only consume one page for add_fds and one page for del_fds which is still very cheap compared to having to go through painful malloc/check/free all the time. In the end that will also remove the need for error handling ;-) > + if (num_del_fds) { > + for (loop = 0; loop < num_del_fds; loop++) { > + int async_fd = del_fds[loop]; > + > + fd_stop_recv(async_fd); > + fd_async_delete(async_fd); So I suspect you indeed don't want to close in fd_async_delete(), but then fd_stop_both() will be enough to replace both this and fd_stop_recv(). > + } > + } > + > + if (num_add_fds) { > + for (loop = 0; loop < num_add_fds; loop++) { > + int async_fd = add_fds[loop]; > + > + fd_async_insert(async_fd); Thus from what I'm seeing, I think that fd_async_insert() should perform a conditional insert. An fd always has an owner if it's valid, so this function could allocate the fd on the fly if .owner is NULL or do nothing otherwise. > + fdtab[async_fd].iocb = conn_fd_handler; That's interesting. In fact you're calling the original FD's connection handler when you get activity on the crypto fd. I didn't think about doing it this way. I'm seeing a few corner cases where it will not work, because the fd passed to the connection handler will be the engine's fd and not the connection's fd, and there are a few places in the connection handler where we check the readiness for recv or send on this fd (which is the wrong one here). We could modify the connection handler to retrieve the FD again from the connection but that becomes trickier, especially with the rare cases where you have a race condition after an fd may be reported just after it was detached from the connection. I suspect you could instead proceed with your own FD handler taking care of this, probably like this (error checking left out for simplicity) : void ssl_async_fd_handler(int fd) { struct connection *conn = fdtab[fd].owner; conn_fd_handler(conn->t.sock.fd); } But I'm still seeing a few corner cases there so in the end it should be much cleaner and more reliable to re-enable the original FD in the cache when you see that the engine reported some activity : void ssl_async_fd_handler(int fd) { struct connection *conn = fdtab[fd].owner; int conn_fd = conn->t.sock.fd; fd_may_recv(conn_fd); fd_may_send(conn_fd); } This will automatically cause the connection handler to be called by the event cache with the appropriate status on the connection's file descriptor, also allowing the connection handler to manipulate this status (since the FD is supposed to be active when we enter the conn_fd_handler). > + conn->async_fd = async_fd; Aha! here we have something interesting : this proves that you can only deal with a single async_fd per connection since only the last one will be kept (and I'm not surprized we don't need more). Thus here's what I would prefer : - just have one entry for add_fds and del_fds - abort if the stack wants to have more than 1 in num_add_fds/num_del_fds - get rid of the loops ; at best they're confusing since we're seeing that the first async_fds are lost. - and get rid of the malloc/free dance around add_fds/del_fds. > + fdtab[async_fd].owner = conn; > + fd_want_recv(async_fd); > + } > + } > + > + if (add_fds) > + OPENSSL_free(add_fds); > + if (del_fds) > + OPENSSL_free(del_fds); > + > + return 1; > +} > + > +/* > * This function returns the number of seconds elapsed > * since the Epoch, 1970-01-01 00:00:00 +0000 (UTC) and the > * date presented un ASN1_GENERALIZEDTIME. > @@ -2900,6 +2971,9 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, > struct ssl_bind_conf *ssl_ > cfgerr++; > } > > + if (global_ssl.async) > + sslmode |= SSL_MODE_ASYNC; > + This one will have to be enclosed with #if on the OPENSSL_VERSION. > if (conf_ssl_options & BC_SSL_O_NO_SSLV3) > ssloptions |= SSL_OP_NO_SSLv3; > if (conf_ssl_options & BC_SSL_O_NO_TLSV10) > @@ -3335,6 +3409,10 @@ int ssl_sock_prepare_srv_ctx(struct server *srv) > #endif > > SSL_CTX_set_options(srv->ssl_ctx.ctx, options); > + > + if (global_ssl.async) > + mode |= SSL_MODE_ASYNC; > + Here as well. > SSL_CTX_set_mode(srv->ssl_ctx.ctx, mode); > > if (global.ssl_server_verify == SSL_SERVER_VERIFY_REQUIRED) > @@ -3791,6 +3869,12 @@ 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_ASYNC) { > + ssl_async_process_fds(conn); > + return 0; > + } Here as well. > if (ret == SSL_ERROR_WANT_WRITE) { > /* SSL handshake needs to write, L4 connection > may not be ready */ > __conn_sock_stop_recv(conn); > @@ -3873,6 +3957,11 @@ int ssl_sock_handshake(struct connection *conn, > unsigned int flag) > /* handshake did not complete, let's find why */ > ret = SSL_get_error(conn->xprt_ctx, ret); > > + if (ret == SSL_ERROR_WANT_ASYNC) { > + ssl_async_process_fds(conn); > + return 0; > + } Here as well. > if (ret == SSL_ERROR_WANT_WRITE) { > /* SSL handshake needs to write, L4 connection may not > be ready */ > __conn_sock_stop_recv(conn); > @@ -4056,6 +4145,11 @@ static int ssl_sock_to_buf(struct connection *conn, > struct buffer *buf, int coun > } > else { > ret = SSL_get_error(conn->xprt_ctx, ret); > + if (ret == SSL_ERROR_WANT_ASYNC) { > + ssl_async_process_fds(conn); > + break; > + } > + Here as well. > if (ret == SSL_ERROR_WANT_WRITE) { > /* handshake is running, and it needs to enable > write */ > conn->flags |= CO_FL_SSL_WAIT_HS; > @@ -4157,6 +4251,12 @@ 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_ASYNC) { > + ssl_async_process_fds(conn); > + break; > + } > + Here as well. > if (ret == SSL_ERROR_WANT_WRITE) { > if (SSL_renegotiate_pending(conn->xprt_ctx)) { > /* handshake is running, and it may > need to re-enable write */ > @@ -4191,6 +4291,11 @@ 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 (global_ssl.async) { > + remove_fd_from_epoll(conn->async_fd); > + fd_stop_recv(conn->async_fd); > + fd_async_delete(conn->async_fd); > + } So here just like I mentionned above, simply doing fd_stop_both() will be enough, the FD will not be reported anymore. Ideally in order to avoid any race condition (we'll have threads one day), it would be nice to set the owner to a dummy value that cannot be confused with a connection pointer for example (eg: point to itself) and the iocb set to NULL. The purpose is only to ensure that any late event reported after the end of the connection will silently be ignored. Having NULL in the owner normally is equivalent to an unused FD which is why I don't feel comfortable with it. > SSL_free(conn->xprt_ctx); > conn->xprt_ctx = NULL; > sslconns--; > @@ -6404,6 +6509,17 @@ static int ssl_parse_global_ca_crt_base(char **args, > int section_type, struct pr > return 0; > } > > +/* parse the "ssl-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) > +{ > + global_ssl.async = 1; Here you'll want to put a #ifdef as well so that the option is not available on non-supported libraries (eg: it can report "unsupported" as an error message, see other option parsers for an example). > + return 0; > +} > + > /* parse the "ssl-engine" keyword in global section. > * Returns <0 on alert, >0 on warning, 0 on success. > */ > @@ -7000,6 +7116,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-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 OK that's pretty cool and reviewable, that's nice. I'm pretty sure that we'll be able to merge this after just a few iterations. I'm appending here your patches that I rebased on 1.8-dev as well as the two old patches for engines configuration for reference if that helps. I'm particularly interested in your insights regarding the configuration part. I'll also harrass you off-list to know a bit more how you tested this because I have access to a C2518-based board which features a QAT engine so I could possibly test your code on it but I'm not sure where to start. Thanks! Willy
>From 6e016e7fb566aeb763d2f5e28124509b1558641a Mon Sep 17 00:00:00 2001 From: Emeric Brun <[email protected]> Date: Tue, 11 Sep 2012 10:32:22 +0200 Subject: MEDIUM: ssl: add basic support for OpenSSL crypto engine X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 This patch adds the global 'ssl-engine' keyword. First arg is an engine identifier or 'all' to load all detected engines. If the openssl version is too old, an error is reported when the option is used. --- doc/configuration.txt | 9 +++++++++ src/cfgparse.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/doc/configuration.txt b/doc/configuration.txt index 30647a9..13de27f 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -465,6 +465,7 @@ The following keywords are supported in the "global" section : - nopoll - nosplice - spread-checks + - ssl-engine - tune.bufsize - tune.chksize - tune.comp.maxlevel @@ -770,6 +771,14 @@ spread-checks <0..50, in percent> some randomness in the check interval between 0 and +/- 50%. A value between 2 and 5 seems to show good results. The default value remains at 0. +ssl-engine <name> + Sets the OpenSSL engine to <name>. List of valid values for <name> may be + obtained using the command "openssl engine". The special name "all" enables + support for all engines. 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. + tune.bufsize <number> Sets the buffer size to this size (in bytes). Lower values allow more sessions to coexist in the same amount of RAM, and higher values allow some diff --git a/src/cfgparse.c b/src/cfgparse.c index 175e30c..d1ce91d 100644 --- a/src/cfgparse.c +++ b/src/cfgparse.c @@ -72,6 +72,7 @@ #include <types/ssl_sock.h> #include <proto/ssl_sock.h> #include <proto/shctx.h> +#include <openssl/engine.h> #endif /*USE_OPENSSL */ /* This is the SSLv3 CLIENT HELLO packet used in conjunction with the @@ -995,6 +996,47 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm) } global.pidfile = strdup(args[1]); } +#ifdef USE_OPENSSL + else if (!strcmp(args[0], "ssl-engine")) { +#ifdef ENGINE_METHOD_ALL + ENGINE_load_builtin_engines(); + if (strcmp(args[1], "all") == 0) { + ENGINE_register_all_complete(); + } + else { + ENGINE *e; + + e = ENGINE_by_id(args[1]); + if (!e) { + Alert("parsing [%s:%d] : '%s' specified crypto engine '%s' not found.\n", file, linenum, args[0], args[1]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + + if (!ENGINE_init(e)) { + Alert("parsing [%s:%d] : '%s' unable to init crypto engine '%s'.\n", file, linenum, args[0], args[1]); + err_code |= ERR_ALERT | ERR_FATAL; + ENGINE_free(e); + goto out; + } + + if (!ENGINE_set_default(e, ENGINE_METHOD_ALL)) { + Alert("parsing [%s:%d] : '%s' unable to set crypto engine methods as defaults.\n", file, linenum, args[0]); + err_code |= ERR_ALERT | ERR_FATAL; + ENGINE_finish(e); + ENGINE_free(e); + goto out; + } + ENGINE_finish(e); + ENGINE_free(e); + } +#else + Alert("parsing [%s:%d] : '%s' cannot be used, openssl version is too old.\n", file, linenum, args[0]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; +#endif + } +#endif else if (!strcmp(args[0], "unix-bind")) { int cur_arg = 1; while (*(args[cur_arg])) { -- 1.7.12.1
>From 7d011a57eb80b6d7970e4242a2fd25f5231a5430 Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Fri, 25 Jan 2013 12:04:25 +0100 Subject: WIP: openssl engine configuration X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 --- src/cfgparse.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/src/cfgparse.c b/src/cfgparse.c index d1ce91d..43749d8 100644 --- a/src/cfgparse.c +++ b/src/cfgparse.c @@ -1001,10 +1001,36 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm) #ifdef ENGINE_METHOD_ALL ENGINE_load_builtin_engines(); if (strcmp(args[1], "all") == 0) { - ENGINE_register_all_complete(); + int cur_arg = 2; + + if (!*args[cur_arg]) + ENGINE_register_all_complete(); + else while (*args[cur_arg]) { + if (strcmp(args[cur_arg], "rsa") == 0) + ENGINE_register_all_RSA(); + else if (strcmp(args[cur_arg], "dsa") == 0) + ENGINE_register_all_DSA(); + else if (strcmp(args[cur_arg], "dh") == 0) + ENGINE_register_all_DH(); + else if (strcmp(args[cur_arg], "rand") == 0) + ENGINE_register_all_RAND(); + else if (strcmp(args[cur_arg], "ciphers") == 0) + ENGINE_register_all_ciphers(); + else if (strcmp(args[cur_arg], "digests") == 0) + ENGINE_register_all_digests(); + else if (strcmp(args[cur_arg], "all") == 0) + ENGINE_register_all_complete(); + else { + Alert("parsing [%s:%d] : '%s' unknown crypto engine method '%s', only 'rsa', 'dsa', 'dh', 'rand', 'ciphers', 'digests', and 'all' are supported.\n", file, linenum, args[0], args[cur_arg]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + cur_arg++; + } } else { ENGINE *e; + int cur_arg = 2; e = ENGINE_by_id(args[1]); if (!e) { @@ -1020,13 +1046,46 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm) goto out; } - if (!ENGINE_set_default(e, ENGINE_METHOD_ALL)) { - Alert("parsing [%s:%d] : '%s' unable to set crypto engine methods as defaults.\n", file, linenum, args[0]); + if (!*args[cur_arg] && !ENGINE_set_default(e, ENGINE_METHOD_ALL)) { + Alert("parsing [%s:%d] : '%s' unable to set default methods for crypto engine '%s'.\n", + file, linenum, args[0], args[1]); err_code |= ERR_ALERT | ERR_FATAL; ENGINE_finish(e); ENGINE_free(e); goto out; } + else while (*args[cur_arg]) { + int ret; + + if (strcmp(args[cur_arg], "rsa") == 0) + ret = ENGINE_set_default_RSA(e); + else if (strcmp(args[cur_arg], "dsa") == 0) + ret = ENGINE_set_default_DSA(e); + else if (strcmp(args[cur_arg], "dh") == 0) + ret = ENGINE_set_default_DH(e); + else if (strcmp(args[cur_arg], "rand") == 0) + ret = ENGINE_set_default_RAND(e); + else if (strcmp(args[cur_arg], "ciphers") == 0) + ret = ENGINE_set_default_ciphers(e); + else if (strcmp(args[cur_arg], "digests") == 0) + ret = ENGINE_set_default_digests(e); + else if (strcmp(args[cur_arg], "all") == 0) + ret = ENGINE_set_default(e, ENGINE_METHOD_ALL); + else { + Alert("parsing [%s:%d] : '%s' unknown crypto engine method '%s', only 'rsa', 'dsa', 'dh', 'rand', 'ciphers', 'digests', and 'all' are supported.\n", + file, linenum, args[0], args[cur_arg]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + + if (!ret) { + Alert("parsing [%s:%d] : '%s' : crypto engine '%s' failed to initialize for method '%s'.\n", + file, linenum, args[0], args[1], args[cur_arg]); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + cur_arg++; + } ENGINE_finish(e); ENGINE_free(e); } -- 1.7.12.1
>From 3dac65175bd7ffc88b7d464e9dc126fbd0f1d259 Mon Sep 17 00:00:00 2001 From: Grant Zhang <[email protected]> Date: Sat, 14 Jan 2017 01:42:14 +0000 Subject: RFC: add openssl engine support X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Global configuration parameter "ssl_engine" may be used to specify openssl engine. --- include/proto/ssl_sock.h | 2 ++ src/ssl_sock.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/include/proto/ssl_sock.h b/include/proto/ssl_sock.h index 6f779fa..c2a1da0 100644 --- a/include/proto/ssl_sock.h +++ b/include/proto/ssl_sock.h @@ -75,6 +75,8 @@ SSL_CTX *ssl_sock_get_generated_cert(unsigned int key, struct bind_conf *bind_co int ssl_sock_set_generated_cert(SSL_CTX *ctx, unsigned int key, struct bind_conf *bind_conf); unsigned int ssl_sock_generated_cert_key(const void *data, size_t len); +void ssl_init_engine(const char *engine_id); + #endif /* _PROTO_SSL_SOCK_H */ /* diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 232a497..8932d7c 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -56,6 +56,8 @@ #include <import/lru.h> #include <import/xxhash.h> +#include <openssl/engine.h> + #include <common/buffer.h> #include <common/compat.h> #include <common/config.h> @@ -135,6 +137,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 */ + char *engine; /* openssl engine to use */ char *listen_default_ciphers; char *connect_default_ciphers; @@ -262,6 +265,42 @@ struct ocsp_cbk_arg { }; }; +static ENGINE *engine; + +void ssl_init_engine(const char *engine_id) +{ + OpenSSL_add_all_algorithms(); + ENGINE_load_builtin_engines(); + RAND_set_rand_method(NULL); + + /* grab the structural reference to the engine */ + engine = ENGINE_by_id(engine_id); + if (engine == NULL) { + Alert("Engine %s: failed to get structural reference\n", engine_id); + exit(-1); + } + + if (!ENGINE_init(engine)) { + /* the engine couldn't initialise, release it */ + Alert("Engine %s: failed to initialize\n", engine_id); + ENGINE_free(engine); + return; + } + + if (ENGINE_set_default(engine, ENGINE_METHOD_ALL) == 0) { + Alert("Engine %s: ENGINE_set_default failed\n", engine_id); + ENGINE_finish(engine); + ENGINE_free(engine); + return; + } + + /* release the functional reference from ENGINE_init() */ + ENGINE_finish(engine); + + /* release the structural reference from ENGINE_by_id() */ + ENGINE_free(engine); +} + /* * This function returns the number of seconds elapsed * since the Epoch, 1970-01-01 00:00:00 +0000 (UTC) and the @@ -6375,6 +6414,26 @@ 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) +{ + if (global_ssl.engine != NULL) { + memprintf(err, "'%s' already specified.", args[0]); + return -1; + } + if (*(args[1]) == 0) { + memprintf(err, "global statement '%s' expects a valid engine name as an argument.", args[0]); + return -1; + } + global_ssl.engine = strdup(args[1]); + ssl_init_engine(global_ssl.engine); + return 0; +} + /* parse the "ssl-default-bind-ciphers" / "ssl-default-server-ciphers" keywords * in global section. Returns <0 on alert, >0 on warning, 0 on success. */ @@ -6948,6 +7007,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 }, @@ -7101,6 +7161,9 @@ static void __ssl_sock_deinit(void) #if OPENSSL_VERSION_NUMBER >= 0x00907000L CRYPTO_cleanup_all_ex_data(); #endif + + free(global_ssl.engine); + global_ssl.engine = NULL; } -- 1.7.12.1
>From 0287e1facb81fd84c16312a7cd0e271eb37a9f2e Mon Sep 17 00:00:00 2001 From: Grant Zhang <[email protected]> Date: Sat, 14 Jan 2017 01:42:15 +0000 Subject: RFC: add openssl async support X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 ssl_async is a global configuration parameter which enables asynchronous processing in OPENSSL for all SSL connections haproxy handles. With SSL_MODE_ASYNC mode 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. --- include/common/epoll.h | 2 + include/proto/fd.h | 8 ++++ include/types/connection.h | 2 + include/types/fd.h | 1 + src/ev_epoll.c | 11 +++++ src/fd.c | 13 +++++ src/ssl_sock.c | 117 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 154 insertions(+) diff --git a/include/common/epoll.h b/include/common/epoll.h index cc395cd..dd1c7d6 100644 --- a/include/common/epoll.h +++ b/include/common/epoll.h @@ -70,6 +70,8 @@ struct epoll_event { } data; }; +int remove_fd_from_epoll(int fd); + #if defined(CONFIG_HAP_LINUX_VSYSCALL) && defined(__linux__) && defined(__i386__) /* Those are our self-defined functions */ extern int epoll_create(int size); diff --git a/include/proto/fd.h b/include/proto/fd.h index 87309bf..922fee5 100644 --- a/include/proto/fd.h +++ b/include/proto/fd.h @@ -40,6 +40,7 @@ extern int fd_nbupdt; // number of updates in the list * The file descriptor is also closed. */ void fd_delete(int fd); +void fd_async_delete(int fd); /* disable the specified poller */ void disable_poller(const char *poller_name); @@ -339,6 +340,13 @@ static inline void fd_insert(int fd) maxfd = fd + 1; } +/* Prepares async <fd> for being polled */ +static inline void fd_async_insert(int fd) +{ + fdtab[fd].state = 0; + fdtab[fd].async = 1; + fd_insert(fd); +} #endif /* _PROTO_FD_H */ diff --git a/include/types/connection.h b/include/types/connection.h index c644dd5..1c8e1a8 100644 --- a/include/types/connection.h +++ b/include/types/connection.h @@ -303,6 +303,8 @@ 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 */ + + OSSL_ASYNC_FD async_fd; }; /* proxy protocol v2 definitions */ diff --git a/include/types/fd.h b/include/types/fd.h index 7f63093..f3d03f8 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/ev_epoll.c b/src/ev_epoll.c index ccb7c33..27d630e 100644 --- a/src/ev_epoll.c +++ b/src/ev_epoll.c @@ -41,6 +41,17 @@ static struct epoll_event ev; #define EPOLLRDHUP 0x2000 #endif +int remove_fd_from_epoll(int fd) +{ + struct epoll_event ee; + int ret; + + ee.events = 0; + ee.data.ptr = NULL; + ret = epoll_ctl(epoll_fd, EPOLL_CTL_DEL, fd, &ee); + return ret; +} + /* * Immediately remove file descriptor from epoll set upon close. * Since we forked, some fds share inodes with the other process, and epoll may diff --git a/src/fd.c b/src/fd.c index aeee602..483f3eb 100644 --- a/src/fd.c +++ b/src/fd.c @@ -198,6 +198,19 @@ void fd_delete(int fd) maxfd--; } +void fd_async_delete(int fd) +{ + fd_release_cache_entry(fd); + + fdtab[fd].state = 0; + fdtab[fd].async = 0; + fdtab[fd].owner = NULL; + fdtab[fd].new = 0; + + while ((maxfd-1 >= 0) && !fdtab[maxfd-1].owner) + maxfd--; +} + /* Scan and process the cached events. This should be called right after * the poller. The loop may cause new entries to be created, for example * if a listener causes an accept() to initiate a new incoming connection diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 8932d7c..03bd3b8 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -57,6 +57,7 @@ #include <import/xxhash.h> #include <openssl/engine.h> +#include <openssl/async.h> #include <common/buffer.h> #include <common/compat.h> @@ -68,6 +69,7 @@ #include <common/time.h> #include <common/cfgparse.h> #include <common/base64.h> +#include <common/epoll.h> #include <ebsttree.h> @@ -138,6 +140,7 @@ static struct { char *crt_base; /* base directory path for certificates */ char *ca_base; /* base directory path for CAs and CRLs */ char *engine; /* openssl engine to use */ + int async; /* whether we use ssl async mode */ char *listen_default_ciphers; char *connect_default_ciphers; @@ -302,6 +305,74 @@ void ssl_init_engine(const char *engine_id) } /* + * openssl async fd handler + */ +int ssl_async_process_fds(struct connection *conn) +{ + OSSL_ASYNC_FD *add_fds = NULL; + OSSL_ASYNC_FD *del_fds = NULL; + size_t num_add_fds = 0; + size_t num_del_fds = 0; + unsigned int loop = 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 0; + + if (num_add_fds) { + add_fds = OPENSSL_malloc(num_add_fds * sizeof(OSSL_ASYNC_FD)); + if (add_fds == NULL) { + Alert("Memory Allocation Error"); + return 0; + } + } + + if (num_del_fds) { + del_fds = OPENSSL_malloc(num_del_fds * sizeof(OSSL_ASYNC_FD)); + if (del_fds == NULL) { + Alert("Memory Allocation Error"); + if (add_fds) + OPENSSL_free(add_fds); + return 0; + } + } + + SSL_get_changed_async_fds(ssl, add_fds, &num_add_fds, del_fds, + &num_del_fds); + + if (num_del_fds) { + for (loop = 0; loop < num_del_fds; loop++) { + int async_fd = del_fds[loop]; + + fd_stop_recv(async_fd); + fd_async_delete(async_fd); + } + } + + if (num_add_fds) { + for (loop = 0; loop < num_add_fds; loop++) { + int async_fd = add_fds[loop]; + + fd_async_insert(async_fd); + fdtab[async_fd].iocb = conn_fd_handler; + conn->async_fd = async_fd; + fdtab[async_fd].owner = conn; + fd_want_recv(async_fd); + } + } + + if (add_fds) + OPENSSL_free(add_fds); + if (del_fds) + OPENSSL_free(del_fds); + + return 1; +} + +/* * This function returns the number of seconds elapsed * since the Epoch, 1970-01-01 00:00:00 +0000 (UTC) and the * date presented un ASN1_GENERALIZEDTIME. @@ -2906,6 +2977,9 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, struct ssl_bind_conf *ssl_ cfgerr++; } + if (global_ssl.async) + sslmode |= SSL_MODE_ASYNC; + if (conf_ssl_options & BC_SSL_O_NO_SSLV3) ssloptions |= SSL_OP_NO_SSLv3; if (conf_ssl_options & BC_SSL_O_NO_TLSV10) @@ -3340,6 +3414,10 @@ int ssl_sock_prepare_srv_ctx(struct server *srv) #endif #endif SSL_CTX_set_options(srv->ssl_ctx.ctx, options); + + if (global_ssl.async) + mode |= SSL_MODE_ASYNC; + SSL_CTX_set_mode(srv->ssl_ctx.ctx, mode); if (global.ssl_server_verify == SSL_SERVER_VERIFY_REQUIRED) @@ -3796,6 +3874,12 @@ 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_ASYNC) { + ssl_async_process_fds(conn); + return 0; + } + if (ret == SSL_ERROR_WANT_WRITE) { /* SSL handshake needs to write, L4 connection may not be ready */ __conn_sock_stop_recv(conn); @@ -3881,6 +3965,11 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag) /* handshake did not complete, let's find why */ ret = SSL_get_error(conn->xprt_ctx, ret); + if (ret == SSL_ERROR_WANT_ASYNC) { + ssl_async_process_fds(conn); + return 0; + } + if (ret == SSL_ERROR_WANT_WRITE) { /* SSL handshake needs to write, L4 connection may not be ready */ __conn_sock_stop_recv(conn); @@ -4066,6 +4155,11 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun } else { ret = SSL_get_error(conn->xprt_ctx, ret); + if (ret == SSL_ERROR_WANT_ASYNC) { + ssl_async_process_fds(conn); + break; + } + if (ret == SSL_ERROR_WANT_WRITE) { /* handshake is running, and it needs to enable write */ conn->flags |= CO_FL_SSL_WAIT_HS; @@ -4167,6 +4261,12 @@ 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_ASYNC) { + ssl_async_process_fds(conn); + break; + } + if (ret == SSL_ERROR_WANT_WRITE) { if (SSL_renegotiate_pending(conn->xprt_ctx)) { /* handshake is running, and it may need to re-enable write */ @@ -4201,6 +4301,11 @@ 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 (global_ssl.async) { + remove_fd_from_epoll(conn->async_fd); + fd_stop_recv(conn->async_fd); + fd_async_delete(conn->async_fd); + } SSL_free(conn->xprt_ctx); conn->xprt_ctx = NULL; sslconns--; @@ -6414,6 +6519,17 @@ static int ssl_parse_global_ca_crt_base(char **args, int section_type, struct pr return 0; } +/* parse the "ssl-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) +{ + global_ssl.async = 1; + return 0; +} + /* parse the "ssl-engine" keyword in global section. * Returns <0 on alert, >0 on warning, 0 on success. */ @@ -7007,6 +7123,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-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 -- 1.7.12.1
