dnsproxy icon indicating copy to clipboard operation
dnsproxy copied to clipboard

OPT PSEUDOSECTION: EDNS udp buffer always 4096

Open saint-lascivious opened this issue 3 years ago • 4 comments

Example query sample directed at 8.8.8.8:

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512

Example query sample directed at a local dnsproxy instance:

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096

I had expected, perhaps incorrectly, that this was governed by --udp-buf-size=, but in testing it doesn't appear to be the case (chasing --udp-buf-size= around the code base it wasn't actually clear to me where/if this value is/was used at all, but that's very likely my fault).

Possible bug, probable misunderstanding and/or PEBCAK.

saint-lascivious avatar Sep 23 '22 04:09 saint-lascivious

If I recall it correctly we just set it to 4kb all the time. Given that there's an option to configure udp buffer size your suggestion sounds very reasonable, I have no idea why we haven't implemented it that way from the beginning🤷‍♂️

ameshkov avatar Sep 25 '22 05:09 ameshkov

Thank you for your time.

Long time listener, first time caller, as it were. I'm a big fan of the project.

I'm not yet anywhere near as familiar with Go as I'd like to be, and as a byproduct of that couldn't be sure I wasn't missing something.

I couldn't figure out where the value was coming from and honestly I'm still not entirely sure. I ended up just searching for 4096 within the repo, and I found what I thought the culprit was, ~~but changing it and recompiling still yielded the 4096 return value~~ so I basically just put my hands up at that point and admitted defeat and deferred to the experts.

Edited to add:

  • Recompiling with a different hardcoded value (1232) did in fact work as expected.
  • My testing was hampered by a "fun", and apparently very long-standing and widespread bug with dig/bind which sets the EDNS udp buffer size to 4096 if +bufsize=0 is set as a default, which seems to be the case/vary depending on binary version and/or distribution.

Findings:

  • dig isn't necessarily an adequate debugging tool.
  • Having this value be configurable in dnsproxy would be awesome.
  • I'm still not sure what --udb-buf-size does.

saint-lascivious avatar Sep 25 '22 07:09 saint-lascivious

#310

Potterli20 avatar Jan 29 '23 15:01 Potterli20

I just ended up hard-coding the value at 1232 in my builds (which it looks like potter may have also done in the linked duplicate).

There doesn't appear to be any good reason to have it set at 4096, whether the edns reassembly buffer is made configurable or not.

Honestly I'm still uncertain as to whether --udp-buf-size even applies to the broadcast edns reassembly buffer as I thought it would.

The user could set it higher than 1232 if desired (assuming it were made to be configurable), or lower if desired (some more aggressive proxies/recursors opt for 512), but running the 2020 DNS Flag Day recommendation as the default value seems sane either way.

To wit, the following is attached:

diff --git a/proxy/cache_test.go b/proxy/cache_test.go
index d21204a..022880b 100644
--- a/proxy/cache_test.go
+++ b/proxy/cache_test.go
@@ -163,7 +163,7 @@ func TestCacheDO(t *testing.T) {
                },
                Answer: []dns.RR{newRR(t, "google.com. 3600 IN A 8.8.8.8")},
        }).SetQuestion("google.com.", dns.TypeA)
-       reply.SetEdns0(4096, true)
+       reply.SetEdns0(1232, true)

        // Store in cache.
        testCache.set(reply, upstreamWithAddr)
@@ -184,7 +184,7 @@ func TestCacheDO(t *testing.T) {
                        request = reqClone
                })

-               request.SetEdns0(4096, true)
+               request.SetEdns0(1232, true)

                ci, expired, key := testCache.get(request)
                assert.False(t, expired)
diff --git a/proxy/helpers.go b/proxy/helpers.go
index 273e178..1d4959c 100644
--- a/proxy/helpers.go
+++ b/proxy/helpers.go
@@ -150,7 +150,7 @@ func setECS(m *dns.Msg, ip net.IP, scope uint8) (subnet *net.IPNet) {
                },
                Option: []dns.EDNS0{e},
        }
-       o.SetUDPSize(4096)
+       o.SetUDPSize(1232)
        m.Extra = append(m.Extra, o)

        return subnet

It is important for DNS software vendors to comply with DNS standards, and to use a default EDNS buffer size (1232 bytes) that will not cause fragmentation on typical network links.

saint-lascivious avatar Feb 02 '23 11:02 saint-lascivious