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

x/net/dns/dnsmessage: cannot parse mDNS SRV records #24870

Open
mpx opened this issue Apr 15, 2018 · 5 comments
Open

x/net/dns/dnsmessage: cannot parse mDNS SRV records #24870

mpx opened this issue Apr 15, 2018 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mpx
Copy link
Contributor

mpx commented Apr 15, 2018

DNS message compression was disabled SRV Target fields in #10622 / https://golang.org/cl/100055 (as per RFC 2782).

However, compression is explicitly allowed for the MDNS SRV target field (RFC 6762 Sec 18.14):

Unicast DNS does not allow name compression for the target host in an SRV record, [...] all Multicast DNS implementations are REQUIRED to decode compressed SRV records correctly.

Attempting to decode a Chromecast MDNS SRV record with dnsmessage.Message.Unpack now fails with the error:

unpacking Additional: SRV record: Target: compressed name in SRV resource data

Compression support for DNS SRV target fields is necessary to support MDNS. Please consider:

  • adding an option to allow compression/decompression, or
  • reverting the earlier change.

Cc @mdempsky @iangudger

@gopherbot gopherbot added this to the Unreleased milestone Apr 15, 2018
@iangudger
Copy link
Contributor

For the incremental Parser/Builder, we can add additional methods to handle the MDNS SRV fields which I guess the user can use depending on which type of SRV record they expect. Does that sound ok?

I am not sure what we can do for the more automated Pack/Unpack interface. I guess we revert the change just to Unpack to maximize correctness and add a comment. We could also explicitly only allow parsing MDNS SRV records with the incremental interface. Thoughts?

@mpx
Copy link
Contributor Author

mpx commented Apr 16, 2018

Just thinking through the options out loud..

Unicast DNS

  • must not compress the target field
  • must not? should? decompress the target field (might be an indication of corruption, might provide the correct result)

Multicast DNS

  • should compress the target field
  • must decompress the target field

Hence, Message.Pack can't properly support both Unicast/Multicast. It is better to not compress the target field to meet unicast DNS requirement. Also, not compressing the target field is compatible with multicast DNS.

For Message.Unpack, the caller probably won't be able to determine whether result is corrupt or not for unicast DNS. I guess it is probably better to flag the error? This means Unpack can't be used for multicast, but it shouldn't be used for packing multicast either.

I'd recommend:

  • Documenting that Message Pack/Unpack do not compress the SRV target field and are incompatible with multicast DNS.
  • Change Parser/Builder to optionally support compressing/decompressing the target field and allow callers to make the decision (Eg, for multicast DNS, or if they know decompressing unicast DNS is ok).

@mikioh mikioh changed the title x/net/dns/dnsmessage: cannot parse MDNS SRV records x/net/dns/dnsmessage: cannot parse mDNS SRV records Apr 16, 2018
@iangudger
Copy link
Contributor

After looking a bit more at the RFC, I have another idea. What if we add new MulticastPack and MulticastUnpack methods to Message? It appears that the message is either multicast or unicast and that the user should know which one it is.

@mikioh
Copy link
Contributor

mikioh commented Apr 18, 2018

As described in RFC 8222, there's no good way to operate unicast DNS and multicast DNS seamlessly from DNS message handling to DNS label handling. Probably it's better to provide separated APIs for now.

@mateusz834
Copy link
Member

Also RFC 3597 permits use of DNS compression in SRV:

Receiving servers MUST decompress domain names in RRs of well-known
type, and SHOULD also decompress RRs of type RP, AFSDB, RT, SIG, PX,
NXT, NAPTR, and SRV (although the current specification of the SRV RR
in [RFC2782] prohibits compression, [RFC2052] mandated it, and some
servers following that earlier specification are still in use).

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants