search

NLnetLabs / domain

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

Fix TXT record equality #374

Closed dklbreitling closed 3 weeks ago

dklbreitling commented 1 month ago

A TXT record with strings ["foo", "bar"] is not equal to a TXT record with strings ["foob", "ar"]. This is important for resolvers and DNS servers deduplicating RRs. This fixes PartialEq accordingly.

partim commented 1 month ago

Thank you for the PR!

I’m not sure this is a good idea, though, as this implementation means that TXT record data parsed as a Txt<_> and as UnknownRecordData<_> can compare differently. This will become relevant once we add a new record data enum that only treats record data types with possibly compressed domain names specially and keeps everything else as plain data – that new type and AllRecordData<_> would then potentially compare differently.

One option is to implement a wrapper type, say TxtContent<_>, that treats the content as a single string and implements the comparison traits accordingly. Would that work for your use cases?

(As an aside, if we change the PartialEq and Eq impls, we need to also change PartialOrd, Ord, and Hash accordingly.)

janik-cloudflare commented 1 month ago

Are you sure it would compare differently @partim? I'm not too familiar with this crate but if UnknownRecordData implements RFC 3597 then "ab" "cd" and "abc" "d" should also not be considered equal because they have different wire format representations. In general I don't think there's any RFC according to which "ab" "cd" would be equal to "abc" "d"; they are entirely different records for deduplication, DNSSEC and all other purposes that I'm aware of.

Good point about PartialOrd etc.!

partim commented 1 month ago

Erm, I think I’ve gotten this all backwards and you are actually implementing what I am arguing for. Sorry about that.

But wouldn’t it then be easier to just compare the underlying octets?

janik-cloudflare commented 1 month ago

Cool cool :) I think comparing the octets is a great idea in general. Looking at src/rdata/rfc1035/a.rs that just seems to #[derive(..., Eq, Hash, Ord, PartialEq, PartialOrd)], so perhaps we could do the same here? We'll take a stab at it. Thanks for the feedback @partim!

partim commented 1 month ago

Deriving will force trait bounds that will then break the macros that create the rdata enums. I think src/rdata/rfc1035/null.rs is a better example of what you need (ie., you should only require Octs: AsRef<[u8]> for all those traits and that, sadly, means you have to create manual impls).

dklbreitling commented 1 month ago

@partim thanks! We stole the impls from null.rs, as per your suggestion.

Also changed CanonicalOrd. Happy to change if this is not what you intended. Any feedback is welcome.

partim commented 3 weeks ago

Thank you again for the PR and the subsequent changes! This looks good to merge now.

janik-cloudflare commented 3 weeks ago

Great news, super exciting, thanks for the quick merge @partim!

dklbreitling commented 3 weeks ago

Thanks for the quick merge!