Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't skip children of apex in RecordsIter if apex is not there #314

Merged
merged 1 commit into from
May 30, 2024

Conversation

achow101
Copy link
Contributor

@achow101 achow101 commented May 5, 2024

If none of the records in the RecordsIter have the apex as their name, but they are in the apex's zone, don't skip those children. Otherwise rrsigs will not be created for any queries for just subdomains.

@partim
Copy link
Member

partim commented May 6, 2024

Thank you for the PR!

The intention of the current code was to only sign a complete zone. In particular, the code relies on that to create a correct NSEC chain. In this case, you always have to have the apex itself because this is where the DNSKEY records go.

It looks like at least SortedRecords::sign will produce a sensible result, so I suppose adding this check – which totally makes sense semantically – is fine.

I think, however, it should be a separate check before the look rather than testing in every iteration.

@achow101
Copy link
Contributor Author

achow101 commented May 6, 2024

I think, however, it should be a separate check before the look rather than testing in every iteration.

I chose to put it in every iteration because theoretically there could be records that would be before the apex, and then records that are children of the apex, but not the apex itself. However I'm not sure if that's actually something reasonable to consider.

@partim
Copy link
Member

partim commented May 10, 2024

Ah yes, you are right – I forgot about that case. But then, shouldn’t just checking for ends_with be enough? And, because this is actually kind of expensive, perhaps add an outer loop that skips over records with the wrong class without checking?

@achow101 achow101 force-pushed the rrset-sign-children-without-apex branch from 5c59696 to 07db82c Compare May 10, 2024 15:22
@achow101
Copy link
Contributor Author

But then, shouldn’t just checking for ends_with be enough?

Yes. But I don't fully understand what the point of class is, and whether anyone actually uses something other than IN for it. I was basing this off of Family::is_in_zone which checks for class, so figured this should probably do that too.

And, because this is actually kind of expensive, perhaps add an outer loop that skips over records with the wrong class without checking?

I moved the class check to be done first at the top of the loop so it would just skip any records that don't have a class matching the apex.

@partim
Copy link
Member

partim commented May 30, 2024

Apologies for the delay! This looks good now – I will merge it.

@partim partim merged commit 42c0d44 into NLnetLabs:main May 30, 2024
12 checks passed
partim added a commit that referenced this pull request Jun 3, 2024
New

* Allow AllRecordData’s parsing impls to accept an unsized [u8] as the
  source octets. ([#310] by [@xofyarg])
* Made `sign::records::FamilyName` public. ([#312] by [@achow101])
* Added an impl of `FromStr` for `Question`. ([#317])

Bug fixes

* Accept an empty record type bitmap when scanning NSEC/NSEC3 data.
  ([#310] by [@xofyarg])
* Fix serialization of ProtoRrsig to conform with RFC 4034. ([#313 by
  [@achow101])
* Add `?Sized` bounds to `Message::is_answer` and `ParsedRecord::to_record`.
  ([#318] by [@xofyarg], [#325] by [@hunts])
* Bring back `MessageBuilder::as_target`. ([#318] by [@xofyarg])
* Bring back `impl FreezeBuilder for StaticCompressor`. ([#318] by [@xofyarg])
* `sign::records::RecordsIter::skip_before` now stops at the first name in
  zone even if the apex itself doesn’t appear. ([#314] by [@achow101])
* Fix a counting error in `SliceLabelsIter::next` that broke compression
  via `StaticCompressor`. ([#321] by [@hunts])

Unstable features

* New unstable feature `unstable-stelline` for the Stelline testing
  framework as a “normal” module of _domain._ ([#315])
* Renamed the domain name types in `zonetree` from `Dname` to `Name`.
  ([#308])

Other changes

* The minimum Rust version is now 1.78. ([#320])
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.

2 participants