MEDIUM: ssl/server: No longer store the SNI of cached TLS sessions

Thanks to the previous patch, "BUG/MEDIUM: ssl: Don't reuse TLS session
if the connection's SNI differs", it is no useless to store the SNI of
cached TLS sessions. This SNI is no longer tested and new connections
reusing a session must have the same SNI.

The main change here is for the ssl_sock_set_servername() function. It is no
longer possible to compare the SNI of the reused session with the one of the
new connection. So, the SNI is always set, with no other processing. Mainly,
the session is not destroyed when SNIs don't match. It means the commit
119a4084bf ("BUG/MEDIUM: ssl: for a handshake when server-side SNI changes")
is implicitly reverted.

It is good to note that it is unclear for me when and why the reused session
should be destroyed. Because I'm unable to reproduce any issue fixed by the
commit above.

This patch could be backported as far as 3.0 with the commit above.

diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 2dc27ecc43eff64fcdbd9eb45ae9bddfdb1f2b94..7a2a95400b92c9d0a4927cc77fe9df01c66d10c1 100644 (file)
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -486,7 +486,6 @@ struct server {
                        int size;
                        int allocated_size;
                        uint64_t sni_hash; /* Hash of the SNI used for the session */
-                       char *sni; /* SNI used for the session */
                        __decl_thread(HA_RWLOCK_T sess_lock);
                } * reused_sess;
 

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index 8ff6847c061f89d2c06358027ba0976ed8d318bb..900e89de3404e341587750ad13ca66cb42686be1 100644 (file)
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -2770,10 +2770,8 @@ static void __ssl_sock_load_new_ckch_instance(struct ckch_inst *ckchi)
                ckchi->server->ssl_ctx.inst = ckchi;
 
                /* flush the session cache of the server */
-               for (i = 0; i < global.nbthread; i++) {
-                       ha_free(&ckchi->server->ssl_ctx.reused_sess[i].sni);
+               for (i = 0; i < global.nbthread; i++)
                        ha_free(&ckchi->server->ssl_ctx.reused_sess[i].ptr);
-               }
                HA_RWLOCK_WRUNLOCK(SSL_SERVER_LOCK, &ckchi->server->ssl_ctx.lock);
 
        } else {

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 7a9a2da245e42fde7fbb365e4d295faec20edda5..601c976130ceea8c332c067d579d342ca403c13e 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -4201,14 +4201,12 @@ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess)
        if (!(s->ssl_ctx.options & SRV_SSL_O_NO_REUSE)) {
                int len;
                unsigned char *ptr;
-               const char *sni;
 #ifdef USE_QUIC
                struct quic_conn *qc = SSL_get_ex_data(ssl, ssl_qc_app_data_index);
 #endif
 
                /* determine the required len to store this new session */
                len = i2d_SSL_SESSION(sess, NULL);
-               sni = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
                HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &s->ssl_ctx.lock);
 
                ptr = s->ssl_ctx.reused_sess[tid].ptr;
@@ -4245,14 +4243,7 @@ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess)
                        HA_ATOMIC_CAS(&s->ssl_ctx.last_ssl_sess_tid, &old_tid, 0); // no more valid
                else if (s->ssl_ctx.reused_sess[tid].ptr && !old_tid)
                        HA_ATOMIC_CAS(&s->ssl_ctx.last_ssl_sess_tid, &old_tid, tid + 1);
-
-               if (s->ssl_ctx.reused_sess[tid].sni_hash != conn->sni_hash) {
-                       /* if the new sni hash or isn' t the same as the old one */
-                       ha_free(&s->ssl_ctx.reused_sess[tid].sni);
-                       s->ssl_ctx.reused_sess[tid].sni_hash = conn->sni_hash;
-                       if (sni)
-                               s->ssl_ctx.reused_sess[tid].sni = strdup(sni);
-               }
+               s->ssl_ctx.reused_sess[tid].sni_hash = conn->sni_hash;
 #ifdef USE_QUIC
                /* The selected ALPN is not stored without SSL session. */
                if (qc && (s->ssl_ctx.options & SRV_SSL_O_EARLY_DATA) &&
@@ -5477,10 +5468,8 @@ void ssl_sock_free_srv_ctx(struct server *srv)
        if (srv->ssl_ctx.reused_sess) {
                int i;
 
-               for (i = 0; i < global.nbthread; i++) {
+               for (i = 0; i < global.nbthread; i++)
                        ha_free(&srv->ssl_ctx.reused_sess[i].ptr);
-                       ha_free(&srv->ssl_ctx.reused_sess[i].sni);
-               }
                ha_free(&srv->ssl_ctx.reused_sess);
        }
 
@@ -5723,16 +5712,10 @@ int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
                        SSL_SESSION_free(sess);
                        HA_RWLOCK_WRLOCK(SSL_SERVER_LOCK, &srv->ssl_ctx.reused_sess[tid].sess_lock);
                        ha_free(&srv->ssl_ctx.reused_sess[tid].ptr);
-                       HA_RWLOCK_WRTORD(SSL_SERVER_LOCK, &srv->ssl_ctx.reused_sess[tid].sess_lock);
-                       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.reused_sess[tid].sess_lock);
+                       HA_RWLOCK_WRUNLOCK(SSL_SERVER_LOCK, &srv->ssl_ctx.reused_sess[tid].sess_lock);
                } else if (sess) {
                        /* already assigned, not needed anymore */
                        SSL_SESSION_free(sess);
-                       HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &srv->ssl_ctx.reused_sess[tid].sess_lock);
-                       if (srv->ssl_ctx.reused_sess[tid].sni)
-                               SSL_set_tlsext_host_name(ctx->ssl, srv->ssl_ctx.reused_sess[tid].sni);
 #ifdef USE_QUIC
                        if (qc && srv->ssl_ctx.options & SRV_SSL_O_EARLY_DATA) {
                                unsigned char *alpn = (unsigned char *)srv->path_params.nego_alpn;
@@ -5743,7 +5726,6 @@ int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
                                        ret = 1;
                        }
 #endif
-                       HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &srv->ssl_ctx.reused_sess[tid].sess_lock);
                }
        } else {
                /* No session available yet, let's see if we can pick one
@@ -5787,9 +5769,6 @@ int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
                                }
                        }
 
-                       if (srv->ssl_ctx.reused_sess[old_tid-1].sni)
-                               SSL_set_tlsext_host_name(ctx->ssl, srv->ssl_ctx.reused_sess[old_tid-1].sni);
-
                        HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &srv->ssl_ctx.reused_sess[old_tid-1].sess_lock);
                }
        }
@@ -7632,7 +7611,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);
-       char *prev_name;
 
        if (!ctx || !hostname)
                return;
@@ -7640,19 +7618,7 @@ 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));
 
-       /* 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. 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.
-        */
-
-       prev_name = (char *)SSL_get_servername(ctx->ssl, TLSEXT_NAMETYPE_host_name);
-       if (!prev_name || strcmp(hostname, prev_name) != 0) {
-               SSL_set_session(ctx->ssl, NULL);
-               SSL_set_tlsext_host_name(ctx->ssl, hostname);
-       }
+       SSL_set_tlsext_host_name(ctx->ssl, hostname);
 #endif
 }