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.

Sign up for GitHub

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

Jump to bottom

net: use EDNS to increase DNS packet size #51153

Closed
ianlancetaylor opened this issue Feb 11, 2022 · 24 comments
Closed

net: use EDNS to increase DNS packet size #51153

ianlancetaylor opened this issue Feb 11, 2022 · 24 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.19

Comments

@ianlancetaylor
Copy link
Member

Issue #51127 lists a bunch of cases where the Go DNS resolver has trouble because it follows RFC 1035 and only accepts 512 byte packets. https://go.dev/cl/385035 uses EDNS(0) to advertise that the resolver accepts a larger packet size, and accepts packets up that increased size (1232 bytes per https://dnsflagday.net/2020/). The CL is reported to fix the immediate problem found in #51127, that the Go resolver fails on WSL: #51127 (comment).

I would like to request a freeze exception to include https://go.dev/cl/385035 in the 1.18 release. If acceped for 1.18 I'll also open backport issues to earlier releases.

The CL should be safe. Accepting a larger packet size does not cause harm. EDNS(0) was designed to be backward compatible with DNS servers that don't understand it; per RFC 1035, released in 1987, they should just ignore it. EDNS(0) was first specified in RFC 2671 in 1999, and restated and refined in RFC 6891 in 2013. It is unlikely that there any DNS servers out there that can't at least ignore it.

CC @golang/release
@ianlancetaylor ianlancetaylor mentioned this issue Feb 11, 2022
Closed
@ianlancetaylor
Copy link
Member Author

CC @mdempsky

@toothrot
Copy link
Contributor

Although this is late in cycle, this is a small, focused change that we intend to backport regardless, which makes sense to accept.

We've discussed a bit and are supportive of merging this before the RC.

@toothrot toothrot added this to the Go1.18 milestone Feb 11, 2022
@toothrot toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 11, 2022
@mdempsky
Copy link
Contributor

I agree that compliant DNS servers shouldn't have any issues with the use of EDNS. But the rationale for this CL is to accommodate non-compliant servers that send invalid responses, so it seems like we should also be mindful about non-compliant servers/firewalls that break EDNS.

Will there be more release candidates before Go 1.18? If so, I think including in Go 1.18 seems reasonable.

I'm not sure why this is important to backport though?

@toothrot
Copy link
Contributor

Backporting is justified to me by the comments in "Miscellany" in #51127. This tooling is critical for some users, and forcing them to upgrade to (as of yet unreleased 1.18) isn't a great workaround. I assume this impacts the Go command as well.

@seankhliao
Copy link
Member

The go command almost always links to libc, which means using the platform C resolver by default and not the netgo one.

@mateusz834
Copy link
Member

@seankhliao

The go command almost always links to libc, which means using the platform C resolver by default and not the netgo one.

When compiled with libc, the net package decides at runtime whether to use cgo or go resolver

@AaronFriel AaronFriel mentioned this issue Feb 11, 2022
Closed
2 tasks
@ianlancetaylor
Copy link
Member Author

@gopherbot Please open backport issues.

This was referenced Feb 12, 2022
Closed
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #51161 (for 1.16), #51162 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@fweimer-rh
Copy link

@ianlancetaylor This will cause new failures because FORMERR/NOTIMPL fallback appears to be missing from the implementation. It may be necessary to implement timeout handling as well (retrying without EDNS0 in case of a timeout).

@odeke-em
Copy link
Member

@ianlancetaylor your CL https://go-review.googlesource.com/c/go/+/385035 landed 10 hours ago to the master branch. Shall we thus close this issue?

@ianlancetaylor
Copy link
Member Author

@fweimer-rh Do you think that we need to worry about those possibilities in practice? I don't see anything in glibc, although it's true that that case is different because the user must explicitly opt-in to EDNS in the resolv.conf file.

If those are real possibilities then I will revert the CL. It's too late in the release cycle to add that kind of complexity.

Thanks.

@fweimer-rh
Copy link

@ianlancetaylor I expect some problems. How widespread they are I don't know. I'm not aware of any stub resolver implementations that use EDNS0 by default and without any fallback. Go's implementation might be the first one with wide deployment. This change seems rather risky to me at this point.

