From 830302af43106855c8941b4e4fa5ed75dcd29100 Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Mon, 12 Feb 2024 13:04:51 -0600 Subject: [PATCH] test(linux): Low-risk changes to prepare for Linux DNS support (#3625) This splits off the easy parts from #3605. - Add quotes around `PHOENIX_SECURE_COOKIES` because my local `docker-compose` considers unquoted 'false' to be a schema error - Env vars are strings or numbers, not bools, it says - Create `test.httpbin.docker.local` container in a new subnet so it can be used as a DNS resource without the existing CIDR resource picking it up - Add resources and policies to `seeds.exs` per #3342 - Fix warning about `CONNLIB_LOG_UPLOAD_INTERVAL_SECS` not being set - Add `resolv-conf` dep and unit tests to `firezone-tunnel` and `firezone-linux-client` - Impl `on_disconnect` in the Linux client with `tracing::error!` - Add comments ```[tasklist] - [x] (failed) Confirm that the client container actually does stop faster this way - [x] Wait for tests to pass - [x] Mark as ready for review ``` --- docker-compose.yml | 20 ++- elixir/apps/domain/priv/repo/seeds.exs | 52 ++++++++ rust/Cargo.lock | 2 + rust/Dockerfile | 1 + rust/connlib/shared/src/callbacks.rs | 5 +- rust/connlib/tunnel/Cargo.toml | 1 + .../tunnel/src/device_channel/tun_linux.rs | 115 +++++++++++++++++- .../tunnel/src/device_channel/tun_windows.rs | 1 + rust/linux-client/Cargo.toml | 1 + rust/linux-client/src/main.rs | 8 ++ 10 files changed, 200 insertions(+), 6 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 05ba6202f..7be000a36 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -64,7 +64,7 @@ services: # Web Server EXTERNAL_URL: http://localhost:8080/ PHOENIX_HTTP_WEB_PORT: "8080" - PHOENIX_SECURE_COOKIES: false + PHOENIX_SECURE_COOKIES: "false" # Erlang ERLANG_DISTRIBUTION_PORT: 9000 ERLANG_CLUSTER_ADAPTER: "Elixir.Cluster.Strategy.Epmd" @@ -133,6 +133,8 @@ services: condition: "service_healthy" httpbin: condition: "service_healthy" + test.httpbin.docker.local: + condition: "service_healthy" iperf3: condition: "service_healthy" api: @@ -173,6 +175,7 @@ services: networks: app: ipv4_address: 172.28.0.105 + dns_resources: resources: httpbin: @@ -183,6 +186,14 @@ services: resources: ipv4_address: 172.20.0.100 + test.httpbin.docker.local: + image: kennethreitz/httpbin + healthcheck: + test: ["CMD-SHELL", "ps -C gunicorn"] + networks: + dns_resources: + ipv4_address: 172.21.0.100 + iperf3: image: networkstatic/iperf3 healthcheck: @@ -249,7 +260,7 @@ services: # Web Server EXTERNAL_URL: http://localhost:8081/ PHOENIX_HTTP_API_PORT: "8081" - PHOENIX_SECURE_COOKIES: false + PHOENIX_SECURE_COOKIES: "false" # Erlang ERLANG_DISTRIBUTION_PORT: 9000 ERLANG_CLUSTER_ADAPTER: "Elixir.Cluster.Strategy.Epmd" @@ -357,6 +368,11 @@ services: # IPv6 is currently causing flakiness with GH actions and on our testbed. # Disabling until there's more time to debug. networks: + # Using a separate subnet here so that the CIDR resource for 172.20.0.0 won't catch DNS resources + dns_resources: + ipam: + config: + - subnet: 172.21.0.0/24 resources: # enable_ipv6: true ipam: diff --git a/elixir/apps/domain/priv/repo/seeds.exs b/elixir/apps/domain/priv/repo/seeds.exs index f4ca6474e..04cd35a83 100644 --- a/elixir/apps/domain/priv/repo/seeds.exs +++ b/elixir/apps/domain/priv/repo/seeds.exs @@ -679,6 +679,36 @@ IO.puts("") admin_subject ) +{:ok, dns_httpbin_resource} = + Resources.create_resource( + %{ + type: :dns, + name: "?.httpbin.docker.local", + address: "?.httpbin.docker.local", + address_description: "http://test.httpbin.docker.local/", + connections: [%{gateway_group_id: gateway_group.id}], + filters: [ + %{ports: ["80", "433"], protocol: :tcp}, + %{ports: ["53"], protocol: :udp}, + %{protocol: :icmp} + ] + }, + admin_subject + ) + +{:ok, dns_docker_resource} = + Resources.create_resource( + %{ + type: :dns, + name: "*.docker.local", + address: "*.docker.local", + address_description: "*.docker.local/", + connections: [%{gateway_group_id: gateway_group.id}], + filters: [%{protocol: :all}] + }, + admin_subject + ) + IO.puts("Created resources:") IO.puts(" #{dns_google_resource.address} - DNS - gateways: #{gateway_name}") IO.puts(" #{dns_gitlab_resource.address} - DNS - gateways: #{gateway_name}") @@ -687,6 +717,8 @@ IO.puts(" #{firezone_dev.address} - DNS - gateways: #{gateway_name}") IO.puts(" #{example_dns.address} - DNS - gateways: #{gateway_name}") IO.puts(" #{ip_resource.address} - IP - gateways: #{gateway_name}") IO.puts(" #{cidr_resource.address} - CIDR - gateways: #{gateway_name}") +IO.puts(" #{dns_httpbin_resource.address} - DNS - gateways: #{gateway_name}") +IO.puts(" #{dns_docker_resource.address} - DNS - gateways: #{gateway_name}") IO.puts("") {:ok, _} = @@ -759,6 +791,26 @@ IO.puts("") admin_subject ) +{:ok, _} = + Policies.create_policy( + %{ + name: "All Access To httpbin.docker.local", + actor_group_id: everyone_group.id, + resource_id: dns_httpbin_resource.id + }, + admin_subject + ) + +{:ok, _} = + Policies.create_policy( + %{ + name: "All Access To httpbin.docker.local", + actor_group_id: everyone_group.id, + resource_id: dns_docker_resource.id + }, + admin_subject + ) + IO.puts("Policies Created") IO.puts("") diff --git a/rust/Cargo.lock b/rust/Cargo.lock index b55a4bf59..54db107d0 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -2045,6 +2045,7 @@ dependencies = [ "connlib-client-shared", "firezone-cli-utils", "humantime", + "resolv-conf", "secrecy", "tracing", "tracing-subscriber", @@ -2120,6 +2121,7 @@ dependencies = [ "parking_lot", "pnet_packet", "rand_core 0.6.4", + "resolv-conf", "rtnetlink", "secrecy", "serde", diff --git a/rust/Dockerfile b/rust/Dockerfile index 3107df8d0..7cf37958e 100644 --- a/rust/Dockerfile +++ b/rust/Dockerfile @@ -87,6 +87,7 @@ COPY . . ARG TARGET ARG PACKAGE +ENV CONNLIB_LOG_UPLOAD_INTERVAL_SECS=300 RUN cargo build -p ${PACKAGE} $([ -n "${TARGET}" ] && "--target ${TARGET}") # Image which is used to run the application binary diff --git a/rust/connlib/shared/src/callbacks.rs b/rust/connlib/shared/src/callbacks.rs index 6513389d5..83b2e91b4 100644 --- a/rust/connlib/shared/src/callbacks.rs +++ b/rust/connlib/shared/src/callbacks.rs @@ -64,7 +64,10 @@ pub trait Callbacks: Clone + Send + Sync { std::process::exit(0); } - /// Returns the system's default resolver + /// Returns the system's default resolver(s) + /// + /// It's okay for clients to include Firezone's own DNS here, e.g. 100.100.111.1. + /// connlib internally filters them out. fn get_system_default_resolvers(&self) -> Result>, Self::Error> { Ok(None) } diff --git a/rust/connlib/tunnel/Cargo.toml b/rust/connlib/tunnel/Cargo.toml index f145fc692..8f18f98e4 100644 --- a/rust/connlib/tunnel/Cargo.toml +++ b/rust/connlib/tunnel/Cargo.toml @@ -30,6 +30,7 @@ hickory-resolver = { workspace = true } arc-swap = "1.6.0" bimap = "0.6" dns-lookup = { workspace = true } +resolv-conf = "0.7.0" # TODO: research replacing for https://github.com/algesten/str0m webrtc = { workspace = true } diff --git a/rust/connlib/tunnel/src/device_channel/tun_linux.rs b/rust/connlib/tunnel/src/device_channel/tun_linux.rs index d119c2e9d..cea7a6a6e 100644 --- a/rust/connlib/tunnel/src/device_channel/tun_linux.rs +++ b/rust/connlib/tunnel/src/device_channel/tun_linux.rs @@ -91,7 +91,11 @@ impl Tun { utils::poll_raw_fd(&self.fd, |fd| read(fd, buf), cx) } - pub fn new(config: &InterfaceConfig, _: Vec, _: &impl Callbacks) -> Result { + pub fn new( + config: &InterfaceConfig, + dns_config: Vec, + _: &impl Callbacks, + ) -> Result { create_tun_device()?; let fd = match unsafe { open(TUN_FILE.as_ptr() as _, O_RDWR) } { @@ -113,7 +117,9 @@ impl Tun { handle: handle.clone(), connection: join_handle, fd: AsyncFd::new(fd)?, - worker: Mutex::new(Some(set_iface_config(config.clone(), handle).boxed())), + worker: Mutex::new(Some( + set_iface_config(config.clone(), dns_config, handle).boxed(), + )), }) } @@ -190,7 +196,7 @@ impl Tun { } #[tracing::instrument(level = "trace", skip(handle))] -async fn set_iface_config(config: InterfaceConfig, handle: Handle) -> Result<()> { +async fn set_iface_config(config: InterfaceConfig, _: Vec, handle: Handle) -> Result<()> { let index = handle .link() .get() @@ -308,3 +314,106 @@ impl ioctl::Request { struct SetTunFlagsPayload { flags: std::ffi::c_short, } + +#[cfg(test)] +mod tests { + use std::{ + net::{IpAddr, Ipv4Addr, Ipv6Addr}, + str::FromStr, + }; + + const DEBIAN_VM_RESOLV_CONF: &str = r#" +# This is /run/systemd/resolve/stub-resolv.conf managed by man:systemd-resolved(8). +# Do not edit. +# +# This file might be symlinked as /etc/resolv.conf. If you're looking at +# /etc/resolv.conf and seeing this text, you have followed the symlink. +# +# This is a dynamic resolv.conf file for connecting local clients to the +# internal DNS stub resolver of systemd-resolved. This file lists all +# configured search domains. +# +# Run "resolvectl status" to see details about the uplink DNS servers +# currently in use. +# +# Third party programs should typically not access this file directly, but only +# through the symlink at /etc/resolv.conf. To manage man:resolv.conf(5) in a +# different way, replace this symlink by a static file or a different symlink. +# +# See man:systemd-resolved.service(8) for details about the supported modes of +# operation for /etc/resolv.conf. +nameserver 127.0.0.53 +options edns0 trust-ad +search . +"#; + + // Docker seems to have injected the WSL host's resolv.conf into the Alpine container + // Also the nameserver is changed for privacy + const ALPINE_CONTAINER_RESOLV_CONF: &str = r#" +# This file was automatically generated by WSL. To stop automatic generation of this file, add the following entry to /etc/wsl.conf: +# [network] +# generateResolvConf = false +nameserver 9.9.9.9 +"#; + + // From a Debian desktop + const NETWORK_MANAGER_RESOLV_CONF: &str = r" +# Generated by NetworkManager +nameserver 192.168.1.1 +nameserver 2001:db8::%eno1 +"; + + #[test] + fn parse_resolv_conf() { + let parsed = resolv_conf::Config::parse(DEBIAN_VM_RESOLV_CONF).unwrap(); + let mut config = resolv_conf::Config::new(); + config + .nameservers + .push(resolv_conf::ScopedIp::V4(Ipv4Addr::new(127, 0, 0, 53))); + config.set_search(vec![".".into()]); + config.edns0 = true; + config.trust_ad = true; + + assert_eq!(parsed, config); + + let parsed = resolv_conf::Config::parse(ALPINE_CONTAINER_RESOLV_CONF).unwrap(); + let mut config = resolv_conf::Config::new(); + config + .nameservers + .push(resolv_conf::ScopedIp::V4(Ipv4Addr::new(9, 9, 9, 9))); + + assert_eq!(parsed, config); + + let parsed = resolv_conf::Config::parse(NETWORK_MANAGER_RESOLV_CONF).unwrap(); + let mut config = resolv_conf::Config::new(); + config + .nameservers + .push(resolv_conf::ScopedIp::V4(Ipv4Addr::new(192, 168, 1, 1))); + config.nameservers.push(resolv_conf::ScopedIp::V6( + Ipv6Addr::new( + 0x2001, 0x0db8, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, + ), + Some("eno1".into()), + )); + + assert_eq!(parsed, config); + } + + #[test] + fn print_resolv_conf() { + let mut new_resolv_conf = resolv_conf::Config::new(); + for addr in ["100.100.111.1", "100.100.111.2"] { + new_resolv_conf + .nameservers + .push(IpAddr::from_str(addr).unwrap().into()); + } + + let actual = new_resolv_conf.to_string(); + assert_eq!( + actual, + r"nameserver 100.100.111.1 +nameserver 100.100.111.2 +" + ); + } +} diff --git a/rust/connlib/tunnel/src/device_channel/tun_windows.rs b/rust/connlib/tunnel/src/device_channel/tun_windows.rs index 8ff937077..a7a2965a1 100644 --- a/rust/connlib/tunnel/src/device_channel/tun_windows.rs +++ b/rust/connlib/tunnel/src/device_channel/tun_windows.rs @@ -121,6 +121,7 @@ impl Tun { // using QUIC, HTTP/2, or even HTTP/1.1, and so they won't resolve the DNS // again unless you let that connection time out: // + // TODO: If we have a Windows gateway, it shouldn't configure DNS, right? Command::new("powershell") .creation_flags(CREATE_NO_WINDOW) .arg("-Command") diff --git a/rust/linux-client/Cargo.toml b/rust/linux-client/Cargo.toml index d9f5db903..2427aa2b7 100644 --- a/rust/linux-client/Cargo.toml +++ b/rust/linux-client/Cargo.toml @@ -15,3 +15,4 @@ tracing = { workspace = true } clap = { version = "4.4", features = ["derive", "env"] } tracing-subscriber = { version = "0.3.17", features = ["env-filter"] } humantime = "2.1" +resolv-conf = "0.7.0" diff --git a/rust/linux-client/src/main.rs b/rust/linux-client/src/main.rs index d53928eeb..6c27c12a4 100644 --- a/rust/linux-client/src/main.rs +++ b/rust/linux-client/src/main.rs @@ -38,6 +38,14 @@ struct CallbackHandler { impl Callbacks for CallbackHandler { type Error = std::convert::Infallible; + fn on_disconnect( + &self, + error: Option<&connlib_client_shared::Error>, + ) -> Result<(), Self::Error> { + tracing::error!(?error, "Disconnected"); + Ok(()) + } + fn roll_log_file(&self) -> Option { self.handle .as_ref()?