feat(connlib): transparently forward non-resources DNS queries #6181

thomaseizinger · 2024-08-06T06:17:41Z

Currently, connlib depends on hickory-resolver to perform DNS queries for non-resources. This is unnecessary. Instead of buffering the original UDP DNS query, consulting hickory to resolve the name and mapping the response back, we can simply take the UDP payload and send it via our protected socket directly to the original upstream DNS server.

This ensures connlib is as transparent as possible for DNS queries for non-resources. Additionally, it removes a lot of error handling and other cruft that we currently have to perform because we are using hickory. For example, hickory will automatically retry a DNS query after a certain timeout. However, the OS / client talking to connlib will also retry after a certain timeout because it is making DNS queries over an unreliable transport (UDP). It is thus unnecessary for us to do that internally.

To correctly test this change, our test-suite needed some refactoring. Specifically, DNS servers are now modelled as dedicated Hosts that can receive (UDP) traffic.

Lastly, we can remove our dependency on hickory-proto and hickory-resolver everywhere and only use domain for parsing DNS messages.

Resolves: #6141.
Related: #6033.
Related: #4800. (Impossible to happen with this design)

vercel · 2024-08-06T06:17:44Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
firezone	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 7, 2024 6:58am

thomaseizinger · 2024-08-06T06:19:06Z

@conectado Patch-by-patch review is recommended. I can split some of the first commits out into a dedicated PR if you'd like.

thomaseizinger · 2024-08-06T07:57:01Z

rust/connlib/tunnel/src/client.rs

