From 931048a667d91229446f8f5fbfbf8f386a663f9b Mon Sep 17 00:00:00 2001 From: Jamil Date: Sat, 15 Mar 2025 23:37:10 -0500 Subject: [PATCH] chore(connlib): Remove manual expansion of search domain (#8443) Reverts part of #8378 so that our OS-native expansion takes effect on all platforms. --------- Co-authored-by: Thomas Eizinger --- .github/workflows/_rust.yml | 1 - rust/connlib/tunnel/src/dns.rs | 46 +-------------- rust/connlib/tunnel/src/tests/assertions.rs | 10 +--- rust/connlib/tunnel/src/tests/reference.rs | 56 +------------------ rust/connlib/tunnel/src/tests/sim_client.rs | 55 ++---------------- rust/connlib/tunnel/src/tests/sut.rs | 22 ++++---- .../src/dns_control/linux/etc_resolv_conf.rs | 1 + 7 files changed, 23 insertions(+), 168 deletions(-) diff --git a/.github/workflows/_rust.yml b/.github/workflows/_rust.yml index 0f91bc668..e2edaaec4 100644 --- a/.github/workflows/_rust.yml +++ b/.github/workflows/_rust.yml @@ -117,7 +117,6 @@ jobs: rg --count --no-ignore "Truncating DNS response" $TESTCASES_DIR rg --count --no-ignore "Destination is unreachable" $TESTCASES_DIR rg --count --no-ignore "Forwarding query for DNS resource to corresponding site" $TESTCASES_DIR - rg --count --no-ignore "Expanded single-label query into FQDN using search-domain" $TESTCASES_DIR env: # diff --git a/rust/connlib/tunnel/src/dns.rs b/rust/connlib/tunnel/src/dns.rs index 6290470d9..b80d9357d 100644 --- a/rust/connlib/tunnel/src/dns.rs +++ b/rust/connlib/tunnel/src/dns.rs @@ -1,9 +1,9 @@ use crate::client::IpProvider; -use anyhow::{Context as _, Result}; +use anyhow::Result; use connlib_model::ResourceId; use dns_types::{ - prelude::*, DomainName, DomainNameRef, OwnedRecordData, Query, RecordType, Response, - ResponseBuilder, ResponseCode, + DomainName, DomainNameRef, OwnedRecordData, Query, RecordType, Response, ResponseBuilder, + ResponseCode, }; use firezone_logging::{err_with_src, telemetry_span}; use itertools::Itertools; @@ -254,14 +254,6 @@ impl StubResolver { return ResolveStrategy::LocalResponse(Response::nxdomain(query)); } - // We override the `domain` here to ensure that we use the FQDN everywhere from here on. - let (domain, is_single_label_domain) = self - .fully_qualify_using_search_domain(domain.clone()) - .inspect_err( - |e| tracing::debug!(%domain, "Failed to expand single-label domain: {e:#}"), - ) - .unwrap_or((domain, true)); // Failure to fully-qualify it can only happen if it is a single-label domain. - // `match_resource` is `O(N)` which we deem fine for DNS queries. let maybe_resource = self.match_resource_linear(&domain); @@ -290,16 +282,6 @@ impl StubResolver { return ResolveStrategy::LocalResponse(Response::no_error(query)); } - (_, None) if is_single_label_domain => { - // Queries for single-label domains, i.e. local hostnames are never recursively resolved but are instead answered with nxdomain. - - tracing::trace!( - %domain, - "Query for single-label non-resource domain, responding with NXDOMAIN" - ); - - return ResolveStrategy::LocalResponse(Response::nxdomain(query)); - } _ => return ResolveStrategy::RecurseLocal, }; @@ -321,28 +303,6 @@ impl StubResolver { self.search_domain = new_search_domain; } - - fn fully_qualify_using_search_domain(&self, domain: DomainName) -> Result<(DomainName, bool)> { - // Root-label counts as a label. - if domain.label_count() != 2 { - return Ok((domain, false)); - } - - let search_domain = self - .search_domain - .clone() - .context("No search-domain configured to expand single-label domain")?; - - let domain = domain - .into_relative() - .chain(search_domain) - .context("Query + search domain exceeds max allowed domain length")? - .flatten_into(); - - tracing::trace!(target: "tunnel_test_coverage", "Expanded single-label query into FQDN using search-domain"); - - Ok((domain, true)) - } } pub fn is_subdomain(name: &dns_types::DomainName, resource: &str) -> bool { diff --git a/rust/connlib/tunnel/src/tests/assertions.rs b/rust/connlib/tunnel/src/tests/assertions.rs index 51f4fad5e..33382e466 100644 --- a/rust/connlib/tunnel/src/tests/assertions.rs +++ b/rust/connlib/tunnel/src/tests/assertions.rs @@ -206,18 +206,10 @@ fn assert_packets_properties( assert_destination_is_cdir_resource(gateway_received_request, resource_dst) } Destination::DomainName { name, .. } => { - let domain = match ref_client.fully_qualify_using_search_domain(name.clone()) { - Ok(domain) => domain, - Err(e) => { - tracing::error!(target: "assertions", "Failed to fully-qualify domain: {e:#}"); - return; - } - }; - assert_destination_is_dns_resource( gateway_received_request, global_dns_records, - &domain, + name, ); assert_proxy_ip_mapping_is_stable( diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs index 915d7f166..1458d5c15 100644 --- a/rust/connlib/tunnel/src/tests/reference.rs +++ b/rust/connlib/tunnel/src/tests/reference.rs @@ -272,17 +272,6 @@ impl ReferenceState { .prop_map(Transition::SendDnsQueries) }, ) - .with_if_not_empty( - 5, - ( - state.single_label_queries_for_search_domains(), - state.reachable_dns_servers(), - ), - |(domains, dns_servers)| { - dns_queries(sample::select(domains), sample::select(dns_servers)) - .prop_map(Transition::SendDnsQueries) - }, - ) .with_if_not_empty( 1, state @@ -608,16 +597,8 @@ impl ReferenceState { .sending_socket_for(query.dns_server.ip()) .is_some(); - let Ok(domain) = state - .client - .inner() - .fully_qualify_using_search_domain(query.domain.clone()) - else { - return false; - }; - let is_ptr_query = matches!(query.r_type, RecordType::PTR); - let is_known_domain = state.global_dns_records.contains_domain(&domain); + let is_known_domain = state.global_dns_records.contains_domain(&query.domain); // In case we sampled a PTR query, the domain doesn't have to exist. let ptr_or_known_domain = is_ptr_query || is_known_domain; @@ -740,41 +721,6 @@ impl ReferenceState { Vec::from_iter(unique_domains) } - // We surface what are the existing rtypes for a domain so that it's easier - // for the proptests to hit an existing record. - fn single_label_queries_for_search_domains(&self) -> Vec<(DomainName, Vec)> { - let Some(search_domain) = self.client.inner().search_domain.clone() else { - return Vec::default(); - }; - - fn domains_and_rtypes( - records: &DnsRecords, - ) -> impl Iterator)> + use<'_> { - records - .domains_iter() - .map(|d| (d.clone(), records.domain_rtypes(&d))) - } - - // We may have multiple gateways in a site, so we need to dedup. - let unique_domains = self - .gateways - .values() - .flat_map(|g| domains_and_rtypes(g.inner().dns_records())) - .chain(domains_and_rtypes(&self.global_dns_records)) - .filter_map(|(domain, rtypes)| { - let relative_name = domain.strip_suffix(&search_domain).ok()?; - - if relative_name.label_count() != 1 { - return None; - } - - Some((relative_name.into_absolute().unwrap(), rtypes)) - }) - .collect::>(); - - Vec::from_iter(unique_domains) - } - fn reachable_dns_servers(&self) -> Vec { self.client .inner() diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs index de574d701..1075f71ba 100644 --- a/rust/connlib/tunnel/src/tests/sim_client.rs +++ b/rust/connlib/tunnel/src/tests/sim_client.rs @@ -12,11 +12,9 @@ use crate::{ messages::{DnsServer, Interface}, }; use crate::{proptest::*, ClientState}; -use anyhow::Context; -use anyhow::Result; use bimap::BiMap; use connlib_model::{ClientId, GatewayId, RelayId, ResourceId, ResourceStatus, SiteId}; -use dns_types::{prelude::*, DomainName, Query, RecordData, RecordType}; +use dns_types::{DomainName, Query, RecordData, RecordType}; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use ip_network_table::IpNetworkTable; use ip_packet::{Icmpv4Type, Icmpv6Type, IpPacket, Layer4Protocol}; @@ -599,23 +597,6 @@ impl RefClient { } } - pub(crate) fn fully_qualify_using_search_domain( - &self, - domain: DomainName, - ) -> Result { - if domain.label_count() != 2 { - return Ok(domain); // If it is not a single-label domain, it is already fully qualified. - } - - let search_domain = self.search_domain.clone().context("No search domain")?; - let label = domain.into_relative(); - - Ok(label - .chain(search_domain) - .context("Domain too long")? - .flatten_into()) - } - #[expect(clippy::too_many_arguments, reason = "We don't care.")] pub(crate) fn on_icmp_packet( &mut self, @@ -779,6 +760,11 @@ impl RefClient { } pub(crate) fn on_dns_query(&mut self, query: &DnsQuery) { + self.dns_records + .entry(query.domain.clone()) + .or_default() + .insert(query.r_type); + match query.transport { DnsTransport::Udp => { self.expected_udp_dns_handshakes @@ -790,18 +776,6 @@ impl RefClient { } } - if query.domain.label_count() == 2 && self.dns_resource_by_domain(&query.domain).is_none() { - // DNS queries for single-label domains which are not resources are answered with NXDOMAIN. - tracing::debug!("No state change for non-resource single-label DNS query"); - - return; - } - - self.dns_records - .entry(query.domain.clone()) - .or_default() - .insert(query.r_type); - if let Some(resource) = self.is_site_specific_dns_query(query) { self.set_resource_online(resource); return; @@ -858,23 +832,6 @@ impl RefClient { } pub(crate) fn dns_resource_by_domain(&self, domain: &DomainName) -> Option { - let domain = domain.clone(); - - let domain = if domain.label_count() == 2 { - let Some(search_domain) = self.search_domain.clone() else { - tracing::error!("Must have search domain if we are handling single-label queries"); - return None; - }; - - domain - .into_relative() - .chain(search_domain) - .unwrap() - .flatten_into() - } else { - domain - }; - self.resources .iter() .cloned() diff --git a/rust/connlib/tunnel/src/tests/sut.rs b/rust/connlib/tunnel/src/tests/sut.rs index 7547560ad..5be744712 100644 --- a/rust/connlib/tunnel/src/tests/sut.rs +++ b/rust/connlib/tunnel/src/tests/sut.rs @@ -829,18 +829,18 @@ impl TunnelTest { fn address_from_destination(destination: &Destination, state: &TunnelTest, src: &IpAddr) -> IpAddr { match destination { Destination::DomainName { resolved_ip, name } => { - let dns_records = &state.client.inner().dns_records; + let available_ips = state + .client + .inner() + .dns_records + .get(name) + .unwrap() + .iter() + .filter(|ip| match ip { + IpAddr::V4(_) => src.is_ipv4(), + IpAddr::V6(_) => src.is_ipv6(), + }); - let Some(ips) = dns_records.get(name) else { - tracing::error!(%name, ?dns_records, "No DNS records"); - - panic!("No DNS records") - }; - - let available_ips = ips.iter().filter(|ip| match ip { - IpAddr::V4(_) => src.is_ipv4(), - IpAddr::V6(_) => src.is_ipv6(), - }); *resolved_ip.select(available_ips) } Destination::IpAddr(addr) => *addr, diff --git a/rust/headless-client/src/dns_control/linux/etc_resolv_conf.rs b/rust/headless-client/src/dns_control/linux/etc_resolv_conf.rs index 61be67a2c..54165f22e 100644 --- a/rust/headless-client/src/dns_control/linux/etc_resolv_conf.rs +++ b/rust/headless-client/src/dns_control/linux/etc_resolv_conf.rs @@ -97,6 +97,7 @@ fn configure_at_paths( new_resolv_conf.nameservers = dns_config.iter().map(|addr| (*addr).into()).collect(); new_resolv_conf.set_search(search_domain.into_iter().map(|d| d.to_string()).collect()); + new_resolv_conf.ndots = 1; // Must be 1 (e.g. the default) for search-domains to work // Over-writing `/etc/resolv.conf` actually violates Docker's plan for handling DNS // https://docs.docker.com/network/#dns-services