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

When serializing with sorted=True, respect DNSSEC order within RRsets. #114

Closed
wants to merge 2 commits into from

Conversation

tow
Copy link

@tow tow commented Jun 26, 2015

Previously, sorting only applied to Names, but records within each node
were serialized in whatever order they happened to have been stored.

https://tools.ietf.org/html/rfc4034#section-6.3

tow added 2 commits June 26, 2015 15:24
Previously, sorting only applied to Names, but records within each node
were serialized in whatever order they happened to have been stored.

https://tools.ietf.org/html/rfc4034#section-6.3
@rthalley
Copy link
Owner

What value is there in having them serialized in some particular order? Are you trying to compare serialized blobs or something? That's not going to work due to domain name case issues.

@tow
Copy link
Author

tow commented Jul 14, 2015

For me, it's got the same value as having sorted=True work for Names in the way it does already.

The RFC4034 canonical ordering algorithm (required for DNSSEC) is basically 3 rules.
6.1 RRSets should be in the right order
6.2 names within Records should be canonicalized (mostly by case-folding)
6.3 Records within RRsets should be in the right order

Previously, the "sorted" argument only respected rule 6.1. This patch extends it so it also respects rule 6.3. You're right that canonicalization is still not complete unless rule 6.2 (and its 5 constituent subrules) is also implemented.

However, it seems worth taking the partial step towards further canonicalization, especially since the sorted argument has already taken the first step. 6.2 is substantially more complex to implement which is why I haven't done it yet.

However, this patch at least moves in the right direction. And for my particular use case, my source of data for DNS zone records is already "canonical enough" according to 6.2, so being able to get dnspython to finish the canonicalization is very helpful.

@tow
Copy link
Author

tow commented Aug 13, 2015

Hi - is there anything else I could do to move forward on this patch? Happy to do additional work if necessary, this is really key functionality for me & I'd like to be able to rely on stock dnspython for it.

@rthalley
Copy link
Owner

I'm ambivalent about sorting, but opposed to canonicalization. The DNSSEC canonical form's purpose is to enable consistent signature generation and validation, but you're not meant to alter the data by putting it into canonical form. For better or worse, the DNS is supposed to respect case.

@tow
Copy link
Author

tow commented Sep 1, 2015

Thanks for your response, I understand your concerns, but I don't think this patch should cause any issues.

I'm mainly referencing DNSSEC in this instance because the code itself already references it:

From dns/zone.py:464
" @param sorted: if True, the file will be written with the
names sorted in DNSSEC order from least to greatest. Otherwise
the names will be written in whatever order they happen to have
in the zone's dictionary.

but actually, from my perspective, I don't care about DNSSEC or canonicalization of names either way.

What I am really interested in is being able to guarantee a stable sorting of records within a DNS node when output as text. At the moment this is not possible, because there are no guarantees about the order in which records inside each rdataset will be emitted.

There is currently an option to order the names in a zone - however this is not actually useful to me, because whenever a name has multiple records attached, those records are still emitted in an arbitrary order.

My patch simply provides an option to order the records within a name. It doesn't do anything in terms of actually changing the contents of records at all.

If the mention of DNSSEC is muddying the waters, I'm happy to adjust the comments in my patch appropriately (and I could also propose a patch for the DNSSEC mention in zone.py if that helps?)

@rthalley rthalley closed this Sep 20, 2016
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