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

More EDNS(0) fixes. #403

Merged
merged 1 commit into from
Oct 7, 2024
Merged

More EDNS(0) fixes. #403

merged 1 commit into from
Oct 7, 2024

Conversation

ximon18
Copy link
Member

@ximon18 ximon18 commented Oct 3, 2024

Prior to this PR an AXFR of a large zone was causing errors in the log like so:

Cannot add RFC 7828 edns-tcp-keepalive option to response: buffer size exceeded

This PR contains the following EDNS(0) related fixes:

  • If TCP always reserve space for keep alive opt, not just if the request had one.
  • Remove the configured push limit before pushing an OPT into the response otherwise we hit the limit we put in place to prevent others using the space we need.
  • Add an empty OPT if the request had an OPT record and the response lacks one.

This PR doesn't yet include tests that verify the fixes but with these changes the ISC EDNS Compliance Tester (https://ednscomp.isc.org/) approves of the domain EdnsMiddlewareSvc behaviour (whereas prior to this PR it detected multiple failure cases), reporting:

example.com. @185.49.141.18 dns=ok zflag=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok

ok - test passed.

To retrieve this report in the future: https://ednscomp.isc.org/ednscomp/3ff0c43d4d

One issue with this PR is that the configured push limit is cleared entirely. That's also how the TsigMiddlewareSvc code works, but that is likely to be the last middleware in the chain before the server, while EdnsMiddlewareSvc will likely be further along the chain. This means that in principle middleware that post-processes a response after EdnsMiddlewareSvc but before TsigMiddlewareSvc could push more bytes into the response than should be allowed.

A fix for this could be a sort of push/pop sequence of response limits pushed by each middleware on pre-process and popped on post-process, but that is not part of this PR.

- If TCP always reserve space for keep alive opt, not just if the request had one.
- Remove the configured push limit before pushing an OPT into the response otherwise we hit the limit we put in place to prevent others using the space we need.
- Add an empty OPT if the request had an OPT record and the response lacks one.
@ximon18 ximon18 added the bug Something isn't working label Oct 3, 2024
@ximon18 ximon18 requested a review from a team October 3, 2024 22:38
@ximon18 ximon18 merged commit 2266779 into main Oct 7, 2024
26 checks passed
@ximon18 ximon18 deleted the more-edns-fixes branch October 7, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants