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