From 804ef7a3fb7d1718f41463d535f7fdc7b176606c Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 4 Nov 2025 04:55:48 +1100 Subject: [PATCH] fix(connlib): retain order of system/upstream DNS servers (#10773) Right now, connlib hands out a `BiMap` of sentinel IPs <> upstream servers whenever it emits a `TunInterfaceUpdated` event. This `BiMap` internally uses two `HashMap`s. The iteration order of `HashMap`s is non-deterministic and therefore, we lose the order in which the upstream / system resolvers have been passed to us originally. To prevent that, we now emit a dedicated `DnsMapping` type that does not expose its internal data structure but only getters for retrieving the sentinel and upstream servers. Internally, it uses a `Vec` to store this mapping and thus retains the original order. This is asserted as part of our proptests by comparing the resulting `Vec`s. This fix is preceded by a few refactorings that encapsulate the code for creating and updating this DNS mapping. Resolves: #8439 --- rust/client-ffi/src/lib.rs | 5 +- rust/connlib/tunnel/src/client.rs | 262 ++++-------------- rust/connlib/tunnel/src/client/dns_config.rs | 211 ++++++++++++++ rust/connlib/tunnel/src/lib.rs | 10 +- rust/connlib/tunnel/src/tests/sim_client.rs | 31 +-- rust/connlib/tunnel/src/tests/sim_gateway.rs | 5 +- rust/connlib/tunnel/src/tests/sut.rs | 12 +- rust/gui-client/src-tauri/src/service.rs | 2 +- rust/headless-client/src/main.rs | 2 +- website/src/components/Changelog/Android.tsx | 4 + website/src/components/Changelog/Apple.tsx | 4 + website/src/components/Changelog/GUI.tsx | 4 + website/src/components/Changelog/Headless.tsx | 4 + 13 files changed, 314 insertions(+), 242 deletions(-) create mode 100644 rust/connlib/tunnel/src/client/dns_config.rs diff --git a/rust/client-ffi/src/lib.rs b/rust/client-ffi/src/lib.rs index 1bc03563d..01c65f680 100644 --- a/rust/client-ffi/src/lib.rs +++ b/rust/client-ffi/src/lib.rs @@ -379,9 +379,10 @@ impl Session { pub async fn next_event(&self) -> Option { match self.events.lock().await.next().await? { client_shared::Event::TunInterfaceUpdated(config) => { - let dns: Vec = config + let dns = config .dns_by_sentinel - .left_values() + .sentinel_ips() + .into_iter() .map(|ip| ip.to_string()) .collect(); diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index b3bc94586..8f56f3872 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -1,9 +1,11 @@ mod dns_cache; +pub(crate) mod dns_config; mod dns_resource_nat; mod gateway_on_client; mod pending_tun_update; mod resource; +use crate::client::dns_config::DnsConfig; pub(crate) use crate::client::gateway_on_client::GatewayOnClient; use crate::client::pending_tun_update::PendingTunUpdate; use boringtun::x25519; @@ -20,13 +22,12 @@ use secrecy::ExposeSecret as _; use crate::client::dns_cache::DnsCache; use crate::dns::{DnsResourceRecord, StubResolver}; use crate::expiring_map::{self, ExpiringMap}; -use crate::messages::{DnsServer, Interface as InterfaceConfig, IpDnsServer}; +use crate::messages::Interface as InterfaceConfig; use crate::messages::{IceCredentials, SecretKey}; use crate::peer_store::PeerStore; 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, IceCandidate, PublicKey, RelayId, ResourceId, ResourceStatus, ResourceView, }; @@ -40,7 +41,7 @@ use itertools::Itertools; use crate::ClientEvent; use lru::LruCache; use snownet::{ClientNode, NoTurnServers, RelaySocket, Transmit}; -use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, VecDeque}; +use std::collections::{BTreeMap, BTreeSet, HashMap, VecDeque}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; use std::num::NonZeroUsize; use std::ops::ControlFlow; @@ -118,15 +119,9 @@ pub struct ClientState { /// All resources indexed by their ID. resources_by_id: BTreeMap, - /// The DNS resolvers configured on the system outside of connlib. - system_resolvers: Vec, - /// The DNS resolvers configured in the portal. - /// - /// Has priority over system-configured DNS servers. - upstream_dns: Vec, + /// Manages the DNS configuration. + dns_config: DnsConfig, - /// Maps from connlib-assigned IP of a DNS server back to the originally configured system DNS resolver. - dns_mapping: BiMap, /// UDP DNS queries that had their destination IP mangled to redirect them to another DNS resolver through the tunnel. udp_dns_sockets_by_upstream_and_query_id: ExpiringMap<(SocketAddr, u16), SocketAddr>, /// Manages internal dns records and emits forwarding event when not internally handled @@ -209,12 +204,11 @@ impl ClientState { active_cidr_resources: IpNetworkTable::new(), resources_by_id: Default::default(), peers: Default::default(), - dns_mapping: Default::default(), + dns_config: Default::default(), buffered_events: Default::default(), tun_config: Default::default(), buffered_packets: Default::default(), node: ClientNode::new(seed, now), - system_resolvers: Default::default(), sites_status: Default::default(), gateways_site: Default::default(), udp_dns_sockets_by_upstream_and_query_id: Default::default(), @@ -223,7 +217,6 @@ impl ClientState { buffered_transmits: Default::default(), is_internet_resource_active, recently_connected_gateways: LruCache::new(MAX_REMEMBERED_GATEWAYS), - upstream_dns: Default::default(), buffered_dns_queries: Default::default(), tcp_dns_client: dns_over_tcp::Client::new(now, seed), tcp_dns_server: dns_over_tcp::Server::new(now), @@ -629,9 +622,10 @@ impl ClientState { dst: SocketAddr, message: dns_types::Response, ) -> anyhow::Result<()> { - let saddr = *self - .dns_mapping - .get_by_right(&DnsServer::from(from)) + let saddr = self + .dns_config + .mapping() + .sentinel_by_upstream(from) .context("Unknown DNS server")?; let ip_packet = ip_packet::make::udp_packet( @@ -807,15 +801,11 @@ impl ClientState { Ok(Ok(())) } - fn is_upstream_set_by_the_portal(&self) -> bool { - !self.upstream_dns.is_empty() - } - /// For DNS queries to IPs that are a CIDR resources we want to mangle and forward to the gateway that handles that resource. /// /// We only want to do this if the upstream DNS server is set by the portal, otherwise, the server might be a local IP. fn should_forward_dns_query_to_gateway(&self, dns_server: IpAddr) -> bool { - if !self.is_upstream_set_by_the_portal() { + if !self.dns_config.has_custom_upstream() { return false; } if self.active_internet_resource().is_some() { @@ -836,7 +826,7 @@ impl ClientState { return ControlFlow::Break(()); } - let Some(upstream) = self.dns_mapping.get_by_left(&dst).map(|s| s.address()) else { + let Some(upstream) = self.dns_config.mapping().upstream_by_sentinel(dst) else { return ControlFlow::Continue(packet); // Not for our DNS resolver. }; @@ -911,10 +901,6 @@ impl ClientState { self.resources_gateways.get(resource).copied() } - fn set_dns_mapping(&mut self, new_mapping: BiMap) { - self.dns_mapping = new_mapping; - } - fn initialise_tcp_dns_client(&mut self) { let Some(tun_config) = self.tun_config.as_ref() else { return; @@ -927,9 +913,11 @@ impl ClientState { fn initialise_tcp_dns_server(&mut self) { let sentinel_sockets = self - .dns_mapping - .left_values() - .map(|ip| SocketAddr::new(*ip, DNS_PORT)) + .dns_config + .mapping() + .sentinel_ips() + .into_iter() + .map(|ip| SocketAddr::new(ip, DNS_PORT)) .collect(); self.tcp_dns_server @@ -966,10 +954,6 @@ impl ClientState { self.maybe_update_tun_routes(); } - pub fn dns_mapping(&self) -> BiMap { - self.dns_mapping.clone() - } - #[tracing::instrument(level = "debug", skip_all, fields(gateway = %disconnected_gateway))] fn cleanup_connected_gateway(&mut self, disconnected_gateway: &GatewayId) { self.update_site_status_by_gateway(disconnected_gateway, ResourceStatus::Unknown); @@ -1046,16 +1030,38 @@ impl ClientState { } pub fn update_system_resolvers(&mut self, new_dns: Vec) { - tracing::debug!(servers = ?new_dns, "Received system-defined DNS servers"); + let changed = self.dns_config.update_system_resolvers(new_dns); - self.system_resolvers = new_dns; + if !changed { + return; + } - self.update_dns_mapping() + self.dns_cache.flush("DNS servers changed"); + + let Some(config) = self.tun_config.clone() else { + tracing::debug!("Unable to update DNS servers without interface configuration"); + return; + }; + + let dns_by_sentinel = self.dns_config.mapping(); + + self.maybe_update_tun_config(TunConfig { + dns_by_sentinel, + ..config + }); } pub fn update_interface_config(&mut self, config: InterfaceConfig) { tracing::trace!(upstream_dns = ?config.upstream_dns, search_domain = ?config.search_domain, ipv4 = %config.ipv4, ipv6 = %config.ipv6, "Received interface configuration from portal"); + let changed = self + .dns_config + .update_upstream_resolvers(config.upstream_dns); + + if changed { + self.dns_cache.flush("DNS servers changed"); + } + // Create a new `TunConfig` by patching the corresponding fields of the existing one. let new_tun_config = self .tun_config @@ -1065,7 +1071,7 @@ impl ClientState { v4: config.ipv4, v6: config.ipv6, }, - dns_by_sentinel: existing.dns_by_sentinel.clone(), + dns_by_sentinel: self.dns_config.mapping(), search_domain: config.search_domain.clone(), ipv4_routes: existing.ipv4_routes.clone(), ipv6_routes: existing.ipv6_routes.clone(), @@ -1081,7 +1087,7 @@ impl ClientState { v4: config.ipv4, v6: config.ipv6, }, - dns_by_sentinel: Default::default(), + dns_by_sentinel: self.dns_config.mapping(), search_domain: config.search_domain.clone(), ipv4_routes, ipv6_routes, @@ -1090,9 +1096,6 @@ impl ClientState { // Apply the new `TunConfig` if it differs from the existing one. self.maybe_update_tun_config(new_tun_config); - - self.upstream_dns = config.upstream_dns; - self.update_dns_mapping(); } pub fn poll_packets(&mut self) -> Option { @@ -1408,11 +1411,14 @@ impl ClientState { fn handle_tcp_dns_query(&mut self, query: dns_over_tcp::Query, now: Instant) { let query_id = query.message.id(); - let Some(upstream) = self.dns_mapping.get_by_left(&query.local.ip()) else { + let Some(server) = self + .dns_config + .mapping() + .upstream_by_sentinel(query.local.ip()) + else { // This is highly-unlikely but might be possible if our DNS mapping changes whilst the TCP DNS server is processing a request. return; }; - let server = upstream.address(); if let Some(response) = self.dns_cache.try_answer(&query.message, now) { unwrap_or_debug!( @@ -1899,55 +1905,6 @@ impl ClientState { } } - fn update_dns_mapping(&mut self) { - let Some(config) = self.tun_config.clone() else { - // For the Tauri clients this can happen because it's called immediately after phoenix_channel's connect, before on_set_interface_config - tracing::debug!("Unable to update DNS servers without interface configuration"); - - return; - }; - - let effective_dns_servers = - effective_dns_servers(self.upstream_dns.clone(), self.system_resolvers.clone()); - - if HashSet::<&DnsServer>::from_iter(effective_dns_servers.iter()) - == HashSet::from_iter(self.dns_mapping.right_values()) - { - tracing::debug!(servers = ?effective_dns_servers, "Effective DNS servers are unchanged"); - - return; - } - - let dns_mapping = sentinel_dns_mapping( - &effective_dns_servers, - self.dns_mapping() - .left_values() - .copied() - .map(Into::into) - .collect_vec(), - ); - - let (ipv4_routes, ipv6_routes) = self.routes().partition_map(|route| match route { - IpNetwork::V4(v4) => itertools::Either::Left(v4), - IpNetwork::V6(v6) => itertools::Either::Right(v6), - }); - - let new_tun_config = TunConfig { - ip: config.ip, - dns_by_sentinel: dns_mapping - .iter() - .map(|(sentinel_dns, effective_dns)| (*sentinel_dns, effective_dns.address())) - .collect::>(), - search_domain: config.search_domain, - ipv4_routes, - ipv6_routes, - }; - - self.set_dns_mapping(dns_mapping); - self.maybe_update_tun_config(new_tun_config); - self.dns_cache.flush("DNS servers changed"); - } - pub fn update_relays( &mut self, to_remove: BTreeSet, @@ -1996,61 +1953,6 @@ fn peer_by_resource_mut<'p>( Some(peer) } -fn effective_dns_servers( - upstream_dns: Vec, - default_resolvers: Vec, -) -> Vec { - let mut upstream_dns = upstream_dns.into_iter().filter_map(not_sentinel).peekable(); - if upstream_dns.peek().is_some() { - return upstream_dns.collect(); - } - - let mut dns_servers = default_resolvers - .into_iter() - .map(|ip| { - DnsServer::IpPort(IpDnsServer { - address: (ip, DNS_PORT).into(), - }) - }) - .filter_map(not_sentinel) - .peekable(); - - if dns_servers.peek().is_none() { - tracing::info!( - "No system default DNS servers available! Can't initialize resolver. DNS resources won't work." - ); - return Vec::new(); - } - - dns_servers.collect() -} - -fn not_sentinel(srv: DnsServer) -> Option { - let is_v4_dns = IpNetwork::V4(DNS_SENTINELS_V4).contains(srv.ip()); - let is_v6_dns = IpNetwork::V6(DNS_SENTINELS_V6).contains(srv.ip()); - - (!is_v4_dns && !is_v6_dns).then_some(srv) -} - -fn sentinel_dns_mapping( - dns: &[DnsServer], - old_sentinels: Vec, -) -> BiMap { - let mut ip_provider = IpProvider::for_stub_dns_servers(old_sentinels); - - dns.iter() - .cloned() - .map(|i| { - ( - ip_provider - .get_proxy_ip_for(&i.ip()) - .expect("We only support up to 256 IPv4 DNS servers and 256 IPv6 DNS servers"), - i, - ) - }) - .collect() -} - /// Compares the given [`IpAddr`] against a static set of ignored IPs that are definitely not resources. fn is_definitely_not_a_resource(ip: IpAddr) -> bool { /// Source: https://en.wikipedia.org/wiki/Multicast_address#Notable_IPv4_multicast_addresses @@ -2172,8 +2074,12 @@ impl IpProvider { ) } - pub fn for_stub_dns_servers(exclusions: Vec) -> Self { - IpProvider::new(DNS_SENTINELS_V4, DNS_SENTINELS_V6, exclusions) + pub fn for_stub_dns_servers(old_servers: Vec) -> Self { + IpProvider::new( + DNS_SENTINELS_V4, + DNS_SENTINELS_V6, + old_servers.into_iter().map(IpNetwork::from).collect(), + ) } fn new(ipv4: Ipv4Network, ipv6: Ipv6Network, exclusions: Vec) -> Self { @@ -2229,66 +2135,12 @@ mod tests { assert!(is_definitely_not_a_resource(ip("ff02::2"))) } - #[test] - fn sentinel_dns_works() { - let servers = dns_list(); - let sentinel_dns = sentinel_dns_mapping(&servers, vec![]); - - for server in servers { - assert!( - sentinel_dns - .get_by_right(&server) - .is_some_and(|s| sentinel_ranges().iter().any(|e| e.contains(*s))) - ) - } - } - - #[test] - fn sentinel_dns_excludes_old_ones() { - let servers = dns_list(); - let sentinel_dns_old = sentinel_dns_mapping(&servers, vec![]); - let sentinel_dns_new = sentinel_dns_mapping( - &servers, - sentinel_dns_old - .left_values() - .copied() - .map(Into::into) - .collect_vec(), - ); - - assert!( - HashSet::<&IpAddr>::from_iter(sentinel_dns_old.left_values()) - .is_disjoint(&HashSet::from_iter(sentinel_dns_new.left_values())) - ) - } - impl ClientState { pub fn for_test() -> ClientState { ClientState::new(rand::random(), Default::default(), false, Instant::now()) } } - fn sentinel_ranges() -> Vec { - vec![ - IpNetwork::V4(DNS_SENTINELS_V4), - IpNetwork::V6(DNS_SENTINELS_V6), - ] - } - - fn dns_list() -> Vec { - vec![ - dns("1.1.1.1:53"), - dns("1.0.0.1:53"), - dns("[2606:4700:4700::1111]:53"), - ] - } - - fn dns(address: &str) -> DnsServer { - DnsServer::IpPort(IpDnsServer { - address: address.parse().unwrap(), - }) - } - fn ip(addr: &str) -> IpAddr { addr.parse().unwrap() } @@ -2296,6 +2148,8 @@ mod tests { #[cfg(all(test, feature = "proptest"))] mod proptests { + use std::collections::HashSet; + use super::*; use crate::proptest::*; use connlib_model::ResourceView; diff --git a/rust/connlib/tunnel/src/client/dns_config.rs b/rust/connlib/tunnel/src/client/dns_config.rs new file mode 100644 index 000000000..348c2449d --- /dev/null +++ b/rust/connlib/tunnel/src/client/dns_config.rs @@ -0,0 +1,211 @@ +use std::{ + collections::HashSet, + net::{IpAddr, SocketAddr}, +}; + +use ip_network::IpNetwork; + +use crate::{ + client::{DNS_SENTINELS_V4, DNS_SENTINELS_V6, IpProvider}, + dns::DNS_PORT, + messages::DnsServer, +}; + +#[derive(Debug, Default)] +pub(crate) struct DnsConfig { + /// The DNS resolvers configured on the system outside of connlib. + system_resolvers: Vec, + /// The DNS resolvers configured in the portal. + /// + /// Has priority over system-configured DNS servers. + upstream_dns: Vec, + + /// Maps from connlib-assigned IP of a DNS server back to the originally configured system DNS resolver. + mapping: DnsMapping, +} + +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct DnsMapping { + inner: Vec<(IpAddr, SocketAddr)>, +} + +impl DnsMapping { + pub fn sentinel_ips(&self) -> Vec { + self.inner.iter().map(|(ip, _)| ip).copied().collect() + } + + pub fn upstream_sockets(&self) -> Vec { + self.inner + .iter() + .map(|(_, socket)| socket) + .copied() + .collect() + } + + // Implementation note: + // + // These functions perform linear search instead of an O(1) map lookup. + // Most users will only have a handful of DNS servers (like 1-3). + // For such small numbers, linear search is usually more efficient. + // Most importantly, it is much easier for us to retain the ordering of the DNS servers if we don't use a map. + + pub(crate) fn sentinel_by_upstream(&self, upstream: SocketAddr) -> Option { + self.inner + .iter() + .find_map(|(sentinel, candidate)| (candidate == &upstream).then_some(*sentinel)) + } + + pub(crate) fn upstream_by_sentinel(&self, sentinel: IpAddr) -> Option { + self.inner + .iter() + .find_map(|(candidate, upstream)| (candidate == &sentinel).then_some(*upstream)) + } +} + +impl DnsConfig { + #[must_use = "Check if the DNS mapping has changed"] + pub(crate) fn update_system_resolvers(&mut self, servers: Vec) -> bool { + tracing::debug!(?servers, "Received system-defined DNS servers"); + + self.system_resolvers = servers; + + self.update_dns_mapping() + } + + #[must_use = "Check if the DNS mapping has changed"] + pub(crate) fn update_upstream_resolvers(&mut self, servers: Vec) -> bool { + tracing::debug!(?servers, "Received upstream-defined DNS servers"); + + self.upstream_dns = servers.into_iter().map(|s| s.address()).collect(); + + self.update_dns_mapping() + } + + pub(crate) fn has_custom_upstream(&self) -> bool { + !self.upstream_dns.is_empty() + } + + pub(crate) fn mapping(&mut self) -> DnsMapping { + self.mapping.clone() + } + + fn update_dns_mapping(&mut self) -> bool { + let effective_dns_servers = + effective_dns_servers(self.upstream_dns.clone(), self.system_resolvers.clone()); + + if HashSet::::from_iter(effective_dns_servers.clone()) + == HashSet::from_iter(self.mapping.upstream_sockets()) + { + tracing::debug!(servers = ?effective_dns_servers, "Effective DNS servers are unchanged"); + + return false; + } + + self.mapping = sentinel_dns_mapping(&effective_dns_servers, self.mapping.sentinel_ips()); + + true + } +} + +fn effective_dns_servers( + upstream_dns: Vec, + default_resolvers: Vec, +) -> Vec { + let mut upstream_dns = upstream_dns.into_iter().filter_map(not_sentinel).peekable(); + if upstream_dns.peek().is_some() { + return upstream_dns.collect(); + } + + let mut dns_servers = default_resolvers + .into_iter() + .map(|ip| SocketAddr::new(ip, DNS_PORT)) + .filter_map(not_sentinel) + .peekable(); + + if dns_servers.peek().is_none() { + tracing::info!( + "No system default DNS servers available! Can't initialize resolver. DNS resources won't work." + ); + return Vec::new(); + } + + dns_servers.collect() +} + +fn sentinel_dns_mapping(dns: &[SocketAddr], old_sentinels: Vec) -> DnsMapping { + let mut ip_provider = IpProvider::for_stub_dns_servers(old_sentinels); + + let mapping = dns + .iter() + .copied() + .map(|i| { + ( + ip_provider + .get_proxy_ip_for(&i.ip()) + .expect("We only support up to 256 IPv4 DNS servers and 256 IPv6 DNS servers"), + i, + ) + }) + .collect(); + + DnsMapping { inner: mapping } +} + +fn not_sentinel(srv: SocketAddr) -> Option { + let is_v4_dns = IpNetwork::V4(DNS_SENTINELS_V4).contains(srv.ip()); + let is_v6_dns = IpNetwork::V6(DNS_SENTINELS_V6).contains(srv.ip()); + + (!is_v4_dns && !is_v6_dns).then_some(srv) +} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use super::*; + + #[test] + fn sentinel_dns_works() { + let servers = dns_list(); + let sentinel_dns = sentinel_dns_mapping(&servers, vec![]); + + for server in servers { + assert!( + sentinel_dns + .sentinel_by_upstream(server) + .is_some_and(|ip| sentinel_ranges().iter().any(|e| e.contains(ip))) + ) + } + } + + #[test] + fn sentinel_dns_excludes_old_ones() { + let servers = dns_list(); + let sentinel_dns_old = sentinel_dns_mapping(&servers, vec![]); + let sentinel_dns_new = sentinel_dns_mapping(&servers, sentinel_dns_old.sentinel_ips()); + + assert!( + HashSet::::from_iter(sentinel_dns_old.sentinel_ips()) + .is_disjoint(&HashSet::from_iter(sentinel_dns_new.sentinel_ips())) + ) + } + + fn sentinel_ranges() -> Vec { + vec![ + IpNetwork::V4(DNS_SENTINELS_V4), + IpNetwork::V6(DNS_SENTINELS_V6), + ] + } + + fn dns_list() -> Vec { + vec![ + dns("1.1.1.1:53"), + dns("1.0.0.1:53"), + dns("[2606:4700:4700::1111]:53"), + ] + } + + fn dns(address: &str) -> SocketAddr { + address.parse().unwrap() + } +} diff --git a/rust/connlib/tunnel/src/lib.rs b/rust/connlib/tunnel/src/lib.rs index 12022f563..b5a2052e0 100644 --- a/rust/connlib/tunnel/src/lib.rs +++ b/rust/connlib/tunnel/src/lib.rs @@ -6,7 +6,6 @@ #![cfg_attr(test, allow(clippy::unwrap_used))] use anyhow::{Context as _, Result}; -use bimap::BiMap; use chrono::Utc; use connlib_model::{ClientId, GatewayId, IceCandidate, PublicKey, ResourceId, ResourceView}; use dns_types::DomainName; @@ -69,6 +68,7 @@ pub type GatewayTunnel = Tunnel; pub type ClientTunnel = Tunnel; pub use client::ClientState; +pub use client::dns_config::DnsMapping; pub use dns::DnsResourceRecord; pub use gateway::{DnsResourceNatEntry, GatewayState, ResolveDnsRequest}; pub use sockets::UdpSocketThreadStopped; @@ -550,7 +550,7 @@ pub struct TunConfig { /// - The "right" values are the effective DNS servers. /// If upstream DNS servers are configured (in the portal), we will use those. /// Otherwise, we will use the DNS servers configured on the system. - pub dns_by_sentinel: BiMap, + pub dns_by_sentinel: DnsMapping, pub search_domain: Option, #[debug("{}", DisplayBTreeSet(ipv4_routes))] @@ -559,12 +559,6 @@ pub struct TunConfig { pub ipv6_routes: BTreeSet, } -impl TunConfig { - pub fn dns_sentinel_ips(&self) -> Vec { - self.dns_by_sentinel.left_values().copied().collect() - } -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct IpConfig { pub v4: Ipv4Addr, diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs index f670b875b..9ff74dcfd 100644 --- a/rust/connlib/tunnel/src/tests/sim_client.rs +++ b/rust/connlib/tunnel/src/tests/sim_client.rs @@ -7,12 +7,11 @@ use super::{ strategies::latency, transition::{DPort, Destination, DnsQuery, DnsTransport, Identifier, SPort, Seq}, }; -use crate::{ClientState, DnsResourceRecord, proptest::*}; +use crate::{ClientState, DnsMapping, DnsResourceRecord, proptest::*}; use crate::{ client::{CidrResource, DnsResource, InternetResource, Resource}, messages::{DnsServer, Interface}, }; -use bimap::BiMap; use connlib_model::{ClientId, GatewayId, RelayId, ResourceId, ResourceStatus, Site, SiteId}; use dns_types::{DomainName, Query, RecordData, RecordType}; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; @@ -47,7 +46,7 @@ pub(crate) struct SimClient { pub(crate) dns_resource_record_cache: BTreeSet, /// Bi-directional mapping between connlib's sentinel DNS IPs and the effective DNS servers. - dns_by_sentinel: BiMap, + dns_by_sentinel: DnsMapping, pub(crate) ipv4_routes: BTreeSet, pub(crate) ipv6_routes: BTreeSet, @@ -123,26 +122,26 @@ impl SimClient { ); self.search_domain = None; - self.dns_by_sentinel.clear(); + self.dns_by_sentinel = DnsMapping::default(); self.ipv4_routes.clear(); self.ipv6_routes.clear(); } /// Returns the _effective_ DNS servers that connlib is using. - pub(crate) fn effective_dns_servers(&self) -> BTreeSet { - self.dns_by_sentinel.right_values().copied().collect() + pub(crate) fn effective_dns_servers(&self) -> Vec { + self.dns_by_sentinel.upstream_sockets() } pub(crate) fn effective_search_domain(&self) -> Option { self.search_domain.clone() } - pub(crate) fn set_new_dns_servers(&mut self, mapping: BiMap) { + pub(crate) fn set_new_dns_servers(&mut self, mapping: DnsMapping) { self.dns_by_sentinel = mapping; self.tcp_dns_client.reset(); } - pub(crate) fn dns_mapping(&self) -> &BiMap { + pub(crate) fn dns_mapping(&self) -> &DnsMapping { &self.dns_by_sentinel } @@ -155,7 +154,7 @@ impl SimClient { dns_transport: DnsTransport, now: Instant, ) -> Option { - let Some(sentinel) = self.dns_by_sentinel.get_by_right(&upstream).copied() else { + let Some(sentinel) = self.dns_by_sentinel.sentinel_by_upstream(upstream) else { tracing::error!(%upstream, "Unknown DNS server"); return None; }; @@ -309,8 +308,8 @@ impl SimClient { .expect("ip packets on port 53 to be DNS packets"); // Map back to upstream socket so we can assert on it correctly. - let sentinel = SocketAddr::from((packet.source(), udp.source_port())); - let Some(upstream) = self.upstream_dns_by_sentinel(&sentinel) else { + let sentinel = packet.source(); + let Some(upstream) = self.dns_by_sentinel.upstream_by_sentinel(sentinel) else { tracing::error!(%sentinel, mapping = ?self.dns_by_sentinel, "Unknown DNS server"); return; }; @@ -374,12 +373,6 @@ impl SimClient { ) } - fn upstream_dns_by_sentinel(&self, sentinel: &SocketAddr) -> Option { - let socket = self.dns_by_sentinel.get_by_left(&sentinel.ip())?; - - Some(*socket) - } - pub(crate) fn handle_dns_response(&mut self, response: &dns_types::Response) { for record in response.records() { #[expect(clippy::wildcard_enum_match_arm)] @@ -1052,7 +1045,9 @@ impl RefClient { /// /// If there are upstream DNS servers configured in the portal, it should use those. /// Otherwise it should use whatever was configured on the system prior to connlib starting. - pub(crate) fn expected_dns_servers(&self) -> BTreeSet { + /// + /// This purposely returns a `Vec` so we also assert the order! + pub(crate) fn expected_dns_servers(&self) -> Vec { if !self.upstream_dns_resolvers.is_empty() { return self .upstream_dns_resolvers diff --git a/rust/connlib/tunnel/src/tests/sim_gateway.rs b/rust/connlib/tunnel/src/tests/sim_gateway.rs index 4f4d62e5a..6dd2e0729 100644 --- a/rust/connlib/tunnel/src/tests/sim_gateway.rs +++ b/rust/connlib/tunnel/src/tests/sim_gateway.rs @@ -139,7 +139,7 @@ impl SimGateway { pub(crate) fn deploy_new_dns_servers( &mut self, - dns_servers: impl Iterator, + dns_servers: impl IntoIterator, now: Instant, ) { self.udp_dns_server_resources.clear(); @@ -151,7 +151,8 @@ impl SimGateway { return; }; - for server in dns_servers + for server in iter::empty() + .chain(dns_servers) .chain(iter::once(SocketAddr::from(( ip_config.v4, tun_dns_server_port, diff --git a/rust/connlib/tunnel/src/tests/sut.rs b/rust/connlib/tunnel/src/tests/sut.rs index bb65a7952..e2299dccf 100644 --- a/rust/connlib/tunnel/src/tests/sut.rs +++ b/rust/connlib/tunnel/src/tests/sut.rs @@ -643,10 +643,13 @@ impl TunnelTest { while let Some(result) = c.tcp_dns_client.poll_query_result() { match result.result { Ok(message) => { - let upstream = c.dns_mapping().get_by_left(&result.server.ip()).unwrap(); + let upstream = c + .dns_mapping() + .upstream_by_sentinel(result.server.ip()) + .unwrap(); c.received_tcp_dns_responses - .insert((*upstream, result.query.id())); + .insert((upstream, result.query.id())); c.handle_dns_response(&message) } Err(e) => { @@ -898,10 +901,7 @@ impl TunnelTest { for gateway in self.gateways.values_mut() { gateway.exec_mut(|g| { - g.deploy_new_dns_servers( - config.dns_by_sentinel.right_values().copied(), - now, - ) + g.deploy_new_dns_servers(config.dns_by_sentinel.upstream_sockets(), now) }) } diff --git a/rust/gui-client/src-tauri/src/service.rs b/rust/gui-client/src-tauri/src/service.rs index c3eb2e881..29f72d168 100644 --- a/rust/gui-client/src-tauri/src/service.rs +++ b/rust/gui-client/src-tauri/src/service.rs @@ -495,7 +495,7 @@ impl<'a> Handler<'a> { self.tun_device.set_ips(config.ip.v4, config.ip.v6).await?; self.dns_controller - .set_dns(config.dns_sentinel_ips(), config.search_domain) + .set_dns(config.dns_by_sentinel.sentinel_ips(), config.search_domain) .await?; self.tun_device .set_routes(config.ipv4_routes, config.ipv6_routes) diff --git a/rust/headless-client/src/main.rs b/rust/headless-client/src/main.rs index 14c6e67c2..34c657f1b 100644 --- a/rust/headless-client/src/main.rs +++ b/rust/headless-client/src/main.rs @@ -409,7 +409,7 @@ fn try_main() -> Result<()> { } client_shared::Event::TunInterfaceUpdated(config) => { tun_device.set_ips(config.ip.v4, config.ip.v6).await?; - dns_controller.set_dns(config.dns_sentinel_ips(), config.search_domain).await?; + dns_controller.set_dns(config.dns_by_sentinel.sentinel_ips(), config.search_domain).await?; tun_device.set_routes(config.ipv4_routes, config.ipv6_routes).await?; // `on_set_interface_config` is guaranteed to be called when the tunnel is completely ready diff --git a/website/src/components/Changelog/Android.tsx b/website/src/components/Changelog/Android.tsx index ed50b756d..8323946d3 100644 --- a/website/src/components/Changelog/Android.tsx +++ b/website/src/components/Changelog/Android.tsx @@ -24,6 +24,10 @@ export default function Android() { Fixes an issue where the reported client version was out of date. + + Fixes an issue where the order of upstream / system DNS resolvers was + not respected. + diff --git a/website/src/components/Changelog/Apple.tsx b/website/src/components/Changelog/Apple.tsx index 4beec89bf..176cebb72 100644 --- a/website/src/components/Changelog/Apple.tsx +++ b/website/src/components/Changelog/Apple.tsx @@ -28,6 +28,10 @@ export default function Apple() { Fixes an issue where the reported client version was out of date. + + Fixes an issue where the order of upstream / system DNS resolvers was + not respected. + diff --git a/website/src/components/Changelog/GUI.tsx b/website/src/components/Changelog/GUI.tsx index d45c4e1c4..401031493 100644 --- a/website/src/components/Changelog/GUI.tsx +++ b/website/src/components/Changelog/GUI.tsx @@ -17,6 +17,10 @@ export default function GUI({ os }: { os: OS }) { the local network were not routable. )} + + Fixes an issue where the order of upstream / system DNS resolvers was + not respected. + diff --git a/website/src/components/Changelog/Headless.tsx b/website/src/components/Changelog/Headless.tsx index c7c54c23d..37e8d207f 100644 --- a/website/src/components/Changelog/Headless.tsx +++ b/website/src/components/Changelog/Headless.tsx @@ -16,6 +16,10 @@ export default function Headless({ os }: { os: OS }) { the local network were not routable. )} + + Fixes an issue where the order of upstream / system DNS resolvers was + not respected. +