From 439d521b7e41e2462badb72f4bdaee0071903c00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20Rubinos=20Rodr=C3=ADguez?= Date: Tue, 2 Jun 2026 16:56:52 +0200 Subject: [PATCH] fix(handshake): fragment CRYPTO flight to honor peer max_udp_payload_size (#2) Also advertise only the configured version_preference in the version_information transport param instead of all supported_versions. --- src/nquic_conn_init.erl | 10 ++- src/nquic_protocol_handshake.erl | 2 +- src/nquic_protocol_send.erl | 98 ++++++++++++++++++++++++++++++ src/nquic_protocol_send_queues.erl | 10 +-- test/nquic_integration_SUITE.erl | 5 +- test/nquic_protocol_tests.erl | 62 +++++++++++++++++++ 6 files changed, 175 insertions(+), 12 deletions(-) diff --git a/src/nquic_conn_init.erl b/src/nquic_conn_init.erl index 3cb2ee1..93961c3 100644 --- a/src/nquic_conn_init.erl +++ b/src/nquic_conn_init.erl @@ -89,9 +89,15 @@ new_conn_state(Opts) -> SessionTicket = maps:get(session_ticket, Opts, undefined), BaseParams = maps:get(transport_params, Opts, #transport_params{}), + VersionPref = maps:get(version_preference, Opts, [1]), + AvailableVersions = + case Role of + server -> VersionPref; + client -> nquic_packet:supported_versions() + end, VersionInfo = #{ chosen_version => Version, - other_versions => nquic_packet:supported_versions() + other_versions => AvailableVersions }, MaxIdleTimeout = nquic_conn_timers:idle_timeout_to_param( maps:get(idle_timeout, Opts, BaseParams#transport_params.max_idle_timeout) @@ -178,7 +184,7 @@ new_conn_state(Opts) -> max_payload_size = maps:get(max_payload_size, Opts, 1200), local_params = LocalParams, version = Version, - version_preference = maps:get(version_preference, Opts, [1]), + version_preference = VersionPref, crypto = Crypto, streams_state = Streams, path = PathMgmt, diff --git a/src/nquic_protocol_handshake.erl b/src/nquic_protocol_handshake.erl index ed6884c..99dec00 100644 --- a/src/nquic_protocol_handshake.erl +++ b/src/nquic_protocol_handshake.erl @@ -972,7 +972,7 @@ apply_server_compat_version_switch(NewVersion, State) -> undefined -> #{ chosen_version => NewVersion, - other_versions => nquic_packet:supported_versions() + other_versions => State#conn_state.version_preference }; #{} -> OldVI#{chosen_version => NewVersion} diff --git a/src/nquic_protocol_send.erl b/src/nquic_protocol_send.erl index 6ed3820..4e8ba74 100644 --- a/src/nquic_protocol_send.erl +++ b/src/nquic_protocol_send.erl @@ -22,6 +22,7 @@ builders module-qualified; the dependency is one-way. -export([ build_app_packet/2, build_handshake_packet/2, + build_handshake_packets/2, build_initial_packet/2, build_initial_packet/3 ]). @@ -214,6 +215,103 @@ build_handshake_packet(Frames, State) -> end end. +-doc """ +Build one or more Handshake-space packets from `Frames`, fragmenting the +CRYPTO flight so that no datagram exceeds the path/peer UDP payload limit +(`min(max_payload_size, peer max_udp_payload_size)`, RFC 9000 §14.1 and +§18.2). Each batch becomes its own Handshake packet/datagram; oversized +CRYPTO frames are split at their offset boundary (RFC 9000 §19.6) and +reassemble on the peer. Without this split a large server flight (cert +chain) is sent as one >1200-byte datagram, which Chromium/quiche drops on +receipt (`ERR_MSG_TOO_BIG`) since it exceeds the peer's advertised +`max_udp_payload_size`, stalling the handshake. + +Stops early and re-queues the unsent remainder if anti-amplification +blocks a packet, so the rest is resent on the next flush / PTO. +""". +-spec build_handshake_packets([nquic_frame:t()], nquic_protocol:state()) -> + {ok, [iodata()], nquic_protocol:state()}. +build_handshake_packets(Frames, State) -> + Budget = handshake_payload_budget(State), + Batches = batch_frames_by_budget(presplit_crypto_frames(Frames, Budget), Budget), + build_handshake_batches(Batches, State, []). + +-spec build_handshake_batches([[nquic_frame:t()]], nquic_protocol:state(), [iodata()]) -> + {ok, [iodata()], nquic_protocol:state()}. +build_handshake_batches([], State, Acc) -> + {ok, lists:reverse(Acc), State}; +build_handshake_batches([Batch | Rest], State, Acc) -> + case build_handshake_packet(Batch, State) of + {ok, <<>>, State1} -> + {ok, lists:reverse(Acc), requeue_handshake_frames([Batch | Rest], State1)}; + {ok, Packet, State1} -> + build_handshake_batches(Rest, State1, [Packet | Acc]); + {error, _, State1} -> + {ok, lists:reverse(Acc), State1} + end. + +-spec batch_frames_by_budget([nquic_frame:t()], pos_integer()) -> [[nquic_frame:t()]]. +batch_frames_by_budget(Frames, Budget) -> + batch_frames(Frames, Budget, 0, [], []). + +-spec batch_frames( + [nquic_frame:t()], pos_integer(), non_neg_integer(), [nquic_frame:t()], [[nquic_frame:t()]] +) -> [[nquic_frame:t()]]. +batch_frames([], _Budget, _Used, [], Batches) -> + lists:reverse(Batches); +batch_frames([], _Budget, _Used, Cur, Batches) -> + lists:reverse([lists:reverse(Cur) | Batches]); +batch_frames([F | Rest], Budget, Used, Cur, Batches) -> + Size = iolist_size(nquic_frame:encode(F)), + case Cur of + [] -> + batch_frames(Rest, Budget, Size, [F], Batches); + _ when Used + Size =< Budget -> + batch_frames(Rest, Budget, Used + Size, [F | Cur], Batches); + _ -> + batch_frames(Rest, Budget, Size, [F], [lists:reverse(Cur) | Batches]) + end. + +-spec handshake_payload_budget(nquic_protocol:state()) -> pos_integer(). +handshake_payload_budget(#conn_state{ + dcid = DCID, scid = SCID, max_payload_size = MaxPayload, remote_params = RP +}) -> + PeerMax = + case RP of + #transport_params{max_udp_payload_size = M} when is_integer(M), M >= 1200 -> M; + _ -> 1200 + end, + Limit = min(MaxPayload, PeerMax), + Overhead = 1 + 4 + 1 + byte_size(DCID) + 1 + byte_size(SCID) + 2 + 4 + ?AEAD_TAG_SIZE, + max(1, Limit - Overhead). + +-spec presplit_crypto_frames([nquic_frame:t()], pos_integer()) -> [nquic_frame:t()]. +presplit_crypto_frames(Frames, Budget) -> + MaxData = max(1, Budget - 8), + lists:flatmap(fun(F) -> split_crypto_frame(F, MaxData) end, Frames). + +-spec requeue_handshake_frames([[nquic_frame:t()]], nquic_protocol:state()) -> + nquic_protocol:state(). +requeue_handshake_frames(Batches, #conn_state{flow = Flow} = State) -> + Remaining = lists:append(Batches), + Pending = Flow#conn_flow.pending_handshake_frames, + State#conn_state{ + flow = Flow#conn_flow{pending_handshake_frames = Pending ++ lists:reverse(Remaining)} + }. + +-spec split_crypto_frame(nquic_frame:t(), pos_integer()) -> [nquic_frame:t()]. +split_crypto_frame(#crypto{offset = Off, data = Data}, MaxData) when byte_size(Data) > MaxData -> + split_crypto_data(Off, Data, MaxData, []); +split_crypto_frame(Frame, _MaxData) -> + [Frame]. + +-spec split_crypto_data(non_neg_integer(), binary(), pos_integer(), [#crypto{}]) -> [#crypto{}]. +split_crypto_data(Off, Data, MaxData, Acc) when byte_size(Data) > MaxData -> + <> = Data, + split_crypto_data(Off + MaxData, Rest, MaxData, [#crypto{offset = Off, data = Chunk} | Acc]); +split_crypto_data(Off, Data, _MaxData, Acc) -> + lists:reverse([#crypto{offset = Off, data = Data} | Acc]). + -doc """ Build an encrypted Initial-space packet from a list of frames. Returns `{ok, iodata(), nquic_protocol:state()}` with the masked packet and the state diff --git a/src/nquic_protocol_send_queues.erl b/src/nquic_protocol_send_queues.erl index b13d488..f3d4ddf 100644 --- a/src/nquic_protocol_send_queues.erl +++ b/src/nquic_protocol_send_queues.erl @@ -152,14 +152,8 @@ flush_handshake(State) -> Frames = lists:reverse(Flow0#conn_flow.pending_handshake_frames), FlowEmpty = Flow0#conn_flow{pending_handshake_frames = []}, State1 = State#conn_state{flow = FlowEmpty}, - case nquic_protocol_send:build_handshake_packet(Frames, State1) of - {ok, <<>>, State2} -> - {[], State2}; - {ok, Packet, State2} -> - {[Packet], State2}; - {error, _, State2} -> - {[], State2} - end. + {ok, Packets, State2} = nquic_protocol_send:build_handshake_packets(Frames, State1), + {Packets, State2}. -spec flush_initial(nquic_protocol:state()) -> {[iodata()], nquic_protocol:state()}. flush_initial(#conn_state{flow = #conn_flow{pending_initial_frames = []}} = State) -> diff --git a/test/nquic_integration_SUITE.erl b/test/nquic_integration_SUITE.erl index bd6015b..d565167 100644 --- a/test/nquic_integration_SUITE.erl +++ b/test/nquic_integration_SUITE.erl @@ -965,9 +965,12 @@ compat_version_negotiation_default_v1_test(_Config) -> {ClientDrv, ServerDrv, Helper} = establish(Port, Listener), {ok, #conn_state{version = ServerVersion}} = nquic_ctx_driver:conn_state(ServerDrv), - {ok, #conn_state{version = ClientVersion}} = nquic_ctx_driver:conn_state(ClientDrv), + {ok, #conn_state{version = ClientVersion, remote_params = ClientRemote}} = + nquic_ctx_driver:conn_state(ClientDrv), ?assertEqual(1, ServerVersion), ?assertEqual(1, ClientVersion), + #transport_params{version_information = #{other_versions := Advertised}} = ClientRemote, + ?assertEqual([1], Advertised), teardown(ClientDrv, ServerDrv, Helper), catch gen_server:stop(Listener), diff --git a/test/nquic_protocol_tests.erl b/test/nquic_protocol_tests.erl index 16dc635..fb5cc26 100644 --- a/test/nquic_protocol_tests.erl +++ b/test/nquic_protocol_tests.erl @@ -1638,6 +1638,68 @@ build_handshake_packet_no_keys_returns_error_test() -> nquic_protocol_send:build_handshake_packet([#ping{}], State) ). +make_handshake_send_state(PeerMaxUdp) -> + Cipher = aes_128_gcm, + Secret = crypto:strong_rand_bytes(32), + {K, IV, HP} = nquic_keys:derive_packet_protection(Secret, Cipher, 1), + RoleKeys = nquic_keys:make_role_keys(Cipher, K, IV, HP), + RemoteParams = + case PeerMaxUdp of + undefined -> undefined; + _ -> #transport_params{max_udp_payload_size = PeerMaxUdp} + end, + #conn_state{ + role = server, + version = 1, + dcid = <<1, 2, 3, 4, 5, 6, 7, 8>>, + scid = <<9, 10, 11, 12, 13, 14, 15, 16>>, + max_payload_size = 1200, + remote_params = RemoteParams, + crypto = #conn_crypto{cipher = Cipher, keys = #{handshake => #{server => RoleKeys}}}, + pn_spaces = #{handshake => #{next_pn => 0}}, + loss_state = nquic_loss:init(), + path = #conn_path_mgmt{address_validated = true}, + flow = #conn_flow{}, + gso_size = undefined + }. + +queue_handshake_flight(State0, Flight) -> + Flow = State0#conn_state.flow, + State0#conn_state{ + flow = Flow#conn_flow{pending_handshake_frames = [#crypto{offset = 0, data = Flight}]} + }. + +build_handshake_packets_small_flight_single_packet_test() -> + State = queue_handshake_flight(make_handshake_send_state(1472), crypto:strong_rand_bytes(100)), + {Packets, _State1} = nquic_protocol_send_queues:flush_handshake(State), + ?assertEqual(1, length(Packets)). + +build_handshake_packets_splits_large_flight_test() -> + State = queue_handshake_flight(make_handshake_send_state(1472), crypto:strong_rand_bytes(4000)), + {Packets, _State1} = nquic_protocol_send_queues:flush_handshake(State), + ?assert(length(Packets) >= 4), + lists:foreach( + fun(P) -> ?assert(byte_size(iolist_to_binary(P)) =< 1200) end, + Packets + ). + +build_handshake_packets_respects_peer_max_udp_payload_test() -> + PeerMax = 1300, + State0 = (make_handshake_send_state(PeerMax))#conn_state{max_payload_size = 9000}, + State = queue_handshake_flight(State0, crypto:strong_rand_bytes(6000)), + {Packets, _State1} = nquic_protocol_send_queues:flush_handshake(State), + ?assert(length(Packets) >= 2), + lists:foreach( + fun(P) -> ?assert(byte_size(iolist_to_binary(P)) =< PeerMax) end, + Packets + ). + +build_handshake_packets_distinct_packet_numbers_test() -> + State = queue_handshake_flight(make_handshake_send_state(1472), crypto:strong_rand_bytes(4000)), + {Packets, State1} = nquic_protocol_send_queues:flush_handshake(State), + HsSpace = maps:get(handshake, State1#conn_state.pn_spaces), + ?assertEqual(length(Packets), maps:get(next_pn, HsSpace)). + build_initial_packet_no_keys_returns_error_test() -> State = #conn_state{ role = client,