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

dnsdist: Keep accepting fragmented UDP datagrams on DNSCrypt binds #8974

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

rgacogne
Copy link
Member

Short description

DNSCrypt pads its queries for privacy purposes, and thus requires larger queries than plain DNS ones. Discarding fragmented datagrams doesn't make sense in that case, and actually leads to a very degraded service, as reported in #7410 (comment).
At the moment I don't see any reason to accept fragmented datagrams for plain DNS queries, so this MR only applies to DNSCrypt binds for now.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

DNSCrypt pads its queries for privacy purposes, and thus requires
larger queries than plain DNS ones. Discarding fragmented datagrams
doesn't make sense in that case, and actually leads to a very
degraded service.
@rgacogne rgacogne added this to the dnsdist-1.5.0 milestone Mar 26, 2020
@rgacogne rgacogne merged commit d93a215 into PowerDNS:master Mar 26, 2020
@rgacogne rgacogne deleted the ddist-pmtu-dnscrypt branch March 26, 2020 13:44
@jedisct1
Copy link

Hi @rgacogne ,

Packets larger than 1500 bytes are still rejected. Here is the root cause:

static const uint16_t s_udpIncomingBufferSize{1500}; // don't accept UDP queries larger than this value

Packets are rejected in isUDPQueryAcceptable().

@rgacogne
Copy link
Member Author

Hi Frank,

That time we are no longer talking about fragmented and reassembled packets, but about packets that are truncated because they are larger than the size of our incoming buffer, or actually than the size we are willing to accept since our actual buffer is a bit larger (to allow addition of EDNS options, handling of responses from the packet cache, ...). We could advertise a larger buffer when DNSCrypt is enabled but it might be a bit more complicated. I'll look into it.

@jedisct1
Copy link

Hi Remi,

I totally understand that can't just change that constant as it would have implications for non-DNSCrypt users :(

On the bright side, I tested dnsdist with an increased incoming buffer size, and this, in combination with your previous change, seems to have fixed all known issues. No more dropped queries.

@rgacogne
Copy link
Member Author

I have not tested it yet, but I believe something like this might be enough, it uses the full available size for DNSCrypt binds: rgacogne@0e214db

@jedisct1
Copy link

Just tested it, and it totally works!

@rgacogne
Copy link
Member Author

Thanks a lot! I just opened a new merge request with this fix, I'll merge it once our CI is green :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants