feat(connlib): always use candidates in order of priority (#10063)

To make things easier to debug, we enforce the order that candidates are
processed in. We want candidates to be processed in the order of their
inverse priority as higher priorities are better. For example, a host
candidate has a higher priority than a relay candidate.

This will make our logs more consistent because a `0-0` candidate pair
is always a `host-host` pair.

We enforce this with our own `IceCandidate` type which implements
`PartialOrd` and `Ord`. This now moves the deserialisation for the
portal messages to a `Deserialise` impl on this type. In order to ensure
that a single faulty candidate doesn't invalidate the entire list, we
use `serde_with` to skip over those elements that cannot be
deserialised.
This commit is contained in:
Thomas Eizinger
2025-08-01 01:57:29 +00:00
committed by GitHub
parent 9b8efdcf08
commit 17a18fdfbb
11 changed files with 223 additions and 68 deletions

68
rust/Cargo.lock generated
View File

@@ -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",

View File

@@ -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"

View File

@@ -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]

View File

@@ -209,3 +209,82 @@ impl IpStack {
}
}
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct IceCandidate(str0m::Candidate);
impl From<str0m::Candidate> for IceCandidate {
fn from(value: str0m::Candidate) -> Self {
Self(value)
}
}
impl From<IceCandidate> for str0m::Candidate {
fn from(value: IceCandidate) -> Self {
value.0
}
}
impl PartialOrd for IceCandidate {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
serializer.serialize_str(&self.0.to_sdp_string())
}
}
impl<'de> Deserialize<'de> for IceCandidate {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
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()
}
}

View File

@@ -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<TId, RId>(
.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<TId>(
pending_events.push_back(Event::NewIceCandidate {
connection: id,
candidate: candidate.to_sdp_string(),
candidate: candidate.clone(),
})
}
@@ -1437,7 +1425,7 @@ fn remove_local_candidate<TId>(
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<TId>(
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<str0m::IceCreds> for Credentials {
#[derive(Debug, PartialEq, Clone)]
pub enum Event<TId> {
/// 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),

View File

@@ -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 }

View File

@@ -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::<GatewayId, BTreeSet<String>>::default();
let mut removed_ice_candidates = BTreeMap::<GatewayId, BTreeSet<String>>::default();
let mut added_ice_candidates = BTreeMap::<GatewayId, BTreeSet<IceCandidate>>::default();
let mut removed_ice_candidates = BTreeMap::<GatewayId, BTreeSet<IceCandidate>>::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);

View File

@@ -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::<ClientId, BTreeSet<String>>::default();
let mut removed_ice_candidates = BTreeMap::<ClientId, BTreeSet<String>>::default();
let mut added_ice_candidates = BTreeMap::<ClientId, BTreeSet<IceCandidate>>::default();
let mut removed_ice_candidates = BTreeMap::<ClientId, BTreeSet<IceCandidate>>::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(_) => {}
}

View File

@@ -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<String>,
candidates: BTreeSet<IceCandidate>,
},
RemovedIceCandidates {
conn_id: GatewayId,
candidates: BTreeSet<String>,
candidates: BTreeSet<IceCandidate>,
},
ConnectionIntent {
resource: ResourceId,
@@ -465,11 +465,11 @@ impl IpConfig {
pub enum GatewayEvent {
AddedIceCandidates {
conn_id: ClientId,
candidates: BTreeSet<String>,
candidates: BTreeSet<IceCandidate>,
},
RemovedIceCandidates {
conn_id: ClientId,
candidates: BTreeSet<String>,
candidates: BTreeSet<IceCandidate>,
},
ResolveDns(ResolveDnsRequest),
}

View File

@@ -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<GatewayId>,
/// Actual RTC ice candidates
pub candidates: BTreeSet<String>,
pub candidates: BTreeSet<IceCandidate>,
}
#[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<String>,
#[serde_as(as = "serde_with::VecSkipError<_>")]
pub candidates: Vec<IceCandidate>,
}
// 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);
}
}

View File

@@ -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<ClientId>,
/// Actual RTC ice candidates
pub candidates: BTreeSet<String>,
pub candidates: BTreeSet<IceCandidate>,
}
/// 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<String>,
#[serde_as(as = "serde_with::VecSkipError<_>")]
pub candidates: Vec<IceCandidate>,
}
// 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);
}
}