[PR #2774] [MERGED] Fix NSEC3 validation's logic for covering records #3306

New issue

Closed

opened 2026-03-16 11:36:34 +03:00 by kerem · 0 comments

kerem commented

2026-03-16 11:36:34 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/2774
Author: @divergentdave
Created: 2/12/2025
Status: ✅ Merged
Merged: 2/13/2025
Merged by: @divergentdave

Base: main ← Head: david/nsec3-covers-fix

📝 Commits (3)

1982b83 Store base32-encoded next hashed name in NSEC3
3024269 Replace Nsec3Base32Hash with Label
a75bf49 NSEC3 validation: Fix find_covering_record()

📊 Changes

2 files changed (+106 additions, -86 deletions)

View changed files

📝 crates/proto/src/dnssec/dnssec_dns_handle/nsec3_validation.rs (+45 -83)
📝 crates/proto/src/dnssec/rdata/nsec3.rs (+61 -3)

📄 Description

The find_covering_record() helper function in the NSEC3 validation code is incorrect because it always returns something when given a nonempty set of NSEC3 records. This PR removes the fallback in or_else() and instead adds a separate case to handle the last NSEC3 record in a zone. RFC 5155 says "The value of the Next Hashed Owner Name field in the last NSEC3 RR in the zone is the same as the hashed owner name of the first NSEC3 RR in the zone in hash order", so we need to test that the target hashed owner name is in the wrap-around range between the two hashed names represented by the record.

To detect which case the NSEC3 record falls in, we need to compare a base32-encoded hashed owner name from the record's name and the next hashed owner name from the RDATA, which is a raw digest. Thus, I added a field to the NSEC3 struct that stores the base32 encoding of the next hashed owner name, so it can be compared directly. I also replaced the recently introduced Nsec3Base32Hash struct, used for case-insensitive comparisons, with Label, which has the same comparison semantics and is already used elsewhere.

I haven't written a test case for this yet, I might try to cobble together one from the zone signing utilities and a dnslib server.

_{🔄 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/2774 **Author:** [@divergentdave](https://github.com/divergentdave) **Created:** 2/12/2025 **Status:** ✅ Merged **Merged:** 2/13/2025 **Merged by:** [@divergentdave](https://github.com/divergentdave) **Base:** `main` ← **Head:** `david/nsec3-covers-fix` --- ### 📝 Commits (3) - [`1982b83`](https://github.com/hickory-dns/hickory-dns/commit/1982b830ea3ac8ce9f7de524a45c5960b0e4bad4) Store base32-encoded next hashed name in NSEC3 - [`3024269`](https://github.com/hickory-dns/hickory-dns/commit/30242695a42d18c5305f46b89c7f5a28bc2c5ce0) Replace Nsec3Base32Hash with Label - [`a75bf49`](https://github.com/hickory-dns/hickory-dns/commit/a75bf490ab8d185f6251a36992234c6c2c7736a7) NSEC3 validation: Fix find_covering_record() ### 📊 Changes **2 files changed** (+106 additions, -86 deletions) <details> <summary>View changed files</summary> 📝 `crates/proto/src/dnssec/dnssec_dns_handle/nsec3_validation.rs` (+45 -83) 📝 `crates/proto/src/dnssec/rdata/nsec3.rs` (+61 -3) </details> ### 📄 Description The `find_covering_record()` helper function in the NSEC3 validation code is incorrect because it always returns something when given a nonempty set of NSEC3 records. This PR removes the fallback in `or_else()` and instead adds a separate case to handle the last NSEC3 record in a zone. RFC 5155 says "The value of the Next Hashed Owner Name field in the last NSEC3 RR in the zone is the same as the hashed owner name of the first NSEC3 RR in the zone in hash order", so we need to test that the target hashed owner name is in the wrap-around range between the two hashed names represented by the record. To detect which case the NSEC3 record falls in, we need to compare a base32-encoded hashed owner name from the record's name and the next hashed owner name from the RDATA, which is a raw digest. Thus, I added a field to the `NSEC3` struct that stores the base32 encoding of the next hashed owner name, so it can be compared directly. I also replaced the recently introduced `Nsec3Base32Hash` struct, used for case-insensitive comparisons, with `Label`, which has the same comparison semantics and is already used elsewhere. I haven't written a test case for this yet, I might try to cobble together one from the zone signing utilities and a dnslib server. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>