From 973e48cf687a86d45f0afca2fbbe76e60f0bf716 Mon Sep 17 00:00:00 2001
From: Gabi <gabrielalejandro7@gmail.com>
Date: Wed, 17 Jan 2024 23:30:30 -0300
Subject: [PATCH] Fix dns bad nxdomain (#3299)

Some dns servers return NXDOMAIN for queries where the address exists
but there is no
answer for the given query type(e.g. AAAA-only records). This is not up
to spec and
musl PROPERLY assumes that means there is no record of any type. Saddly,
this happens even
with google DNS so we can expect it to happen everywhere. So we use
getaddrinfo to separate
requests for A and AAAA queries and preventing this.

Seems to work locally, though the exact situation where we have a record
that returns NXDOMAIN while it exists is easier to reproduce in staging,
we should test it after we merge.

Fixes #3215
---
 rust/Cargo.lock                               | 14 +++++
 rust/Cargo.toml                               |  1 +
 rust/connlib/shared/Cargo.toml                |  1 +
 rust/connlib/tunnel/Cargo.toml                |  1 +
 .../tunnel/src/control_protocol/gateway.rs    | 54 +++++++++++++++++--
 5 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index fdef81d6b..01c18e4fd 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -1221,6 +1221,7 @@ dependencies = [
  "base64 0.21.7",
  "boringtun",
  "chrono",
+ "dns-lookup",
  "domain",
  "futures",
  "futures-util",
@@ -1634,6 +1635,18 @@ dependencies = [
  "syn 2.0.48",
 ]
 
+[[package]]
+name = "dns-lookup"
+version = "2.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e5766087c2235fec47fafa4cfecc81e494ee679d0fd4a59887ea0919bfb0e4fc"
+dependencies = [
+ "cfg-if",
+ "libc",
+ "socket2 0.5.5",
+ "windows-sys 0.48.0",
+]
+
 [[package]]
 name = "domain"
 version = "0.9.3"
@@ -2057,6 +2070,7 @@ dependencies = [
  "bytes",
  "chrono",
  "connlib-shared",
+ "dns-lookup",
  "domain",
  "futures",
  "futures-bounded",
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index e330369c6..1aa8556d9 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -30,6 +30,7 @@ webrtc = "0.9"
 str0m = "0.4"
 futures-bounded = "0.2.1"
 domain = { version = "0.9", features = ["serde"] }
+dns-lookup = "2.0"
 
 connlib-client-android = { path = "connlib/clients/android"}
 connlib-client-apple = { path = "connlib/clients/apple"}
diff --git a/rust/connlib/shared/Cargo.toml b/rust/connlib/shared/Cargo.toml
index c1d39cc2f..343c49b23 100644
--- a/rust/connlib/shared/Cargo.toml
+++ b/rust/connlib/shared/Cargo.toml
@@ -37,6 +37,7 @@ ring = "0.17"
 hickory-resolver = { workspace = true }
 domain = { workspace = true }
 libc = "0.2"
+dns-lookup = { workspace = true }
 
 # Needed for Android logging until tracing is working
 log = "0.4"
diff --git a/rust/connlib/tunnel/Cargo.toml b/rust/connlib/tunnel/Cargo.toml
index 8aa3b01ee..396a4595d 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 }
 
 # TODO: research replacing for https://github.com/algesten/str0m
 webrtc = { workspace = true }
diff --git a/rust/connlib/tunnel/src/control_protocol/gateway.rs b/rust/connlib/tunnel/src/control_protocol/gateway.rs
index cc02efdeb..50386b414 100644
--- a/rust/connlib/tunnel/src/control_protocol/gateway.rs
+++ b/rust/connlib/tunnel/src/control_protocol/gateway.rs
@@ -101,10 +101,9 @@ where
                     return Err(Error::InvalidResource);
                 }
 
-                (domain.to_string(), 0)
-                    .to_socket_addrs()?
-                    .map(|addrs| addrs.ip().into())
-                    .collect()
+                // TODO: we should make this async, this is acceptable for now though
+                // in the future we will use hickory-resolver for this anyways.
+                resolve_addresses(&domain.to_string())?
             }
             ResourceDescription::Cidr(ref cidr) => vec![cidr.address],
         };
@@ -262,3 +261,50 @@ where
         Ok(())
     }
 }
+
+#[cfg(target_os = "windows")]
+fn resolve_addresses(_: &str) -> std::io::Result<Vec<IpNetwork>> {
+    unimplemented!()
+}
+
+#[cfg(not(target_os = "windows"))]
+fn resolve_addresses(addr: &str) -> std::io::Result<Vec<IpNetwork>> {
+    use libc::{AF_INET, AF_INET6};
+    let addr_v4: std::io::Result<Vec<_>> = resolve_address_family(addr, AF_INET)
+        .map_err(|e| e.into())
+        .and_then(|a| a.collect());
+    let addr_v6: std::io::Result<Vec<_>> = 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<AddrInfoIter, LookupError> {
+    use libc::SOCK_STREAM;
+
+    dns_lookup::getaddrinfo(
+        Some(addr),
+        None,
+        Some(AddrInfoHints {
+            socktype: SOCK_STREAM,
+            address: family,
+            ..Default::default()
+        }),
+    )
+}