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 frag retry if advertised udp payload size is not smaller #987

Merged
merged 1 commit into from Jan 5, 2024

Conversation

borisVanhoof
Copy link

Hi,

If a serviced query is in UDP_EDNS_FRAG mode, and EDNS_ADVERTISED_SIZE is 1232 (the default) or less, then the retry will have the same edns udp payload size with the same result.

This patch will, if the advertised udp payload size is not smaller, increase the retry count and continue until answer or no more targets to forward to.

I used this conf to test and the following firewall rule to drop responses and cause timeout net event.

$ cat example.conf 
server:
    username: ""
    chroot: ""
    directory: "/tmp"
    do-daemonize: no
    use-syslog: no
    module-config: "iterator"
    ip-freebind: yes
    interface: lo
    infra-keep-probing: yes

forward-zone:
    name: "."
    forward-addr: 8.8.8.8

$ sudo iptables-legacy -I INPUT -p udp -s 8.8.8.8 --sport 53 -j DROP

I have added pre and post patch pcap files in pcapfiles.tar.gz. The pre file has 9 (I was expecting 10) requests to 8.8.8.8 and the post file has the expected 5 requests before servfail.

Kind regards and a happy new year 2024!

If serviced query is in UDP_EDNS_FRAG mode, and EDNS_ADVERTISED_SIZE
is 1232 (the default) or more, then the retry will have the same edns
udp payload size with the same result.
Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

The code is neat and well written. The reuse of the function makes is easier to read. Moving to retry at a smaller size that then is going to be the same value as before, is not useful for resolving the query. So this code change looks good to have.

@wcawijngaards wcawijngaards merged commit 52a7658 into NLnetLabs:master Jan 5, 2024
1 check passed
@wcawijngaards
Copy link
Member

Thank you for the patch! That should avoid unnecessary retries, since the flag day changes made the default edns size drop from 4096 to 1232, the size is not smaller in that retry any more. The code has been merged into the code repository.

wcawijngaards added a commit that referenced this pull request Jan 5, 2024
- Merge #987: skip edns frag retry if advertised udp payload size is
  not smaller.
@borisVanhoof
Copy link
Author

Thank you.

jedisct1 added a commit to jedisct1/unbound that referenced this pull request Jan 7, 2024
* nlnet/master: (40 commits)
  - Fix unit test for NLnetLabs#987 change in udp1xxx retry packet send.
  Changelog note for NLnetLabs#987 - Merge NLnetLabs#987: skip edns frag retry if advertised udp payload size is   not smaller.
  skip edns frag retry if advertised udp payload size is not smaller
  - Remove unneeded newlines and improve indentation in remote control   code.
  - Fix NLnetLabs#983: Sha1 runtime insecure change was incomplete.
  Changelog note for NLnetLabs#985. - Merge NLnetLabs#985: Add DoH and DoT to dnstap message.
  Changelog note for NLnetLabs#979 and NLnetLabs#980. - Merge NLnetLabs#980: DoH: reject non-h2 early. To fix NLnetLabs#979: Improve errors   for non-HTTP/2 DoH clients.
  Add DoH and DoT to dnstap message
  - Update example.conf with cookie options.
  DoH: reject non-h2 early
  Fixup doc/Changelog.
  - Fix root_zonemd unit test, it checks that the root ZONEMD verifies,   now that the root has a valid ZONEMD.
  Changelog note for NLnetLabs#975 - Merge NLnetLabs#975: Fixed some syntax errors in rpl files.
  Fixed some syntax errors in rpl files.
  - Fix NLnetLabs#974: doc: default number of outgoing ports without libevent.
  - Use the origin (DNAME) TTL for syntesized CNAMEs as per RFC 6672.
  - Fix tests to use new common.sh functions, wait_logfile and   kill_from_pidfile.
  - Update test script file common.sh.
  - Updated IPv4 and IPv6 address for b.root-servers.net in root hints.
  - iana portlist update.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants