added umask

code_review_report.md

@@ -0,0 +1,121 @@
+# 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.

src/bin/punchnet/main.rs

@@ -338,6 +338,11 @@ async fn login_with_token_or_user_pass(
 }
 
 fn main() {
+    #[cfg(not(target_os = "windows"))]
+    unsafe {
+        libc::umask(0o077);
+    }
+
     set_base_dir(DEFAULT_BASE_DIR);
     // let _guard = log::init_log(&format!("{}/.output", get_base_dir()));