diff --git a/quiche/src/lib.rs b/quiche/src/lib.rs index d42d53d9d9..1372d1e694 100644 --- a/quiche/src/lib.rs +++ b/quiche/src/lib.rs @@ -1607,6 +1607,7 @@ impl Connection { let path_mgr = path::PathManager::new( path, config.local_transport_params.active_conn_id_limit as usize, + is_server, config.events, ); @@ -2750,40 +2751,8 @@ impl Connection { recv_pid != active_path_id && self.pkt_num_spaces[epoch].largest_rx_non_probing_pkt_num == pn { - self.path_mgr.set_active_path(recv_pid)?; - let recv_path = self.path_mgr.get_mut(recv_pid)?; - // Requests path validation if needed. - if !recv_path.validated() && !recv_path.under_validation() { - recv_path.request_validation(); - } - - let local_addr = recv_path.local_addr(); - let peer_addr = recv_path.peer_addr(); - let no_spare_dcid = recv_path.active_dcid_seq.is_none(); - - // Notify the application. - self.path_mgr.notify_event(path::PathEvent::PeerMigrated( - local_addr, peer_addr, - )); - - if no_spare_dcid { - if self.disable_dcid_reuse { - warn!( - "Connection migrated on a new path but no spare \ - DCID; waiting for a new one" - ); - } else { - self.path_mgr.get_mut(recv_pid)?.active_dcid_seq = self - .path_mgr - .get_mut(active_path_id)? - .active_dcid_seq - .take(); - warn!( - "Connection migrated on a new path but no spare \ - DCID; using the previous active path's one" - ); - } - } + self.path_mgr + .on_peer_migrated(recv_pid, self.disable_dcid_reuse)?; } } @@ -6486,7 +6455,7 @@ impl Connection { }, frame::Frame::PathResponse { data } => { - self.path_mgr.on_response_received(data); + self.path_mgr.on_response_received(data)?; }, frame::Frame::ConnectionClose { @@ -14040,11 +14009,11 @@ mod tests { ); assert_eq!( pipe.server.poll_path(), - Ok(PathEvent::PeerMigrated(server_addr, client_addr_3)) + Ok(PathEvent::Validated(server_addr, client_addr_3)) ); assert_eq!( pipe.server.poll_path(), - Ok(PathEvent::Validated(server_addr, client_addr_3)) + Ok(PathEvent::PeerMigrated(server_addr, client_addr_3)) ); assert_eq!(pipe.server.poll_path(), Err(Error::Done)); assert_eq!( @@ -14181,11 +14150,11 @@ mod tests { ); assert_eq!( pipe.server.poll_path(), - Ok(PathEvent::PeerMigrated(server_addr, client_addr_2)) + Ok(PathEvent::Validated(server_addr, client_addr_2)) ); assert_eq!( pipe.server.poll_path(), - Ok(PathEvent::Validated(server_addr, client_addr_2)) + Ok(PathEvent::PeerMigrated(server_addr, client_addr_2)) ); assert_eq!(pipe.server.poll_path(), Err(Error::Done)); assert_eq!( @@ -14336,10 +14305,6 @@ mod tests { pipe.server.poll_path(), Ok(PathEvent::New(server_addr, spoofed_client_addr)) ); - assert_eq!( - pipe.server.poll_path(), - Ok(PathEvent::PeerMigrated(server_addr, spoofed_client_addr)) - ); assert_eq!( pipe.server.is_path_validated(server_addr, client_addr), diff --git a/quiche/src/path.rs b/quiche/src/path.rs index 57100842f1..fa401fbb7b 100644 --- a/quiche/src/path.rs +++ b/quiche/src/path.rs @@ -96,17 +96,7 @@ pub enum PathEvent { /// denoted by the pair of `SocketAddr`, i.e., non-probing packets have been /// received on this network path. This is a server side only event. /// - /// Note that this does not imply that this network path has been validated, - /// but `quiche` automatically triggers a path validation on this new path - /// if needed. The connection has fully migrated if either - /// - /// * it later receives a `Validated` event with the same addresses, or - /// - /// * the path was already validated before. - /// - /// If the path validation fails, it receives a `FailedValidation` event - /// containing the path addresses and the host migrates back to the - /// previously active working path. + /// Note that this event is only raised if the path has been validated. PeerMigrated(SocketAddr, SocketAddr), } @@ -167,6 +157,9 @@ pub struct Path { challenge_requested: bool, /// Whether the failure of this path was notified. failure_notified: bool, + /// Whether the connection tries to migrate to this path, but it still needs + /// to be validated. + migrating: bool, } impl Path { @@ -205,6 +198,7 @@ impl Path { peer_verified_local_address: false, challenge_requested: false, failure_notified: false, + migrating: false, } } @@ -457,13 +451,17 @@ pub struct PathManager { /// Path-specific events to be notified to the application. events: Option>, + + /// Whether this manager serves a connection as a server. + is_server: bool, } impl PathManager { /// Creates a new `PathManager` with the initial provided `path` and a /// capacity limit. pub fn new( - mut initial_path: Path, max_concurrent_paths: usize, enable_events: bool, + mut initial_path: Path, max_concurrent_paths: usize, is_server: bool, + enable_events: bool, ) -> Self { let local_addr = initial_path.local_addr; let peer_addr = initial_path.peer_addr; @@ -485,6 +483,7 @@ impl PathManager { addrs_to_paths, events, + is_server, } } @@ -698,32 +697,94 @@ impl PathManager { } /// Handles incoming PATH_RESPONSE data. - pub fn on_response_received(&mut self, data: [u8; 8]) { - if let Some((_, p)) = self + pub fn on_response_received(&mut self, data: [u8; 8]) -> Result<()> { + let active_pid = self.get_active_path_id()?; + if let Some((pid, p)) = self .paths_mut() .find(|(_, p)| p.has_pending_challenge(data)) { if p.on_response_received(data) { let local_addr = p.local_addr; let peer_addr = p.peer_addr; + let was_migrating = p.migrating; + + p.migrating = false; // Notifies the application. self.notify_event(PathEvent::Validated(local_addr, peer_addr)); + + // If this path was the candidate for migration, notifies the + // application. + if pid == active_pid && was_migrating { + self.notify_event(PathEvent::PeerMigrated( + local_addr, peer_addr, + )); + } } } + Ok(()) } /// Sets the path with identifier 'path_id' to be active. /// /// There can be exactly one active path on which non-probing packets can be /// sent. If another path is marked as active, it will be superseeded by the - /// one having `path_id` as identfier. + /// one having `path_id` as identifier. + /// + /// A server should always ensure that the active path is validated. If it + /// is already the case, it notifies the application that the connection + /// migrated. Otherwise, it triggers a path validation and defers the + /// notification once it is actually validated. pub fn set_active_path(&mut self, path_id: usize) -> Result<()> { if let Ok(old_active_path) = self.get_active_mut() { old_active_path.active = false; } + let is_server = self.is_server; let new_active_path = self.get_mut(path_id)?; new_active_path.active = true; + if is_server { + if new_active_path.validated() { + let local_addr = new_active_path.local_addr(); + let peer_addr = new_active_path.peer_addr(); + self.notify_event(PathEvent::PeerMigrated(local_addr, peer_addr)); + } else { + new_active_path.migrating = true; + // Requests path validation if needed. + if !new_active_path.under_validation() { + new_active_path.request_validation(); + } + } + } + Ok(()) + } + + /// Handles potential connection migration. + pub fn on_peer_migrated( + &mut self, new_pid: usize, disable_dcid_reuse: bool, + ) -> Result<()> { + let active_path_id = self.get_active_path_id()?; + if active_path_id == new_pid { + return Ok(()); + } + self.set_active_path(new_pid)?; + + let no_spare_dcid = self.get_mut(new_pid)?.active_dcid_seq.is_none(); + if no_spare_dcid { + if disable_dcid_reuse { + trace!( + "Connection migrated on a new path but no spare \ + DCID; waiting for a new one" + ); + } else { + self.get_mut(new_pid)?.active_dcid_seq = + self.get_mut(active_path_id)?.active_dcid_seq; + trace!( + "Connection migrated on a new path but no spare \ + DCID; using the previous active path's one" + ); + } + } + Ok(()) } } @@ -837,16 +898,21 @@ mod tests { #[test] fn path_validation_limited_mtu() { let client_addr = "127.0.0.1:1234".parse().unwrap(); + let client_addr_2 = "127.0.0.1:5678".parse().unwrap(); let server_addr = "127.0.0.1:4321".parse().unwrap(); let config = Config::new(crate::PROTOCOL_VERSION).unwrap(); let recovery_config = RecoveryConfig::from_config(&config); - let path = Path::new(client_addr, server_addr, &recovery_config, false); - let mut path_mgr = PathManager::new(path, 2, true); + let path = Path::new(client_addr, server_addr, &recovery_config, true); + let mut path_mgr = PathManager::new(path, 2, false, true); + + let probed_path = + Path::new(client_addr_2, server_addr, &recovery_config, false); + path_mgr.insert_path(probed_path, false).unwrap(); let pid = path_mgr - .path_id_from_addrs(&(client_addr, server_addr)) + .path_id_from_addrs(&(client_addr_2, server_addr)) .unwrap(); path_mgr.get_mut(pid).unwrap().request_validation(); assert_eq!(path_mgr.get_mut(pid).unwrap().validation_requested(), true); @@ -873,7 +939,7 @@ mod tests { // Receives the response. The path is reachable, but the MTU is not // validated yet. - path_mgr.on_response_received(data); + path_mgr.on_response_received(data).unwrap(); assert_eq!(path_mgr.get_mut(pid).unwrap().validation_requested(), true); assert_eq!(path_mgr.get_mut(pid).unwrap().probing_required(), true); @@ -897,7 +963,7 @@ mod tests { ) .unwrap(); - path_mgr.on_response_received(data); + path_mgr.on_response_received(data).unwrap(); assert_eq!(path_mgr.get_mut(pid).unwrap().validation_requested(), false); assert_eq!(path_mgr.get_mut(pid).unwrap().probing_required(), false); @@ -906,7 +972,7 @@ mod tests { assert_eq!(path_mgr.get_mut(pid).unwrap().state, PathState::Validated); assert_eq!( path_mgr.pop_event(), - Some(PathEvent::Validated(client_addr, server_addr)) + Some(PathEvent::Validated(client_addr_2, server_addr)) ); } @@ -918,8 +984,8 @@ mod tests { let config = Config::new(crate::PROTOCOL_VERSION).unwrap(); let recovery_config = RecoveryConfig::from_config(&config); - let path = Path::new(client_addr, server_addr, &recovery_config, false); - let mut client_path_mgr = PathManager::new(path, 2, true); + let path = Path::new(client_addr, server_addr, &recovery_config, true); + let mut client_path_mgr = PathManager::new(path, 2, false, true); let mut server_path = Path::new(server_addr, client_addr, &recovery_config, false); @@ -965,7 +1031,7 @@ mod tests { assert_eq!(server_path.received_challenges.len(), 2); // Response for first probe. - client_path_mgr.on_response_received(data); + client_path_mgr.on_response_received(data).unwrap(); assert_eq!( client_path_mgr .get(client_pid) @@ -976,7 +1042,7 @@ mod tests { ); // Response for second probe. - client_path_mgr.on_response_received(data_2); + client_path_mgr.on_response_received(data_2).unwrap(); assert_eq!( client_path_mgr .get(client_pid)