Skip to content

Propagate NX domain and no record found errors#2502

Merged
marcus0x62 merged 6 commits into
hickory-dns:mainfrom
marcus0x62:forward_nxdomain_fix
Oct 19, 2024
Merged

Propagate NX domain and no record found errors#2502
marcus0x62 merged 6 commits into
hickory-dns:mainfrom
marcus0x62:forward_nxdomain_fix

Conversation

@marcus0x62
Copy link
Copy Markdown
Collaborator

@marcus0x62 marcus0x62 commented Oct 8, 2024

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:

@marcus0x62 marcus0x62 force-pushed the forward_nxdomain_fix branch from c7787f1 to 7b44aae Compare October 8, 2024 21:16
Copy link
Copy Markdown
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Sounds good!

(Tiny nit: consider keeping the first line of commit messages under ~70 characters, would avoid weird wrapping in the GitHub UI.)

Comment thread crates/proto/src/error.rs Outdated
Comment thread crates/proto/src/error.rs
Comment thread crates/recursor/src/recursor.rs Outdated
@bluejekyll
Copy link
Copy Markdown
Member

I guess I didn't fix the insecure look up issues before based on this PR.

Comment thread crates/proto/src/error.rs
Comment thread crates/recursor/src/error.rs Outdated
Comment thread crates/recursor/src/error.rs Outdated
Comment thread crates/proto/src/error.rs Outdated
Comment thread crates/server/src/authority/catalog.rs Outdated
Comment thread crates/server/src/authority/auth_lookup.rs Outdated
Comment thread crates/recursor/src/recursor.rs Outdated
Comment thread crates/recursor/src/recursor.rs Outdated
Comment thread crates/recursor/src/recursor.rs Outdated
Comment thread crates/proto/src/xfer/dnssec_dns_handle/mod.rs Outdated
@marcus0x62 marcus0x62 force-pushed the forward_nxdomain_fix branch 4 times, most recently from 820ab50 to 537e4ff Compare October 14, 2024 19:11
Comment thread crates/recursor/src/error.rs
@djc djc mentioned this pull request Oct 18, 2024
@marcus0x62 marcus0x62 force-pushed the forward_nxdomain_fix branch from 537e4ff to 35a97a4 Compare October 18, 2024 23:36
@marcus0x62 marcus0x62 added this pull request to the merge queue Oct 18, 2024
Merged via the queue into hickory-dns:main with commit 28cbb47 Oct 19, 2024
@marcus0x62 marcus0x62 deleted the forward_nxdomain_fix branch October 19, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants