From 085351f455ae7009d9f2c8e1c8748e93376d530b Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Thu, 15 Feb 2024 13:26:34 -0600 Subject: [PATCH] revert: 3622 to fix failing DNS CI test (#3654) Reverts #3622 I don't know why, but that change seemed to cause the `/etc/resolv.conf` test to fail in CI and I was thinking of the "roll back first" principle https://cloud.google.com/blog/products/gcp/reliable-releases-and-rollbacks-cre-life-lessons ~~I also change one `ping` in CI to `until ping`. This was an earlier attempt before I did the revert, and it seems safe to leave it in.~~ --- rust/Cargo.lock | 4 +- rust/connlib/shared/src/messages.rs | 4 +- rust/connlib/tunnel/Cargo.toml | 1 + rust/connlib/tunnel/src/control_protocol.rs | 2 +- .../tunnel/src/control_protocol/gateway.rs | 110 ++++++++++++------ rust/connlib/tunnel/src/lib.rs | 2 +- rust/connlib/tunnel/src/peer.rs | 3 +- rust/gateway/Cargo.toml | 3 - rust/gateway/src/eventloop.rs | 107 ++--------------- 9 files changed, 92 insertions(+), 144 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index f2dba2530..59e225b89 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -2018,14 +2018,11 @@ dependencies = [ "chrono", "clap", "connlib-shared", - "dns-lookup", "domain", "firezone-cli-utils", "firezone-tunnel", "futures", "futures-bounded", - "ip_network", - "libc", "phoenix-channel", "secrecy", "serde", @@ -2111,6 +2108,7 @@ dependencies = [ "bytes", "chrono", "connlib-shared", + "dns-lookup", "domain", "futures", "futures-bounded", diff --git a/rust/connlib/shared/src/messages.rs b/rust/connlib/shared/src/messages.rs index 82125e5a0..01dfef422 100644 --- a/rust/connlib/shared/src/messages.rs +++ b/rust/connlib/shared/src/messages.rs @@ -131,8 +131,8 @@ impl Eq for RequestConnection {} #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq)] #[serde(tag = "type", rename_all = "snake_case")] -pub enum ResourceDescription { - Dns(TDNS), +pub enum ResourceDescription { + Dns(ResourceDescriptionDns), Cidr(ResourceDescriptionCidr), } diff --git a/rust/connlib/tunnel/Cargo.toml b/rust/connlib/tunnel/Cargo.toml index 6240cff31..ce956d737 100644 --- a/rust/connlib/tunnel/Cargo.toml +++ b/rust/connlib/tunnel/Cargo.toml @@ -29,6 +29,7 @@ futures-bounded = { workspace = true } 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 diff --git a/rust/connlib/tunnel/src/control_protocol.rs b/rust/connlib/tunnel/src/control_protocol.rs index 58631a01d..e547ed50d 100644 --- a/rust/connlib/tunnel/src/control_protocol.rs +++ b/rust/connlib/tunnel/src/control_protocol.rs @@ -24,7 +24,7 @@ use crate::{ }; mod client; -pub mod gateway; +mod gateway; const ICE_CANDIDATE_BUFFER: usize = 100; // We should use not more than 1-2 relays (WebRTC in Firefox breaks at 5) due to combinatoric diff --git a/rust/connlib/tunnel/src/control_protocol/gateway.rs b/rust/connlib/tunnel/src/control_protocol/gateway.rs index 5cf95b1ce..acff30b7b 100644 --- a/rust/connlib/tunnel/src/control_protocol/gateway.rs +++ b/rust/connlib/tunnel/src/control_protocol/gateway.rs @@ -2,38 +2,23 @@ use crate::{ control_protocol::{insert_peers, start_handlers}, dns::is_subdomain, peer::{PacketTransformGateway, Peer}, - ConnectedPeer, Error, GatewayState, PeerConfig, Tunnel, PEER_QUEUE_SIZE, + ConnectedPeer, GatewayState, PeerConfig, Tunnel, PEER_QUEUE_SIZE, }; use chrono::{DateTime, Utc}; use connlib_shared::{ - messages::{ClientId, ConnectionAccepted, DomainResponse, Relay, ResourceId}, - Callbacks, Dname, Result, + messages::{ + ClientId, ClientPayload, ConnectionAccepted, DomainResponse, Relay, ResourceAccepted, + ResourceDescription, + }, + Callbacks, Dname, Error, Result, }; use ip_network::IpNetwork; use std::sync::Arc; use webrtc::ice_transport::{ - ice_parameters::RTCIceParameters, ice_role::RTCIceRole, - ice_transport_state::RTCIceTransportState, RTCIceTransport, + ice_role::RTCIceRole, ice_transport_state::RTCIceTransportState, RTCIceTransport, }; -/// Description of a resource that maps to a DNS record which had its domain already resolved. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct ResolvedResourceDescriptionDns { - pub id: ResourceId, - /// Internal resource's domain name. - pub domain: String, - /// Name of the resource. - /// - /// Used only for display. - pub name: String, - - pub addresses: Vec, -} - -pub type ResourceDescription = - connlib_shared::messages::ResourceDescription; - use super::{new_ice_connection, IceConnection}; #[tracing::instrument(level = "trace", skip(ice))] @@ -72,14 +57,12 @@ where /// /// # Returns /// The connection details - #[allow(clippy::too_many_arguments)] pub async fn set_peer_connection_request( self: &Arc, - ice_parameters: RTCIceParameters, + client_payload: ClientPayload, peer: PeerConfig, relays: Vec, client_id: ClientId, - domain: Option, expires_at: Option>, resource: ResourceDescription, ) -> Result { @@ -109,16 +92,17 @@ where let resource_addresses = match &resource { ResourceDescription::Dns(r) => { - let Some(domain) = domain.clone() else { + let Some(domain) = client_payload.domain.clone() else { return Err(Error::ControlProtocolError); }; - if !is_subdomain(&domain, &r.domain) { + if !is_subdomain(&domain, &r.address) { let _ = ice.stop().await; return Err(Error::InvalidResource); } - r.addresses.clone() + tokio::task::spawn_blocking(move || resolve_addresses(&domain.to_string())) + .await?? } ResourceDescription::Cidr(ref cidr) => vec![cidr.address], }; @@ -128,7 +112,7 @@ where let tunnel = self.clone(); tokio::spawn(async move { if let Err(e) = ice - .start(&ice_parameters, Some(RTCIceRole::Controlled)) + .start(&client_payload.ice_parameters, Some(RTCIceRole::Controlled)) .await .map_err(Into::into) .and_then(|_| { @@ -166,7 +150,7 @@ where Ok(ConnectionAccepted { ice_parameters: local_params, - domain_response: domain.map(|domain| DomainResponse { + domain_response: client_payload.domain.map(|domain| DomainResponse { domain, address: resource_addresses .into_iter() @@ -176,13 +160,13 @@ where }) } - pub fn allow_access( + pub async fn allow_access( &self, resource: ResourceDescription, client_id: ClientId, expires_at: Option>, domain: Option, - ) -> Option { + ) -> Option { let Some(peer) = self .role_state .lock() @@ -199,11 +183,14 @@ where return None; }; - if !is_subdomain(&domain, &r.domain) { + if !is_subdomain(&domain, &r.address) { return None; } - r.addresses.clone() + tokio::task::spawn_blocking(move || resolve_addresses(&domain.to_string())) + .await + .ok()? + .ok()? } ResourceDescription::Cidr(cidr) => vec![cidr.address], }; @@ -214,9 +201,11 @@ where } if let Some(domain) = domain { - return Some(DomainResponse { - domain, - address: addresses.iter().map(|i| i.network_address()).collect(), + return Some(ResourceAccepted { + domain_response: DomainResponse { + domain, + address: addresses.iter().map(|i| i.network_address()).collect(), + }, }); } @@ -270,3 +259,50 @@ where Ok(()) } } + +#[cfg(target_os = "windows")] +fn resolve_addresses(_: &str) -> std::io::Result> { + unimplemented!() +} + +#[cfg(not(target_os = "windows"))] +fn resolve_addresses(addr: &str) -> std::io::Result> { + use libc::{AF_INET, AF_INET6}; + let addr_v4: std::io::Result> = resolve_address_family(addr, AF_INET) + .map_err(|e| e.into()) + .and_then(|a| a.collect()); + let addr_v6: std::io::Result> = resolve_address_family(addr, AF_INET6) + .map_err(|e| e.into()) + .and_then(|a| a.collect()); + match (addr_v4, addr_v6) { + (Ok(v4), Ok(v6)) => Ok(v6 + .iter() + .map(|a| a.sockaddr.ip().into()) + .chain(v4.iter().map(|a| a.sockaddr.ip().into())) + .collect()), + (Ok(v4), Err(_)) => Ok(v4.iter().map(|a| a.sockaddr.ip().into()).collect()), + (Err(_), Ok(v6)) => Ok(v6.iter().map(|a| a.sockaddr.ip().into()).collect()), + (Err(e), Err(_)) => Err(e), + } +} + +#[cfg(not(target_os = "windows"))] +use dns_lookup::{AddrInfoHints, AddrInfoIter, LookupError}; + +#[cfg(not(target_os = "windows"))] +fn resolve_address_family( + addr: &str, + family: i32, +) -> std::result::Result { + use libc::SOCK_STREAM; + + dns_lookup::getaddrinfo( + Some(addr), + None, + Some(AddrInfoHints { + socktype: SOCK_STREAM, + address: family, + ..Default::default() + }), + ) +} diff --git a/rust/connlib/tunnel/src/lib.rs b/rust/connlib/tunnel/src/lib.rs index 02e10fbd3..97925b3f2 100644 --- a/rust/connlib/tunnel/src/lib.rs +++ b/rust/connlib/tunnel/src/lib.rs @@ -48,7 +48,7 @@ use connlib_shared::{ pub use client::ClientState; use connlib_shared::error::ConnlibError; -pub use control_protocol::{gateway::ResolvedResourceDescriptionDns, Request}; +pub use control_protocol::Request; pub use gateway::GatewayState; use crate::ip_packet::MutableIpPacket; diff --git a/rust/connlib/tunnel/src/peer.rs b/rust/connlib/tunnel/src/peer.rs index 35fff80c0..0cc5adb88 100644 --- a/rust/connlib/tunnel/src/peer.rs +++ b/rust/connlib/tunnel/src/peer.rs @@ -13,14 +13,13 @@ use bytes::Bytes; use chrono::{DateTime, Utc}; use connlib_shared::messages::DnsServer; use connlib_shared::IpProvider; -use connlib_shared::{Error, Result}; +use connlib_shared::{messages::ResourceDescription, Error, Result}; use ip_network::IpNetwork; use ip_network_table::IpNetworkTable; use parking_lot::{Mutex, RwLock}; use pnet_packet::Packet; use secrecy::ExposeSecret; -use crate::control_protocol::gateway::ResourceDescription; use crate::MAX_UDP_SIZE; use crate::{device_channel, ip_packet::MutableIpPacket, PeerConfig}; diff --git a/rust/gateway/Cargo.toml b/rust/gateway/Cargo.toml index 7f85d1480..70215e4f9 100644 --- a/rust/gateway/Cargo.toml +++ b/rust/gateway/Cargo.toml @@ -29,9 +29,6 @@ url = { version = "2.4.1", default-features = false } webrtc = { workspace = true } domain = { workspace = true } uuid = { version = "1.7.0", features = ["v4"] } -ip_network = { version = "0.4", default-features = false } -dns-lookup = { workspace = true } -libc = { version = "0.2", default-features = false, features = ["std", "const-extern-fn", "extra_traits"] } [dev-dependencies] serde_json = { version = "1.0", default-features = false, features = ["std"] } diff --git a/rust/gateway/src/eventloop.rs b/rust/gateway/src/eventloop.rs index c39b7b616..3d870d4f5 100644 --- a/rust/gateway/src/eventloop.rs +++ b/rust/gateway/src/eventloop.rs @@ -4,12 +4,9 @@ use crate::messages::{ }; use crate::CallbackHandler; use anyhow::{anyhow, Result}; -use connlib_shared::messages::{ - ClientId, DomainResponse, GatewayResponse, ResourceAccepted, ResourceDescription, -}; +use connlib_shared::messages::{ClientId, GatewayResponse, ResourceAccepted}; use connlib_shared::Error; -use firezone_tunnel::{Event, GatewayState, ResolvedResourceDescriptionDns, Tunnel}; -use ip_network::IpNetwork; +use firezone_tunnel::{Event, GatewayState, Tunnel}; use phoenix_channel::PhoenixChannel; use std::convert::Infallible; use std::sync::Arc; @@ -26,7 +23,7 @@ pub struct Eventloop { connection_request_tasks: futures_bounded::FuturesMap<(ClientId, String), Result>, add_ice_candidate_tasks: futures_bounded::FuturesSet>, - allow_access_tasks: futures_bounded::FuturesMap>, + allow_access_tasks: futures_bounded::FuturesMap>, print_stats_timer: tokio::time::Interval, } @@ -113,14 +110,12 @@ impl Eventloop { } match self.allow_access_tasks.poll_unpin(cx) { - Poll::Ready((reference, Ok(Some(domain_response)))) => { + Poll::Ready((reference, Ok(Some(res)))) => { self.portal.send( PHOENIX_TOPIC, EgressMessages::ConnectionReady(ConnectionReady { reference, - gateway_payload: GatewayResponse::ResourceAccepted(ResourceAccepted { - domain_response, - }), + gateway_payload: GatewayResponse::ResourceAccepted(res), }), ); continue; @@ -162,22 +157,18 @@ impl Eventloop { let tunnel = Arc::clone(&self.tunnel); let connection_request = async move { - let resource = resolve_resource_description(req.resource).await?; - let conn = tunnel .set_peer_connection_request( - req.client.payload.ice_parameters, + req.client.payload, req.client.peer.into(), req.relays, req.client.id, - req.client.payload.domain, req.expires_at, - resource, + req.resource, ) .await?; Ok(GatewayResponse::ConnectionAccepted(conn)) }; - match self .connection_request_tasks .try_push((req.client.id, req.reference.clone()), connection_request) @@ -190,7 +181,6 @@ impl Eventloop { } Ok(()) => {} }; - continue; } Poll::Ready(phoenix_channel::Event::InboundMessage { @@ -208,15 +198,13 @@ impl Eventloop { let tunnel = Arc::clone(&self.tunnel); - let allow_access = async move { - let resource = resolve_resource_description(resource).await.ok()?; - - tunnel.allow_access(resource, client_id, expires_at, payload) - }; - if self .allow_access_tasks - .try_push(reference, allow_access) + .try_push(reference, async move { + tunnel + .allow_access(resource, client_id, expires_at, payload) + .await + }) .is_err() { tracing::warn!("Too many allow access requests, dropping existing one"); @@ -269,74 +257,3 @@ impl Eventloop { } } } - -async fn resolve_resource_description( - resource: ResourceDescription, -) -> io::Result> { - match resource { - ResourceDescription::Dns(dns) => { - let addresses = tokio::task::spawn_blocking({ - let domain = dns.address.clone(); - - move || resolve_addresses(&domain) - }) - .await??; - - Ok(ResourceDescription::Dns(ResolvedResourceDescriptionDns { - id: dns.id, - domain: dns.address, - name: dns.name, - addresses, - })) - } - ResourceDescription::Cidr(cdir) => Ok(ResourceDescription::Cidr(cdir)), - } -} - -#[cfg(target_os = "windows")] -fn resolve_addresses(_: &str) -> std::io::Result> { - unimplemented!() -} - -#[cfg(not(target_os = "windows"))] -fn resolve_addresses(addr: &str) -> std::io::Result> { - use libc::{AF_INET, AF_INET6}; - let addr_v4: std::io::Result> = resolve_address_family(addr, AF_INET) - .map_err(|e| e.into()) - .and_then(|a| a.collect()); - let addr_v6: std::io::Result> = resolve_address_family(addr, AF_INET6) - .map_err(|e| e.into()) - .and_then(|a| a.collect()); - match (addr_v4, addr_v6) { - (Ok(v4), Ok(v6)) => Ok(v6 - .iter() - .map(|a| a.sockaddr.ip().into()) - .chain(v4.iter().map(|a| a.sockaddr.ip().into())) - .collect()), - (Ok(v4), Err(_)) => Ok(v4.iter().map(|a| a.sockaddr.ip().into()).collect()), - (Err(_), Ok(v6)) => Ok(v6.iter().map(|a| a.sockaddr.ip().into()).collect()), - (Err(e), Err(_)) => Err(e), - } -} - -#[cfg(not(target_os = "windows"))] -use dns_lookup::{AddrInfoHints, AddrInfoIter, LookupError}; -use std::io; - -#[cfg(not(target_os = "windows"))] -fn resolve_address_family( - addr: &str, - family: i32, -) -> std::result::Result { - use libc::SOCK_STREAM; - - dns_lookup::getaddrinfo( - Some(addr), - None, - Some(AddrInfoHints { - socktype: SOCK_STREAM, - address: family, - ..Default::default() - }), - ) -}