dnsdist: add support for incoming DoQ #13280

chbruyand · 2023-09-20T09:48:12Z

Short description

A few items on the todo list:

Closes #9897

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)

rgacogne · 2023-09-22T12:27:27Z

tests: fix checks with terminated streams with errors (dnspython doesn't notice and timeout)

This seems fixed with dnspython >= 2.4.0, see rthalley/dnspython#954

rgacogne · 2023-09-22T12:31:36Z

One issue is that libquiche.a comes with BoringSSL symbols, which of course conflict with some of the ones from OpenSSL that we use for DoT and DoH. We currently work-around that by using libquiche.so so that the BoringSSL symbols are only visible inside the libquiche.so object, so Quiche is happy while not polluting the remaining namespace. But it means that we cannot statically link against libquiche.a, like we were doing for libh2o when it was not packaged by the distribution. Our options are:

provide libquiche.so as part of the dnsdist package, which seems awfully wrong
generate a separate libquiche package on all distributions for which we want to support DoQ
beg the distributions we care about to package Quiche, which I'm not very optimistic about.
?

zeha · 2023-09-22T13:32:14Z

provide libquiche.so as part of the dnsdist package, which seems awfully wrong

libpdns-quiche.so - in a per-version private directory, so more like /usr/lib/x86_64-.../dnsdist-1.9.xyz/libdnsdist-quiche.so. This is going to be ugly, but maybe the only way of doing it?

rgacogne · 2023-09-22T13:37:34Z

Why the per-version private directory?

zeha · 2023-09-22T13:39:14Z

Why the per-version private directory?

Sorry, that's a thinko. Multiple versions of the same product are irrelevant.

Would still need one .so for each product.

rgacogne · 2023-09-22T13:41:06Z

Would still need one .so for each product.

Ah, yes, good one!

pdns/dnsdistdist/docs/reference/config.rst

+  * ``idleTimeout=5``: int - Set the idle timeout, in seconds.
+  * ``internalPipeBufferSize=0``: int - Set the size in bytes of the internal buffer of the pipes used internally to pass queries and responses between threads. Requires support for ``F_SETPIPE_SZ`` which is present in Linux since 2.6.35. The actual size might be rounded up to a multiple of a page size. 0 means that the OS default size is used. The default value is 0, except on Linux where it is 1048576 since 1.6.0.
+  * ``maxInFlight=0``: int - Maximum number of in-flight queries. The default is 0, which disables out-of-order processing.
+  * ``congestionControlAlgo="reno"``: str - The congestion control algorithm to be chosen between ``reno``, ``cubic`` and ``bbr``


rgacogne · 2023-09-25T10:12:05Z

I pushed two commits:

test asynchronous queries / response with the DoQ transport
optionally log TLS keying material to a file with DoQ

rgacogne · 2023-09-28T11:49:01Z

Would still need one .so for each product.

Ah, yes, good one!

For the record, this PR now builds Quiche for all supported distributions except el-7 (clang is too old for boring-sys there) and ships libdnsdist-quiche.so in our dnsdist packages.

omoerbeek

Basic local test looks good.
I'm seeing a few errors in the regression tests in debian bookworm:

FAILED test_Async.py::TestAsyncFFI::testAcceptThenDrop - doqclient.StreamRese...
FAILED test_Async.py::TestAsyncFFI::testDrop - doqclient.StreamResetError: St...
FAILED test_Async.py::TestAsyncLua::testAcceptThenDrop - doqclient.StreamRese...
FAILED test_Async.py::TestAsyncLua::testDrop - doqclient.StreamResetError: St...
FAILED test_RulesActions.py::TestAdvancedEDNSVersionRule::testBadVers - Asser...
======= 5 failed, 646 passed, 2 skipped, 7 warnings in 192.73s (0:03:12) =======

Also: the CI regression tests have quite some failures.

pdns/dnsdistdist/docs/reference/config.rst

omoerbeek · 2023-09-29T11:13:50Z

pdns/dnsdistdist/doq.cc

+    {
+      /* don't keep that pointer around, it will be invalidated if the buffer is ever resized */
+      // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
+      auto* dnsHeader = reinterpret_cast<struct dnsheader*>(unit->query.data());


We should watch out for unaligned access here

I have looked into this, and also other places where it might happen, which led to a pretty big commit: ca0346f I think we might want to make this a separate PR.

I pushed a new commit taking care of potential unaligned accesses in the DoQ code. I'll open a new PR for the existing code once we have merged this one.

omoerbeek · 2023-09-29T11:35:44Z

Embedded BoringSSL does not build on OpenBSD and the quiche install script has some Linuxism and fixed target locations. It's a pity quiche does not seem to be packaged on the platforms I looked at.

pdns/dnsdist-lua.cc

pdns/dnsdistdist/docs/guides/dns-over-quic.rst

rgacogne · 2023-09-29T12:04:25Z

the quiche install script has some Linuxism and fixed target locations.

Do you mean builder-support/helpers/install_quiche.sh? If so I'll fix the issues, just let me know what they are :)

omoerbeek · 2023-09-29T12:04:49Z

the quiche install script has some Linuxism and fixed target locations.

Do you mean builder-support/helpers/install_quiche.sh? If so I'll fix the issues, just let me know what they are :)

…about it

rgacogne · 2023-10-09T11:38:45Z

Rebased on master to fix a conflict.

coveralls · 2023-10-09T12:25:17Z

coverage: 56.415%. first build when pulling d5d9573 on chbruyand:dnsdist-doq-quiche into 1e7c7f1 on PowerDNS:master.

omoerbeek · 2023-10-13T10:41:54Z

There's an issue on macOS: while the link works and produces an executable, I get

....
CXXLD    dnsdist
ld: warning: -bind_at_load is deprecated on macOS
mbp:dnsdistdist otto$ ./dnsdist 
dyld[37051]: Library not loaded: /private/tmp/quiche-0.18.0/target/release/deps/libquiche.dylib
  Referenced from: <E692B5D9-8631-32A9-BDAF-BA2AD2993EC4> /Users/otto/pdns/pdns/dnsdistdist/dnsdist
  Reason: tried: '/private/tmp/quiche-0.18.0/target/release/deps/libquiche.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/private/tmp/quiche-0.18.0/target/release/deps/libquiche.dylib' (no such file), '/private/tmp/quiche-0.18.0/target/release/deps/libquiche.dylib' (no such file)
Abort trap: 6

when I try to run it. This is not a blocker for an alpha2 release as far as I am concerned.

It appears that after renaming/moving a library, you need to use install_name_tool:
x.diff.txt

With that, dnsdist starts up and my QUIC tests work!

omoerbeek

I tested this successfully on Debian bookworm and macOS and I read through the code (though not in great detail), saw two small nits in addition to my earlier comments that were already addressed. Great work!

pdns/dnsdistdist/doq.cc

Co-authored-by: Otto Moerbeek <[email protected]>

rgacogne · 2023-10-13T12:50:00Z

It appears that after renaming/moving a library, you need to use install_name_tool: x.diff.txt

With that, dnsdist starts up and my QUIC tests work!

Nice, I have pushed that change! I wonder what that tool does exactly, but that's a discussion for another day :)

omoerbeek · 2023-10-16T05:59:48Z

Pondering how to solve the OpenBSD build, might try not using the built-in BoringSSL and just link everything against the BoringSSL package, not using LibreSSL at all.

chbruyand added enhancement dnsdist labels Sep 20, 2023

rgacogne added this to the dnsdist-1.9 milestone Sep 20, 2023

chbruyand force-pushed the dnsdist-doq-quiche branch 2 times, most recently from ab81558 to bc1dc86 Compare September 22, 2023 08:40

chbruyand force-pushed the dnsdist-doq-quiche branch from e8f1e96 to abcecbc Compare September 22, 2023 12:46

chbruyand force-pushed the dnsdist-doq-quiche branch from 15c9b97 to cecb4da Compare September 22, 2023 13:39

github-advanced-security bot found potential problems Sep 22, 2023

View reviewed changes

chbruyand force-pushed the dnsdist-doq-quiche branch from 3b50840 to 1116b9f Compare September 22, 2023 15:13

chbruyand force-pushed the dnsdist-doq-quiche branch from 209c3b7 to 563a0f7 Compare September 26, 2023 13:13

chbruyand marked this pull request as ready for review September 29, 2023 09:10

chbruyand requested a review from omoerbeek September 29, 2023 09:10

chbruyand force-pushed the dnsdist-doq-quiche branch 2 times, most recently from 918da60 to a5ec588 Compare September 29, 2023 11:18

omoerbeek reviewed Sep 29, 2023

View reviewed changes

chbruyand force-pushed the dnsdist-doq-quiche branch from a5ec588 to a161783 Compare September 29, 2023 11:40

omoerbeek reviewed Sep 29, 2023

View reviewed changes

pdns/dnsdist-lua.cc Outdated Show resolved Hide resolved

omoerbeek reviewed Sep 29, 2023

View reviewed changes

pdns/dnsdistdist/docs/guides/dns-over-quic.rst Outdated Show resolved Hide resolved

omoerbeek closed this Sep 29, 2023

rgacogne and others added 13 commits October 9, 2023 13:38

dnsdist: Fix the type of DoQ frontends

0250b85

dnsdist: Move the definition of the DoQ thread to doq.hh

7e290c7

dnsdist: Fix the maximum size of a DoQ retry token

dd9ebcf

dnsdist: Make a separate DoQ connections map per bind

f016a7d

dnsdist: Use a random port in DoQ regression tests

f9a9515

dnsdist: Handle cross-protocol I/O errors for DoQ queries

ef4350f

dnsdist: Delint doq.cc and doq.hh

3201998

dnsdist: Get the final size of an encrypted DoQ token in a cleaner way

c0e87fe

doq: make sure connection is properly reset if necessary in the tests

e7000cc

doq: make cert and key parameters of addDOQLocal mandatory

59b2d7a

doq: doc fix, thanks Otto

fe263e5

Install quiche lib in proper location for macOS, and teach pkgconfig …

4af3e47

…about it

dnsdist: Prevent unaligned access when reading the DNS header in DoQ

d5d9573

rgacogne force-pushed the dnsdist-doq-quiche branch from 5e266bb to d5d9573 Compare October 9, 2023 11:38

rgacogne requested a review from omoerbeek October 10, 2023 09:36

omoerbeek approved these changes Oct 13, 2023

View reviewed changes

pdns/dnsdistdist/doq.cc Outdated Show resolved Hide resolved

pdns/dnsdistdist/doq.cc Outdated Show resolved Hide resolved

rgacogne and others added 3 commits October 13, 2023 14:46

dnsdist: Remove now unneeded clang-tidy suppression

7ae3a63

Co-authored-by: Otto Moerbeek <[email protected]>

dnsdist: Use the QuicheConnection type, as suggested by Otto

7450fc0

Co-authored-by: Otto Moerbeek <[email protected]>

Quiche: Fix the installation of library on macOS

092247f

rgacogne added 2 commits October 13, 2023 17:04

dnsdist: Fix clang-tidy warnings

a764342

dnsdist: More clang-tidy delinting

61e8b85

dnsdist: More clang-tidy delinting

e76816c

rgacogne merged commit 87b575e into PowerDNS:master Oct 16, 2023
74 checks passed

rgacogne mentioned this pull request Oct 16, 2023

dnsdist: Prevent dnsheader alignment issues #13372

Merged

7 tasks

Harvester57 mentioned this pull request Oct 30, 2023

dnsdist: Ponder DNS over HTTP3 support #8914

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dnsdist: add support for incoming DoQ #13280

dnsdist: add support for incoming DoQ #13280

chbruyand commented Sep 20, 2023 •

edited

Loading

rgacogne commented Sep 22, 2023

rgacogne commented Sep 22, 2023

zeha commented Sep 22, 2023 •

edited

Loading

rgacogne commented Sep 22, 2023

zeha commented Sep 22, 2023

rgacogne commented Sep 22, 2023

rgacogne commented Sep 25, 2023

rgacogne commented Sep 28, 2023

omoerbeek left a comment •

edited

Loading

omoerbeek Sep 29, 2023

rgacogne Oct 5, 2023

rgacogne Oct 6, 2023

omoerbeek commented Sep 29, 2023 •

edited

Loading

rgacogne commented Sep 29, 2023

omoerbeek commented Sep 29, 2023

rgacogne commented Oct 9, 2023

coveralls commented Oct 9, 2023

omoerbeek commented Oct 13, 2023 •

edited

Loading

omoerbeek left a comment

rgacogne commented Oct 13, 2023

omoerbeek commented Oct 16, 2023

dnsdist: add support for incoming DoQ #13280

dnsdist: add support for incoming DoQ #13280

Conversation

chbruyand commented Sep 20, 2023 • edited Loading

Short description

Checklist

rgacogne commented Sep 22, 2023

rgacogne commented Sep 22, 2023

zeha commented Sep 22, 2023 • edited Loading

rgacogne commented Sep 22, 2023

zeha commented Sep 22, 2023

rgacogne commented Sep 22, 2023

rgacogne commented Sep 25, 2023

rgacogne commented Sep 28, 2023

omoerbeek left a comment • edited Loading

Choose a reason for hiding this comment

omoerbeek Sep 29, 2023

Choose a reason for hiding this comment

rgacogne Oct 5, 2023

Choose a reason for hiding this comment

rgacogne Oct 6, 2023

Choose a reason for hiding this comment

omoerbeek commented Sep 29, 2023 • edited Loading

rgacogne commented Sep 29, 2023

omoerbeek commented Sep 29, 2023

rgacogne commented Oct 9, 2023

coveralls commented Oct 9, 2023

omoerbeek commented Oct 13, 2023 • edited Loading

omoerbeek left a comment

Choose a reason for hiding this comment

rgacogne commented Oct 13, 2023

omoerbeek commented Oct 16, 2023

chbruyand commented Sep 20, 2023 •

edited

Loading

zeha commented Sep 22, 2023 •

edited

Loading

omoerbeek left a comment •

edited

Loading

omoerbeek commented Sep 29, 2023 •

edited

Loading

omoerbeek commented Oct 13, 2023 •

edited

Loading