# Punchnet (sdlan-lib-rs) Code Review Report

This report provides a detailed, comprehensive review of the `punchnet` SD-LAN (Software-Defined LAN / VPN) client repository. It covers the overall architecture, highlights key design strengths, details security and code quality risks, and suggests practical refactoring opportunities.

---

## 1. Repository Overview & Architecture

`punchnet` is a virtual networking client written in Rust designed to establish secure, peer-to-peer Software-Defined LAN connections. 

The architecture consists of two main planes:
1. **Control Plane (QUIC)**: Communicates with a centralized supernode over QUIC (via `quinn`) to handle registration (`RegisterSuper`), policy exchange, peer discovery, and NAT type discovery.
2. **Data Plane (UDP)**: Encrypted virtual network traffic is sent directly between peer nodes using UDP. It performs NAT traversal (using STUN-like hole punching) to establish direct P2P connections, falling back to relaying via the supernode if direct routing fails.

### Core Components

```mermaid
graph TD
    A[OS Virtual NIC: TUN/TAP] <-->|Read/Write L3/L2 Packets| B[Packet Routing Engine: packet.rs]
    B <-->|Encrypt/Decrypt: AES or ChaCha20| C[UDP Socket Engine: socks.rs]
    D[Control Plane: quic.rs] -->|Session Keys / ACL Rules| B
    D <-->|QUIC Protocol| E[Supernode Server]
    C <-->|P2P Encrypted Data| F[Remote Peers]
    B -->|Local Domain Filter| G[DNS Hijacking Proxy: dns.rs]
    G <-->|Proxy Queries| H[AliDNS 223.5.5.5 / Supernode DNS]
```

*   **Platform-specific NIC Adapters** (`src/network/tun_win.rs` & `src/network/tun_linux.rs`): Handles raw packet read/write. Windows implements L3 TUN using the Wintun driver (relying on `wintun.dll`), simulating Ethernet frames (L2) locally. Linux binds directly to `/dev/net/tun` via `ioctl` (supported by helper C-code `tuntap.c`).
*   **Routing & Encapsulation Engine** (`src/network/packet.rs` & `src/network/route.rs`): Identifies packet destination MACs and resolves them to peer IP/ports using a custom ARP table. Packs IP/Ethernet payloads into protobuf messages (`SdlData`), encrypts them, and forwards them. Uses a custom lock-free IP Trie (`src/utils/system_action.rs`) for prefix route resolution.
*   **DNS Proxy Engine** (`src/utils/dns.rs`): Automatically intercepts DNS queries. Queries for private virtual domains (e.g. `*.network_domain`) are sent to the supernode's DNS server (`server_ip:15353`), while other public queries are proxied to AliDNS (`223.5.5.5:53`) over dynamically rotated UDP sockets.

---

## 2. Key Design Highlights (Strengths)

*   **Lock-Free Concurrent Routing Lookups**:
    Outbound lookups on the routing table (`RouteTable2`) happen on every single packet. The project handles this very efficiently by implementing an `IpTrie` wrapped in an `ArcSwap`. Write operations (inserting or clearing routes) clone the trie and perform an atomic pointer swap, enabling read threads to query routes concurrently without locking.
*   **Dynamic DNS Socket Rotation**:
    To prevent firewalls or stateful NAT routers from blocking persistent DNS proxy connections, `DynamicDNSClient` rotates its local UDP socket either every 60 seconds or after 100 queries. It spawns a new receiver thread on a fresh port and allows the old socket to time out and close gracefully, which is a highly resilient approach to DNS proxying.
*   **Clean Separation of Control and Data Planes**:
    Using QUIC for signaling and standard UDP for data plane routing provides a robust control connection (resilient to head-of-line blocking and network changes) while keeping the overhead of data forwarding as low as possible.

---

## 3. Security Vulnerabilities & Risks

