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 <[email protected]>

diff --git a/src/plugins/hs_apps/alpn_server.c b/src/plugins/hs_apps/alpn_server.c
index 5ac82da..7de4be4 100644 (file)
--- 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 af06ccd..e52b0b0 100644 (file)
--- 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 dfe5679..f6fc3b3 100644 (file)
--- 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 4ff93ec..6d57c85 100644 (file)
--- 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 48d2359..ea885a8 100644 (file)
--- 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) {