diff --git a/rust/Cargo.lock b/rust/Cargo.lock index f17548d1d..2af2e97ce 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1441,6 +1441,7 @@ dependencies = [ "ip_network", "itertools 0.14.0", "serde", + "str0m", "uuid", ] @@ -2663,6 +2664,7 @@ dependencies = [ "secrecy", "serde", "serde_json", + "serde_with", "sha2", "snownet", "socket-factory", @@ -5937,6 +5939,26 @@ dependencies = [ "thiserror 2.0.12", ] +[[package]] +name = "ref-cast" +version = "1.0.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4a0ae411dbe946a674d89546582cea4ba2bb8defac896622d6496f14c23ba5cf" +dependencies = [ + "ref-cast-impl", +] + +[[package]] +name = "ref-cast-impl" +version = "1.0.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1165225c21bff1f3bbce98f5a1f889949bc902d3575308cc7b0de30b4f6d27c7" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.104", +] + [[package]] name = "regex" version = "1.11.1" @@ -6274,6 +6296,30 @@ dependencies = [ "uuid", ] +[[package]] +name = "schemars" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4cd191f9397d57d581cddd31014772520aa448f65ef991055d7f61582c65165f" +dependencies = [ + "dyn-clone", + "ref-cast", + "serde", + "serde_json", +] + +[[package]] +name = "schemars" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "82d20c4491bc164fa2f6c5d44565947a52ad80b9505d8e36f8d54c27c739fcd0" +dependencies = [ + "dyn-clone", + "ref-cast", + "serde", + "serde_json", +] + [[package]] name = "schemars_derive" version = "0.8.22" @@ -6662,15 +6708,17 @@ dependencies = [ [[package]] name = "serde_with" -version = "3.12.0" +version = "3.14.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d6b6f7f2fcb69f747921f79f3926bd1e203fce4fef62c268dd3abfb6d86029aa" +checksum = "f2c45cd61fefa9db6f254525d46e392b852e0e61d9a1fd36e5bd183450a556d5" dependencies = [ "base64 0.22.1", "chrono", "hex", "indexmap 1.9.3", "indexmap 2.9.0", + "schemars 0.9.0", + "schemars 1.0.4", "serde", "serde_derive", "serde_json", @@ -6680,9 +6728,9 @@ dependencies = [ [[package]] name = "serde_with_macros" -version = "3.12.0" +version = "3.14.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8d00caa5193a3c8362ac2b73be6b9e768aa5a4b2f721d8f4b339600c3cb51f8e" +checksum = "de90945e6565ce0d9a25098082ed4ee4002e047cb59892c318d66821e14bb30f" dependencies = [ "darling", "proc-macro2", @@ -7398,7 +7446,7 @@ dependencies = [ "glob", "heck 0.5.0", "json-patch", - "schemars", + "schemars 0.8.22", "semver", "serde", "serde_json", @@ -7458,7 +7506,7 @@ dependencies = [ "anyhow", "glob", "plist", - "schemars", + "schemars 0.8.22", "serde", "serde_json", "tauri-utils", @@ -7494,7 +7542,7 @@ dependencies = [ "dunce", "glob", "percent-encoding", - "schemars", + "schemars 0.8.22", "serde", "serde_json", "serde_repr", @@ -7536,7 +7584,7 @@ dependencies = [ "objc2-app-kit", "objc2-foundation", "open", - "schemars", + "schemars 0.8.22", "serde", "serde_json", "tauri", @@ -7558,7 +7606,7 @@ dependencies = [ "open", "os_pipe", "regex", - "schemars", + "schemars 0.8.22", "serde", "serde_json", "shared_child", @@ -7668,7 +7716,7 @@ dependencies = [ "proc-macro2", "quote", "regex", - "schemars", + "schemars 0.8.22", "semver", "serde", "serde-untagged", diff --git a/rust/Cargo.toml b/rust/Cargo.toml index 53f734184..872dd89a0 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -149,6 +149,7 @@ sentry-tracing = "0.41.0" serde = "1.0.219" serde_json = "1.0.141" serde_variant = "0.1.3" +serde_with = "3.14.0" sha2 = "0.10.9" smallvec = "1.15.1" smbios-lib = "0.9.2" diff --git a/rust/connlib/model/Cargo.toml b/rust/connlib/model/Cargo.toml index 4509b50ea..72b463d4a 100644 --- a/rust/connlib/model/Cargo.toml +++ b/rust/connlib/model/Cargo.toml @@ -8,6 +8,7 @@ license = { workspace = true } boringtun = { workspace = true } ip_network = { workspace = true, features = ["serde"] } serde = { workspace = true, features = ["derive", "std"] } +str0m = { workspace = true } uuid = { workspace = true, features = ["std", "v4", "serde"] } [dev-dependencies] diff --git a/rust/connlib/model/src/lib.rs b/rust/connlib/model/src/lib.rs index bbbcb16fa..ca1840c24 100644 --- a/rust/connlib/model/src/lib.rs +++ b/rust/connlib/model/src/lib.rs @@ -209,3 +209,82 @@ impl IpStack { } } } + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct IceCandidate(str0m::Candidate); + +impl From for IceCandidate { + fn from(value: str0m::Candidate) -> Self { + Self(value) + } +} + +impl From for str0m::Candidate { + fn from(value: IceCandidate) -> Self { + value.0 + } +} + +impl PartialOrd for IceCandidate { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for IceCandidate { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.0.prio().cmp(&other.0.prio()).reverse() + } +} + +impl Serialize for IceCandidate { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_str(&self.0.to_sdp_string()) + } +} + +impl<'de> Deserialize<'de> for IceCandidate { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::Error as _; + + let string = String::deserialize(deserializer)?; + let candidate = str0m::Candidate::from_sdp_string(&string).map_err(D::Error::custom)?; + + Ok(IceCandidate(candidate)) + } +} + +#[cfg(test)] +mod tests { + use std::{collections::BTreeSet, net::SocketAddr}; + + use super::*; + + #[test] + fn ice_candidate_ordering() { + let host = IceCandidate::from(str0m::Candidate::host(sock("1.1.1.1:80"), "udp").unwrap()); + let srflx = IceCandidate::from( + str0m::Candidate::server_reflexive(sock("1.1.1.1:80"), sock("3.3.3.3:80"), "udp") + .unwrap(), + ); + let relay = IceCandidate::from( + str0m::Candidate::relayed(sock("1.1.1.1:80"), sock("2.2.2.2:80"), "udp").unwrap(), + ); + + let candidate_set = BTreeSet::from([relay.clone(), host.clone(), srflx.clone()]); + + let candidate_list = Vec::from_iter(candidate_set); + + assert_eq!(candidate_list, vec![host, srflx, relay]); + } + + fn sock(s: &str) -> SocketAddr { + s.parse().unwrap() + } +} diff --git a/rust/connlib/snownet/src/node.rs b/rust/connlib/snownet/src/node.rs index 313213cac..00d07f2e1 100644 --- a/rust/connlib/snownet/src/node.rs +++ b/rust/connlib/snownet/src/node.rs @@ -9,7 +9,6 @@ use boringtun::x25519::PublicKey; use boringtun::{noise::rate_limiter::RateLimiter, x25519::StaticSecret}; use bufferpool::{Buffer, BufferPool}; use core::fmt; -use firezone_logging::err_with_src; use hex_display::HexDisplayExt; use ip_packet::{IpPacket, IpPacketBuf}; use itertools::Itertools; @@ -328,15 +327,7 @@ where } #[tracing::instrument(level = "info", skip_all, fields(%cid))] - pub fn add_remote_candidate(&mut self, cid: TId, candidate: String, now: Instant) { - let candidate = match Candidate::from_sdp_string(&candidate) { - Ok(c) => c, - Err(e) => { - tracing::debug!("Failed to parse candidate: {}", err_with_src(&e)); - return; - } - }; - + pub fn add_remote_candidate(&mut self, cid: TId, candidate: Candidate, now: Instant) { let Some((agent, relay)) = self.connections.agent_mut(cid) else { tracing::debug!(ignored_candidate = %candidate, "Unknown connection"); return; @@ -377,15 +368,7 @@ where } #[tracing::instrument(level = "info", skip_all, fields(%cid))] - pub fn remove_remote_candidate(&mut self, cid: TId, candidate: String, now: Instant) { - let candidate = match Candidate::from_sdp_string(&candidate) { - Ok(c) => c, - Err(e) => { - tracing::debug!("Failed to parse candidate: {}", err_with_src(&e)); - return; - } - }; - + pub fn remove_remote_candidate(&mut self, cid: TId, candidate: Candidate, now: Instant) { if let Some((agent, _)) = self.connections.agent_mut(cid) { agent.invalidate_candidate(&candidate); agent.handle_timeout(now); // We may have invalidated the last candidate, ensure we check our nomination state. @@ -1151,7 +1134,12 @@ fn seed_agent_with_local_candidates( .into_iter() .flat_map(|allocation| allocation.current_relay_candidates()); - for candidate in shared_candidates.chain(relay_candidates) { + // Candidates with a higher priority are better, therefore: Reverse the ordering by priority. + for candidate in shared_candidates + .chain(relay_candidates) + .sorted_by_key(|c| c.prio()) + .rev() + { add_local_candidate(connection, agent, candidate, pending_events); } } @@ -1407,7 +1395,7 @@ fn signal_candidate_to_remote( pending_events.push_back(Event::NewIceCandidate { connection: id, - candidate: candidate.to_sdp_string(), + candidate: candidate.clone(), }) } @@ -1437,7 +1425,7 @@ fn remove_local_candidate( if candidate.kind() == CandidateKind::ServerReflexive { pending_events.push_back(Event::InvalidateIceCandidate { connection: id, - candidate: candidate.to_sdp_string(), + candidate: candidate.clone(), }); return; } @@ -1447,7 +1435,7 @@ fn remove_local_candidate( if was_present { pending_events.push_back(Event::InvalidateIceCandidate { connection: id, - candidate: candidate.to_sdp_string(), + candidate: candidate.clone(), }) } } @@ -1494,19 +1482,15 @@ impl From for Credentials { #[derive(Debug, PartialEq, Clone)] pub enum Event { /// We created a new candidate for this connection and ask to signal it to the remote party. - /// - /// Candidates are in SDP format although this may change and should be considered an implementation detail of the application. NewIceCandidate { connection: TId, - candidate: String, + candidate: Candidate, }, /// We invalidated a candidate for this connection and ask to signal that to the remote party. - /// - /// Candidates are in SDP format although this may change and should be considered an implementation detail of the application. InvalidateIceCandidate { connection: TId, - candidate: String, + candidate: Candidate, }, ConnectionEstablished(TId), diff --git a/rust/connlib/tunnel/Cargo.toml b/rust/connlib/tunnel/Cargo.toml index 99d0c9f42..86acfaf72 100644 --- a/rust/connlib/tunnel/Cargo.toml +++ b/rust/connlib/tunnel/Cargo.toml @@ -44,6 +44,7 @@ ringbuffer = { workspace = true } secrecy = { workspace = true, features = ["serde"] } serde = { workspace = true, features = ["derive", "std"] } serde_json = { workspace = true } +serde_with = { workspace = true } snownet = { workspace = true } socket-factory = { workspace = true } socket2 = { workspace = true } diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 05ada3d8c..e6091924b 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -17,7 +17,9 @@ use crate::unique_packet_buffer::UniquePacketBuffer; use crate::{IPV4_TUNNEL, IPV6_TUNNEL, IpConfig, TunConfig, dns, is_peer, p2p_control}; use anyhow::Context; use bimap::BiMap; -use connlib_model::{GatewayId, PublicKey, RelayId, ResourceId, ResourceStatus, ResourceView}; +use connlib_model::{ + GatewayId, IceCandidate, PublicKey, RelayId, ResourceId, ResourceStatus, ResourceView, +}; use connlib_model::{Site, SiteId}; use firezone_logging::{err_with_src, unwrap_or_debug, unwrap_or_warn}; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; @@ -571,8 +573,14 @@ impl ClientState { Ok(()) } - pub fn add_ice_candidate(&mut self, conn_id: GatewayId, ice_candidate: String, now: Instant) { - self.node.add_remote_candidate(conn_id, ice_candidate, now); + pub fn add_ice_candidate( + &mut self, + conn_id: GatewayId, + ice_candidate: IceCandidate, + now: Instant, + ) { + self.node + .add_remote_candidate(conn_id, ice_candidate.into(), now); self.node.handle_timeout(now); self.drain_node_events(); } @@ -580,11 +588,11 @@ impl ClientState { pub fn remove_ice_candidate( &mut self, conn_id: GatewayId, - ice_candidate: String, + ice_candidate: IceCandidate, now: Instant, ) { self.node - .remove_remote_candidate(conn_id, ice_candidate, now); + .remove_remote_candidate(conn_id, ice_candidate.into(), now); self.node.handle_timeout(now); self.drain_node_events(); } @@ -1421,8 +1429,8 @@ impl ClientState { fn drain_node_events(&mut self) { let mut resources_changed = false; // Track this separately to batch together `ResourcesChanged` events. - let mut added_ice_candidates = BTreeMap::>::default(); - let mut removed_ice_candidates = BTreeMap::>::default(); + let mut added_ice_candidates = BTreeMap::>::default(); + let mut removed_ice_candidates = BTreeMap::>::default(); while let Some(event) = self.node.poll_event() { match event { @@ -1437,7 +1445,7 @@ impl ClientState { added_ice_candidates .entry(connection) .or_default() - .insert(candidate); + .insert(candidate.into()); } snownet::Event::InvalidateIceCandidate { connection, @@ -1446,7 +1454,7 @@ impl ClientState { removed_ice_candidates .entry(connection) .or_default() - .insert(candidate); + .insert(candidate.into()); } snownet::Event::ConnectionEstablished(id) => { self.update_site_status_by_gateway(&id, ResourceStatus::Online); diff --git a/rust/connlib/tunnel/src/gateway.rs b/rust/connlib/tunnel/src/gateway.rs index e7d14c6ee..8f4d32af2 100644 --- a/rust/connlib/tunnel/src/gateway.rs +++ b/rust/connlib/tunnel/src/gateway.rs @@ -6,7 +6,7 @@ use crate::{peer::ClientOnGateway, peer_store::PeerStore}; use anyhow::{Context, Result}; use boringtun::x25519::PublicKey; use chrono::{DateTime, Utc}; -use connlib_model::{ClientId, RelayId, ResourceId}; +use connlib_model::{ClientId, IceCandidate, RelayId, ResourceId}; use dns_types::DomainName; use ip_packet::{FzP2pControlSlice, IpPacket}; use secrecy::{ExposeSecret as _, Secret}; @@ -180,15 +180,26 @@ impl GatewayState { self.peers.remove(id); } - pub fn add_ice_candidate(&mut self, conn_id: ClientId, ice_candidate: String, now: Instant) { - self.node.add_remote_candidate(conn_id, ice_candidate, now); + pub fn add_ice_candidate( + &mut self, + conn_id: ClientId, + ice_candidate: IceCandidate, + now: Instant, + ) { + self.node + .add_remote_candidate(conn_id, ice_candidate.into(), now); self.node.handle_timeout(now); self.drain_node_events(); } - pub fn remove_ice_candidate(&mut self, conn_id: ClientId, ice_candidate: String, now: Instant) { + pub fn remove_ice_candidate( + &mut self, + conn_id: ClientId, + ice_candidate: IceCandidate, + now: Instant, + ) { self.node - .remove_remote_candidate(conn_id, ice_candidate, now); + .remove_remote_candidate(conn_id, ice_candidate.into(), now); self.node.handle_timeout(now); self.drain_node_events(); } @@ -384,8 +395,8 @@ impl GatewayState { } fn drain_node_events(&mut self) { - let mut added_ice_candidates = BTreeMap::>::default(); - let mut removed_ice_candidates = BTreeMap::>::default(); + let mut added_ice_candidates = BTreeMap::>::default(); + let mut removed_ice_candidates = BTreeMap::>::default(); while let Some(event) = self.node.poll_event() { match event { @@ -399,7 +410,7 @@ impl GatewayState { added_ice_candidates .entry(connection) .or_default() - .insert(candidate); + .insert(candidate.into()); } snownet::Event::InvalidateIceCandidate { connection, @@ -408,7 +419,7 @@ impl GatewayState { removed_ice_candidates .entry(connection) .or_default() - .insert(candidate); + .insert(candidate.into()); } snownet::Event::ConnectionEstablished(_) => {} } diff --git a/rust/connlib/tunnel/src/lib.rs b/rust/connlib/tunnel/src/lib.rs index c200b9a86..1f69d33cb 100644 --- a/rust/connlib/tunnel/src/lib.rs +++ b/rust/connlib/tunnel/src/lib.rs @@ -8,7 +8,7 @@ use anyhow::Result; use bimap::BiMap; use chrono::Utc; -use connlib_model::{ClientId, GatewayId, PublicKey, ResourceId, ResourceView}; +use connlib_model::{ClientId, GatewayId, IceCandidate, PublicKey, ResourceId, ResourceView}; use dns_types::DomainName; use gat_lending_iterator::LendingIterator; use io::{Buffers, Io}; @@ -411,11 +411,11 @@ impl GatewayTunnel { pub enum ClientEvent { AddedIceCandidates { conn_id: GatewayId, - candidates: BTreeSet, + candidates: BTreeSet, }, RemovedIceCandidates { conn_id: GatewayId, - candidates: BTreeSet, + candidates: BTreeSet, }, ConnectionIntent { resource: ResourceId, @@ -465,11 +465,11 @@ impl IpConfig { pub enum GatewayEvent { AddedIceCandidates { conn_id: ClientId, - candidates: BTreeSet, + candidates: BTreeSet, }, RemovedIceCandidates { conn_id: ClientId, - candidates: BTreeSet, + candidates: BTreeSet, }, ResolveDns(ResolveDnsRequest), } diff --git a/rust/connlib/tunnel/src/messages/client.rs b/rust/connlib/tunnel/src/messages/client.rs index 9360dff88..452555472 100644 --- a/rust/connlib/tunnel/src/messages/client.rs +++ b/rust/connlib/tunnel/src/messages/client.rs @@ -1,7 +1,7 @@ //! Client related messages that are needed within connlib use crate::messages::{IceCredentials, Interface, Key, Relay, RelaysPresence, SecretKey}; -use connlib_model::{GatewayId, IpStack, ResourceId, Site, SiteId}; +use connlib_model::{GatewayId, IceCandidate, IpStack, ResourceId, Site, SiteId}; use ip_network::IpNetwork; use serde::{Deserialize, Serialize}; use std::{ @@ -161,15 +161,17 @@ pub struct GatewaysIceCandidates { /// The list of gateway IDs these candidates will be broadcast to. pub gateway_ids: Vec, /// Actual RTC ice candidates - pub candidates: BTreeSet, + pub candidates: BTreeSet, } +#[serde_with::serde_as] #[derive(Debug, Deserialize)] pub struct GatewayIceCandidates { /// Gateway's id the ice candidates are from pub gateway_id: GatewayId, /// Actual RTC ice candidates - pub candidates: Vec, + #[serde_as(as = "serde_with::VecSkipError<_>")] + pub candidates: Vec, } // These messages can be sent from a client to a control pane @@ -488,4 +490,13 @@ mod tests { assert_eq!(actual_json, expected_json); } + + #[test] + fn faulty_candidate_get_skipped() { + let bad_candidates = serde_json::json!({ "gateway_id": "f16ecfa0-a94f-4bfd-a2ef-1cc1f2ef3da3", "candidates": ["foo", "bar", "baz", "candidate:fffeff6435be70ddbf995982 1 udp 1694498559 87.121.72.60 57114 typ srflx raddr 0.0.0.0 rport 0"] }); + + let gateway_candidates = GatewayIceCandidates::deserialize(bad_candidates).unwrap(); + + assert_eq!(gateway_candidates.candidates.len(), 1); + } } diff --git a/rust/connlib/tunnel/src/messages/gateway.rs b/rust/connlib/tunnel/src/messages/gateway.rs index 9093261e7..ace6269bb 100644 --- a/rust/connlib/tunnel/src/messages/gateway.rs +++ b/rust/connlib/tunnel/src/messages/gateway.rs @@ -8,7 +8,7 @@ use chrono::{ DateTime, Utc, serde::{ts_seconds, ts_seconds_option}, }; -use connlib_model::{ClientId, ResourceId}; +use connlib_model::{ClientId, IceCandidate, ResourceId}; use ip_network::IpNetwork; use serde::{Deserialize, Serialize}; use std::{ @@ -240,16 +240,18 @@ pub struct ClientsIceCandidates { /// Client's id the ice candidates are meant for pub client_ids: Vec, /// Actual RTC ice candidates - pub candidates: BTreeSet, + pub candidates: BTreeSet, } /// A client's ice candidate message. +#[serde_with::serde_as] #[derive(Debug, Deserialize, Clone)] pub struct ClientIceCandidates { /// Client's id the ice candidates came from pub client_id: ClientId, /// Actual RTC ice candidates - pub candidates: Vec, + #[serde_as(as = "serde_with::VecSkipError<_>")] + pub candidates: Vec, } // These messages can be sent from a gateway @@ -590,4 +592,13 @@ mod tests { assert!(matches!(message, IngressMessages::AuthorizeFlow(_))); } + + #[test] + fn faulty_candidate_get_skipped() { + let bad_candidates = serde_json::json!({ "client_id": "f16ecfa0-a94f-4bfd-a2ef-1cc1f2ef3da3", "candidates": ["foo", "bar", "baz", "candidate:fffeff6435be70ddbf995982 1 udp 1694498559 87.121.72.60 57114 typ srflx raddr 0.0.0.0 rport 0"] }); + + let client_candidates = ClientIceCandidates::deserialize(bad_candidates).unwrap(); + + assert_eq!(client_candidates.candidates.len(), 1); + } }