@AaronFriel
Copy link

AaronFriel commented Feb 12, 2022 •
edited
Loading

An alternative would be to merge #51128 or a variant thereof, which does not advertise EDSN0 support in the request but accepts a larger (safe) buffer size by default.

This is akin to what glibc does, though reading C and executing it are often two different things, it looks to me like glibc may behave better here solely because it allocates a larger buffer, without advertising EDSN0 support.

A common cause of cannot unmarshal DNS... errors were that the users were on a VPN such as AnyConnect, which likely just had a DNS proxy that didn't implement message compression, so compressed upstream sub-512 byte responses would be expanded to >512 byte responses. That was also the case for Consul & Mesos DNS servers.

Allocating a ~1200 byte buffer by default, or perhaps 1024 byte is all that's needed, would mitigate this issue without additional protocol implications. Glibc defaults to 1024 bytes, I think?

https://github.com/bminor/glibc/blob/b92a49359f33a461db080a33940d73f47c756126/resolv/nss_dns/dns-network.c#L128

@fweimer-rh
Copy link

@AaronFriel The initial buffer sizes vary on the NSS functions used (1024 and 2048, see resolv/nss_dns/dns-hosts.c). There is also the NSS-based ERANGE retry loop, but I'm not sure if this only applies after TCP fallback. (There is dynamic buffer allocation in resolv/res_send.c, but that's only for the second response in a dual/parallel AF_UNSPEC lookup.) However, I wouldn't recommend the glibc implementation as a model for a stub resolver.

@AaronFriel
Copy link

Fair, I also wouldn't use glibc as a reference. I'm merely offering a compromise to maximize compatibility with real world DNS resolvers without advertising full EDNS0 support.

@ianlancetaylor
Copy link
Member Author

After discussion, we currently plan to do this:

  • roll back CL 385035
  • add a change to accept DNS packets up to 1232 bytes, to help WSL
  • backport that DNS packet change to 1.16 and 1.17
  • reinstall CL 385035 after the 1.18 release, so that it has some testing for 1.19

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/386015 mentions this issue: net: increase maximum accepted DNS packet to 1232 bytes

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/386016 mentions this issue: net: send EDNS(0) packet length in DNS query

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/386014 mentions this issue: Revert "net: send EDNS(0) packet length in DNS query"

gopherbot pushed a commit that referenced this issue Feb 15, 2022
@ianlancetaylor
Revert "net: send EDNS(0) packet length in DNS query"
d3c9ef5 
This reverts https://go.dev/cl/385035. For 1.18 we will use a simple
change to increase the accepted DNS packet size, to handle what appear
to be broken resolvers that don't honor the 512 byte limit. For 1.19
we will restore CL 385035 to make a proper EDNS request, so that it
has more testing time before it goes out in a release.

For #6464
For #21160
For #44135
For #51127
For #51153

Change-Id: Ie4a0eb85ca0a6a73bee5cd4cfc6b7d2a15ef259f
Reviewed-on: https://go-review.googlesource.com/c/go/+/386014
Trust: Ian Lance Taylor <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/386034 mentions this issue: [release-branch.go1.16] net: increase maximum accepted DNS packet to 1232 bytes

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/386035 mentions this issue: [release-branch.go1.17] net: increase maximum accepted DNS packet to 1232 bytes

@ianlancetaylor ianlancetaylor changed the title net: use EDNS to increase DNS packet size [freeze exception] net: use EDNS to increase DNS packet size Feb 15, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Go1.18, Go1.19 Feb 15, 2022
@ianlancetaylor
Copy link
Member Author

OK, changes made. Moving this issue to 1.19 to use EDNS.

gopherbot pushed a commit that referenced this issue Feb 15, 2022
@ianlancetaylor
net: increase maximum accepted DNS packet to 1232 bytes
6e82ff8 
The existing value of 512 bytes as is specified by RFC 1035.
However, the WSL resolver reportedly sends larger packets without
setting the truncation bit, which breaks using the Go resolver.
For 1.18 and backports, just increase the accepted packet size.
This is what GNU glibc does (they use 65536 bytes).

For 1.19 we plan to use EDNS to set the accepted packet size.
That will give us more time to test whether that causes any problems.