-    #[test]
-    fn update_system_dns_works() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_without_dns());
-
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-
-        assert!(dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.1.1.1:53")]);
-    }
-
-    #[test]
-    fn update_system_dns_without_change_is_a_no_op() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_without_dns());
-
-        let _ = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-
-        assert!(!dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.1.1.1:53")]);
-    }
-
-    #[test]
-    fn update_system_dns_with_change_works() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_without_dns());
-
-        let _ = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.0.0.1")]);
-
-        assert!(dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.0.0.1:53")]);
-    }
-
-    #[test]
-    fn update_to_system_with_sentinels_are_ignored() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_without_dns());
-
-        let _ = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-        let dns_changed = client_state.update_system_resolvers(vec![
-            ip("1.1.1.1"),
-            ip("100.100.111.1"),
-            ip("fd00:2021:1111:8000:100:100:111:0"),
-        ]);
-
-        assert!(!dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.1.1.1:53")]);
-    }
-
-    #[test]
-    fn upstream_dns_wins_over_system() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_with_dns());
-
-        let dns_changed = client_state.update_dns_mapping();
-        assert!(dns_changed);
-
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.0.0.1")]);
-        assert!(!dns_changed);
-
-        dns_mapping_is_exactly(client_state.dns_mapping(), dns_list());
-    }
-
-    #[test]
-    fn upstream_dns_change_updates() {
-        let mut client_state = ClientState::for_test();
-
-        let dns_changed = client_state.update_interface_config(interface_config_with_dns());
-
-        assert!(dns_changed);
-
-        let dns_changed = client_state.update_interface_config(InterfaceConfig {
-            upstream_dns: vec![dns("8.8.8.8:53")],
-            ..interface_config_without_dns()
-        });
-
-        assert!(dns_changed);
-
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("8.8.8.8:53")]);
-    }
-
-    #[test]
-    fn upstream_dns_no_change_is_a_no_op() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_with_dns());
-
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.0.0.1")]);
-
-        assert!(dns_changed);
-
-        let dns_changed = client_state.update_interface_config(interface_config_with_dns());
-
-        assert!(!dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), dns_list());
-    }
-
-    #[test]
-    fn upstream_dns_sentinels_are_ignored() {
-        let mut client_state = ClientState::for_test();
-        let mut config = interface_config_with_dns();
-
-        let _ = client_state.update_interface_config(config.clone());
-
-        config.upstream_dns.push(dns("100.100.111.1:53"));
-        config
-            .upstream_dns
-            .push(dns("[fd00:2021:1111:8000:100:100:111:0]:53"));
-
-        let dns_changed = client_state.update_interface_config(config);
-
-        assert!(!dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), dns_list())
-    }
-
-    #[test]
-    fn system_dns_takes_over_when_upstream_are_unset() {
-        let mut client_state = ClientState::for_test();
-        let _ = client_state.update_interface_config(interface_config_with_dns());
-
-        let _ = client_state.update_system_resolvers(vec![ip("1.0.0.1")]);
-        let dns_changed = client_state.update_interface_config(interface_config_without_dns());
-
-        assert!(dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.0.0.1:53")]);
-    }


I decided to remove these tests because this is all covered by tunnel_test.

Does tunnel_test now cover that the sentinel dns should never repeat sentinel ips twice in a row?

I deliberately left those tests in place! The ones I deleted are only about which DNS servers take effect (system vs upstream).

github-actions · 2024-08-06T08:09:19Z

Bencher

Report	Wed, August 7, 2024 at 07:07:14 UTC
Project	Firezone
Branch	refactor/connlib/no-hickory
Testbed	github-actions

Click to view all benchmark results

Benchmark	Throughput	Throughput Results bits/s \| (Δ%)	Throughput Lower Boundary bits/s \| (%)
direct-tcp-client2server	✅ (view plot)	240,094,492.42 (-0.80%)	237,403,467.53 (98.88%)
direct-tcp-server2client	✅ (view plot)	254,570,272.28 (+2.47%)	241,569,573.08 (94.89%)
direct-udp-client2server	✅ (view plot)	277,250,928.19 (-4.07%)	271,846,028.41 (98.05%)
direct-udp-server2client	✅ (view plot)	400,171,041.24 (+1.01%)	384,359,562.07 (96.05%)
relayed-tcp-client2server	✅ (view plot)	250,241,014.12 (+1.75%)	239,488,547.68 (95.70%)
relayed-tcp-server2client	✅ (view plot)	260,331,066.20 (+1.10%)	247,079,397.77 (94.91%)
relayed-udp-client2server	✅ (view plot)	241,970,245.31 (+5.38%)	218,548,668.43 (90.32%)
relayed-udp-server2client	✅ (view plot)	336,763,255.98 (-0.25%)	318,281,721.70 (94.51%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

thomaseizinger · 2024-08-06T11:28:22Z

Reminder to myself to write a changelog entry.

jamilbk

Would it still be possible to forward queries via DoH using this design? Asking because I know it's going to come up as a requirement at some point (see #4668 )

thomaseizinger · 2024-08-06T22:30:42Z

Would it still be possible to forward queries via DoH using this design? Asking because I know it's going to come up as a requirement at some point (see #4668 )

Yes. It will require us to add a DNS client again or write one ourselves. I do think that byte-for-byte forwarding is the better baseline though.

For secure DNS, we'd have to think whether we still want the client to use UDP and we map internally or whether we'd configure the system to directly attempt to do DoH / DoT with us.

conectado

Great work! DNS looks much better now and I expect DNS resolution will be much smoother when we ship this

rust/connlib/tunnel/src/client.rs

conectado · 2024-08-07T02:01:55Z

rust/connlib/tunnel/src/client.rs

-    #[test]
-    fn update_system_dns_works() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_without_dns());
-
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-
-        assert!(dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.1.1.1:53")]);
-    }
-
-    #[test]
-    fn update_system_dns_without_change_is_a_no_op() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_without_dns());
-
-        let _ = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-
-        assert!(!dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.1.1.1:53")]);
-    }
-
-    #[test]
-    fn update_system_dns_with_change_works() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_without_dns());
-
-        let _ = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.0.0.1")]);
-
-        assert!(dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.0.0.1:53")]);
-    }
-
-    #[test]
-    fn update_to_system_with_sentinels_are_ignored() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_without_dns());
-
-        let _ = client_state.update_system_resolvers(vec![ip("1.1.1.1")]);
-        let dns_changed = client_state.update_system_resolvers(vec![
-            ip("1.1.1.1"),
-            ip("100.100.111.1"),
-            ip("fd00:2021:1111:8000:100:100:111:0"),
-        ]);
-
-        assert!(!dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.1.1.1:53")]);
-    }
-
-    #[test]
-    fn upstream_dns_wins_over_system() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_with_dns());
-
-        let dns_changed = client_state.update_dns_mapping();
-        assert!(dns_changed);
-
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.0.0.1")]);
-        assert!(!dns_changed);
-
-        dns_mapping_is_exactly(client_state.dns_mapping(), dns_list());
-    }
-
-    #[test]
-    fn upstream_dns_change_updates() {
-        let mut client_state = ClientState::for_test();
-
-        let dns_changed = client_state.update_interface_config(interface_config_with_dns());
-
-        assert!(dns_changed);
-
-        let dns_changed = client_state.update_interface_config(InterfaceConfig {
-            upstream_dns: vec![dns("8.8.8.8:53")],
-            ..interface_config_without_dns()
-        });
-
-        assert!(dns_changed);
-
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("8.8.8.8:53")]);
-    }
-
-    #[test]
-    fn upstream_dns_no_change_is_a_no_op() {
-        let mut client_state = ClientState::for_test();
-        client_state.interface_config = Some(interface_config_with_dns());
-
-        let dns_changed = client_state.update_system_resolvers(vec![ip("1.0.0.1")]);
-
-        assert!(dns_changed);
-
-        let dns_changed = client_state.update_interface_config(interface_config_with_dns());
-
-        assert!(!dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), dns_list());
-    }
-
-    #[test]
-    fn upstream_dns_sentinels_are_ignored() {
-        let mut client_state = ClientState::for_test();
-        let mut config = interface_config_with_dns();
-
-        let _ = client_state.update_interface_config(config.clone());
-
-        config.upstream_dns.push(dns("100.100.111.1:53"));
-        config
-            .upstream_dns
-            .push(dns("[fd00:2021:1111:8000:100:100:111:0]:53"));
-
-        let dns_changed = client_state.update_interface_config(config);
-
-        assert!(!dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), dns_list())
-    }
-
-    #[test]
-    fn system_dns_takes_over_when_upstream_are_unset() {
-        let mut client_state = ClientState::for_test();
-        let _ = client_state.update_interface_config(interface_config_with_dns());
-
-        let _ = client_state.update_system_resolvers(vec![ip("1.0.0.1")]);
-        let dns_changed = client_state.update_interface_config(interface_config_without_dns());
-
-        assert!(dns_changed);
-        dns_mapping_is_exactly(client_state.dns_mapping(), vec![dns("1.0.0.1:53")]);
-    }


