Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AsyncResolver take hosts file into account #2149

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 13 additions & 8 deletions crates/resolver/src/async_resolver.rs
Expand Up @@ -19,7 +19,7 @@ use proto::xfer::{DnsRequestOptions, RetryDnsHandle};
use tracing::{debug, trace};

use crate::caching_client::CachingClient;
use crate::config::{ResolverConfig, ResolverOpts};
use crate::config::{ResolveHosts, ResolverConfig, ResolverOpts};
use crate::dns_lru::{self, DnsLru};
use crate::error::*;
use crate::lookup::{self, Lookup, LookupEither, LookupFuture};
Expand Down Expand Up @@ -223,10 +223,9 @@ impl<P: ConnectionProvider> AsyncResolver<P> {
either = LookupEither::Retry(client);
}

let hosts = if options.use_hosts_file {
Some(Arc::new(Hosts::new()))
} else {
None
let hosts = match options.use_hosts_file {
ResolveHosts::Always | ResolveHosts::Auto => Some(Arc::new(Hosts::new())),
ResolveHosts::Never => None,
};

trace!("handle passed back");
Expand Down Expand Up @@ -367,9 +366,15 @@ impl<P: ConnectionProvider> AsyncResolver<P> {
L: From<Lookup> + Send + 'static,
{
let names = self.build_names(name);
LookupFuture::lookup(names, record_type, options, self.client_cache.clone())
.await
.map(L::from)
LookupFuture::lookup_with_hosts(
names,
record_type,
options,
self.client_cache.clone(),
self.hosts.clone(),
)
.await
.map(L::from)
}

/// Performs a dual-stack DNS lookup for the IP for the given hostname.
Expand Down
21 changes: 18 additions & 3 deletions crates/resolver/src/config.rs
Expand Up @@ -906,6 +906,21 @@ impl Default for ServerOrderingStrategy {
}
}

/// Whether the system hosts file should be respected by the resolver.
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "serde-config", derive(Serialize, Deserialize))]
pub enum ResolveHosts {
/// Always attempt to look up IP addresses from the system hosts file.
/// If the hostname cannot be found, query the DNS.
Always,
/// The DNS will always be queried.
Never,
/// Use local resolver configurations only when this resolver is not used in
/// a DNS forwarder. This is the default.
#[default]
Auto,
}

/// Configuration for the Resolver
#[derive(Debug, Clone, Eq, PartialEq)]
#[cfg_attr(
Expand Down Expand Up @@ -937,8 +952,8 @@ pub struct ResolverOpts {
pub ip_strategy: LookupIpStrategy,
/// Cache size is in number of records (some records can be large)
pub cache_size: usize,
/// Check /ect/hosts file before dns requery (only works for unix like OS)
pub use_hosts_file: bool,
/// Check /etc/hosts file before dns requery (only works for unix like OS)
pub use_hosts_file: ResolveHosts,
/// Optional minimum TTL for positive responses.
///
/// If this is set, any positive responses with a TTL lower than this value will have a TTL of
Expand Down Expand Up @@ -999,7 +1014,7 @@ impl Default for ResolverOpts {
validate: false,
ip_strategy: LookupIpStrategy::default(),
cache_size: 32,
use_hosts_file: true,
use_hosts_file: ResolveHosts::default(),
positive_min_ttl: None,
negative_min_ttl: None,
positive_max_ttl: None,
Expand Down
33 changes: 30 additions & 3 deletions crates/resolver/src/lookup.rs
Expand Up @@ -26,6 +26,7 @@ use crate::{
caching_client::CachingClient,
dns_lru::MAX_TTL,
error::*,
hosts::Hosts,
lookup_ip::LookupIpIter,
name_server::{ConnectionProvider, NameServerPool},
proto::{
Expand Down Expand Up @@ -280,19 +281,45 @@ where
/// * `client_cache` - cache with a connection to use for performing all lookups
#[doc(hidden)]
pub fn lookup(
names: Vec<Name>,
record_type: RecordType,
options: DnsRequestOptions,
client_cache: CachingClient<C>,
) -> Self {
Self::lookup_with_hosts(names, record_type, options, client_cache, None)
}

/// Perform a lookup from a name and type to a set of RDatas, taking the local
/// hosts file into account.
///
/// # Arguments
///
/// * `names` - a set of DNS names to attempt to resolve, they will be attempted in queue order, i.e. the first is `names.pop()`. Upon each failure, the next will be attempted.
/// * `record_type` - type of record being sought
/// * `client_cache` - cache with a connection to use for performing all lookups
/// * `hosts` - the local host file, the records inside it will be prioritized over the upstream DNS server
#[doc(hidden)]
pub fn lookup_with_hosts(
hch12907 marked this conversation as resolved.
Show resolved Hide resolved
mut names: Vec<Name>,
record_type: RecordType,
options: DnsRequestOptions,
mut client_cache: CachingClient<C>,
hosts: Option<Arc<Hosts>>,
hch12907 marked this conversation as resolved.
Show resolved Hide resolved
) -> Self {
let name = names.pop().ok_or_else(|| {
ResolveError::from(ResolveErrorKind::Message("can not lookup for no names"))
});

let query: Pin<Box<dyn Future<Output = Result<Lookup, ResolveError>> + Send>> = match name {
Ok(name) => client_cache
.lookup(Query::query(name, record_type), options)
.boxed(),
Ok(name) => {
let query = Query::query(name, record_type);

if let Some(lookup) = hosts.and_then(|h| h.lookup_static_host(&query)) {
future::ok(lookup).boxed()
} else {
client_cache.lookup(query, options).boxed()
}
}
Err(err) => future::err(err).boxed(),
};

Expand Down
14 changes: 12 additions & 2 deletions crates/server/src/store/forwarder/authority.rs
Expand Up @@ -7,7 +7,7 @@

use std::io;

use hickory_resolver::name_server::TokioConnectionProvider;
use hickory_resolver::{config::ResolveHosts, name_server::TokioConnectionProvider};
use tracing::{debug, info};

use crate::{
Expand Down Expand Up @@ -75,6 +75,12 @@ impl ForwardAuthority {
options.preserve_intermediates = true;
}

// Require people to explicitly request for /etc/hosts usage in forwarder
// configs
if options.use_hosts_file == ResolveHosts::Auto {
options.use_hosts_file = ResolveHosts::Never;
}

let config = ResolverConfig::from_parts(None, vec![], name_servers);

let resolver = TokioAsyncResolver::new(config, options, TokioConnectionProvider::default());
Expand Down Expand Up @@ -127,7 +133,11 @@ impl Authority for ForwardAuthority {
debug_assert!(self.origin.zone_of(name));

debug!("forwarding lookup: {} {}", name, rtype);
let name: LowerName = name.clone();

// Ignore FQDN when we forward DNS queries. Without this we can't look
// up addresses from system hosts file.
let mut name: Name = name.clone().into();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a non-trivial change -- why do you think this is the right thing to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query sent to upstream DNS is the same no matter FQDN is enabled or not. It seems that FQDN only affects hosts lookup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want the Forwarder to use the local resolve config at all? Do other DNS servers do that on forwarded requests? I don't think so. I think if we want a Hosts file to be used, we should create a separate Authority type for that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dnsmasq does that, not too sure though... But maybe we can disable use_hosts_file by default in ForwardConfig?

Copy link
Collaborator

@djc djc Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me.

name.set_fqdn(false);
let resolve = self.resolver.lookup(name, rtype).await;

resolve.map(ForwardLookup).map_err(LookupError::from)
Expand Down