Welcome! Log In Create A New Profile

Advanced

[PATCHES] Fix TLS 1.3 session resumption, and 0RTT with threads.

Posted by Olivier Houchard 
Olivier Houchard
[PATCHES] Fix TLS 1.3 session resumption, and 0RTT with threads.
November 16, 2017 06:40PM
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
On Thu, Nov 16, 2017 at 06:33:35PM +0100, Olivier Houchard wrote:
> 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.

Merged, thanks!

Willy
Sorry, only registered users may post in this forum.

Click here to login