Does tunnel_test now cover that the sentinel dns should never repeat sentinel ips twice in a row?

rust/connlib/tunnel/src/dns.rs

rust/ip-packet/src/lib.rs

This will allow us to answer DNS requests from clients that are emitted as `Transmit`s.

thomaseizinger · 2024-08-07T08:43:18Z

I dogfooded this for the afternoon on my machine and it worked flawlessly.

Add either new changelog entries for next version or modify existing changelog entries to point out #6181.

thomaseizinger requested review from conectado and jamilbk August 6, 2024 06:17

thomaseizinger force-pushed the refactor/connlib/no-hickory branch from 5989b9e to 87faaf3 Compare August 6, 2024 06:18

vercel bot deployed to Preview August 6, 2024 06:20 View deployment

vercel bot deployed to Preview August 6, 2024 06:38 View deployment

thomaseizinger force-pushed the refactor/connlib/no-hickory branch from 96329c4 to f84e329 Compare August 6, 2024 07:42

thomaseizinger changed the title ~~feat(connlib): strictly pass-through UDP DNS queries for non-resources~~ feat(connlib): passthrough UDP DNS queries for non-resources Aug 6, 2024

vercel bot deployed to Preview August 6, 2024 07:43 View deployment

thomaseizinger force-pushed the refactor/connlib/no-hickory branch from f84e329 to 6af88ee Compare August 6, 2024 07:49

