diff --git a/rust/connlib/clients/shared/src/eventloop.rs b/rust/connlib/clients/shared/src/eventloop.rs index cd727e6b0..457de12b3 100644 --- a/rust/connlib/clients/shared/src/eventloop.rs +++ b/rust/connlib/clients/shared/src/eventloop.rs @@ -171,15 +171,11 @@ where firezone_tunnel::ClientEvent::ResourcesChanged { resources } => { self.callbacks.on_update_resources(resources) } - firezone_tunnel::ClientEvent::TunInterfaceUpdated { - ip4, - ip6, - dns_by_sentinel, - } => { - let dns_servers = dns_by_sentinel.left_values().copied().collect(); + firezone_tunnel::ClientEvent::TunInterfaceUpdated(config) => { + let dns_servers = config.dns_by_sentinel.left_values().copied().collect(); self.callbacks - .on_set_interface_config(ip4, ip6, dns_servers); + .on_set_interface_config(config.ip4, config.ip6, dns_servers); } firezone_tunnel::ClientEvent::TunRoutesUpdated { ip4, ip6 } => { self.callbacks.on_update_routes(ip4, ip6); diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 65b4fa9f3..616510af7 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -1,6 +1,6 @@ use crate::dns::StubResolver; use crate::peer_store::PeerStore; -use crate::{dns, BUF_SIZE}; +use crate::{dns, TunConfig, BUF_SIZE}; use anyhow::Context; use bimap::BiMap; use connlib_shared::callbacks::Status; @@ -244,6 +244,10 @@ pub struct ClientState { /// 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. dns_mapping: BiMap, @@ -267,7 +271,7 @@ pub struct ClientState { stub_resolver: StubResolver, /// Configuration of the TUN device, when it is up. - interface_config: Option, + tun_config: Option, /// Resources that have been disabled by the UI disabled_resources: BTreeSet, @@ -302,7 +306,7 @@ impl ClientState { peers: Default::default(), dns_mapping: Default::default(), buffered_events: Default::default(), - interface_config: Default::default(), + tun_config: Default::default(), buffered_packets: Default::default(), node: ClientNode::new(private_key.into(), BUF_SIZE, seed), system_resolvers: Default::default(), @@ -315,17 +319,18 @@ impl ClientState { buffered_transmits: Default::default(), internet_resource: None, recently_connected_gateways: LruCache::new(MAX_REMEMBERED_GATEWAYS), + upstream_dns: Default::default(), } } #[cfg(all(test, feature = "proptest"))] pub(crate) fn tunnel_ip4(&self) -> Option { - Some(self.interface_config.as_ref()?.ipv4) + Some(self.tun_config.as_ref()?.ip4) } #[cfg(all(test, feature = "proptest"))] pub(crate) fn tunnel_ip6(&self) -> Option { - Some(self.interface_config.as_ref()?.ipv6) + Some(self.tun_config.as_ref()?.ip6) } #[cfg(all(test, feature = "proptest"))] @@ -618,11 +623,7 @@ impl ClientState { } fn is_upstream_set_by_the_portal(&self) -> bool { - let Some(interface) = &self.interface_config else { - return false; - }; - - !interface.upstream_dns.is_empty() + !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. @@ -786,8 +787,6 @@ impl ClientState { } fn set_dns_mapping(&mut self, new_mapping: BiMap) { - tracing::debug!(mapping = ?new_mapping, "Updating DNS servers"); - self.dns_mapping = new_mapping; self.mangled_dns_queries.clear(); } @@ -828,6 +827,8 @@ impl ClientState { .map(|(ip, _)| ip) .chain(iter::once(IPV4_RESOURCES.into())) .chain(iter::once(IPV6_RESOURCES.into())) + .chain(iter::once(DNS_SENTINELS_V4.into())) + .chain(iter::once(DNS_SENTINELS_V6.into())) .chain( self.internet_resource .map(|_| Ipv4Network::DEFAULT_ROUTE.into()), @@ -836,7 +837,6 @@ impl ClientState { self.internet_resource .map(|_| Ipv6Network::DEFAULT_ROUTE.into()), ) - .chain(self.dns_mapping.left_values().copied().map(Into::into)) } fn is_resource_enabled(&self, resource: &ResourceId) -> bool { @@ -862,13 +862,32 @@ impl ClientState { } pub(crate) fn update_system_resolvers(&mut self, new_dns: Vec) { + tracing::debug!(servers = ?new_dns, "Received system-defined DNS servers"); + self.system_resolvers = new_dns; self.update_dns_mapping() } pub(crate) fn update_interface_config(&mut self, config: InterfaceConfig) { - self.interface_config = Some(config); + tracing::trace!(upstream_dns = ?config.upstream_dns, ipv4 = %config.ipv4, ipv6 = %config.ipv6, "Received interface configuration from portal"); + + match self.tun_config.as_mut() { + Some(existing) => { + // We don't really expect these to change but let's update them anyway. + existing.ip4 = config.ipv4; + existing.ip6 = config.ipv6; + } + None => { + self.tun_config = Some(TunConfig { + ip4: config.ipv4, + ip6: config.ipv6, + dns_by_sentinel: Default::default(), + }) + } + } + + self.upstream_dns = config.upstream_dns; self.update_dns_mapping() } @@ -1118,7 +1137,7 @@ impl ClientState { } fn update_dns_mapping(&mut self) { - let Some(config) = &self.interface_config else { + 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"); @@ -1126,12 +1145,12 @@ impl ClientState { }; let effective_dns_servers = - effective_dns_servers(config.upstream_dns.clone(), self.system_resolvers.clone()); + 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!("Effective DNS servers are unchanged"); + tracing::debug!(servers = ?effective_dns_servers, "Effective DNS servers are unchanged"); return; } @@ -1145,26 +1164,28 @@ impl ClientState { .collect_vec(), ); - let ip4 = config.ipv4; - let ip6 = config.ipv6; + let new_tun_config = TunConfig { + ip4: config.ip4, + ip6: config.ip6, + dns_by_sentinel: dns_mapping + .iter() + .map(|(sentinel_dns, effective_dns)| (*sentinel_dns, effective_dns.address())) + .collect::>(), + }; self.set_dns_mapping(dns_mapping); + if new_tun_config == config { + tracing::debug!(current = ?config, "TUN device configuration unchanged"); + + return; + } + + tracing::info!(config = ?new_tun_config, "Updating TUN device"); + + self.tun_config = Some(new_tun_config.clone()); self.buffered_events - .push_back(ClientEvent::TunInterfaceUpdated { - ip4, - ip6, - dns_by_sentinel: self - .dns_mapping - .iter() - .map(|(sentinel_dns, effective_dns)| (*sentinel_dns, effective_dns.address())) - .collect(), - }); - self.buffered_events - .push_back(ClientEvent::TunRoutesUpdated { - ip4: self.routes().filter_map(utils::ipv4).collect(), - ip6: self.routes().filter_map(utils::ipv6).collect(), - }); + .push_back(ClientEvent::TunInterfaceUpdated(new_tun_config)); } pub fn update_relays( @@ -1764,7 +1785,9 @@ mod proptests { resource_routes .into_iter() .chain(iter::once(IPV4_RESOURCES.into())) - .chain(iter::once(IPV6_RESOURCES.into())), + .chain(iter::once(IPV6_RESOURCES.into())) + .chain(iter::once(DNS_SENTINELS_V4.into())) + .chain(iter::once(DNS_SENTINELS_V6.into())), ) } diff --git a/rust/connlib/tunnel/src/lib.rs b/rust/connlib/tunnel/src/lib.rs index 8a469b62e..8ea3f70e8 100644 --- a/rust/connlib/tunnel/src/lib.rs +++ b/rust/connlib/tunnel/src/lib.rs @@ -302,23 +302,26 @@ pub enum ClientEvent { resources: Vec, }, // TODO: Make this more fine-granular. - TunInterfaceUpdated { - ip4: Ipv4Addr, - ip6: Ipv6Addr, - /// The map of DNS servers that connlib will use. - /// - /// - The "left" values are the connlib-assigned, proxy (or "sentinel") IPs. - /// - 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. - dns_by_sentinel: BiMap, - }, + TunInterfaceUpdated(TunConfig), TunRoutesUpdated { ip4: Vec, ip6: Vec, }, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct TunConfig { + pub ip4: Ipv4Addr, + pub ip6: Ipv6Addr, + /// The map of DNS servers that connlib will use. + /// + /// - The "left" values are the connlib-assigned, proxy (or "sentinel") IPs. + /// - 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, +} + #[derive(Debug, Clone)] pub enum GatewayEvent { AddedIceCandidates { diff --git a/rust/connlib/tunnel/src/tests/assertions.rs b/rust/connlib/tunnel/src/tests/assertions.rs index de4900438..db5db96fb 100644 --- a/rust/connlib/tunnel/src/tests/assertions.rs +++ b/rust/connlib/tunnel/src/tests/assertions.rs @@ -127,6 +127,15 @@ pub(crate) fn assert_known_hosts_are_valid(ref_client: &RefClient, sim_client: & } } +pub(crate) fn assert_dns_servers_are_valid(ref_client: &RefClient, sim_client: &SimClient) { + let expected = ref_client.expected_dns_servers(); + let actual = sim_client.effective_dns_servers(); + + if actual != expected { + tracing::error!(target: "assertions", ?actual, ?expected, "❌ Effective DNS servers are incorrect"); + } +} + pub(crate) fn assert_dns_packets_properties(ref_client: &RefClient, sim_client: &SimClient) { let unexpected_dns_replies = find_unexpected_entries( &ref_client.expected_dns_handshakes, diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs index aa1c6cb9b..16ee14eb4 100644 --- a/rust/connlib/tunnel/src/tests/reference.rs +++ b/rust/connlib/tunnel/src/tests/reference.rs @@ -59,15 +59,17 @@ impl ReferenceStateMachine for ReferenceState { type Transition = Transition; fn init_state() -> BoxedStrategy { - stub_portal() - .prop_flat_map(|portal| { + (stub_portal(), dns_servers()) + .prop_flat_map(|(portal, dns_servers)| { let gateways = portal.gateways(); let dns_resource_records = portal.dns_resource_records(); - let client = portal.client(); + let client = portal.client( + system_dns_servers(dns_servers.values().cloned().collect()), + upstream_dns_servers(dns_servers.values().cloned().collect()), + ); let relays = relays(); let global_dns_records = global_dns_records(); // Start out with a set of global DNS records so we have something to resolve outside of DNS resources. let drop_direct_client_traffic = any::(); - let dns_servers = dns_servers(); ( client, @@ -77,7 +79,7 @@ impl ReferenceStateMachine for ReferenceState { relays, global_dns_records, drop_direct_client_traffic, - dns_servers, + Just(dns_servers), ) }) .prop_filter_map( @@ -176,11 +178,13 @@ impl ReferenceStateMachine for ReferenceState { CompositeStrategy::default() .with( 1, - update_system_dns_servers(state.dns_servers.values().cloned().collect()), + system_dns_servers(state.dns_servers.values().cloned().collect()) + .prop_map(Transition::UpdateSystemDnsServers), ) .with( 1, - update_upstream_dns_servers(state.dns_servers.values().cloned().collect()), + upstream_dns_servers(state.dns_servers.values().cloned().collect()) + .prop_map(Transition::UpdateUpstreamDnsServers), ) .with_if_not_empty( 5, @@ -410,12 +414,12 @@ impl ReferenceStateMachine for ReferenceState { Transition::UpdateSystemDnsServers(servers) => { state .client - .exec_mut(|client| client.system_dns_resolvers.clone_from(servers)); + .exec_mut(|client| client.set_system_dns_resolvers(servers)); } Transition::UpdateUpstreamDnsServers(servers) => { state .client - .exec_mut(|client| client.upstream_dns_resolvers.clone_from(servers)); + .exec_mut(|client| client.set_upstream_dns_resolvers(servers)); } Transition::RoamClient { ip4, ip6, .. } => { state.network.remove_host(&state.client); diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs index c44b5a93b..f84af2a72 100644 --- a/rust/connlib/tunnel/src/tests/sim_client.rs +++ b/rust/connlib/tunnel/src/tests/sim_client.rs @@ -257,10 +257,10 @@ pub struct RefClient { /// The DNS resolvers configured on the client outside of connlib. #[derivative(Debug = "ignore")] - pub(crate) system_dns_resolvers: Vec, + system_dns_resolvers: Vec, /// The upstream DNS resolvers configured in the portal. #[derivative(Debug = "ignore")] - pub(crate) upstream_dns_resolvers: Vec, + upstream_dns_resolvers: Vec, /// Tracks all resources in the order they have been added in. /// @@ -731,6 +731,18 @@ impl RefClient { pub(crate) fn all_resources(&self) -> Vec { self.resources.clone() } + + pub(crate) fn set_system_dns_resolvers(&mut self, servers: &Vec) { + self.system_dns_resolvers.clone_from(servers); + } + + pub(crate) fn set_upstream_dns_resolvers(&mut self, servers: &Vec) { + self.upstream_dns_resolvers.clone_from(servers); + } + + pub(crate) fn upstream_dns_resolvers(&self) -> Vec { + self.upstream_dns_resolvers.clone() + } } // This function only works on the tests because we are limited to resources with a single wildcard at the beginning of the resource. @@ -757,11 +769,13 @@ fn is_subdomain(name: &str, record: &str) -> bool { pub(crate) fn ref_client_host( tunnel_ip4s: impl Strategy, tunnel_ip6s: impl Strategy, + system_dns: impl Strategy>, + upstream_dns: impl Strategy>, ) -> impl Strategy> { host( any_ip_stack(), any_port(), - ref_client(tunnel_ip4s, tunnel_ip6s), + ref_client(tunnel_ip4s, tunnel_ip6s, system_dns, upstream_dns), latency(300), // TODO: Increase with #6062. ) } @@ -769,41 +783,58 @@ pub(crate) fn ref_client_host( fn ref_client( tunnel_ip4s: impl Strategy, tunnel_ip6s: impl Strategy, + system_dns: impl Strategy>, + upstream_dns: impl Strategy>, ) -> impl Strategy { ( tunnel_ip4s, tunnel_ip6s, + system_dns, + upstream_dns, client_id(), private_key(), known_hosts(), ) .prop_map( - move |(tunnel_ip4, tunnel_ip6, id, key, known_hosts)| RefClient { + move |( + tunnel_ip4, + tunnel_ip6, + system_dns_resolvers, + upstream_dns_resolvers, id, key, known_hosts, - tunnel_ip4, - tunnel_ip6, - system_dns_resolvers: Default::default(), - upstream_dns_resolvers: Default::default(), - internet_resource: Default::default(), - cidr_resources: IpNetworkTable::new(), - dns_records: Default::default(), - connected_cidr_resources: Default::default(), - connected_dns_resources: Default::default(), - connected_internet_resource: Default::default(), - expected_icmp_handshakes: Default::default(), - expected_dns_handshakes: Default::default(), - disabled_resources: Default::default(), - resources: Default::default(), + )| { + RefClient { + id, + key, + known_hosts, + tunnel_ip4, + tunnel_ip6, + system_dns_resolvers, + upstream_dns_resolvers, + internet_resource: Default::default(), + cidr_resources: IpNetworkTable::new(), + dns_records: Default::default(), + connected_cidr_resources: Default::default(), + connected_dns_resources: Default::default(), + connected_internet_resource: Default::default(), + expected_icmp_handshakes: Default::default(), + expected_dns_handshakes: Default::default(), + disabled_resources: Default::default(), + resources: Default::default(), + } }, ) } fn known_hosts() -> impl Strategy>> { collection::btree_map( - domain_name(2..4).prop_map(|d| d.parse().unwrap()), - collection::vec(any::(), 1..6), - 0..15, + prop_oneof![ + Just("api.firezone.dev".to_owned()), + Just("api.firez.one".to_owned()) + ], + collection::vec(any::(), 1..3), + 1..=2, ) } diff --git a/rust/connlib/tunnel/src/tests/sim_dns.rs b/rust/connlib/tunnel/src/tests/sim_dns.rs index b8b78f505..233c98cfb 100644 --- a/rust/connlib/tunnel/src/tests/sim_dns.rs +++ b/rust/connlib/tunnel/src/tests/sim_dns.rs @@ -61,6 +61,7 @@ impl SimDns { let query = query.sole_question().unwrap(); let name = query.qname().to_vec(); + let qtype = query.qtype(); let records = global_dns_records .get(&name) @@ -68,7 +69,7 @@ impl SimDns { .flatten() .filter(|ip| { #[allow(clippy::wildcard_enum_match_arm)] - match query.qtype() { + match qtype { Rtype::A => ip.is_ipv4(), Rtype::AAAA => ip.is_ipv6(), _ => todo!(), @@ -87,7 +88,7 @@ impl SimDns { let payload = answers.finish(); - tracing::debug!(%name, "Responding to DNS query"); + tracing::debug!(%name, %qtype, "Responding to DNS query"); Some(Transmit { src: Some(transmit.dst), diff --git a/rust/connlib/tunnel/src/tests/strategies.rs b/rust/connlib/tunnel/src/tests/strategies.rs index 8984fe5a5..37c5a3e90 100644 --- a/rust/connlib/tunnel/src/tests/strategies.rs +++ b/rust/connlib/tunnel/src/tests/strategies.rs @@ -11,7 +11,7 @@ use connlib_shared::{ client::{ ResourceDescriptionCidr, ResourceDescriptionDns, ResourceDescriptionInternet, Site, }, - RelayId, + DnsServer, RelayId, }, DomainName, }; @@ -260,3 +260,21 @@ pub(crate) fn documentation_ip6s(subnet: u16, num_ips: usize) -> impl Strategy>, +) -> impl Strategy> { + let max = dns_servers.len(); + + sample::subsequence(dns_servers, ..=max) + .prop_map(|seq| seq.into_iter().map(|h| h.single_socket().ip()).collect()) +} + +pub(crate) fn upstream_dns_servers( + dns_servers: Vec>, +) -> impl Strategy> { + let max = dns_servers.len(); + + sample::subsequence(dns_servers, ..=max) + .prop_map(|seq| seq.into_iter().map(|h| h.single_socket().into()).collect()) +} diff --git a/rust/connlib/tunnel/src/tests/stub_portal.rs b/rust/connlib/tunnel/src/tests/stub_portal.rs index 05f1dc6ca..03a8b6780 100644 --- a/rust/connlib/tunnel/src/tests/stub_portal.rs +++ b/rust/connlib/tunnel/src/tests/stub_portal.rs @@ -6,7 +6,7 @@ use super::{ }; use crate::proptest::*; use connlib_shared::{ - messages::{client, gateway, GatewayId, ResourceId}, + messages::{client, gateway, DnsServer, GatewayId, ResourceId}, DomainName, }; use ip_network::{Ipv4Network, Ipv6Network}; @@ -197,11 +197,20 @@ impl StubPortal { .prop_map(BTreeMap::from_iter) } - pub(crate) fn client(&self) -> impl Strategy> { + pub(crate) fn client( + &self, + system_dns: impl Strategy>, + upstream_dns: impl Strategy>, + ) -> impl Strategy> { let client_tunnel_ip4 = tunnel_ip4s().next().unwrap(); let client_tunnel_ip6 = tunnel_ip6s().next().unwrap(); - ref_client_host(Just(client_tunnel_ip4), Just(client_tunnel_ip6)) + ref_client_host( + Just(client_tunnel_ip4), + Just(client_tunnel_ip6), + system_dns, + upstream_dns, + ) } pub(crate) fn dns_resource_records( diff --git a/rust/connlib/tunnel/src/tests/sut.rs b/rust/connlib/tunnel/src/tests/sut.rs index db3d91cc6..7db057272 100644 --- a/rust/connlib/tunnel/src/tests/sut.rs +++ b/rust/connlib/tunnel/src/tests/sut.rs @@ -257,7 +257,7 @@ impl TunnelTest { Transition::ReconnectPortal => { let ipv4 = state.client.inner().sut.tunnel_ip4().unwrap(); let ipv6 = state.client.inner().sut.tunnel_ip6().unwrap(); - let upstream_dns = ref_state.client.inner().upstream_dns_resolvers.clone(); + let upstream_dns = ref_state.client.inner().upstream_dns_resolvers(); let all_resources = ref_state.client.inner().all_resources(); // Simulate receiving `init`. @@ -351,11 +351,7 @@ impl TunnelTest { ); assert_dns_packets_properties(ref_client, sim_client); assert_known_hosts_are_valid(ref_client, sim_client); - assert_eq!( - sim_client.effective_dns_servers(), - ref_client.expected_dns_servers(), - "Effective DNS servers should match either system or upstream DNS" - ); + assert_dns_servers_are_valid(ref_client, sim_client); } } @@ -676,11 +672,13 @@ impl TunnelTest { ClientEvent::ResourcesChanged { .. } => { tracing::warn!("Unimplemented"); } - ClientEvent::TunInterfaceUpdated { - dns_by_sentinel, .. - } => { + ClientEvent::TunInterfaceUpdated(config) => { + if self.client.inner().dns_by_sentinel == config.dns_by_sentinel { + tracing::error!("Emitted `TunInterfaceUpdated` without changing DNS servers"); + } + self.client - .exec_mut(|c| c.dns_by_sentinel = dns_by_sentinel); + .exec_mut(|c| c.dns_by_sentinel = config.dns_by_sentinel); } ClientEvent::TunRoutesUpdated { .. } => {} ClientEvent::RequestConnection { diff --git a/rust/connlib/tunnel/src/tests/transition.rs b/rust/connlib/tunnel/src/tests/transition.rs index a6a838459..5b5cb1b26 100644 --- a/rust/connlib/tunnel/src/tests/transition.rs +++ b/rust/connlib/tunnel/src/tests/transition.rs @@ -1,7 +1,4 @@ -use super::{ - sim_dns::RefDns, - sim_net::{any_ip_stack, any_port, Host}, -}; +use super::sim_net::{any_ip_stack, any_port, Host}; use connlib_shared::{ messages::{client::ResourceDescription, DnsServer, RelayId, ResourceId}, DomainName, @@ -220,27 +217,3 @@ pub(crate) fn roam_client() -> impl Strategy { port, }) } - -pub(crate) fn update_system_dns_servers( - dns_servers: Vec>, -) -> impl Strategy { - let max = dns_servers.len(); - - sample::subsequence(dns_servers, ..=max).prop_map(|seq| { - Transition::UpdateSystemDnsServers( - seq.into_iter().map(|h| h.single_socket().ip()).collect(), - ) - }) -} - -pub(crate) fn update_upstream_dns_servers( - dns_servers: Vec>, -) -> impl Strategy { - let max = dns_servers.len(); - - sample::subsequence(dns_servers, ..=max).prop_map(|seq| { - Transition::UpdateUpstreamDnsServers( - seq.into_iter().map(|h| h.single_socket().into()).collect(), - ) - }) -}