feat(sandbox): bound proxy connections and finish the SOCKS5 fuzzing (#290)

* feat(sandbox): bound proxy connections and finish the SOCKS5 fuzzing

Three gaps #246 left open.

The accept loop spawned a thread per connection with no bound, so anything
that can reach the port could make the proxy spawn threads, and for IP-mode
clients drive a DNS lookup per allowlist hostname on each. Past 64 in flight
the proxy now closes the connection rather than queuing it; a SOCKS5 client
sees a failed connect. The counter is raised before the spawn and lowered by
a failed one, so the cap cannot drift downward.

Case-insensitive matching is documented but was not asserted: the target now
requires the same verdict for a host with its case flipped. Removing the fold
in is_host_allowed fails it on "c" vs "C".

is_ip_allowed had no coverage because it resolves allowlisted hostnames. It is
reachable through NetAllowlist::is_ip_allowed, and the target restricts its
patterns to the shapes that function never resolves — literal IPs, "*" and
"*." wildcards — so it stays off the network. The resolving branch needs a
stubbed resolver and is still uncovered.

Signed-off-by: Luca Muscariello <muscariello@ieee.org>

* test(sandbox): unit-test NetAllowlist::is_ip_allowed

The method was added for the fuzz target, and fuzz targets are not in the
coverage build, so a new public method arrived with nothing exercising it —
codecov put the patch at 79.6%.

Literal IPv4 and IPv6 matches, deny-by-default on an empty list, the "*"
short-circuit ahead of address parsing, and a "*." pattern that cannot match
an address on its own. All patterns are literals or wildcards so the tests
never reach the resolver.

Signed-off-by: Luca Muscariello <muscariello@ieee.org>

* refactor(sandbox): make the connection permit RAII and cover the rest

Measuring locally rather than reading codecov's per-line field left five
uncovered lines among the change.

Three were the failed-spawn branch that lowered the counter by hand. A permit
that lowers it on drop removes the branch instead of testing it: the handler
holds it, and a spawn that never runs the closure drops it too. One exit path
per counter beats one branch per exit path.

The other two were a poisoned-lock arm, now tested — a panic while the
allowlist was held must not become permission to connect — and an Err arm in
the shedding test's own connect loop, which cannot fire below the cap and is
now an outright failure instead.

Every added line in net_proxy.rs is covered.

Signed-off-by: Luca Muscariello <muscariello@ieee.org>

---------

Signed-off-by: Luca Muscariello <muscariello@ieee.org>

Author: muscariello

crates/shadi_sandbox/fuzz/fuzz_targets/socks5-frame.rs

@@ -47,5 +47,58 @@ fuzz_target!(|data: &[u8]| {
             "empty allowlist accepted {host}"
         );
         let _ = allowlist.is_allowed(host);
+
+        // Matching is documented as case-insensitive, so the same host in a
+        // different case cannot produce a different verdict. Both the host and
+        // the patterns are folded, so flipping either side must agree.
+        let flipped: String = host
+            .chars()
+            .map(|c| {
+                if c.is_ascii_lowercase() {
+                    c.to_ascii_uppercase()
+                } else {
+                    c.to_ascii_lowercase()
+                }
+            })
+            .collect();
+        assert_eq!(
+            allowlist.is_allowed(host),
+            allowlist.is_allowed(&flipped),
+            "case changed the verdict for {host:?} vs {flipped:?}"
+        );
+    }
+
+    // is_ip_allowed resolves any allowlisted hostname to compare against the
+    // incoming IP, so the patterns here are restricted to the shapes it never
+    // resolves — literal IPs, `*`, and `*.` wildcards — to keep the target off
+    // the network. The resolving branch needs a stubbed resolver and is not
+    // covered here.
+    let ip_safe: Vec<String> = patterns
+        .iter()
+        .filter(|p| {
+            let t = p.trim();
+            t == "*" || t.starts_with("*.") || t.parse::<std::net::IpAddr>().is_ok()
+        })
+        .cloned()
+        .collect();
+    let ip_list = NetAllowlist::new(ip_safe.clone());
+    let wide_open = ip_safe.iter().any(|p| p.trim() == "*");
+
+    for candidate in ["127.0.0.1", "::1", "10.0.0.1", "not-an-ip"] {
+        assert!(
+            !empty.is_ip_allowed(candidate),
+            "empty allowlist accepted ip {candidate}"
+        );
+        let verdict = ip_list.is_ip_allowed(candidate);
+        if wide_open {
+            // `*` short-circuits before the IP is parsed, so even a
+            // non-address is allowed by it.
+            assert!(verdict, "`*` did not allow {candidate}");
+        } else if candidate == "not-an-ip" && !ip_safe.iter().any(|p| p.trim() == candidate) {
+            assert!(
+                !verdict,
+                "an unparseable address was allowed without a matching pattern"
+            );
+        }
     }
 });

