From 2e918f27dd7cba61e9a45e671fae9a471851ac73 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 24 Jan 2025 14:51:34 +0200 Subject: [PATCH 01/17] fix: Enable MLKEM768X25519 by default WIP --- neqo-transport/src/crypto.rs | 43 +++++++++++++----------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index ee0547a549..cc351d8a25 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -79,34 +79,21 @@ impl Crypto { TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, ])?; - match &mut agent { - Agent::Server(c) => { - // Clients do not send mlkem768x25519 shares by default, but servers should accept - // them. - c.set_groups(&[ - TLS_GRP_KEM_MLKEM768X25519, - TLS_GRP_EC_X25519, - TLS_GRP_EC_SECP256R1, - TLS_GRP_EC_SECP384R1, - TLS_GRP_EC_SECP521R1, - ])?; - } - Agent::Client(c) => { - c.set_groups(&[ - TLS_GRP_EC_X25519, - TLS_GRP_EC_SECP256R1, - TLS_GRP_EC_SECP384R1, - TLS_GRP_EC_SECP521R1, - ])?; - - // Configure clients to send both X25519 and P256 to reduce - // the rate of HRRs. - c.send_additional_key_shares(1)?; - - // Always enable 0-RTT on the client, but the server needs - // more configuration passed to server_enable_0rtt. - c.enable_0rtt()?; - } + agent.set_groups(&[ + TLS_GRP_KEM_MLKEM768X25519, + TLS_GRP_EC_X25519, + TLS_GRP_EC_SECP256R1, + TLS_GRP_EC_SECP384R1, + TLS_GRP_EC_SECP521R1, + ])?; + if let Agent::Client(c) = &mut agent { + // Configure clients to send MLKEM768X25519, X25519 and P256 to + // reduce the rate of HRRs. + c.send_additional_key_shares(2)?; + + // Always enable 0-RTT on the client, but the server needs + // more configuration passed to server_enable_0rtt. + c.enable_0rtt()?; } agent.set_alpn(&protocols)?; agent.disable_end_of_early_data()?; From 85abfebf2a4d994b7e933b7c8a9ca80e62452d9c Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 24 Jan 2025 16:24:23 +0200 Subject: [PATCH 02/17] Improve SNI slicing for large CIs --- neqo-transport/src/crypto.rs | 51 +++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index cc351d8a25..120c270d10 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -1494,13 +1494,43 @@ impl CryptoStreams { Some((offset, length)) } + fn mark_as_sent( + cs: &mut CryptoStream, + space: PacketNumberSpace, + tokens: &mut Vec, + chunk: (u64, usize), + stats: &mut FrameStats, + ) { + let (offset, len) = chunk; + cs.tx.mark_as_sent(offset, len); + qdebug!("CRYPTO for {space} offset={offset}, len={len}"); + tokens.push(RecoveryToken::Crypto(CryptoRecoveryToken { + space, + offset, + length: len, + })); + stats.crypto += 1; + } + let cs = self.get_mut(space).unwrap(); - if let Some((offset, data)) = cs.tx.next_bytes() { + // Because the TLS extensions in the CRYPTO data are randomly ordered, if the CH is + // larger than fits into a single packet, depending on where the SNI is located, slicing and + // reordering it can cause the generation of discontiguous chunks. We need to loop here to + // make sure we fill the packets, and not send superfluous packets that are mostly padding. + while let Some((offset, data)) = cs.tx.next_bytes() { let written = if sni_slicing && offset == 0 { if let Some(sni) = find_sni(data) { // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. let mid = sni.start + (sni.end - sni.start) / 2; let (left, right) = data.split_at(mid); + // Figure out how many `limit`-sized packets the entire data + // would need. Then, set the per-packet limit so the data is + // spread over that number of packets and all of them end up + // zero-padded, which apparently helps with traversal of + // some middleboxes. This is ignores CRYPTO frame headers so + // undercounts a little, but that's fine. + let packets_needed = data.len() / builder.limit() + 1; + builder.set_limit(data.len() / packets_needed); [ write_chunk(offset + mid as u64, right, builder), write_chunk(offset, left, builder), @@ -1513,15 +1543,16 @@ impl CryptoStreams { // Not at the start of the crypto stream, write the entire data. [write_chunk(offset, data, builder), None] }; - for (offset, length) in written.into_iter().flatten() { - cs.tx.mark_as_sent(offset, length); - qdebug!("CRYPTO for {space} offset={offset}, len={length}"); - tokens.push(RecoveryToken::Crypto(CryptoRecoveryToken { - space, - offset, - length, - })); - stats.crypto += 1; + + match written { + [None, None] => break, + [None, Some(chunk)] | [Some(chunk), None] => { + mark_as_sent(cs, space, tokens, chunk, stats); + } + [Some(chunk1), Some(chunk2)] => { + mark_as_sent(cs, space, tokens, chunk1, stats); + mark_as_sent(cs, space, tokens, chunk2, stats); + } } } } From 5370a20decc4e51a2cf8c0d03dffa5e503c6dc5c Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 27 Jan 2025 18:27:50 +0200 Subject: [PATCH 03/17] WIP --- neqo-http3/src/connection_client.rs | 54 ++- .../tests/webtransport/mod.rs | 6 +- neqo-http3/src/frames/tests/mod.rs | 6 +- neqo-http3/src/server.rs | 9 +- neqo-http3/tests/httpconn.rs | 27 +- neqo-http3/tests/priority.rs | 6 +- neqo-http3/tests/webtransport.rs | 6 +- neqo-transport/src/connection/mod.rs | 61 ++-- neqo-transport/src/connection/params.rs | 14 + neqo-transport/src/connection/tests/close.rs | 8 +- neqo-transport/src/connection/tests/ecn.rs | 4 +- .../src/connection/tests/handshake.rs | 314 ++++++++++++------ neqo-transport/src/connection/tests/idle.rs | 14 +- neqo-transport/src/connection/tests/keys.rs | 34 +- .../src/connection/tests/migration.rs | 8 +- .../src/connection/tests/priority.rs | 80 ++--- .../src/connection/tests/recovery.rs | 38 ++- .../src/connection/tests/resumption.rs | 5 +- neqo-transport/src/connection/tests/stream.rs | 29 +- neqo-transport/src/connection/tests/vn.rs | 58 ++-- .../src/connection/tests/zerortt.rs | 51 ++- neqo-transport/src/crypto.rs | 46 ++- neqo-transport/src/send_stream.rs | 5 + neqo-transport/tests/common/mod.rs | 10 +- neqo-transport/tests/connection.rs | 13 +- neqo-transport/tests/retry.rs | 83 +++-- neqo-transport/tests/server.rs | 54 ++- test-fixture/src/sim/connection.rs | 6 +- 28 files changed, 720 insertions(+), 329 deletions(-) diff --git a/neqo-http3/src/connection_client.rs b/neqo-http3/src/connection_client.rs index 5641dc9401..8056887d46 100644 --- a/neqo-http3/src/connection_client.rs +++ b/neqo-http3/src/connection_client.rs @@ -1624,15 +1624,21 @@ mod tests { fn handshake_only(client: &mut Http3Client, server: &mut TestServer) -> Output { assert_eq!(client.state(), Http3State::Initializing); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::Initializing); assert_eq!(*server.conn.state(), State::Init); - let out = server.conn.process(out.dgram(), now()); + _ = server.conn.process(out1.dgram(), now()); + let out = server.conn.process(out2.dgram(), now()); assert_eq!(*server.conn.state(), State::Handshaking); let out = client.process(out.dgram(), now()); let out = server.conn.process(out.dgram(), now()); + + let out = client.process(out.dgram(), now()); + let out = server.conn.process(out.dgram(), now()); + assert!(out.as_dgram_ref().is_none()); let authentication_needed = |e| matches!(e, Http3ClientEvent::AuthenticationNeeded); @@ -4156,12 +4162,13 @@ mod tests { #[test] fn zero_rtt_negotiated() { let (mut client, mut server) = start_with_0rtt(); - - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::ZeroRtt); assert_eq!(*server.conn.state(), State::Init); - let out = server.conn.process(out.dgram(), now()); + _ = server.conn.process(out1.dgram(), now()); + let out = server.conn.process(out2.dgram(), now()); // Check that control and qpack streams are received and a // SETTINGS frame has been received. @@ -4175,6 +4182,9 @@ mod tests { assert_eq!(*server.conn.state(), State::Handshaking); let out = client.process(out.dgram(), now()); + let out = server.conn.process(out.dgram(), now()); + let out = client.process(out.dgram(), now()); + assert_eq!(client.state(), Http3State::Connected); drop(server.conn.process(out.dgram(), now())); @@ -4192,11 +4202,13 @@ mod tests { make_request(&mut client, true, &[Header::new("myheaders", "myvalue")]); assert_eq!(request_stream_id, 0); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::ZeroRtt); assert_eq!(*server.conn.state(), State::Init); - let out = server.conn.process(out.dgram(), now()); + _ = server.conn.process(out1.dgram(), now()); + let out = server.conn.process(out2.dgram(), now()); // Check that control and qpack streams are received and a // SETTINGS frame has been received. @@ -4210,6 +4222,8 @@ mod tests { assert_eq!(*server.conn.state(), State::Handshaking); let out = client.process(out.dgram(), now()); + let out = server.conn.process(out.dgram(), now()); + let out = client.process(out.dgram(), now()); assert_eq!(client.state(), Http3State::Connected); let out = server.conn.process(out.dgram(), now()); assert!(server.conn.state().connected()); @@ -4269,28 +4283,30 @@ mod tests { let zerortt_event = |e| matches!(e, Http3ClientEvent::StateChange(Http3State::ZeroRtt)); assert!(client.events().any(zerortt_event)); - // Send ClientHello. - let client_hs = client.process_output(now()); - assert!(client_hs.as_dgram_ref().is_some()); - // Create a request let request_stream_id = make_request(&mut client, false, &[]); assert_eq!(request_stream_id, 0); + // Send ClientHello. + let client_hs = client.process_output(now()); + assert!(client_hs.as_dgram_ref().is_some()); + let client_0rtt = client.process_output(now()); assert!(client_0rtt.as_dgram_ref().is_some()); - let server_hs = server.process(client_hs.dgram(), now()); + _ = server.process(client_hs.dgram(), now()); + let server_hs = server.process(client_0rtt.dgram(), now()); assert!(server_hs.as_dgram_ref().is_some()); // Should produce ServerHello etc... - let server_ignored = server.process(client_0rtt.dgram(), now()); - assert!(server_ignored.as_dgram_ref().is_none()); + + let dgram = client.process(server_hs.dgram(), now()).dgram(); + let dgram = server.process(dgram, now()); // The server shouldn't receive that 0-RTT data. let recvd_stream_evt = |e| matches!(e, ConnectionEvent::NewStream { .. }); assert!(!server.events().any(recvd_stream_evt)); // Client should get a rejection. - let client_out = client.process(server_hs.dgram(), now()); + let client_out = client.process(dgram.dgram(), now()); assert!(client_out.as_dgram_ref().is_some()); let recvd_0rtt_reject = |e| e == Http3ClientEvent::ZeroRttRejected; assert!(client.events().any(recvd_0rtt_reject)); @@ -4328,11 +4344,13 @@ mod tests { .enable_resumption(now(), &token) .expect("Set resumption token"); assert_eq!(client.state(), Http3State::ZeroRtt); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::ZeroRtt); assert_eq!(*server.conn.state(), State::Init); - let out = server.conn.process(out.dgram(), now()); + _ = server.conn.process(out1.dgram(), now()); + let out = server.conn.process(out2.dgram(), now()); // Check that control and qpack streams and a SETTINGS frame are received. // Also qpack encoder stream will send "change capacity" instruction because it has @@ -4345,6 +4363,8 @@ mod tests { assert_eq!(*server.conn.state(), State::Handshaking); let out = client.process(out.dgram(), now()); + let out = server.conn.process(out.dgram(), now()); + let out = client.process(out.dgram(), now()); assert_eq!(client.state(), Http3State::Connected); drop(server.conn.process(out.dgram(), now())); 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 15b6c343cb..cee26e4876 100644 --- a/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs +++ b/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs @@ -81,9 +81,13 @@ fn exchange_packets(client: &mut Http3Client, server: &mut Http3Server) { // Perform only QUIC transport handshake. fn connect_with(client: &mut Http3Client, server: &mut Http3Server) { assert_eq!(client.state(), Http3State::Initializing); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::Initializing); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); + let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); diff --git a/neqo-http3/src/frames/tests/mod.rs b/neqo-http3/src/frames/tests/mod.rs index 3f93c11547..58fa022e8f 100644 --- a/neqo-http3/src/frames/tests/mod.rs +++ b/neqo-http3/src/frames/tests/mod.rs @@ -20,7 +20,11 @@ pub fn enc_dec>(d: &Encoder, st: &str, remaining: usize) -> T let mut conn_c = default_client(); let mut conn_s = default_server(); - let out = conn_c.process_output(now()); + let out1 = conn_c.process_output(now()); + let out2 = conn_c.process_output(now()); + _ = conn_s.process(out1.dgram(), now()); + let out = conn_s.process(out2.dgram(), now()); + let out = conn_c.process(out.dgram(), now()); let out = conn_s.process(out.dgram(), now()); let out = conn_c.process(out.dgram(), now()); drop(conn_s.process(out.dgram(), now())); diff --git a/neqo-http3/src/server.rs b/neqo-http3/src/server.rs index ad8025f55f..91f18134fc 100644 --- a/neqo-http3/src/server.rs +++ b/neqo-http3/src/server.rs @@ -410,8 +410,12 @@ mod tests { fn connect_transport(server: &mut Http3Server, client: &mut Connection, resume: bool) { let c1 = client.process_output(now()); - let s1 = server.process(c1.dgram(), now()); + let c11 = client.process_output(now()); + _ = server.process(c1.dgram(), now()); + let s1 = server.process(c11.dgram(), now()); let c2 = client.process(s1.dgram(), now()); + let s2 = server.process(c2.dgram(), now()); + let c2 = client.process(s2.dgram(), now()); let needs_auth = client .events() .any(|e| e == ConnectionEvent::AuthenticationNeeded); @@ -430,8 +434,7 @@ mod tests { assert!(client.state().connected()); let s2 = server.process(c2.dgram(), now()); assert_connected(server); - let c3 = client.process(s2.dgram(), now()); - assert!(c3.dgram().is_none()); + _ = client.process(s2.dgram(), now()); } // Start a client/server and check setting frame. diff --git a/neqo-http3/tests/httpconn.rs b/neqo-http3/tests/httpconn.rs index 90494361c5..18f6ede120 100644 --- a/neqo-http3/tests/httpconn.rs +++ b/neqo-http3/tests/httpconn.rs @@ -91,9 +91,13 @@ fn process_client_events(conn: &mut Http3Client) { fn connect_peers(hconn_c: &mut Http3Client, hconn_s: &mut Http3Server) -> Option { assert_eq!(hconn_c.state(), Http3State::Initializing); - let out = hconn_c.process_output(now()); // Initial - let out = hconn_s.process(out.dgram(), now()); // Initial + Handshake - let out = hconn_c.process(out.dgram(), now()); // ACK + let out1 = hconn_c.process_output(now()); // Initial + let out2 = hconn_c.process_output(now()); // Initial + _ = hconn_s.process(out1.dgram(), now()); // ACK + let out = hconn_s.process(out2.dgram(), now()); // Initial + Handshake + let out = hconn_c.process(out.dgram(), now()); + let out = hconn_s.process(out.dgram(), now()); + let out = hconn_c.process(out.dgram(), now()); drop(hconn_s.process(out.dgram(), now())); // consume ACK let authentication_needed = |e| matches!(e, Http3ClientEvent::AuthenticationNeeded); assert!(hconn_c.events().any(authentication_needed)); @@ -118,9 +122,15 @@ fn connect_peers_with_network_propagation_delay( let net_delay = Duration::from_millis(net_delay); assert_eq!(hconn_c.state(), Http3State::Initializing); let mut now = now(); - let out = hconn_c.process_output(now); // Initial + let out1 = hconn_c.process_output(now); // Initial + let out2 = hconn_c.process_output(now); // Initial + now += net_delay; + _ = hconn_s.process(out1.dgram(), now); // ACK + let out = hconn_s.process(out2.dgram(), now); now += net_delay; - let out = hconn_s.process(out.dgram(), now); // Initial + Handshake + let out = hconn_c.process(out.dgram(), now); + now += net_delay; + let out = hconn_s.process(out.dgram(), now); now += net_delay; let out = hconn_c.process(out.dgram(), now); // ACK now += net_delay; @@ -445,7 +455,12 @@ fn zerortt() { .unwrap(); hconn_c.stream_close_send(req).unwrap(); - let out = hconn_c.process(dgram, now()); + let out1 = hconn_c.process(dgram, now()); + let out2 = hconn_c.process_output(now()); + _ = hconn_s.process(out1.dgram(), now()); + let out = hconn_s.process(out2.dgram(), now()); + + let out = hconn_c.process(out.dgram(), now()); let out = hconn_s.process(out.dgram(), now()); let mut request_stream = None; diff --git a/neqo-http3/tests/priority.rs b/neqo-http3/tests/priority.rs index 7edb2d03b9..35bf1c453b 100644 --- a/neqo-http3/tests/priority.rs +++ b/neqo-http3/tests/priority.rs @@ -28,9 +28,13 @@ fn exchange_packets(client: &mut Http3Client, server: &mut Http3Server) { // Perform only QUIC transport handshake. fn connect_with(client: &mut Http3Client, server: &mut Http3Server) { assert_eq!(client.state(), Http3State::Initializing); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::Initializing); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); + let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); diff --git a/neqo-http3/tests/webtransport.rs b/neqo-http3/tests/webtransport.rs index 6291020f58..9018c8f27f 100644 --- a/neqo-http3/tests/webtransport.rs +++ b/neqo-http3/tests/webtransport.rs @@ -41,9 +41,13 @@ fn connect() -> (Http3Client, Http3Server) { ) .expect("create a server"); assert_eq!(client.state(), Http3State::Initializing); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::Initializing); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); + let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index dcdbb7bebe..52f311c962 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -383,6 +383,7 @@ impl Connection { let tphandler = Rc::new(RefCell::new(tps)); let crypto = Crypto::new( conn_params.get_versions().initial(), + &conn_params, agent, protocols.iter().map(P::as_ref).map(String::from).collect(), Rc::clone(&tphandler), @@ -915,7 +916,7 @@ impl Connection { // error. This may happen when client_start fails. self.set_state(State::Closed(error), now); } - State::WaitInitial => { + State::WaitInitial | State::WaitVersion => { // We don't have any state yet, so don't bother with // the closing state, just send one CONNECTION_CLOSE. if let Some(path) = path.or_else(|| self.paths.primary()) { @@ -1448,6 +1449,7 @@ impl Connection { { self.crypto.resend_unacked(PacketNumberSpace::Initial); self.resend_0rtt(now); + qdebug!("XXX resend done"); } } (PacketType::VersionNegotiation | PacketType::Retry | PacketType::OtherVersion, ..) => { @@ -1520,6 +1522,26 @@ impl Connection { self.start_handshake(path, packet, now); } + if matches!(self.state, State::WaitInitial | State::WaitVersion) { + self.set_state( + if self.has_version() { + State::Handshaking + } else { + State::WaitVersion + }, + now, + ); + if self.role == Role::Server { + self.zero_rtt_state = + if self.crypto.enable_0rtt(self.version, self.role) == Ok(true) { + qdebug!("[{self}] Accepted 0-RTT"); + ZeroRttState::AcceptedServer + } else { + ZeroRttState::Rejected + }; + } + } + if self.state.connected() { self.handle_migration(path, d, migrate, now); } else if self.role != Role::Client @@ -1817,34 +1839,25 @@ impl Connection { debug_assert_eq!(packet.packet_type(), PacketType::Initial); self.remote_initial_source_cid = Some(ConnectionId::from(packet.scid())); - let got_version = if self.role == Role::Server { + if self.role == Role::Server { self.cid_manager .add_odcid(self.original_destination_cid.as_ref().unwrap().clone()); // Make a path on which to run the handshake. self.setup_handshake_path(path, now); - - self.zero_rtt_state = if self.crypto.enable_0rtt(self.version, self.role) == Ok(true) { - qdebug!("[{self}] Accepted 0-RTT"); - ZeroRttState::AcceptedServer - } else { - qtrace!("[{self}] Rejected 0-RTT"); - ZeroRttState::Rejected - }; - - // The server knows the final version if it has remote transport parameters. - self.tps.borrow().remote.is_some() } else { qdebug!("[{self}] Changing to use Server CID={}", packet.scid()); debug_assert!(path.borrow().is_primary()); path.borrow_mut().set_remote_cid(packet.scid()); + }; + } + fn has_version(&self) -> bool { + if self.role == Role::Server { + // The server knows the final version if it has remote transport parameters. + self.tps.borrow().remote.is_some() + } else { // The client knows the final version if it processed a CRYPTO frame. self.stats.borrow().frame_rx.crypto > 0 - }; - if got_version { - self.set_state(State::Handshaking, now); - } else { - self.set_state(State::WaitVersion, now); } } @@ -2483,7 +2496,7 @@ impl Connection { if padded { needs_padding = false; self.loss_recovery.on_packet_sent(path, sent, now); - } else if pt == PacketType::Initial && (self.role == Role::Client || ack_eliciting) { + } else if pt == PacketType::Initial { // Packets containing Initial packets might need padding, and we want to // track that padding along with the Initial packet. So defer tracking. initial_sent = Some(sent); @@ -2503,6 +2516,14 @@ impl Connection { // but wait until after sending an ACK. self.discard_keys(PacketNumberSpace::Handshake, now); } + + // If the client has more Initial CRYPTO data queued up, do not coalesce. + if self.role == Role::Client + && *space == PacketNumberSpace::Initial + && !self.crypto.streams.is_empty(*space) + { + break; + } } if encoder.is_empty() { @@ -2512,7 +2533,7 @@ impl Connection { // Perform additional padding for Initial packets as necessary. let mut packets: Vec = encoder.into(); if let Some(mut initial) = initial_sent.take() { - if needs_padding { + if needs_padding && packets.len() < profile.limit() { qdebug!( "[{self}] pad Initial from {} to PLPMTU {}", packets.len(), diff --git a/neqo-transport/src/connection/params.rs b/neqo-transport/src/connection/params.rs index 5260415b2d..2ec2142edf 100644 --- a/neqo-transport/src/connection/params.rs +++ b/neqo-transport/src/connection/params.rs @@ -85,6 +85,8 @@ pub struct ConnectionParameters { pmtud: bool, /// Whether the connection should use SNI slicing. sni_slicing: bool, + /// Whether to enable mlkem768nistp256-sha256. + mlkem: bool, } impl Default for ConnectionParameters { @@ -110,6 +112,7 @@ impl Default for ConnectionParameters { pacing: true, pmtud: false, sni_slicing: true, + mlkem: true, } } } @@ -381,6 +384,17 @@ impl ConnectionParameters { self } + #[must_use] + pub const fn mlkem_enabled(&self) -> bool { + self.mlkem + } + + #[must_use] + pub const fn mlkem(mut self, mlkem: bool) -> Self { + self.mlkem = mlkem; + self + } + /// # Errors /// When a connection ID cannot be obtained. /// # Panics diff --git a/neqo-transport/src/connection/tests/close.rs b/neqo-transport/src/connection/tests/close.rs index a69db15220..a9883b987b 100644 --- a/neqo-transport/src/connection/tests/close.rs +++ b/neqo-transport/src/connection/tests/close.rs @@ -109,9 +109,11 @@ fn bad_tls_version() { .unwrap(); let mut server = default_server(); - let dgram = client.process_output(now()).dgram(); - assert!(dgram.is_some()); - let dgram = server.process(dgram, now()).dgram(); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + assert!(dgram1.is_some() && dgram2.is_some()); + _ = server.process(dgram1, now()).dgram(); + let dgram = server.process(dgram2, now()).dgram(); assert_eq!( *server.state(), State::Closed(CloseReason::Transport(Error::ProtocolViolation)) diff --git a/neqo-transport/src/connection/tests/ecn.rs b/neqo-transport/src/connection/tests/ecn.rs index 8704d5b9ab..1aba007dc7 100644 --- a/neqo-transport/src/connection/tests/ecn.rs +++ b/neqo-transport/src/connection/tests/ecn.rs @@ -76,7 +76,9 @@ fn drop_ecn_marked_datagrams() -> fn(Datagram) -> Option { #[test] fn handshake_delay_with_ecn_blackhole() { let start = now(); - let mut client = default_client(); + // `handshake_with_modifier` with-multi packet Intial flights will throw off the RTT calculation + // below. + let mut client = new_client(ConnectionParameters::default().mlkem(false)); let mut server = default_server(); let finish = handshake_with_modifier( &mut client, diff --git a/neqo-transport/src/connection/tests/handshake.rs b/neqo-transport/src/connection/tests/handshake.rs index c9ff3a5f41..b20e726ab2 100644 --- a/neqo-transport/src/connection/tests/handshake.rs +++ b/neqo-transport/src/connection/tests/handshake.rs @@ -46,13 +46,16 @@ const ECH_PUBLIC_NAME: &str = "public.example"; fn full_handshake(pmtud: bool) { qdebug!("---- client: generate CH"); let mut client = new_client(ConnectionParameters::default().pmtud(pmtud)); - let out = client.process_output(now()); - assert!(out.as_dgram_ref().is_some()); - assert_eq!(out.as_dgram_ref().unwrap().len(), client.plpmtu()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); + assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + assert_eq!(out1.as_dgram_ref().unwrap().len(), client.plpmtu()); + assert_eq!(out2.as_dgram_ref().unwrap().len(), client.plpmtu()); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = new_server(ConnectionParameters::default().pmtud(pmtud)); - let out = server.process(out.dgram(), now()); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); assert_eq!(out.as_dgram_ref().unwrap().len(), server.plpmtu()); @@ -60,6 +63,9 @@ fn full_handshake(pmtud: bool) { let out = client.process(out.dgram(), now()); assert!(out.as_dgram_ref().is_some()); + let out = server.process(out.dgram(), now()); + let out = client.process(out.dgram(), now()); + let out = server.process(out.dgram(), now()); assert!(out.as_dgram_ref().is_none()); @@ -101,18 +107,23 @@ fn handshake_pmtud() { fn handshake_failed_authentication() { qdebug!("---- client: generate CH"); let mut client = default_client(); - let out = client.process_output(now()); - assert!(out.as_dgram_ref().is_some()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); + assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = default_server(); - let out = server.process(out.dgram(), now()); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); qdebug!("---- client: cert verification"); let out = client.process(out.dgram(), now()); assert!(out.as_dgram_ref().is_some()); - + let out = server.process(out.dgram(), now()); + assert!(out.as_dgram_ref().is_some()); + let out = client.process(out.dgram(), now()); + assert!(out.as_dgram_ref().is_some()); let out = server.process(out.dgram(), now()); assert!(out.as_dgram_ref().is_none()); @@ -155,22 +166,33 @@ fn no_alpn() { } #[test] +#[allow(clippy::cognitive_complexity)] fn dup_server_flight1() { qdebug!("---- client: generate CH"); let mut client = default_client(); - let out = client.process_output(now()); - assert!(out.as_dgram_ref().is_some()); - assert_eq!(out.as_dgram_ref().unwrap().len(), client.plpmtu()); - qdebug!("Output={:0x?}", out.as_dgram_ref()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); + assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + assert_eq!(out1.as_dgram_ref().unwrap().len(), client.plpmtu()); + assert_eq!(out2.as_dgram_ref().unwrap().len(), client.plpmtu()); + qdebug!( + "Output={:0x?} {:0x?}", + out1.as_dgram_ref(), + out2.as_dgram_ref() + ); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = default_server(); - let out_to_rep = server.process(out.dgram(), now()); + _ = server.process(out1.dgram(), now()); + let out_to_rep = server.process(out2.dgram(), now()); assert!(out_to_rep.as_dgram_ref().is_some()); qdebug!("Output={:0x?}", out_to_rep.as_dgram_ref()); qdebug!("---- client: cert verification"); let out = client.process(Some(out_to_rep.as_dgram_ref().cloned().unwrap()), now()); + let out_to_rep2 = server.process(out.dgram(), now()); + let out = client.process(out_to_rep2.clone().dgram(), now()); + assert!(out.as_dgram_ref().is_some()); qdebug!("Output={:0x?}", out.as_dgram_ref()); @@ -184,27 +206,29 @@ fn dup_server_flight1() { assert!(out.as_dgram_ref().is_some()); qdebug!("Output={:0x?}", out.as_dgram_ref()); - assert_eq!(3, client.stats().packets_rx); + assert_eq!(4, client.stats().packets_rx); assert_eq!(0, client.stats().dups_rx); assert_eq!(1, client.stats().dropped_rx); qdebug!("---- Dup, ignored"); - let out = client.process(out_to_rep.dgram(), now()); + _ = client.process(out_to_rep.dgram(), now()); + let out = client.process(out_to_rep2.dgram(), now()); assert!(out.as_dgram_ref().is_none()); qdebug!("Output={:0x?}", out.as_dgram_ref()); // Four packets total received, 1 of them is a dup and one has been dropped because Initial keys // are dropped. Add 2 counts of the padding that the server adds to Initial packets. - assert_eq!(6, client.stats().packets_rx); + assert_eq!(8, client.stats().packets_rx); assert_eq!(1, client.stats().dups_rx); - assert_eq!(3, client.stats().dropped_rx); + assert_eq!(4, client.stats().dropped_rx); } // Test that we split crypto data if they cannot fit into one packet. // To test this we will use a long server certificate. #[test] fn crypto_frame_split() { - let mut client = default_client(); + // This test has its own logic for generating large CRYPTO frames, so turn off MLKEM. + let mut client = new_client(ConnectionParameters::default().mlkem(false)); let mut server = Connection::new_server( test_fixture::LONG_CERT_KEYS, @@ -273,8 +297,10 @@ fn send_05rtt() { let mut server = default_server(); let c1 = client.process_output(now()).dgram(); - assert!(c1.is_some()); - let s1 = server.process(c1, now()).dgram().unwrap(); + let c2 = client.process_output(now()).dgram(); + assert!(c1.is_some() && c2.is_some()); + server.process_input(c1.unwrap(), now()); + let s1 = server.process(c2, now()).dgram().unwrap(); assert_eq!(s1.len(), server.plpmtu()); // The server should accept writes at this point. @@ -282,11 +308,12 @@ fn send_05rtt() { // Complete the handshake at the client. client.process_input(s1, now()); - maybe_authenticate(&mut client); - assert_eq!(*client.state(), State::Connected); // The client should receive the 0.5-RTT data now. client.process_input(s2, now()); + maybe_authenticate(&mut client); + assert_eq!(*client.state(), State::Connected); + let mut buf = vec![0; DEFAULT_STREAM_DATA.len() + 1]; let stream_id = client .events() @@ -310,8 +337,10 @@ fn reorder_05rtt() { let mut server = default_server(); let c1 = client.process_output(now()).dgram(); - assert!(c1.is_some()); - let s1 = server.process(c1, now()).dgram().unwrap(); + let c2 = client.process_output(now()).dgram(); + assert!(c1.is_some() && c2.is_some()); + server.process_input(c1.unwrap(), now()); + let s1 = server.process(c2, now()).dgram().unwrap(); // The server should accept writes at this point. let s2 = send_something(&mut server, now()); @@ -370,10 +399,12 @@ fn reorder_05rtt_with_0rtt() { // Send ClientHello and some 0-RTT. let c1 = send_something(&mut client, now); - assert_coalesced_0rtt(&c1[..]); + let c2 = send_something(&mut client, now); + assert_coalesced_0rtt(&c2[..]); + server.process_input(c1, now); // Drop the 0-RTT from the coalesced datagram, so that the server // acknowledges the next 0-RTT packet. - let (c1, _) = split_datagram(&c1); + let (c1, _) = split_datagram(&c2); let c2 = send_something(&mut client, now); // Handle the first packet and send 0.5-RTT in response. Drop the response. @@ -390,6 +421,7 @@ fn reorder_05rtt_with_0rtt() { // Now PTO at the client and cause the server to re-send handshake packets. now += AT_LEAST_PTO; + _ = client.process_output(now).dgram(); let c3 = client.process_output(now).dgram(); assert_coalesced_0rtt(c3.as_ref().unwrap()); @@ -402,12 +434,12 @@ fn reorder_05rtt_with_0rtt() { client.process_input(s3, now); maybe_authenticate(&mut client); let c4 = client.process_output(now).dgram(); - assert_eq!(*client.state(), State::Connected); + // assert_eq!(*client.state(), State::Connected); assert_eq!(client.paths.rtt(), RTT); now += RTT / 2; server.process_input(c4.unwrap(), now); - assert_eq!(*server.state(), State::Confirmed); + // assert_eq!(*server.state(), State::Confirmed); // Don't check server RTT as it will be massively inflated by a // poor initial estimate received when the server dropped the // Initial packet number space. @@ -425,9 +457,11 @@ fn coalesce_05rtt() { // The first exchange doesn't offer a chance for the server to send. // So drop the server flight and wait for the PTO. let c1 = client.process_output(now).dgram(); - assert!(c1.is_some()); + let c11 = client.process_output(now).dgram(); + assert!(c1.is_some() && c11.is_some()); now += RTT / 2; - let s1 = server.process(c1, now).dgram(); + _ = server.process(c1, now).dgram(); + let s1 = server.process(c11, now).dgram(); assert!(s1.is_some()); // Drop the server flight. Then send some data. @@ -440,9 +474,15 @@ fn coalesce_05rtt() { // including the application data, which it sends in a 1-RTT packet. now += AT_LEAST_PTO; let c2 = client.process_output(now).dgram(); - assert!(c2.is_some()); + let c21 = client.process_output(now).dgram(); + assert!(c2.is_some() && c21.is_some()); now += RTT / 2; + _ = server.process(c21, now).dgram(); let s2 = server.process(c2, now).dgram(); + + let dgram = client.process(s2, now).dgram(); + let s2 = server.process(dgram, now).dgram(); + // Even though there is a 1-RTT packet at the end of the datagram, the // flight should be padded to full size. assert_eq!(s2.as_ref().unwrap().len(), server.plpmtu()); @@ -454,7 +494,7 @@ fn coalesce_05rtt() { drop(client.process(s2, now).dgram()); // This packet will contain an ACK, but we can ignore it. assert_eq!(client.stats().dropped_rx, 0); - assert_eq!(client.stats().packets_rx, 3); + assert_eq!(client.stats().packets_rx, 4); assert_eq!(client.stats().saved_datagrams, 1); // After (successful) authentication, the packet is processed. @@ -462,7 +502,7 @@ fn coalesce_05rtt() { let c3 = client.process_output(now).dgram(); assert!(c3.is_some()); assert_eq!(client.stats().dropped_rx, 0); // No Initial padding. - assert_eq!(client.stats().packets_rx, 4); + assert_eq!(client.stats().packets_rx, 5); assert_eq!(client.stats().saved_datagrams, 1); assert!(client.stats().frame_rx.padding > 0); // Padding uses frames. @@ -486,10 +526,18 @@ fn reorder_handshake() { let mut now = now(); let c1 = client.process_output(now).dgram(); - assert!(c1.is_some()); + let c2 = client.process_output(now).dgram(); + assert!(c1.is_some() && c2.is_some()); now += RTT / 2; - let s1 = server.process(c1, now).dgram(); + server.process_input(c1.unwrap(), now); + let s1 = server.process(c2, now).dgram(); + assert!(s1.is_some()); + now += RTT / 2; + let dgram = client.process(s1, now).dgram(); + assert!(dgram.is_some()); + now += RTT / 2; + let s1 = server.process(dgram, now).dgram(); assert!(s1.is_some()); // Drop the Initial packet from this. @@ -498,11 +546,11 @@ fn reorder_handshake() { // Pass just the handshake packet in and the client can't handle it yet. // It can only send another Initial packet. - now += RTT / 2; + now += RTT + RTT / 2; // With multi-packet MLKEM flights, client needs more time here. let dgram = client.process(s_hs, now).dgram(); assertions::assert_initial(dgram.as_ref().unwrap(), false); assert_eq!(client.stats().saved_datagrams, 1); - assert_eq!(client.stats().packets_rx, 1); + assert_eq!(client.stats().packets_rx, 2); // Get the server to try again. // Though we currently allow the server to arm its PTO timer, use @@ -520,11 +568,11 @@ fn reorder_handshake() { now += RTT / 2; client.process_input(s_hs.unwrap(), now); assert_eq!(client.stats().saved_datagrams, 2); - assert_eq!(client.stats().packets_rx, 2); + assert_eq!(client.stats().packets_rx, 3); client.process_input(s_init, now); // Each saved packet should now be "received" again. - assert_eq!(client.stats().packets_rx, 7); + assert_eq!(client.stats().packets_rx, 8); maybe_authenticate(&mut client); let c3 = client.process_output(now).dgram(); assert!(c3.is_some()); @@ -553,14 +601,22 @@ fn reorder_1rtt() { let mut now = now(); let c1 = client.process_output(now).dgram(); - assert!(c1.is_some()); + let c2 = client.process_output(now).dgram(); + assert!(c1.is_some() && c2.is_some()); now += RTT / 2; - let s1 = server.process(c1, now).dgram(); + server.process_input(c1.unwrap(), now); + let s1 = server.process(c2, now).dgram(); assert!(s1.is_some()); now += RTT / 2; - client.process_input(s1.unwrap(), now); + let dgram = client.process(s1, now).dgram(); + + now += RTT / 2; + let dgram = server.process(dgram, now).dgram(); + + now += RTT / 2; + client.process_input(dgram.unwrap(), now); maybe_authenticate(&mut client); let c2 = client.process_output(now).dgram(); assert!(c2.is_some()); @@ -572,18 +628,18 @@ fn reorder_1rtt() { server.process_input(d, now + RTT / 2); } // The server has now received those packets, and saved them. - // The two extra received are Initial + the junk we use for padding. - assert_eq!(server.stats().packets_rx, PACKETS + 2); + // The six extra received are Initial + the junk we use for padding. + assert_eq!(server.stats().packets_rx, PACKETS + 6); assert_eq!(server.stats().saved_datagrams, PACKETS); - assert_eq!(server.stats().dropped_rx, 1); + assert_eq!(server.stats().dropped_rx, 3); now += RTT / 2; let s2 = server.process(c2, now).dgram(); // The server has now received those packets, and saved them. // The two additional are a Handshake and a 1-RTT (w/ NEW_CONNECTION_ID). - assert_eq!(server.stats().packets_rx, PACKETS * 2 + 4); + assert_eq!(server.stats().packets_rx, PACKETS * 2 + 8); assert_eq!(server.stats().saved_datagrams, PACKETS); - assert_eq!(server.stats().dropped_rx, 1); + assert_eq!(server.stats().dropped_rx, 3); assert_eq!(*server.state(), State::Confirmed); assert_eq!(server.paths.rtt(), RTT); @@ -662,13 +718,18 @@ fn extra_initial_hs() { let mut server = default_server(); let mut now = now(); - let c_init = client.process_output(now).dgram(); - assert!(c_init.is_some()); + let c_init1 = client.process_output(now).dgram(); + let c_init2 = client.process_output(now).dgram(); + assert!(c_init1.is_some() && c_init2.is_some()); now += DEFAULT_RTT / 2; - let s_init = server.process(c_init, now).dgram(); + _ = server.process(c_init1, now).dgram(); + let s_init = server.process(c_init2, now).dgram(); assert!(s_init.is_some()); now += DEFAULT_RTT / 2; + let dgram = client.process(s_init, now).dgram(); + let s_init = server.process(dgram, now).dgram(); + // Drop the Initial packet, keep only the Handshake. let (_, undecryptable) = split_datagram(&s_init.unwrap()); assert!(undecryptable.is_some()); @@ -677,7 +738,14 @@ fn extra_initial_hs() { // Do that EXTRA_INITIALS times and each time the client will emit // another Initial packet. for _ in 0..=super::super::EXTRA_INITIALS { - let c_init = client.process(undecryptable.clone(), now).dgram(); + let c_init = match client.process(undecryptable.clone(), now) { + Output::None => todo!(), + Output::Datagram(c_init) => Some(c_init), + Output::Callback(duration) => { + now += duration; + client.process_output(now).dgram() + } + }; assertions::assert_initial(c_init.as_ref().unwrap(), false); now += DEFAULT_RTT / 10; } @@ -687,7 +755,7 @@ fn extra_initial_hs() { assert!(nothing.is_none()); // Until PTO, where another Initial can be used to complete the handshake. - now += AT_LEAST_PTO; + now += client.process_output(now).callback(); let c_init = client.process_output(now).dgram(); assertions::assert_initial(c_init.as_ref().unwrap(), false); now += DEFAULT_RTT / 2; @@ -708,13 +776,18 @@ fn extra_initial_invalid_cid() { let mut server = default_server(); let mut now = now(); - let c_init = client.process_output(now).dgram(); - assert!(c_init.is_some()); + let c_init1 = client.process_output(now).dgram(); + let c_init2 = client.process_output(now).dgram(); + assert!(c_init1.is_some() && c_init2.is_some()); now += DEFAULT_RTT / 2; - let s_init = server.process(c_init, now).dgram(); + _ = server.process(c_init1, now).dgram(); + let s_init = server.process(c_init2, now).dgram(); assert!(s_init.is_some()); now += DEFAULT_RTT / 2; + let dgram = client.process(s_init, now).dgram(); + let s_init = server.process(dgram, now).dgram(); + // If the client receives a packet that contains the wrong connection // ID, it won't send another Initial. let (_, hs) = split_datagram(&s_init.unwrap()); @@ -761,7 +834,8 @@ fn connect_one_version() { #[test] fn anti_amplification() { - let mut client = default_client(); + // This test has its own logic for generating large CRYPTO frames, so turn off MLKEM. + let mut client = new_client(ConnectionParameters::default().mlkem(false)); let mut server = default_server(); let mut now = now(); @@ -828,11 +902,13 @@ fn garbage_initial() { #[test] fn drop_initial_packet_from_wrong_address() { let mut client = default_client(); - let out = client.process_output(now()); - assert!(out.as_dgram_ref().is_some()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); + assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); let mut server = default_server(); - let out = server.process(out.dgram(), now()); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); let p = out.dgram().unwrap(); @@ -918,7 +994,11 @@ fn ech_retry() { .client_enable_ech(damaged_ech_config(server.ech_config())) .unwrap(); - let dgram = client.process_output(now()).dgram(); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + _ = server.process(dgram1, now()).dgram(); + let dgram = server.process(dgram2, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); client.process_input(dgram.unwrap(), now()); let auth_event = ConnectionEvent::EchFallbackAuthenticationNeeded { @@ -973,7 +1053,11 @@ fn ech_retry_fallback_rejected() { .client_enable_ech(damaged_ech_config(server.ech_config())) .unwrap(); - let dgram = client.process_output(now()).dgram(); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + _ = server.process(dgram1, now()).dgram(); + let dgram = server.process(dgram2, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); client.process_input(dgram.unwrap(), now()); let auth_event = ConnectionEvent::EchFallbackAuthenticationNeeded { @@ -1006,7 +1090,11 @@ fn bad_min_ack_delay() { .unwrap(); let mut client = default_client(); - let dgram = client.process_output(now()).dgram(); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + _ = server.process(dgram1, now()).dgram(); + let dgram = server.process(dgram2, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); client.process_input(dgram.unwrap(), now()); client.authenticated(AuthenticationStatus::Ok, now()); @@ -1030,10 +1118,14 @@ fn only_server_initial() { let mut client = default_client(); let mut now = now(); - let client_dgram = client.process_output(now).dgram(); + let client_dgram1 = client.process_output(now).dgram(); + let client_dgram2 = client.process_output(now).dgram(); // Now fetch two flights of messages from the server. - let server_dgram1 = server.process(client_dgram, now).dgram(); + server.process_input(client_dgram1.unwrap(), now); + let dgram = server.process(client_dgram2, now).dgram(); + let dgram = client.process(dgram, now).dgram(); + let server_dgram1 = server.process(dgram, now).dgram(); let server_dgram2 = server.process_output(now + AT_LEAST_PTO).dgram(); // Only pass on the Initial from the first. We should get a Handshake in return. @@ -1080,24 +1172,30 @@ fn no_extra_probes_after_confirmed() { let mut now = now(); // First, collect a client Initial. - let spare_initial = client.process_output(now).dgram(); - assert!(spare_initial.is_some()); + let spare_initial1 = client.process_output(now).dgram(); + let spare_initial2 = client.process_output(now).dgram(); + assert!(spare_initial1.is_some() && spare_initial2.is_some()); // Collect ANOTHER client Initial. now += AT_LEAST_PTO; - let dgram = client.process_output(now).dgram(); - let (replay_initial, _) = split_datagram(dgram.as_ref().unwrap()); + let dgram1 = client.process_output(now).dgram(); + _ = client.process_output(now).dgram(); + let (replay_initial, _) = split_datagram(dgram1.as_ref().unwrap()); // Finally, run the handshake. now += AT_LEAST_PTO * 2; - let dgram = client.process_output(now).dgram(); - let dgram = server.process(dgram, now).dgram(); + let dgram1 = client.process_output(now).dgram(); + let dgram2 = client.process_output(now).dgram(); + server.process_input(dgram1.unwrap(), now); + let dgram = server.process(dgram2, now).dgram(); // The server should have dropped the Initial keys now, so passing in the Initial // should elicit a retransmit rather than having it completely ignored. let spare_handshake = server.process(Some(replay_initial), now).dgram(); assert!(spare_handshake.is_some()); + let dgram = client.process(dgram, now).dgram(); + let dgram = server.process(dgram, now).dgram(); client.process_input(dgram.unwrap(), now); maybe_authenticate(&mut client); let dgram = client.process_output(now).dgram(); @@ -1107,7 +1205,7 @@ fn no_extra_probes_after_confirmed() { assert_eq!(*client.state(), State::Confirmed); assert_eq!(*server.state(), State::Confirmed); - let probe = server.process(spare_initial, now).dgram(); + let probe = server.process(spare_initial1, now).dgram(); assert!(probe.is_none()); let probe = client.process(spare_handshake, now).dgram(); assert!(probe.is_none()); @@ -1120,7 +1218,13 @@ fn implicit_rtt_server() { let mut client = default_client(); let mut now = now(); - let dgram = client.process_output(now).dgram(); + let dgram1 = client.process_output(now).dgram(); + let dgram2 = client.process_output(now).dgram(); + now += RTT / 2; + server.process_input(dgram1.unwrap(), now); + let dgram = server.process(dgram2, now).dgram(); + now += RTT / 2; + let dgram = client.process(dgram, now).dgram(); now += RTT / 2; let dgram = server.process(dgram, now).dgram(); now += RTT / 2; @@ -1142,19 +1246,24 @@ fn emit_authentication_needed_once() { test_fixture::LONG_CERT_KEYS, test_fixture::DEFAULT_ALPN, Rc::new(RefCell::new(CountingConnectionIdGenerator::default())), - ConnectionParameters::default(), + // TODO: Why is this needed to avoind the 5ms pacing delay? + ConnectionParameters::default().pacing(false), ) .expect("create a server"); let client1 = client.process_output(now()); - assert!(client1.as_dgram_ref().is_some()); + let client2 = client.process_output(now()); + assert!(client1.as_dgram_ref().is_some() && client2.as_dgram_ref().is_some()); // The entire server flight doesn't fit in a single packet because the // certificate is large, therefore the server will produce 2 packets. - let server1 = server.process(client1.dgram(), now()); + _ = server.process(client1.dgram(), now()); + let server1 = server.process(client2.dgram(), now()); assert!(server1.as_dgram_ref().is_some()); let server2 = server.process_output(now()); assert!(server2.as_dgram_ref().is_some()); + let server3 = server.process_output(now()); + assert!(server3.as_dgram_ref().is_some()); let authentication_needed_count = |client: &mut Connection| { client @@ -1163,7 +1272,7 @@ fn emit_authentication_needed_once() { .count() }; - // Upon receiving the first packet, the client has the server certificate, + // Upon receiving the first two packet, the client has the server certificate, // but not yet all required handshake data. It moves to // `HandshakeState::AuthenticationPending` and emits a // `ConnectionEvent::AuthenticationNeeded` event. @@ -1174,6 +1283,7 @@ fn emit_authentication_needed_once() { // also fit in the same packet. Our default test setup achieves this, but // changes to the setup might invalidate this test. _ = client.process(server1.dgram(), now()); + _ = client.process(server2.dgram(), now()); assert_eq!(1, authentication_needed_count(&mut client)); assert!(client.peer_certificate().is_some()); @@ -1181,24 +1291,27 @@ fn emit_authentication_needed_once() { // `Connection::authenticated`. On receiving the second packet from the // server, the client must not emit a another // `ConnectionEvent::AuthenticationNeeded`. - _ = client.process(server2.dgram(), now()); + _ = client.process(server3.dgram(), now()); assert_eq!(0, authentication_needed_count(&mut client)); } #[test] fn client_initial_retransmits_identical() { let mut now = now(); - let mut client = default_client(); + // TODO: With pacing on, why does the delay callback return by 5ms and then PTO after 295ms? + let mut client = new_client(ConnectionParameters::default().pacing(false)); - // Force the client to retransmit its Initial packet a number of times and make sure the + // Force the client to retransmit its Initial flight a number of times and make sure the // retranmissions are identical to the original. Also, verify the PTO durations. for i in 1..=5 { - let ci = client.process_output(now).dgram().unwrap(); - assert_eq!(ci.len(), client.plpmtu()); + let ci1 = client.process_output(now).dgram().unwrap(); + assert_eq!(ci1.len(), client.plpmtu()); + let ci2 = client.process_output(now).dgram().unwrap(); + assert_eq!(ci2.len(), client.plpmtu()); assert_eq!( client.stats().frame_tx, FrameStats { - crypto: 2 * i, + crypto: 3 * i, ..Default::default() } ); @@ -1212,34 +1325,43 @@ fn client_initial_retransmits_identical() { fn server_initial_retransmits_identical() { let mut now = now(); let mut client = default_client(); - let mut ci = client.process_output(now).dgram(); + let mut ci1 = client.process_output(now).dgram(); + let mut ci2 = client.process_output(now).dgram(); - // Force the server to retransmit its Initial packet a number of times and make sure the + // Force the server to retransmit its Initial flight a number of times and make sure the // retranmissions are identical to the original. Also, verify the PTO durations. - let mut server = default_server(); + let mut server = new_server(ConnectionParameters::default().pacing(false)); let mut total_ptos = Duration::from_secs(0); - for i in 1..=3 { - let si = server.process(ci.take(), now).dgram().unwrap(); - assert_eq!(si.len(), server.plpmtu()); + for i in 1..=2 { + let si1 = server.process(ci1.take(), now).dgram().unwrap(); + assert_eq!(si1.len(), server.plpmtu()); + let si2 = server.process(ci2.take(), now).dgram().unwrap(); + assert_eq!(si2.len(), server.plpmtu()); + if i == 1 { + // On the first iteration, the server will want to send its entire flight. + // During later ones, we will have hit a PTO and can hence only send two packets. + _ = server.process(ci2.take(), now).dgram().unwrap(); + } assert_eq!( server.stats().frame_tx, FrameStats { - crypto: i * 2, - ack: i, + crypto: i * 3, + ack: i * 2 - usize::from(i != 1), + largest_acknowledged: i as u64 - u64::from(i != 1), ..Default::default() } ); let pto = server.process_output(now).callback(); - if i < 3 { - assert_eq!(pto, DEFAULT_RTT * 3 * (1 << (i - 1))); - } else { - // Server is amplification-limited after three (re)transmissions. - assert_eq!(pto, server.conn_params.get_idle_timeout() - total_ptos); - } now += pto; total_ptos += pto; } + + // Server is amplification-limited now. + let si1 = server.process(ci1.take(), now).dgram().unwrap(); + assert_eq!(si1.len(), server.plpmtu()); + let pto = server.process_output(now).callback(); + assert_eq!(pto, server.conn_params.get_idle_timeout() - total_ptos); } #[test] diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index 38f657b89b..362b2ad100 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -289,7 +289,11 @@ fn idle_caching() { // Perform the first round trip, but drop the Initial from the server. // The client then caches the Handshake packet. - let dgram = client.process_output(start).dgram(); + let dgram1 = client.process_output(start).dgram(); + let dgram2 = client.process_output(start).dgram(); + _ = server.process(dgram1, start).dgram(); + let dgram = server.process(dgram2, start).dgram(); + let dgram = client.process(dgram, start).dgram(); let dgram = server.process(dgram, start).dgram(); let (_, handshake) = split_datagram(&dgram.unwrap()); client.process_input(handshake.unwrap(), start); @@ -679,7 +683,13 @@ fn keep_alive_with_ack_eliciting_packet_lost() { // - Idle time out will trigger (at the timeout + IDLE_TIMEOUT) const IDLE_TIMEOUT: Duration = Duration::from_millis(6000); - let mut client = new_client(ConnectionParameters::default().idle_timeout(IDLE_TIMEOUT)); + // This tests makes too many assumptions about single-packet flights and PTOs for multi-packet + // MLKEM flights to work. + let mut client = new_client( + ConnectionParameters::default() + .idle_timeout(IDLE_TIMEOUT) + .mlkem(false), + ); let mut server = default_server(); let mut now = connect_rtt_idle(&mut client, &mut server, RTT); // connect_rtt_idle increase now by RTT / 2; diff --git a/neqo-transport/src/connection/tests/keys.rs b/neqo-transport/src/connection/tests/keys.rs index d8686b7943..300677727e 100644 --- a/neqo-transport/src/connection/tests/keys.rs +++ b/neqo-transport/src/connection/tests/keys.rs @@ -55,25 +55,31 @@ fn overwrite_invocations(n: PacketNumber) { fn discarded_initial_keys() { qdebug!("---- client: generate CH"); let mut client = default_client(); - let init_pkt_c = client.process_output(now()).dgram(); - assert!(init_pkt_c.is_some()); - assert_eq!(init_pkt_c.as_ref().unwrap().len(), client.plpmtu()); + let init_pkt_c1 = client.process_output(now()).dgram(); + let init_pkt_c2 = client.process_output(now()).dgram(); + assert!(init_pkt_c1.is_some() && init_pkt_c2.is_some()); + assert_eq!(init_pkt_c1.as_ref().unwrap().len(), client.plpmtu()); + assert_eq!(init_pkt_c2.as_ref().unwrap().len(), client.plpmtu()); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = default_server(); - let init_pkt_s = server.process(init_pkt_c.clone(), now()).dgram(); + server.process_input(init_pkt_c1.clone().unwrap(), now()); + let init_pkt_s = server.process(init_pkt_c2, now()).dgram(); assert!(init_pkt_s.is_some()); qdebug!("---- client: cert verification"); let out = client.process(init_pkt_s.clone(), now()).dgram(); assert!(out.is_some()); + let out = server.process(out, now()).dgram(); + client.process_input(out.unwrap(), now()); + // The client has received a handshake packet. It will remove the Initial keys. // We will check this by processing init_pkt_s a second time. // The initial packet should be dropped. The packet contains a Handshake packet as well, which // will be marked as dup. And it will contain padding, which will be "dropped". // The client will generate a Handshake packet here to avoid stalling. - check_discarded(&mut client, &init_pkt_s.unwrap(), true, 2, 1); + check_discarded(&mut client, &init_pkt_s.unwrap(), true, 1, 0); assert!(maybe_authenticate(&mut client)); @@ -81,7 +87,7 @@ fn discarded_initial_keys() { // packet from the client. // We will check this by processing init_pkt_c a second time. // The dropped packet is padding. The Initial packet has been mark dup. - check_discarded(&mut server, &init_pkt_c.clone().unwrap(), false, 1, 1); + check_discarded(&mut server, &init_pkt_c1.clone().unwrap(), false, 1, 1); qdebug!("---- client: SH..FIN -> FIN"); let out = client.process_output(now()).dgram(); @@ -96,7 +102,7 @@ fn discarded_initial_keys() { // We will check this by processing init_pkt_c a third time. // The Initial packet has been dropped and padding that follows it. // There is no dups, everything has been dropped. - check_discarded(&mut server, &init_pkt_c.unwrap(), false, 1, 0); + check_discarded(&mut server, &init_pkt_c1.unwrap(), false, 1, 0); } #[test] @@ -222,11 +228,21 @@ fn key_update_before_confirmed() { assert_update_blocked(&mut server); // Client Initial - let dgram = client.process_output(now()).dgram(); - assert!(dgram.is_some()); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + assert!(dgram1.is_some() && dgram2.is_some()); assert_update_blocked(&mut client); // Server Initial + Handshake + server.process_input(dgram1.unwrap(), now()); + let dgram = server.process(dgram2, now()).dgram(); + assert!(dgram.is_some()); + assert_update_blocked(&mut server); + + let dgram = client.process(dgram, now()).dgram(); + assert!(dgram.is_some()); + assert_update_blocked(&mut client); + let dgram = server.process(dgram, now()).dgram(); assert!(dgram.is_some()); assert_update_blocked(&mut server); diff --git a/neqo-transport/src/connection/tests/migration.rs b/neqo-transport/src/connection/tests/migration.rs index 949ed96134..567df7a989 100644 --- a/neqo-transport/src/connection/tests/migration.rs +++ b/neqo-transport/src/connection/tests/migration.rs @@ -572,7 +572,7 @@ fn migrate_same_fail() { assert_path_challenge_min_len(&client, &probe, now); // -1 because first PATH_CHALLENGE already sent above - for _ in 0..MAX_PATH_PROBES * 2 - 1 { + for _ in 0..MAX_PATH_PROBES - 1 { let cb = client.process_output(now).callback(); assert_ne!(cb, Duration::new(0, 0)); now += cb; @@ -703,7 +703,11 @@ fn migration_client_empty_cid() { /// Drive the handshake in the most expeditious fashion. /// Returns the packet containing `HANDSHAKE_DONE` from the server. fn fast_handshake(client: &mut Connection, server: &mut Connection) -> Option { - let dgram = client.process_output(now()).dgram(); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + _ = server.process(dgram1, now()).dgram(); + let dgram = server.process(dgram2, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); client.process_input(dgram.unwrap(), now()); assert!(maybe_authenticate(client)); diff --git a/neqo-transport/src/connection/tests/priority.rs b/neqo-transport/src/connection/tests/priority.rs index e770d08f31..36e6216eff 100644 --- a/neqo-transport/src/connection/tests/priority.rs +++ b/neqo-transport/src/connection/tests/priority.rs @@ -6,7 +6,7 @@ use std::{cell::RefCell, rc::Rc}; -use neqo_common::event::Provider as _; +use neqo_common::{event::Provider as _, Datagram}; use test_fixture::now; use super::{ @@ -201,18 +201,31 @@ fn repairing_loss() { )); } -#[test] -fn critical() { +fn connect_for_0rtt() -> (Connection, Connection) { let mut client = default_client(); let mut server = default_server(); - let now = now(); // Rather than connect, send stream data in 0.5-RTT. // That allows this to test that critical streams preempt most frame types. - let dgram = client.process_output(now).dgram(); - let dgram = server.process(dgram, now).dgram(); - client.process_input(dgram.unwrap(), now); - maybe_authenticate(&mut client); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + server.process_input(dgram1.unwrap(), now()); + let dgram = server.process(dgram2, now()).dgram(); + client.process_input(dgram.unwrap(), now()); + (client, server) +} + +fn complete_handshake(client: &mut Connection, server: &mut Connection, dgram: Option) { + let dgram = client.process(dgram, now()).dgram(); + maybe_authenticate(client); + let dgram = server.process(dgram, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); + server.process_input(dgram.unwrap(), now()); +} + +#[test] +fn critical() { + let (mut client, mut server) = connect_for_0rtt(); let id = server.stream_create(StreamType::UniDi).unwrap(); server @@ -229,21 +242,20 @@ fn critical() { fill_stream(&mut server, id); let stats_before = server.stats().frame_tx; - let dgram = server.process_output(now).dgram(); + let dgram = server.process_output(now()).dgram(); let stats_after = server.stats().frame_tx; - assert_eq!(stats_after.crypto, stats_before.crypto); + assert_eq!(stats_after.crypto, stats_before.crypto + 2); assert_eq!(stats_after.streams_blocked, 0); assert_eq!(stats_after.new_connection_id, 0); assert_eq!(stats_after.new_token, 0); assert_eq!(stats_after.handshake_done, 0); // Complete the handshake. - let dgram = client.process(dgram, now).dgram(); - server.process_input(dgram.unwrap(), now); + complete_handshake(&mut client, &mut server, dgram); // Critical beats everything but HANDSHAKE_DONE. let stats_before = server.stats().frame_tx; - drop(fill_cwnd(&mut server, id, now)); + drop(fill_cwnd(&mut server, id, now())); let stats_after = server.stats().frame_tx; assert_eq!(stats_after.crypto, stats_before.crypto); assert_eq!(stats_after.streams_blocked, 0); @@ -254,16 +266,7 @@ fn critical() { #[test] fn important() { - let mut client = default_client(); - let mut server = default_server(); - let now = now(); - - // Rather than connect, send stream data in 0.5-RTT. - // That allows this to test that important streams preempt most frame types. - let dgram = client.process_output(now).dgram(); - let dgram = server.process(dgram, now).dgram(); - client.process_input(dgram.unwrap(), now); - maybe_authenticate(&mut client); + let (mut client, mut server) = connect_for_0rtt(); let id = server.stream_create(StreamType::UniDi).unwrap(); server @@ -280,9 +283,9 @@ fn important() { while server.stream_create(StreamType::UniDi).is_ok() {} let stats_before = server.stats().frame_tx; - let dgram = server.process_output(now).dgram(); + let dgram = server.process_output(now()).dgram(); let stats_after = server.stats().frame_tx; - assert_eq!(stats_after.crypto, stats_before.crypto); + assert_eq!(stats_after.crypto, stats_before.crypto + 2); assert_eq!(stats_after.streams_blocked, 1); assert_eq!(stats_after.new_connection_id, 0); assert_eq!(stats_after.new_token, 0); @@ -290,12 +293,11 @@ fn important() { assert_eq!(stats_after.stream, stats_before.stream + 1); // Complete the handshake. - let dgram = client.process(dgram, now).dgram(); - server.process_input(dgram.unwrap(), now); + complete_handshake(&mut client, &mut server, dgram); // Important beats everything but flow control. let stats_before = server.stats().frame_tx; - drop(fill_cwnd(&mut server, id, now)); + drop(fill_cwnd(&mut server, id, now())); let stats_after = server.stats().frame_tx; assert_eq!(stats_after.crypto, stats_before.crypto); assert_eq!(stats_after.streams_blocked, 1); @@ -307,16 +309,7 @@ fn important() { #[test] fn high_normal() { - let mut client = default_client(); - let mut server = default_server(); - let now = now(); - - // Rather than connect, send stream data in 0.5-RTT. - // That allows this to test that important streams preempt most frame types. - let dgram = client.process_output(now).dgram(); - let dgram = server.process(dgram, now).dgram(); - client.process_input(dgram.unwrap(), now); - maybe_authenticate(&mut client); + let (mut client, mut server) = connect_for_0rtt(); let id = server.stream_create(StreamType::UniDi).unwrap(); server @@ -333,9 +326,9 @@ fn high_normal() { while server.stream_create(StreamType::UniDi).is_ok() {} let stats_before = server.stats().frame_tx; - let dgram = server.process_output(now).dgram(); + let dgram = server.process_output(now()).dgram(); let stats_after = server.stats().frame_tx; - assert_eq!(stats_after.crypto, stats_before.crypto); + assert_eq!(stats_after.crypto, stats_before.crypto + 2); assert_eq!(stats_after.streams_blocked, 1); assert_eq!(stats_after.new_connection_id, 0); assert_eq!(stats_after.new_token, 0); @@ -343,14 +336,13 @@ fn high_normal() { assert_eq!(stats_after.stream, stats_before.stream + 1); // Complete the handshake. - let dgram = client.process(dgram, now).dgram(); - server.process_input(dgram.unwrap(), now); + complete_handshake(&mut client, &mut server, dgram); // High or Normal doesn't beat NEW_CONNECTION_ID, // but they beat CRYPTO/NEW_TOKEN. let stats_before = server.stats().frame_tx; - server.send_ticket(now, &[]).unwrap(); - drop(fill_cwnd(&mut server, id, now)); + server.send_ticket(now(), &[]).unwrap(); + drop(fill_cwnd(&mut server, id, now())); let stats_after = server.stats().frame_tx; assert_eq!(stats_after.crypto, stats_before.crypto); assert_eq!(stats_after.streams_blocked, 1); diff --git a/neqo-transport/src/connection/tests/recovery.rs b/neqo-transport/src/connection/tests/recovery.rs index dcaabccbdf..a001a31fc1 100644 --- a/neqo-transport/src/connection/tests/recovery.rs +++ b/neqo-transport/src/connection/tests/recovery.rs @@ -166,17 +166,21 @@ fn pto_initial() { qdebug!("---- client: generate CH"); let mut client = default_client(); let pkt1 = client.process_output(now).dgram(); - assert!(pkt1.is_some()); - assert_eq!(pkt1.clone().unwrap().len(), client.plpmtu()); + let pkt2 = client.process_output(now).dgram(); + assert!(pkt1.is_some() && pkt2.is_some()); + assert_eq!(pkt1.unwrap().len(), client.plpmtu()); + assert_eq!(pkt2.unwrap().len(), client.plpmtu()); let delay = client.process_output(now).callback(); assert_eq!(delay, INITIAL_PTO); // Resend initial after PTO. now += delay; + let pkt1 = client.process_output(now).dgram(); let pkt2 = client.process_output(now).dgram(); - assert!(pkt2.is_some()); - assert_eq!(pkt2.unwrap().len(), client.plpmtu()); + assert!(pkt1.is_some() && pkt2.is_some()); + assert_eq!(pkt1.clone().unwrap().len(), client.plpmtu()); + assert_eq!(pkt2.clone().unwrap().len(), client.plpmtu()); let delay = client.process_output(now).callback(); // PTO has doubled. @@ -184,7 +188,8 @@ fn pto_initial() { // Server process the first initial pkt. let mut server = default_server(); - let out = server.process(pkt1, now).dgram(); + _ = server.process(pkt1, now).dgram(); + let out = server.process(pkt2, now).dgram(); assert!(out.is_some()); // Client receives ack for the first initial packet as well a Handshake packet. @@ -212,13 +217,16 @@ fn pto_handshake_complete() { let mut client = default_client(); let mut server = default_server(); - let pkt = client.process_output(now).dgram(); - assert_initial(pkt.as_ref().unwrap(), false); + let pkt1 = client.process_output(now).dgram(); + let pkt2 = client.process_output(now).dgram(); + assert_initial(pkt1.as_ref().unwrap(), false); + assert_initial(pkt2.as_ref().unwrap(), false); let cb = client.process_output(now).callback(); assert_eq!(cb, Duration::from_millis(300)); now += HALF_RTT; - let pkt = server.process(pkt, now).dgram(); + _ = server.process(pkt1, now).dgram(); + let pkt = server.process(pkt2, now).dgram(); assert_initial(pkt.as_ref().unwrap(), false); now += HALF_RTT; @@ -331,12 +339,14 @@ fn pto_handshake_frames() { let mut now = now(); qdebug!("---- client: generate CH"); let mut client = default_client(); - let pkt = client.process_output(now); + let pkt1 = client.process_output(now); + let pkt2 = client.process_output(now); now += Duration::from_millis(10); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = default_server(); - let pkt = server.process(pkt.dgram(), now); + _ = server.process(pkt1.dgram(), now); + let pkt = server.process(pkt2.dgram(), now); now += Duration::from_millis(10); qdebug!("---- client: cert verification"); @@ -774,7 +784,13 @@ fn fast_pto() { /// based on the "true" value of the timer. #[test] fn fast_pto_persistent_congestion() { - let mut client = new_client(ConnectionParameters::default().fast_pto(FAST_PTO_SCALE * 2)); + // This tests makes too many assumptions about single-packet PTOs for multi-packet MLKEM flights + // to work. + let mut client = new_client( + ConnectionParameters::default() + .fast_pto(FAST_PTO_SCALE * 2) + .mlkem(false), + ); let mut server = default_server(); let mut now = connect_rtt_idle(&mut client, &mut server, DEFAULT_RTT); diff --git a/neqo-transport/src/connection/tests/resumption.rs b/neqo-transport/src/connection/tests/resumption.rs index 7b976712fe..1a3d650a85 100644 --- a/neqo-transport/src/connection/tests/resumption.rs +++ b/neqo-transport/src/connection/tests/resumption.rs @@ -90,8 +90,11 @@ fn ticket_rtt(rtt: Duration) -> Duration { // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. const ACK_FRAME_1: &[u8] = &[0x03, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00]; + // This test needs to decrypt the CI, so turn off MLKEM. let mut client = new_client( - ConnectionParameters::default().versions(Version::Version1, vec![Version::Version1]), + ConnectionParameters::default() + .versions(Version::Version1, vec![Version::Version1]) + .mlkem(false), ); let mut server = default_server(); let mut now = now(); diff --git a/neqo-transport/src/connection/tests/stream.rs b/neqo-transport/src/connection/tests/stream.rs index 2cb47b08c2..2848c0159b 100644 --- a/neqo-transport/src/connection/tests/stream.rs +++ b/neqo-transport/src/connection/tests/stream.rs @@ -32,10 +32,14 @@ use crate::{ fn stream_create() { let mut client = default_client(); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); let mut server = default_server(); - let out = server.process(out.dgram(), now()); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); + let out = client.process(out.dgram(), now()); + let out = server.process(out.dgram(), now()); let out = client.process(out.dgram(), now()); drop(server.process(out.dgram(), now())); assert!(maybe_authenticate(&mut client)); @@ -752,16 +756,21 @@ fn client_fin_reorder() { let mut server = default_server(); // Send ClientHello. - let client_hs = client.process_output(now()); - assert!(client_hs.as_dgram_ref().is_some()); + let client_hs1 = client.process_output(now()); + let client_hs2 = client.process_output(now()); + assert!(client_hs1.as_dgram_ref().is_some() && client_hs2.as_dgram_ref().is_some()); - let server_hs = server.process(client_hs.dgram(), now()); + _ = server.process(client_hs1.dgram(), now()); + let server_hs = server.process(client_hs2.dgram(), now()); assert!(server_hs.as_dgram_ref().is_some()); // ServerHello, etc... let client_ack = client.process(server_hs.dgram(), now()); assert!(client_ack.as_dgram_ref().is_some()); - let server_out = server.process(client_ack.dgram(), now()); + let dgram = server.process(client_ack.dgram(), now()); + let dgram = client.process(dgram.dgram(), now()); + + let server_out = server.process(dgram.dgram(), now()); assert!(server_out.as_dgram_ref().is_none()); assert!(maybe_authenticate(&mut client)); @@ -1272,14 +1281,18 @@ fn session_flow_control_affects_all_streams() { fn connect_w_different_limit(bidi_limit: u64, unidi_limit: u64) { let mut client = default_client(); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); let mut server = new_server( ConnectionParameters::default() .max_streams(StreamType::BiDi, bidi_limit) .max_streams(StreamType::UniDi, unidi_limit), ); - let out = server.process(out.dgram(), now()); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); + let out = client.process(out.dgram(), now()); + let out = server.process(out.dgram(), now()); let out = client.process(out.dgram(), now()); drop(server.process(out.dgram(), now())); diff --git a/neqo-transport/src/connection/tests/vn.rs b/neqo-transport/src/connection/tests/vn.rs index 60fafd56aa..e305af583d 100644 --- a/neqo-transport/src/connection/tests/vn.rs +++ b/neqo-transport/src/connection/tests/vn.rs @@ -76,6 +76,7 @@ fn version_negotiation_current_version() { .dgram() .expect("a datagram") .to_vec(); + _ = client.process_output(now()).dgram().expect("a datagram"); let vn = create_vn( &initial_pkt, @@ -83,7 +84,8 @@ fn version_negotiation_current_version() { ); let dgram = datagram(vn); - let delay = client.process(Some(dgram), now()).callback(); + let mut delay = client.process(Some(dgram), now()).callback(); + delay += client.process_output(now() + delay).callback(); // TODO: Why is there first a 5ms pacing delay before the PTO? assert_eq!(delay, INITIAL_PTO); assert_eq!(*client.state(), State::WaitInitial); assert_eq!(1, client.stats().dropped_rx); @@ -98,11 +100,13 @@ fn version_negotiation_version0() { .dgram() .expect("a datagram") .to_vec(); + _ = client.process_output(now()).dgram().expect("a datagram"); let vn = create_vn(&initial_pkt, &[0, 0x1a1a_1a1a]); let dgram = datagram(vn); - let delay = client.process(Some(dgram), now()).callback(); + let mut delay = client.process(Some(dgram), now()).callback(); + delay += client.process_output(now() + delay).callback(); // TODO: Why is there first a 5ms pacing delay before the PTO? assert_eq!(delay, INITIAL_PTO); assert_eq!(*client.state(), State::WaitInitial); assert_eq!(1, client.stats().dropped_rx); @@ -139,11 +143,13 @@ fn version_negotiation_corrupted() { .dgram() .expect("a datagram") .to_vec(); + _ = client.process_output(now()).dgram().expect("a datagram"); let vn = create_vn(&initial_pkt, &[0x1a1a_1a1a, 0x2a2a_2a2a]); let dgram = datagram(vn[..vn.len() - 1].to_vec()); - let delay = client.process(Some(dgram), now()).callback(); + let mut delay = client.process(Some(dgram), now()).callback(); + delay += client.process_output(now() + delay).callback(); // TODO: Why is there first a 5ms pacing delay before the PTO? assert_eq!(delay, INITIAL_PTO); assert_eq!(*client.state(), State::WaitInitial); assert_eq!(1, client.stats().dropped_rx); @@ -158,11 +164,13 @@ fn version_negotiation_empty() { .dgram() .expect("a datagram") .to_vec(); + _ = client.process_output(now()).dgram().expect("a datagram"); let vn = create_vn(&initial_pkt, &[]); let dgram = datagram(vn); - let delay = client.process(Some(dgram), now()).callback(); + let mut delay = client.process(Some(dgram), now()).callback(); + delay += client.process_output(now() + delay).callback(); // TODO: Why is there first a 5ms pacing delay before the PTO? assert_eq!(delay, INITIAL_PTO); assert_eq!(*client.state(), State::WaitInitial); assert_eq!(1, client.stats().dropped_rx); @@ -198,12 +206,14 @@ fn version_negotiation_bad_cid() { .dgram() .expect("a datagram") .to_vec(); + _ = client.process_output(now()).dgram().expect("a datagram"); initial_pkt[6] ^= 0xc4; let vn = create_vn(&initial_pkt, &[0x1a1a_1a1a, 0x2a2a_2a2a, 0xff00_0001]); let dgram = datagram(vn); - let delay = client.process(Some(dgram), now()).callback(); + let mut delay = client.process(Some(dgram), now()).callback(); + delay += client.process_output(now() + delay).callback(); // TODO: Why is there first a 5ms pacing delay before the PTO? assert_eq!(delay, INITIAL_PTO); assert_eq!(*client.state(), State::WaitInitial); assert_eq!(1, client.stats().dropped_rx); @@ -238,9 +248,11 @@ fn compatible_upgrade_large_initial() { // Client Initial should take 2 packets. // Each should elicit a Version 1 ACK from the server. - let dgram = client.process_output(now()).dgram(); - assert!(dgram.is_some()); - let dgram = server.process(dgram, now()).dgram(); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + assert!(dgram1.is_some() && dgram2.is_some()); + _ = server.process(dgram1, now()).dgram(); + let dgram = server.process(dgram2, now()).dgram(); assert!(dgram.is_some()); // The following uses the Version from *outside* this crate. assertions::assert_version(dgram.as_ref().unwrap(), Version::Version1.wire_version()); @@ -250,8 +262,8 @@ fn compatible_upgrade_large_initial() { assert_eq!(client.version(), Version::Version2); assert_eq!(server.version(), Version::Version2); // Only handshake padding is "dropped". - assert_eq!(client.stats().dropped_rx, 1); - assert_eq!(server.stats().dropped_rx, 2); + assert_eq!(client.stats().dropped_rx, 3); + assert_eq!(server.stats().dropped_rx, 3); } /// A server that supports versions 1 and 2 might prefer version 1 and that's OK. @@ -327,13 +339,15 @@ fn invalid_server_version() { let mut server = new_server(ConnectionParameters::default().versions(Version::Version2, Version::all())); - let dgram = client.process_output(now()).dgram(); - server.process_input(dgram.unwrap(), now()); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + server.process_input(dgram1.unwrap(), now()); + server.process_input(dgram2.unwrap(), now()); - // One packet received. - assert_eq!(server.stats().packets_rx, 1); - // None dropped; the server will have decrypted it successfully. - assert_eq!(server.stats().dropped_rx, 0); + // Three packets received (one is zero padding). + assert_eq!(server.stats().packets_rx, 3); + // One dropped (the zero padding). + assert_eq!(server.stats().dropped_rx, 1); assert_eq!(server.stats().saved_datagrams, 0); // The server effectively hasn't reacted here. match server.state() { @@ -456,10 +470,12 @@ fn compatible_upgrade_0rtt_rejected() { client.enable_resumption(now(), token).unwrap(); // Create a packet with 0-RTT from the client. - let initial = send_something(&mut client, now()); - assertions::assert_version(&initial, Version::Version1.wire_version()); - assertions::assert_coalesced_0rtt(&initial); - server.process_input(initial, now()); + let initial1 = send_something(&mut client, now()); + let initial2 = send_something(&mut client, now()); + assertions::assert_version(&initial1, Version::Version1.wire_version()); + assertions::assert_coalesced_0rtt(&initial2); + server.process_input(initial1, now()); + server.process_input(initial2, now()); assert!(!server .events() .any(|e| matches!(e, ConnectionEvent::NewStream { .. }))); @@ -467,6 +483,8 @@ fn compatible_upgrade_0rtt_rejected() { // Finalize the connection. Don't use connect() because it uses // maybe_authenticate() too liberally and that eats the events we want to check. let dgram = server.process_output(now()).dgram(); // ServerHello flight + let dgram = client.process(dgram, now()).dgram(); + let dgram = server.process(dgram, now()).dgram(); let dgram = client.process(dgram, now()).dgram(); // Client Finished (note: no authentication) let dgram = server.process(dgram, now()).dgram(); // HANDSHAKE_DONE client.process_input(dgram.unwrap(), now()); diff --git a/neqo-transport/src/connection/tests/zerortt.rs b/neqo-transport/src/connection/tests/zerortt.rs index 80929e5f72..ec7fc4ee10 100644 --- a/neqo-transport/src/connection/tests/zerortt.rs +++ b/neqo-transport/src/connection/tests/zerortt.rs @@ -52,18 +52,23 @@ fn zero_rtt_send_recv() { let mut server = resumed_server(&client); // Send ClientHello. - let client_hs = client.process_output(now()); - assert!(client_hs.as_dgram_ref().is_some()); + let client_hs1 = client.process_output(now()); + let client_hs2 = client.process_output(now()); + assert!(client_hs1.as_dgram_ref().is_some() && client_hs2.as_dgram_ref().is_some()); + + // Wait + let delay = client.process_output(now()).callback(); // Now send a 0-RTT packet. let client_stream_id = client.stream_create(StreamType::UniDi).unwrap(); client.stream_send(client_stream_id, &[1, 2, 3]).unwrap(); - let client_0rtt = client.process_output(now()); + let client_0rtt = client.process_output(now() + delay); assert!(client_0rtt.as_dgram_ref().is_some()); // 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.dgram(), now()); + _ = server.process(client_hs1.dgram(), now()); + let server_hs = server.process(client_hs2.dgram(), now()); assert!(server_hs.as_dgram_ref().is_some()); // ServerHello, etc... let all_frames = server.stats().frame_tx.all(); @@ -100,11 +105,14 @@ fn zero_rtt_send_coalesce() { // This should result in a datagram that coalesces Initial and 0-RTT. let client_stream_id = client.stream_create(StreamType::UniDi).unwrap(); client.stream_send(client_stream_id, &[1, 2, 3]).unwrap(); + let client_init = client.process_output(now()); + assert!(client_init.as_dgram_ref().is_some()); let client_0rtt = client.process_output(now()); assert!(client_0rtt.as_dgram_ref().is_some()); assertions::assert_coalesced_0rtt(&client_0rtt.as_dgram_ref().unwrap()[..]); + _ = server.process(client_init.dgram(), now()); let server_hs = server.process(client_0rtt.dgram(), now()); assert!(server_hs.as_dgram_ref().is_some()); // Should produce ServerHello etc... @@ -152,27 +160,31 @@ fn zero_rtt_send_reject() { .server_enable_0rtt(&ar, AllowZeroRtt {}) .expect("enable 0-RTT"); + // Write some data on the client. + let stream_id = client.stream_create(StreamType::UniDi).unwrap(); + client.stream_send(stream_id, MESSAGE).unwrap(); + // Send ClientHello. let client_hs = client.process_output(now()); assert!(client_hs.as_dgram_ref().is_some()); - // Write some data on the client. - let stream_id = client.stream_create(StreamType::UniDi).unwrap(); - client.stream_send(stream_id, MESSAGE).unwrap(); let client_0rtt = client.process_output(now()); assert!(client_0rtt.as_dgram_ref().is_some()); let server_hs = server.process(client_hs.dgram(), now()); assert!(server_hs.as_dgram_ref().is_some()); // Should produce ServerHello etc... - let server_ignored = server.process(client_0rtt.dgram(), now()); - assert!(server_ignored.as_dgram_ref().is_none()); + let server_hs2 = server.process(client_0rtt.dgram(), now()); // The server shouldn't receive that 0-RTT data. let recvd_stream_evt = |e| matches!(e, ConnectionEvent::NewStream { .. }); assert!(!server.events().any(recvd_stream_evt)); + _ = client.process(server_hs.dgram(), now()); + let dgram = client.process(server_hs2.dgram(), now()).dgram(); + let dgram = server.process(dgram, now()).dgram(); + // Client should get a rejection. - let client_fin = client.process(server_hs.dgram(), now()); + let client_fin = client.process(dgram, now()); let recvd_0rtt_reject = |e| e == ConnectionEvent::ZeroRttRejected; assert!(client.events().any(recvd_0rtt_reject)); @@ -227,15 +239,19 @@ fn zero_rtt_update_flow_control() { ); // Stream limits should be low for 0-RTT. - let client_hs = client.process_output(now()).dgram(); + let client_hs1 = client.process_output(now()).dgram(); + let client_hs2 = client.process_output(now()).dgram(); let uni_stream = client.stream_create(StreamType::UniDi).unwrap(); assert!(!client.stream_send_atomic(uni_stream, MESSAGE).unwrap()); let bidi_stream = client.stream_create(StreamType::BiDi).unwrap(); assert!(!client.stream_send_atomic(bidi_stream, MESSAGE).unwrap()); // Now get the server transport parameters. - let server_hs = server.process(client_hs, now()).dgram(); - client.process_input(server_hs.unwrap(), now()); + _ = server.process(client_hs1, now()).dgram(); + let server_hs = server.process(client_hs2, now()).dgram(); + let client_hs3 = client.process(server_hs, now()).dgram(); + let server_hs2 = server.process(client_hs3, now()).dgram(); + client.process_input(server_hs2.unwrap(), now()); // The streams should report a writeable event. let mut uni_stream_event = false; @@ -288,7 +304,9 @@ fn zero_rtt_loss_accepted() { client.stream_send(client_stream_id, &[1, 2, 3]).unwrap(); let mut ci = client.process_output(now); assert!(ci.as_dgram_ref().is_some()); - assertions::assert_coalesced_0rtt(&ci.as_dgram_ref().unwrap()[..]); + let mut c0rtt = client.process_output(now); + assert!(c0rtt.as_dgram_ref().is_some()); + assertions::assert_coalesced_0rtt(&c0rtt.as_dgram_ref().unwrap()[..]); // Drop CI/0-RTT a number of times qdebug!("Drop CI/0-RTT {i} extra times"); @@ -296,10 +314,13 @@ fn zero_rtt_loss_accepted() { now += client.process_output(now).callback(); ci = client.process_output(now); assert!(ci.as_dgram_ref().is_some()); + c0rtt = client.process_output(now); + assert!(c0rtt.as_dgram_ref().is_some()); } // Process CI/0-RTT - let si = server.process(ci.dgram(), now); + _ = server.process(ci.dgram(), now); + let si = server.process(c0rtt.dgram(), now); assert!(si.as_dgram_ref().is_some()); let server_stream_id = server diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 120c270d10..757e7c8918 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -37,7 +37,7 @@ use crate::{ tparams::{TpZeroRttChecker, TransportParameters, TransportParametersHandler}, tracking::PacketNumberSpace, version::Version, - Error, Res, + ConnectionParameters, Error, Res, }; const MAX_AUTH_TAG: usize = 32; @@ -69,6 +69,7 @@ type TpHandler = Rc>; impl Crypto { pub fn new( version: Version, + conn_params: &ConnectionParameters, mut agent: Agent, protocols: Vec, tphandler: TpHandler, @@ -79,17 +80,25 @@ impl Crypto { TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, ])?; - agent.set_groups(&[ - TLS_GRP_KEM_MLKEM768X25519, - TLS_GRP_EC_X25519, - TLS_GRP_EC_SECP256R1, - TLS_GRP_EC_SECP384R1, - TLS_GRP_EC_SECP521R1, - ])?; + agent.set_groups(if conn_params.mlkem_enabled() { + &[ + TLS_GRP_KEM_MLKEM768X25519, + TLS_GRP_EC_X25519, + TLS_GRP_EC_SECP256R1, + TLS_GRP_EC_SECP384R1, + TLS_GRP_EC_SECP521R1, + ] + } else { + &[ + TLS_GRP_EC_X25519, + TLS_GRP_EC_SECP256R1, + TLS_GRP_EC_SECP384R1, + TLS_GRP_EC_SECP521R1, + ] + })?; if let Agent::Client(c) = &mut agent { - // Configure clients to send MLKEM768X25519, X25519 and P256 to - // reduce the rate of HRRs. - c.send_additional_key_shares(2)?; + // Configure clients to send additional key shares to reduce the rate of HRRs. + c.send_additional_key_shares(1)?; // Always enable 0-RTT on the client, but the server needs // more configuration passed to server_enable_0rtt. @@ -1421,6 +1430,10 @@ impl CryptoStreams { } } + pub fn is_empty(&mut self, space: PacketNumberSpace) -> bool { + self.get_mut(space).map_or(true, |cs| cs.tx.is_empty()) + } + const fn get(&self, space: PacketNumberSpace) -> Option<&CryptoStream> { let (initial, hs, app) = match self { Self::Initial { @@ -1518,7 +1531,7 @@ impl CryptoStreams { // reordering it can cause the generation of discontiguous chunks. We need to loop here to // make sure we fill the packets, and not send superfluous packets that are mostly padding. while let Some((offset, data)) = cs.tx.next_bytes() { - let written = if sni_slicing && offset == 0 { + let written = if sni_slicing { if let Some(sni) = find_sni(data) { // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. let mid = sni.start + (sni.end - sni.start) / 2; @@ -1529,8 +1542,15 @@ impl CryptoStreams { // zero-padded, which apparently helps with traversal of // some middleboxes. This is ignores CRYPTO frame headers so // undercounts a little, but that's fine. + // + // TODO: It would be slightly better if we managed to pad + // the first Initial with the minimum amount of zeros to + // help middlebox traversal, so the second packets had more + // space for coalescing. let packets_needed = data.len() / builder.limit() + 1; - builder.set_limit(data.len() / packets_needed); + if packets_needed > 1 { + builder.set_limit(data.len() / packets_needed); + } [ write_chunk(offset + mid as u64, right, builder), write_chunk(offset, left, builder), diff --git a/neqo-transport/src/send_stream.rs b/neqo-transport/src/send_stream.rs index 915997737a..347edc9d65 100644 --- a/neqo-transport/src/send_stream.rs +++ b/neqo-transport/src/send_stream.rs @@ -498,6 +498,11 @@ impl TxBuffer { can_buffer } + pub fn is_empty(&mut self) -> bool { + let (start, _) = self.ranges.first_unmarked_range(); + start == self.retired() + u64::try_from(self.buffered()).unwrap() + } + #[allow(clippy::missing_panics_doc)] // These are not possible. pub fn next_bytes(&mut self) -> Option<(u64, &[u8])> { let (start, maybe_len) = self.ranges.first_unmarked_range(); diff --git a/neqo-transport/tests/common/mod.rs b/neqo-transport/tests/common/mod.rs index f8b9c98d6a..c43800e662 100644 --- a/neqo-transport/tests/common/mod.rs +++ b/neqo-transport/tests/common/mod.rs @@ -58,15 +58,19 @@ pub fn connect(client: &mut Connection, server: &mut Server) -> ConnectionRef { server.set_validation(ValidateAddress::Never); assert_eq!(*client.state(), State::Init); - let out = client.process_output(now()); // ClientHello - assert!(out.as_dgram_ref().is_some()); - let out = server.process(out.dgram(), now()); // ServerHello... + let out1 = client.process_output(now()); // ClientHello + let out2 = client.process_output(now()); // ClientHello + assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + _ = server.process(out1.dgram(), now()); // ACK + let out = server.process(out2.dgram(), now()); // ServerHello... assert!(out.as_dgram_ref().is_some()); // Ingest the server Certificate. let out = client.process(out.dgram(), now()); assert!(out.as_dgram_ref().is_some()); // This should just be an ACK. let out = server.process(out.dgram(), now()); + let out = client.process(out.dgram(), now()); + let out = server.process(out.dgram(), now()); assert!(out.as_dgram_ref().is_none()); // So the server should have nothing to say. // Now mark the server as authenticated. diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index 12b4829732..6ef70f7ff0 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -29,10 +29,14 @@ fn truncate_long_packet() { let mut client = default_client(); let mut server = default_server(); - let out = client.process_output(now()); + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); + assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + _ = server.process(out1.dgram(), now()); + let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); + let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); - assert!(out.as_dgram_ref().is_some()); // This will truncate the Handshake packet from the server. let dupe = out.as_dgram_ref().unwrap().clone(); @@ -66,8 +70,11 @@ fn reorder_server_initial() { // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. const ACK_FRAME: &[u8] = &[0x03, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00]; + // This test needs to decrypt the CI, so turn off MLKEM. let mut client = new_client( - ConnectionParameters::default().versions(Version::Version1, vec![Version::Version1]), + ConnectionParameters::default() + .versions(Version::Version1, vec![Version::Version1]) + .mlkem(false), ); let mut server = default_server(); diff --git a/neqo-transport/tests/retry.rs b/neqo-transport/tests/retry.rs index 99abfd96b9..b7470bb3c3 100644 --- a/neqo-transport/tests/retry.rs +++ b/neqo-transport/tests/retry.rs @@ -17,7 +17,8 @@ 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, MIN_INITIAL_PACKET_SIZE, + server::ValidateAddress, CloseReason, ConnectionParameters, Error, State, StreamType, + MIN_INITIAL_PACKET_SIZE, }; use test_fixture::{ assertions, datagram, default_client, @@ -34,13 +35,19 @@ fn retry_basic() { server.set_validation(ValidateAddress::Always); let mut client = default_client(); - let dgram = client.process_output(now()).dgram(); // Initial - assert!(dgram.is_some()); - let dgram = server.process(dgram, now()).dgram().unwrap(); // Retry + let dgram1 = client.process_output(now()).dgram(); // Initial + let dgram2 = client.process_output(now()).dgram(); // Initial + assert!(dgram1.is_some() && dgram2.is_some()); + _ = server.process(dgram1, now()).dgram().unwrap(); // Retry + let dgram = server.process(dgram2, now()).dgram().unwrap(); // Retry assertions::assert_retry(&dgram); - let dgram = client.process(Some(dgram), now()).dgram(); // Initial w/token - assert!(dgram.is_some()); + let dgram1 = client.process(Some(dgram), now()).dgram(); // Initial w/token + let dgram2 = client.process_output(now()).dgram(); // Initial + assert!(dgram1.is_some() && dgram2.is_some()); + _ = server.process(dgram1, now()).dgram().unwrap(); + let dgram = server.process(dgram2, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); // Initial, HS assert!(dgram.is_some()); drop(client.process(dgram, now()).dgram()); // Ingest, drop any ACK. @@ -108,21 +115,27 @@ fn retry_0rtt() { let client_stream = client.stream_create(StreamType::UniDi).unwrap(); client.stream_send(client_stream, &[1, 2, 3]).unwrap(); - let dgram = client.process_output(now()).dgram(); // Initial w/0-RTT - assert!(dgram.is_some()); - assertions::assert_coalesced_0rtt(dgram.as_ref().unwrap()); - let dgram = server.process(dgram, now()).dgram(); // Retry + let dgram1 = client.process_output(now()).dgram(); // Initial + let dgram2 = client.process_output(now()).dgram(); // Initial w/0-RTT + assert!(dgram1.is_some() && dgram2.is_some()); + assertions::assert_coalesced_0rtt(dgram2.as_ref().unwrap()); + _ = server.process(dgram1, now()).dgram(); // Retry + let dgram = server.process(dgram2, now()).dgram(); // Retry assert!(dgram.is_some()); assertions::assert_retry(dgram.as_ref().unwrap()); // After retry, there should be a token and still coalesced 0-RTT. - let dgram = client.process(dgram, now()).dgram(); - assert!(dgram.is_some()); - assertions::assert_coalesced_0rtt(dgram.as_ref().unwrap()); + let dgram1 = client.process(dgram, now()).dgram(); // Initial + let dgram2 = client.process_output(now()).dgram(); // Initial w/0-RTT + assert!(dgram1.is_some() && dgram2.is_some()); + assertions::assert_coalesced_0rtt(dgram2.as_ref().unwrap()); - let dgram = server.process(dgram, now()).dgram(); // Initial, HS + _ = server.process(dgram1, now()).dgram(); // ACK + let dgram = server.process(dgram2, now()).dgram(); // Initial, HS assert!(dgram.is_some()); let dgram = client.process(dgram, now()).dgram(); + let dgram = server.process(dgram, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); // Note: the client doesn't need to authenticate the server here // as there is no certificate; authentication is based on the ticket. assert!(dgram.is_some()); @@ -212,9 +225,11 @@ fn retry_after_initial() { retry_server.set_validation(ValidateAddress::Always); let mut client = default_client(); - let cinit = client.process_output(now()).dgram(); // Initial - assert!(cinit.is_some()); - let server_flight = server.process(cinit.clone(), now()).dgram(); // Initial + let cinit1 = client.process_output(now()).dgram(); // Initial + let cinit2 = client.process_output(now()).dgram(); // Initial + assert!(cinit1.is_some() && cinit2.is_some()); + _ = server.process(cinit1.clone(), now()).dgram(); // Initial + let server_flight = server.process(cinit2, now()).dgram(); // Initial assert!(server_flight.is_some()); // We need to have the client just process the Initial. @@ -223,7 +238,7 @@ fn retry_after_initial() { assert!(dgram.is_some()); assert!(*client.state() != State::Connected); - let retry = retry_server.process(cinit, now()).dgram(); // Retry! + let retry = retry_server.process(cinit1, now()).dgram(); // Retry! assert!(retry.is_some()); assertions::assert_retry(retry.as_ref().unwrap()); @@ -233,6 +248,8 @@ fn retry_after_initial() { // Either way, the client should still be able to process the server flight and connect. let dgram = client.process(server_flight, now()).dgram(); + let dgram = server.process(dgram, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); assert!(dgram.is_some()); // Drop this one. assert!(test_fixture::maybe_authenticate(&mut client)); let dgram = client.process_output(now()).dgram(); @@ -250,9 +267,11 @@ fn retry_bad_integrity() { server.set_validation(ValidateAddress::Always); let mut client = default_client(); - let dgram = client.process_output(now()).dgram(); // Initial - assert!(dgram.is_some()); - let dgram = server.process(dgram, now()).dgram(); // Retry + let dgram1 = client.process_output(now()).dgram(); // Initial + let dgram2 = client.process_output(now()).dgram(); // Initial + assert!(dgram1.is_some() && dgram2.is_some()); + _ = server.process(dgram1, now()).dgram(); // Retry + let dgram = server.process(dgram2, now()).dgram(); // Retry assert!(dgram.is_some()); let retry = &dgram.as_ref().unwrap(); @@ -298,17 +317,20 @@ fn retry_after_pto() { server.set_validation(ValidateAddress::Always); let mut now = now(); - let ci = client.process_output(now).dgram(); - assert!(ci.is_some()); // sit on this for a bit.RefCell + let ci1 = client.process_output(now).dgram(); + let ci2 = client.process_output(now).dgram(); + assert!(ci1.is_some() && ci2.is_some()); // sit on this for a bit // Let PTO fire on the client and then let it exhaust its PTO packets. now += Duration::from_secs(1); let pto = client.process_output(now).dgram(); assert!(pto.unwrap().len() >= MIN_INITIAL_PACKET_SIZE); + _ = client.process_output(now).dgram(); let cb = client.process_output(now).callback(); assert_ne!(cb, Duration::new(0, 0)); - let retry = server.process(ci, now).dgram(); + _ = server.process(ci1, now).dgram(); + let retry = server.process(ci2, now).dgram(); assertions::assert_retry(retry.as_ref().unwrap()); let ci2 = client.process(retry, now).dgram(); @@ -321,15 +343,19 @@ fn vn_after_retry() { server.set_validation(ValidateAddress::Always); let mut client = default_client(); - let dgram = client.process_output(now()).dgram(); // Initial - assert!(dgram.is_some()); - let dgram = server.process(dgram, now()).dgram(); // Retry + let dgram1 = client.process_output(now()).dgram(); // Initial + let dgram2 = client.process_output(now()).dgram(); // Initial + assert!(dgram1.is_some() && dgram2.is_some()); + _ = server.process(dgram1, now()).dgram(); // Retry + let dgram = server.process(dgram2, now()).dgram(); // Retry assert!(dgram.is_some()); assertions::assert_retry(dgram.as_ref().unwrap()); let dgram = client.process(dgram, now()).dgram(); // Initial w/token assert!(dgram.is_some()); + let dgram = server.process(dgram, now()).dgram(); + _ = client.process(dgram, now()).dgram(); let mut encoder = Encoder::default(); encoder.encode_byte(0x80); @@ -357,7 +383,8 @@ fn vn_after_retry() { // long enough connection ID. #[test] fn mitm_retry() { - let mut client = default_client(); + // This test decrypts packets and hence does not work with MLKEM enabled. + let mut client = test_fixture::new_client(ConnectionParameters::default().mlkem(false)); let mut retry_server = default_server(); retry_server.set_validation(ValidateAddress::Always); let mut server = default_server(); diff --git a/neqo-transport/tests/server.rs b/neqo-transport/tests/server.rs index c0a10b9afb..0f641c3080 100644 --- a/neqo-transport/tests/server.rs +++ b/neqo-transport/tests/server.rs @@ -311,6 +311,7 @@ fn zero_rtt() { }; // Now generate a bunch of 0-RTT packets... + let c0 = client_send(); let c1 = client_send(); assertions::assert_coalesced_0rtt(&c1); let c2 = client_send(); @@ -322,9 +323,10 @@ fn zero_rtt() { assert!(server.active_connections().is_empty()); // Now handshake and let another 0-RTT packet in. + _ = server.process(Some(c0), now); let shs = server.process(Some(c1), now); drop(server.process(Some(c3), now)); - // The server will have received two STREAM frames now if it processed both packets. + // The server will have received three STREAM frames now if it processed both packets. // `ActiveConnectionRef` `Hash` implementation doesn’t access any of the interior mutable types. #[allow(clippy::mutable_key_type)] let active = server.active_connections(); @@ -338,7 +340,7 @@ fn zero_rtt() { .stats() .frame_rx .stream, - 2 + 3 ); // Complete the handshake. As the client was pacing 0-RTT packets, extend the time @@ -362,7 +364,7 @@ fn zero_rtt() { .stats() .frame_rx .stream, - 2 + 4 ); } @@ -378,15 +380,19 @@ fn new_token_0rtt() { let client_stream = client.stream_create(StreamType::UniDi).unwrap(); client.stream_send(client_stream, &[1, 2, 3]).unwrap(); - let out = client.process_output(now()); // Initial w/0-RTT - assert!(out.as_dgram_ref().is_some()); - assertions::assert_initial(out.as_dgram_ref().unwrap(), true); - assertions::assert_coalesced_0rtt(out.as_dgram_ref().unwrap()); - let out = server.process(out.dgram(), now()); // Initial + let out1 = client.process_output(now()); + let out2 = client.process_output(now()); // Initial w/0-RTT + assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + assertions::assert_initial(out1.as_dgram_ref().unwrap(), true); + assertions::assert_coalesced_0rtt(out2.as_dgram_ref().unwrap()); + _ = server.process(out1.dgram(), now()); // Initial + let out = server.process(out2.dgram(), now()); // Initial assert!(out.as_dgram_ref().is_some()); assertions::assert_initial(out.as_dgram_ref().unwrap(), false); let dgram = client.process(out.as_dgram_ref().cloned(), now()); + let dgram = server.process(dgram.as_dgram_ref().cloned(), now()); + let dgram = client.process(dgram.as_dgram_ref().cloned(), now()); // Note: the client doesn't need to authenticate the server here // as there is no certificate; authentication is based on the ticket. assert!(out.as_dgram_ref().is_some()); @@ -421,7 +427,8 @@ fn new_token_different_port() { #[test] fn bad_client_initial() { - let mut client = default_client(); + // This test needs to decrypt the CI, so turn off MLKEM. + let mut client = new_client(ConnectionParameters::default().mlkem(false)); let mut server = default_server(); let dgram = client.process_output(now()).dgram().expect("a datagram"); @@ -567,6 +574,7 @@ fn version_negotiation_ignored() { // Any packet will do, but let's make something that looks real. let dgram = client.process_output(now()).dgram().expect("a datagram"); + _ = client.process_output(now()).dgram().expect("a datagram"); let mut input = dgram.to_vec(); input[1] ^= 0x12; let damaged = Datagram::new( @@ -649,18 +657,24 @@ fn version_negotiation_and_compatible() { // Run the full exchange so that we can observe the versions in use. // Version Negotiation - let dgram = client.process_output(now()).dgram(); - assert!(dgram.is_some()); - assertions::assert_version(dgram.as_ref().unwrap(), ORIG_VERSION.wire_version()); - let dgram = server.process(dgram, now()).dgram(); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + assert!(dgram1.is_some() && dgram2.is_some()); + assertions::assert_version(dgram1.as_ref().unwrap(), ORIG_VERSION.wire_version()); + _ = server.process(dgram1, now()).dgram(); + let dgram = server.process(dgram2, now()).dgram(); assertions::assert_vn(dgram.as_ref().unwrap()); client.process_input(dgram.unwrap(), now()); - let dgram = client.process_output(now()).dgram(); // ClientHello - assertions::assert_version(dgram.as_ref().unwrap(), VN_VERSION.wire_version()); - let dgram = server.process(dgram, now()).dgram(); // ServerHello... + let dgram1 = client.process_output(now()).dgram(); // ClientHello + let dgram2 = client.process_output(now()).dgram(); // ClientHello + assertions::assert_version(dgram1.as_ref().unwrap(), VN_VERSION.wire_version()); + _ = server.process(dgram1, now()).dgram(); // ServerHello... + let dgram = server.process(dgram2, now()).dgram(); // ServerHello... assertions::assert_version(dgram.as_ref().unwrap(), COMPAT_VERSION.wire_version()); - client.process_input(dgram.unwrap(), now()); + let dgram = client.process(dgram, now()).dgram(); + let dgram = server.process(dgram, now()).dgram(); + _ = client.process(dgram, now()).dgram(); client.authenticated(AuthenticationStatus::Ok, now()); let dgram = client.process_output(now()).dgram(); @@ -825,7 +839,11 @@ fn max_streams_after_0rtt_rejection() { let mut client = default_client(); client.enable_resumption(now(), &token).unwrap(); _ = client.stream_create(StreamType::BiDi).unwrap(); - let dgram = client.process_output(now()).dgram(); + let dgram1 = client.process_output(now()).dgram(); + let dgram2 = client.process_output(now()).dgram(); + _ = server.process(dgram1, now()).dgram(); + let dgram = server.process(dgram2, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); let dgram = client.process(dgram, now()).dgram(); assert!(dgram.is_some()); // We're far enough along to complete the test now. diff --git a/test-fixture/src/sim/connection.rs b/test-fixture/src/sim/connection.rs index 74241963c8..a7094c2531 100644 --- a/test-fixture/src/sim/connection.rs +++ b/test-fixture/src/sim/connection.rs @@ -81,7 +81,8 @@ impl ConnectionNode { pub fn default_client(goals: impl IntoIterator>) -> Self { Self::new_client( - ConnectionParameters::default().pmtud(true), + // Simulator logic does not work with multi-packet MLKEM crypto flights. + ConnectionParameters::default().pmtud(true).mlkem(false), boxed![ReachState::new(State::Confirmed)], goals, ) @@ -89,7 +90,8 @@ impl ConnectionNode { pub fn default_server(goals: impl IntoIterator>) -> Self { Self::new_server( - ConnectionParameters::default().pmtud(true), + // Simulator logic does not work with multi-packet MLKEM crypto flights. + ConnectionParameters::default().pmtud(true).mlkem(false), boxed![ReachState::new(State::Confirmed)], goals, ) From 16817581d6da6a54e9b854edf77992dd74380fa4 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Tue, 28 Jan 2025 16:45:10 +0200 Subject: [PATCH 04/17] Finalize --- neqo-http3/src/connection_client.rs | 30 ++-- .../tests/webtransport/mod.rs | 4 +- neqo-http3/src/frames/tests/mod.rs | 4 +- neqo-http3/tests/httpconn.rs | 12 +- neqo-http3/tests/priority.rs | 4 +- neqo-http3/tests/webtransport.rs | 4 +- neqo-transport/src/connection/mod.rs | 3 +- neqo-transport/src/connection/tests/close.rs | 6 +- .../src/connection/tests/handshake.rs | 135 +++++++++--------- neqo-transport/src/connection/tests/idle.rs | 4 +- neqo-transport/src/connection/tests/keys.rs | 18 +-- .../src/connection/tests/migration.rs | 4 +- .../src/connection/tests/priority.rs | 4 +- .../src/connection/tests/recovery.rs | 59 +++++--- neqo-transport/src/connection/tests/stream.rs | 14 +- neqo-transport/src/connection/tests/vn.rs | 18 +-- .../src/connection/tests/zerortt.rs | 14 +- neqo-transport/tests/common/mod.rs | 6 +- neqo-transport/tests/connection.rs | 6 +- neqo-transport/tests/retry.rs | 58 ++++---- neqo-transport/tests/server.rs | 28 ++-- 21 files changed, 226 insertions(+), 209 deletions(-) diff --git a/neqo-http3/src/connection_client.rs b/neqo-http3/src/connection_client.rs index 8056887d46..61696ca0fb 100644 --- a/neqo-http3/src/connection_client.rs +++ b/neqo-http3/src/connection_client.rs @@ -1624,21 +1624,19 @@ mod tests { fn handshake_only(client: &mut Http3Client, server: &mut TestServer) -> Output { assert_eq!(client.state(), Http3State::Initializing); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::Initializing); assert_eq!(*server.conn.state(), State::Init); - _ = server.conn.process(out1.dgram(), now()); + server.conn.process_input(out.dgram().unwrap(), now()); let out = server.conn.process(out2.dgram(), now()); assert_eq!(*server.conn.state(), State::Handshaking); let out = client.process(out.dgram(), now()); let out = server.conn.process(out.dgram(), now()); - let out = client.process(out.dgram(), now()); let out = server.conn.process(out.dgram(), now()); - assert!(out.as_dgram_ref().is_none()); let authentication_needed = |e| matches!(e, Http3ClientEvent::AuthenticationNeeded); @@ -4162,12 +4160,13 @@ mod tests { #[test] fn zero_rtt_negotiated() { let (mut client, mut server) = start_with_0rtt(); - let out1 = client.process_output(now()); + + let out = client.process_output(now()); let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::ZeroRtt); assert_eq!(*server.conn.state(), State::Init); - _ = server.conn.process(out1.dgram(), now()); + server.conn.process_input(out.dgram().unwrap(), now()); let out = server.conn.process(out2.dgram(), now()); // Check that control and qpack streams are received and a @@ -4184,7 +4183,6 @@ mod tests { let out = client.process(out.dgram(), now()); let out = server.conn.process(out.dgram(), now()); let out = client.process(out.dgram(), now()); - assert_eq!(client.state(), Http3State::Connected); drop(server.conn.process(out.dgram(), now())); @@ -4202,12 +4200,12 @@ mod tests { make_request(&mut client, true, &[Header::new("myheaders", "myvalue")]); assert_eq!(request_stream_id, 0); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::ZeroRtt); assert_eq!(*server.conn.state(), State::Init); - _ = server.conn.process(out1.dgram(), now()); + server.conn.process_input(out.dgram().unwrap(), now()); let out = server.conn.process(out2.dgram(), now()); // Check that control and qpack streams are received and a @@ -4283,18 +4281,18 @@ mod tests { let zerortt_event = |e| matches!(e, Http3ClientEvent::StateChange(Http3State::ZeroRtt)); assert!(client.events().any(zerortt_event)); - // Create a request - let request_stream_id = make_request(&mut client, false, &[]); - assert_eq!(request_stream_id, 0); - // Send ClientHello. let client_hs = client.process_output(now()); assert!(client_hs.as_dgram_ref().is_some()); + // Create a request + let request_stream_id = make_request(&mut client, false, &[]); + assert_eq!(request_stream_id, 0); + let client_0rtt = client.process_output(now()); assert!(client_0rtt.as_dgram_ref().is_some()); - _ = server.process(client_hs.dgram(), now()); + server.process_input(client_hs.dgram().unwrap(), now()); let server_hs = server.process(client_0rtt.dgram(), now()); assert!(server_hs.as_dgram_ref().is_some()); // Should produce ServerHello etc... @@ -4344,12 +4342,12 @@ mod tests { .enable_resumption(now(), &token) .expect("Set resumption token"); assert_eq!(client.state(), Http3State::ZeroRtt); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::ZeroRtt); assert_eq!(*server.conn.state(), State::Init); - _ = server.conn.process(out1.dgram(), now()); + server.conn.process_input(out.dgram().unwrap(), now()); let out = server.conn.process(out2.dgram(), now()); // Check that control and qpack streams and a SETTINGS frame are received. 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 cee26e4876..a8a3be4521 100644 --- a/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs +++ b/neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs @@ -81,11 +81,11 @@ fn exchange_packets(client: &mut Http3Client, server: &mut Http3Server) { // Perform only QUIC transport handshake. fn connect_with(client: &mut Http3Client, server: &mut Http3Server) { assert_eq!(client.state(), Http3State::Initializing); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::Initializing); - _ = server.process(out1.dgram(), now()); + _ = server.process(out.dgram(), now()); let out = server.process(out2.dgram(), now()); let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); diff --git a/neqo-http3/src/frames/tests/mod.rs b/neqo-http3/src/frames/tests/mod.rs index 58fa022e8f..b6238190d3 100644 --- a/neqo-http3/src/frames/tests/mod.rs +++ b/neqo-http3/src/frames/tests/mod.rs @@ -20,9 +20,9 @@ pub fn enc_dec>(d: &Encoder, st: &str, remaining: usize) -> T let mut conn_c = default_client(); let mut conn_s = default_server(); - let out1 = conn_c.process_output(now()); + let out = conn_c.process_output(now()); let out2 = conn_c.process_output(now()); - _ = conn_s.process(out1.dgram(), now()); + _ = conn_s.process(out.dgram(), now()); let out = conn_s.process(out2.dgram(), now()); let out = conn_c.process(out.dgram(), now()); let out = conn_s.process(out.dgram(), now()); diff --git a/neqo-http3/tests/httpconn.rs b/neqo-http3/tests/httpconn.rs index 18f6ede120..e267627908 100644 --- a/neqo-http3/tests/httpconn.rs +++ b/neqo-http3/tests/httpconn.rs @@ -91,9 +91,9 @@ fn process_client_events(conn: &mut Http3Client) { fn connect_peers(hconn_c: &mut Http3Client, hconn_s: &mut Http3Server) -> Option { assert_eq!(hconn_c.state(), Http3State::Initializing); - let out1 = hconn_c.process_output(now()); // Initial + let out = hconn_c.process_output(now()); // Initial let out2 = hconn_c.process_output(now()); // Initial - _ = hconn_s.process(out1.dgram(), now()); // ACK + _ = hconn_s.process(out.dgram(), now()); // ACK let out = hconn_s.process(out2.dgram(), now()); // Initial + Handshake let out = hconn_c.process(out.dgram(), now()); let out = hconn_s.process(out.dgram(), now()); @@ -122,10 +122,10 @@ fn connect_peers_with_network_propagation_delay( let net_delay = Duration::from_millis(net_delay); assert_eq!(hconn_c.state(), Http3State::Initializing); let mut now = now(); - let out1 = hconn_c.process_output(now); // Initial + let out = hconn_c.process_output(now); // Initial let out2 = hconn_c.process_output(now); // Initial now += net_delay; - _ = hconn_s.process(out1.dgram(), now); // ACK + _ = hconn_s.process(out.dgram(), now); // ACK let out = hconn_s.process(out2.dgram(), now); now += net_delay; let out = hconn_c.process(out.dgram(), now); @@ -455,9 +455,9 @@ fn zerortt() { .unwrap(); hconn_c.stream_close_send(req).unwrap(); - let out1 = hconn_c.process(dgram, now()); + let out = hconn_c.process(dgram, now()); let out2 = hconn_c.process_output(now()); - _ = hconn_s.process(out1.dgram(), now()); + _ = hconn_s.process(out.dgram(), now()); let out = hconn_s.process(out2.dgram(), now()); let out = hconn_c.process(out.dgram(), now()); diff --git a/neqo-http3/tests/priority.rs b/neqo-http3/tests/priority.rs index 35bf1c453b..6a27b2bb73 100644 --- a/neqo-http3/tests/priority.rs +++ b/neqo-http3/tests/priority.rs @@ -28,11 +28,11 @@ fn exchange_packets(client: &mut Http3Client, server: &mut Http3Server) { // Perform only QUIC transport handshake. fn connect_with(client: &mut Http3Client, server: &mut Http3Server) { assert_eq!(client.state(), Http3State::Initializing); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::Initializing); - _ = server.process(out1.dgram(), now()); + _ = server.process(out.dgram(), now()); let out = server.process(out2.dgram(), now()); let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); diff --git a/neqo-http3/tests/webtransport.rs b/neqo-http3/tests/webtransport.rs index 9018c8f27f..9ae6cbee7e 100644 --- a/neqo-http3/tests/webtransport.rs +++ b/neqo-http3/tests/webtransport.rs @@ -41,11 +41,11 @@ fn connect() -> (Http3Client, Http3Server) { ) .expect("create a server"); assert_eq!(client.state(), Http3State::Initializing); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); assert_eq!(client.state(), Http3State::Initializing); - _ = server.process(out1.dgram(), now()); + _ = server.process(out.dgram(), now()); let out = server.process(out2.dgram(), now()); let out = client.process(out.dgram(), now()); let out = server.process(out.dgram(), now()); diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 52f311c962..c942a4485a 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1449,7 +1449,6 @@ impl Connection { { self.crypto.resend_unacked(PacketNumberSpace::Initial); self.resend_0rtt(now); - qdebug!("XXX resend done"); } } (PacketType::VersionNegotiation | PacketType::Retry | PacketType::OtherVersion, ..) => { @@ -2496,7 +2495,7 @@ impl Connection { if padded { needs_padding = false; self.loss_recovery.on_packet_sent(path, sent, now); - } else if pt == PacketType::Initial { + } else if pt == PacketType::Initial && (self.role == Role::Client || ack_eliciting) { // Packets containing Initial packets might need padding, and we want to // track that padding along with the Initial packet. So defer tracking. initial_sent = Some(sent); diff --git a/neqo-transport/src/connection/tests/close.rs b/neqo-transport/src/connection/tests/close.rs index a9883b987b..4f7ef59171 100644 --- a/neqo-transport/src/connection/tests/close.rs +++ b/neqo-transport/src/connection/tests/close.rs @@ -109,10 +109,10 @@ fn bad_tls_version() { .unwrap(); let mut server = default_server(); - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - assert!(dgram1.is_some() && dgram2.is_some()); - _ = server.process(dgram1, now()).dgram(); + assert!(dgram.is_some() && dgram2.is_some()); + server.process_input(dgram.unwrap(), now()); let dgram = server.process(dgram2, now()).dgram(); assert_eq!( *server.state(), diff --git a/neqo-transport/src/connection/tests/handshake.rs b/neqo-transport/src/connection/tests/handshake.rs index b20e726ab2..484c11cd0f 100644 --- a/neqo-transport/src/connection/tests/handshake.rs +++ b/neqo-transport/src/connection/tests/handshake.rs @@ -46,15 +46,15 @@ const ECH_PUBLIC_NAME: &str = "public.example"; fn full_handshake(pmtud: bool) { qdebug!("---- client: generate CH"); let mut client = new_client(ConnectionParameters::default().pmtud(pmtud)); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); - assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); - assert_eq!(out1.as_dgram_ref().unwrap().len(), client.plpmtu()); + assert!(out.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + assert_eq!(out.as_dgram_ref().unwrap().len(), client.plpmtu()); assert_eq!(out2.as_dgram_ref().unwrap().len(), client.plpmtu()); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = new_server(ConnectionParameters::default().pmtud(pmtud)); - _ = server.process(out1.dgram(), now()); + server.process_input(out.dgram().unwrap(), now()); let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); assert_eq!(out.as_dgram_ref().unwrap().len(), server.plpmtu()); @@ -107,19 +107,20 @@ fn handshake_pmtud() { fn handshake_failed_authentication() { qdebug!("---- client: generate CH"); let mut client = default_client(); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); - assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + assert!(out.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = default_server(); - _ = server.process(out1.dgram(), now()); + server.process_input(out.dgram().unwrap(), now()); let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); qdebug!("---- client: cert verification"); let out = client.process(out.dgram(), now()); assert!(out.as_dgram_ref().is_some()); + let out = server.process(out.dgram(), now()); assert!(out.as_dgram_ref().is_some()); let out = client.process(out.dgram(), now()); @@ -170,20 +171,20 @@ fn no_alpn() { fn dup_server_flight1() { qdebug!("---- client: generate CH"); let mut client = default_client(); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); - assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); - assert_eq!(out1.as_dgram_ref().unwrap().len(), client.plpmtu()); + assert!(out.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + assert_eq!(out.as_dgram_ref().unwrap().len(), client.plpmtu()); assert_eq!(out2.as_dgram_ref().unwrap().len(), client.plpmtu()); qdebug!( "Output={:0x?} {:0x?}", - out1.as_dgram_ref(), + out.as_dgram_ref(), out2.as_dgram_ref() ); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = default_server(); - _ = server.process(out1.dgram(), now()); + server.process_input(out.dgram().unwrap(), now()); let out_to_rep = server.process(out2.dgram(), now()); assert!(out_to_rep.as_dgram_ref().is_some()); qdebug!("Output={:0x?}", out_to_rep.as_dgram_ref()); @@ -211,7 +212,7 @@ fn dup_server_flight1() { assert_eq!(1, client.stats().dropped_rx); qdebug!("---- Dup, ignored"); - _ = client.process(out_to_rep.dgram(), now()); + client.process_input(out_to_rep.dgram().unwrap(), now()); let out = client.process(out_to_rep2.dgram(), now()); assert!(out.as_dgram_ref().is_none()); qdebug!("Output={:0x?}", out.as_dgram_ref()); @@ -313,7 +314,6 @@ fn send_05rtt() { client.process_input(s2, now()); maybe_authenticate(&mut client); assert_eq!(*client.state(), State::Connected); - let mut buf = vec![0; DEFAULT_STREAM_DATA.len() + 1]; let stream_id = client .events() @@ -333,14 +333,13 @@ fn send_05rtt() { /// Test that a client buffers 0.5-RTT data when it arrives early. #[test] fn reorder_05rtt() { - let mut client = default_client(); + // This tests makes too many assumptions about single-packet PTOs for multi-packet MLKEM flights + let mut client = new_client(ConnectionParameters::default().mlkem(false)); let mut server = default_server(); let c1 = client.process_output(now()).dgram(); - let c2 = client.process_output(now()).dgram(); - assert!(c1.is_some() && c2.is_some()); - server.process_input(c1.unwrap(), now()); - let s1 = server.process(c2, now()).dgram().unwrap(); + assert!(c1.is_some()); + let s1 = server.process(c1, now()).dgram().unwrap(); // The server should accept writes at this point. let s2 = send_something(&mut server, now()); @@ -393,18 +392,18 @@ fn reorder_05rtt_with_0rtt() { client.process_input(ticket, now); let token = get_tokens(&mut client).pop().unwrap(); - let mut client = default_client(); + // This tests makes too many assumptions about what's in the packets to work with multi-packet + // MLKEM flights. + let mut client = new_client(ConnectionParameters::default().mlkem(false)); client.enable_resumption(now, token).unwrap(); let mut server = resumed_server(&client); // Send ClientHello and some 0-RTT. let c1 = send_something(&mut client, now); - let c2 = send_something(&mut client, now); - assert_coalesced_0rtt(&c2[..]); - server.process_input(c1, now); + assert_coalesced_0rtt(&c1[..]); // Drop the 0-RTT from the coalesced datagram, so that the server // acknowledges the next 0-RTT packet. - let (c1, _) = split_datagram(&c2); + let (c1, _) = split_datagram(&c1); let c2 = send_something(&mut client, now); // Handle the first packet and send 0.5-RTT in response. Drop the response. @@ -421,7 +420,6 @@ fn reorder_05rtt_with_0rtt() { // Now PTO at the client and cause the server to re-send handshake packets. now += AT_LEAST_PTO; - _ = client.process_output(now).dgram(); let c3 = client.process_output(now).dgram(); assert_coalesced_0rtt(c3.as_ref().unwrap()); @@ -434,12 +432,12 @@ fn reorder_05rtt_with_0rtt() { client.process_input(s3, now); maybe_authenticate(&mut client); let c4 = client.process_output(now).dgram(); - // assert_eq!(*client.state(), State::Connected); + assert_eq!(*client.state(), State::Connected); assert_eq!(client.paths.rtt(), RTT); now += RTT / 2; server.process_input(c4.unwrap(), now); - // assert_eq!(*server.state(), State::Confirmed); + assert_eq!(*server.state(), State::Confirmed); // Don't check server RTT as it will be massively inflated by a // poor initial estimate received when the server dropped the // Initial packet number space. @@ -460,7 +458,7 @@ fn coalesce_05rtt() { let c11 = client.process_output(now).dgram(); assert!(c1.is_some() && c11.is_some()); now += RTT / 2; - _ = server.process(c1, now).dgram(); + server.process_input(c1.unwrap(), now); let s1 = server.process(c11, now).dgram(); assert!(s1.is_some()); @@ -477,7 +475,7 @@ fn coalesce_05rtt() { let c21 = client.process_output(now).dgram(); assert!(c2.is_some() && c21.is_some()); now += RTT / 2; - _ = server.process(c21, now).dgram(); + server.process_input(c21.unwrap(), now); let s2 = server.process(c2, now).dgram(); let dgram = client.process(s2, now).dgram(); @@ -718,11 +716,11 @@ fn extra_initial_hs() { let mut server = default_server(); let mut now = now(); - let c_init1 = client.process_output(now).dgram(); + let c_init = client.process_output(now).dgram(); let c_init2 = client.process_output(now).dgram(); - assert!(c_init1.is_some() && c_init2.is_some()); + assert!(c_init.is_some() && c_init2.is_some()); now += DEFAULT_RTT / 2; - _ = server.process(c_init1, now).dgram(); + server.process_input(c_init.unwrap(), now); let s_init = server.process(c_init2, now).dgram(); assert!(s_init.is_some()); now += DEFAULT_RTT / 2; @@ -776,11 +774,11 @@ fn extra_initial_invalid_cid() { let mut server = default_server(); let mut now = now(); - let c_init1 = client.process_output(now).dgram(); + let c_init = client.process_output(now).dgram(); let c_init2 = client.process_output(now).dgram(); - assert!(c_init1.is_some() && c_init2.is_some()); + assert!(c_init.is_some() && c_init2.is_some()); now += DEFAULT_RTT / 2; - _ = server.process(c_init1, now).dgram(); + server.process_input(c_init.unwrap(), now); let s_init = server.process(c_init2, now).dgram(); assert!(s_init.is_some()); now += DEFAULT_RTT / 2; @@ -902,12 +900,12 @@ fn garbage_initial() { #[test] fn drop_initial_packet_from_wrong_address() { let mut client = default_client(); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); - assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + assert!(out.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); let mut server = default_server(); - _ = server.process(out1.dgram(), now()); + server.process_input(out.dgram().unwrap(), now()); let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); @@ -927,12 +925,17 @@ fn drop_initial_packet_from_wrong_address() { fn drop_handshake_packet_from_wrong_address() { let mut client = default_client(); let out = client.process_output(now()); - assert!(out.as_dgram_ref().is_some()); + let out2 = client.process_output(now()); + assert!(out.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); let mut server = default_server(); - let out = server.process(out.dgram(), now()); + server.process_input(out.dgram().unwrap(), now()); + let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); + let out = client.process(out.dgram(), now()); + let out = server.process(out.dgram(), now()); + let (s_in, s_hs) = split_datagram(&out.dgram().unwrap()); // Pass the initial packet. @@ -994,9 +997,9 @@ fn ech_retry() { .client_enable_ech(damaged_ech_config(server.ech_config())) .unwrap(); - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - _ = server.process(dgram1, now()).dgram(); + server.process_input(dgram.unwrap(), now()); let dgram = server.process(dgram2, now()).dgram(); let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); @@ -1053,9 +1056,9 @@ fn ech_retry_fallback_rejected() { .client_enable_ech(damaged_ech_config(server.ech_config())) .unwrap(); - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - _ = server.process(dgram1, now()).dgram(); + server.process_input(dgram.unwrap(), now()); let dgram = server.process(dgram2, now()).dgram(); let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); @@ -1090,9 +1093,9 @@ fn bad_min_ack_delay() { .unwrap(); let mut client = default_client(); - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - _ = server.process(dgram1, now()).dgram(); + server.process_input(dgram.unwrap(), now()); let dgram = server.process(dgram2, now()).dgram(); let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); @@ -1118,11 +1121,11 @@ fn only_server_initial() { let mut client = default_client(); let mut now = now(); - let client_dgram1 = client.process_output(now).dgram(); + let client_dgram = client.process_output(now).dgram(); let client_dgram2 = client.process_output(now).dgram(); // Now fetch two flights of messages from the server. - server.process_input(client_dgram1.unwrap(), now); + server.process_input(client_dgram.unwrap(), now); let dgram = server.process(client_dgram2, now).dgram(); let dgram = client.process(dgram, now).dgram(); let server_dgram1 = server.process(dgram, now).dgram(); @@ -1172,9 +1175,9 @@ fn no_extra_probes_after_confirmed() { let mut now = now(); // First, collect a client Initial. - let spare_initial1 = client.process_output(now).dgram(); + let spare_initial = client.process_output(now).dgram(); let spare_initial2 = client.process_output(now).dgram(); - assert!(spare_initial1.is_some() && spare_initial2.is_some()); + assert!(spare_initial.is_some() && spare_initial2.is_some()); // Collect ANOTHER client Initial. now += AT_LEAST_PTO; @@ -1184,9 +1187,9 @@ fn no_extra_probes_after_confirmed() { // Finally, run the handshake. now += AT_LEAST_PTO * 2; - let dgram1 = client.process_output(now).dgram(); + let dgram = client.process_output(now).dgram(); let dgram2 = client.process_output(now).dgram(); - server.process_input(dgram1.unwrap(), now); + server.process_input(dgram.unwrap(), now); let dgram = server.process(dgram2, now).dgram(); // The server should have dropped the Initial keys now, so passing in the Initial @@ -1205,7 +1208,7 @@ fn no_extra_probes_after_confirmed() { assert_eq!(*client.state(), State::Confirmed); assert_eq!(*server.state(), State::Confirmed); - let probe = server.process(spare_initial1, now).dgram(); + let probe = server.process(spare_initial, now).dgram(); assert!(probe.is_none()); let probe = client.process(spare_handshake, now).dgram(); assert!(probe.is_none()); @@ -1218,10 +1221,10 @@ fn implicit_rtt_server() { let mut client = default_client(); let mut now = now(); - let dgram1 = client.process_output(now).dgram(); + let dgram = client.process_output(now).dgram(); let dgram2 = client.process_output(now).dgram(); now += RTT / 2; - server.process_input(dgram1.unwrap(), now); + server.process_input(dgram.unwrap(), now); let dgram = server.process(dgram2, now).dgram(); now += RTT / 2; let dgram = client.process(dgram, now).dgram(); @@ -1304,8 +1307,8 @@ fn client_initial_retransmits_identical() { // Force the client to retransmit its Initial flight a number of times and make sure the // retranmissions are identical to the original. Also, verify the PTO durations. for i in 1..=5 { - let ci1 = client.process_output(now).dgram().unwrap(); - assert_eq!(ci1.len(), client.plpmtu()); + let ci = client.process_output(now).dgram().unwrap(); + assert_eq!(ci.len(), client.plpmtu()); let ci2 = client.process_output(now).dgram().unwrap(); assert_eq!(ci2.len(), client.plpmtu()); assert_eq!( @@ -1325,29 +1328,27 @@ fn client_initial_retransmits_identical() { fn server_initial_retransmits_identical() { let mut now = now(); let mut client = default_client(); - let mut ci1 = client.process_output(now).dgram(); + let mut ci = client.process_output(now).dgram(); let mut ci2 = client.process_output(now).dgram(); // Force the server to retransmit its Initial flight a number of times and make sure the // retranmissions are identical to the original. Also, verify the PTO durations. let mut server = new_server(ConnectionParameters::default().pacing(false)); let mut total_ptos = Duration::from_secs(0); - for i in 1..=2 { - let si1 = server.process(ci1.take(), now).dgram().unwrap(); - assert_eq!(si1.len(), server.plpmtu()); - let si2 = server.process(ci2.take(), now).dgram().unwrap(); - assert_eq!(si2.len(), server.plpmtu()); + for i in 1..=3 { + _ = server.process(ci.take(), now); + _ = server.process(ci2.take(), now); if i == 1 { // On the first iteration, the server will want to send its entire flight. // During later ones, we will have hit a PTO and can hence only send two packets. - _ = server.process(ci2.take(), now).dgram().unwrap(); + _ = server.process(ci2.take(), now); } assert_eq!( server.stats().frame_tx, FrameStats { crypto: i * 3, - ack: i * 2 - usize::from(i != 1), - largest_acknowledged: i as u64 - u64::from(i != 1), + ack: i * 2 - i.saturating_sub(1), + largest_acknowledged: (i - i.saturating_sub(1)) as u64, ..Default::default() } ); @@ -1358,8 +1359,6 @@ fn server_initial_retransmits_identical() { } // Server is amplification-limited now. - let si1 = server.process(ci1.take(), now).dgram().unwrap(); - assert_eq!(si1.len(), server.plpmtu()); let pto = server.process_output(now).callback(); assert_eq!(pto, server.conn_params.get_idle_timeout() - total_ptos); } diff --git a/neqo-transport/src/connection/tests/idle.rs b/neqo-transport/src/connection/tests/idle.rs index 362b2ad100..023298159b 100644 --- a/neqo-transport/src/connection/tests/idle.rs +++ b/neqo-transport/src/connection/tests/idle.rs @@ -289,9 +289,9 @@ fn idle_caching() { // Perform the first round trip, but drop the Initial from the server. // The client then caches the Handshake packet. - let dgram1 = client.process_output(start).dgram(); + let dgram = client.process_output(start).dgram(); let dgram2 = client.process_output(start).dgram(); - _ = server.process(dgram1, start).dgram(); + server.process_input(dgram.unwrap(), start); let dgram = server.process(dgram2, start).dgram(); let dgram = client.process(dgram, start).dgram(); let dgram = server.process(dgram, start).dgram(); diff --git a/neqo-transport/src/connection/tests/keys.rs b/neqo-transport/src/connection/tests/keys.rs index 300677727e..0ef2b79b98 100644 --- a/neqo-transport/src/connection/tests/keys.rs +++ b/neqo-transport/src/connection/tests/keys.rs @@ -55,15 +55,15 @@ fn overwrite_invocations(n: PacketNumber) { fn discarded_initial_keys() { qdebug!("---- client: generate CH"); let mut client = default_client(); - let init_pkt_c1 = client.process_output(now()).dgram(); + let init_pkt_c = client.process_output(now()).dgram(); let init_pkt_c2 = client.process_output(now()).dgram(); - assert!(init_pkt_c1.is_some() && init_pkt_c2.is_some()); - assert_eq!(init_pkt_c1.as_ref().unwrap().len(), client.plpmtu()); + assert!(init_pkt_c.is_some() && init_pkt_c2.is_some()); + assert_eq!(init_pkt_c.as_ref().unwrap().len(), client.plpmtu()); assert_eq!(init_pkt_c2.as_ref().unwrap().len(), client.plpmtu()); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = default_server(); - server.process_input(init_pkt_c1.clone().unwrap(), now()); + server.process_input(init_pkt_c.clone().unwrap(), now()); let init_pkt_s = server.process(init_pkt_c2, now()).dgram(); assert!(init_pkt_s.is_some()); @@ -87,7 +87,7 @@ fn discarded_initial_keys() { // packet from the client. // We will check this by processing init_pkt_c a second time. // The dropped packet is padding. The Initial packet has been mark dup. - check_discarded(&mut server, &init_pkt_c1.clone().unwrap(), false, 1, 1); + check_discarded(&mut server, &init_pkt_c.clone().unwrap(), false, 1, 1); qdebug!("---- client: SH..FIN -> FIN"); let out = client.process_output(now()).dgram(); @@ -102,7 +102,7 @@ fn discarded_initial_keys() { // We will check this by processing init_pkt_c a third time. // The Initial packet has been dropped and padding that follows it. // There is no dups, everything has been dropped. - check_discarded(&mut server, &init_pkt_c1.unwrap(), false, 1, 0); + check_discarded(&mut server, &init_pkt_c.unwrap(), false, 1, 0); } #[test] @@ -228,13 +228,13 @@ fn key_update_before_confirmed() { assert_update_blocked(&mut server); // Client Initial - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - assert!(dgram1.is_some() && dgram2.is_some()); + assert!(dgram.is_some() && dgram2.is_some()); assert_update_blocked(&mut client); // Server Initial + Handshake - server.process_input(dgram1.unwrap(), now()); + server.process_input(dgram.unwrap(), now()); let dgram = server.process(dgram2, now()).dgram(); assert!(dgram.is_some()); assert_update_blocked(&mut server); diff --git a/neqo-transport/src/connection/tests/migration.rs b/neqo-transport/src/connection/tests/migration.rs index 567df7a989..8c01c037e6 100644 --- a/neqo-transport/src/connection/tests/migration.rs +++ b/neqo-transport/src/connection/tests/migration.rs @@ -703,9 +703,9 @@ fn migration_client_empty_cid() { /// Drive the handshake in the most expeditious fashion. /// Returns the packet containing `HANDSHAKE_DONE` from the server. fn fast_handshake(client: &mut Connection, server: &mut Connection) -> Option { - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - _ = server.process(dgram1, now()).dgram(); + server.process_input(dgram.unwrap(), now()); let dgram = server.process(dgram2, now()).dgram(); let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); diff --git a/neqo-transport/src/connection/tests/priority.rs b/neqo-transport/src/connection/tests/priority.rs index 36e6216eff..e904edab76 100644 --- a/neqo-transport/src/connection/tests/priority.rs +++ b/neqo-transport/src/connection/tests/priority.rs @@ -207,9 +207,9 @@ fn connect_for_0rtt() -> (Connection, Connection) { // Rather than connect, send stream data in 0.5-RTT. // That allows this to test that critical streams preempt most frame types. - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - server.process_input(dgram1.unwrap(), now()); + server.process_input(dgram.unwrap(), now()); let dgram = server.process(dgram2, now()).dgram(); client.process_input(dgram.unwrap(), now()); (client, server) diff --git a/neqo-transport/src/connection/tests/recovery.rs b/neqo-transport/src/connection/tests/recovery.rs index a001a31fc1..a2afd6ca65 100644 --- a/neqo-transport/src/connection/tests/recovery.rs +++ b/neqo-transport/src/connection/tests/recovery.rs @@ -163,24 +163,21 @@ fn pto_initial() { const INITIAL_PTO: Duration = Duration::from_millis(300); let mut now = now(); + // This tests makes too many assumptions about single-packet PTOs for multi-packet MLKEM flights qdebug!("---- client: generate CH"); - let mut client = default_client(); + let mut client = new_client(ConnectionParameters::default().mlkem(false)); let pkt1 = client.process_output(now).dgram(); - let pkt2 = client.process_output(now).dgram(); - assert!(pkt1.is_some() && pkt2.is_some()); - assert_eq!(pkt1.unwrap().len(), client.plpmtu()); - assert_eq!(pkt2.unwrap().len(), client.plpmtu()); + assert!(pkt1.is_some()); + assert_eq!(pkt1.clone().unwrap().len(), client.plpmtu()); let delay = client.process_output(now).callback(); assert_eq!(delay, INITIAL_PTO); // Resend initial after PTO. now += delay; - let pkt1 = client.process_output(now).dgram(); let pkt2 = client.process_output(now).dgram(); - assert!(pkt1.is_some() && pkt2.is_some()); - assert_eq!(pkt1.clone().unwrap().len(), client.plpmtu()); - assert_eq!(pkt2.clone().unwrap().len(), client.plpmtu()); + assert!(pkt2.is_some()); + assert_eq!(pkt2.unwrap().len(), client.plpmtu()); let delay = client.process_output(now).callback(); // PTO has doubled. @@ -188,8 +185,7 @@ fn pto_initial() { // Server process the first initial pkt. let mut server = default_server(); - _ = server.process(pkt1, now).dgram(); - let out = server.process(pkt2, now).dgram(); + let out = server.process(pkt1, now).dgram(); assert!(out.is_some()); // Client receives ack for the first initial packet as well a Handshake packet. @@ -217,18 +213,26 @@ fn pto_handshake_complete() { let mut client = default_client(); let mut server = default_server(); - let pkt1 = client.process_output(now).dgram(); + let pkt = client.process_output(now).dgram(); let pkt2 = client.process_output(now).dgram(); - assert_initial(pkt1.as_ref().unwrap(), false); + assert_initial(pkt.as_ref().unwrap(), false); assert_initial(pkt2.as_ref().unwrap(), false); let cb = client.process_output(now).callback(); - assert_eq!(cb, Duration::from_millis(300)); + assert_eq!(cb, Duration::from_millis(5)); // Pacing delay now += HALF_RTT; - _ = server.process(pkt1, now).dgram(); + server.process_input(pkt.unwrap(), now); let pkt = server.process(pkt2, now).dgram(); assert_initial(pkt.as_ref().unwrap(), false); + now += HALF_RTT; + let pkt = client.process(pkt, now).dgram(); + assert_initial(pkt.as_ref().unwrap(), false); + + now += HALF_RTT; + let pkt = server.process(pkt, now).dgram(); + assert_initial(pkt.as_ref().unwrap(), false); + now += HALF_RTT; let pkt = client.process(pkt, now).dgram(); assert_handshake(pkt.as_ref().unwrap()); @@ -339,19 +343,24 @@ fn pto_handshake_frames() { let mut now = now(); qdebug!("---- client: generate CH"); let mut client = default_client(); - let pkt1 = client.process_output(now); + let pkt = client.process_output(now); let pkt2 = client.process_output(now); now += Duration::from_millis(10); qdebug!("---- server: CH -> SH, EE, CERT, CV, FIN"); let mut server = default_server(); - _ = server.process(pkt1.dgram(), now); + server.process_input(pkt.dgram().unwrap(), now); let pkt = server.process(pkt2.dgram(), now); now += Duration::from_millis(10); qdebug!("---- client: cert verification"); let pkt = client.process(pkt.dgram(), now); + now += Duration::from_millis(10); + let pkt = server.process(pkt.dgram(), now); + now += Duration::from_millis(10); + let pkt = client.process(pkt.dgram(), now); + now += Duration::from_millis(10); drop(server.process(pkt.dgram(), now)); @@ -388,7 +397,9 @@ fn pto_handshake_frames() { fn handshake_ack_pto() { const RTT: Duration = Duration::from_millis(10); let mut now = now(); - let mut client = default_client(); + // This tests makes too many assumptions about single-packet PTOs for multi-packet MLKEM flights + // to work. + let mut client = new_client(ConnectionParameters::default().mlkem(false)); let mut server = default_server(); // This is a greasing transport parameter, and large enough that the // server needs to send two Handshake packets. @@ -570,7 +581,9 @@ fn lost_but_kept_and_lr_timer() { fn loss_time_past_largest_acked() { const RTT: Duration = Duration::from_secs(10); const INCR: Duration = Duration::from_millis(1); - let mut client = default_client(); + // This tests makes too many assumptions about single-packet PTOs for multi-packet MLKEM flights + // to work. + let mut client = new_client(ConnectionParameters::default().mlkem(false)); let mut server = default_server(); let mut now = now(); @@ -747,7 +760,13 @@ fn expected_pto(rtt: Duration) -> Duration { #[test] fn fast_pto() { - let mut client = new_client(ConnectionParameters::default().fast_pto(FAST_PTO_SCALE / 2)); + // This tests makes too many assumptions about single-packet PTOs for multi-packet MLKEM flights + // to work. + let mut client = new_client( + ConnectionParameters::default() + .fast_pto(FAST_PTO_SCALE / 2) + .mlkem(false), + ); let mut server = default_server(); let mut now = connect_rtt_idle(&mut client, &mut server, DEFAULT_RTT); diff --git a/neqo-transport/src/connection/tests/stream.rs b/neqo-transport/src/connection/tests/stream.rs index 2848c0159b..2921603471 100644 --- a/neqo-transport/src/connection/tests/stream.rs +++ b/neqo-transport/src/connection/tests/stream.rs @@ -32,10 +32,10 @@ use crate::{ fn stream_create() { let mut client = default_client(); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); let mut server = default_server(); - _ = server.process(out1.dgram(), now()); + server.process_input(out.dgram().unwrap(), now()); let out = server.process(out2.dgram(), now()); let out = client.process(out.dgram(), now()); @@ -756,11 +756,11 @@ fn client_fin_reorder() { let mut server = default_server(); // Send ClientHello. - let client_hs1 = client.process_output(now()); + let client_hs = client.process_output(now()); let client_hs2 = client.process_output(now()); - assert!(client_hs1.as_dgram_ref().is_some() && client_hs2.as_dgram_ref().is_some()); + assert!(client_hs.as_dgram_ref().is_some() && client_hs2.as_dgram_ref().is_some()); - _ = server.process(client_hs1.dgram(), now()); + server.process_input(client_hs.dgram().unwrap(), now()); let server_hs = server.process(client_hs2.dgram(), now()); assert!(server_hs.as_dgram_ref().is_some()); // ServerHello, etc... @@ -1281,14 +1281,14 @@ fn session_flow_control_affects_all_streams() { fn connect_w_different_limit(bidi_limit: u64, unidi_limit: u64) { let mut client = default_client(); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); let mut server = new_server( ConnectionParameters::default() .max_streams(StreamType::BiDi, bidi_limit) .max_streams(StreamType::UniDi, unidi_limit), ); - _ = server.process(out1.dgram(), now()); + server.process_input(out.dgram().unwrap(), now()); let out = server.process(out2.dgram(), now()); let out = client.process(out.dgram(), now()); diff --git a/neqo-transport/src/connection/tests/vn.rs b/neqo-transport/src/connection/tests/vn.rs index e305af583d..3f86700211 100644 --- a/neqo-transport/src/connection/tests/vn.rs +++ b/neqo-transport/src/connection/tests/vn.rs @@ -248,10 +248,10 @@ fn compatible_upgrade_large_initial() { // Client Initial should take 2 packets. // Each should elicit a Version 1 ACK from the server. - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - assert!(dgram1.is_some() && dgram2.is_some()); - _ = server.process(dgram1, now()).dgram(); + assert!(dgram.is_some() && dgram2.is_some()); + server.process_input(dgram.unwrap(), now()); let dgram = server.process(dgram2, now()).dgram(); assert!(dgram.is_some()); // The following uses the Version from *outside* this crate. @@ -262,7 +262,7 @@ fn compatible_upgrade_large_initial() { assert_eq!(client.version(), Version::Version2); assert_eq!(server.version(), Version::Version2); // Only handshake padding is "dropped". - assert_eq!(client.stats().dropped_rx, 3); + assert_eq!(client.stats().dropped_rx, 1); assert_eq!(server.stats().dropped_rx, 3); } @@ -339,9 +339,9 @@ fn invalid_server_version() { let mut server = new_server(ConnectionParameters::default().versions(Version::Version2, Version::all())); - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - server.process_input(dgram1.unwrap(), now()); + server.process_input(dgram.unwrap(), now()); server.process_input(dgram2.unwrap(), now()); // Three packets received (one is zero padding). @@ -470,11 +470,11 @@ fn compatible_upgrade_0rtt_rejected() { client.enable_resumption(now(), token).unwrap(); // Create a packet with 0-RTT from the client. - let initial1 = send_something(&mut client, now()); + let initial = send_something(&mut client, now()); let initial2 = send_something(&mut client, now()); - assertions::assert_version(&initial1, Version::Version1.wire_version()); + assertions::assert_version(&initial, Version::Version1.wire_version()); assertions::assert_coalesced_0rtt(&initial2); - server.process_input(initial1, now()); + server.process_input(initial, now()); server.process_input(initial2, now()); assert!(!server .events() diff --git a/neqo-transport/src/connection/tests/zerortt.rs b/neqo-transport/src/connection/tests/zerortt.rs index ec7fc4ee10..b64472f591 100644 --- a/neqo-transport/src/connection/tests/zerortt.rs +++ b/neqo-transport/src/connection/tests/zerortt.rs @@ -52,9 +52,9 @@ fn zero_rtt_send_recv() { let mut server = resumed_server(&client); // Send ClientHello. - let client_hs1 = client.process_output(now()); + let client_hs = client.process_output(now()); let client_hs2 = client.process_output(now()); - assert!(client_hs1.as_dgram_ref().is_some() && client_hs2.as_dgram_ref().is_some()); + assert!(client_hs.as_dgram_ref().is_some() && client_hs2.as_dgram_ref().is_some()); // Wait let delay = client.process_output(now()).callback(); @@ -67,7 +67,7 @@ fn zero_rtt_send_recv() { // 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); - _ = server.process(client_hs1.dgram(), now()); + server.process_input(client_hs.dgram().unwrap(), now()); let server_hs = server.process(client_hs2.dgram(), now()); assert!(server_hs.as_dgram_ref().is_some()); // ServerHello, etc... @@ -75,7 +75,7 @@ fn zero_rtt_send_recv() { let ack_frames = server.stats().frame_tx.ack; let server_process_0rtt = server.process(client_0rtt.dgram(), now()); assert!(server_process_0rtt.dgram().is_some()); - assert_eq!(server.stats().frame_tx.all(), all_frames + 1); + assert_eq!(server.stats().frame_tx.all(), all_frames + 3); assert_eq!(server.stats().frame_tx.ack, ack_frames + 1); let server_stream_id = server @@ -239,7 +239,7 @@ fn zero_rtt_update_flow_control() { ); // Stream limits should be low for 0-RTT. - let client_hs1 = client.process_output(now()).dgram(); + let client_hs = client.process_output(now()).dgram(); let client_hs2 = client.process_output(now()).dgram(); let uni_stream = client.stream_create(StreamType::UniDi).unwrap(); assert!(!client.stream_send_atomic(uni_stream, MESSAGE).unwrap()); @@ -247,7 +247,7 @@ fn zero_rtt_update_flow_control() { assert!(!client.stream_send_atomic(bidi_stream, MESSAGE).unwrap()); // Now get the server transport parameters. - _ = server.process(client_hs1, now()).dgram(); + server.process_input(client_hs.unwrap(), now()); let server_hs = server.process(client_hs2, now()).dgram(); let client_hs3 = client.process(server_hs, now()).dgram(); let server_hs2 = server.process(client_hs3, now()).dgram(); @@ -319,7 +319,7 @@ fn zero_rtt_loss_accepted() { } // Process CI/0-RTT - _ = server.process(ci.dgram(), now); + server.process_input(ci.dgram().unwrap(), now); let si = server.process(c0rtt.dgram(), now); assert!(si.as_dgram_ref().is_some()); diff --git a/neqo-transport/tests/common/mod.rs b/neqo-transport/tests/common/mod.rs index c43800e662..3ebf829666 100644 --- a/neqo-transport/tests/common/mod.rs +++ b/neqo-transport/tests/common/mod.rs @@ -58,10 +58,10 @@ pub fn connect(client: &mut Connection, server: &mut Server) -> ConnectionRef { server.set_validation(ValidateAddress::Never); assert_eq!(*client.state(), State::Init); - let out1 = client.process_output(now()); // ClientHello + let out = client.process_output(now()); // ClientHello let out2 = client.process_output(now()); // ClientHello - assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); - _ = server.process(out1.dgram(), now()); // ACK + assert!(out.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + _ = server.process(out.dgram(), now()); // ACK let out = server.process(out2.dgram(), now()); // ServerHello... assert!(out.as_dgram_ref().is_some()); diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index 6ef70f7ff0..2609a548a5 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -29,10 +29,10 @@ fn truncate_long_packet() { let mut client = default_client(); let mut server = default_server(); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); - assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); - _ = server.process(out1.dgram(), now()); + assert!(out.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + server.process_input(out.dgram().unwrap(), now()); let out = server.process(out2.dgram(), now()); assert!(out.as_dgram_ref().is_some()); let out = client.process(out.dgram(), now()); diff --git a/neqo-transport/tests/retry.rs b/neqo-transport/tests/retry.rs index b7470bb3c3..3a481c9733 100644 --- a/neqo-transport/tests/retry.rs +++ b/neqo-transport/tests/retry.rs @@ -35,17 +35,17 @@ fn retry_basic() { server.set_validation(ValidateAddress::Always); let mut client = default_client(); - let dgram1 = client.process_output(now()).dgram(); // Initial + let dgram = client.process_output(now()).dgram(); // Initial let dgram2 = client.process_output(now()).dgram(); // Initial - assert!(dgram1.is_some() && dgram2.is_some()); - _ = server.process(dgram1, now()).dgram().unwrap(); // Retry + assert!(dgram.is_some() && dgram2.is_some()); + _ = server.process(dgram, now()).dgram().unwrap(); // Retry let dgram = server.process(dgram2, now()).dgram().unwrap(); // Retry assertions::assert_retry(&dgram); - let dgram1 = client.process(Some(dgram), now()).dgram(); // Initial w/token + let dgram = client.process(Some(dgram), now()).dgram(); // Initial w/token let dgram2 = client.process_output(now()).dgram(); // Initial - assert!(dgram1.is_some() && dgram2.is_some()); - _ = server.process(dgram1, now()).dgram().unwrap(); + assert!(dgram.is_some() && dgram2.is_some()); + _ = server.process(dgram, now()).dgram().unwrap(); let dgram = server.process(dgram2, now()).dgram(); let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); // Initial, HS @@ -115,22 +115,22 @@ fn retry_0rtt() { let client_stream = client.stream_create(StreamType::UniDi).unwrap(); client.stream_send(client_stream, &[1, 2, 3]).unwrap(); - let dgram1 = client.process_output(now()).dgram(); // Initial + let dgram = client.process_output(now()).dgram(); // Initial let dgram2 = client.process_output(now()).dgram(); // Initial w/0-RTT - assert!(dgram1.is_some() && dgram2.is_some()); + assert!(dgram.is_some() && dgram2.is_some()); assertions::assert_coalesced_0rtt(dgram2.as_ref().unwrap()); - _ = server.process(dgram1, now()).dgram(); // Retry + _ = server.process(dgram, now()).dgram(); // Retry let dgram = server.process(dgram2, now()).dgram(); // Retry assert!(dgram.is_some()); assertions::assert_retry(dgram.as_ref().unwrap()); // After retry, there should be a token and still coalesced 0-RTT. - let dgram1 = client.process(dgram, now()).dgram(); // Initial + let dgram = client.process(dgram, now()).dgram(); // Initial let dgram2 = client.process_output(now()).dgram(); // Initial w/0-RTT - assert!(dgram1.is_some() && dgram2.is_some()); + assert!(dgram.is_some() && dgram2.is_some()); assertions::assert_coalesced_0rtt(dgram2.as_ref().unwrap()); - _ = server.process(dgram1, now()).dgram(); // ACK + _ = server.process(dgram, now()).dgram(); // ACK let dgram = server.process(dgram2, now()).dgram(); // Initial, HS assert!(dgram.is_some()); let dgram = client.process(dgram, now()).dgram(); @@ -225,20 +225,23 @@ fn retry_after_initial() { retry_server.set_validation(ValidateAddress::Always); let mut client = default_client(); - let cinit1 = client.process_output(now()).dgram(); // Initial + let cinit = client.process_output(now()).dgram(); // Initial let cinit2 = client.process_output(now()).dgram(); // Initial - assert!(cinit1.is_some() && cinit2.is_some()); - _ = server.process(cinit1.clone(), now()).dgram(); // Initial + assert!(cinit.is_some() && cinit2.is_some()); + _ = server.process(cinit.clone(), now()).dgram(); // Initial let server_flight = server.process(cinit2, now()).dgram(); // Initial assert!(server_flight.is_some()); + let dgram = client.process(server_flight, now()).dgram(); + let server_flight = server.process(dgram, now()).dgram(); + // We need to have the client just process the Initial. let (server_initial, _other) = split_datagram(server_flight.as_ref().unwrap()); let dgram = client.process(Some(server_initial), now()).dgram(); assert!(dgram.is_some()); assert!(*client.state() != State::Connected); - let retry = retry_server.process(cinit1, now()).dgram(); // Retry! + let retry = retry_server.process(cinit, now()).dgram(); // Retry! assert!(retry.is_some()); assertions::assert_retry(retry.as_ref().unwrap()); @@ -248,11 +251,10 @@ fn retry_after_initial() { // Either way, the client should still be able to process the server flight and connect. let dgram = client.process(server_flight, now()).dgram(); - let dgram = server.process(dgram, now()).dgram(); - let dgram = client.process(dgram, now()).dgram(); assert!(dgram.is_some()); // Drop this one. assert!(test_fixture::maybe_authenticate(&mut client)); - let dgram = client.process_output(now()).dgram(); + let dgram = server.process(dgram, now()).dgram(); + let dgram = client.process(dgram, now()).dgram(); assert!(dgram.is_some()); assert_eq!(*client.state(), State::Connected); @@ -267,10 +269,10 @@ fn retry_bad_integrity() { server.set_validation(ValidateAddress::Always); let mut client = default_client(); - let dgram1 = client.process_output(now()).dgram(); // Initial + let dgram = client.process_output(now()).dgram(); // Initial let dgram2 = client.process_output(now()).dgram(); // Initial - assert!(dgram1.is_some() && dgram2.is_some()); - _ = server.process(dgram1, now()).dgram(); // Retry + assert!(dgram.is_some() && dgram2.is_some()); + _ = server.process(dgram, now()).dgram(); // Retry let dgram = server.process(dgram2, now()).dgram(); // Retry assert!(dgram.is_some()); @@ -317,9 +319,9 @@ fn retry_after_pto() { server.set_validation(ValidateAddress::Always); let mut now = now(); - let ci1 = client.process_output(now).dgram(); + let ci = client.process_output(now).dgram(); let ci2 = client.process_output(now).dgram(); - assert!(ci1.is_some() && ci2.is_some()); // sit on this for a bit + assert!(ci.is_some() && ci2.is_some()); // sit on this for a bit // Let PTO fire on the client and then let it exhaust its PTO packets. now += Duration::from_secs(1); @@ -329,7 +331,7 @@ fn retry_after_pto() { let cb = client.process_output(now).callback(); assert_ne!(cb, Duration::new(0, 0)); - _ = server.process(ci1, now).dgram(); + _ = server.process(ci, now).dgram(); let retry = server.process(ci2, now).dgram(); assertions::assert_retry(retry.as_ref().unwrap()); @@ -343,10 +345,10 @@ fn vn_after_retry() { server.set_validation(ValidateAddress::Always); let mut client = default_client(); - let dgram1 = client.process_output(now()).dgram(); // Initial + let dgram = client.process_output(now()).dgram(); // Initial let dgram2 = client.process_output(now()).dgram(); // Initial - assert!(dgram1.is_some() && dgram2.is_some()); - _ = server.process(dgram1, now()).dgram(); // Retry + assert!(dgram.is_some() && dgram2.is_some()); + _ = server.process(dgram, now()).dgram(); // Retry let dgram = server.process(dgram2, now()).dgram(); // Retry assert!(dgram.is_some()); diff --git a/neqo-transport/tests/server.rs b/neqo-transport/tests/server.rs index 0f641c3080..984e1bf270 100644 --- a/neqo-transport/tests/server.rs +++ b/neqo-transport/tests/server.rs @@ -380,12 +380,12 @@ fn new_token_0rtt() { let client_stream = client.stream_create(StreamType::UniDi).unwrap(); client.stream_send(client_stream, &[1, 2, 3]).unwrap(); - let out1 = client.process_output(now()); + let out = client.process_output(now()); let out2 = client.process_output(now()); // Initial w/0-RTT - assert!(out1.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); - assertions::assert_initial(out1.as_dgram_ref().unwrap(), true); + assert!(out.as_dgram_ref().is_some() && out2.as_dgram_ref().is_some()); + assertions::assert_initial(out.as_dgram_ref().unwrap(), true); assertions::assert_coalesced_0rtt(out2.as_dgram_ref().unwrap()); - _ = server.process(out1.dgram(), now()); // Initial + _ = server.process(out.dgram(), now()); // Initial let out = server.process(out2.dgram(), now()); // Initial assert!(out.as_dgram_ref().is_some()); assertions::assert_initial(out.as_dgram_ref().unwrap(), false); @@ -657,24 +657,24 @@ fn version_negotiation_and_compatible() { // Run the full exchange so that we can observe the versions in use. // Version Negotiation - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - assert!(dgram1.is_some() && dgram2.is_some()); - assertions::assert_version(dgram1.as_ref().unwrap(), ORIG_VERSION.wire_version()); - _ = server.process(dgram1, now()).dgram(); + assert!(dgram.is_some() && dgram2.is_some()); + assertions::assert_version(dgram.as_ref().unwrap(), ORIG_VERSION.wire_version()); + _ = server.process(dgram, now()).dgram(); let dgram = server.process(dgram2, now()).dgram(); assertions::assert_vn(dgram.as_ref().unwrap()); client.process_input(dgram.unwrap(), now()); - let dgram1 = client.process_output(now()).dgram(); // ClientHello + let dgram = client.process_output(now()).dgram(); // ClientHello let dgram2 = client.process_output(now()).dgram(); // ClientHello - assertions::assert_version(dgram1.as_ref().unwrap(), VN_VERSION.wire_version()); - _ = server.process(dgram1, now()).dgram(); // ServerHello... + assertions::assert_version(dgram.as_ref().unwrap(), VN_VERSION.wire_version()); + _ = server.process(dgram, now()).dgram(); // ServerHello... let dgram = server.process(dgram2, now()).dgram(); // ServerHello... assertions::assert_version(dgram.as_ref().unwrap(), COMPAT_VERSION.wire_version()); let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); - _ = client.process(dgram, now()).dgram(); + client.process_input(dgram.unwrap(), now()); client.authenticated(AuthenticationStatus::Ok, now()); let dgram = client.process_output(now()).dgram(); @@ -839,9 +839,9 @@ fn max_streams_after_0rtt_rejection() { let mut client = default_client(); client.enable_resumption(now(), &token).unwrap(); _ = client.stream_create(StreamType::BiDi).unwrap(); - let dgram1 = client.process_output(now()).dgram(); + let dgram = client.process_output(now()).dgram(); let dgram2 = client.process_output(now()).dgram(); - _ = server.process(dgram1, now()).dgram(); + _ = server.process(dgram, now()).dgram(); let dgram = server.process(dgram2, now()).dgram(); let dgram = client.process(dgram, now()).dgram(); let dgram = server.process(dgram, now()).dgram(); From 45d39234cc07efca216603f3a3bc7c3e2c42bf3a Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Tue, 28 Jan 2025 17:07:00 +0200 Subject: [PATCH 05/17] clippy --- neqo-transport/src/send_stream.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/neqo-transport/src/send_stream.rs b/neqo-transport/src/send_stream.rs index 347edc9d65..7dbed53425 100644 --- a/neqo-transport/src/send_stream.rs +++ b/neqo-transport/src/send_stream.rs @@ -498,6 +498,7 @@ impl TxBuffer { can_buffer } + #[allow(clippy::missing_panics_doc)] // These are not possible. pub fn is_empty(&mut self) -> bool { let (start, _) = self.ranges.first_unmarked_range(); start == self.retired() + u64::try_from(self.buffered()).unwrap() From 812cd6b8c20f03d6c71de3dcbf8a23062c778f7d Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Tue, 28 Jan 2025 17:15:07 +0200 Subject: [PATCH 06/17] Fixes --- neqo-transport/src/connection/tests/vn.rs | 2 +- neqo-transport/tests/network.rs | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/neqo-transport/src/connection/tests/vn.rs b/neqo-transport/src/connection/tests/vn.rs index 7d3e6dc6d7..3c88439407 100644 --- a/neqo-transport/src/connection/tests/vn.rs +++ b/neqo-transport/src/connection/tests/vn.rs @@ -263,7 +263,7 @@ fn compatible_upgrade_large_initial() { assert_eq!(server.version(), Version::Version2); // Only handshake padding is "dropped". assert_eq!(client.stats().dropped_rx, 1); - assert!(matches!(server.stats().dropped_rx, 2 | 3)); + assert!(matches!(server.stats().dropped_rx, 3 | 4)); } /// A server that supports versions 1 and 2 might prefer version 1 and that's OK. diff --git a/neqo-transport/tests/network.rs b/neqo-transport/tests/network.rs index 68a835a436..2d88171e31 100644 --- a/neqo-transport/tests/network.rs +++ b/neqo-transport/tests/network.rs @@ -60,7 +60,9 @@ simulate!( idle_timeout_crazy_rtt, [ ConnectionNode::new_client( - ConnectionParameters::default().idle_timeout(weeks(1000)), + ConnectionParameters::default() + .idle_timeout(weeks(1000)) + .mlkem(false), boxed![ReachState::new(State::Confirmed),], boxed![ReachState::new(State::Closed(CloseReason::Transport( Error::IdleTimeout @@ -69,7 +71,9 @@ simulate!( Delay::new(weeks(6)..weeks(6)), Drop::percentage(10), ConnectionNode::new_server( - ConnectionParameters::default().idle_timeout(weeks(1000)), + ConnectionParameters::default() + .idle_timeout(weeks(1000)) + .mlkem(false), boxed![ReachState::new(State::Confirmed),], boxed![ReachState::new(State::Closed(CloseReason::Transport( Error::IdleTimeout From 1c1730dde0052c25b151007c3c0e6f71d3d63860 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Tue, 28 Jan 2025 18:34:57 +0200 Subject: [PATCH 07/17] MLKEM off in simulator --- neqo-transport/benches/transfer.rs | 10 ++++++++-- neqo-transport/tests/network.rs | 16 ++++++++-------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/neqo-transport/benches/transfer.rs b/neqo-transport/benches/transfer.rs index ca658d1aee..8776081022 100644 --- a/neqo-transport/benches/transfer.rs +++ b/neqo-transport/benches/transfer.rs @@ -33,14 +33,20 @@ fn benchmark_transfer(c: &mut Criterion, label: &str, seed: Option Date: Wed, 29 Jan 2025 08:38:46 +0200 Subject: [PATCH 08/17] Update neqo-transport/src/connection/mod.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert --- neqo-transport/src/connection/mod.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index c942a4485a..3cffaf330d 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1522,14 +1522,12 @@ impl Connection { } if matches!(self.state, State::WaitInitial | State::WaitVersion) { - self.set_state( - if self.has_version() { - State::Handshaking - } else { - State::WaitVersion - }, - now, - ); + let new_state = if self.has_version() { + State::Handshaking + } else { + State::WaitVersion + }; + self.set_state(new_state, now); if self.role == Role::Server { self.zero_rtt_state = if self.crypto.enable_0rtt(self.version, self.role) == Ok(true) { From e7299beabed3cafdc001be22ba6e6758b461c0a5 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 29 Jan 2025 08:41:13 +0200 Subject: [PATCH 09/17] Update neqo-transport/src/crypto.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert --- neqo-transport/src/crypto.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 757e7c8918..e76981e977 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -97,8 +97,9 @@ impl Crypto { ] })?; if let Agent::Client(c) = &mut agent { - // Configure clients to send additional key shares to reduce the rate of HRRs. - c.send_additional_key_shares(1)?; + // Configure clients to send additional key shares to reduce the rate of HRRs + // when enabling MLKEM. + c.send_additional_key_shares(usize::from(conn_params.mlkem_enabled()))?; // Always enable 0-RTT on the client, but the server needs // more configuration passed to server_enable_0rtt. From d231033da24cfc0df6eca8022794f58f3c46a06a Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 29 Jan 2025 08:41:36 +0200 Subject: [PATCH 10/17] Update neqo-transport/src/connection/mod.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert --- neqo-transport/src/connection/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 3cffaf330d..7430e471c1 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1528,7 +1528,7 @@ impl Connection { State::WaitVersion }; self.set_state(new_state, now); - if self.role == Role::Server { + if self.role == Role::Server && new_state == State::Handshaking { self.zero_rtt_state = if self.crypto.enable_0rtt(self.version, self.role) == Ok(true) { qdebug!("[{self}] Accepted 0-RTT"); From 1754d032fc195d0656712c4200013b3ada308f21 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 29 Jan 2025 09:31:20 +0200 Subject: [PATCH 11/17] Fixes --- neqo-transport/src/connection/mod.rs | 3 ++- neqo-transport/src/crypto.rs | 9 ++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 7430e471c1..50fb6be3a0 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1528,7 +1528,7 @@ impl Connection { State::WaitVersion }; self.set_state(new_state, now); - if self.role == Role::Server && new_state == State::Handshaking { + if self.role == Role::Server && self.state == State::Handshaking { self.zero_rtt_state = if self.crypto.enable_0rtt(self.version, self.role) == Ok(true) { qdebug!("[{self}] Accepted 0-RTT"); @@ -2515,6 +2515,7 @@ impl Connection { } // If the client has more Initial CRYPTO data queued up, do not coalesce. + // FIXME: This is a temporary hack to avoid coalescing 0-RTT packets too early. if self.role == Role::Client && *space == PacketNumberSpace::Initial && !self.crypto.streams.is_empty(*space) diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index e76981e977..81e505a31e 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -1527,10 +1527,9 @@ impl CryptoStreams { } let cs = self.get_mut(space).unwrap(); - // Because the TLS extensions in the CRYPTO data are randomly ordered, if the CH is - // larger than fits into a single packet, depending on where the SNI is located, slicing and - // reordering it can cause the generation of discontiguous chunks. We need to loop here to - // make sure we fill the packets, and not send superfluous packets that are mostly padding. + // Sending fresh data will always fill a packet. However, when there are + // lost packets, the outstanding CRYPTO chunks might not be contiguous + // and packet-filling. So we need to loop. while let Some((offset, data)) = cs.tx.next_bytes() { let written = if sni_slicing { if let Some(sni) = find_sni(data) { @@ -1548,7 +1547,7 @@ impl CryptoStreams { // the first Initial with the minimum amount of zeros to // help middlebox traversal, so the second packets had more // space for coalescing. - let packets_needed = data.len() / builder.limit() + 1; + let packets_needed = data.len().div_ceil(builder.limit()); if packets_needed > 1 { builder.set_limit(data.len() / packets_needed); } From 2fe51fd17b6de0bd79d01f5865ec21f182e3e5af Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 31 Jan 2025 12:20:20 +0200 Subject: [PATCH 12/17] SNI slicing revamp --- neqo-common/src/chunk.rs | 103 +++++++++++++++++++ neqo-common/src/lib.rs | 1 + neqo-transport/src/connection/tests/vn.rs | 2 +- neqo-transport/src/crypto.rs | 114 +++++++++++++--------- 4 files changed, 172 insertions(+), 48 deletions(-) create mode 100644 neqo-common/src/chunk.rs diff --git a/neqo-common/src/chunk.rs b/neqo-common/src/chunk.rs new file mode 100644 index 0000000000..9f7d7e8553 --- /dev/null +++ b/neqo-common/src/chunk.rs @@ -0,0 +1,103 @@ +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#[derive(Debug)] +pub struct Chunk<'a> { + data: &'a [u8], + offset: u64, +} + +impl Chunk<'_> { + #[must_use] + pub const fn data(&self) -> &[u8] { + self.data + } + + #[must_use] + pub const fn offset(&self) -> u64 { + self.offset + } + + #[allow(clippy::len_without_is_empty)] + #[must_use] + pub const fn len(&self) -> usize { + self.data.len() + } + + #[must_use] + pub const fn split_at(&self, at: usize) -> (Self, Self) { + let (left, right) = self.data.split_at(at); + ( + Chunk { + data: left, + offset: self.offset, + }, + Chunk { + data: right, + offset: self.offset + at as u64, + }, + ) + } + + pub const fn limit_to(&mut self, limit: usize) { + let (left, _) = self.data.split_at(limit); + self.data = left; + } +} + +impl<'a> From> for (u64, usize) { + fn from(val: Chunk<'a>) -> Self { + (val.offset, val.data.len()) + } +} + +impl<'a> From<(u64, &'a [u8])> for Chunk<'a> { + fn from(value: (u64, &'a [u8])) -> Self { + Chunk { + data: value.1, + offset: value.0, + } + } +} + +#[derive(Debug)] +pub struct ChunkRange { + offset: u64, + len: usize, +} + +impl ChunkRange { + #[must_use] + pub const fn new(offset: u64, len: usize) -> Self { + Self { offset, len } + } + + #[must_use] + pub const fn offset(&self) -> u64 { + self.offset + } + + #[allow(clippy::len_without_is_empty)] + #[must_use] + pub const fn len(&self) -> usize { + self.len + } +} + +impl From<&Chunk<'_>> for ChunkRange { + fn from(chunk: &Chunk<'_>) -> Self { + Self { + offset: chunk.offset, + len: chunk.len(), + } + } +} + +impl From for (u64, usize) { + fn from(val: ChunkRange) -> Self { + (val.offset, val.len) + } +} diff --git a/neqo-common/src/lib.rs b/neqo-common/src/lib.rs index 1e7a53ad19..ae7a5fee35 100644 --- a/neqo-common/src/lib.rs +++ b/neqo-common/src/lib.rs @@ -6,6 +6,7 @@ #![allow(clippy::module_name_repetitions)] // This lint doesn't work here. +pub mod chunk; mod codec; mod datagram; pub mod event; diff --git a/neqo-transport/src/connection/tests/vn.rs b/neqo-transport/src/connection/tests/vn.rs index 3c88439407..7d3e6dc6d7 100644 --- a/neqo-transport/src/connection/tests/vn.rs +++ b/neqo-transport/src/connection/tests/vn.rs @@ -263,7 +263,7 @@ fn compatible_upgrade_large_initial() { assert_eq!(server.version(), Version::Version2); // Only handshake padding is "dropped". assert_eq!(client.stats().dropped_rx, 1); - assert!(matches!(server.stats().dropped_rx, 3 | 4)); + assert!(matches!(server.stats().dropped_rx, 2 | 3)); } /// A server that supports versions 1 and 2 might prefer version 1 and that's OK. diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 81e505a31e..46f9022273 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -16,7 +16,10 @@ use std::{ time::Instant, }; -use neqo_common::{hex, hex_snip_middle, qdebug, qinfo, qtrace, Encoder, Role}; +use neqo_common::{ + chunk::{Chunk, ChunkRange}, + hex, hex_snip_middle, qdebug, qinfo, qtrace, Encoder, Role, +}; use neqo_crypto::{ hkdf, hp::HpKey, Aead, Agent, AntiReplay, Cipher, Epoch, Error as CryptoError, HandshakeState, PrivateKey, PublicKey, Record, RecordList, ResumptionToken, SymKey, ZeroRttChecker, @@ -1475,6 +1478,7 @@ impl CryptoStreams { } } + #[allow(clippy::too_many_lines)] pub fn write_frame( &mut self, space: PacketNumberSpace, @@ -1484,38 +1488,37 @@ impl CryptoStreams { stats: &mut FrameStats, ) { fn write_chunk( - offset: u64, - data: &[u8], + chunk: &Chunk, builder: &mut PacketBuilder, - ) -> Option<(u64, usize)> { - let mut header_len = 1 + Encoder::varint_len(offset) + 1; + ) -> (Option, Option) { + let mut header_len = 1 + Encoder::varint_len(chunk.offset()) + 1; // Don't bother if there isn't room for the header and some data. if builder.remaining() < header_len + 1 { - return None; + return (None, Some(chunk.into())); } // Calculate length of data based on the minimum of: // - available data // - remaining space, less the header, which counts only one byte for the length at // first to avoid underestimating length - let length = min(data.len(), builder.remaining() - header_len); + let length = min(chunk.len(), builder.remaining() - header_len); header_len += Encoder::varint_len(u64::try_from(length).unwrap()) - 1; - let length = min(data.len(), builder.remaining() - header_len); + let length = min(chunk.len(), builder.remaining() - header_len); builder.encode_varint(crate::frame::FRAME_TYPE_CRYPTO); - builder.encode_varint(offset); - builder.encode_vvec(&data[..length]); - Some((offset, length)) + builder.encode_varint(chunk.offset()); + builder.encode_vvec(&chunk.data()[..length]); + (Some(ChunkRange::new(chunk.offset(), length)), None) } fn mark_as_sent( cs: &mut CryptoStream, space: PacketNumberSpace, tokens: &mut Vec, - chunk: (u64, usize), + chunk: ChunkRange, stats: &mut FrameStats, ) { - let (offset, len) = chunk; + let (offset, len) = chunk.into(); cs.tx.mark_as_sent(offset, len); qdebug!("CRYPTO for {space} offset={offset}, len={len}"); tokens.push(RecoveryToken::Crypto(CryptoRecoveryToken { @@ -1526,50 +1529,67 @@ impl CryptoStreams { stats.crypto += 1; } + fn limit_chunks<'a>( + mut left: Chunk<'a>, + mut right: Chunk<'a>, + limit: usize, + ) -> (Chunk<'a>, Chunk<'a>) { + if left.len() + right.len() <= limit { + // Nothing to do. + } else if left.len() <= limit { + // `left` is short enough to fit into this packet entirely. So send the *end* of + // `right`, so that the second half of the SNI is in another packet. + (_, right) = right.split_at(right.len() + left.len() - limit); + } else if right.len() <= limit { + left.limit_to(limit - right.len()); + } else { + left.limit_to(limit / 2); + right.limit_to(limit / 2); + } + (left, right) + } + let cs = self.get_mut(space).unwrap(); - // Sending fresh data will always fill a packet. However, when there are - // lost packets, the outstanding CRYPTO chunks might not be contiguous - // and packet-filling. So we need to loop. - while let Some((offset, data)) = cs.tx.next_bytes() { - let written = if sni_slicing { - if let Some(sni) = find_sni(data) { - // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. + if let Some(chunk) = cs.tx.next_bytes() { + let chunk: Chunk = chunk.into(); + let packets_needed = chunk.len().div_ceil(builder.limit()); + let limit = chunk.len() / packets_needed; + let (written, skipped) = if sni_slicing { + // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. + if let Some(sni) = find_sni(chunk.data()) { let mid = sni.start + (sni.end - sni.start) / 2; - let (left, right) = data.split_at(mid); - // Figure out how many `limit`-sized packets the entire data - // would need. Then, set the per-packet limit so the data is - // spread over that number of packets and all of them end up - // zero-padded, which apparently helps with traversal of - // some middleboxes. This is ignores CRYPTO frame headers so - // undercounts a little, but that's fine. - // - // TODO: It would be slightly better if we managed to pad - // the first Initial with the minimum amount of zeros to - // help middlebox traversal, so the second packets had more - // space for coalescing. - let packets_needed = data.len().div_ceil(builder.limit()); - if packets_needed > 1 { - builder.set_limit(data.len() / packets_needed); - } - [ - write_chunk(offset + mid as u64, right, builder), - write_chunk(offset, left, builder), - ] + let (left, right) = chunk.split_at(mid); + let (left, right) = limit_chunks(left, right, limit); + let (written_right, skipped_right) = write_chunk(&right, builder); + let (written_left, skipped_left) = write_chunk(&left, builder); + ((written_left, written_right), (skipped_left, skipped_right)) } else { - // No SNI found, write the entire data. - [write_chunk(offset, data, builder), None] + // No SNI found. + let (written, skipped) = write_chunk(&chunk, builder); + ((written, None), (skipped, None)) } } else { - // Not at the start of the crypto stream, write the entire data. - [write_chunk(offset, data, builder), None] + // SNI slicing disabled. + let (written, skipped) = write_chunk(&chunk, builder); + ((written, None), (skipped, None)) }; + match skipped { + (None, None) => (), + (None, Some(chunk)) | (Some(chunk), None) => { + cs.tx.mark_as_lost(chunk.offset(), chunk.len()); + } + (Some(chunk1), Some(chunk2)) => { + cs.tx.mark_as_lost(chunk1.offset(), chunk1.len()); + cs.tx.mark_as_lost(chunk2.offset(), chunk2.len()); + } + } match written { - [None, None] => break, - [None, Some(chunk)] | [Some(chunk), None] => { + (None, None) => (), + (None, Some(chunk)) | (Some(chunk), None) => { mark_as_sent(cs, space, tokens, chunk, stats); } - [Some(chunk1), Some(chunk2)] => { + (Some(chunk1), Some(chunk2)) => { mark_as_sent(cs, space, tokens, chunk1, stats); mark_as_sent(cs, space, tokens, chunk2, stats); } From 59f910e87de36a5eeb5c55ff110d0a1732bf0bcc Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 31 Jan 2025 22:16:48 +0100 Subject: [PATCH 13/17] clippy --- neqo-common/src/chunk.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-common/src/chunk.rs b/neqo-common/src/chunk.rs index 9f7d7e8553..0a27b61c7a 100644 --- a/neqo-common/src/chunk.rs +++ b/neqo-common/src/chunk.rs @@ -42,7 +42,7 @@ impl Chunk<'_> { ) } - pub const fn limit_to(&mut self, limit: usize) { + pub fn limit_to(&mut self, limit: usize) { let (left, _) = self.data.split_at(limit); self.data = left; } From 0a078cbd1e9fe626713f42edccd90810fa8e87ce Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 31 Jan 2025 22:44:03 +0100 Subject: [PATCH 14/17] Minimize diff --- neqo-common/src/chunk.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/neqo-common/src/chunk.rs b/neqo-common/src/chunk.rs index 0a27b61c7a..0ed0aa688a 100644 --- a/neqo-common/src/chunk.rs +++ b/neqo-common/src/chunk.rs @@ -48,12 +48,6 @@ impl Chunk<'_> { } } -impl<'a> From> for (u64, usize) { - fn from(val: Chunk<'a>) -> Self { - (val.offset, val.data.len()) - } -} - impl<'a> From<(u64, &'a [u8])> for Chunk<'a> { fn from(value: (u64, &'a [u8])) -> Self { Chunk { From 703dfa812c6bd114237832d724618b478b54855d Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Sat, 1 Feb 2025 06:52:03 +0100 Subject: [PATCH 15/17] Minimize --- neqo-common/src/chunk.rs | 97 ---------------------------- neqo-common/src/lib.rs | 1 - neqo-transport/src/crypto.rs | 119 +++++++++++++++++------------------ 3 files changed, 59 insertions(+), 158 deletions(-) delete mode 100644 neqo-common/src/chunk.rs diff --git a/neqo-common/src/chunk.rs b/neqo-common/src/chunk.rs deleted file mode 100644 index 0ed0aa688a..0000000000 --- a/neqo-common/src/chunk.rs +++ /dev/null @@ -1,97 +0,0 @@ -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -#[derive(Debug)] -pub struct Chunk<'a> { - data: &'a [u8], - offset: u64, -} - -impl Chunk<'_> { - #[must_use] - pub const fn data(&self) -> &[u8] { - self.data - } - - #[must_use] - pub const fn offset(&self) -> u64 { - self.offset - } - - #[allow(clippy::len_without_is_empty)] - #[must_use] - pub const fn len(&self) -> usize { - self.data.len() - } - - #[must_use] - pub const fn split_at(&self, at: usize) -> (Self, Self) { - let (left, right) = self.data.split_at(at); - ( - Chunk { - data: left, - offset: self.offset, - }, - Chunk { - data: right, - offset: self.offset + at as u64, - }, - ) - } - - pub fn limit_to(&mut self, limit: usize) { - let (left, _) = self.data.split_at(limit); - self.data = left; - } -} - -impl<'a> From<(u64, &'a [u8])> for Chunk<'a> { - fn from(value: (u64, &'a [u8])) -> Self { - Chunk { - data: value.1, - offset: value.0, - } - } -} - -#[derive(Debug)] -pub struct ChunkRange { - offset: u64, - len: usize, -} - -impl ChunkRange { - #[must_use] - pub const fn new(offset: u64, len: usize) -> Self { - Self { offset, len } - } - - #[must_use] - pub const fn offset(&self) -> u64 { - self.offset - } - - #[allow(clippy::len_without_is_empty)] - #[must_use] - pub const fn len(&self) -> usize { - self.len - } -} - -impl From<&Chunk<'_>> for ChunkRange { - fn from(chunk: &Chunk<'_>) -> Self { - Self { - offset: chunk.offset, - len: chunk.len(), - } - } -} - -impl From for (u64, usize) { - fn from(val: ChunkRange) -> Self { - (val.offset, val.len) - } -} diff --git a/neqo-common/src/lib.rs b/neqo-common/src/lib.rs index ae7a5fee35..1e7a53ad19 100644 --- a/neqo-common/src/lib.rs +++ b/neqo-common/src/lib.rs @@ -6,7 +6,6 @@ #![allow(clippy::module_name_repetitions)] // This lint doesn't work here. -pub mod chunk; mod codec; mod datagram; pub mod event; diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 46f9022273..2246a7ac02 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -16,10 +16,7 @@ use std::{ time::Instant, }; -use neqo_common::{ - chunk::{Chunk, ChunkRange}, - hex, hex_snip_middle, qdebug, qinfo, qtrace, Encoder, Role, -}; +use neqo_common::{hex, hex_snip_middle, qdebug, qinfo, qtrace, Encoder, Role}; use neqo_crypto::{ hkdf, hp::HpKey, Aead, Agent, AntiReplay, Cipher, Epoch, Error as CryptoError, HandshakeState, PrivateKey, PublicKey, Record, RecordList, ResumptionToken, SymKey, ZeroRttChecker, @@ -1488,37 +1485,38 @@ impl CryptoStreams { stats: &mut FrameStats, ) { fn write_chunk( - chunk: &Chunk, + offset: u64, + data: &[u8], builder: &mut PacketBuilder, - ) -> (Option, Option) { - let mut header_len = 1 + Encoder::varint_len(chunk.offset()) + 1; + ) -> Option<(u64, usize)> { + let mut header_len = 1 + Encoder::varint_len(offset) + 1; // Don't bother if there isn't room for the header and some data. if builder.remaining() < header_len + 1 { - return (None, Some(chunk.into())); + return None; } // Calculate length of data based on the minimum of: // - available data // - remaining space, less the header, which counts only one byte for the length at // first to avoid underestimating length - let length = min(chunk.len(), builder.remaining() - header_len); + let length = min(data.len(), builder.remaining() - header_len); header_len += Encoder::varint_len(u64::try_from(length).unwrap()) - 1; - let length = min(chunk.len(), builder.remaining() - header_len); + let length = min(data.len(), builder.remaining() - header_len); builder.encode_varint(crate::frame::FRAME_TYPE_CRYPTO); - builder.encode_varint(chunk.offset()); - builder.encode_vvec(&chunk.data()[..length]); - (Some(ChunkRange::new(chunk.offset(), length)), None) + builder.encode_varint(offset); + builder.encode_vvec(&data[..length]); + Some((offset, length)) } fn mark_as_sent( cs: &mut CryptoStream, space: PacketNumberSpace, tokens: &mut Vec, - chunk: ChunkRange, + offset: u64, + len: usize, stats: &mut FrameStats, ) { - let (offset, len) = chunk.into(); cs.tx.mark_as_sent(offset, len); qdebug!("CRYPTO for {space} offset={offset}, len={len}"); tokens.push(RecoveryToken::Crypto(CryptoRecoveryToken { @@ -1529,69 +1527,70 @@ impl CryptoStreams { stats.crypto += 1; } - fn limit_chunks<'a>( - mut left: Chunk<'a>, - mut right: Chunk<'a>, + #[allow(clippy::type_complexity)] + const fn limit_chunks<'a>( + left: (u64, &'a [u8]), + right: (u64, &'a [u8]), limit: usize, - ) -> (Chunk<'a>, Chunk<'a>) { + ) -> ((u64, &'a [u8]), (u64, &'a [u8])) { + let (left_offset, mut left) = left; + let (mut right_offset, mut right) = right; if left.len() + right.len() <= limit { - // Nothing to do. + // Nothing to do. Both chunks will fit into one packet, meaning the SNI isn't spread + // over multiple packets. But at least it's in two unordered CRYPTO frames. } else if left.len() <= limit { - // `left` is short enough to fit into this packet entirely. So send the *end* of - // `right`, so that the second half of the SNI is in another packet. - (_, right) = right.split_at(right.len() + left.len() - limit); + // `left` is short enough to fit into this packet. So send from the *end* + // of `right`, so that the second half of the SNI is in another packet. + let right_len = right.len() + left.len() - limit; + right_offset += right_len as u64; + (_, right) = right.split_at(right_len); } else if right.len() <= limit { - left.limit_to(limit - right.len()); + // `right` is short enough to fit into this packet. So only send a part of `left`. + // The SNI begins at the end of `left`, so sent the beginnig of it in this packet. + (left, _) = left.split_at(limit - right.len()); } else { - left.limit_to(limit / 2); - right.limit_to(limit / 2); + // Both chunks are too long to fit into one packet. Just send a part of each. + let half_limit = limit / 2; + (left, _) = left.split_at(half_limit); + (right, _) = right.split_at(half_limit); } - (left, right) + ((left_offset, left), (right_offset, right)) } let cs = self.get_mut(space).unwrap(); - if let Some(chunk) = cs.tx.next_bytes() { - let chunk: Chunk = chunk.into(); - let packets_needed = chunk.len().div_ceil(builder.limit()); - let limit = chunk.len() / packets_needed; - let (written, skipped) = if sni_slicing { - // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. - if let Some(sni) = find_sni(chunk.data()) { + if let Some((offset, data)) = cs.tx.next_bytes() { + let written = if sni_slicing { + if let Some(sni) = find_sni(data) { + // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. let mid = sni.start + (sni.end - sni.start) / 2; - let (left, right) = chunk.split_at(mid); - let (left, right) = limit_chunks(left, right, limit); - let (written_right, skipped_right) = write_chunk(&right, builder); - let (written_left, skipped_left) = write_chunk(&left, builder); - ((written_left, written_right), (skipped_left, skipped_right)) + let (left, right) = data.split_at(mid); + + // Truncate the chunks so we can fit them into roughly evenly-filled packets. + let packets_needed = data.len().div_ceil(builder.limit()); + let limit = data.len() / packets_needed; + let ((left_offset, left), (right_offset, right)) = + limit_chunks((offset, left), (offset + mid as u64, right), limit); + ( + write_chunk(right_offset, right, builder), + write_chunk(left_offset, left, builder), + ) } else { - // No SNI found. - let (written, skipped) = write_chunk(&chunk, builder); - ((written, None), (skipped, None)) + // No SNI found, write the entire data. + (write_chunk(offset, data, builder), None) } } else { - // SNI slicing disabled. - let (written, skipped) = write_chunk(&chunk, builder); - ((written, None), (skipped, None)) + // SNI slicing disabled, write the entire data. + (write_chunk(offset, data, builder), None) }; - match skipped { - (None, None) => (), - (None, Some(chunk)) | (Some(chunk), None) => { - cs.tx.mark_as_lost(chunk.offset(), chunk.len()); - } - (Some(chunk1), Some(chunk2)) => { - cs.tx.mark_as_lost(chunk1.offset(), chunk1.len()); - cs.tx.mark_as_lost(chunk2.offset(), chunk2.len()); - } - } match written { (None, None) => (), - (None, Some(chunk)) | (Some(chunk), None) => { - mark_as_sent(cs, space, tokens, chunk, stats); + (None, Some((offset, len))) | (Some((offset, len)), None) => { + mark_as_sent(cs, space, tokens, offset, len, stats); } - (Some(chunk1), Some(chunk2)) => { - mark_as_sent(cs, space, tokens, chunk1, stats); - mark_as_sent(cs, space, tokens, chunk2, stats); + (Some((offset1, len1)), Some((offset2, len2))) => { + mark_as_sent(cs, space, tokens, offset1, len1, stats); + mark_as_sent(cs, space, tokens, offset2, len2, stats); } } } From f15cb683a59955dd4d336a2dbc86928473971b55 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Sat, 1 Feb 2025 07:04:28 +0100 Subject: [PATCH 16/17] clippy --- neqo-transport/src/crypto.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 2246a7ac02..fcb5233923 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -1475,7 +1475,6 @@ impl CryptoStreams { } } - #[allow(clippy::too_many_lines)] pub fn write_frame( &mut self, space: PacketNumberSpace, From 234c7f076bc0b4f7c719a960d6505cc76c28bd9e Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Sat, 1 Feb 2025 15:14:55 +0100 Subject: [PATCH 17/17] Comments --- neqo-transport/src/connection/mod.rs | 7 +++++-- neqo-transport/src/crypto.rs | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 35d2126fc5..c8eaefb3e5 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -2497,8 +2497,11 @@ impl Connection { self.discard_keys(PacketNumberSpace::Handshake, now); } - // If the client has more Initial CRYPTO data queued up, do not coalesce. - // FIXME: This is a temporary hack to avoid coalescing 0-RTT packets too early. + // If the client has more CRYPTO data queued up, do not coalesce if + // this packet is an Initial. Without this, 0-RTT packets could be + // coalesced with the first Initial, which some server (e.g., ours) + // do not support, because may do not save packets they can't + // decrypt yet. if self.role == Role::Client && *space == PacketNumberSpace::Initial && !self.crypto.streams.is_empty(*space) diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index fcb5233923..5f9701301e 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -1545,7 +1545,7 @@ impl CryptoStreams { (_, right) = right.split_at(right_len); } else if right.len() <= limit { // `right` is short enough to fit into this packet. So only send a part of `left`. - // The SNI begins at the end of `left`, so sent the beginnig of it in this packet. + // The SNI begins at the end of `left`, so send the beginnig of it in this packet. (left, _) = left.split_at(limit - right.len()); } else { // Both chunks are too long to fit into one packet. Just send a part of each.