MEDIUM: ssl_sock: always use the SSL's server name, not the one from the tid

In ssl_sock_set_servername(), we're retrieving the current server name
from the current thread, hoping it will not have changed. This is a
bit dangerous as strictly speaking it's not easy to prove that no other
connection had to use one between the moment it was retrieved in
ssl_sock_init() and the moment it's being read here. In addition, this
forces us to maintain one session per thread while this is not the real
need, in practice we only need one session per SNI. And the current model
prevents us from sharing sessions between threads.

This had been done in 2.5 via commit e18d4e828 ("BUG/MEDIUM: ssl: backend
TLS resumption with sni and TLSv1.3"), but as analyzed with William, it
turns out that a saner approach consists in keeping the call to
SSL_get_servername() there and instead to always assign the SNI to the
current SSL context via SSL_set_tlsext_host_name() immediately when the
session is retreived. This way the session and SNI are consulted atomically
and the host name is only checked from the session and not from possibly
changing elements.

As a bonus the rdlock that was added by that commit could now be removed,
though it didn't cost much.

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index f37d9e93148e3bb3b8bb22f54ec56f3368ebc8e4..59c3f549a5f38b2232c25cf5a8891c948f997753 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5722,9 +5722,13 @@ static int ssl_sock_init(struct connection *conn, void **xprt_ctx)
                        if (sess && !SSL_set_session(ctx->ssl, sess)) {
                                SSL_SESSION_free(sess);
                                ha_free(&srv->ssl_ctx.reused_sess[tid].ptr);
+                               if (srv->ssl_ctx.reused_sess[tid].sni)
+                                       SSL_set_tlsext_host_name(ctx->ssl, srv->ssl_ctx.reused_sess[tid].sni);
                        } else if (sess) {
                                /* already assigned, not needed anymore */
                                SSL_SESSION_free(sess);
+                               if (srv->ssl_ctx.reused_sess[tid].sni)
+                                       SSL_set_tlsext_host_name(ctx->ssl, srv->ssl_ctx.reused_sess[tid].sni);
                        }
                }
                HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &srv->ssl_ctx.lock);
@@ -6836,7 +6840,6 @@ void ssl_sock_set_servername(struct connection *conn, const char *hostname)
 {
 #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
        struct ssl_sock_ctx *ctx = conn_get_ssl_sock_ctx(conn);
-       struct server *s;
        char *prev_name;
 
        if (!ctx)
@@ -6845,22 +6848,21 @@ void ssl_sock_set_servername(struct connection *conn, const char *hostname)
        BUG_ON(!(conn->flags & CO_FL_WAIT_L6_CONN));
        BUG_ON(!(conn->flags & CO_FL_SSL_WAIT_HS));
 
-       s = __objt_server(conn->target);
-
        /* if the SNI changes, we must destroy the reusable context so that a
         * new connection will present a new SNI. compare with the SNI
-        * previously stored in the reused_sess */
-       /* the RWLOCK is used to ensure that we are not trying to flush the
-        * cache from the CLI */
+        * previously stored in the reused_sess. If the session was reused,
+        * the associated SNI (if any) has already been assigned to the SSL
+        * during ssl_sock_init() so SSL_get_servername() will properly
+        * retrieve the currently known hostname for the SSL.
+        */
 
-       HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
-       prev_name = s->ssl_ctx.reused_sess[tid].sni;
+       prev_name = (char *)SSL_get_servername(ctx->ssl, TLSEXT_NAMETYPE_host_name);
        if ((!prev_name && hostname) ||
-           (prev_name && (!hostname || strcmp(hostname, prev_name) != 0)))
+           !hostname ||
+           strcmp(hostname, prev_name) != 0) {
                SSL_set_session(ctx->ssl, NULL);
-       HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
-
-       SSL_set_tlsext_host_name(ctx->ssl, hostname);
+               SSL_set_tlsext_host_name(ctx->ssl, hostname);
+       }
 #endif
 }