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

Skip EDNS Cookies in the packet cache #8993

Merged
merged 3 commits into from Sep 17, 2020

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Apr 2, 2020

Short description

This PR changes how our packet caches operate with regard to EDNS Cookies:

  • the authoritative server's one will skip the content of EDNS cookies, if any, potentially serving an answer from the packet cache for a query with a different cookie than the one that caused the answer to be generated. This is fine since the authoritative server doesn't include EDNS cookies in its responses. EDNS Client Subnet values are still hashed given that they might be used to provide the answer ;
  • the dnsdist's one will skip the content of EDNS cookies as well by default, but this behaviour can be changed by passing cookieHashing=true to the newPacketCache() function. Doing so is needed if at least one of the backend return responses containing EDNS cookies. EDNS Client Subnet values are still hashed ;
  • the recursor's one will skip the content of EDNS cookies and EDNS Client Subnet.

This PR could use a serious review (or several :)) and should not be merged lightly!

Fixes #5131.

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)

pdns/ednsoptions.cc Outdated Show resolved Hide resolved
pdns/packetcache.hh Outdated Show resolved Hide resolved
pdns/packetcache.hh Outdated Show resolved Hide resolved

if (!skip) {
/* hash the option code, length and content */
currentHash = burtle(reinterpret_cast<const unsigned char*>(&packet.at(pos)), 4 + optionLen, currentHash);
Copy link
Member

@omoerbeek omoerbeek Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An option having the same opcode but different length does not hash the same.
If options are variable length, this would miss some cache hits. But I guess that's ok, since the equality test also does considers them inequal. Actually, do variable length options exist?

They do exist, e.g. subnet IPv4 vs ECS IPv6, but those should never compare equal. So this is all good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was intended for options with the same opcode but different lengths to compare differently, as the content simply can't be the same. That might change, however, if/when we decide to skip EDNS padding in queries. I guess we would need to special case the EDNS padding option to skip the size of the content.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would make sense to skip the length of all options right now. It would make sense for EDNS padding and would also mean that we would fully ignore the content of EDNS Client Subnet options, including the size of the source, which seems like a win when we really do want to ignore ECS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might actually be an issue for ECS source zero 1, which is a way for the client to ask that no EDNS value be sent to authoritative servers. So we don't want to ignore that one, I guess..

@rgacogne
Copy link
Member Author

rgacogne commented Apr 14, 2020

Pushed a few changes:

  • Making conversions to uint16_t explicit, as suggested by Otto ;
  • Skipping EDNS padding in addition to EDNS cookies in the auth and rec ;
  • Skipping EDNS padding, still optionally skipping EDNS cookies in dnsdist ;
  • Skipping the size of the rdata length for the OPT record, as well as the size of some padding options (padding, cookies). The size of EDNS Client Subnet options is not ignored as we don't want to confuse an EDNS source zero with a different one.

I had to rebase but the first two commits are unchanged (up to and excluding "Make conversion to uint16_t explicit, as suggested by Otto").

@jsoref
Copy link
Contributor

jsoref commented May 17, 2020

Hi, for the time being (and hopefully going forward), pdns master is running the check-spelling action. In order to avoid a when merged jsoref@03669c0#commitcomment-39229219, please see jsoref@03669c0#commitcomment-39229308 -- you'll need to rebase onto master to get the check (and the files).

@rgacogne
Copy link
Member Author

rgacogne commented Jul 7, 2020

Rebased to fix conflicts.

@rgacogne
Copy link
Member Author

Rebased to fix a conflict.

@rgacogne
Copy link
Member Author

I'll try to do more performance testing of this PR then the numbers look good I'd like to get it merged, since it might have a very nice impact on our packet cache hit ratio.

@rgacogne
Copy link
Member Author

I've done a fair number of tests, and for all products there does not seem to be any measurable slowdown when there is no EDNS options present (or no EDNS at all). As expected there is a small slowdown when EDNS options are present (it doesn't seem noticeable in the rec when only one option is present, since we were already looking for ECS), I measured around 7% when three EDNS options are present. On dnsdist this is completely eclipsed by the gains from #9420.
In my opinion this slowdown is worth it based on the much better cache hit ratio that skipping Cookies (and perhaps padding later?) would get us for these queries :-)

Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a review back in April and did a quick read again today. After a rebase it should be good.

Detected by OSS-Fuzz. Also make sure that we don't try to parse
packets smaller than 12 bytes in the fuzzing target, those are
usually dropped earlier.
@rgacogne
Copy link
Member Author

rgacogne commented Sep 1, 2020

Thanks Otto! I guess I'll merge this PR once an "auth-4.4.x" branch has been created, then :-)

@rgacogne rgacogne added this to the dnsdist-1.6.0 milestone Sep 1, 2020
@Habbie
Copy link
Member

Habbie commented Sep 7, 2020

the authoritative server's one will skip the content of EDNS cookies, if any, potentially serving an answer from the packet cache for a query with a different cookie than the one that caused the answer to be generated. This is fine since the authoritative server doesn't include EDNS cookies in its responses. EDNS Client Subnet values are still hashed given that they might be used to provide the answer ;

I am fine with this for auth 4.4.0. I did not review the code, though.

@rgacogne rgacogne merged commit ee179a1 into PowerDNS:master Sep 17, 2020
24 checks passed
@rgacogne rgacogne deleted the packetcache-cookies branch September 17, 2020 12:22
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.

dnsdist, auth, rec: EDNS COOKIE option busts packet cache?
4 participants