BUG/MINOR: quic: missing app ops init during backend 0-RTT sessions

The QUIC mux requires "application operations" (app ops), which are a list
of callbacks associated with the application level (i.e., h3, h0.9) and
derived from the ALPN. For 0-RTT, when the session cache cannot be reused
before activation, the current code fails to reach the initialization of
these app ops, causing the mux to crash during its initialization.

To fix this, this patch restores the behavior of
ssl_sock_srv_try_reuse_sess(), whose purpose was to reuse sessions stored
in the session cache regardless of whether 0-RTT was enabled, prior to
this commit:

  MEDIUM: quic-be: modify ssl_sock_srv_try_reuse_sess() to reuse backend
  sessions (0-RTT)

With this patch, this function now does only one thing: attempt to reuse a
session, and that's it!

This patch allows ignoring whether a session was successfully reused from
the cache or not. This directly fixes the issue where app ops
initialization was skipped upon a session cache reuse failure. From a
functional standpoint, starting a mux without reusing the session cache
has no negative impact; the mux will start, but with no early data to
send.

Finally, there is the case where the ALPN is reset when the backend is
stopped. It is critical to continue locking read access to the ALPN to
secure shared access, which this patch does. It is indeed possible for the
server to be stopped between the call to connect_server() and
quic_reuse_srv_params(). But this cannot prevent the mux to start
without app ops. This is why a 'TODO' section was added, as a reminder that a
race condition regarding the ALPN reset still needs to be fixed.

Must be backported to 3.3

diff --git a/include/haproxy/ssl_sock.h b/include/haproxy/ssl_sock.h
index a48a2e2aabbc803f768d0ab1ec9ad89aade845ae..83b6de30067dcf53675eddd05284ec16dc37a494 100644 (file)
--- a/include/haproxy/ssl_sock.h
+++ b/include/haproxy/ssl_sock.h
@@ -73,7 +73,7 @@ int ssl_sock_get_alpn(const struct connection *conn, void *xprt_ctx,
                       const char **str, int *len);
 int ssl_bio_and_sess_init(struct connection *conn, SSL_CTX *ssl_ctx,
                           SSL **ssl, BIO **bio, BIO_METHOD *bio_meth, void *ctx);
-int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv);
+void ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv);
 const char *ssl_sock_get_sni(struct connection *conn);
 const char *ssl_sock_get_cert_sig(struct connection *conn);
 const char *ssl_sock_get_cipher_name(struct connection *conn);

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 5b643fd3a1b63243da3010f44cb3854df2f5db21..ac4cf2a84026ca5441a3065fa5a6ae23cb1f9abf 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -285,6 +285,8 @@ int quic_set_app_ops(struct quic_conn *qc, const unsigned char *alpn, size_t alp
 }
 
 /* Try to reuse <alpn> ALPN and <etps> early transport parameters.
+ * This function also sets the application operations calling
+ * quic_set_app_ops().
  * Return 1 if succeeded, 0 if not.
  */
 int quic_reuse_srv_params(struct quic_conn *qc,

diff --git a/src/quic_ssl.c b/src/quic_ssl.c
index c838a5e0676771512218f500293c18160f974c79..00e2c1e320bb463994bb628393a6d2b1b643b94a 100644 (file)
--- a/src/quic_ssl.c
+++ b/src/quic_ssl.c
@@ -1353,23 +1353,39 @@ int qc_alloc_ssl_sock_ctx(struct quic_conn *qc, void *target)
                if (!qc_ssl_set_quic_transport_params(ctx->ssl, qc, quic_version_1, 0))
                        goto err;
 
-               if (!(srv->ssl_ctx.options & SRV_SSL_O_EARLY_DATA))
-                   ssl_sock_srv_try_reuse_sess(ctx, srv);
+               ssl_sock_srv_try_reuse_sess(ctx, srv);
 #if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L) && defined(HAVE_SSL_0RTT_QUIC)
-               else {
-                       /* Enable early data only if the SSL session, transport parameters
-                        * and application protocol could be reused. This insures the mux is
-                        * correctly selected.
+               if ((srv->ssl_ctx.options & SRV_SSL_O_EARLY_DATA)) {
+                       int ret;
+                       unsigned char *alpn;
+                       struct quic_early_transport_params *etps;
+                       /* This code is called by connect_server() by way of
+                        * conn_prepare().
+                        * XXX TODO XXX: there is a remaining race condition where
+                        * the negotiated alpn could be resetted before running this code
+                        * here. In this case the app_ops for the mux will not be
+                        * set by quic_reuse_srv_params().
+                        *
+                        * Enable the early data only if the transport parameters
+                        * and application protocol could be reused. This insures that
+                        * no early-data level secrets will be derived if this is not
+                        * the case, leading the mux to be started but without being
+                        * able to send data at early-data level.
                         */
-                       if (ssl_sock_srv_try_reuse_sess(ctx, srv))
+                       HA_RWLOCK_RDLOCK(SERVER_LOCK, &srv->path_params.param_lock);
+                       alpn = (unsigned char *)srv->path_params.nego_alpn;
+                       etps = &srv->path_params.tps;
+                       ret = quic_reuse_srv_params(qc, alpn, etps);
+                       HA_RWLOCK_RDUNLOCK(SERVER_LOCK, &srv->path_params.param_lock);
+                       if (ret) {
                                SSL_set_quic_early_data_enabled(ctx->ssl, 1);
+                       }
                        else {
                                /* No error here. 0-RTT will not be enabled. */
                                TRACE_PROTO("Could not reuse any ALPN", QUIC_EV_CONN_NEW, qc);
                        }
                }
 #endif
-
                SSL_set_connect_state(ctx->ssl);
        }
 

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index c1ebf7c353bdaf4295dda637f38e7077dae11421..19b975283d3f0f70b19a5f87b9b02a333c7cea24 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -5689,7 +5689,7 @@ int increment_sslconn()
  * Return 1 if succeeded, 0 if not. Always succeeds for TCP socket. May fail
  * for QUIC sockets.
  */