crates/shadi_sandbox/src/net_proxy.rs

@@ -111,6 +111,18 @@ impl NetAllowlist {
         };
         is_host_allowed(host, &guard)
     }
+
+    /// Check whether a literal `ip` is permitted by the current list.
+    ///
+    /// Unlike [`Self::is_allowed`] this also matches an allowlisted hostname
+    /// that resolves to `ip`, so it can perform DNS lookups.
+    pub fn is_ip_allowed(&self, ip: &str) -> bool {
+        let guard = match self.0.read() {
+            Ok(g) => g,
+            Err(_) => return false,
+        };
+        is_ip_allowed(ip, &guard)
+    }
 }
 
 fn is_host_allowed(host: &str, list: &[String]) -> bool {
@@ -303,22 +315,68 @@ impl Drop for NetProxy {
 // Accept loop
 // ---------------------------------------------------------------------------
 
+/// Concurrent connection handlers the proxy will run at once.
+///
+/// One thread per accepted connection is unbounded: anything that can reach
+/// the port — the sandboxed process, or any process of the same user — can
+/// make the proxy spawn threads and, for IP-mode clients, drive a DNS lookup
+/// per allowlist hostname on each one. Past this many the proxy sheds load by
+/// closing the connection instead of queuing it, which a SOCKS5 client sees
+/// as a failed connect.
+pub const MAX_CONCURRENT_CONNECTIONS: usize = 64;
+
+/// Raises the in-flight connection count for as long as it is held.
+struct ConnectionPermit(Arc<std::sync::atomic::AtomicUsize>);
+
+impl ConnectionPermit {
+    fn acquire(counter: &Arc<std::sync::atomic::AtomicUsize>) -> Self {
+        counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
+        Self(Arc::clone(counter))
+    }
+}
+
+impl Drop for ConnectionPermit {
+    fn drop(&mut self) {
+        self.0.fetch_sub(1, std::sync::atomic::Ordering::SeqCst);
+    }
+}
+
 fn accept_loop(
     listener: TcpListener,
     allowlist: NetAllowlist,
     stop: Arc<std::sync::atomic::AtomicBool>,
 ) {
     listener.set_nonblocking(false).ok();
+    let in_flight = Arc::new(std::sync::atomic::AtomicUsize::new(0));
     loop {
         match listener.accept() {
             Ok((stream, _peer)) => {
                 if stop.load(std::sync::atomic::Ordering::SeqCst) {
                     break;
                 }
+                if in_flight.load(std::sync::atomic::Ordering::SeqCst)
+                    >= MAX_CONCURRENT_CONNECTIONS
+                {
+                    warn!(
+                        "net proxy: {} connections in flight, shedding",
+                        MAX_CONCURRENT_CONNECTIONS
+                    );
+                    drop(stream);
+                    continue;
+                }
                 let al = allowlist.clone();
+                // Held by the handler, so the count falls however the handler
+                // ends — including a spawn that never runs it, which drops the
+                // closure and the permit with it. A hand-rolled decrement
+                // needs a branch per exit and leaks the cap downward if one
+                // is missed.
+                let permit = ConnectionPermit::acquire(&in_flight);
                 thread::Builder::new()
                     .name("shadi-proxy-conn".into())
-                    .spawn(move || handle_connection(stream, al))
+                    .spawn(move || {
+                        let _permit = permit;
+                        handle_connection(stream, al);
+                    })
                     .ok();
             }
             Err(ref e) if e.kind() == std::io::ErrorKind::ConnectionAborted => continue,
@@ -803,6 +861,101 @@ mod tests {
         assert_eq!(&buf, b"hello");
     }
 
+    #[test]
+    fn is_ip_allowed_matches_literals_and_denies_by_default() {
+        // Only literal IPs and `*` here: a hostname pattern would send
+        // is_ip_allowed to the resolver, and a unit test must not do DNS.
+        let empty = NetAllowlist::new(vec![]);
+        assert!(!empty.is_ip_allowed("127.0.0.1"), "empty list is deny-all");
+        assert!(!empty.is_ip_allowed("not-an-ip"));
+
+        let literal = NetAllowlist::new(vec!["127.0.0.1".into(), "10.0.0.7".into()]);
+        assert!(literal.is_ip_allowed("127.0.0.1"));
+        assert!(literal.is_ip_allowed("10.0.0.7"));
+        assert!(!literal.is_ip_allowed("10.0.0.8"));
+
+        // `*` short-circuits before the address is parsed, so it allows even
+        // a value that is not an address.
+        let open = NetAllowlist::new(vec!["*".into()]);
+        assert!(open.is_ip_allowed("127.0.0.1"));
+        assert!(open.is_ip_allowed("not-an-ip"));
+
+        // A `*.` pattern cannot be resolved to a fixed address, so it never
+        // matches an IP on its own.
+        let wildcard = NetAllowlist::new(vec!["*.example.com".into()]);
+        assert!(!wildcard.is_ip_allowed("127.0.0.1"));
+    }
+
+    #[test]
+    fn is_ip_allowed_denies_when_the_lock_is_poisoned() {
+        // A poisoned allowlist must fail closed, like is_allowed does: a
+        // panic while the list was held cannot become permission to connect.
+        let list = NetAllowlist::new(vec!["127.0.0.1".into()]);
+        assert!(list.is_ip_allowed("127.0.0.1"), "sanity before poisoning");
+
+        let poisoner = list.clone();
+        let _ = std::thread::spawn(move || {
+            let _held = poisoner.0.write().unwrap();
+            panic!("poison the allowlist lock");
+        })
+        .join();
+
+        assert!(
+            list.0.read().is_err(),
+            "the lock should be poisoned for this test to mean anything"
+        );
+        assert!(
+            !list.is_ip_allowed("127.0.0.1"),
+            "a poisoned allowlist allowed a connection"
+        );
+    }
+
+    #[test]
+    fn is_ip_allowed_accepts_ipv6_literals() {
+        let list = NetAllowlist::new(vec!["::1".into()]);
+        assert!(list.is_ip_allowed("::1"));
+        assert!(!list.is_ip_allowed("::2"));
+    }
+
+    #[test]
+    fn proxy_sheds_load_past_the_concurrency_cap() {
+        let _guard = lock_proxy_ports();
+        // Deny everything: handle_connection still reads the greeting, so each
+        // held connection occupies a handler thread without needing upstream.
+        let proxy = NetProxy::start(NetAllowlist::new(vec![])).unwrap();
+        let port = proxy.port();
+
+        // Open the cap's worth of connections and leave them mid-handshake,
+        // so every handler thread is parked on a read.
+        let mut held = Vec::new();
+        for i in 0..MAX_CONCURRENT_CONNECTIONS {
+            let conn = std::net::TcpStream::connect(format!("127.0.0.1:{port}"))
+                .unwrap_or_else(|e| panic!("proxy refused connection {i} below its own cap: {e}"));
+            held.push(conn);
+        }
+
+        // Give the accept loop time to spawn a handler for each.
+        std::thread::sleep(std::time::Duration::from_millis(200));
+
+        // One more: the proxy accepts the TCP connection (the listener backlog
+        // does that) and then closes it without a SOCKS5 reply. A client sees
+        // EOF rather than a greeting response.
+        let mut extra = std::net::TcpStream::connect(format!("127.0.0.1:{port}"))
+            .expect("listener still accepts");
+        extra.write_all(&[5, 1, 0]).ok();
+        extra
+            .set_read_timeout(Some(std::time::Duration::from_secs(2)))
+            .ok();
+        let mut reply = [0u8; 2];
+        let read = extra.read(&mut reply);
+        assert!(
+            matches!(read, Ok(0)) || read.is_err(),
+            "a shed connection answered the greeting: {read:?}"
+        );
+
+        drop(held);
+    }
+
     #[test]
     fn proxy_restart_rebinds_to_same_port() {
         // Serialize against other tests that bind port 0 so the OS cannot