From 36dfee2c4273622b77fb5b02acfc33ce5125ffb4 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 7 Oct 2025 00:26:07 +0000 Subject: [PATCH] refactor(connlib): explicitly enable/disable Internet Resource (#10507) Instead of the generic "disable any kind of resource"-functionality that connlib currently exposes, we now provide an API to only enable / disable the Internet Resource. This is a lot simpler to deal with and reason about than the previous system, especially when it comes to the proptests. Those need to model connlib's behaviour correctly across its entire API surface which makes them unnecessarily complex if we only ever use the `set_disabled_resources` API with a single resource. In preparation for #4789, I want to extend the proptests to cover traffic filters (#7126). This will make them a fair bit more complicated, so any prior removal of complexity is appreciated. Simplifying the implementation here is also a good starting point to fix #10255. Not implicitly enabling the Internet Resource when it gets added should be quite simple after this change. Finally, resolving #8885 should also be quite easy. We just need to store the state of the Internet Resource once per API URL instead of globally. Resolves: #8404 --------- Signed-off-by: Thomas Eizinger Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../firezone/android/tunnel/TunnelService.kt | 21 +-- rust/apple-client-ffi/src/lib.rs | 18 +-- rust/client-ffi/src/lib.rs | 9 +- rust/client-shared/src/eventloop.rs | 8 +- rust/client-shared/src/lib.rs | 9 +- .../tunnel/proptest-regressions/tests.txt | 4 + rust/connlib/tunnel/src/client.rs | 131 +++++++++--------- rust/connlib/tunnel/src/tests/reference.rs | 42 ++---- rust/connlib/tunnel/src/tests/sim_client.rs | 90 +++++++----- rust/connlib/tunnel/src/tests/sut.rs | 13 +- rust/connlib/tunnel/src/tests/transition.rs | 16 ++- rust/gui-client/src-tauri/src/controller.rs | 28 +--- rust/gui-client/src-tauri/src/service.rs | 13 +- .../FirezoneNetworkExtension/Adapter.swift | 34 +---- 14 files changed, 184 insertions(+), 252 deletions(-) diff --git a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt index fecd428d3..0e9052ffa 100644 --- a/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt +++ b/kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt @@ -235,14 +235,9 @@ class TunnelService : VpnService() { // UI updates for resources fun resourcesUpdated() { - val currentlyDisabled = - if (internetResource() != null && !resourceState.isEnabled()) { - setOf(internetResource()!!.id) - } else { - emptySet() - } - - sendTunnelCommand(TunnelCommand.SetDisabledResources(Gson().toJson(currentlyDisabled))) + sendTunnelCommand( + TunnelCommand.SetInternetResourceState(resourceState.isEnabled()), + ) } fun internetResourceToggled(state: ResourceState) { @@ -446,8 +441,8 @@ class TunnelService : VpnService() { sealed class TunnelCommand { data object Disconnect : TunnelCommand() - data class SetDisabledResources( - val disabledResources: String, + data class SetInternetResourceState( + val active: Boolean, ) : TunnelCommand() data class SetDns( @@ -487,11 +482,9 @@ class TunnelService : VpnService() { session.disconnect() // Sending disconnect will close the event-stream which will exit this loop } - - is TunnelCommand.SetDisabledResources -> { - session.setDisabledResources(command.disabledResources) + is TunnelCommand.SetInternetResourceState -> { + session.setInternetResourceState(command.active) } - is TunnelCommand.SetDns -> { session.setDns(command.dnsServers) } diff --git a/rust/apple-client-ffi/src/lib.rs b/rust/apple-client-ffi/src/lib.rs index 61cf91e21..8932543e2 100644 --- a/rust/apple-client-ffi/src/lib.rs +++ b/rust/apple-client-ffi/src/lib.rs @@ -78,11 +78,8 @@ mod ffi { #[swift_bridge(swift_name = "setDns", return_with = err_to_string)] fn set_dns(self: &mut WrappedSession, dns_servers: String) -> Result<(), String>; - #[swift_bridge(swift_name = "setDisabledResources", return_with = err_to_string)] - fn set_disabled_resources( - self: &mut WrappedSession, - disabled_resources: String, - ) -> Result<(), String>; + #[swift_bridge(swift_name = "setInternetResourceState")] + fn set_internet_resource_state(self: &mut WrappedSession, active: bool); #[swift_bridge(swift_name = "setLogDirectives", return_with = err_to_string)] fn set_log_directives(self: &mut WrappedSession, directives: String) -> Result<(), String>; @@ -361,15 +358,8 @@ impl WrappedSession { Ok(()) } - fn set_disabled_resources(&mut self, disabled_resources: String) -> Result<()> { - tracing::debug!(%disabled_resources); - - let disabled_resources = serde_json::from_str(&disabled_resources) - .context("Failed to deserialize disabled resources from JSON")?; - - self.inner.set_disabled_resources(disabled_resources); - - Ok(()) + fn set_internet_resource_state(&mut self, active: bool) { + self.inner.set_internet_resource_state(active); } fn set_log_directives(&mut self, directives: String) -> Result<()> { diff --git a/rust/client-ffi/src/lib.rs b/rust/client-ffi/src/lib.rs index 359305ca0..a7fc8f7e1 100644 --- a/rust/client-ffi/src/lib.rs +++ b/rust/client-ffi/src/lib.rs @@ -125,13 +125,8 @@ impl Session { Ok(()) } - pub fn set_disabled_resources(&self, disabled_resources: String) -> Result<(), Error> { - let disabled_resources = serde_json::from_str(&disabled_resources) - .context("Failed to deserialize disabled resource IDs")?; - - self.inner.set_disabled_resources(disabled_resources); - - Ok(()) + pub fn set_internet_resource_state(&self, active: bool) { + self.inner.set_internet_resource_state(active); } pub fn set_dns(&self, dns_servers: String) -> Result<(), Error> { diff --git a/rust/client-shared/src/eventloop.rs b/rust/client-shared/src/eventloop.rs index ec2ca1345..d803ea34f 100644 --- a/rust/client-shared/src/eventloop.rs +++ b/rust/client-shared/src/eventloop.rs @@ -1,6 +1,6 @@ use crate::PHOENIX_TOPIC; use anyhow::{Context as _, Result}; -use connlib_model::{PublicKey, ResourceId, ResourceView}; +use connlib_model::{PublicKey, ResourceView}; use firezone_tunnel::messages::RelaysPresence; use firezone_tunnel::messages::client::{ EgressMessages, FailReason, FlowCreated, FlowCreationFailed, GatewayIceCandidates, @@ -65,7 +65,7 @@ pub enum Command { Stop, SetDns(Vec), SetTun(Box), - SetDisabledResources(BTreeSet), + SetInternetResourceState(bool), } enum PortalCommand { @@ -193,14 +193,14 @@ impl Eventloop { tunnel.state_mut().update_system_resolvers(dns); } - Command::SetDisabledResources(resources) => { + Command::SetInternetResourceState(active) => { let Some(tunnel) = self.tunnel.as_mut() else { return Ok(ControlFlow::Continue(())); }; tunnel .state_mut() - .set_disabled_resources(resources, Instant::now()) + .set_internet_resource_state(active, Instant::now()) } Command::SetTun(tun) => { let Some(tunnel) = self.tunnel.as_mut() else { diff --git a/rust/client-shared/src/lib.rs b/rust/client-shared/src/lib.rs index a550c7c56..3bae79663 100644 --- a/rust/client-shared/src/lib.rs +++ b/rust/client-shared/src/lib.rs @@ -6,13 +6,12 @@ pub use firezone_tunnel::TunConfig; pub use firezone_tunnel::messages::client::{IngressMessages, ResourceDescription}; use anyhow::Result; -use connlib_model::{ResourceId, ResourceView}; +use connlib_model::ResourceView; use eventloop::{Command, Eventloop}; use futures::future::Fuse; use futures::{FutureExt, StreamExt}; use phoenix_channel::{PhoenixChannel, PublicKeyParam}; use socket_factory::{SocketFactory, TcpSocket, UdpSocket}; -use std::collections::BTreeSet; use std::future; use std::net::IpAddr; use std::sync::Arc; @@ -123,10 +122,8 @@ impl Session { let _ = self.channel.send(Command::SetDns(new_dns)); } - pub fn set_disabled_resources(&self, disabled_resources: BTreeSet) { - let _ = self - .channel - .send(Command::SetDisabledResources(disabled_resources)); + pub fn set_internet_resource_state(&self, active: bool) { + let _ = self.channel.send(Command::SetInternetResourceState(active)); } /// Sets a new [`Tun`] device handle. diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt index 8a71706eb..2a9ec0289 100644 --- a/rust/connlib/tunnel/proptest-regressions/tests.txt +++ b/rust/connlib/tunnel/proptest-regressions/tests.txt @@ -204,3 +204,7 @@ cc 322edb50d106668e778b987239de10ce457bbb34600dd4a6b7a4707e9182ab63 cc 2956260fdc1827251129cfdf31db35a03eb48dbb6d1a6d1fb1555181a11f1bbd cc 354fde8dc9df1ec01c1e15195b12a8f78c0030470ab3d554e6b8489cef9c740c cc 9d9360570abfa59ab1dec6ebe16c8c23e7cb497c3d0b4af2a5f24cb0a1422ad1 +cc fdfbc47a6e26893cf317a3e2dd0a95a7d07a3fc1d7c19accbf20b5582e9453e4 +cc 5ca1f7f615da306a4bbcd7f1300459288600ed1740c650fa75fdefade2d789d7 +cc 71380222e92940bac1d7a6ab8b13eb7180da63481c08c7c0b168c0007e02906d +cc d2a87f2284b2ae5a2206bd7091aa31015599bd8301e19644a860e46c0113dafb diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index f63acd479..02e7b34dc 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -4,9 +4,9 @@ mod resource; use dns_resource_nat::DnsResourceNat; use dns_types::ResponseCode; use firezone_telemetry::{analytics, feature_flags}; -pub(crate) use resource::{CidrResource, Resource}; #[cfg(all(feature = "proptest", test))] -pub(crate) use resource::{DnsResource, InternetResource}; +pub(crate) use resource::DnsResource; +pub(crate) use resource::{CidrResource, InternetResource, Resource}; use ringbuffer::{AllocRingBuffer, RingBuffer}; use crate::dns::{DnsResourceRecord, StubResolver}; @@ -38,7 +38,7 @@ use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; use std::num::NonZeroUsize; use std::ops::ControlFlow; use std::time::{Duration, Instant}; -use std::{io, iter}; +use std::{io, iter, mem}; pub(crate) const IPV4_RESOURCES: Ipv4Network = match Ipv4Network::new(Ipv4Addr::new(100, 96, 0, 0), 11) { @@ -107,8 +107,7 @@ pub struct ClientState { /// All CIDR resources we know about, indexed by the IP range they cover (like `1.1.0.0/8`). active_cidr_resources: IpNetworkTable, - /// `Some` if the Internet resource is enabled. - internet_resource: Option, + is_internet_resource_active: bool, /// All resources indexed by their ID. resources_by_id: BTreeMap, @@ -129,9 +128,6 @@ pub struct ClientState { /// Configuration of the TUN device, when it is up. tun_config: Option, - /// Resources that have been disabled by the UI - disabled_resources: BTreeSet, - tcp_dns_client: dns_over_tcp::Client, tcp_dns_server: dns_over_tcp::Server, /// Tracks the TCP stream (i.e. socket-pair) on which we received a TCP DNS query by the ID of the recursive DNS query we issued. @@ -206,9 +202,8 @@ impl ClientState { gateways_site: Default::default(), udp_dns_sockets_by_upstream_and_query_id: Default::default(), stub_resolver: StubResolver::new(records), - disabled_resources: Default::default(), buffered_transmits: Default::default(), - internet_resource: None, + is_internet_resource_active: false, recently_connected_gateways: LruCache::new(MAX_REMEMBERED_GATEWAYS), upstream_dns: Default::default(), buffered_dns_queries: Default::default(), @@ -790,7 +785,7 @@ impl ClientState { if !self.is_upstream_set_by_the_portal() { return false; } - if self.internet_resource.is_some() { + if self.active_internet_resource().is_some() { return true; } @@ -908,30 +903,33 @@ impl ClientState { .set_listen_addresses::(sentinel_sockets); } - pub fn set_disabled_resources( - &mut self, - new_disabled_resources: BTreeSet, - now: Instant, - ) { - let current_disabled_resources = self.disabled_resources.clone(); - - // We set disabled_resources before anything else so that add_resource knows what resources are enabled right now. - self.disabled_resources.clone_from(&new_disabled_resources); - - for re_enabled_resource in current_disabled_resources.difference(&new_disabled_resources) { - let Some(resource) = self.resources_by_id.get(re_enabled_resource) else { - continue; - }; - - self.add_resource(resource.clone(), now); + /// Sets the Internet Resource state. + /// + /// In order for the Internet Resource to actually be active, the user must also have access to it. + /// In other words, it needs to be present in the resources list provided by the portal. + /// + /// That list may be provided asynchronously to this call, which is why set it as active, + /// regardless as to whether it is present or not. + pub fn set_internet_resource_state(&mut self, active: bool, now: Instant) { + // Be idempotent. + if self.is_internet_resource_active == active { + return; } - for new_disabled_resource in new_disabled_resources.difference(¤t_disabled_resources) - { - self.disable_resource(*new_disabled_resource, now); + let previous = std::mem::replace(&mut self.is_internet_resource_active, active); + + let resource = self.internet_resource(); + + // If we are enabling a known Internet Resource, log it. + if active && let Some(resource) = resource.cloned() { + self.log_activating_resource(&Resource::Internet(resource)); + } + + // Check if we need to disable the current one. + if previous && let Some(current) = resource { + self.disable_resource(current.id, now); } - self.active_cidr_resources = self.recalculate_active_cidr_resources(); self.maybe_update_tun_routes(); } @@ -959,26 +957,21 @@ impl ClientState { .chain(iter::once(DNS_SENTINELS_V4.into())) .chain(iter::once(DNS_SENTINELS_V6.into())) .chain( - self.internet_resource + self.active_internet_resource() .map(|_| Ipv4Network::DEFAULT_ROUTE.into()), ) .chain( - self.internet_resource + self.active_internet_resource() .map(|_| Ipv6Network::DEFAULT_ROUTE.into()), ) } - fn is_resource_enabled(&self, resource: &ResourceId) -> bool { - !self.disabled_resources.contains(resource) && self.resources_by_id.contains_key(resource) - } - fn get_resource_by_destination(&self, destination: IpAddr) -> Option { // We need to filter disabled resources because we never remove resources from the stub_resolver let maybe_dns_resource_id = self .stub_resolver .resolve_resource_by_ip(&destination) .map(|(_, r)| *r) - .filter(|resource| self.is_resource_enabled(resource)) .inspect( |rid| tracing::trace!(target: "tunnel_test_coverage", %destination, %rid, "Packet for DNS resource"), ); @@ -992,14 +985,31 @@ impl ClientState { |rid| tracing::trace!(target: "tunnel_test_coverage", %destination, %rid, "Packet for CIDR resource"), ); + let maybe_internet_resource = self.active_internet_resource() + .map(|r| r.id) + .inspect(|rid| { + tracing::trace!(target: "tunnel_test_coverage", %destination, %rid, "Packet for Internet resource") + }); + maybe_dns_resource_id .or(maybe_cidr_resource_id) - .or(self.internet_resource) - .inspect(|r| { - if Some(*r) == self.internet_resource { - tracing::trace!(target: "tunnel_test_coverage", %destination, "Packet for Internet resource") - } - }) + .or(maybe_internet_resource) + } + + fn active_internet_resource(&self) -> Option<&InternetResource> { + if !self.is_internet_resource_active { + return None; + } + + self.internet_resource() + } + + fn internet_resource(&self) -> Option<&InternetResource> { + self.resources_by_id.values().find_map(|r| match r { + Resource::Dns(_) => None, + Resource::Cidr(_) => None, + Resource::Internet(internet_resource) => Some(internet_resource), + }) } pub fn update_system_resolvers(&mut self, new_dns: Vec) { @@ -1454,10 +1464,6 @@ impl ClientState { continue; }; - if !self.is_resource_enabled(&resource.id) { - continue; - } - if let Some(active_resource) = active_cidr_resources.exact_match(resource.address) && self.is_cidr_resource_connected(&active_resource.id) { @@ -1667,11 +1673,6 @@ impl ClientState { self.resources_by_id .insert(new_resource.id(), new_resource.clone()); - if !self.is_resource_enabled(&(new_resource.id())) { - self.emit_resources_changed(); // We still have a new resource but it is disabled, let the client know. - return; - } - let activated = match &new_resource { Resource::Dns(dns) => { self.stub_resolver @@ -1695,17 +1696,11 @@ impl ClientState { None => true, } } - Resource::Internet(resource) => { - self.internet_resource.replace(resource.id) != Some(resource.id) - } + Resource::Internet(_) => !mem::replace(&mut self.is_internet_resource_active, true), }; if activated { - let name = new_resource.name(); - let address = new_resource.address_string().map(tracing::field::display); - let sites = new_resource.sites_string(); - - tracing::info!(%name, address, %sites, "Activating resource"); + self.log_activating_resource(&new_resource); } if matches!(new_resource, Resource::Cidr(_)) { @@ -1716,6 +1711,14 @@ impl ClientState { self.emit_resources_changed(); } + fn log_activating_resource(&self, resource: &Resource) { + let name = resource.name(); + let address = resource.address_string().map(tracing::field::display); + let sites = resource.sites_string(); + + tracing::info!(%name, address, %sites, "Activating resource"); + } + #[tracing::instrument(level = "debug", skip_all, fields(?id))] pub fn remove_resource(&mut self, id: ResourceId, now: Instant) { self.disable_resource(id, now); @@ -1753,11 +1756,7 @@ impl ClientState { match resource { Resource::Dns(_) => self.stub_resolver.remove_resource(id), Resource::Cidr(_) => {} - Resource::Internet(_) => { - if self.internet_resource.is_some_and(|r_id| r_id == id) { - self.internet_resource = None; - } - } + Resource::Internet(_) => self.is_internet_resource_active = false, } let name = resource.name(); diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs index ea1aef0b2..44d11000e 100644 --- a/rust/connlib/tunnel/src/tests/reference.rs +++ b/rust/connlib/tunnel/src/tests/reference.rs @@ -228,10 +228,10 @@ impl ReferenceState { .with_if_not_empty( 5, state.all_resources_not_known_to_client(), - |resource_ids| sample::select(resource_ids).prop_map(Transition::ActivateResource), + |resource_ids| sample::select(resource_ids).prop_map(Transition::AddResource), ) .with_if_not_empty(1, state.client.inner().all_resource_ids(), |resource_ids| { - sample::select(resource_ids).prop_map(Transition::DeactivateResource) + sample::select(resource_ids).prop_map(Transition::RemoveResource) }) .with(1, roam_client()) .with(1, relays(relay_id()).prop_map(Transition::DeployNewRelays)) @@ -246,11 +246,10 @@ impl ReferenceState { .with(1, Just(Transition::ReconnectPortal)) .with(1, Just(Transition::Idle)) .with(1, private_key().prop_map(Transition::RestartClient)) - .with_if_not_empty(1, state.client.inner().all_resource_ids(), |resources_id| { - sample::subsequence(resources_id.clone(), resources_id.len()).prop_map( - |resources_id| Transition::DisableResources(BTreeSet::from_iter(resources_id)), - ) - }) + .with( + 1, + any::().prop_map(Transition::SetInternetResourceState), + ) .with_if_not_empty(1, state.client.inner().all_resource_ids(), |resources_id| { sample::select(resources_id) .prop_map(Transition::DeauthorizeWhileGatewayIsPartitioned) @@ -395,7 +394,7 @@ impl ReferenceState { /// Here is where we implement the "expected" logic. pub(crate) fn apply(mut state: Self, transition: &Transition) -> Self { match transition { - Transition::ActivateResource(resource) => { + Transition::AddResource(resource) => { state.client.exec_mut(|client| match resource { client::Resource::Dns(r) => { client.add_dns_resource(r.clone()); @@ -414,17 +413,13 @@ impl ReferenceState { client::Resource::Internet(r) => client.add_internet_resource(r.clone()), }); } - Transition::DeactivateResource(id) => { + Transition::RemoveResource(id) => { state.client.exec_mut(|client| { client.remove_resource(id); }); } - Transition::DisableResources(resources) => state.client.exec_mut(|client| { - client.disabled_resources.clone_from(resources); - - for id in resources { - client.disconnect_resource(id) - } + Transition::SetInternetResourceState(active) => state.client.exec_mut(|client| { + client.set_internet_resource_state(*active); }), Transition::SendDnsQueries(queries) => { for query in queries { @@ -535,7 +530,7 @@ impl ReferenceState { /// Any additional checks on whether a particular [`Transition`] can be applied to a certain state. pub(crate) fn is_valid_transition(state: &Self, transition: &Transition) -> bool { match transition { - Transition::ActivateResource(resource) => { + Transition::AddResource(resource) => { // Don't add resource we already have. if state.client.inner().has_resource(resource.id()) { return false; @@ -543,18 +538,7 @@ impl ReferenceState { true } - Transition::DisableResources(resources) => resources.iter().all(|r| { - let has_resource = state.client.inner().has_resource(*r); - let has_tcp_connection = state - .client - .inner() - .tcp_connection_tuple_to_resource(*r) - .is_some(); - - // Don't disabled resources we don't have. It doesn't hurt but makes the logs of reduced testcases weird. - // Also don't disable resources where we have TCP connections as those would get interrupted. - has_resource && !has_tcp_connection - }), + Transition::SetInternetResourceState(_) => true, Transition::SendIcmpPacket { src, dst: Destination::DomainName { name, .. }, @@ -687,7 +671,7 @@ impl ReferenceState { !is_assigned_ip4 && !is_assigned_ip6 } Transition::ReconnectPortal => true, - Transition::DeactivateResource(r) => { + Transition::RemoveResource(r) => { let has_resource = state.client.inner().has_resource(*r); let has_tcp_connection = state .client diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs index f0acb476d..3860cb9e4 100644 --- a/rust/connlib/tunnel/src/tests/sim_client.rs +++ b/rust/connlib/tunnel/src/tests/sim_client.rs @@ -436,7 +436,7 @@ pub struct RefClient { resources: Vec, #[debug(skip)] - internet_resource: Option, + internet_resource_active: bool, /// The CIDR resources the client is aware of. #[debug(skip)] @@ -461,10 +461,6 @@ pub struct RefClient { #[debug(skip)] pub(crate) connected_dns_resources: BTreeSet, - /// Actively disabled resources by the UI - #[debug(skip)] - pub(crate) disabled_resources: BTreeSet, - /// The [`ResourceStatus`] of each site. #[debug(skip)] site_status: BTreeMap, @@ -515,7 +511,7 @@ impl RefClient { self.connected_cidr_resources.remove(resource); self.connected_dns_resources.remove(resource); - if self.internet_resource.is_some_and(|r| &r == resource) { + if self.internet_resource().is_some_and(|r| r == *resource) { self.connected_internet_resource = false; } @@ -540,11 +536,33 @@ impl RefClient { } } + pub(crate) fn set_internet_resource_state(&mut self, active: bool) { + let resource = self + .resources + .iter() + .find(|r| matches!(r, Resource::Internet(_))); + + self.internet_resource_active = active; + + let Some(resource) = resource else { + return; + }; + + if active { + self.ipv4_routes + .insert(resource.id(), Ipv4Network::DEFAULT_ROUTE); + self.ipv6_routes + .insert(resource.id(), Ipv6Network::DEFAULT_ROUTE); + } else { + self.disconnect_resource(&resource.id()); + } + } + pub(crate) fn remove_resource(&mut self, resource: &ResourceId) { self.disconnect_resource(resource); - if self.internet_resource.is_some_and(|r| &r == resource) { - self.internet_resource = None; + if self.internet_resource().is_some_and(|r| r == *resource) { + self.internet_resource_active = false; } self.resources.retain(|r| r.id() != *resource); @@ -557,8 +575,9 @@ impl RefClient { .chain(self.connected_cidr_resources.clone()) .chain(self.connected_dns_resources.clone()) .chain( - self.internet_resource - .filter(|_| self.connected_internet_resource), + self.connected_internet_resource + .then(|| self.internet_resource()) + .flatten(), ) } @@ -569,10 +588,6 @@ impl RefClient { continue; }; - if self.disabled_resources.contains(&resource.id) { - continue; - } - if let Some(overlapping_resource) = table.exact_match(resource.address) && self.is_connected_to_internet_or_cidr(*overlapping_resource) { @@ -610,26 +625,20 @@ impl RefClient { } } - pub(crate) fn add_internet_resource(&mut self, r: InternetResource) { - self.internet_resource = Some(r.id); - self.resources.push(Resource::Internet(r.clone())); + pub(crate) fn add_internet_resource(&mut self, resource: InternetResource) { + self.resources.push(Resource::Internet(resource.clone())); + self.internet_resource_active = true; - if self.disabled_resources.contains(&r.id) { - return; - } - - self.ipv4_routes.insert(r.id, Ipv4Network::DEFAULT_ROUTE); - self.ipv6_routes.insert(r.id, Ipv6Network::DEFAULT_ROUTE); + self.ipv4_routes + .insert(resource.id, Ipv4Network::DEFAULT_ROUTE); + self.ipv6_routes + .insert(resource.id, Ipv6Network::DEFAULT_ROUTE); } pub(crate) fn add_cidr_resource(&mut self, r: CidrResource) { self.resources.push(Resource::Cidr(r.clone())); self.cidr_resources = self.recalculate_cidr_routes(); - if self.disabled_resources.contains(&r.id) { - return; - } - match r.address { IpNetwork::V4(v4) => { self.ipv4_routes.insert(r.id, v4); @@ -647,7 +656,7 @@ impl RefClient { /// Re-adds all resources in the order they have been initially added. pub(crate) fn readd_all_resources(&mut self) { self.cidr_resources = IpNetworkTable::new(); - self.internet_resource = None; + self.internet_resource_active = false; for resource in mem::take(&mut self.resources) { match resource { @@ -833,7 +842,10 @@ impl RefClient { } fn connect_to_internet_or_cidr_resource(&mut self, rid: ResourceId) { - if self.internet_resource.is_some_and(|r| r == rid) { + if self.internet_resource_active + && let Some(internet) = self.internet_resource() + && internet == rid + { self.connected_internet_resource = true; return; } @@ -910,8 +922,9 @@ impl RefClient { } pub(crate) fn active_internet_resource(&self) -> Option { - self.internet_resource - .filter(|r| !self.disabled_resources.contains(r)) + self.internet_resource_active + .then(|| self.internet_resource()) + .flatten() } fn resource_by_dst(&self, destination: &Destination) -> Option { @@ -938,8 +951,7 @@ impl RefClient { .filter_map(|r| r.into_dns()) .filter(|r| is_subdomain(&domain.to_string(), &r.address)) .sorted_by_key(|r| r.address.len()) - .rev() - .find(|r| !self.disabled_resources.contains(&r.id)) + .next_back() } fn resolved_domains(&self) -> impl Iterator)> + '_ { @@ -1047,7 +1059,6 @@ impl RefClient { let (_, r) = self .cidr_resources .matches(ip) - .filter(|(_, r)| !self.disabled_resources.contains(r)) .sorted_by(|(n1, _), (n2, _)| n1.netmask().cmp(&n2.netmask()).reverse()) // Highest netmask is most specific. .next()?; @@ -1137,6 +1148,14 @@ impl RefClient { self.resources.clone() } + fn internet_resource(&self) -> Option { + self.resources.iter().find_map(|r| match r { + Resource::Dns(_) => None, + Resource::Cidr(_) => None, + Resource::Internet(internet_resource) => Some(internet_resource.id), + }) + } + pub(crate) fn system_dns_resolvers(&self) -> Vec { self.system_dns_resolvers.clone() } @@ -1258,7 +1277,7 @@ fn ref_client( system_dns_resolvers, upstream_dns_resolvers, search_domain, - internet_resource: Default::default(), + internet_resource_active: Default::default(), cidr_resources: IpNetworkTable::new(), dns_records: Default::default(), connected_cidr_resources: Default::default(), @@ -1269,7 +1288,6 @@ fn ref_client( expected_tcp_connections: Default::default(), expected_udp_dns_handshakes: Default::default(), expected_tcp_dns_handshakes: Default::default(), - disabled_resources: Default::default(), resources: Default::default(), ipv4_routes: Default::default(), ipv6_routes: Default::default(), diff --git a/rust/connlib/tunnel/src/tests/sut.rs b/rust/connlib/tunnel/src/tests/sut.rs index 58ebc3e6a..81ed3f056 100644 --- a/rust/connlib/tunnel/src/tests/sut.rs +++ b/rust/connlib/tunnel/src/tests/sut.rs @@ -126,7 +126,7 @@ impl TunnelTest { // Act: Apply the transition match transition { - Transition::ActivateResource(resource) => { + Transition::AddResource(resource) => { state.client.exec_mut(|c| { // Flush DNS. match &resource { @@ -146,7 +146,7 @@ impl TunnelTest { c.sut.add_resource(resource, now); }); } - Transition::DeactivateResource(rid) => { + Transition::RemoveResource(rid) => { state.client.exec_mut(|c| c.sut.remove_resource(rid, now)); if let Some(gateway) = ref_state @@ -157,9 +157,9 @@ impl TunnelTest { gateway.exec_mut(|g| g.sut.remove_access(&state.client.inner().id, &rid, now)); } } - Transition::DisableResources(resources) => state + Transition::SetInternetResourceState(active) => state .client - .exec_mut(|c| c.sut.set_disabled_resources(resources, now)), + .exec_mut(|c| c.sut.set_internet_resource_state(active, now)), Transition::SendIcmpPacket { src, dst, @@ -373,13 +373,14 @@ impl TunnelTest { let system_dns = ref_state.client.inner().system_dns_resolvers(); let upstream_dns = ref_state.client.inner().upstream_dns_resolvers(); let all_resources = ref_state.client.inner().all_resources(); - let disabled_resources = ref_state.client.inner().disabled_resources.clone(); + let internet_resource_state = ref_state.client.inner().active_internet_resource(); state.client.exec_mut(|c| { c.restart(key, now); // Apply to new instance. - c.sut.set_disabled_resources(disabled_resources, now); + c.sut + .set_internet_resource_state(internet_resource_state.is_some(), now); c.sut.update_interface_config(Interface { ipv4, ipv6, diff --git a/rust/connlib/tunnel/src/tests/transition.rs b/rust/connlib/tunnel/src/tests/transition.rs index ada08cf9c..811d0ed47 100644 --- a/rust/connlib/tunnel/src/tests/transition.rs +++ b/rust/connlib/tunnel/src/tests/transition.rs @@ -13,7 +13,7 @@ use crate::messages::DnsServer; use prop::collection; use proptest::{prelude::*, sample}; use std::{ - collections::{BTreeMap, BTreeSet}, + collections::BTreeMap, net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, num::NonZeroU16, }; @@ -21,12 +21,14 @@ use std::{ /// The possible transitions of the state machine. #[derive(Clone, Debug)] pub(crate) enum Transition { - /// Activate a resource on the client. - ActivateResource(Resource), - /// Deactivate a resource on the client. - DeactivateResource(ResourceId), - /// Client-side disable resource - DisableResources(BTreeSet), + /// Add a resource on the client. + AddResource(Resource), + /// Remove a resource on the client. + RemoveResource(ResourceId), + + /// Toggle the Internet Resource on / off + SetInternetResourceState(bool), + /// Send an ICMP packet to destination (IP resource, DNS resource or IP non-resource). SendIcmpPacket { src: IpAddr, diff --git a/rust/gui-client/src-tauri/src/controller.rs b/rust/gui-client/src-tauri/src/controller.rs index ada8f230c..54ba7506a 100644 --- a/rust/gui-client/src-tauri/src/controller.rs +++ b/rust/gui-client/src-tauri/src/controller.rs @@ -17,7 +17,7 @@ use futures::{ stream::{self, BoxStream}, }; use secrecy::{ExposeSecret as _, SecretString}; -use std::{collections::BTreeSet, ops::ControlFlow, path::PathBuf, task::Poll, time::Duration}; +use std::{ops::ControlFlow, path::PathBuf, task::Poll, time::Duration}; use tokio::sync::{mpsc, oneshot}; use tokio_stream::wrappers::ReceiverStream; use url::Url; @@ -145,16 +145,6 @@ impl Status { Status::TunnelReady { .. } | Status::WaitingForTunnel => true, } } - - fn internet_resource(&self) -> Option { - #[expect(clippy::wildcard_enum_match_arm)] - match self { - Status::TunnelReady { resources } => { - resources.iter().find(|r| r.is_internet_resource()).cloned() - } - _ => None, - } - } } enum EventloopTick { @@ -761,20 +751,10 @@ impl Controller { async fn update_disabled_resources(&mut self) -> Result<()> { settings::save_general(&self.general_settings).await?; - let Some(internet_resource) = self.status.internet_resource() else { - return Ok(()); - }; + let state = self.general_settings.internet_resource_enabled(); - let mut disabled_resources = BTreeSet::new(); - - if !self.general_settings.internet_resource_enabled() { - disabled_resources.insert(internet_resource.id()); - } - - self.send_ipc(&service::ClientMsg::SetDisabledResources( - disabled_resources, - )) - .await?; + self.send_ipc(&service::ClientMsg::SetInternetResourceState(state)) + .await?; self.refresh_ui_state(); Ok(()) diff --git a/rust/gui-client/src-tauri/src/service.rs b/rust/gui-client/src-tauri/src/service.rs index fb72b4573..3e4e9874d 100644 --- a/rust/gui-client/src-tauri/src/service.rs +++ b/rust/gui-client/src-tauri/src/service.rs @@ -5,7 +5,7 @@ use crate::{ use anyhow::{Context as _, Result, bail}; use atomicwrites::{AtomicFile, OverwriteBehavior}; use backoff::ExponentialBackoffBuilder; -use connlib_model::{ResourceId, ResourceView}; +use connlib_model::ResourceView; use firezone_bin_shared::{ DnsControlMethod, DnsController, TunDeviceManager, device_id::{self, DeviceId}, @@ -24,7 +24,6 @@ use futures::{ use phoenix_channel::{DeviceInfo, LoginUrl, PhoenixChannel, get_user_agent}; use secrecy::{Secret, SecretString}; use std::{ - collections::BTreeSet, io::{self, Write}, mem, pin::pin, @@ -59,7 +58,7 @@ pub enum ClientMsg { ApplyLogFilter { directives: String, }, - SetDisabledResources(BTreeSet), + SetInternetResourceState(bool), StartTelemetry { environment: String, release: String, @@ -555,14 +554,14 @@ impl<'a> Handler<'a> { tracing::warn!(path = %path.display(), %directives, "Failed to write new log directives: {}", err_with_src(&e)); } } - ClientMsg::SetDisabledResources(disabled_resources) => { + ClientMsg::SetInternetResourceState(state) => { let Some(connlib) = self.session.as_connlib() else { - // At this point, the GUI has already saved the disabled Resources to disk, so it'll be correct on the next sign-in anyway. - tracing::debug!("Cannot set disabled resources if we're signed out"); + // At this point, the GUI has already saved the state to disk, so it'll be correct on the next sign-in anyway. + tracing::debug!("Cannot enable/disable Internet Resource if we're signed out"); return Ok(()); }; - connlib.set_disabled_resources(disabled_resources); + connlib.set_internet_resource_state(state); } ClientMsg::StartTelemetry { environment, diff --git a/swift/apple/FirezoneNetworkExtension/Adapter.swift b/swift/apple/FirezoneNetworkExtension/Adapter.swift index 6a71a61be..3d717a91d 100644 --- a/swift/apple/FirezoneNetworkExtension/Adapter.swift +++ b/swift/apple/FirezoneNetworkExtension/Adapter.swift @@ -21,8 +21,6 @@ enum AdapterError: Error { case setDnsError(String) - case setDisabledResourcesError(String) - var localizedDescription: String { switch self { case .invalidSession(let session): @@ -32,8 +30,6 @@ enum AdapterError: Error { return "connlib failed to start: \(error)" case .setDnsError(let error): return "failed to set new DNS serversn: \(error)" - case .setDisabledResourcesError(let error): - return "failed to set new disabled resources: \(error)" } } } @@ -280,33 +276,7 @@ class Adapter { guard let self = self else { return } self.internetResourceEnabled = enabled - self.resourcesUpdated() - } - } - - func resourcesUpdated() { - let decoder = JSONDecoder() - decoder.keyDecodingStrategy = .convertFromSnakeCase - - internetResource = resources().filter { $0.isInternetResource() }.first - - var disablingResources: Set = [] - if let internetResource = internetResource, !internetResourceEnabled { - disablingResources.insert(internetResource.id) - } - - guard let currentlyDisabled = try? JSONEncoder().encode(disablingResources), - let toSet = String(data: currentlyDisabled, encoding: .utf8) - else { - fatalError("Should be able to encode 'disablingResources'") - } - - do { - try session?.setDisabledResources(toSet) - } catch let error { - // `toString` needed to deep copy the string and avoid a possible dangling pointer - let msg = (error as? RustString)?.toString() ?? "Unknown error" - Log.error(AdapterError.setDisabledResourcesError(msg)) + session?.setInternetResourceState(enabled) } } } @@ -382,7 +352,7 @@ extension Adapter: CallbackHandlerDelegate { networkSettings.apply() } - self.resourcesUpdated() + session?.setInternetResourceState(self.internetResourceEnabled) } }