From f965ca50703cbf84378989310414786acc9554bb Mon Sep 17 00:00:00 2001 From: Gabi Date: Tue, 20 Aug 2024 19:18:15 -0300 Subject: [PATCH] feat(connlib): handle internet resource (#6325) This PR handles the internet resource both on the gateway and client. In the gateway, it's handled like any other resource save for the address which is both ipv6 and ipv4. In the client it's handled mostly like any other resource but with some exceptions for DNS forwarded packets, because we want to work as a CIDR resource for the matter of forwarding packets to the gateway. Fixes: #6313. --------- Signed-off-by: Gabi Co-authored-by: Thomas Eizinger --- rust/connlib/shared/src/messages/client.rs | 1 + rust/connlib/shared/src/messages/gateway.rs | 7 ++- .../tunnel/proptest-regressions/tests.txt | 5 ++ rust/connlib/tunnel/src/client.rs | 57 +++++++++++++++---- rust/connlib/tunnel/src/tests/reference.rs | 23 ++++---- rust/connlib/tunnel/src/tests/sim_client.rs | 57 ++++++++++++++++--- rust/connlib/tunnel/src/tests/stub_portal.rs | 7 +-- 7 files changed, 122 insertions(+), 35 deletions(-) diff --git a/rust/connlib/shared/src/messages/client.rs b/rust/connlib/shared/src/messages/client.rs index 9c0de97f7..ae1aba083 100644 --- a/rust/connlib/shared/src/messages/client.rs +++ b/rust/connlib/shared/src/messages/client.rs @@ -195,6 +195,7 @@ impl ResourceDescription { (ResourceDescription::Cidr(cidr_a), ResourceDescription::Cidr(cidr_b)) => { cidr_a.address != cidr_b.address } + (ResourceDescription::Internet(_), ResourceDescription::Internet(_)) => false, _ => true, } } diff --git a/rust/connlib/shared/src/messages/gateway.rs b/rust/connlib/shared/src/messages/gateway.rs index 77e5959a9..18750fbbb 100644 --- a/rust/connlib/shared/src/messages/gateway.rs +++ b/rust/connlib/shared/src/messages/gateway.rs @@ -2,7 +2,7 @@ use std::net::IpAddr; -use ip_network::IpNetwork; +use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use itertools::Itertools; use serde::Deserialize; @@ -146,7 +146,10 @@ impl ResourceDescription { match self { ResourceDescription::Dns(r) => r.addresses.iter().copied().map_into().collect_vec(), ResourceDescription::Cidr(r) => vec![r.address], - ResourceDescription::Internet(_) => vec![], + ResourceDescription::Internet(_) => vec![ + Ipv4Network::DEFAULT_ROUTE.into(), + Ipv6Network::DEFAULT_ROUTE.into(), + ], } } diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt index ed8a22275..822253e08 100644 --- a/rust/connlib/tunnel/proptest-regressions/tests.txt +++ b/rust/connlib/tunnel/proptest-regressions/tests.txt @@ -85,3 +85,8 @@ cc ec2f348067458f6a7d3f2fbd1ab708a53fc27708440a3fcb6ed8557adc6db7d3 cc 2984b737f902f82c96ffec888a624afd7117078c125822b85de908c05f8e0b4c cc 51ad9fe7ef585d42bd1a6369da810a5adb6d756e71aa393362e542f1560d0273 cc b926f32ea3b2a04753bddd37be4804fd38fe35646e08507e68565883bd9fe2ed +cc f2b3aed5ec01491ac9f95c6baaadbf9fd05786940bc906d5a243a318b15907ea +cc 405da6a8d05abdc6fb7dfed17ca87d5f0893b289a277381eac757532992642f2 +cc 79e8ea948dddadd5db8bb7f587f8155490bcf2862c026eaf62c1039e5e1fd73d +cc 2ef061cf2eabbbef7db30f80486ec771e4b0d79ed55215fd86c9d14943e12eb4 +cc fa1ad96fcad83aa29156936f13a7722ccc1b349bc8c2022de4a6a20c3f4e9121 diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 73f365f5e..cf77113f6 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -248,6 +248,8 @@ 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, /// All resources indexed by their ID. resources_by_id: HashMap, @@ -318,6 +320,7 @@ impl ClientState { stub_resolver: StubResolver::new(known_hosts), disabled_resources: Default::default(), buffered_transmits: Default::default(), + internet_resource: None, } } @@ -623,6 +626,22 @@ impl ClientState { !interface.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() { + return false; + } + if self.internet_resource.is_some() { + return true; + } + + self.active_cidr_resources + .longest_match(dns_server) + .is_some() + } + /// Attempt to handle the given packet as a DNS query packet. /// /// Returns `Ok` if the packet is in fact a DNS query with an optional response to send back. @@ -645,10 +664,7 @@ impl ClientState { }) => { let ip = server.ip(); - // In case the DNS server is a CIDR resource, it needs to go through the tunnel. - if self.is_upstream_set_by_the_portal() - && self.active_cidr_resources.longest_match(ip).is_some() - { + if self.should_forward_dns_query_to_gateway(ip) { return Err((packet, ip)); } @@ -812,6 +828,14 @@ impl ClientState { .map(|(ip, _)| ip) .chain(iter::once(IPV4_RESOURCES.into())) .chain(iter::once(IPV6_RESOURCES.into())) + .chain( + self.internet_resource + .map(|_| Ipv4Network::DEFAULT_ROUTE.into()), + ) + .chain( + self.internet_resource + .map(|_| Ipv6Network::DEFAULT_ROUTE.into()), + ) .chain(self.dns_mapping.left_values().copied().map(Into::into)) } @@ -820,17 +844,21 @@ impl ClientState { } fn get_resource_by_destination(&self, destination: IpAddr) -> Option { - let maybe_dns_resource_id = self.stub_resolver.resolve_resource_by_ip(&destination); + // 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) + .filter(|resource| self.is_resource_enabled(resource)); + // We don't need to filter from here because resources are removed from the active_cidr_resources as soon as they are disabled. let maybe_cidr_resource_id = self .active_cidr_resources .longest_match(destination) .map(|(_, res)| res.id); - // We need to filter disabled resources because we never remove resources from the stub_resolver maybe_dns_resource_id .or(maybe_cidr_resource_id) - .filter(|resource| self.is_resource_enabled(resource)) + .or(self.internet_resource) } pub(crate) fn update_system_resolvers(&mut self, new_dns: Vec) { @@ -1006,7 +1034,9 @@ impl ClientState { None => true, } } - ResourceDescription::Internet(_) => false, // FIXME: Update with real check once Internet Resources get added. + ResourceDescription::Internet(resource) => { + self.internet_resource.replace(resource.id) != Some(resource.id) + } }; if !added { @@ -1034,7 +1064,11 @@ impl ClientState { match resource { ResourceDescription::Dns(_) => self.stub_resolver.remove_resource(id), ResourceDescription::Cidr(_) => self.active_cidr_resources.retain(|_, r| r.id != id), - ResourceDescription::Internet(_) => {} + ResourceDescription::Internet(_) => { + if self.internet_resource.is_some_and(|r_id| r_id == id) { + self.internet_resource = None; + } + } } let name = resource.name(); @@ -1161,7 +1195,10 @@ fn get_addresses_for_awaiting_resource( .map_into() .collect_vec(), ResourceDescription::Cidr(r) => vec![r.address], - ResourceDescription::Internet(_) => vec![], + ResourceDescription::Internet(_) => vec![ + Ipv4Network::DEFAULT_ROUTE.into(), + Ipv6Network::DEFAULT_ROUTE.into(), + ], } } diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs index ac281e237..c5f93212e 100644 --- a/rust/connlib/tunnel/src/tests/reference.rs +++ b/rust/connlib/tunnel/src/tests/reference.rs @@ -305,7 +305,7 @@ impl ReferenceStateMachine for ReferenceState { client.cidr_resources.insert(r.address, r.clone()); } client::ResourceDescription::Internet(r) => { - client.internet_resource = Some(r.id) + client.internet_resource = Some(r.clone()) } }); } @@ -338,10 +338,9 @@ impl ReferenceStateMachine for ReferenceState { continue; } - // Check if we the DNS server is defined as a CIDR resource. - let Some(resource) = state.client.inner().dns_query_via_cidr_resource(query) - else { - // Not a CIDR resource, process normally. + // Check if the DNS server is defined as a resource. + let Some(resource) = state.client.inner().dns_query_via_resource(query) else { + // Not a resource, process normally. state.client.exec_mut(|client| client.on_dns_query(query)); continue; }; @@ -352,10 +351,14 @@ impl ReferenceStateMachine for ReferenceState { continue; } - if !state.client.inner().is_connected_to_cidr(resource) { - state - .client - .exec_mut(|client| client.connected_cidr_resources.insert(resource)); + if !state + .client + .inner() + .is_connected_to_internet_or_cidr(resource) + { + state.client.exec_mut(|client| { + client.connect_to_internet_or_cidr_resource(resource) + }); pending_connections.insert(resource); continue; } @@ -554,7 +557,7 @@ impl ReferenceStateMachine for ReferenceState { .expected_dns_servers() .contains(&query.dns_server); let gateway_is_present_in_case_dns_server_is_cidr_resource = - match state.client.inner().dns_query_via_cidr_resource(query) { + match state.client.inner().dns_query_via_resource(query) { Some(r) => { let Some(gateway) = state.portal.gateway_for_resource(r) else { return false; diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs index f7521cbc2..eb928c05d 100644 --- a/rust/connlib/tunnel/src/tests/sim_client.rs +++ b/rust/connlib/tunnel/src/tests/sim_client.rs @@ -10,7 +10,10 @@ use crate::{proptest::*, ClientState}; use bimap::BiMap; use connlib_shared::{ messages::{ - client::{ResourceDescription, ResourceDescriptionCidr, ResourceDescriptionDns}, + client::{ + ResourceDescription, ResourceDescriptionCidr, ResourceDescriptionDns, + ResourceDescriptionInternet, + }, ClientId, DnsServer, GatewayId, Interface, RelayId, ResourceId, }, DomainName, @@ -256,7 +259,7 @@ pub struct RefClient { /// The upstream DNS resolvers configured in the portal. pub(crate) upstream_dns_resolvers: Vec, - pub(crate) internet_resource: Option, + pub(crate) internet_resource: Option, /// The CIDR resources the client is aware of. #[derivative(Debug = "ignore")] @@ -322,6 +325,7 @@ impl RefClient { pub(crate) fn reset_connections(&mut self) { self.connected_cidr_resources.clear(); self.connected_dns_resources.clear(); + self.connected_internet_resources = false; } pub(crate) fn is_tunnel_ip(&self, ip: IpAddr) -> bool { @@ -350,7 +354,7 @@ impl RefClient { tracing::Span::current().record("dst", tracing::field::display(dst)); // Second, if we are not yet connected, check if we have a resource for this IP. - let Some(rid) = self.internet_resource else { + let Some(rid) = self.internet_resource.as_ref().map(|r| r.id) else { tracing::debug!("No internet resource"); return; }; @@ -402,7 +406,7 @@ impl RefClient { return; }; - if self.is_connected_to_cidr(rid) && self.is_tunnel_ip(src) { + if self.is_connected_to_internet_or_cidr(rid) && self.is_tunnel_ip(src) { tracing::debug!("Connected to CIDR resource, expecting packet to be routed"); self.expected_icmp_handshakes .entry(gateway) @@ -413,7 +417,7 @@ impl RefClient { // If we have a resource, the first packet will initiate a connection to the gateway. tracing::debug!("Not connected to resource, expecting to trigger connection intent"); - self.connected_cidr_resources.insert(rid); + self.connect_to_internet_or_cidr_resource(rid); } #[tracing::instrument(level = "debug", skip_all, fields(dst, resource))] @@ -463,6 +467,25 @@ impl RefClient { } } + pub(crate) fn is_connected_to_internet_or_cidr(&self, resource: ResourceId) -> bool { + self.is_connected_to_cidr(resource) || self.is_connected_to_internet(resource) + } + + pub(crate) fn connect_to_internet_or_cidr_resource(&mut self, resource: ResourceId) { + if self + .internet_resource + .as_ref() + .is_some_and(|r| r.id == resource) + { + self.connected_internet_resources = true; + return; + } + + if self.cidr_resources.iter().any(|(_, r)| r.id == resource) { + self.connected_cidr_resources.insert(resource); + } + } + pub(crate) fn on_dns_query(&mut self, query: &DnsQuery) { self.dns_records .entry(query.domain.clone()) @@ -487,6 +510,11 @@ impl RefClient { .collect_vec() } + fn is_connected_to_internet(&self, id: ResourceId) -> bool { + self.internet_resource.as_ref().map(|r| r.id) == Some(id) + && self.connected_internet_resources + } + pub(crate) fn is_connected_to_cidr(&self, id: ResourceId) -> bool { self.connected_cidr_resources.contains(&id) } @@ -615,10 +643,10 @@ impl RefClient { .copied() } - /// Returns the CIDR resource we will forward the DNS query for the given name to. + /// Returns the resource we will forward the DNS query for the given name to. /// /// DNS servers may be resources, in which case queries that need to be forwarded actually need to be encapsulated. - pub(crate) fn dns_query_via_cidr_resource(&self, query: &DnsQuery) -> Option { + pub(crate) fn dns_query_via_resource(&self, query: &DnsQuery) -> Option { // Unless we are using upstream resolvers, DNS queries are never routed through the tunnel. if self.upstream_dns_resolvers.is_empty() { return None; @@ -630,6 +658,7 @@ impl RefClient { } self.cidr_resource_by_ip(query.dns_server.ip()) + .or(self.internet_resource.as_ref().map(|r| r.id)) } pub(crate) fn all_resource_ids(&self) -> Vec { @@ -648,7 +677,11 @@ impl RefClient { return true; } - if self.internet_resource.is_some_and(|r| r == resource_id) { + if self + .internet_resource + .as_ref() + .is_some_and(|r| r.id == resource_id) + { return true; } @@ -662,13 +695,19 @@ impl RefClient { .map(|(_, r)| r) .cloned() .map(ResourceDescription::Cidr); + let dns_resources = self .dns_resources .values() .cloned() .map(ResourceDescription::Dns); - Vec::from_iter(cidr_resources.chain(dns_resources)) + let internet_resource = self + .internet_resource + .clone() + .map(ResourceDescription::Internet); + + Vec::from_iter(cidr_resources.chain(dns_resources).chain(internet_resource)) } } diff --git a/rust/connlib/tunnel/src/tests/stub_portal.rs b/rust/connlib/tunnel/src/tests/stub_portal.rs index d38e970cb..73b03c424 100644 --- a/rust/connlib/tunnel/src/tests/stub_portal.rs +++ b/rust/connlib/tunnel/src/tests/stub_portal.rs @@ -107,10 +107,9 @@ impl StubPortal { .cloned() .map(client::ResourceDescription::Dns), ) - // TODO: Enable once we actually implement the Internet resource - // .chain(iter::once(client::ResourceDescription::Internet( - // self.internet_resource.clone(), - // ))) + .chain(iter::once(client::ResourceDescription::Internet( + self.internet_resource.clone(), + ))) .collect() }