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

RFC 9660 The DNS Zone Version (ZONEVERSION) Option #398

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wtoorop
Copy link
Member

@wtoorop wtoorop commented Oct 26, 2024

No description provided.

@huguei
Copy link

huguei commented Oct 27, 2024

Thanks Willem! Just two comments:

  • line 1820 of query.c: in the spirit of future maintainability, would it not be better to define a constant, for example “ZONEVERSION_SOA-SERIAL" with value 0 and referring to https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#zoneversion-type-values , just in case new ones appear in the future?
  • in my patches on an older version of nsd, I added a “(RCODE(q->packet) == RCODE_OK)” condition to the option. At the moment I don't remember the reason for that test, but couldn't it be that we're returning zonerversion on some edge condition that shouldn't happen? Sorry, but I haven't compiled the code to do live compliance testing.

@wtoorop
Copy link
Member Author

wtoorop commented Oct 27, 2024

Thanks Willem! Just two comments:

Thanks for the prompt review @huguei

Good point! I'll add that shortly.

  • in my patches on an older version of nsd, I added a “(RCODE(q->packet) == RCODE_OK)” condition to the option. At the moment I don't remember the reason for that test, but couldn't it be that we're returning zonerversion on some edge condition that shouldn't happen?

But the last paragraph of Section 3.2 of RFC 9660, Responders states that ZONEVERSION should also be returned for NXDOMAIN and for SERVFAIL. So I reckoned that if nsd could find a zone for the question (i.e. the zone field in struct query is set) it can also return a ZONEVERSION for that zone regardless the kind of response. WDYT?

Sorry, but I haven't compiled the code to do live compliance testing.

I can do that for you and serve some zones on a droplet in digital ocean ;). Do you have a zone in mind (or several zones) for you to test?
I suppose we should capture those compliance tests in a test package, so you can review that script.

Currently only 1: The SOA-SERIAL type.
Copy link
Contributor

@k0ekk0ek k0ekk0ek left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Looks like good code. Support for the edns option could be useful to disambiguate query results for what zone and serial number of that zone it is from.

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