Skip to content

LLT-6364: Update trust-dns #1512

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.

Sign up for GitHub

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

Open
mathiaspeters wants to merge 1 commit into
base: main
Choose a base branch
from
Open

LLT-6364: Update trust-dns #1512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .unreleased/LLT-6364
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update trust-dns to v4.0.1 which bumps our fork from upstream v0.24.0 to v0.25.2
89 changes: 61 additions & 28 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/telio-dns/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ publish = false

[dependencies]
rand = { workspace = true, default-features = false }
hickory-server = { git = "https://github.com/NordSecurity/trust-dns.git", tag = "v3.0.1", features = ["hickory-resolver"], default-features = false }
hickory-server = { git = "https://github.com/NordSecurity/trust-dns.git", tag = "v4.0.1", features = ["resolver"], default-features = false }
async-trait.workspace = true
neptun.workspace = true
x25519-dalek.workspace = true
Expand Down
104 changes: 59 additions & 45 deletions crates/telio-dns/src/forward.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,25 @@ use std::{
io,
net::{Ipv4Addr, Ipv6Addr, SocketAddr},
pin::Pin,
time::Duration,
};

use async_trait::async_trait;
use hickory_server::{
authority::{
Authority, LookupError, LookupObject, LookupOptions, MessageRequest, UpdateResult, ZoneType,
Authority, LookupControlFlow, LookupError, LookupObject, LookupOptions, MessageRequest,
UpdateResult, ZoneType,
},
proto::{
op::ResponseCode,
rr::{LowerName, Name, Record, RecordType},
runtime::{RuntimeProvider, TokioRuntimeProvider},
udp::{DnsUdpSocket, UdpSocket as ProtoUdpSocket},
ProtoErrorKind,
},
resolver::{
config::ResolverConfig,
error::ResolveErrorKind,
lookup::Lookup as ResolverLookup,
name_server::{GenericConnector, RuntimeProvider, TokioRuntimeProvider},
AsyncResolver,
config::ResolverConfig, lookup::Lookup as ResolverLookup, name_server::GenericConnector,
ResolveError, ResolveErrorKind, Resolver,
},
server::RequestInfo,
store::forwarder::ForwardConfig,
Expand All @@ -49,8 +50,10 @@ impl RuntimeProvider for TelioRuntimeProvider {
fn connect_tcp(
&self,
server_addr: SocketAddr,
bind_addr: Option<SocketAddr>,
timeout: Option<Duration>,
) -> Pin<Box<dyn Send + Future<Output = io::Result<Self::Tcp>>>> {
self.0.connect_tcp(server_addr)
self.0.connect_tcp(server_addr, bind_addr, timeout)
}

fn bind_udp(
Expand All @@ -66,7 +69,7 @@ impl RuntimeProvider for TelioRuntimeProvider {
}
}

pub type TelioAsyncResolver = AsyncResolver<GenericConnector<TelioRuntimeProvider>>;
pub type TelioAsyncResolver = Resolver<GenericConnector<TelioRuntimeProvider>>;

pub struct TelioUdpSocket(UdpSocket);

Expand Down Expand Up @@ -168,7 +171,9 @@ impl ForwardAuthority {

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

let resolver = TelioAsyncResolver::new(config, options, GenericConnector::default());
let resolver = TelioAsyncResolver::builder_with_config(config, GenericConnector::default())
.with_options(options)
.build();

telio_log_info!("forward resolver configured: {}: ", origin);

Expand All @@ -178,6 +183,41 @@ impl ForwardAuthority {
resolver,
})
}

fn transform_lookup_error(name: &LowerName, err: ResolveError) -> LookupError {
if let ResolveErrorKind::Proto(proto) = err.kind() {
if let ProtoErrorKind::NoRecordsFound {
query: _,
soa: _,
ns: _,
negative_ttl: _,
response_code,
trusted: _,
authorities: _,
} = proto.kind()
{
return if *response_code == ResponseCode::NoError {
telio_log_debug!("Got an error response with NoError code for {name}, this should not happen so converting to ServFail");
// Failed query with no error - convert that to a real error,
// otherwise the LookupError::from will panic in debug builds.
// If we use a number from the 'private use' range:
// https://datatracker.ietf.org/doc/html/rfc2929#section-2.3 like
// LookupError::from(ResponseCode::Unknown(3841))
// the trust-dns will end up looping until the requests with some other error
// is returned. This will make the original dns request (eg. by nslookup or dig or some app)
// never complete. To avoid that, lets return ServFail which will produce
// an empty respones.
LookupError::from(ResponseCode::ServFail)
} else {
LookupError::from(*response_code)
};
}
}

// NOTE: this is probably incorrect, and should be at least 24, most likely
// in range 3841-4095 ('private use' range), instead of '0'.
LookupError::from(ResponseCode::Unknown(0))
Copy link
Contributor

@Jauler Jauler Nov 7, 2025

Choose a reason for hiding this comment

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

I think we are still converting any error other then err.kind() == ResolveErrorKind::Proto(...) to LookupError::ResponseCode which IMHO is incorrect.

IMHO we should convert to LookupError::ProtoError()`

Copy link
Contributor Author

@mathiaspeters mathiaspeters Nov 12, 2025

Choose a reason for hiding this comment

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

Generally I would agree with you, but this has a specific use case: https://github.com/NordSecurity/trust-dns/blob/release/0.25.2/crates/server/src/authority/catalog.rs#L837

The way this is handled in our fork of hickory is that we want to have certain errors actually result in an error and others resulting in an empty response. I believe this was initially set to Unknown so that we can have more control over when that happens, and this just copies that. We can change this to be one of the variants of ProtoError, but then I think it's more likely that the special handling of errors in our hickory fork might behave differently

Copy link
Contributor

@Jauler Jauler Nov 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 🤔 🤔

In the link you provided, in one of the cases - we are converting IO errors (hence DNS request was probably not even sent, or response not even received into empty response? As telio-dns is kind of transparent proxy - it means we should not respond as well 🤔. And definitely should not convert the response into empty, but successful response.

Additionally if we have unknown code (the other case) responded from upstream server - we should not convert it to successful response as well.

}
}

#[async_trait::async_trait]
Expand All @@ -186,7 +226,7 @@ impl Authority for ForwardAuthority {

/// Always Forward
fn zone_type(&self) -> ZoneType {
ZoneType::Forward
ZoneType::External
}

/// Always false for Forward zones
Expand All @@ -213,7 +253,7 @@ impl Authority for ForwardAuthority {
name: &LowerName,
rtype: RecordType,
_lookup_options: LookupOptions,
) -> Result<Self::Lookup, LookupError> {
) -> LookupControlFlow<Self::Lookup> {
// TODO: make this an error?
debug_assert!(self.origin.zone_of(name));

Expand All @@ -227,7 +267,7 @@ impl Authority for ForwardAuthority {
//
// Log such errors with lower logging level
Err(ref e)
if matches!(e.kind(), ResolveErrorKind::NoRecordsFound { .. })
if e.is_no_records_found()
&& (rtype == RecordType::AAAA || rtype == RecordType::SOA) =>
{
telio_log_debug!("DNS name resolution failed with {:?}", e);
Expand All @@ -237,43 +277,17 @@ impl Authority for ForwardAuthority {
Ok(_) => (),
};

resolve
let result = resolve
.map(ForwardLookup)
.map_err(|code| match code.kind() {
ResolveErrorKind::NoRecordsFound {
query: _,
soa: _,
negative_ttl: _,
response_code,
trusted: _,
} => {
if *response_code == ResponseCode::NoError {
telio_log_debug!("Got an error response with NoError code for {name}, this should not happen so converting to ServFail");
// Failed query with no error - convert that to a real error,
// otherwise the LookupError::from will panic in debug builds.
// If we use a number from the 'private use' range:
// https://datatracker.ietf.org/doc/html/rfc2929#section-2.3 like
// LookupError::from(ResponseCode::Unknown(3841))
// the trust-dns will end up looping until the requests with some other error
// is returned. This will make the original dns request (eg. by nslookup or dig or some app)
// never complete. To avoid that, lets return ServFail which will produce
// an empty respones.
LookupError::from(ResponseCode::ServFail)
} else {
LookupError::from(*response_code)
}
}
// NOTE: this is probably incorrect, and should be at least 24, most likely
// in range 3841-4095 ('private use' range), instead of '0'.
_ => LookupError::from(ResponseCode::Unknown(0)),
})
.map_err(|err| Self::transform_lookup_error(name, err));
LookupControlFlow::Continue(result)
}

async fn search(
&self,
request_info: RequestInfo<'_>,
lookup_options: LookupOptions,
) -> Result<Self::Lookup, LookupError> {
) -> LookupControlFlow<Self::Lookup> {
self.lookup(
request_info.query.name(),
request_info.query.query_type(),
Expand All @@ -286,10 +300,10 @@ impl Authority for ForwardAuthority {
&self,
_name: &LowerName,
_lookup_options: LookupOptions,
) -> Result<Self::Lookup, LookupError> {
Err(LookupError::from(io::Error::other(
) -> LookupControlFlow<Self::Lookup> {
LookupControlFlow::Break(Err(LookupError::from(io::Error::other(
"Getting NSEC records is unimplemented for the forwarder",
)))
))))
}
}

Expand Down
Loading
Loading