diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 7ea97e69b..5ae904482 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -309,6 +309,10 @@ pub struct Connection { // paths. Or a set together with a minimum. Or something. abandoned_paths: FxHashSet, + /// Set of [`FourTuple`]s for which path validation (as defined in RFC9000 ยง8.2) has succeeded. + // TODO(@divma): this needs expiry or a max size. + validated_network_paths: ValidatedNetworkPaths, + /// State for n0's () nat traversal protocol. n0_nat_traversal: n0_nat_traversal::State, qlog: QlogSink, @@ -332,7 +336,7 @@ impl Connection { qlog: QlogSink, ) -> Self { let pref_addr_cid = side_args.pref_addr_cid(); - let path_validated = side_args.path_validated(); + let remote_address_validated = side_args.remote_address_validated(); let connection_side = ConnectionSide::from(side_args); let side = connection_side.side(); let mut rng = StdRng::from_seed(rng_seed); @@ -431,12 +435,13 @@ impl Connection { remote_max_path_id: PathId::ZERO, max_path_id_with_cids: PathId::ZERO, abandoned_paths: Default::default(), + validated_network_paths: Default::default(), n0_nat_traversal: Default::default(), qlog, }; - if path_validated { - this.on_path_validated(PathId::ZERO); + if remote_address_validated { + this.on_remote_address_validated(PathId::ZERO); } if side.is_client() { // Kick off the connection @@ -646,7 +651,9 @@ impl Connection { if reason.is_locally_initiated() { let has_remaining_validated_paths = self.paths.iter().any(|(id, path)| { - *id != path_id && !self.abandoned_paths.contains(id) && path.data.validated + *id != path_id + && !self.abandoned_paths.contains(id) + && path.data.remote_address_validated }); if !has_remaining_validated_paths { return Err(ClosePathError::LastOpenPath); @@ -870,19 +877,16 @@ impl Connection { /// Find an open, validated path that's on the same network path as the given network path. /// /// Returns the first path matching, even if there's multiple. - fn find_validated_path_on_network_path( + fn find_address_validated_path_on_network_path( &self, network_path: FourTuple, ) -> Option<(&PathId, &PathState)> { self.paths.iter().find(|(path_id, path_state)| { - path_state.data.validated + path_state.data.remote_address_validated // Would this use the same network path, if network_path were used to send right now? && network_path.is_probably_same_path(&path_state.data.network_path) && !self.abandoned_paths.contains(path_id) }) - // TODO(@divma): we might want to ensure the path has been recently active to consider the - // address validated - // matheus23: Perhaps looking at !self.abandoned_paths.contains(path_id) is enough, given keep-alives? } /// Creates the [`PathData`] for a new [`PathId`]. @@ -895,9 +899,10 @@ impl Connection { now: Instant, pn: Option, ) -> &mut PathData { - let valid_path = self.find_validated_path_on_network_path(network_path); - let validated = valid_path.is_some(); - let initial_rtt = valid_path.map(|(_, path)| path.data.rtt.conservative()); + let initial_rtt = self + .find_address_validated_path_on_network_path(network_path) + .map(|(_, path)| path.data.rtt.conservative()); + let vacant_entry = match self.paths.entry(path_id) { btree_map::Entry::Vacant(vacant_entry) => vacant_entry, btree_map::Entry::Occupied(occupied_entry) => { @@ -905,7 +910,10 @@ impl Connection { } }; - debug!(%validated, %path_id, %network_path, "path added"); + let remote_address_validated = self + .validated_network_paths + .is_remote_address_validated(network_path); + debug!(%remote_address_validated, %path_id, %network_path, "path added"); let peer_max_udp_payload_size = u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX); self.path_generation_counter = self.path_generation_counter.wrapping_add(1); @@ -918,7 +926,7 @@ impl Connection { &self.config, ); - data.validated = validated; + data.remote_address_validated = remote_address_validated; if let Some(initial_rtt) = initial_rtt { data.rtt.reset_initial_rtt(initial_rtt); } @@ -1035,7 +1043,7 @@ impl Connection { // Is there any open, validated and status available path with dst CIDs? If so we'll // want to set path_exclusive_only for any other paths. let have_available_path = self.paths.iter().any(|(id, path)| { - path.data.validated + path.data.remote_address_validated && path.data.local_status() == PathStatus::Available && self.remote_cids.contains_key(id) }); @@ -1718,7 +1726,7 @@ impl Connection { /// - The MTU Discovery subsystem wants to probe the path. fn get_mtu_probe_data(&mut self, now: Instant, path_id: PathId) -> Option<(ConnectionId, u16)> { let active_cid = self.remote_cids.get(&path_id).map(CidQueue::active)?; - let is_eligible = self.path_data(path_id).validated + let is_eligible = self.path_data(path_id).remote_address_validated && !self.path_data(path_id).is_validating_path() && !self.abandoned_paths.contains(&path_id); @@ -1947,7 +1955,7 @@ impl Connection { ) -> Option { let server_side = self.n0_nat_traversal.server_side_mut().ok()?; let probe = server_side.next_probe()?; - if !self.paths.get(&path_id)?.data.validated { + if !self.paths.get(&path_id)?.data.remote_address_validated { // Path is not usable for probing return None; } @@ -4081,7 +4089,7 @@ impl Connection { if self .paths .get(&path_id) - .map(|p| p.data.validated && p.data.network_path == network_path) + .map(|p| p.data.remote_address_validated && p.data.network_path == network_path) .unwrap_or(false) { self.connection_close_pending = true; @@ -4270,7 +4278,7 @@ impl Connection { ); return Ok(()); } - self.on_path_validated(path_id); + self.on_remote_address_validated(path_id); self.process_early_payload(now, path_id, packet, qlog)?; if self.state.is_closed() { @@ -4626,7 +4634,13 @@ impl Connection { .data .on_path_response_received(now, response.0, network_path) { - OnPath { was_open } => { + OnPath { + was_open, + validated_network_path, + } => { + debug!(%response, %validated_network_path, "on path response"); + self.validated_network_paths + .insert(validated_network_path, now); let qlog = self.qlog.with_time(now); self.timers @@ -4661,14 +4675,20 @@ impl Connection { prev.reset_on_path_challenges(); } } - OffPath => { - debug!(%response, "Valid response to off-path PATH_CHALLENGE"); + OffPath { + validated_network_path, + } => { + debug!(%response, %validated_network_path, "off path response"); + self.validated_network_paths + .insert(validated_network_path, now) } Ignored { - sent_on, - current_path, + validated_network_path, } => { - debug!(%sent_on, %current_path, %response, "ignoring valid PATH_RESPONSE") + debug!(%response, %validated_network_path, "valid path response for oudated network path"); + + self.validated_network_paths + .insert(validated_network_path, now) } Unknown => debug!(%response, "ignoring invalid PATH_RESPONSE"), } @@ -5246,6 +5266,9 @@ impl Connection { addr: updated, })); } + new_path_data.remote_address_validated = self + .validated_network_paths + .is_remote_address_validated(network_path); new_path_data.pending_on_path_challenge = true; let mut prev_path_data = mem::replace(&mut path.data, new_path_data); @@ -5258,7 +5281,7 @@ impl Connection { // yet. In this case we would never have sent on this path yet and would not be able // to send a PATH_CHALLENGE either, which is currently a fire-and-forget affair // anyway. So don't store such a path either. - if !prev_path_data.validated + if !prev_path_data.remote_address_validated && let Some(cid) = self.remote_cids.get(&path_id).map(CidQueue::active) { prev_path_data.pending_on_path_challenge = true; @@ -5686,7 +5709,10 @@ impl Connection { self.qlog.with_time(now), ); - if is_multipath_negotiated && !path.validated && path.pending_on_path_challenge { + if is_multipath_negotiated + && !path.remote_address_validated + && path.pending_on_path_challenge + { // queue informing the path status along with the challenge space.pending.path_status.insert(path_id); } @@ -6507,9 +6533,10 @@ impl Connection { packet_crypto.map_or(16, |x| x.tag_len()) } - /// Mark the path as validated, and enqueue NEW_TOKEN frames to be sent as appropriate - fn on_path_validated(&mut self, path_id: PathId) { - self.path_data_mut(path_id).validated = true; + /// Mark the address of the given path as validated, and enqueue NEW_TOKEN frames to be sent as + /// appropriate. + fn on_remote_address_validated(&mut self, path_id: PathId) { + self.path_data_mut(path_id).remote_address_validated = true; let ConnectionSide::Server { server_config } = &self.side else { return; }; @@ -6682,7 +6709,7 @@ impl Connection { if !addresses_to_probe .iter() .any(|(_, probe)| *probe == (ip, port)) - && !path.validated + && !path.remote_address_validated && !self.abandoned_paths.contains(&path_id) { trace!(%path_id, "closing path from previous round"); @@ -6760,6 +6787,23 @@ impl fmt::Debug for Connection { } } +#[derive(Debug, derive_more::Deref, derive_more::DerefMut, Default)] +struct ValidatedNetworkPaths(FxHashMap); + +impl ValidatedNetworkPaths { + fn insert(&mut self, network_path: FourTuple, now: Instant) { + if network_path.local_ip.is_some() && self.0.insert(network_path, now).is_none() { + debug!(%network_path, "network path validated"); + } + } + + fn is_remote_address_validated(&self, network_path: FourTuple) -> bool { + self.0 + .keys() + .any(|validated_network_path| validated_network_path.remote == network_path.remote) + } +} + /// Hints when the caller identifies a network change. pub trait NetworkChangeHint: std::fmt::Debug + 'static { /// Inform the connection if a path may recover after a network change. @@ -6889,7 +6933,7 @@ impl From for ConnectionSide { SideArgs::Server { server_config, pref_addr_cid: _, - path_validated: _, + remote_address_validated: _, } => Self::Server { server_config }, } } @@ -6904,7 +6948,7 @@ pub(crate) enum SideArgs { Server { server_config: Arc, pref_addr_cid: Option, - path_validated: bool, + remote_address_validated: bool, }, } @@ -6916,10 +6960,13 @@ impl SideArgs { } } - pub(crate) fn path_validated(&self) -> bool { + pub(crate) fn remote_address_validated(&self) -> bool { match *self { Self::Client { .. } => true, - Self::Server { path_validated, .. } => path_validated, + Self::Server { + remote_address_validated, + .. + } => remote_address_validated, } } diff --git a/noq-proto/src/connection/paths.rs b/noq-proto/src/connection/paths.rs index 81e06626c..66a9d087f 100644 --- a/noq-proto/src/connection/paths.rs +++ b/noq-proto/src/connection/paths.rs @@ -163,7 +163,7 @@ pub(super) struct PathData { /// /// Initially equal to `use_stateless_retry` for servers, and becomes false again on every /// migration. Always true for clients. - pub(super) validated: bool, + pub(super) remote_address_validated: bool, /// Total size of all UDP datagrams sent on this path pub(super) total_sent: u64, /// Total size of all UDP datagrams received on this path @@ -268,7 +268,7 @@ impl PathData { off_path_challenges_unconfirmed: Default::default(), pending_on_path_challenge: false, path_responses: PathResponses::default(), - validated: false, + remote_address_validated: false, total_sent: 0, total_recvd: 0, mtud: config @@ -324,7 +324,7 @@ impl PathData { off_path_challenges_unconfirmed: Default::default(), pending_on_path_challenge: false, path_responses: PathResponses::default(), - validated: false, + remote_address_validated: false, total_sent: 0, total_recvd: 0, mtud: prev.mtud.clone(), @@ -353,7 +353,7 @@ impl PathData { /// Indicates whether we're a server that hasn't validated the peer's address and hasn't /// received enough data from the peer to permit sending `bytes_to_send` additional bytes pub(super) fn anti_amplification_blocked(&self, bytes_to_send: u64) -> bool { - !self.validated && self.total_recvd * 3 < self.total_sent + bytes_to_send + !self.remote_address_validated && self.total_recvd * 3 < self.total_sent + bytes_to_send } /// Returns the path's current MTU @@ -402,7 +402,7 @@ impl PathData { /// Increment the total size of sent UDP datagrams pub(super) fn inc_total_sent(&mut self, inc: u64) { self.total_sent = self.total_sent.saturating_add(inc); - if !self.validated { + if !self.remote_address_validated { trace!( network_path = %self.network_path, anti_amplification_budget = %(self.total_recvd * 3).saturating_sub(self.total_sent), @@ -414,7 +414,7 @@ impl PathData { /// Increment the total size of received UDP datagrams pub(super) fn inc_total_recvd(&mut self, inc: u64) { self.total_recvd = self.total_recvd.saturating_add(inc); - if !self.validated { + if !self.remote_address_validated { trace!( network_path = %self.network_path, anti_amplification_budget = %(self.total_recvd * 3).saturating_sub(self.total_sent), @@ -461,9 +461,9 @@ impl PathData { Some(info) if info.network_path.is_probably_same_path(&self.network_path) => { self.network_path.update_local_if_same_remote(&network_path); let sent_instant = info.sent_instant; - if !std::mem::replace(&mut self.validated, true) { - trace!("new path validated"); - } + // Path validation implies address validation. + self.remote_address_validated = true; + // Clear any other on-path sent challenge self.on_path_challenges_unconfirmed.clear(); @@ -477,12 +477,17 @@ impl PathData { let prev_status = std::mem::replace(&mut self.open_status, OpenStatus::Informed); OnPathResponseReceived::OnPath { was_open: prev_status == OpenStatus::Informed, + validated_network_path: self.network_path, // validates the updated path } } - // Response to an on-path PathChallenge that does not validate this path + // Response to an on-path PathChallenge that does not validate the current network + // path. Some(info) => { // This is a valid path response, but this validates a path we no longer have in - // use. Keep only sent challenges for the current path. + // use. This might happen if a challenge is sent using a local address, but the + // path migrates to a new local one. + // + // Keep only sent challenges for the current path. self.on_path_challenges_unconfirmed .retain(|_token, i| i.network_path == self.network_path); @@ -492,18 +497,20 @@ impl PathData { self.pending_on_path_challenge = true; } OnPathResponseReceived::Ignored { - sent_on: info.network_path, - current_path: self.network_path, + validated_network_path: info.network_path, } } None => match self.off_path_challenges_unconfirmed.remove(&token) { // Response to an off-path PathChallenge - Some(info) => { + Some(mut info) => { // Since we do not store validation state for these paths, we only really care // about reaching the same remote self.off_path_challenges_unconfirmed .retain(|_token, i| i.network_path.remote != info.network_path.remote); - OnPathResponseReceived::OffPath + info.network_path.update_local_if_same_remote(&network_path); + OnPathResponseReceived::OffPath { + validated_network_path: info.network_path, + } } // Response to an unknown PathChallenge. Does not indicate failure None => OnPathResponseReceived::Unknown, @@ -602,17 +609,24 @@ impl PathData { } pub(super) enum OnPathResponseReceived { - /// This response validates the path on its current remote address. - OnPath { was_open: bool }, + /// This response validates the current network path. + OnPath { + was_open: bool, + /// The FourTuple that was confirmed reachable (the one the challenge was sent on). + validated_network_path: FourTuple, + }, /// This response is valid, but it's for a remote other than the path's current remote address. - OffPath, - /// The received token is unknown. - Unknown, - /// The response is valid but it's not usable for path validation. + OffPath { + /// The network path that was confirmed reachable (the one the off-path challenge was sent on). + validated_network_path: FourTuple, + }, + /// The response is valid but it's not usable for this multipath path. Ignored { - sent_on: FourTuple, - current_path: FourTuple, + /// The FourTuple that was confirmed reachable (the one the challenge was sent on). + validated_network_path: FourTuple, }, + /// The received token is unknown. + Unknown, } #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] diff --git a/noq-proto/src/endpoint.rs b/noq-proto/src/endpoint.rs index 7e9612dda..695b1f083 100644 --- a/noq-proto/src/endpoint.rs +++ b/noq-proto/src/endpoint.rs @@ -645,7 +645,7 @@ impl Endpoint { SideArgs::Server { server_config, pref_addr_cid, - path_validated: remote_address_validated, + remote_address_validated, }, ¶ms, );