From d7f33e2054ee875c71ce018e6067c32b431af7a0 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 13 May 2024 14:22:22 +0300 Subject: [PATCH] chore: Use `MIN_INITIAL_PACKET_SIZE` instead of literal `1200` (#1895) --- fuzz/fuzz_targets/client_initial.rs | 6 +++--- fuzz/fuzz_targets/server_initial.rs | 6 +++--- neqo-http3/src/connection_client.rs | 7 ++++--- .../extended_connect/tests/webtransport/mod.rs | 4 ++-- .../src/connection/tests/datagram.rs | 18 +++++++++++------- neqo-transport/src/connection/tests/vn.rs | 6 +++--- neqo-transport/src/connection/tests/zerortt.rs | 9 ++++++--- neqo-transport/src/lib.rs | 1 + neqo-transport/src/packet/mod.rs | 4 ++++ neqo-transport/src/server.rs | 5 +---- neqo-transport/src/tparams.rs | 3 ++- neqo-transport/tests/connection.rs | 10 ++++++---- neqo-transport/tests/retry.rs | 14 ++++++++------ neqo-transport/tests/server.rs | 15 ++++++++------- test-fixture/src/assertions.rs | 4 ++-- 15 files changed, 64 insertions(+), 48 deletions(-) diff --git a/fuzz/fuzz_targets/client_initial.rs b/fuzz/fuzz_targets/client_initial.rs index f018298961..8e982b37ce 100644 --- a/fuzz/fuzz_targets/client_initial.rs +++ b/fuzz/fuzz_targets/client_initial.rs @@ -24,7 +24,7 @@ fuzz_target!(|data: &[u8]| { let (aead, hp) = initial_aead_and_hp(d_cid, Role::Client); let (_, pn) = remove_header_protection(&hp, header, payload); - let mut payload_enc = Encoder::with_capacity(1200); + let mut payload_enc = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE); payload_enc.encode(data); // Add fuzzed data. // Make a new header with a 1 byte packet number length. @@ -49,8 +49,8 @@ fuzz_target!(|data: &[u8]| { ) .unwrap(); assert_eq!(header_enc.len() + v.len(), ciphertext.len()); - // Pad with zero to get up to 1200. - ciphertext.resize(1200, 0); + // Pad with zero to get up to MIN_INITIAL_PACKET_SIZE. + ciphertext.resize(MIN_INITIAL_PACKET_SIZE, 0); apply_header_protection( &hp, diff --git a/fuzz/fuzz_targets/server_initial.rs b/fuzz/fuzz_targets/server_initial.rs index f4bd5ec0b9..f264619fe7 100644 --- a/fuzz/fuzz_targets/server_initial.rs +++ b/fuzz/fuzz_targets/server_initial.rs @@ -30,7 +30,7 @@ fuzz_target!(|data: &[u8]| { let (aead, hp) = initial_aead_and_hp(d_cid, Role::Server); let (_, pn) = remove_header_protection(&hp, header, payload); - let mut payload_enc = Encoder::with_capacity(1200); + let mut payload_enc = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE); payload_enc.encode(data); // Add fuzzed data. // Make a new header with a 1 byte packet number length. @@ -55,8 +55,8 @@ fuzz_target!(|data: &[u8]| { ) .unwrap(); assert_eq!(header_enc.len() + v.len(), ciphertext.len()); - // Pad with zero to get up to 1200. - ciphertext.resize(1200, 0); + // Pad with zero to get up to MIN_INITIAL_PACKET_SIZE. + ciphertext.resize(MIN_INITIAL_PACKET_SIZE, 0); apply_header_protection( &hp, diff --git a/neqo-http3/src/connection_client.rs b/neqo-http3/src/connection_client.rs index 18e513e743..7ca5bdba05 100644 --- a/neqo-http3/src/connection_client.rs +++ b/neqo-http3/src/connection_client.rs @@ -1292,7 +1292,7 @@ mod tests { use neqo_qpack::{encoder::QPackEncoder, QpackSettings}; use neqo_transport::{ CloseReason, ConnectionEvent, ConnectionParameters, Output, State, StreamId, StreamType, - Version, RECV_BUFFER_SIZE, SEND_BUFFER_SIZE, + Version, MIN_INITIAL_PACKET_SIZE, RECV_BUFFER_SIZE, SEND_BUFFER_SIZE, }; use test_fixture::{ anti_replay, default_server_h3, fixture_init, new_server, now, @@ -7157,8 +7157,9 @@ mod tests { #[test] fn priority_update_during_full_buffer() { // set a lower MAX_DATA on the server side to restrict the data the client can send - let (mut client, mut server) = - connect_with_connection_parameters(ConnectionParameters::default().max_data(1200)); + let (mut client, mut server) = connect_with_connection_parameters( + ConnectionParameters::default().max_data(MIN_INITIAL_PACKET_SIZE.try_into().unwrap()), + ); let request_stream_id = make_request_and_exchange_pkts(&mut client, &mut server, false); let data_writable = |e| matches!(e, Http3ClientEvent::DataWritable { .. }); diff --git a/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs b/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs index 3753c3122d..42b14fca11 100644 --- a/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs +++ b/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs @@ -12,7 +12,7 @@ use std::{cell::RefCell, rc::Rc, time::Duration}; use neqo_common::event::Provider; use neqo_crypto::AuthenticationStatus; -use neqo_transport::{ConnectionParameters, StreamId, StreamType}; +use neqo_transport::{ConnectionParameters, StreamId, StreamType, MIN_INITIAL_PACKET_SIZE}; use test_fixture::{ anti_replay, fixture_init, now, CountingConnectionIdGenerator, DEFAULT_ADDR, DEFAULT_ALPN_H3, DEFAULT_KEYS, DEFAULT_SERVER_NAME, @@ -25,7 +25,7 @@ use crate::{ WebTransportServerEvent, WebTransportSessionAcceptAction, }; -const DATAGRAM_SIZE: u64 = 1200; +const DATAGRAM_SIZE: u64 = MIN_INITIAL_PACKET_SIZE as u64; pub fn wt_default_parameters() -> Http3Parameters { Http3Parameters::default() diff --git a/neqo-transport/src/connection/tests/datagram.rs b/neqo-transport/src/connection/tests/datagram.rs index f1b64b3c8f..864f6e05b6 100644 --- a/neqo-transport/src/connection/tests/datagram.rs +++ b/neqo-transport/src/connection/tests/datagram.rs @@ -19,14 +19,14 @@ use crate::{ packet::PacketBuilder, quic_datagrams::MAX_QUIC_DATAGRAM, send_stream::{RetransmissionPriority, TransmissionPriority}, - CloseReason, Connection, ConnectionParameters, Error, StreamType, + CloseReason, Connection, ConnectionParameters, Error, StreamType, MIN_INITIAL_PACKET_SIZE, }; const DATAGRAM_LEN_MTU: u64 = 1310; const DATA_MTU: &[u8] = &[1; 1310]; const DATA_BIGGER_THAN_MTU: &[u8] = &[0; 2620]; -const DATAGRAM_LEN_SMALLER_THAN_MTU: u64 = 1200; -const DATA_SMALLER_THAN_MTU: &[u8] = &[0; 1200]; +const DATAGRAM_LEN_SMALLER_THAN_MTU: u64 = MIN_INITIAL_PACKET_SIZE as u64; +const DATA_SMALLER_THAN_MTU: &[u8] = &[0; MIN_INITIAL_PACKET_SIZE]; const DATA_SMALLER_THAN_MTU_2: &[u8] = &[0; 600]; const OUTGOING_QUEUE: usize = 2; @@ -259,7 +259,9 @@ fn datagram_after_stream_data() { // Create a stream with normal priority and send some data. let stream_id = client.stream_create(StreamType::BiDi).unwrap(); - client.stream_send(stream_id, &[6; 1200]).unwrap(); + client + .stream_send(stream_id, &[6; MIN_INITIAL_PACKET_SIZE]) + .unwrap(); assert!( matches!(send_packet_and_get_server_event(&mut client, &mut server), ConnectionEvent::RecvStreamReadable { stream_id: s } if s == stream_id) @@ -289,7 +291,9 @@ fn datagram_before_stream_data() { RetransmissionPriority::default(), ) .unwrap(); - client.stream_send(stream_id, &[6; 1200]).unwrap(); + client + .stream_send(stream_id, &[6; MIN_INITIAL_PACKET_SIZE]) + .unwrap(); // Write a datagram. let dgram_sent = client.stats().frame_tx.datagram; @@ -440,7 +444,7 @@ fn send_datagram(sender: &mut Connection, receiver: &mut Connection, data: &[u8] #[test] fn multiple_datagram_events() { - const DATA_SIZE: usize = 1200; + const DATA_SIZE: usize = MIN_INITIAL_PACKET_SIZE; const MAX_QUEUE: usize = 3; const FIRST_DATAGRAM: &[u8] = &[0; DATA_SIZE]; const SECOND_DATAGRAM: &[u8] = &[1; DATA_SIZE]; @@ -486,7 +490,7 @@ fn multiple_datagram_events() { #[test] fn too_many_datagram_events() { - const DATA_SIZE: usize = 1200; + const DATA_SIZE: usize = MIN_INITIAL_PACKET_SIZE; const MAX_QUEUE: usize = 2; const FIRST_DATAGRAM: &[u8] = &[0; DATA_SIZE]; const SECOND_DATAGRAM: &[u8] = &[1; DATA_SIZE]; diff --git a/neqo-transport/src/connection/tests/vn.rs b/neqo-transport/src/connection/tests/vn.rs index 815868d78d..28f830cb3e 100644 --- a/neqo-transport/src/connection/tests/vn.rs +++ b/neqo-transport/src/connection/tests/vn.rs @@ -17,7 +17,7 @@ use super::{ use crate::{ packet::PACKET_BIT_LONG, tparams::{self, TransportParameter}, - ConnectionParameters, Error, Version, + ConnectionParameters, Error, Version, MIN_INITIAL_PACKET_SIZE, }; // The expected PTO duration after the first Initial is sent. @@ -30,7 +30,7 @@ fn unknown_version() { mem::drop(client.process(None, now()).dgram()); let mut unknown_version_packet = vec![0x80, 0x1a, 0x1a, 0x1a, 0x1a]; - unknown_version_packet.resize(1200, 0x0); + unknown_version_packet.resize(MIN_INITIAL_PACKET_SIZE, 0x0); mem::drop(client.process(Some(&datagram(unknown_version_packet)), now())); assert_eq!(1, client.stats().dropped_rx); } @@ -40,7 +40,7 @@ fn server_receive_unknown_first_packet() { let mut server = default_server(); let mut unknown_version_packet = vec![0x80, 0x1a, 0x1a, 0x1a, 0x1a]; - unknown_version_packet.resize(1200, 0x0); + unknown_version_packet.resize(MIN_INITIAL_PACKET_SIZE, 0x0); assert_eq!( server.process(Some(&datagram(unknown_version_packet,)), now(),), diff --git a/neqo-transport/src/connection/tests/zerortt.rs b/neqo-transport/src/connection/tests/zerortt.rs index b5e5f0d758..a34e5acdb2 100644 --- a/neqo-transport/src/connection/tests/zerortt.rs +++ b/neqo-transport/src/connection/tests/zerortt.rs @@ -14,7 +14,10 @@ use super::{ super::Connection, connect, default_client, default_server, exchange_ticket, new_server, resumed_server, CountingConnectionIdGenerator, }; -use crate::{events::ConnectionEvent, ConnectionParameters, Error, StreamType, Version}; +use crate::{ + events::ConnectionEvent, ConnectionParameters, Error, StreamType, Version, + MIN_INITIAL_PACKET_SIZE, +}; #[test] fn zero_rtt_negotiate() { @@ -57,8 +60,8 @@ fn zero_rtt_send_recv() { client.stream_send(client_stream_id, &[1, 2, 3]).unwrap(); let client_0rtt = client.process(None, now()); assert!(client_0rtt.as_dgram_ref().is_some()); - // 0-RTT packets on their own shouldn't be padded to 1200. - assert!(client_0rtt.as_dgram_ref().unwrap().len() < 1200); + // 0-RTT packets on their own shouldn't be padded to MIN_INITIAL_PACKET_SIZE. + assert!(client_0rtt.as_dgram_ref().unwrap().len() < MIN_INITIAL_PACKET_SIZE); let server_hs = server.process(client_hs.as_dgram_ref(), now()); assert!(server_hs.as_dgram_ref().is_some()); // ServerHello, etc... diff --git a/neqo-transport/src/lib.rs b/neqo-transport/src/lib.rs index 723a86980e..54b08a9339 100644 --- a/neqo-transport/src/lib.rs +++ b/neqo-transport/src/lib.rs @@ -61,6 +61,7 @@ pub use self::{ }, events::{ConnectionEvent, ConnectionEvents}, frame::CloseError, + packet::MIN_INITIAL_PACKET_SIZE, quic_datagrams::DatagramTracking, recv_stream::{RecvStreamStats, RECV_BUFFER_SIZE}, send_stream::{SendStreamStats, SEND_BUFFER_SIZE}, diff --git a/neqo-transport/src/packet/mod.rs b/neqo-transport/src/packet/mod.rs index 10d9b13208..1c38e8a94b 100644 --- a/neqo-transport/src/packet/mod.rs +++ b/neqo-transport/src/packet/mod.rs @@ -23,6 +23,10 @@ use crate::{ Error, Res, }; +/// `MIN_INITIAL_PACKET_SIZE` is the smallest packet that can be used to establish +/// a new connection across all QUIC versions this server supports. +pub const MIN_INITIAL_PACKET_SIZE: usize = 1200; + pub const PACKET_BIT_LONG: u8 = 0x80; const PACKET_BIT_SHORT: u8 = 0x00; const PACKET_BIT_FIXED_QUIC: u8 = 0x40; diff --git a/neqo-transport/src/server.rs b/neqo-transport/src/server.rs index 60909d71e1..bce87beb4d 100644 --- a/neqo-transport/src/server.rs +++ b/neqo-transport/src/server.rs @@ -33,7 +33,7 @@ use crate::{ addr_valid::{AddressValidation, AddressValidationResult}, cid::{ConnectionId, ConnectionIdDecoder, ConnectionIdGenerator, ConnectionIdRef}, connection::{Connection, Output, State}, - packet::{PacketBuilder, PacketType, PublicPacket}, + packet::{PacketBuilder, PacketType, PublicPacket, MIN_INITIAL_PACKET_SIZE}, ConnectionParameters, Res, Version, }; @@ -43,9 +43,6 @@ pub enum InitialResult { Retry(Vec), } -/// `MIN_INITIAL_PACKET_SIZE` is the smallest packet that can be used to establish -/// a new connection across all QUIC versions this server supports. -const MIN_INITIAL_PACKET_SIZE: usize = 1200; /// The size of timer buckets. This is higher than the actual timer granularity /// as this depends on there being some distribution of events. const TIMER_GRANULARITY: Duration = Duration::from_millis(4); diff --git a/neqo-transport/src/tparams.rs b/neqo-transport/src/tparams.rs index eada56cc4c..5161b69d6b 100644 --- a/neqo-transport/src/tparams.rs +++ b/neqo-transport/src/tparams.rs @@ -22,6 +22,7 @@ use neqo_crypto::{ use crate::{ cid::{ConnectionId, ConnectionIdEntry, CONNECTION_ID_SEQNO_PREFERRED, MAX_CONNECTION_ID_LEN}, + packet::MIN_INITIAL_PACKET_SIZE, version::{Version, VersionConfig, WireVersion}, Error, Res, }; @@ -278,7 +279,7 @@ impl TransportParameter { }, MAX_UDP_PAYLOAD_SIZE => match d.decode_varint() { - Some(v) if v >= 1200 => Self::Integer(v), + Some(v) if v >= MIN_INITIAL_PACKET_SIZE.try_into()? => Self::Integer(v), _ => return Err(Error::TransportParameterError), }, diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index 3cc711f80b..f57684bec6 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -7,7 +7,9 @@ mod common; use neqo_common::{Datagram, Decoder, Encoder, Role}; -use neqo_transport::{CloseReason, ConnectionParameters, Error, State, Version}; +use neqo_transport::{ + CloseReason, ConnectionParameters, Error, State, Version, MIN_INITIAL_PACKET_SIZE, +}; use test_fixture::{ default_client, default_server, header_protection::{ @@ -104,7 +106,7 @@ fn reorder_server_initial() { // And rebuild a packet. let mut packet = header.clone(); - packet.resize(1200, 0); + packet.resize(MIN_INITIAL_PACKET_SIZE, 0); aead.encrypt(pn, &header, &plaintext, &mut packet[header.len()..]) .unwrap(); apply_header_protection(&hp, &mut packet, protected_header.len()..header.len()); @@ -237,7 +239,7 @@ fn overflow_crypto() { let plen = payload.len(); payload.pad_to(plen + 1000, 44); - let mut packet = Encoder::with_capacity(1200); + let mut packet = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE); packet .encode_byte(0xc1) // Initial with packet number length of 2. .encode_uint(4, Version::Version1.wire_version()) @@ -254,7 +256,7 @@ fn overflow_crypto() { aead.encrypt(pn, &header, payload.as_ref(), &mut packet[header.len()..]) .unwrap(); apply_header_protection(&hp, &mut packet, pn_offset..(pn_offset + 2)); - packet.resize(1200, 0); // Initial has to be 1200 bytes! + packet.resize(MIN_INITIAL_PACKET_SIZE, 0); // Initial has to be MIN_INITIAL_PACKET_SIZE bytes! let dgram = Datagram::new( server_initial.source(), diff --git a/neqo-transport/tests/retry.rs b/neqo-transport/tests/retry.rs index 3f95511c3e..07eee6635d 100644 --- a/neqo-transport/tests/retry.rs +++ b/neqo-transport/tests/retry.rs @@ -17,7 +17,9 @@ use std::{ use common::{connected_server, default_server, generate_ticket}; use neqo_common::{hex_with_len, qdebug, qtrace, Datagram, Encoder, Role}; use neqo_crypto::AuthenticationStatus; -use neqo_transport::{server::ValidateAddress, CloseReason, Error, State, StreamType}; +use neqo_transport::{ + server::ValidateAddress, CloseReason, Error, State, StreamType, MIN_INITIAL_PACKET_SIZE, +}; use test_fixture::{ assertions, datagram, default_client, header_protection::{ @@ -331,7 +333,7 @@ fn retry_after_pto() { // Let PTO fire on the client and then let it exhaust its PTO packets. now += Duration::from_secs(1); let pto = client.process(None, now).dgram(); - assert!(pto.unwrap().len() >= 1200); + assert!(pto.unwrap().len() >= MIN_INITIAL_PACKET_SIZE); let cb = client.process(None, now).callback(); assert_ne!(cb, Duration::new(0, 0)); @@ -339,7 +341,7 @@ fn retry_after_pto() { assertions::assert_retry(retry.as_ref().unwrap()); let ci2 = client.process(retry.as_ref(), now).dgram(); - assert!(ci2.unwrap().len() >= 1200); + assert!(ci2.unwrap().len() >= MIN_INITIAL_PACKET_SIZE); } #[test] @@ -430,11 +432,11 @@ fn mitm_retry() { qtrace!("notoken_header={}", hex_with_len(¬oken_header)); // Encrypt. - let mut notoken_packet = Encoder::with_capacity(1200) + let mut notoken_packet = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE) .encode(¬oken_header) .as_ref() .to_vec(); - notoken_packet.resize_with(1200, u8::default); + notoken_packet.resize_with(MIN_INITIAL_PACKET_SIZE, u8::default); aead.encrypt( pn, ¬oken_header, @@ -443,7 +445,7 @@ fn mitm_retry() { ) .unwrap(); // Unlike with decryption, don't truncate. - // All 1200 bytes are needed to reach the minimum datagram size. + // All MIN_INITIAL_PACKET_SIZE bytes are needed to reach the minimum datagram size. apply_header_protection(&hp, &mut notoken_packet, pn_offset..(pn_offset + pn_len)); qtrace!("packet={}", hex_with_len(¬oken_packet)); diff --git a/neqo-transport/tests/server.rs b/neqo-transport/tests/server.rs index 4740d26ded..36a49a48cf 100644 --- a/neqo-transport/tests/server.rs +++ b/neqo-transport/tests/server.rs @@ -16,6 +16,7 @@ use neqo_crypto::{ use neqo_transport::{ server::{ActiveConnectionRef, Server, ValidateAddress}, CloseReason, Connection, ConnectionParameters, Error, Output, State, StreamType, Version, + MIN_INITIAL_PACKET_SIZE, }; use test_fixture::{ assertions, datagram, default_client, @@ -227,14 +228,14 @@ fn drop_non_initial() { let mut server = default_server(); // This is big enough to look like an Initial, but it uses the Retry type. - let mut header = neqo_common::Encoder::with_capacity(1200); + let mut header = neqo_common::Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE); header .encode_byte(0xfa) .encode_uint(4, Version::default().wire_version()) .encode_vec(1, CID) .encode_vec(1, CID); let mut bogus_data: Vec = header.into(); - bogus_data.resize(1200, 66); + bogus_data.resize(MIN_INITIAL_PACKET_SIZE, 66); let bogus = datagram(bogus_data); assert!(server.process(Some(&bogus), now()).dgram().is_none()); @@ -425,8 +426,8 @@ fn bad_client_initial() { ) .unwrap(); assert_eq!(header_enc.len() + v.len(), ciphertext.len()); - // Pad with zero to get up to 1200. - ciphertext.resize(1200, 0); + // Pad with zero to get up to MIN_INITIAL_PACKET_SIZE. + ciphertext.resize(MIN_INITIAL_PACKET_SIZE, 0); apply_header_protection( &hp, @@ -488,7 +489,7 @@ fn bad_client_initial_connection_close() { let (aead, hp) = initial_aead_and_hp(d_cid, Role::Client); let (_, pn) = remove_header_protection(&hp, header, payload); - let mut payload_enc = Encoder::with_capacity(1200); + let mut payload_enc = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE); payload_enc.encode(&[0x1c, 0x01, 0x00, 0x00]); // Add a CONNECTION_CLOSE frame. // Make a new header with a 1 byte packet number length. @@ -513,8 +514,8 @@ fn bad_client_initial_connection_close() { ) .unwrap(); assert_eq!(header_enc.len() + v.len(), ciphertext.len()); - // Pad with zero to get up to 1200. - ciphertext.resize(1200, 0); + // Pad with zero to get up to MIN_INITIAL_PACKET_SIZE. + ciphertext.resize(MIN_INITIAL_PACKET_SIZE, 0); apply_header_protection( &hp, diff --git a/test-fixture/src/assertions.rs b/test-fixture/src/assertions.rs index 191c81f7ab..9e62e7167e 100644 --- a/test-fixture/src/assertions.rs +++ b/test-fixture/src/assertions.rs @@ -7,7 +7,7 @@ use std::net::SocketAddr; use neqo_common::{Datagram, Decoder}; -use neqo_transport::{version::WireVersion, Version}; +use neqo_transport::{version::WireVersion, Version, MIN_INITIAL_PACKET_SIZE}; use crate::{DEFAULT_ADDR, DEFAULT_ADDR_V4}; @@ -62,7 +62,7 @@ pub fn assert_vn(payload: &[u8]) { /// /// If the tests fail. pub fn assert_coalesced_0rtt(payload: &[u8]) { - assert!(payload.len() >= 1200); + assert!(payload.len() >= MIN_INITIAL_PACKET_SIZE); let mut dec = Decoder::from(payload); let initial_type = dec.decode_byte().unwrap(); // Initial let version = assert_default_version(&mut dec);