vercel bot deployed to Preview August 6, 2024 07:50 View deployment

thomaseizinger commented Aug 6, 2024

View reviewed changes

vercel bot deployed to Preview August 6, 2024 07:57 View deployment

vercel bot deployed to Preview August 6, 2024 08:00 View deployment

thomaseizinger changed the title ~~feat(connlib): passthrough UDP DNS queries for non-resources~~ feat(connlib): strict passthrough of non-resources DNS queries Aug 6, 2024

thomaseizinger force-pushed the refactor/connlib/no-hickory branch from dd3dd27 to ddec58c Compare August 6, 2024 08:18

vercel bot deployed to Preview August 6, 2024 08:20 View deployment

thomaseizinger changed the title ~~feat(connlib): strict passthrough of non-resources DNS queries~~ feat(connlib): transparently forward non-resources DNS queries Aug 6, 2024

thomaseizinger self-assigned this Aug 6, 2024

jamilbk reviewed Aug 6, 2024

View reviewed changes

conectado approved these changes Aug 7, 2024

View reviewed changes

thomaseizinger added 5 commits August 7, 2024 16:48

Introduce DNS servers as dedicated Hosts

df1a335

This will allow us to answer DNS requests from clients that are emitted as `Transmit`s.

Remove forwarding of DNS queries to hickory

d84d292

Forward DNS queries as UDP packets directly

6837237

Remove hickory dependencies

70f297f

Remove unused dependencies

6b0ec67

thomaseizinger added 6 commits August 7, 2024 16:48

Fix features

8db31d7

Only handle a single query in SimDns

35c90fa

Remove unnecessary bool return value

6b9bc63

Remove obsolete tests

d0b38ab

Add explanation for timeout

24ebfc5

Move comment to appropriate code section

6abd9c6

thomaseizinger force-pushed the refactor/connlib/no-hickory branch from ddec58c to 6abd9c6 Compare August 7, 2024 06:48

Don't drop transmit

a6aab74

vercel bot deployed to Preview August 7, 2024 06:53 View deployment

Undo left-over code piece from earlier iteration

72d75cc

vercel bot deployed to Preview August 7, 2024 06:58 View deployment

thomaseizinger enabled auto-merge August 7, 2024 08:43

auto-merge was automatically disabled August 7, 2024 08:45
Pull Request is not mergeable

thomaseizinger added this pull request to the merge queue Aug 7, 2024

Merged via the queue into main with commit 128d0eb Aug 7, 2024

thomaseizinger deleted the refactor/connlib/no-hickory branch August 7, 2024 09:07

This was referenced Aug 9, 2024

fix(connlib): index forwarded DNS queries by ID + socket #6233

Merged

chore: add changelog entries for #6181 #6236

Merged

github-merge-queue bot pushed a commit that referenced this pull request Aug 9, 2024

chore: add changelog entries for #6181 (#6236)

78bb2ac

Add either new changelog entries for next version or modify existing changelog entries to point out #6181.

This was referenced Aug 10, 2024

refactor(connlib): tidy-up tech-debt around mangling of forwarded DNS queries #5391

Closed

Use different source ports for DNS queries #6297

Closed

Investigate behavior of poorly-behaved NATs that drop source port mappings #6290

Closed

feat(connlib): transparently forward non-resources DNS queries #6181

feat(connlib): transparently forward non-resources DNS queries #6181

Uh oh!

Conversation

thomaseizinger commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomaseizinger commented Aug 6, 2024

Uh oh!

thomaseizinger Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

conectado Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bencher

Uh oh!

thomaseizinger commented Aug 6, 2024

Uh oh!

jamilbk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger commented Aug 6, 2024

Uh oh!

conectado left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

conectado Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomaseizinger commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thomaseizinger commented Aug 6, 2024 •

edited

Loading

vercel bot commented Aug 6, 2024 •

edited

Loading

github-actions bot commented Aug 6, 2024 •

edited

Loading

jamilbk left a comment •

edited

Loading