NLnetLabs / domain

A DNS library for Rust.
https://nlnetlabs.nl/projects/domain/about/
BSD 3-Clause "New" or "Revised" License
332 stars 56 forks source link

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

Closed achow101 closed 1 month ago

achow101 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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