Hi, The first patch attempts fo fix session resumption with TLS 1.3, when haproxy acts as a client, by storing the ASN1-encoded session in the struct server, instead of storing the SSL_SESSION *directly. Directly keeping SSL_SESSION doesn't seem to work well when concurrent connections are made using the same session. The second patch tries to make sure the SSL handshake is done before calling the shutw method. Not doing so may be result in getting errors, which ultimately leads to the client connection being closed, when it shouldn't be. This mostly happens when more than 1 thread is used.
Regards, Olivier
>From e32a831c1cbff1fcfb66565273ec98052f3a7f79 Mon Sep 17 00:00:00 2001 From: Olivier Houchard <[email protected]> Date: Thu, 16 Nov 2017 17:42:52 +0100 Subject: [PATCH 1/2] MINOR: SSL: Store the ASN1 representation of client sessions. Instead of storing the SSL_SESSION pointer directly in the struct server, store the ASN1 representation, otherwise, session resumption is broken with TLS 1.3, when multiple outgoing connections want to use the same session. --- include/types/server.h | 6 ++++- src/ssl_sock.c | 60 ++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 49 insertions(+), 17 deletions(-) diff --git a/include/types/server.h b/include/types/server.h index 76225f7d3..92dcdbb5c 100644 --- a/include/types/server.h +++ b/include/types/server.h @@ -274,7 +274,11 @@ struct server { char *sni_expr; /* Temporary variable to store a sample expression for SNI */ struct { SSL_CTX *ctx; - SSL_SESSION **reused_sess; + struct { + unsigned char *ptr; + int size; + int allocated_size; + } * reused_sess; char *ciphers; /* cipher suite to use if non-null */ int options; /* ssl options */ struct tls_version_filter methods; /* ssl methods */ diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 72d9b8aee..1162aa318 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -3856,19 +3856,35 @@ static int sh_ssl_sess_store(unsigned char *s_id, unsigned char *data, int data_ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess) { struct connection *conn = SSL_get_app_data(ssl); + struct server *s; - /* check if session was reused, if not store current session on server for reuse */ - if (objt_server(conn->target)->ssl_ctx.reused_sess[tid]) { - SSL_SESSION_free(objt_server(conn->target)->ssl_ctx.reused_sess[tid]); - objt_server(conn->target)->ssl_ctx.reused_sess[tid] = NULL; - } + s = objt_server(conn->target); - if (!(objt_server(conn->target)->ssl_ctx.options & SRV_SSL_O_NO_REUSE)) - objt_server(conn->target)->ssl_ctx.reused_sess[tid] = SSL_get1_session(conn->xprt_ctx); + if (!(s->ssl_ctx.options & SRV_SSL_O_NO_REUSE)) { + int len; + unsigned char *ptr; - return 1; + len = i2d_SSL_SESSION(sess, NULL); + if (s->ssl_ctx.reused_sess[tid].ptr && s->ssl_ctx.reused_sess[tid].allocated_size >= len) { + ptr = s->ssl_ctx.reused_sess[tid].ptr; + } else { + free(s->ssl_ctx.reused_sess[tid].ptr); + ptr = s->ssl_ctx.reused_sess[tid].ptr = malloc(len); + s->ssl_ctx.reused_sess[tid].allocated_size = len; + } + if (s->ssl_ctx.reused_sess[tid].ptr) { + s->ssl_ctx.reused_sess[tid].size = i2d_SSL_SESSION(sess, + &ptr); + } + } else { + free(s->ssl_ctx.reused_sess[tid].ptr); + s->ssl_ctx.reused_sess[tid].ptr = NULL; + } + + return 0; } + /* SSL callback used on new session creation */ int sh_ssl_sess_new_cb(SSL *ssl, SSL_SESSION *sess) { @@ -4434,7 +4450,7 @@ int ssl_sock_prepare_srv_ctx(struct server *srv) /* Initiate SSL context for current server */ if (!srv->ssl_ctx.reused_sess) { - if ((srv->ssl_ctx.reused_sess = calloc(1, global.nbthread*sizeof(SSL_SESSION*))) == NULL) { + if ((srv->ssl_ctx.reused_sess = calloc(1, global.nbthread*sizeof(*srv->ssl_ctx.reused_sess))) == NULL) { Alert("Proxy '%s', server '%s' [%s:%d] out of memory.\n", curproxy->id, srv->id, srv->conf.file, srv->conf.line); @@ -4923,10 +4939,15 @@ static int ssl_sock_init(struct connection *conn) } SSL_set_connect_state(conn->xprt_ctx); - if (objt_server(conn->target)->ssl_ctx.reused_sess) { - if(!SSL_set_session(conn->xprt_ctx, objt_server(conn->target)->ssl_ctx.reused_sess[tid])) { - SSL_SESSION_free(objt_server(conn->target)->ssl_ctx.reused_sess[tid]); - objt_server(conn->target)->ssl_ctx.reused_sess[tid] = NULL; + if (objt_server(conn->target)->ssl_ctx.reused_sess[tid].ptr) { + const unsigned char *ptr = objt_server(conn->target)->ssl_ctx.reused_sess[tid].ptr; + SSL_SESSION *sess = d2i_SSL_SESSION(NULL, &ptr, objt_server(conn->target)->ssl_ctx.reused_sess[tid].size); + if(sess && !SSL_set_session(conn->xprt_ctx, sess)) { + SSL_SESSION_free(sess); + free(objt_server(conn->target)->ssl_ctx.reused_sess[tid].ptr); + objt_server(conn->target)->ssl_ctx.reused_sess[tid].ptr = NULL; + } else if (sess) { + SSL_SESSION_free(sess); } } @@ -5134,6 +5155,13 @@ check_error: if (ret != 1) { /* handshake did not complete, let's find why */ ret = SSL_get_error(conn->xprt_ctx, ret); + if (ret != SSL_ERROR_WANT_READ && ret != SSL_ERROR_WANT_WRITE) + { + printf("bite lol %d %p %s conn %p\n", ret, objt_server(conn->target), ERR_error_string(ERR_get_error(), NULL), conn); + if (objt_server(conn->target)) { + printf("SES %p\n", SSL_get0_session(conn->xprt_ctx)); + } + } if (ret == SSL_ERROR_WANT_WRITE) { /* SSL handshake needs to write, L4 connection may not be ready */ @@ -5261,9 +5289,9 @@ reneg_ok: ERR_clear_error(); /* free resumed session if exists */ - if (objt_server(conn->target) && objt_server(conn->target)->ssl_ctx.reused_sess[tid]) { - SSL_SESSION_free(objt_server(conn->target)->ssl_ctx.reused_sess[tid]); - objt_server(conn->target)->ssl_ctx.reused_sess[tid] = NULL; + if (objt_server(conn->target) && objt_server(conn->target)->ssl_ctx.reused_sess[tid].ptr) { + free(objt_server(conn->target)->ssl_ctx.reused_sess[tid].ptr); + objt_server(conn->target)->ssl_ctx.reused_sess[tid].ptr = NULL; } /* Fail on all other handshake errors */ -- 2.13.5
>From eefffc65fd1c45cd10692e7efd8b872dbdf05ab5 Mon Sep 17 00:00:00 2001 From: Olivier Houchard <[email protected]> Date: Thu, 16 Nov 2017 17:49:25 +0100 Subject: [PATCH 2/2] MINOR: ssl: Make sure we don't shutw the connection before the handshake. Instead of trying to finish the handshake in ssl_sock_shutw, which may fail, try not to shutdown until the handshake is finished. --- src/ssl_sock.c | 7 ------- src/stream_interface.c | 4 +++- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 1162aa318..0b5b6361c 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -5663,13 +5663,6 @@ static void ssl_sock_close(struct connection *conn) { */ static void ssl_sock_shutw(struct connection *conn, int clean) { - /* If we're done with the connection before we did the handshake - * force the handshake anyway, so that the session is in a consistent - * state - */ - if (conn->flags & CO_FL_EARLY_SSL_HS) - SSL_do_handshake(conn->xprt_ctx); - if (conn->flags & CO_FL_HANDSHAKE) return; if (!clean) diff --git a/src/stream_interface.c b/src/stream_interface.c index 9eef3a2f0..3ccb2ec7d 100644 --- a/src/stream_interface.c +++ b/src/stream_interface.c @@ -458,8 +458,10 @@ void stream_int_notify(struct stream_interface *si) /* process consumer side */ if (channel_is_empty(oc)) { + struct connection *conn = objt_cs(si->end) ? objt_cs(si->end)->conn : NULL; + if (((oc->flags & (CF_SHUTW|CF_SHUTW_NOW)) == CF_SHUTW_NOW) && - (si->state == SI_ST_EST)) + (si->state == SI_ST_EST) && (!conn || !(conn->flags & (CO_FL_HANDSHAKE | CO_FL_EARLY_SSL_HS)))) si_shutw(si); oc->wex = TICK_ETERNITY; } -- 2.13.5