-int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
+void ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
 {
 #ifdef USE_QUIC
        struct quic_conn *qc = ctx->qc;
@@ -5697,11 +5697,9 @@ int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
         * be set to success(1) only if the QUIC connection parameters
         * (transport parameters and ALPN) are successfully reused.
         */
-       int ret = qc && (srv->ssl_ctx.options & SRV_SSL_O_EARLY_DATA) ? 0 : 1;
        struct connection *conn = qc ? qc->conn : ctx->conn;
 #else
        /* Always succeeds for TCP sockets. */
-       int ret = 1;
        struct connection *conn = ctx->conn;
 #endif
 
@@ -5709,7 +5707,7 @@ int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
         * Always fail for check connections
         */
        if (conn->flags & CO_FL_SSL_NO_CACHED_INFO)
-               return 0;
+               return;
 
        HA_RWLOCK_RDLOCK(SSL_SERVER_LOCK, &srv->ssl_ctx.lock);
        if (srv->ssl_ctx.reused_sess[tid].ptr) {
@@ -5742,16 +5740,6 @@ int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
                } else if (sess) {
                        /* already assigned, not needed anymore */
                        SSL_SESSION_free(sess);
-#ifdef USE_QUIC
-                       if (qc && srv->ssl_ctx.options & SRV_SSL_O_EARLY_DATA) {
-                               unsigned char *alpn = (unsigned char *)srv->path_params.nego_alpn;
-                               struct quic_early_transport_params *etps = &srv->path_params.tps;
-
-                               if (quic_reuse_srv_params(qc, alpn, etps))
-                                       /* Success */
-                                       ret = 1;
-                       }
-#endif
                }
        } else {
                /* No session available yet, let's see if we can pick one
@@ -5781,16 +5769,6 @@ int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
                                if (sess) {
                                        if (!SSL_set_session(ctx->ssl, sess))
                                                HA_ATOMIC_CAS(&srv->ssl_ctx.last_ssl_sess_tid, &old_tid, 0); // no more valid
-#ifdef USE_QUIC
-                                       else if (qc && srv->ssl_ctx.options & SRV_SSL_O_EARLY_DATA) {
-                                               unsigned char *alpn = (unsigned char *)srv->path_params.nego_alpn;
-                                               struct quic_early_transport_params *etps = &srv->path_params.tps;
-
-                                               if (quic_reuse_srv_params(qc, alpn, etps))
-                                                       /* Success */
-                                                       ret = 1;
-                                       }
-#endif
                                        SSL_SESSION_free(sess);
                                }
                        }
@@ -5800,8 +5778,6 @@ int ssl_sock_srv_try_reuse_sess(struct ssl_sock_ctx *ctx, struct server *srv)
        }
   out:
        HA_RWLOCK_RDUNLOCK(SSL_SERVER_LOCK, &srv->ssl_ctx.lock);
-
-       return ret;
 }
 
 /*