> [!CAUTION]
> ### 1. ACL/Firewall Rules Completely Bypassed
> In [acl_session.rs](file:///d:/work/rust/sdlan-lib-rs/src/utils/acl_session.rs#L141-L146), the core firewall verification method `is_identity_ok` unconditionally returns `(true, false)`:
> ```rust
> pub fn is_identity_ok(&self, allow_routing: bool, identity: IdentityID, info: FiveTuple) -> (bool, ShouldRenew) {
>     if allow_routing {
>         return (true, false);
>     }
>     return (true, false);
> 
>     error!("is identity ok? {:?}", info);
>     // ... remaining validation logic is dead code ...
> }
> ```
> **Risk**: Any peer on the SD-LAN network can send traffic to any port of the client, bypassing all security group policies pushed from the supernode server.

> [!WARNING]
> ### 2. Hardcoded Signature Key for HTTP API
> In [api/mod.rs](file:///d:/work/rust/sdlan-lib-rs/src/bin/punchnet/api/mod.rs#L10), the signing key for REST APIs is hardcoded:
> ```rust
> const DIGEST_KEY: &'static str = "H6p*2RfEu4ITcL";
> ```
> **Risk**: This key is used to sign requests sent to the central controller via HMAC-MD5 (sent as `X-sign`). Since the key is hardcoded in the client, an attacker could reverse-engineer the binary, retrieve the key, and forge authenticated API requests.

> [!WARNING]
> ### 3. Weak Directory Permissions for Local Credentials
> In [node.rs](file:///d:/work/rust/sdlan-lib-rs/src/network/node.rs#L64-L73) and [main.rs](file:///d:/work/rust/sdlan-lib-rs/src/bin/punchnet/main.rs#L344), the client directory `.client` is created inside the base directory to hold the generated RSA key pair (`id_rsa`/`id_rsa.pub`). The code does not restrict folder permissions on Unix.
> **Risk**: If the app is run as root (needed to configure TUN/TAP) and saves files in a shared base directory (e.g. `/usr/local/punchnet`), other non-root local users might be able to read the private key file `id_rsa` if the default `umask` is weak.

---

## 4. Code Quality & Potential Bugs

### 1. DNS Settings Corruption on Improper Exit
In [main.rs](file:///d:/work/rust/sdlan-lib-rs/src/bin/punchnet/main.rs#L268-L280), the code restores system DNS settings on `ctrl_c` by invoking `restore_dns`.
*   **Bug**: If the process crashes, panics, is killed via `kill -9` (`SIGKILL`), or system shutdown happens without triggering the tokio runtime signal handler, the system's DNS settings (e.g., `/etc/resolv.conf` on Linux or static DNS addresses on Windows) will remain pointed to the hijacked address `100.100.100.100`. This will break the host machine's internet connectivity until manually fixed.

### 2. PID File Race Conditions & False Positives
In [main.rs](file:///d:/work/rust/sdlan-lib-rs/src/bin/punchnet/main.rs#L633-L645), `is_process_running()` checks for the PID file `/tmp/punchnet.pid` and verifies whether that process ID exists in the system:
```rust
fn is_pid_running(pid: u32) -> bool {
    let mut sys = sysinfo::System::new_all();
    sys.refresh_all();
    sys.process(sysinfo::Pid::from_u32(pid)).is_some()
}
```
*   **Bug**: If `punchnet` crashes, the PID file is left over. If another completely unrelated application starts later and gets assigned the exact same process ID, `punchnet` will refuse to launch, erroneously reporting `"process is running, aborting..."`. It is safer to verify if the process name matches `"punchnet"` or check the binary executable path.

### 3. Blocking Operations on Async Thread Pool
In `async_main.rs` and `node.rs`, there are multiple instances where filesystem operations (`fs::read_to_string`, `fs::write`) or platform calls (`Command::new("netsh")`, `Command::new("route")`) are executed synchronously within async tokio contexts instead of using `tokio::fs` or `spawn_blocking`.
*   **Performance Impact**: These blocking system operations can stall the Tokio worker threads, reducing throughput and leading to increased latency in packet forwarding.

---

## 5. Suggestions for Improvement & Refactoring

1.  **Fix the ACL Shortcut**:
    Remove the early `return (true, false)` from `is_identity_ok` in `src/utils/acl_session.rs` and fully integrate the rules received from the server to secure the client against unauthorized peer access.
2.  **Move Secret Keys to Environment/Config**:
    Remove `DIGEST_KEY` from source files. Instead, use a key derivation mechanism or load it from a secure local environment variable or configuration file.
3.  **Ensure Atomic & Safe File Permissions**:
    When creating folders or writing key pairs on Unix, restrict the directory permissions explicitly to `0700` and files to `0600` using standard library extensions:
    ```rust
    #[cfg(unix)]
    {
        use std::os::unix::fs::DirBuilderExt;
        let mut builder = std::fs::DirBuilder::new();
        builder.mode(0o700);
        builder.create(rsa_path)?;
    }
    ```
4.  **Use Subprocess Wrappers and Async Routines**:
    Migrate blocking file I/O and process spawns to async counterparts (e.g. `tokio::fs` and `tokio::process::Command`) to prevent worker thread starvation.
5.  **Graceful Recovery of System DNS Settings**:
    Consider writing a secondary daemon / sentinel wrapper, or register a shell-level script trap to ensure that system DNS settings are automatically restored even under fatal panics/crashes.