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

Fix TXT record equality #374

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Fix TXT record equality #374

merged 1 commit into from
Aug 20, 2024

Conversation

dklbreitling
Copy link
Contributor

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
Copy link
Member

partim commented Aug 14, 2024

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
Copy link

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
Copy link
Member

partim commented Aug 14, 2024

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
Copy link

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
Copy link
Member

partim commented Aug 14, 2024

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
Copy link
Contributor Author

@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.

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 uses the underlying octets for comparison.
@partim
Copy link
Member

partim commented Aug 20, 2024

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

@partim partim merged commit 95096cb into NLnetLabs:main Aug 20, 2024
13 checks passed
@janik-cloudflare
Copy link

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

@dklbreitling
Copy link
Contributor Author

dklbreitling commented Aug 20, 2024

Thanks for the quick merge!

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