From: Matus Fabian Date: Thu, 9 Oct 2025 12:42:22 +0000 (-0400) Subject: quic: failed handshake improvements X-Git-Url: https://gerrit.fd.io/r/gitweb?a=commitdiff_plain;h=refs%2Fchanges%2F66%2F43866%2F2;p=vpp.git quic: failed handshake improvements - notify client app that connect failed - do not create session for server app Type: improvement Change-Id: Id93d1f438ddd656fad2249528981bf9c936eb7a7 Signed-off-by: Matus Fabian --- diff --git a/src/plugins/hs_apps/alpn_server.c b/src/plugins/hs_apps/alpn_server.c index 5ac82da7452..7de4be4a760 100644 --- a/src/plugins/hs_apps/alpn_server.c +++ b/src/plugins/hs_apps/alpn_server.c @@ -14,6 +14,7 @@ typedef struct u32 ckpair_index; u8 alpn_protos[4]; vlib_main_t *vlib_main; + u32 accepted_count; } alpn_server_main_t; alpn_server_main_t alpn_server_main; @@ -35,6 +36,7 @@ as_ts_tx_callback (session_t *ts) static int as_ts_accept_callback (session_t *ts) { + alpn_server_main_t *sm = &alpn_server_main; tls_alpn_proto_t alpn_proto; ts->session_state = SESSION_STATE_READY; @@ -42,6 +44,7 @@ as_ts_accept_callback (session_t *ts) alpn_proto = transport_get_alpn_selected ( session_get_transport_proto (ts), ts->connection_index, ts->thread_index); clib_warning ("ALPN selected: %U", format_tls_alpn_proto, alpn_proto); + sm->accepted_count++; return 0; } @@ -250,6 +253,26 @@ VLIB_CLI_COMMAND (alpn_server_create_command, static) = { .function = alpn_server_create_command_fn, }; +static clib_error_t * +show_alpn_server_fn (vlib_main_t *vm, unformat_input_t *input, + vlib_cli_command_t *cmd) +{ + alpn_server_main_t *sm = &alpn_server_main; + + if (unformat_check_input (input) != UNFORMAT_END_OF_INPUT) + return clib_error_return (0, "unknown input `%U'", format_unformat_error, + input); + + vlib_cli_output (vm, "accepted connections %u", sm->accepted_count); + return 0; +} + +VLIB_CLI_COMMAND (show_alpn_server, static) = { + .path = "show test alpn server", + .short_help = "show test alpn server", + .function = show_alpn_server_fn, +}; + clib_error_t * alpn_server_main_init (vlib_main_t *vm) { diff --git a/src/plugins/quic/quic.c b/src/plugins/quic/quic.c index af06ccdb5d7..e52b0b09e8d 100644 --- a/src/plugins/quic/quic.c +++ b/src/plugins/quic/quic.c @@ -646,7 +646,7 @@ quic_udp_session_connected_callback (u32 quic_app_index, u32 ctx_index, if (app_wrk) { api_context = ctx->c_s_index; - app_worker_connect_notify (app_wrk, 0, SESSION_E_NONE, api_context); + app_worker_connect_notify (app_wrk, 0, err, api_context); } return 0; } diff --git a/src/plugins/quic_quicly/quic_quicly.c b/src/plugins/quic_quicly/quic_quicly.c index dfe5679290b..f6fc3b306dc 100644 --- a/src/plugins/quic_quicly/quic_quicly.c +++ b/src/plugins/quic_quicly/quic_quicly.c @@ -89,6 +89,24 @@ quic_quicly_connection_delete (quic_ctx_t *ctx) session_transport_delete_notify (&ctx->connection); } +static void +quic_quicly_notify_app_connect_failed (quic_ctx_t *ctx, session_error_t err) +{ + app_worker_t *app_wrk; + int rv; + + app_wrk = app_worker_get (ctx->parent_app_wrk_id); + if (!app_wrk) + { + QUIC_DBG (2, "no app worker: ctx_index %u, thread %u", ctx->c_c_index, + ctx->c_thread_index); + return; + } + if ((rv = app_worker_connect_notify (app_wrk, 0, err, ctx->client_opaque))) + QUIC_ERR ("failed to notify app: err %d, ctx_index %u, thread %u", rv, + ctx->c_c_index, ctx->c_thread_index); +} + /** * Called when quicly return an error * This function interacts tightly with quic_quicly_proto_on_close @@ -99,9 +117,6 @@ quic_quicly_connection_closed (quic_ctx_t *ctx) QUIC_DBG (2, "QUIC connection %u/%u closed", ctx->c_thread_index, ctx->c_c_index); - /* TODO if connection is not established, just delete the session? */ - /* Actually should send connect or accept error */ - switch (ctx->conn_state) { case QUIC_CONN_STATE_READY: @@ -122,8 +137,12 @@ quic_quicly_connection_closed (quic_ctx_t *ctx) /* App already confirmed close, we can delete the connection */ quic_quicly_connection_delete (ctx); break; - case QUIC_CONN_STATE_OPENED: case QUIC_CONN_STATE_HANDSHAKE: + /* handshake failed notify app that connect failed */ + quic_quicly_notify_app_connect_failed (ctx, SESSION_E_TLS_HANDSHAKE); + quic_quicly_connection_delete (ctx); + break; + case QUIC_CONN_STATE_OPENED: case QUIC_CONN_STATE_ACTIVE_CLOSING: quic_quicly_connection_delete (ctx); break; @@ -893,6 +912,16 @@ quic_quicly_accept_connection (quic_quicly_rx_packet_ctx_t *pctx) quic_quicly_store_conn_ctx (conn, ctx); ctx->conn = conn; + /* if handshake failed (e.g. ALPN negotiation failed) quicly connection is in + * closing state, in this case we don't need to create session and notify + * app, connection will be closed when error response is sent */ + if (quicly_get_state (conn) >= QUICLY_STATE_CLOSING) + { + QUIC_DBG (2, "Handshake failed, closing: ctx_index %u, thread %u", + ctx->c_c_index, ctx->c_thread_index); + return; + } + quic_session = session_alloc (ctx->c_thread_index); QUIC_DBG (2, "Accept connection (new quic_session): session 0x%lx, " diff --git a/src/plugins/quic_quicly/quic_quicly_crypto.c b/src/plugins/quic_quicly/quic_quicly_crypto.c index 4ff93ecaf7d..6d57c85b557 100644 --- a/src/plugins/quic_quicly/quic_quicly_crypto.c +++ b/src/plugins/quic_quicly/quic_quicly_crypto.c @@ -222,6 +222,12 @@ quic_quicly_on_closed_by_remote (quicly_closed_by_remote_t *self, ctx->c_c_index, ctx->c_thread_index); } #endif + if (ctx->conn_state == QUIC_CONN_STATE_HANDSHAKE) + { + QUIC_DBG (2, "Handshake failed: ctx_index %u, thread %u", ctx->c_c_index, + ctx->c_thread_index); + return; + } ctx->conn_state = QUIC_CONN_STATE_PASSIVE_CLOSING; if (ctx->c_s_index != QUIC_SESSION_INVALID) { diff --git a/test-c/hs-test/quic_test.go b/test-c/hs-test/quic_test.go index 48d235952ec..ea885a82606 100644 --- a/test-c/hs-test/quic_test.go +++ b/test-c/hs-test/quic_test.go @@ -5,10 +5,10 @@ import ( ) func init() { - RegisterVethTests(QuicAlpMatchTest, QuicAlpnOverlapMatchTest, QuicAlpnServerPriorityMatchTest, QuicAlpnMismatchTest, QuicAlpnEmptyServerListTest, QuicAlpnEmptyClientListTest) + RegisterVethTests(QuicAlpnMatchTest, QuicAlpnOverlapMatchTest, QuicAlpnServerPriorityMatchTest, QuicAlpnMismatchTest, QuicAlpnEmptyServerListTest, QuicAlpnEmptyClientListTest) } -func QuicAlpMatchTest(s *VethsSuite) { +func QuicAlpnMatchTest(s *VethsSuite) { serverAddress := s.Interfaces.Server.Ip4AddressString() + ":" + s.Ports.Port1 s.Log(s.Containers.ServerVpp.VppInstance.Vppctl("test alpn server alpn-proto1 3 uri quic://" + serverAddress)) @@ -48,17 +48,30 @@ func QuicAlpnServerPriorityMatchTest(s *VethsSuite) { } func QuicAlpnMismatchTest(s *VethsSuite) { - s.Skip("QUIC bug: handshake failure not reported to client app as connect error, skipping...") + serverVpp := s.Containers.ServerVpp.VppInstance + clientVpp := s.Containers.ClientVpp.VppInstance serverAddress := s.Interfaces.Server.Ip4AddressString() + ":" + s.Ports.Port1 - s.Log(s.Containers.ServerVpp.VppInstance.Vppctl("test alpn server alpn-proto1 2 alpn-proto2 1 uri quic://" + serverAddress)) + s.Log(serverVpp.Vppctl("test alpn server alpn-proto1 2 alpn-proto2 1 uri quic://" + serverAddress)) uri := "quic://" + serverAddress - o := s.Containers.ClientVpp.VppInstance.Vppctl("test alpn client alpn-proto1 3 alpn-proto2 4 uri " + uri) + o := clientVpp.Vppctl("test alpn client alpn-proto1 3 alpn-proto2 4 uri " + uri) s.Log(o) s.AssertNotContains(o, "timeout") s.AssertNotContains(o, "ALPN selected") // connection refused on mismatch - s.AssertContains(o, "connect error failed quic handshake") + s.AssertContains(o, "connect error failed tls handshake") + // check if everything is cleanup + // server should have only 2 listener sessions (udp and quic) and app no accepted connection + o = serverVpp.Vppctl("show test alpn server") + s.Log(o) + s.AssertContains(o, "accepted connections 0") + o = serverVpp.Vppctl("show session verbose 2") + s.Log(o) + s.AssertContains(o, "active sessions 2") + // no session on client + o = clientVpp.Vppctl("show session verbose 2") + s.Log(o) + s.AssertContains(o, "no sessions") } func QuicAlpnEmptyServerListTest(s *VethsSuite) {