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

Add RFC4034 domain comparison + NSEC Cover #1315

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

Conversation

monoidic
Copy link
Contributor

I noticed that this library has a good Cover() method for NSEC3 RRs, but not for NSEC. This PR adds a similar Cover() method for NSEC, plus a DomainCompare function for comparing domain names as defined by RFC4034's canonical ordering.

Feedback is welcome.

nsecx.go Outdated Show resolved Hide resolved
@monoidic
Copy link
Contributor Author

monoidic commented Nov 22, 2021

Whoops.

Thank you for your feedback! Indeed, the code could be more performant. The new implementation is similar to your implementation in CoreDNS, with some small differences.
I took the doDDD helper function from there as well. The helper functions it uses already exist in this repository under msg.go. dddToByte is only used there, though isDigit is also used in some other places. Perhaps those helper functions could be moved somewhere better.

we already have CompareDomainName, so this probably needs to be renamed, although I don't know into what, mayby Less - from the sort package?

I do not believe that Less would be a good fit. It implies that the function does a boolean < comparison rather than a three-way comparison, and it would make more sense as a method of some struct like dns.Name than as a stand-alone function. The three-way comparison can be reduced down to any two-way comparison by a wrapper function anyhow, and it doesn't make sense to me to make this function less useful.
If anything, just Compare would make more sense, given the examples of strings.Compare and bytes.Compare

Also the function signature needs to be different as well, so it can fit that pkg.

I'm not sure I understand. The less functions passed to sorting functions in the sort package take slice indices as arguments, so I don't think there's a way to create a comparison function here that you wouldn't have to provide a wrapper for.

I changed the name to CanonicalCompare for now. I believe it should describe the function well enough. Though if you, I, or perhaps somebody else, comes up with a better name, then I'm open to suggestions.

@monoidic monoidic requested a review from miekg November 22, 2021 18:54
@monoidic
Copy link
Contributor Author

Is there still something wrong with this PR?

@tmthrgd
Copy link
Collaborator

tmthrgd commented Dec 18, 2021

Is this comparison algorithm used for anything other than RFC4034 or NSEC? It's rather generically named, so perhaps something like RFC4034Compare might be better if not. Though it's probably fine if it is.

Also how necessary is it to be exported? Does it have any meaningful uses outside of Cover?

@monoidic
Copy link
Contributor Author

monoidic commented Dec 18, 2021

To my knowledge, NSEC is the only use for ordering records. Though as it is often referred to as canonical ordering and there are no other methods of ordering records by name beyond plain old string comparison, I think CanonicalCompare is more clear than something with the RFC number in its name.

Besides the Cover use case, the compare function could be used e.g in DNS servers/zone signing tools/etc to perform sorting of RRs for NSEC, so the implementation in CoreDNS linked by miekg could be replaced by calls to dns.CanonicalCompare instead.

@andrewtj
Copy link
Collaborator

Is this comparison algorithm used for anything other than RFC4034 or NSEC?

ZONEMD uses it too.

@monoidic
Copy link
Contributor Author

Is there still something for me to fix in this PR? NSEC Cover() was already (though broken) part of the API before being removed in 2017. Is adding RFC4034 comparison to the public API a deal-breaker?

@miekg
Copy link
Owner

miekg commented Jan 25, 2022 via email

@monoidic
Copy link
Contributor Author

CompareDomainName already exists, we can't have two compare function in this lib. So one
is wrong

It has the description:

CompareDomainName compares the names s1 and s2 and
returns how many labels they have in common starting from the *right*.

It seems to me that something like CountCommonLabels would be a more descriptive name for it than CompareDomainName. It could be renamed, with the old name being kept around as a deprecated wrapper function, then?

@miekg
Copy link
Owner

miekg commented Jan 25, 2022 via email

@miekg
Copy link
Owner

miekg commented May 10, 2022

OK, I think we can do the following, and just have two Compare* functions.

Rename to Compare and move it to labels.go and it should be possible to use the equal function there to make it work with non-lowercased strings?

We might also need to add some docs on when to choose which compare function, but that may be overkill.