No test because I'm not sure how to write one and it wouldn't really
be useful anyhow.

Fixes #6464
Fixes #21160
Fixes #44135
Fixes #51127
For #51153

Change-Id: I0243f274a06e010ebb714e138a65386086aecf17
Reviewed-on: https://go-review.googlesource.com/c/go/+/386015
Trust: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@ianlancetaylor ianlancetaylor added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Feb 15, 2022
gopherbot pushed a commit that referenced this issue Feb 17, 2022
@ianlancetaylor @dmitshur
[release-branch.go1.17] net: increase maximum accepted DNS packet to …
7d70adf 
…1232 bytes

The existing value of 512 bytes as is specified by RFC 1035.
However, the WSL resolver reportedly sends larger packets without
setting the truncation bit, which breaks using the Go resolver.
For 1.18 and backports, just increase the accepted packet size.
This is what GNU glibc does (they use 65536 bytes).

For 1.19 we plan to use EDNS to set the accepted packet size.
That will give us more time to test whether that causes any problems.

No test because I'm not sure how to write one and it wouldn't really
be useful anyhow.

For #6464
For #21160
For #44135
For #51127
For #51153
Fixes #51162

Change-Id: I0243f274a06e010ebb714e138a65386086aecf17
Reviewed-on: https://go-review.googlesource.com/c/go/+/386015
Trust: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
(cherry picked from commit 6e82ff8)
Reviewed-on: https://go-review.googlesource.com/c/go/+/386035
Reviewed-by: Dmitri Shuralyov <[email protected]>
gopherbot pushed a commit that referenced this issue Feb 17, 2022
@ianlancetaylor @dmitshur
[release-branch.go1.16] net: increase maximum accepted DNS packet to …
288ff40 
…1232 bytes

The existing value of 512 bytes as is specified by RFC 1035.
However, the WSL resolver reportedly sends larger packets without
setting the truncation bit, which breaks using the Go resolver.
For 1.18 and backports, just increase the accepted packet size.
This is what GNU glibc does (they use 65536 bytes).

For 1.19 we plan to use EDNS to set the accepted packet size.
That will give us more time to test whether that causes any problems.

No test because I'm not sure how to write one and it wouldn't really
be useful anyhow.

For #6464
For #21160
For #44135
For #51127
For #51153
Fixes #51161

Change-Id: I0243f274a06e010ebb714e138a65386086aecf17
Reviewed-on: https://go-review.googlesource.com/c/go/+/386015
Trust: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
(cherry picked from commit 6e82ff8)
Reviewed-on: https://go-review.googlesource.com/c/go/+/386034
Reviewed-by: Dmitri Shuralyov <[email protected]>
@AaronFriel AaronFriel mentioned this issue Feb 28, 2022
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/389634 mentions this issue: doc/go1.19: mention use of EDNS(0)

gopherbot pushed a commit that referenced this issue Mar 4, 2022
@ianlancetaylor
doc/go1.19: mention use of EDNS(0)
797e889 
For #51153

Change-Id: I4374c63498b62ba7a08f146eebd034cbd50623f6
Reviewed-on: https://go-review.googlesource.com/c/go/+/389634
Trust: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Emmanuel Odeke <[email protected]>
@bradfitz bradfitz mentioned this issue Mar 8, 2022
Merged
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/415579 mentions this issue: net: ignore edns0 option in resolv.conf

gopherbot pushed a commit that referenced this issue Aug 14, 2022
@ianlancetaylor @gopherbot
net: ignore edns0 option in resolv.conf
7acbbbe 
We use EDNS(0) by default. No need to fall back to netdns=cgo if we
see a explicit request for EDNS(0) in resolv.conf.

For #51153

Change-Id: I135363112e3de43ce877aad45aba71d1448068b7
Reviewed-on: https://go-review.googlesource.com/c/go/+/415579
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
@apparentlymart apparentlymart mentioned this issue Oct 5, 2022
Closed
@golang golang locked and limited conversation to collaborators Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
9 participants
@toothrot @mdempsky @AaronFriel @ianlancetaylor @odeke-em @gopherbot @seankhliao @mateusz834 @fweimer-rh