[PR #2213] [MERGED] `DnssecDnsHandle`: check RRSIG validity as per RFC4035 #2883

New issue

Closed

opened 2026-03-16 11:13:20 +03:00 by kerem · 0 comments

kerem commented

2026-03-16 11:13:20 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/2213
Author: @japaric
Created: 5/15/2024
Status: ✅ Merged
Merged: 7/1/2024
Merged by: @djc

Base: main ← Head: ja-gh2209

📝 Commits (3)

7e60b54 refactor Rrset into its own module
bd4d764 use RecordRef to simplify dnssec_dns_handle
f52e5f8 check RRSIG validity

📊 Changes

3 files changed (+245 additions, -113 deletions)

View changed files

📝 crates/proto/src/rr/resource.rs (+16 -0)
📝 crates/proto/src/xfer/dnssec_dns_handle.rs (+225 -105)
📝 crates/server/tests/authority_battery/dnssec.rs (+4 -8)

📄 Description

this PR adds the validity checks specified in section 5.3.1 of RFC4035, including the time check that was reported as missing in issue #2209

these new checks overlap with some of the existing ones but I opted for not removing the existing ones because I do not know how well the existing code is covered by tests.

I checked that I did not break this query:

let config = ResolverConfig::default();
let mut opts = ResolverOpts::default();
opts.validate = true;

let resolver = Resolver::new(config, opts)?;

let lookup = resolver.lookup("example.com.", RecordType::A)?;
lookup.record_iter().for_each(|record| {
    assert_eq!(Proof::Secure, record.proof());
});

It would be good to check that expired signatures are rejected. There are plans to add those kind of tests to the conformance test suite (https://github.com/ferrous-systems/dnssec-tests/issues/61) but given that Recursor does not use DnssecDnsHandle those tests won't exercise the code paths added here. I'm looking into reusing some of the existing code in DnssecDnsHandle to implement DNSSEC validation in Recursor (then conformance tests will cover some of DnssecDnsHandle code) but that's still ways off time-wise.

Another way to check the code here end to end would be to modify util/src/bin/resolve.rs to do DNSSEC validation (e.g. via a new CLI flag) then it would behave just like delv. That updated resolve could be comparatively against delv using the dns-test framework, that is without relying on external services / the internet.

I'm leaving this in draft state because the implementation will be cleaner when #2211 is implemented.

_{🔄 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/2213 **Author:** [@japaric](https://github.com/japaric) **Created:** 5/15/2024 **Status:** ✅ Merged **Merged:** 7/1/2024 **Merged by:** [@djc](https://github.com/djc) **Base:** `main` ← **Head:** `ja-gh2209` --- ### 📝 Commits (3) - [`7e60b54`](https://github.com/hickory-dns/hickory-dns/commit/7e60b547d17220d82490e78130435422a2d8718f) refactor `Rrset` into its own module - [`bd4d764`](https://github.com/hickory-dns/hickory-dns/commit/bd4d764e43926b8c1d1f538a17d12129675e26c6) use RecordRef to simplify dnssec_dns_handle - [`f52e5f8`](https://github.com/hickory-dns/hickory-dns/commit/f52e5f8e71e11b4392e797d0edbe6ee154bfaa72) check RRSIG validity ### 📊 Changes **3 files changed** (+245 additions, -113 deletions) <details> <summary>View changed files</summary> 📝 `crates/proto/src/rr/resource.rs` (+16 -0) 📝 `crates/proto/src/xfer/dnssec_dns_handle.rs` (+225 -105) 📝 `crates/server/tests/authority_battery/dnssec.rs` (+4 -8) </details> ### 📄 Description this PR adds the validity checks specified in section 5.3.1 of RFC4035, including the time check that was reported as missing in issue #2209 these new checks overlap with some of the existing ones but I opted for not removing the existing ones because I do not know how well the existing code is covered by tests. I checked that I did not break this query: ``` rust let config = ResolverConfig::default(); let mut opts = ResolverOpts::default(); opts.validate = true; let resolver = Resolver::new(config, opts)?; let lookup = resolver.lookup("example.com.", RecordType::A)?; lookup.record_iter().for_each(|record| { assert_eq!(Proof::Secure, record.proof()); }); ``` It would be good to check that expired signatures are rejected. There are plans to add those kind of tests to the conformance test suite (https://github.com/ferrous-systems/dnssec-tests/issues/61) but given that `Recursor` does not use `DnssecDnsHandle` those tests won't exercise the code paths added here. I'm looking into reusing some of the existing code in `DnssecDnsHandle` to implement DNSSEC validation in `Recursor` (then conformance tests will cover some of `DnssecDnsHandle` code) but that's still ways off time-wise. Another way to check the code here end to end would be to modify `util/src/bin/resolve.rs` to do DNSSEC validation (e.g. via a new CLI flag) then it would behave just like `delv`. That updated `resolve` could be comparatively against `delv` using the `dns-test` framework, that is without relying on external services / the internet. I'm leaving this in draft state because the implementation will be cleaner when #2211 is implemented. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>