[PR #2502] [MERGED] Propagate NX domain and no record found errors #3094

New issue

Closed

opened 2026-03-16 11:24:45 +03:00 by kerem · 0 comments

kerem commented

2026-03-16 11:24:45 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/2502
Author: @marcus0x62
Created: 10/8/2024
Status: ✅ Merged
Merged: 10/19/2024
Merged by: @marcus0x62

Base: main ← Head: forward_nxdomain_fix

📝 Commits (6)

b787a39 Preserve authority records when translating between error types
5ada206 Preserve authority records in forwarded responses from the catalog
4ccccaa Preserve authority records in the recursor DNSSEC dns handle
aa7f609 Process error messages in the dnssec response handler
eeecf0b Response code coverage tests
35a97a4 Existing test updates

📊 Changes

17 files changed (+593 additions, -87 deletions)

View changed files

📝 conformance/packages/conformance-tests/src/resolver/dns/scenarios.rs (+0 -1)
📝 conformance/packages/conformance-tests/src/resolver/nsec.rs (+0 -4)
📝 crates/proto/src/error.rs (+69 -6)
📝 crates/proto/src/xfer/dnssec_dns_handle/mod.rs (+28 -1)
📝 crates/recursor/src/error.rs (+80 -16)
📝 crates/recursor/src/recursor.rs (+79 -17)
📝 crates/recursor/src/recursor_dns_handle.rs (+4 -4)
📝 crates/resolver/src/caching_client.rs (+8 -3)
📝 crates/resolver/src/dns_lru.rs (+4 -0)
📝 crates/resolver/src/error.rs (+19 -1)
📝 crates/server/src/authority/auth_lookup.rs (+22 -0)
📝 crates/server/src/authority/catalog.rs (+111 -31)
📝 crates/server/src/authority/error.rs (+25 -2)
📝 tests/e2e-tests/src/recursor.rs (+1 -0)
➕ tests/e2e-tests/src/recursor/basic.rs (+1 -0)
➕ tests/e2e-tests/src/recursor/basic/scenarios.rs (+141 -0)
📝 tests/integration-tests/tests/integration/chained_authority_tests.rs (+1 -1)

📄 Description

This PR allows the Hickory recursor to more accurately provide NXDomain and NoData responses to queries. This required a number of changes to accomplish:

Change the ProtoErrorKind::Forward variant to preserve authority records and introduce a conversion method from RecursorErrorKind::Forward to ProtoErrorKind::NoRecordsFound for sending accurate responses back to clients.
Change the catalog forward response building method to preserve authority records
Change the DNSSEC Handler in the recursor to send back ProtoErrorKind::NoRecordsFound when dealing with NXDomain and NoData responses
Change the DNSSEC send handler to process error messages. Previously, the DNSSEC handler would ignore any error responses, which resulted in those responses not undergoing validation, even when Hickory is configured in validating mode.

There are also new tests for basic response code correctness to validate the changes above, and un-ignoring some conformance tests that now pass.

This change-set covers all of the NXDomain and NoError response mangling scenarios I could find in the code base, and it fixes a few DNSSEC-related issues. Significant DNSSEC-related processing issues remain, however, and I think will need to be addressed in a separate PR. While working on this fix, I also noticed:

The NSEC3 code does not appear to handle opt-out proofs at all
The DNSSEC code does not appear to correctly handle insecure delegations, partially due to the above.

The net result of this is that any queries to an insecure delegation from a secure parent will fail if hickory is configured as a validating resolver (i.e., almost all queries will fail if hickory is configured as a validating resolver.)

This PR does not fix the insecure delegation validation problem, but any realistic fix is contingent on fixing this issues this PR does address.

Related Issues/PRs:

Issue #2503
Issue #2435
Issue #2395
Issue #2514
PR #2477
PR #2388

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/hickory-dns/hickory-dns/pull/2502 **Author:** [@marcus0x62](https://github.com/marcus0x62) **Created:** 10/8/2024 **Status:** ✅ Merged **Merged:** 10/19/2024 **Merged by:** [@marcus0x62](https://github.com/marcus0x62) **Base:** `main` ← **Head:** `forward_nxdomain_fix` --- ### 📝 Commits (6) - [`b787a39`](https://github.com/hickory-dns/hickory-dns/commit/b787a3961589f172dc07629fc9750973bda8bf7f) Preserve authority records when translating between error types - [`5ada206`](https://github.com/hickory-dns/hickory-dns/commit/5ada20642d05987d3737a70837d452797fe6ceba) Preserve authority records in forwarded responses from the catalog - [`4ccccaa`](https://github.com/hickory-dns/hickory-dns/commit/4ccccaaa39acd686c5720cbc8095f910ec1b9cd5) Preserve authority records in the recursor DNSSEC dns handle - [`aa7f609`](https://github.com/hickory-dns/hickory-dns/commit/aa7f609606dcefceb04c387cf280df6d0787c1dc) Process error messages in the dnssec response handler - [`eeecf0b`](https://github.com/hickory-dns/hickory-dns/commit/eeecf0be196b7d3fd143f19dc9cc02da729bf39b) Response code coverage tests - [`35a97a4`](https://github.com/hickory-dns/hickory-dns/commit/35a97a4878ed7ba997f44a8093f6d1053fcf6140) Existing test updates ### 📊 Changes **17 files changed** (+593 additions, -87 deletions) <details> <summary>View changed files</summary> 📝 `conformance/packages/conformance-tests/src/resolver/dns/scenarios.rs` (+0 -1) 📝 `conformance/packages/conformance-tests/src/resolver/nsec.rs` (+0 -4) 📝 `crates/proto/src/error.rs` (+69 -6) 📝 `crates/proto/src/xfer/dnssec_dns_handle/mod.rs` (+28 -1) 📝 `crates/recursor/src/error.rs` (+80 -16) 📝 `crates/recursor/src/recursor.rs` (+79 -17) 📝 `crates/recursor/src/recursor_dns_handle.rs` (+4 -4) 📝 `crates/resolver/src/caching_client.rs` (+8 -3) 📝 `crates/resolver/src/dns_lru.rs` (+4 -0) 📝 `crates/resolver/src/error.rs` (+19 -1) 📝 `crates/server/src/authority/auth_lookup.rs` (+22 -0) 📝 `crates/server/src/authority/catalog.rs` (+111 -31) 📝 `crates/server/src/authority/error.rs` (+25 -2) 📝 `tests/e2e-tests/src/recursor.rs` (+1 -0) ➕ `tests/e2e-tests/src/recursor/basic.rs` (+1 -0) ➕ `tests/e2e-tests/src/recursor/basic/scenarios.rs` (+141 -0) 📝 `tests/integration-tests/tests/integration/chained_authority_tests.rs` (+1 -1) </details> ### 📄 Description This PR allows the Hickory recursor to more accurately provide NXDomain and NoData responses to queries. This required a number of changes to accomplish: 1. Change the ProtoErrorKind::Forward variant to preserve authority records and introduce a conversion method from RecursorErrorKind::Forward to ProtoErrorKind::NoRecordsFound for sending accurate responses back to clients. 2. Change the catalog forward response building method to preserve authority records 3. Change the DNSSEC Handler in the recursor to send back ProtoErrorKind::NoRecordsFound when dealing with NXDomain and NoData responses 4. Change the DNSSEC send handler to process error messages. Previously, the DNSSEC handler would ignore any error responses, which resulted in those responses not undergoing validation, even when Hickory is configured in validating mode. There are also new tests for basic response code correctness to validate the changes above, and un-ignoring some conformance tests that now pass. This change-set covers all of the NXDomain and NoError response mangling scenarios I could find in the code base, and it fixes a few DNSSEC-related issues. Significant DNSSEC-related processing issues remain, however, and I think will need to be addressed in a separate PR. While working on this fix, I also noticed: * The NSEC3 code does not appear to handle opt-out proofs at all * The DNSSEC code does not appear to correctly handle insecure delegations, partially due to the above. The net result of this is that any queries to an insecure delegation from a secure parent will fail if hickory is configured as a validating resolver (i.e., almost all queries will fail if hickory is configured as a validating resolver.) This PR does not fix the insecure delegation validation problem, but any realistic fix is contingent on fixing this issues this PR does address. Related Issues/PRs: * Issue #2503 * Issue #2435 * Issue #2395 * Issue #2514 * PR #2477 * PR #2388 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>