WDYT?

@monoidic
Copy link
Contributor Author

[...] it should be possible to use the equal function there to make it work with non-lowercased strings?

So, comparing the labels, right to left, up until the rightmost non-matching labels with equal, then converting those non-matching labels to lowercase before using bytes.Compare, as opposed to assuming the inputs are lowercased and using bytes.Compare on all labels?

@monoidic
Copy link
Contributor Author

monoidic commented May 10, 2022

Whoops yet again...
I believe this should be what you described, code-wise. Also updated the tests slightly (the uppercased parts from the RFC4034 examples for Compare and the end of the range for NSEC Cover)

@monoidic
Copy link
Contributor Author

Just noticed that the dddToByte in this repo and the one in coredns are slightly different...

@monoidic
Copy link
Contributor Author

monoidic commented May 13, 2022

The last change might be controversial and I can revert it if you deem so fit.
It does a bit of extra work in Compare so that , for names x.example.com. and y.example.com., it compares x and y rather than x. and y., which is done by the code I mostly copied from CoreDNS.
The reason for this is so that sightly more exotic names, where the final leftmost compared label's final character's ASCII value is lesser than that of ., and the other label does not include said character, get treated properly, e.g name*.intra. vs name.intra. as a contrived example, because name < name*, but name. > name*.
If this is an acceptable edge case to handle, then I could open a similar PR for less in CoreDNS.

Or I suppose the less in CoreDNS could just be replaced entirely with dns.Compare

@miekg
Copy link
Owner

miekg commented May 13, 2022 via email

labels.go Outdated Show resolved Hide resolved
labels.go Outdated Show resolved Hide resolved
labels.go Outdated Show resolved Hide resolved
@miekg
Copy link
Owner

miekg commented May 18, 2022 via email

@monoidic
Copy link
Contributor Author

monoidic commented May 18, 2022

Thanks for doing that. But can you explain why "equal" isn't good enough? Is it broken? I prefer not to have two comparison functions in there, but happy to drop either.

On Wed, 18 May 2022, 08:20 monoidic, @.> wrote: @monoidic https://github.com/monoidic requested your review on: #1315 <#1315> Add RFC4034 domain comparison + NSEC Cover. — Reply to this email directly, view it on GitHub <#1315 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIW3QH7MOSY3W5TVZLQTVKSD3DANCNFSM5IOOWG3Q . You are receiving this because your review was requested.Message ID: @.>

equal only does equality (==) while labelCompare does a three-way comparison (<=>). They are almost the same, except for the way that equal short-circuits to false if the lengths are not equal and labelCompare returning two different "not equal" values.
I suppose there's two main options for how to proceed, depending on how desirable this short-circuit behavior is.

  • reduce equal to a wrapper for labelCompare that immediately returns false if the lengths don't match and otherwise does return labelCompare(a, b) == 0
  • replace any calls to equal(a, b) with labelCompare(a, b) == 0 and remove equal

@monoidic
Copy link
Contributor Author

monoidic commented Jun 2, 2022

@miekg
I've procceeded with the "make equal a wrapper for labelCompare" plan, do you see anything still wrong with this PR?

@andrewtj
Copy link
Collaborator

andrewtj commented Jun 2, 2022

From a quick glance equal and labelCompare don't appear to normalise escape sequences. (Eg: \097 and a, or \a, and uppercase variants thereof should be equal.) Should they, or is that out of scope?

@monoidic
Copy link
Contributor Author

monoidic commented Jun 3, 2022

From a quick glance equal and labelCompare don't appear to normalise escape sequences. (Eg: \097 and a, or \a, and uppercase variants thereof should be equal.) Should they, or is that out of scope?

\097 should be handled by doDDD in labelCompare and equal Case-insensitive comparisons are performed. Though \a would not be handled properly. The same seems to be true for CompareDomainName, however. I'll check on this.
Also, noticed an issue with doDDD...

The doDDD is in Compare, equal/labelCompare do not handle any normalization themselves and only perform case-insensitive comparisons.

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