diff --git a/dc/s2n-quic-dc/src/psk/io.rs b/dc/s2n-quic-dc/src/psk/io.rs index c3c33cebdb..1541ce3241 100644 --- a/dc/s2n-quic-dc/src/psk/io.rs +++ b/dc/s2n-quic-dc/src/psk/io.rs @@ -12,7 +12,7 @@ use s2n_quic::{ }, server::Name, }; -use s2n_quic_core::{endpoint::Type, inet::SocketAddress}; +use s2n_quic_core::{connection, endpoint::Type, inet::SocketAddress}; use std::{ any::Any, hash::BuildHasher, @@ -37,6 +37,16 @@ pub const DEFAULT_MTU: u16 = DEFAULT_BASE_MTU; pub const DEFAULT_PTO_JITTER_PERCENTAGE: u8 = 33; const DEFAULT_INITIAL_RTT: Duration = Duration::from_millis(1); const DC_QUIC_VERSION: u32 = 0; +/// Application error codes the client uses to close a connection whose dcQUIC handshake did not +/// complete. Both must be non-zero so the close is emitted as an application `CONNECTION_CLOSE` +/// rather than the clean, no-error close produced by dropping the connection handle. +/// Distinct codes let the peer/operator tell the two failure modes apart. +/// +/// `ConfirmComplete::wait_ready` reported an error before the dc handshake completed. +const DC_HANDSHAKE_INCOMPLETE_ERROR: u32 = 1; +/// `ConfirmComplete::wait_ready` did not resolve before the handshake deadline elapsed. +const DC_HANDSHAKE_TIMEOUT_ERROR: u32 = 2; + /// Number of threads used to make progress on the TLS handshake pub const DEFAULT_THREAD_COUNT: usize = 0; @@ -97,7 +107,6 @@ impl s2n_quic::provider::tls::offload::ExporterHandler for DCExporter { )) } } - pub struct Server { server: s2n_quic::Server, } @@ -247,9 +256,37 @@ pub(super) async fn server< // ConnectionClose from the client is lost. This timeout covers both the dc handshake // confirmation and MTU probing completion. let result = tokio::time::timeout(Duration::from_secs(10), async { - // FIXME: add more logging information if the subscriber is not registered with the endpoint. - if ConfirmComplete::wait_ready(&mut connection).await.is_ok() { - MtuConfirmComplete::wait_ready(&mut connection).await; + match ConfirmComplete::wait_ready(&mut connection).await { + Ok(()) => { + MtuConfirmComplete::wait_ready(&mut connection).await; + } + Err(error) => { + // The dc handshake did not complete before the connection closed. Surface + // the reason so it is observable. The dc application error codes the client + // sends on its failure paths would otherwise render as a bare integer, so + // translate them into a readable reason. + let reason = match error + .get_ref() + .and_then(|inner| inner.downcast_ref::()) + { + Some(connection::Error::Application { error: code, .. }) + if *code == DC_HANDSHAKE_INCOMPLETE_ERROR.into() => + { + "peer reported that its dc handshake did not complete" + } + Some(connection::Error::Application { error: code, .. }) + if *code == DC_HANDSHAKE_TIMEOUT_ERROR.into() => + { + "peer reported that its dc handshake timed out" + } + _ => "connection closed before the dc handshake completed", + }; + tracing::debug!( + peer_address = ?connection.remote_addr().ok(), + error = %error, + "{reason}" + ); + } } }) .await; @@ -534,10 +571,30 @@ impl HandshakeQueue { } Ok(Err(e)) => { // ConfirmComplete::wait_ready failed. We should treat the handshake as failed. + // + // Explicitly close instead of letting `connection` drop, which would emit a + // clean (no-error) CONNECTION_CLOSE. A clean close is the signal the server + // uses to complete the dc handshake when the token ACK is lost; since the + // handshake did not complete here, we must not send it. Any explicit close is + // emitted as an application CONNECTION_CLOSE (`connection::Error::Application`), + // which the server does not treat as completion. If the connection is already + // closed this is a no-op. + // + // This is safe to deploy ahead of the corresponding server-side change: a server that predates it + // has no close-based dc completion (it completes only when its own `DC_STATELESS_RESET_TOKENS` are + // acknowledged), so this application close neither completes nor harms it. On a failed handshake + // such a server correctly stays incomplete, and its `ConfirmComplete` observes the application + // error as a failure instead of the false success a clean close would have produced. + // Both sides therefore agree the handshake did not complete. + connection.close(DC_HANDSHAKE_INCOMPLETE_ERROR.into()); return Err(e); } Err(_elapsed) => { // Handshake timeout occurred. We should treat the handshake as failed. + // + // Close with an explicit error, as in the failure case above, but with a + // distinct code so a timeout can be distinguished from other failures. + connection.close(DC_HANDSHAKE_TIMEOUT_ERROR.into()); return Err(io::Error::new( io::ErrorKind::TimedOut, "ConfirmComplete handshake timeout", diff --git a/quic/s2n-quic-tests/src/tests/dc.rs b/quic/s2n-quic-tests/src/tests/dc.rs index 2d56fa9a3a..39c1acf3a3 100644 --- a/quic/s2n-quic-tests/src/tests/dc.rs +++ b/quic/s2n-quic-tests/src/tests/dc.rs @@ -26,7 +26,7 @@ use s2n_quic_core::{ Timestamp, }, frame::ConnectionClose, - packet::interceptor::{Datagram, Interceptor}, + packet::interceptor::{Datagram, Interceptor, Packet}, stateless_reset::{ self, token::testing::{TEST_TOKEN_1, TEST_TOKEN_2}, @@ -1434,3 +1434,365 @@ impl ExporterHandler for Exporter { )) } } + +/// Drives a dc handshake where the server's standalone token ACKs are neutralized by a +/// packet interceptor, then has the client close and linger so the server can only reach `Complete` +/// via the token ACK that rides on the client's close. +/// +/// The server MTU is pinned so it never probes, because its ACKs dropped it couldn't drive an +/// MTU search anyway, while the client probes normally against the un-intercepted server->client path. +/// +/// Returns the client and server `DcRecorder`s so the caller can assert on dc state. +#[track_caller] +fn dc_completes_through_close( + server: server::Builder, + client: client::Builder, + client_closing: Arc, + client_linger: Duration, + packet_snapshots: (PacketSnapshot, PacketSnapshot), +) -> (DcRecorder, DcRecorder) { + let model = Model::default(); + let rtt = Duration::from_millis(100); + model.set_delay(rtt / 2); + + // Pin the server's MTU to prevent it from probing. Since we are dropping all ACKs for the + // server, the server can't perform MTU probing. + const SERVER_PINNED_MTU: u16 = 1500; + + let (server_packet_snapshot, client_packet_snapshot) = packet_snapshots; + + let server_subscriber = DcRecorder::new(); + let server_events = server_subscriber.clone(); + let client_subscriber = DcRecorder::new(); + let client_events = client_subscriber.clone(); + + test(model.clone(), |handle| { + let server_event = ( + (dc::ConfirmComplete, dc::MtuConfirmComplete), + ( + (tracing_events(false, model.clone()), server_packet_snapshot), + server_subscriber, + ), + ); + + let mut server = server + .with_io( + handle + .builder() + .with_max_mtu(SERVER_PINNED_MTU) + .with_base_mtu(SERVER_PINNED_MTU) + .with_initial_mtu(SERVER_PINNED_MTU) + .build()?, + )? + .with_event(server_event)? + .with_random(Random::with_seed(456))? + .start()?; + + let addr = server.local_addr()?; + + spawn(async move { + if let Some(mut conn) = server.accept().await { + // Mirror the real dc server: wait for the dc handshake, then MTU probing. + // Reaching `Complete` under this interception can only happen via the ACK + // bundled onto the client's close. + let result = dc::ConfirmComplete::wait_ready(&mut conn).await; + assert!( + result.is_ok(), + "server dc handshake did not complete: {result:?}" + ); + dc::MtuConfirmComplete::wait_ready(&mut conn).await; + } + }); + + let client_event = ( + (dc::ConfirmComplete, dc::MtuConfirmComplete), + ( + ( + (tracing_events(false, model.clone()), client_packet_snapshot), + client_subscriber, + ), + // Flips `client_closing` the moment the client begins closing. + ClientCloseWatcher(client_closing.clone()), + ), + ); + + let client = client + .with_io(handle.builder().build().unwrap())? + .with_event(client_event)? + .with_random(Random::with_seed(456))? + .start()?; + + primary::spawn(async move { + let connect = Connect::new(addr) + .with_server_name("localhost") + .with_deduplicate(true); + let mut conn = client.connect(connect).await.unwrap(); + // Mirror the real dc client: wait for BOTH the dc handshake and MTU probing + // before closing. The client closes once its own MTU search completes and it + // has the server's tokens, which is exactly the timing that triggers the bug. + dc::ConfirmComplete::wait_ready(&mut conn).await.unwrap(); + dc::MtuConfirmComplete::wait_ready(&mut conn).await; + // Closing here sends a CONNECTION_CLOSE carrying the pending token ACK. + drop(conn); + // Keep this (primary) task alive so the simulation continues running long + // enough for the close (or its retransmission) to reach the server. + delay(client_linger).await; + }); + + Ok(addr) + }) + .unwrap(); + + (client_events, server_events) +} + +// dcQUIC endpoints to drop all ACKs to see if dc states will reach complete +#[test] +fn dc_handshake_completes_when_all_acks_are_dropped() -> Result<()> { + let server = Server::builder() + .with_tls((certificates::CERT_PKCS1_PEM, certificates::KEY_PKCS1_PEM))? + .with_dc(MockDcEndpoint::new(&SERVER_TOKENS))? + .with_packet_interceptor(DropClientStandaloneAcks)?; + let client = Client::builder() + .with_tls(certificates::CERT_PKCS1_PEM)? + .with_dc(MockDcEndpoint::new(&CLIENT_TOKENS))?; + + // Even though every standalone ACK is dropped, the server reaches Complete when it processes + // the client's clean CONNECTION_CLOSE: a no-error close means the client finished the dc + // handshake, which it only does after receiving the server's tokens. A short linger is enough + // for that single close to arrive. This test doesn't drop the close, so the close-watcher flag + // is unused. + let (client_events, server_events) = dc_completes_through_close( + server, + client, + Default::default(), + Duration::from_millis(300), + ( + PacketSnapshot::named_snapshot( + "dc_handshake_completes_when_all_acks_are_dropped__server", + ), + PacketSnapshot::named_snapshot( + "dc_handshake_completes_when_all_acks_are_dropped__client", + ), + ), + ); + + assert_dc_complete( + &client_events + .dc_state_changed_events() + .lock() + .unwrap() + .clone(), + ); + assert_dc_complete( + &server_events + .dc_state_changed_events() + .lock() + .unwrap() + .clone(), + ); + + Ok(()) +} + +/// Server-side interceptor that neutralizes the client's standalone ACKs in the application space. +struct DropClientStandaloneAcks; + +impl Interceptor for DropClientStandaloneAcks { + #[inline] + fn intercept_rx_payload<'a>( + &mut self, + _subject: &Subject, + packet: &Packet, + payload: DecoderBufferMut<'a>, + ) -> DecoderBufferMut<'a> { + if !packet.number.space().is_application_data() { + return payload; + } + + let bytes = payload.into_less_safe_slice(); + + if !is_standalone_ack(bytes) { + return DecoderBufferMut::new(bytes); + } + + // Neutralize to a single PADDING frame. + bytes[0] = 0; + DecoderBufferMut::new(&mut bytes[..1]) + } +} + +/// Returns true if the payload carries an ACK frame and does not carry the client's tokens or a CONNECTION_CLOSE. +/// We only want to drop a standalone ACK. +fn is_standalone_ack(bytes: &mut [u8]) -> bool { + use s2n_quic_core::frame::{Frame as CoreFrame, FrameMut}; + + let mut has_ack = false; + let mut buffer = DecoderBufferMut::new(bytes); + while !buffer.is_empty() { + match buffer.decode::() { + Ok((frame, remaining)) => { + match frame { + // Never drop the packets the server needs to make progress or complete. + CoreFrame::DcStatelessResetTokens(_) | CoreFrame::ConnectionClose(_) => { + return false + } + CoreFrame::Ack(_) => has_ack = true, + _ => {} + } + buffer = remaining; + } + // If it does not parse cleanly, leave it untouched. + Err(_) => return false, + } + } + has_ack +} + +// Verify if CONNECTION_CLOSE got dropped, dcQUIC endpoints can reach complete +#[test] +fn dc_handshake_completes_when_all_acks_and_first_close_is_dropped() -> Result<()> { + use std::sync::atomic::AtomicBool; + + // Shared across the client's close watcher and the server's interceptor. + let client_closing: Arc = Default::default(); + let close_dropped: Arc = Default::default(); + + let server = Server::builder() + .with_tls((certificates::CERT_PKCS1_PEM, certificates::KEY_PKCS1_PEM))? + .with_dc(MockDcEndpoint::new(&SERVER_TOKENS))? + .with_packet_interceptor(DropAcksAndFirstClose { + client_closing: client_closing.clone(), + close_dropped: close_dropped.clone(), + seen_while_closing: Vec::new(), + })?; + let client = Client::builder() + .with_tls(certificates::CERT_PKCS1_PEM)? + .with_dc(MockDcEndpoint::new(&CLIENT_TOKENS))?; + + let (client_events, server_events) = dc_completes_through_close( + server, + client, + client_closing, + Duration::from_secs(3), + ( + PacketSnapshot::named_snapshot( + "dc_handshake_completes_when_all_acks_and_first_close_is_dropped__server", + ), + PacketSnapshot::named_snapshot( + "dc_handshake_completes_when_all_acks_and_first_close_is_dropped__client", + ), + ), + ); + + assert!( + close_dropped.load(Ordering::Relaxed), + "no close was dropped and redelivered, so retransmission wasn't exercised" + ); + + assert_dc_complete( + &client_events + .dc_state_changed_events() + .lock() + .unwrap() + .clone(), + ); + assert_dc_complete( + &server_events + .dc_state_changed_events() + .lock() + .unwrap() + .clone(), + ); + + Ok(()) +} + +/// Client-side event subscriber that flips a shared flag when the client begins closing. +struct ClientCloseWatcher(Arc); + +impl events::Subscriber for ClientCloseWatcher { + type ConnectionContext = (); + + fn create_connection_context( + &mut self, + _meta: &events::ConnectionMeta, + _info: &events::ConnectionInfo, + ) -> Self::ConnectionContext { + } + + fn on_connection_closed( + &mut self, + _context: &mut Self::ConnectionContext, + _meta: &events::ConnectionMeta, + _event: &events::ConnectionClosed, + ) { + self.0.store(true, std::sync::atomic::Ordering::Relaxed); + } +} + +struct DropAcksAndFirstClose { + client_closing: Arc, + /// Set once a byte-identical retransmission is allowed through, i.e. once a dropped close + /// has been redelivered. Proves the retransmission path was actually exercised. + close_dropped: Arc, + /// Distinct datagrams already seen while closing, used to tell a first transmission (which + /// is dropped) from a retransmission (which is allowed). + seen_while_closing: Vec>, +} + +impl Interceptor for DropAcksAndFirstClose { + fn intercept_rx_datagram<'a>( + &mut self, + _subject: &Subject, + _datagram: &Datagram, + payload: DecoderBufferMut<'a>, + ) -> DecoderBufferMut<'a> { + use std::sync::atomic::Ordering::Relaxed; + + // Before the client starts closing, everything is ordinary handshake/data traffic that + // must be delivered untouched. + if !self.client_closing.load(Relaxed) { + return payload; + } + + let bytes = payload.into_less_safe_slice(); + + if self + .seen_while_closing + .iter() + .any(|seen| seen[..] == *bytes) + { + // A byte-identical repeat: this is a retransmitted close. Let it through and record + // that a dropped close was successfully redelivered. + self.close_dropped.store(true, Relaxed); + return DecoderBufferMut::new(bytes); + } + + // First time we've seen this datagram while closing (a pre-close straggler or the first + // CONNECTION_CLOSE). Drop it at the datagram level so its packet number is never recorded + // and the retransmission is processed rather than discarded as a duplicate. + self.seen_while_closing.push(bytes.to_vec()); + DecoderBufferMut::new(&mut bytes[..0]) + } + + #[inline] + fn intercept_rx_payload<'a>( + &mut self, + _subject: &Subject, + packet: &Packet, + payload: DecoderBufferMut<'a>, + ) -> DecoderBufferMut<'a> { + if !packet.number.space().is_application_data() { + return payload; + } + + let bytes = payload.into_less_safe_slice(); + if !is_standalone_ack(bytes) { + return DecoderBufferMut::new(bytes); + } + + bytes[0] = 0; + DecoderBufferMut::new(&mut bytes[..1]) + } +} diff --git a/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_and_first_close_is_dropped__client.snap b/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_and_first_close_is_dropped__client.snap new file mode 100644 index 0000000000..65528b05ff --- /dev/null +++ b/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_and_first_close_is_dropped__client.snap @@ -0,0 +1,78 @@ +--- +source: quic/s2n-quic-core/src/event/snapshot.rs +input_file: quic/s2n-quic-tests/src/tests/dc.rs +--- + milli.micro | datagrams (D) contain packets (P) which contain frames (F) + + + 0.1 > D len=1200 + 0.1 > P Initial(0) len=1200 + 0.1 > F Initial(0) CRYPTO(off=0, len=290) + 0.1 > F Initial(0) PADDING(len=870) + + 100. < D len=1480 + 100. < P Initial(0) len=200 + 100. < F Initial(0) ACK + 100. < F Initial(0) CRYPTO(off=0, len=130) + 100. < P Handshake(0) len=1280 + 100. < F Handshake(0) CRYPTO(off=0, len=1220) + 100. < D len=300 + 100. < P Handshake(1) len=250 + 100. < F Handshake(1) CRYPTO(off=1220, len=190) + 100. < P OneRtt(0) len=60 + 100. < F OneRtt(0) MTU_PROBING_COMPLETE(mtu=1472) + 100. < F OneRtt(0) PADDING(len=20) + 100. > D len=1200 + 100. > P Initial(1) len=80 + 100. > F Initial(1) ACK + 100. > F Initial(1) PADDING(len=20) + 100. > P Handshake(0) len=110 + 100. > F Handshake(0) ACK + 100. > F Handshake(0) CRYPTO(off=0, len=40) + 100. > P OneRtt(0) len=1020 + 100. > F OneRtt(0) ACK + 100. > F OneRtt(0) DC_STATELESS_RESET_TOKENS + 100. > F OneRtt(0) PADDING(len=960) + + 200. < D len=180 + 200. < P OneRtt(1) len=180 + 200. < F OneRtt(1) ACK + 200. < F OneRtt(1) HANDSHAKE_DONE + 200. < F OneRtt(1) DC_STATELESS_RESET_TOKENS + 200. | dc_state_changed=Complete + 200. < F OneRtt(1) NEW_CONNECTION_ID + 200. < F OneRtt(1) NEW_CONNECTION_ID + 200. < F OneRtt(1) NEW_CONNECTION_ID + 200. > D len=1480 + 200. > P OneRtt(1) len=1480 + 200. > F OneRtt(1) PING + 200. > F OneRtt(1) PADDING(len=1440) + 200. > D len=160 + 200. > P OneRtt(2) len=160 + 200. > F OneRtt(2) ACK + 200. > F OneRtt(2) NEW_CONNECTION_ID + 200. > F OneRtt(2) NEW_CONNECTION_ID + 200. > F OneRtt(2) NEW_CONNECTION_ID + 200. > F OneRtt(2) RETIRE_CONNECTION_ID + + 325. < D len=70 + 325. < P OneRtt(2) len=70 + 325. < F OneRtt(2) ACK + 325. < F OneRtt(2) HANDSHAKE_DONE + 325. < F OneRtt(2) DC_STATELESS_RESET_TOKENS + 325. > D len=60 + 325. > P OneRtt(3) len=60 + 325. > F OneRtt(3) ACK + 325. > F OneRtt(3) MTU_PROBING_COMPLETE(mtu=1472) + 325. > F OneRtt(3) PADDING(len=10) + + 325. > D len=60 + 325. > P OneRtt(4) len=60 + 325. > F OneRtt(4) CONNECTION_CLOSE + 325. > F OneRtt(4) PADDING(len=20) + + + 600. < D len=70 + 600. < D len=70 + + 725. > D len=60 diff --git a/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_and_first_close_is_dropped__server.snap b/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_and_first_close_is_dropped__server.snap new file mode 100644 index 0000000000..81d97e1f72 --- /dev/null +++ b/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_and_first_close_is_dropped__server.snap @@ -0,0 +1,73 @@ +--- +source: quic/s2n-quic-core/src/event/snapshot.rs +input_file: quic/s2n-quic-tests/src/tests/dc.rs +--- + milli.micro | datagrams (D) contain packets (P) which contain frames (F) + + + 50. < D len=1200 + 50. < F Initial(0) CRYPTO(off=0, len=290) + 50. < F Initial(0) PADDING(len=870) + 50. > D len=1480 + 50. > P Initial(0) len=200 + 50. > F Initial(0) ACK + 50. > F Initial(0) CRYPTO(off=0, len=130) + 50. > P Handshake(0) len=1280 + 50. > F Handshake(0) CRYPTO(off=0, len=1220) + 50. > D len=300 + 50. > P Handshake(1) len=250 + 50. > F Handshake(1) CRYPTO(off=1220, len=190) + 50. > P OneRtt(0) len=60 + 50. > F OneRtt(0) MTU_PROBING_COMPLETE(mtu=1472) + 50. > F OneRtt(0) PADDING(len=20) + + 150. < D len=1200 + 150. < P Initial(1) len=80 + 150. < F Initial(1) ACK + 150. < F Initial(1) PADDING(len=20) + 150. < P Handshake(0) len=110 + 150. < F Handshake(0) ACK + 150. < F Handshake(0) CRYPTO(off=0, len=40) + 150. < P OneRtt(0) len=1020 + 150. < F OneRtt(0) ACK + 150. < F OneRtt(0) DC_STATELESS_RESET_TOKENS + 150. < F OneRtt(0) PADDING(len=960) + 150. > D len=180 + 150. > P OneRtt(1) len=180 + 150. > F OneRtt(1) ACK + 150. > F OneRtt(1) HANDSHAKE_DONE + 150. > F OneRtt(1) DC_STATELESS_RESET_TOKENS + 150. > F OneRtt(1) NEW_CONNECTION_ID + 150. > F OneRtt(1) NEW_CONNECTION_ID + 150. > F OneRtt(1) NEW_CONNECTION_ID + + 250. < D len=1480 + 250. < P OneRtt(1) len=1480 + 250. < F OneRtt(1) PING + 250. < F OneRtt(1) PADDING(len=1440) + 250. < D len=160 + 250. < P OneRtt(2) len=160 + 250. < F OneRtt(2) PADDING(len=10) + + 275. > D len=70 + 275. > P OneRtt(2) len=70 + 275. > F OneRtt(2) ACK + 275. > F OneRtt(2) HANDSHAKE_DONE + 275. > F OneRtt(2) DC_STATELESS_RESET_TOKENS + + + 550. > D len=70 + 550. > P OneRtt(4) len=70 + 550. > F OneRtt(4) ACK + 550. > F OneRtt(4) HANDSHAKE_DONE + 550. > F OneRtt(4) DC_STATELESS_RESET_TOKENS + 550. > D len=70 + 550. > P OneRtt(6) len=70 + 550. > F OneRtt(6) ACK + 550. > F OneRtt(6) HANDSHAKE_DONE + 550. > F OneRtt(6) DC_STATELESS_RESET_TOKENS + + 775. < D len=60 + 775. < P OneRtt(4) len=60 + 775. < F OneRtt(4) CONNECTION_CLOSE + 775. | dc_state_changed=Complete diff --git a/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_are_dropped__client.snap b/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_are_dropped__client.snap new file mode 100644 index 0000000000..4f5b8c10e0 --- /dev/null +++ b/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_are_dropped__client.snap @@ -0,0 +1,72 @@ +--- +source: quic/s2n-quic-core/src/event/snapshot.rs +input_file: quic/s2n-quic-tests/src/tests/dc.rs +--- + milli.micro | datagrams (D) contain packets (P) which contain frames (F) + + + 0.1 > D len=1200 + 0.1 > P Initial(0) len=1200 + 0.1 > F Initial(0) CRYPTO(off=0, len=290) + 0.1 > F Initial(0) PADDING(len=870) + + 100. < D len=1480 + 100. < P Initial(0) len=200 + 100. < F Initial(0) ACK + 100. < F Initial(0) CRYPTO(off=0, len=130) + 100. < P Handshake(0) len=1280 + 100. < F Handshake(0) CRYPTO(off=0, len=1220) + 100. < D len=300 + 100. < P Handshake(1) len=250 + 100. < F Handshake(1) CRYPTO(off=1220, len=190) + 100. < P OneRtt(0) len=60 + 100. < F OneRtt(0) MTU_PROBING_COMPLETE(mtu=1472) + 100. < F OneRtt(0) PADDING(len=20) + 100. > D len=1200 + 100. > P Initial(1) len=80 + 100. > F Initial(1) ACK + 100. > F Initial(1) PADDING(len=20) + 100. > P Handshake(0) len=110 + 100. > F Handshake(0) ACK + 100. > F Handshake(0) CRYPTO(off=0, len=40) + 100. > P OneRtt(0) len=1020 + 100. > F OneRtt(0) ACK + 100. > F OneRtt(0) DC_STATELESS_RESET_TOKENS + 100. > F OneRtt(0) PADDING(len=960) + + 200. < D len=180 + 200. < P OneRtt(1) len=180 + 200. < F OneRtt(1) ACK + 200. < F OneRtt(1) HANDSHAKE_DONE + 200. < F OneRtt(1) DC_STATELESS_RESET_TOKENS + 200. | dc_state_changed=Complete + 200. < F OneRtt(1) NEW_CONNECTION_ID + 200. < F OneRtt(1) NEW_CONNECTION_ID + 200. < F OneRtt(1) NEW_CONNECTION_ID + 200. > D len=1480 + 200. > P OneRtt(1) len=1480 + 200. > F OneRtt(1) PING + 200. > F OneRtt(1) PADDING(len=1440) + 200. > D len=160 + 200. > P OneRtt(2) len=160 + 200. > F OneRtt(2) ACK + 200. > F OneRtt(2) NEW_CONNECTION_ID + 200. > F OneRtt(2) NEW_CONNECTION_ID + 200. > F OneRtt(2) NEW_CONNECTION_ID + 200. > F OneRtt(2) RETIRE_CONNECTION_ID + + 325. < D len=70 + 325. < P OneRtt(2) len=70 + 325. < F OneRtt(2) ACK + 325. < F OneRtt(2) HANDSHAKE_DONE + 325. < F OneRtt(2) DC_STATELESS_RESET_TOKENS + 325. > D len=60 + 325. > P OneRtt(3) len=60 + 325. > F OneRtt(3) ACK + 325. > F OneRtt(3) MTU_PROBING_COMPLETE(mtu=1472) + 325. > F OneRtt(3) PADDING(len=10) + + 325. > D len=60 + 325. > P OneRtt(4) len=60 + 325. > F OneRtt(4) CONNECTION_CLOSE + 325. > F OneRtt(4) PADDING(len=20) diff --git a/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_are_dropped__server.snap b/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_are_dropped__server.snap new file mode 100644 index 0000000000..076a9919e5 --- /dev/null +++ b/quic/s2n-quic-tests/src/tests/snapshots/dc_handshake_completes_when_all_acks_are_dropped__server.snap @@ -0,0 +1,64 @@ +--- +source: quic/s2n-quic-core/src/event/snapshot.rs +input_file: quic/s2n-quic-tests/src/tests/dc.rs +--- + milli.micro | datagrams (D) contain packets (P) which contain frames (F) + + + 50. < D len=1200 + 50. < F Initial(0) CRYPTO(off=0, len=290) + 50. < F Initial(0) PADDING(len=870) + 50. > D len=1480 + 50. > P Initial(0) len=200 + 50. > F Initial(0) ACK + 50. > F Initial(0) CRYPTO(off=0, len=130) + 50. > P Handshake(0) len=1280 + 50. > F Handshake(0) CRYPTO(off=0, len=1220) + 50. > D len=300 + 50. > P Handshake(1) len=250 + 50. > F Handshake(1) CRYPTO(off=1220, len=190) + 50. > P OneRtt(0) len=60 + 50. > F OneRtt(0) MTU_PROBING_COMPLETE(mtu=1472) + 50. > F OneRtt(0) PADDING(len=20) + + 150. < D len=1200 + 150. < P Initial(1) len=80 + 150. < F Initial(1) ACK + 150. < F Initial(1) PADDING(len=20) + 150. < P Handshake(0) len=110 + 150. < F Handshake(0) ACK + 150. < F Handshake(0) CRYPTO(off=0, len=40) + 150. < P OneRtt(0) len=1020 + 150. < F OneRtt(0) ACK + 150. < F OneRtt(0) DC_STATELESS_RESET_TOKENS + 150. < F OneRtt(0) PADDING(len=960) + 150. > D len=180 + 150. > P OneRtt(1) len=180 + 150. > F OneRtt(1) ACK + 150. > F OneRtt(1) HANDSHAKE_DONE + 150. > F OneRtt(1) DC_STATELESS_RESET_TOKENS + 150. > F OneRtt(1) NEW_CONNECTION_ID + 150. > F OneRtt(1) NEW_CONNECTION_ID + 150. > F OneRtt(1) NEW_CONNECTION_ID + + 250. < D len=1480 + 250. < P OneRtt(1) len=1480 + 250. < F OneRtt(1) PING + 250. < F OneRtt(1) PADDING(len=1440) + 250. < D len=160 + 250. < P OneRtt(2) len=160 + 250. < F OneRtt(2) PADDING(len=10) + + 275. > D len=70 + 275. > P OneRtt(2) len=70 + 275. > F OneRtt(2) ACK + 275. > F OneRtt(2) HANDSHAKE_DONE + 275. > F OneRtt(2) DC_STATELESS_RESET_TOKENS + + 375. < D len=60 + 375. < P OneRtt(3) len=60 + 375. < F OneRtt(3) PADDING(len=10) + 375. < D len=60 + 375. < P OneRtt(4) len=60 + 375. < F OneRtt(4) CONNECTION_CLOSE + 375. | dc_state_changed=Complete diff --git a/quic/s2n-quic-transport/src/connection/connection_impl.rs b/quic/s2n-quic-transport/src/connection/connection_impl.rs index 9b7d3199d6..221c9aaba2 100644 --- a/quic/s2n-quic-transport/src/connection/connection_impl.rs +++ b/quic/s2n-quic-transport/src/connection/connection_impl.rs @@ -872,9 +872,13 @@ impl connection::Trait for ConnectionImpl { if let Some((space, _)) = self.space_manager.application_mut() { let closed_without_error = matches!(error, connection::Error::Closed { .. }); + let peer_initiated = matches!( + error, + connection::Error::Closed { initiator, .. } if initiator.is_remote() + ); space .dc_manager - .on_close(closed_without_error, &mut publisher); + .on_close(closed_without_error, peer_initiated, &mut publisher); } publisher.on_connection_closed(event::builder::ConnectionClosed { error }); diff --git a/quic/s2n-quic-transport/src/dc/manager.rs b/quic/s2n-quic-transport/src/dc/manager.rs index 985272fd2a..3b7b6806a4 100644 --- a/quic/s2n-quic-transport/src/dc/manager.rs +++ b/quic/s2n-quic-transport/src/dc/manager.rs @@ -66,6 +66,7 @@ impl State { ServerPathSecretsReady => ServerTokensSent ); on_stateless_reset_tokens_acked(ServerTokensSent => Complete); + on_peer_clean_close(ServerTokensSent => Complete); } } @@ -240,14 +241,30 @@ impl Manager { /// so this event is intentionally limited to the silent case: the QUIC handshake completed /// and the connection closed without an error, yet the dc state never /// reached `Complete` and nothing else signals that anything went wrong. + /// + /// A no-error `CONNECTION_CLOSE` sent by the peer means the client finished the dc handshake, + /// which it only does after it has received the server's `DC_STATELESS_RESET_TOKENS`. So if + /// the server has sent its tokens and is only waiting on the acknowledgement, a clean close from + /// the peer confirms the tokens were received and the server can transition to `Complete`. pub fn on_close( &mut self, closed_without_error: bool, + peer_initiated: bool, publisher: &mut Pub, ) { ensure!(closed_without_error); ensure!(!self.state.is_complete()); + // A clean close from the peer confirms it completed the handshake and therefore received + // the server's tokens, so the server can complete even if the token ACK never arrived. + if peer_initiated && self.state.on_peer_clean_close().is_ok() { + self.path.on_dc_handshake_complete(); + publisher.on_dc_state_changed(DcStateChanged { + state: DcState::Complete, + }); + return; + } + publisher.on_dc_state_incomplete(DcStateIncomplete { state: (&self.state).into_event(), }); diff --git a/quic/s2n-quic-transport/src/dc/manager/snapshots/s2n_quic_transport__dc__manager__tests__dot_test.snap b/quic/s2n-quic-transport/src/dc/manager/snapshots/s2n_quic_transport__dc__manager__tests__dot_test.snap index 3e835a94ee..a93ede9d6e 100644 --- a/quic/s2n-quic-transport/src/dc/manager/snapshots/s2n_quic_transport__dc__manager__tests__dot_test.snap +++ b/quic/s2n-quic-transport/src/dc/manager/snapshots/s2n_quic_transport__dc__manager__tests__dot_test.snap @@ -15,4 +15,5 @@ digraph { ClientPathSecretsReady -> Complete [label = "on_peer_stateless_reset_tokens"]; ServerPathSecretsReady -> ServerTokensSent [label = "on_peer_stateless_reset_tokens"]; ServerTokensSent -> Complete [label = "on_stateless_reset_tokens_acked"]; + ServerTokensSent -> Complete [label = "on_peer_clean_close"]; } diff --git a/quic/s2n-quic-transport/src/dc/manager/snapshots/s2n_quic_transport__dc__manager__tests__snapshots.snap b/quic/s2n-quic-transport/src/dc/manager/snapshots/s2n_quic_transport__dc__manager__tests__snapshots.snap index ca4a3aba5a..cb43cd81a3 100644 --- a/quic/s2n-quic-transport/src/dc/manager/snapshots/s2n_quic_transport__dc__manager__tests__snapshots.snap +++ b/quic/s2n-quic-transport/src/dc/manager/snapshots/s2n_quic_transport__dc__manager__tests__snapshots.snap @@ -19,6 +19,12 @@ expression: "State::test_transitions()" event: "on_stateless_reset_tokens_acked", }, ), + on_peer_clean_close: Err( + InvalidTransition { + current: ClientPathSecretsReady, + event: "on_peer_clean_close", + }, + ), }, Complete: { on_path_secrets_ready: Err( @@ -38,6 +44,11 @@ expression: "State::test_transitions()" current: Complete, }, ), + on_peer_clean_close: Err( + NoOp { + current: Complete, + }, + ), }, InitClient: { on_path_secrets_ready: Ok( @@ -55,6 +66,12 @@ expression: "State::test_transitions()" event: "on_stateless_reset_tokens_acked", }, ), + on_peer_clean_close: Err( + InvalidTransition { + current: InitClient, + event: "on_peer_clean_close", + }, + ), }, InitServer: { on_path_secrets_ready: Ok( @@ -72,6 +89,12 @@ expression: "State::test_transitions()" event: "on_stateless_reset_tokens_acked", }, ), + on_peer_clean_close: Err( + InvalidTransition { + current: InitServer, + event: "on_peer_clean_close", + }, + ), }, ServerPathSecretsReady: { on_path_secrets_ready: Err( @@ -89,6 +112,12 @@ expression: "State::test_transitions()" event: "on_stateless_reset_tokens_acked", }, ), + on_peer_clean_close: Err( + InvalidTransition { + current: ServerPathSecretsReady, + event: "on_peer_clean_close", + }, + ), }, ServerTokensSent: { on_path_secrets_ready: Err( @@ -106,5 +135,8 @@ expression: "State::test_transitions()" on_stateless_reset_tokens_acked: Ok( Complete, ), + on_peer_clean_close: Ok( + Complete, + ), }, } diff --git a/quic/s2n-quic-transport/src/dc/manager/tests.rs b/quic/s2n-quic-transport/src/dc/manager/tests.rs index d6335d4bb2..84113a907e 100644 --- a/quic/s2n-quic-transport/src/dc/manager/tests.rs +++ b/quic/s2n-quic-transport/src/dc/manager/tests.rs @@ -321,10 +321,11 @@ fn connection_meta(endpoint_type: s2n_quic_core::endpoint::Type) -> event::build } } -/// A no-error close while the server is stuck in `ServerTokensSent` emits `DcStateIncomplete` -/// reporting that exact state. +/// A locally-initiated no-error close while the server is stuck in `ServerTokensSent` emits +/// `DcStateIncomplete` reporting that exact state: a local close says nothing about what the peer +/// received, so the server cannot infer completion from it. #[test] -fn on_close_server_incomplete_no_error() { +fn on_close_server_local_close_incomplete_no_error() { let mut recorder = IncompleteRecorder::default(); let mut context = (); let mut publisher = event::ConnectionPublisherSubscriber::new( @@ -341,12 +342,44 @@ fn on_close_server_incomplete_no_error() { manager.on_peer_dc_stateless_reset_tokens([TEST_TOKEN_1].iter(), &mut publisher); assert!(manager.state.is_server_tokens_sent()); - manager.on_close(true, &mut publisher); + // peer_initiated = false: the server closed locally, so it cannot conclude the tokens were + // received. + manager.on_close(true, false, &mut publisher); assert!(matches!( recorder.state, Some(event::api::DcHandshakeState::ServerTokensSent { .. }) )); + assert!(!manager.state.is_complete()); +} + +/// A peer-initiated no-error close while the server is in `ServerTokensSent` completes the dc +/// handshake: the peer only closes cleanly after receiving the server's tokens, so the close +/// confirms delivery even if the token ACK was lost. +#[test] +fn on_close_server_peer_close_completes() { + let mut recorder = IncompleteRecorder::default(); + let mut context = (); + let mut publisher = event::ConnectionPublisherSubscriber::new( + connection_meta(s2n_quic_core::endpoint::Type::Server), + 1, + &mut recorder, + &mut context, + ); + + let mut manager: Manager = Manager::new(Some(MockDcPath::default()), 1, &mut publisher); + assert!(manager + .on_path_secrets_ready(&Session, &mut publisher) + .is_ok()); + manager.on_peer_dc_stateless_reset_tokens([TEST_TOKEN_1].iter(), &mut publisher); + assert!(manager.state.is_server_tokens_sent()); + + // peer_initiated = true: a clean close from the peer confirms it received the server's tokens. + manager.on_close(true, true, &mut publisher); + + assert!(manager.state.is_complete()); + // Completing is not an "incomplete" outcome, so no DcStateIncomplete is emitted. + assert!(recorder.state.is_none()); } /// A no-error close while the client is stuck in `ClientPathSecretsReady` emits `DcStateIncomplete` @@ -368,7 +401,9 @@ fn on_close_client_incomplete_no_error() { .is_ok()); assert!(manager.state.is_path_secrets_ready()); - manager.on_close(true, &mut publisher); + // Even a peer-initiated clean close does not complete the client: the client completes only + // by receiving the server's tokens, which it hasn't here. + manager.on_close(true, true, &mut publisher); assert!(matches!( recorder.state, @@ -395,9 +430,12 @@ fn on_close_error_does_not_emit() { manager.on_peer_dc_stateless_reset_tokens([TEST_TOKEN_1].iter(), &mut publisher); assert!(manager.state.is_server_tokens_sent()); - manager.on_close(false, &mut publisher); + // An error close (closed_without_error = false) never completes or emits, regardless of who + // initiated it. + manager.on_close(false, true, &mut publisher); assert!(recorder.state.is_none()); + assert!(!manager.state.is_complete()); } /// A completed dc handshake does not emit `DcStateIncomplete` on close. @@ -420,7 +458,7 @@ fn on_close_complete_does_not_emit() { manager.on_peer_dc_stateless_reset_tokens([TEST_TOKEN_1].iter(), &mut publisher); assert!(manager.state.is_complete()); - manager.on_close(true, &mut publisher); + manager.on_close(true, true, &mut publisher); assert!(recorder.state.is_none()); } @@ -440,7 +478,7 @@ fn on_close_disabled_does_not_emit() { let mut manager: Manager = Manager::disabled(); assert!(manager.state.is_complete()); - manager.on_close(true, &mut publisher); + manager.on_close(true, true, &mut publisher); assert!(recorder.state.is_none()); }