github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 10:18:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

chore(connlib): Remove manual expansion of search domain (#8443)

Browse Source 
Reverts part of #8378 so that our OS-native expansion takes effect on
all platforms.

---------

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
This commit is contained in:
Jamil
2025-03-15 23:37:10 -05:00
committed by GitHub
parent 0c231eb536
commit 931048a667
7 changed files with 23 additions and 168 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
.github/workflows/_rust.yml vendored
View File

@@ -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:
# <https://github.com/rust-lang/cargo/issues/5999>

46
rust/connlib/tunnel/src/dns.rs
View File

@@ -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 {

10
rust/connlib/tunnel/src/tests/assertions.rs
View File

@@ -206,18 +206,10 @@ fn assert_packets_properties<T, U>(
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(

56
rust/connlib/tunnel/src/tests/reference.rs
View File

@@ -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<RecordType>)> {
let Some(search_domain) = self.client.inner().search_domain.clone() else {
return Vec::default();
};
fn domains_and_rtypes(
records: &DnsRecords,
) -> impl Iterator<Item = (DomainName, Vec<RecordType>)> + 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::<BTreeSet<_>>();
Vec::from_iter(unique_domains)
}
fn reachable_dns_servers(&self) -> Vec<SocketAddr> {
self.client
.inner()

55
rust/connlib/tunnel/src/tests/sim_client.rs
View File

@@ -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<DomainName> {
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<ResourceId> {
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()

22
rust/connlib/tunnel/src/tests/sut.rs
View File

@@ -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,

1
rust/headless-client/src/dns_control/linux/etc_resolv_conf.rs
View File

@@ -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