RepoMirrors/haproxy
1
0
Fork 0
mirror of http://git.haproxy.org/git/haproxy.git/ synced 2025-01-05 11:39:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Willy Tarreau 2023-08-30 12:02:33 +02:00
parent 335b5adf2c
commit 95ac5fe4a8
1 changed files with 14 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
src/ssl_sock.c
View File

@ -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
}