LLT-6364: Update trust-dns #1512

mathiaspeters · 2025-10-14T09:39:04Z

Problem

Our trust-dns fork was much behind upstream

Solution

Update the fork from v0.24.0 to v0.25.2 and bump the reference in libtelio

The branch in trust-dns has 7 commits that can also be reviewed

☑️ Definition of Done checklist

Commit history is clean (requirements)
README.md is updated
Functionality is covered by unit or integration tests

tomaszklak

+1

.unreleased/LLT-6364

crates/telio-dns/Cargo.toml

crates/telio-dns/src/forward.rs

Jauler · 2025-10-21T13:37:49Z

crates/telio-dns/src/forward.rs

+            .map_err(|err|
+                match err.kind() {
+                    ResolveErrorKind::Proto(proto) => {
+                        match proto.kind() {
+                            ProtoErrorKind::NoRecordsFound {
+                                query: _,
+                                soa: _,
+                                ns: _,
+                                negative_ttl: _,
+                                response_code,
+                                trusted: _,
+                                authorities:_,
+                            } => {
+                                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)),
+                        }
                    }
-                }
-                // 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)),
-            })
+                    // 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)),
+                });


Now that we have LookupControlFlow maybe we do not need this block with rather weird, and likely incorrect under some edge-cases, conversions and we can simply pass the result without map_err()'ing it?

I looked into it and I think this will be staying, but I tried to clean it up at least a little bit

Basically, this is here to give us greater control over when we get an empty result and when the DNS request properly fails

Could you elaborate a bit more? As this, looking at first glance, does not look correct error mapping 🤔

nat-lab/tests/test_dns.py

Jauler · 2025-11-07T08:59:51Z

crates/telio-dns/src/forward.rs

+
+        // 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))


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()`

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

🤔 🤔 🤔

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.

LukasPukenis · 2025-11-10T08:28:16Z

Cargo.lock

 name = "hickory-resolver"
-version = "0.24.0"
-source = "git+https://github.com/NordSecurity/trust-dns.git?tag=v3.0.1#88204024fd3ecdadccf4574eb7976167e7c52001"
+version = "0.25.2"


Friendly reminder to close a dependabot issue after merging this PR since there's one for hickory-proto

mathiaspeters requested a review from a team as a code owner October 14, 2025 09:39

mathiaspeters temporarily deployed to Internal October 15, 2025 18:53 — with GitHub Actions Inactive

mathiaspeters temporarily deployed to Internal October 16, 2025 16:15 — with GitHub Actions Inactive

mathiaspeters force-pushed the LLT-6364_update_trust_dns branch from bf05b37 to c4011b6 Compare October 16, 2025 17:56

mathiaspeters temporarily deployed to Internal October 16, 2025 17:56 — with GitHub Actions Inactive

mathiaspeters force-pushed the LLT-6364_update_trust_dns branch from c4011b6 to 099641a Compare October 16, 2025 18:52

mathiaspeters temporarily deployed to Internal October 16, 2025 18:53 — with GitHub Actions Inactive

mathiaspeters temporarily deployed to Internal October 16, 2025 21:25 — with GitHub Actions Inactive

mathiaspeters temporarily deployed to Internal October 17, 2025 08:45 — with GitHub Actions Inactive

mathiaspeters force-pushed the LLT-6364_update_trust_dns branch from 788a084 to 6ea0d6b Compare October 17, 2025 10:15

mathiaspeters temporarily deployed to Internal October 17, 2025 10:15 — with GitHub Actions Inactive

tomaszklak previously approved these changes Oct 17, 2025

View reviewed changes

mathiaspeters dismissed tomaszklak’s stale review via d3f34a0 October 17, 2025 16:08

mathiaspeters force-pushed the LLT-6364_update_trust_dns branch from 6ea0d6b to d3f34a0 Compare October 17, 2025 16:08

mathiaspeters temporarily deployed to Internal October 17, 2025 16:08 — with GitHub Actions Inactive

Jauler reviewed Oct 21, 2025

View reviewed changes

.unreleased/LLT-6364 Show resolved Hide resolved

Jauler reviewed Oct 21, 2025

View reviewed changes

crates/telio-dns/Cargo.toml Outdated Show resolved Hide resolved

Jauler reviewed Oct 21, 2025

View reviewed changes

crates/telio-dns/src/forward.rs Outdated Show resolved Hide resolved

Jauler reviewed Oct 21, 2025

View reviewed changes

nat-lab/tests/test_dns.py Outdated Show resolved Hide resolved

mathiaspeters force-pushed the LLT-6364_update_trust_dns branch from d3f34a0 to 19757ca Compare October 28, 2025 18:13

mathiaspeters temporarily deployed to Internal October 28, 2025 18:13 — with GitHub Actions Inactive

Update trust-dns

a3609a7

mathiaspeters force-pushed the LLT-6364_update_trust_dns branch from 19757ca to a3609a7 Compare October 29, 2025 15:57

mathiaspeters temporarily deployed to Internal October 29, 2025 15:58 — with GitHub Actions Inactive

Jauler reviewed Nov 7, 2025

View reviewed changes

LukasPukenis reviewed Nov 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

LLT-6364: Update trust-dns #1512

LLT-6364: Update trust-dns #1512

Uh oh!

mathiaspeters commented Oct 14, 2025 •

edited

Loading

Uh oh!

tomaszklak left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jauler Oct 21, 2025

Uh oh!

mathiaspeters Oct 28, 2025

Uh oh!

Jauler Nov 24, 2025

Uh oh!

Uh oh!

Jauler Nov 7, 2025

Uh oh!

mathiaspeters Nov 12, 2025

Uh oh!

Jauler Nov 24, 2025 •

edited

Loading

Uh oh!

LukasPukenis Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LLT-6364: Update trust-dns #1512

Are you sure you want to change the base?

LLT-6364: Update trust-dns #1512

Uh oh!

Conversation

mathiaspeters commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

☑️ Definition of Done checklist

Uh oh!

tomaszklak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jauler Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

mathiaspeters Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Jauler Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jauler Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

mathiaspeters Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Jauler Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukasPukenis Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mathiaspeters commented Oct 14, 2025 •

edited

Loading

Jauler Nov 24, 2025 •

edited

Loading