feat: transparent DNS interception via in-namespace server #56
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Replace resolv.conf manipulation with transparent nftables redirect for DNS traffic. All UDP port 53 traffic is now intercepted via DNAT rules, similar to HTTP/HTTPS. Changes: - Add configurable DNS proxy port (default 8053) to JailConfig - Implement nftables DNAT rules for UDP port 53 redirection - Remove all resolv.conf modification logic - DNS server now binds to configured proxy port instead of port 53 This approach is more reliable and truly transparent - DNS queries work regardless of the nameserver configuration in resolv.conf. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…direct The nftables DNAT rules don't work properly for localhost destinations (127.0.0.x). To work around this limitation while maintaining transparent DNS redirect: - Create a minimal resolv.conf pointing to 8.8.8.8 (non-localhost) - All DNS queries to ANY address still get transparently redirected to our proxy - This avoids the systemd-resolved localhost issue without losing transparency
CLAUDE.md specifies to prefer pure Rust solutions over shell script wrappers for tests
Replace host-side DNS proxy with a forked DNS server running directly inside the network namespace. This provides truly transparent DNS interception without any resolv.conf modifications. Key changes: - Fork DummyDnsServer into namespace on 127.0.0.1:53 - DNAT rules redirect ALL DNS traffic to localhost:53 - Remove dns_proxy_port configuration (always use standard port 53) - Remove all resolv.conf modification logic - Add robust cleanup with PR_SET_PDEATHSIG and pipe monitoring Architecture: 1. Child process forks and enters namespace via setns() 2. Drops privileges to nobody:nogroup for security 3. Binds to 127.0.0.1:53 to catch all DNS traffic 4. DNAT rule: "udp dport 53 dnat to 127.0.0.1:53" 5. Works with ANY resolv.conf (127.0.0.53, 8.8.8.8, etc.) This approach is simpler and more reliable than the previous host-side proxy approach, as it reuses the existing DummyDnsServer without modification and requires no filesystem changes. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Fix compilation error on Linux by using the fully qualified anyhow::bail! macro
Convert OwnedFd to RawFd for compatibility with libc functions. Also remove unused Arc and Mutex imports.
- Move status variable declaration outside loop for proper scope - Use libc::read directly instead of nix for child process - Properly manage file descriptor lifetime with std::mem::forget
- Bring up loopback interface after entering namespace - Bind to 0.0.0.0:53 instead of 127.0.0.1:53 to avoid binding issues - This should fix the 'Failed to bind DNS server' error in tests
Add documentation about testing Linux strong jail changes directly on the CI machine for faster iteration, avoiding the CI queue.
ammario changed the title With transparent DNS interception via in-namespace server, the fix_systemd_resolved_dns function and NamespaceConfig resource are no longer needed. DNS queries are now intercepted at the network level via nftables DNAT rules, eliminating the need for resolv.conf modifications.
Moved the janky forked DNS server implementation from mod.rs into a proper NamespaceDnsServer struct in dns.rs. This provides better encapsulation and cleaner separation of concerns. Key changes: - Created NamespaceDnsServer struct in dns.rs to handle all forking/namespace logic - Simplified LinuxJail to just use NamespaceDnsServer instead of raw pid/fd handling - Automatic cleanup via Drop trait implementation - All tests continue to pass on both macOS (weak mode) and Linux (strong jail) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
The 'Faster Linux Strong Jail Debugging' section was redundant with the existing 'Manual Testing on CI' section - both described the same process of testing on the CI machine before pushing.
Fixed compilation error where Clone impl was still using the old dns_forwarder_pid and dns_cleanup_pipe fields instead of the new dns_server field.
The forked DNS server process can receive signals which cause recv_from() to return EINTR (Interrupted system call). This is normal and should be retried rather than treated as a fatal error.
The DNS server fork() was happening within a tokio runtime context, which could potentially cause issues with multi-threaded state. Added: 1. Comprehensive safety documentation explaining why current approach is safe 2. Close all unnecessary file descriptors (3-1024) in child process to prevent inheriting tokio's epoll/kqueue fds and other parent resources 3. Only keep stdin/stdout/stderr and our monitoring pipe in the child This ensures clean separation between parent tokio runtime and forked child.
…rate Major improvements to DNS process management: 1. **Better naming**: Renamed NamespaceDnsServer to ForkedDnsProcess to clarify that it manages a forked process, not a server itself 2. **Safer system calls**: Replaced raw libc calls with nix crate wrappers: - nix::unistd::fork() with proper ForkResult enum - nix::sys::prctl::set_pdeathsig() instead of raw prctl - nix::sys::signal::kill() instead of libc::kill - nix::sys::wait::waitpid() instead of libc::waitpid - nix::unistd::setuid/setgid instead of raw libc calls - nix::sched::setns() instead of libc::setns - nix::unistd::pipe2() with O_CLOEXEC flag 3. **Better cleanup**: OwnedFd automatically closes on drop, simplifying lifecycle All tests continue to pass on both macOS and Linux.
The pipe-based parent monitoring was completely redundant since we already use PR_SET_PDEATHSIG on Linux, which causes the kernel to send SIGTERM to the child when the parent dies. Simplified the implementation by: - Removing the pipe creation and OwnedFd field - Removing the monitoring loop that checked for EOF - Using a simple sleep loop instead (process exits via SIGTERM) - Simplifying both parent and child code paths This reduces complexity and removes unnecessary system calls while maintaining the same reliability - the child process is still guaranteed to terminate when the parent dies. All 23 Linux integration tests continue to pass.
Updated the DNS Exfiltration Protection section to reflect the current ForkedDnsProcess implementation: 1. Clarified the architecture with numbered steps showing DNS server fork 2. Added expandable Technical Implementation Details section 3. Updated Linux architecture diagram to show forked DNS server 4. Documented key implementation features: - Fork safety and FD cleanup - Privilege dropping to nobody:nogroup - PR_SET_PDEATHSIG for automatic cleanup - Pure Rust implementation with simple-dns The verbose technical details are now in a collapsible <details> section to keep the main documentation concise while still providing depth for those who need it.
…t cleanup requirements Added proper cleanup for orphaned DNS server processes that could be left running if httpjail crashes. The cleanup_orphaned() function now: 1. Lists all PIDs in the namespace using 'ip netns pids' 2. Kills all processes found (including the DNS server) 3. Then proceeds with normal resource cleanup Also added comprehensive documentation in CLAUDE.md about system resource cleanup requirements: - Listed all system resources that must be cleaned up - Documented cleanup mechanisms (normal exit vs orphan cleanup) - Added implementation requirements for new resources - Included testing instructions for verifying cleanup This ensures no resources are leaked even if httpjail is killed with SIGKILL.
- Remove explicit orphan process killing from cleanup_orphaned, relying on PR_SET_PDEATHSIG - Add TODO comment for future implementation of orphan process cleanup - Simplify ForkedDnsProcess::stop to just SIGKILL since child only sleeps
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
Comment on lines 166 to 244
| /// Start a DNS server in a forked process within the specified namespace | ||
| pub fn start(&mut self, namespace_name: &str) -> Result<()> { | ||
| use nix::unistd::ForkResult; | ||
| | ||
| info!("Starting in-namespace DNS server for {}", namespace_name); | ||
| | ||
| // SAFETY: While forking from a tokio runtime, the child immediately: | ||
| // - Closes unnecessary file descriptors | ||
| // - Sets PR_SET_PDEATHSIG for automatic cleanup when parent dies | ||
| // - Never returns to Rust/tokio code | ||
| // - Runs until terminated by signal | ||
| match unsafe { nix::unistd::fork() }.context("Failed to fork DNS process")? { | ||
| ForkResult::Child => { | ||
| // Child process: Run DNS server in namespace | ||
| | ||
| // Close all unnecessary file descriptors inherited from parent | ||
| // Keep only stdin(0), stdout(1), stderr(2) | ||
| for fd in 3..1024 { | ||
| let _ = nix::unistd::close(fd); | ||
| } | ||
| | ||
| // Request SIGTERM when parent dies | ||
| #[cfg(target_os = "linux")] | ||
| { | ||
| use nix::sys::signal::Signal; | ||
| if let Err(e) = nix::sys::prctl::set_pdeathsig(Signal::SIGTERM) { | ||
| eprintln!("Failed to set parent death signal: {}", e); | ||
| std::process::exit(1); | ||
| } | ||
| } | ||
| | ||
| // Check if parent already died | ||
| if nix::unistd::getppid() == nix::unistd::Pid::from_raw(1) { | ||
| std::process::exit(1); | ||
| } | ||
| | ||
| // Enter the namespace | ||
| let ns_path = format!("/var/run/netns/{}", namespace_name); | ||
| let ns_fd = std::fs::File::open(&ns_path).unwrap_or_else(|e| { | ||
| eprintln!("Failed to open namespace {}: {}", ns_path, e); | ||
| std::process::exit(1); | ||
| }); | ||
| | ||
| // Use nix for setns | ||
| nix::sched::setns(ns_fd.as_raw_fd(), nix::sched::CloneFlags::CLONE_NEWNET) | ||
| .unwrap_or_else(|e| { | ||
| eprintln!("Failed to setns into {}: {}", namespace_name, e); | ||
| std::process::exit(1); | ||
| }); | ||
| | ||
| // Ensure loopback interface is up | ||
| std::process::Command::new("ip") | ||
| .args(["link", "set", "lo", "up"]) | ||
| .output() | ||
| .ok(); | ||
| | ||
| // Start DNS server on all interfaces (requires root for privileged port) | ||
| let mut server = DummyDnsServer::new(); | ||
| if let Err(e) = server.start("0.0.0.0:53") { | ||
| eprintln!("Failed to start DNS server: {}", e); | ||
| std::process::exit(1); | ||
| } | ||
| | ||
| // Drop privileges to nobody after binding | ||
| let nobody_uid = nix::unistd::Uid::from_raw(65534); | ||
| let nogroup_gid = nix::unistd::Gid::from_raw(65534); | ||
| | ||
| nix::unistd::setgroups(&[]).ok(); | ||
| nix::unistd::setgid(nogroup_gid).ok(); | ||
| nix::unistd::setuid(nobody_uid).ok(); | ||
| | ||
| info!("In-namespace DNS server listening on 0.0.0.0:53"); | ||
| | ||
| // The DNS server runs forever. When parent dies, kernel sends SIGTERM | ||
| // due to PR_SET_PDEATHSIG, which terminates this process cleanly. | ||
| // Block here to keep the DNS server thread running. | ||
| loop { | ||
| std::thread::sleep(std::time::Duration::from_secs(3600)); | ||
| } |
There was a problem hiding this comment.
[P1] Avoid stdlib work after fork in multithreaded runtime
The child branch after fork() performs logging, opens files, spawns a thread, and invokes ip via Command. In a Tokio-based process this is unsafe because only async-signal-safe calls are guaranteed to work between fork and exec; if another thread held a mutex in the allocator or tracing subscriber at the time of the fork, these calls can deadlock the child and leave DNS interception broken. Consider execing a helper binary or starting the DNS server without forking from the fully initialised runtime.
Useful? React with 👍 / 👎.
…mination The DummyDnsServer was being dropped when the server variable went out of scope, causing its Drop implementation to stop the DNS thread. This made all Linux integration tests timeout as DNS wasn't actually running.
The previous approach of using DummyDnsServer with thread spawning was causing the DNS server to terminate when the struct was dropped. Now the DNS server runs directly in the forked child process, ensuring it stays alive.
Remove the extra function call and thread complexity. The DNS server now runs directly in the forked child process with a simple recv/send loop.
The child process now immediately execs itself with a special flag (--__internal-dns-server) instead of performing unsafe operations like opening files, allocating memory, or spawning threads after fork(). This prevents potential deadlocks when forking from a tokio runtime where other threads might hold locks in the allocator or other subsystems. The DNS server functionality is moved to a separate code path that runs after exec in a clean process environment.
- Moved run_namespace_dns_server() to dns.rs as run_exec_dns_server() - Moved build_dns_response() to dns.rs as build_dummy_response() - Added proper imports to dns.rs including DUMMY_IPV4 constant - Updated main.rs to call the DNS function from the dns module - Fixed test module to work with the refactored code This provides better separation of concerns with all DNS logic in one file.
- Use std::fs::OpenOptions instead of std::os::unix::fs::OpenOptions - Add QTYPE and QCLASS imports - Fix type comparisons using QTYPE::TYPE() and QCLASS::CLASS() wrappers
The libc::setns call was hanging in the DNS server. Using the nix crate's setns function properly handles the file descriptor and provides better error handling.
The nft delete commands could hang indefinitely when trying to delete non-existent tables or when there's lock contention. This was causing hundreds of stuck processes on CI. - Add 5-second timeout using 'timeout' command wrapper - Handle timeout exit code (124) gracefully - Log timeout events for debugging This prevents resource leaks and test hangs on CI.
…nt DNS server leaks The parent death signal (PR_SET_PDEATHSIG) was being cleared when the DNS server dropped privileges via setuid/setgid. This is documented Linux kernel behavior - the death signal is cleared on credential changes for security reasons. The fix re-establishes PR_SET_PDEATHSIG after calling setuid(), ensuring the DNS server receives SIGTERM when its parent process dies. This prevents orphaned DNS processes from accumulating during test runs or when httpjail is terminated. Verified on ci-1 with stress testing: - 50 normal terminations: 0 leaks - 30 timeout terminations: 0 leaks - Integration tests pass without hanging
These aggressive cleanup scripts could interfere with concurrent test runs. The DNS server leak has been properly fixed at the source, making these workarounds unnecessary. Removed: - .github/workflows/test-with-cleanup.yml - scripts/ci-cleanup.sh - scripts/monitor-ci.sh - src/jail/ci_config.rs
Replace external 'timeout' command with native Rust timeout implementation. This provides better control and avoids dependency on external commands. Changes: - Add command_utils module with execute_with_timeout_poll function - Replace 'timeout' command usage in nftables.rs and docker.rs - Use 5-second timeout for all nft delete operations - Handle timeout with proper exit code (124) matching GNU timeout behavior This prevents nft delete commands from hanging indefinitely when nftables is in an inconsistent state or under heavy load.
Replace polling-based timeout implementation with cleaner tokio approach using block_in_place and async/await patterns. Benefits: - No manual polling or thread sleeping - Leverages tokio's efficient async I/O and timers - Automatic child process cleanup with kill_on_drop(true) - Works correctly in both sync and async contexts - Cleaner, more idiomatic Rust code Technical details: - Uses tokio::task::block_in_place when in async context - Falls back to creating temporary runtime when needed - Maintains backward compatibility with execute_with_timeout_poll alias - Tests updated to use multi-threaded runtime for block_in_place
Resolved conflict in README.md by adopting the new documentation structure from main which moves detailed content to the docs/ directory while keeping a simplified README with links.
…imeout handling - Introduced CommandResult enum that combines success, timeout, and error cases - Eliminated double nesting (was Result<CommandOutput>, now just CommandResult) - Removed magic exit code 124 and platform-specific exit status creation - Updated all call sites to pattern match on CommandResult directly - Added comprehensive helper methods for type checking and data access - Added test coverage for command spawn errors
- Add proper error checking for privilege dropping (setuid/setgid) - Validate namespace names to prevent injection attacks - Implement packet size limits (4KB max) and question limits (10 max) - Use safe file descriptor closure with close_range() syscall - Add dynamic UID/GID lookup for nobody user with fallbacks - Fix race condition in parent death signal handling - Add comprehensive input validation and DoS protection Security improvements address: - Potential privilege escalation if setuid/setgid fails - Command injection via crafted namespace names - DoS attacks via oversized packets or excessive questions - Resource leaks from improper FD handling - Orphaned processes after parent crash Testing: Verified on Linux CI with all integration tests passing 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Summary
Problem
Previously, httpjail modified
/etc/resolv.confin network namespaces to point DNS queries to our dummy DNS server. This approach had several limitations:Solution
This PR implements a cleaner approach using an in-namespace DNS server:
setns()Architecture
Key Changes
1. In-Namespace DNS Server (
src/jail/linux/mod.rs)2. Simplified nftables Rules (
src/jail/linux/nftables.rs)3. Removed Components
Benefits
Testing
✅ All Linux integration tests passing
✅ DNS resolution test specifically validated
✅ Tested directly on CI machine with various configurations
✅ Works with systemd-resolved (127.0.0.53), traditional (127.0.0.1), and public DNS (8.8.8.8)
Technical Details
Cleanup Mechanisms
Why This Works
🤖 Generated with Claude Code