MEDIUM: ssl: add a rwlock for SSL server session cache

When adding the server side support for certificate update over the CLI
we encountered a design problem with the SSL session cache which was not
locked.

Indeed, once a certificate is updated we need to flush the cache, but we
also need to ensure that the cache is not used during the update.
To prevent the use of the cache during an update, this patch introduce a
rwlock for the SSL server session cache.

In the SSL session part this patch only lock in read, even if it writes.
The reason behind this, is that in the session part, there is one cache
storage per thread so it is not a problem to write in the cache from
several threads. The problem is only when trying to write in the cache
from the CLI (which could be on any thread) when a session is trying to
access the cache. So there is a write lock in the CLI part to prevent
simultaneous access by a session and the CLI.

This patch also remove the thread_isolate attempt which is eating too
much CPU time and was not protecting from the use of a free ptr in the
session.

diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 32697a9c426baa307fc0f7cb04235ecc5bdf8f25..f404d30106f39bf8144475dcabab6b0ce21c227d 100644 (file)
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -311,6 +311,7 @@ struct server {
                } * reused_sess;
 
                struct ckch_inst *inst; /* Instance of the ckch_store in which the certificate was loaded (might be null if server has no certificate) */
+               __decl_thread(HA_RWLOCK_T lock); /* lock the cache and SSL_CTX during commit operations */
 
                char *ciphers;                  /* cipher suite to use if non-null */
 #ifdef HAVE_SSL_CTX_SET_CIPHERSUITES

diff --git a/include/haproxy/thread.h b/include/haproxy/thread.h
index e4d630ddf3018404f6a2c5815ddbd8fb0ad88881..9982a763fa2dfbfc2a6858e55e7cf3591975f5cd 100644 (file)
--- a/include/haproxy/thread.h
+++ b/include/haproxy/thread.h
@@ -398,6 +398,7 @@ enum lock_label {
        PROTO_LOCK,
        CKCH_LOCK,
        SNI_LOCK,
+       SSL_SERVER_LOCK,
        SFT_LOCK, /* sink forward target */
        OTHER_LOCK,
        LOCK_LABELS

diff --git a/src/server.c b/src/server.c
index da2325e9abfdf82a931996d4fef092fdebe510a7..453c598668720090a5e5ec34c4061ae0a7e6b743 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -1775,6 +1775,9 @@ struct server *new_server(struct proxy *proxy)
 #endif
 
        srv->extra_counters = NULL;
+#ifdef USE_OPENSSL
+       HA_RWLOCK_INIT(&srv->ssl_ctx.lock);
+#endif
 
        /* please don't put default server settings here, they are set in
         * init_default_instance().

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index f9b8f189a0dd8b1f836f96cae019a09a088934c1..f654b4b52806987c731b0339912367afbafb5750 100644 (file)
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -1400,14 +1400,11 @@ static int cli_io_handler_commit_cert(struct appctx *appctx)
                                        /* The bind_conf will be null on server ckch_instances. */
                                        if (ckchi->is_server_instance) {
                                                int i;
-
-                                               /* The certificate update on the server side (backend)
-                                                * can be done by rewriting a single pointer so no
-                                                * locks are needed here. */
+                                               /* a lock is needed here since we have to free the SSL cache */
+                                               HA_RWLOCK_WRLOCK(SSL_SERVER_LOCK, &ckchi->server->ssl_ctx.lock);
                                                /* free the server current SSL_CTX */
                                                SSL_CTX_free(ckchi->server->ssl_ctx.ctx);
                                                /* Actual ssl context update */
-                                               thread_isolate();
                                                SSL_CTX_up_ref(ckchi->ctx);
                                                ckchi->server->ssl_ctx.ctx = ckchi->ctx;
                                                ckchi->server->ssl_ctx.inst = ckchi;
@@ -1417,7 +1414,7 @@ static int cli_io_handler_commit_cert(struct appctx *appctx)
                                                        free(ckchi->server->ssl_ctx.reused_sess[i].ptr);
                                                        ckchi->server->ssl_ctx.reused_sess[i].ptr = NULL;
                                                }
-                                               thread_release();
+                                               HA_RWLOCK_WRUNLOCK(SSL_SERVER_LOCK, &ckchi->server->ssl_ctx.lock);
 
                                        } else {
                                                HA_RWLOCK_WRLOCK(SNI_LOCK, &ckchi->bind_conf->sni_lock);

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 099e7f922721068eafb28b72152a18bfce1f57ae..db5b01978b0801ab74480e2cdb9d98ab425bc731 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -3984,11 +3984,16 @@ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess)
 
        s = __objt_server(conn->target);
 
+       /* RWLOCK: only read lock the SSL cache even when writing in it because there is
+        * one cache per thread, it only prevents to flush it from the CLI in
+        * another thread */
+
        if (!(s->ssl_ctx.options & SRV_SSL_O_NO_REUSE)) {
                int len;
                unsigned char *ptr;
 
                len = i2d_SSL_SESSION(sess, NULL);
+               HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
                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 {
@@ -4000,9 +4005,12 @@ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess)
                        s->ssl_ctx.reused_sess[tid].size = i2d_SSL_SESSION(sess,
                            &ptr);
                }
+               HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
        } else {
+               HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
                free(s->ssl_ctx.reused_sess[tid].ptr);
                s->ssl_ctx.reused_sess[tid].ptr = NULL;
+               HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
        }
 
        return 0;
@@ -5265,6 +5273,7 @@ static int ssl_sock_init(struct connection *conn, void **xprt_ctx)
                        goto err;
 
                SSL_set_connect_state(ctx->ssl);
+               HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &(__objt_server(conn->target)->ssl_ctx.lock));
                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);
@@ -5276,6 +5285,7 @@ static int ssl_sock_init(struct connection *conn, void **xprt_ctx)
                                SSL_SESSION_free(sess);
                        }
                }
+               HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &(__objt_server(conn->target)->ssl_ctx.lock));
 
                /* leave init state and start handshake */
                conn->flags |= CO_FL_SSL_WAIT_HS | CO_FL_WAIT_L6_CONN;
@@ -5658,9 +5668,18 @@ reneg_ok:
        ERR_clear_error();
 
        /* free resumed session if exists */
-       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;
+       if (objt_server(conn->target)) {
+               struct server *s = __objt_server(conn->target);
+               /* RWLOCK: only rdlock the SSL cache even when writing in it because there is
+                * one cache per thread, it only prevents to flush it from the CLI in
+                * another thread */
+
+               HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
+               if (s->ssl_ctx.reused_sess[tid].ptr) {
+                       free(s->ssl_ctx.reused_sess[tid].ptr);
+                       s->ssl_ctx.reused_sess[tid].ptr = NULL;
+               }
+               HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
        }
 
        if (counters) {