feat(minimald,minimal2): HTTPS reverse proxy with mTLS auth and ssh-forward CLI - #554

crates/minimald/src/net/mod.rs (1)

407-410: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid synchronous filesystem I/O on async control paths.

std::fs::* in attach/ensure_running/stop can block Tokio worker threads during attach/teardown bursts. Prefer tokio::fs (or spawn_blocking) here.

#!/bin/bash
# Verify sync filesystem calls inside async functions in this file.
rg -n --type=rust -C 3 'async fn (attach|ensure_running|stop)|std::fs::(create_dir_all|write|remove_file)' crates/minimald/src/net/mod.rs

Suggested patch

-    pub async fn attach(&mut self) -> Result<AttachResult, NetError> {
+    pub async fn attach(&mut self) -> Result<AttachResult, NetError> {
         let lease = self.allocator.allocate()?;
-        self.write_config()?;
+        self.write_config().await?;
         self.ensure_running().await?;
         self.attached += 1;
         ...
     }

-    fn write_config(&self) -> Result<(), NetError> {
-        std::fs::create_dir_all(&self.state_dir).map_err(|source| NetError::WriteConfig {
+    async fn write_config(&self) -> Result<(), NetError> {
+        tokio::fs::create_dir_all(&self.state_dir).await.map_err(|source| NetError::WriteConfig {
             path: self.state_dir.clone(),
             source,
         })?;
         let path = self.config_path();
         let body = render_gvproxy_config(self.allocator.subnet(), self.allocator.leases());
-        std::fs::write(&path, body).map_err(|source| NetError::WriteConfig { path, source })
+        tokio::fs::write(&path, body).await.map_err(|source| NetError::WriteConfig { path, source })
     }

-        match std::fs::remove_file(&sock) {
+        match tokio::fs::remove_file(&sock).await {
             Ok(()) => {}
             Err(e) if e.kind() == io::ErrorKind::NotFound => {}
             Err(e) => return Err(NetError::Io(e)),
         }

-        let _ = std::fs::remove_file(self.control_socket());
+        let _ = tokio::fs::remove_file(self.control_socket()).await;
         Ok(())
     }

Also applies to: 437-445, 482-485, 595-595

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/net/mod.rs` around lines 407 - 410, Replace all
synchronous filesystem I/O operations in the async functions `attach`,
`ensure_running`, and `stop` with their asynchronous equivalents from the
`tokio::fs` module. Specifically, convert `std::fs::create_dir_all`,
`std::fs::write`, and `std::fs::remove_file` calls to
`tokio::fs::create_dir_all`, `tokio::fs::write`, and `tokio::fs::remove_file`
respectively, and await each of these async operations. If there are filesystem
operations without direct tokio equivalents, wrap them using
`tokio::task::spawn_blocking` to prevent blocking the Tokio worker threads
during attach/teardown operations.

crates/minimald/src/net/proxy.rs (1)

364-391: 🩺 Stability & Availability | 🔵 Trivial

The ephemeral CA behavior is already documented — consider clarifying it at the function definition level.

The code correctly implements an in-memory CA that is regenerated at daemon startup (no persistence). This is already acknowledged in ServerState in server.rs (lines 100–101): "daemon's lifetime; clients must call minimal login again after a restart."

The implication you identified is accurate: every restart invalidates previously-issued client certs and rotates the server cert, requiring users to re-run minimal login. This is intentional MVP behavior.

However, the doc comment on CertAuthority::generate() (lines 355–362) currently describes only the technical details (ECDSA P-256, localhost SAN) without mentioning the ephemeral nature. Consider expanding the doc comment to clarify: "This CA exists only for the lifetime of the daemon and is regenerated on restart, invalidating any previously-issued client certificates."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/net/proxy.rs` around lines 364 - 391, The doc comment for
the CertAuthority::generate() function currently only describes the technical
implementation details (ECDSA P-256 key, localhost SAN) but omits documentation
about the ephemeral nature of the CA. Expand the doc comment for the generate()
method to clarify that the CA exists only for the lifetime of the daemon, is
regenerated on each restart, and that any previously-issued client certificates
become invalid after a daemon restart. This will make the MVP behavior explicit
to users and developers reading the code.

crates/minimal2/src/main.rs (2)

39-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Help text default path is Linux-specific.

The help text states the default is ~/.config/minimal/, but cmd_login resolves the directory via dirs::config_dir(), which on macOS returns ~/Library/Application Support. Since the daemon socket resolution in client.rs is cfg(target_os)-aware for both Linux and macOS, this static help/example will mislead macOS users. Consider phrasing it as "the platform config directory" or noting the macOS path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimal2/src/main.rs` around lines 39 - 73, The docstring for the
Login command variant contains hardcoded Linux-specific paths like
~/.config/minimal/ that do not match the actual behavior on macOS, where
dirs::config_dir() returns ~/Library/Application Support. Update the help text
and examples in the Login variant's docstring to indicate the paths are
platform-specific or use a generic term like "the platform config directory"
instead of hardcoding the Linux path, so that documentation accurately reflects
the behavior across both Linux and macOS systems.

---

556-570: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Forward-spec validation is effectively a no-op.

splitn(3, ':') only guarantees three colon-separated segments; forward_arg then reconstructs a string identical to args.forward, so this block adds no normalization. The ports are also never validated as numeric, so malformed input (e.g. abc:host:def) is silently forwarded to ssh -L, surfacing as an opaque ssh error. The comment about handling compact IPv4 forms is also misleading—nothing here distinguishes that case, and an IPv6 remote-host containing colons would be mis-split.

Consider validating local_port/remote_port parse as u16 and dropping the redundant reconstruction.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimal2/src/main.rs` around lines 556 - 570, The forward-spec
validation block is redundant and does not actually validate port numbers. The
code splits the forward spec into three parts and then reconstructs an identical
string in forward_arg, which provides no normalization. To fix this, parse
local_port and remote_port as u16 values to ensure they are valid numeric port
numbers, returning an error with a helpful message if parsing fails for either
port. Remove the forward_arg variable entirely since it is identical to
args.forward and serves no purpose. This will catch malformed input like
abc:host:def early with a clear error message instead of silently passing
invalid input to ssh.

crates/minimald/src/connection.rs (1)

464-518: 🩺 Stability & Availability | 🔵 Trivial

Upstream connect blocks the SSH connection handler, serializing message processing for up to 10 seconds.

The TcpStream::connect awaited in this handler will prevent the russh Handler from returning until the connection succeeds, times out, or fails. Since russh dispatches Handler methods as async futures that must complete before other messages are processed on the same connection, a slow or unreachable target stalls all concurrent channel operations (channel_open, requests, etc.) on that SSH session for the duration of the timeout. The impact is scoped to the single client, but multiplexed forwarding sessions could experience head-of-line blocking.

If you want to preserve the clean rejection behavior (avoiding accepting the channel then failing to connect), this is an acceptable tradeoff. Otherwise consider a shorter grace period or accepting optimistically and connecting in the spawned relay task.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/connection.rs` around lines 464 - 518, The
`channel_open_direct_tcpip` handler is blocking the SSH connection handler while
awaiting the upstream `TcpStream::connect` call with a 10-second timeout, which
serializes all message processing on that connection. Either reduce the grace
period by decreasing the `Duration::from_secs(10)` value to a shorter timeout,
or accept the channel optimistically without waiting for the upstream connection
to succeed and instead spawn the TCP connection logic in the relay task that
handles the actual data forwarding.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/minimald/src/net/mod.rs (1)

443-451: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Sync std::fs calls now run on the async runtime thread.

write_config is invoked from the async attach (Line 415), and the same conversion lands in ensure_running (Line 488) and stop (Line 601). Replacing tokio::fs with std::fs performs create_dir_all/write/remove_file as blocking syscalls on a tokio worker thread, which can stall other tasks on that thread. For these small local files the stall is typically brief, but it is a resilience regression versus the prior non-blocking implementation. Consider reverting to tokio::fs for the async paths or wrapping the work in tokio::task::spawn_blocking.

Please confirm whether attach/ensure_running/stop sit on a latency-sensitive path where blocking the executor would be observable (e.g. many concurrent attaches on a shared runtime).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/net/mod.rs` around lines 443 - 451, The write_config
method uses blocking std::fs operations (create_dir_all and write) which run on
the async runtime thread when called from async functions like attach,
ensure_running, and stop. This can stall other tasks on the same executor
thread. Either replace the std::fs::create_dir_all and std::fs::write calls in
write_config with their tokio::fs equivalents and make write_config an async
function, or alternatively wrap the entire std::fs work block in
tokio::task::spawn_blocking to offload the blocking operations to a dedicated
thread pool. Choose based on whether attach, ensure_running, and stop are on
latency-sensitive paths where blocking the executor would cause observable
stalls.

crates/minvmd/src/net.rs (1)

95-97: 🎯 Functional Correctness | 🔵 Trivial

Restore prefix validation in SwitchSubnet::new to prevent silent MAC collisions and misconfiguration.

SwitchSubnet::new in minvmd is infallible and accepts any prefix, diverging from the sister module minimald where it validates prefixes within 8..=29 via NetError::InvalidPrefix. This creates two risks:

• MAC collisions: MacAddr::for_switch_ip uses only octets [1], [2], [3] (line 153); a prefix wider than /8 lets octet [0] vary uncovered by the derived MAC, producing silent collisions.
• Insufficient subnet space: A prefix narrower than /29 leaves no room for the four reserved addresses (network, gateway, host-alias, broadcast) plus a PTask. The default /16 is safe, but this public constructor no longer guards misconfiguration.

Consider restoring the checked constructor from minimald (returning Result<Self, NetError>), or documenting why minvmd intentionally diverges.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minvmd/src/net.rs` around lines 95 - 97, The `SwitchSubnet::new`
constructor currently accepts any prefix value without validation, which can
lead to MAC collisions when prefix is wider than /8 (since
MacAddr::for_switch_ip only uses octets [1], [2], [3]) and insufficient subnet
space when prefix is narrower than /29. Restore prefix validation by changing
the constructor to return Result<Self, NetError> instead of Self, and add
validation logic that rejects prefixes outside the range 8..=29, returning
NetError::InvalidPrefix for invalid values. This aligns the implementation with
the corresponding validation in the sister module minimald.

crates/minimald/src/connection.rs (1)

491-554: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

direct-tcpip authorization is fail-open and unscoped — arbitrary internal targets reachable.

Two problems compound here:

1. Validation is skipped entirely for non-UUID / missing usernames. The session check is nested inside if let Some(ref uname) and if let Ok(session_id) = SessionId::parse_str(uname). If username is None, or simply not a UUID, both blocks are skipped and the channel is accepted with no session check. An authenticated local client can connect with any arbitrary -l <non-uuid> and bypass the check completely. Security checks should fail closed.

2. No binding of target to the session. Even on the happy path only session existence is verified; host_to_connect/port_to_connect are never validated against the named session's switch address, so ssh -L can reach any TCP endpoint visible from the daemon's network namespace (R4.9). This extends the previously flagged concern with the specific bypass mechanism above.

🔒 Suggested fail-closed restructuring (target scoping still required)

-        // Validate the session identified by the SSH username (R4.9). The client
-        // passes the session UUID as `-l <uuid>` so the server can confirm the
-        // session exists before accepting the forward.
-        if let Some(ref uname) = username {
-            if let Ok(session_id) = SessionId::parse_str(uname) {
-                let mngr = serv.sessions_manager().await;
-                match mngr.get_session(SessionKeyPredicate::Id(session_id)).await {
-                    Ok(Some(_)) => {}
-                    Ok(None) => {
-                        tracing::warn!(
-                            %session_id,
-                            "direct-tcpip rejected: session not found"
-                        );
-                        return Ok(false);
-                    }
-                    Err(e) => {
-                        tracing::warn!(
-                            %session_id,
-                            error = %e,
-                            "direct-tcpip rejected: session lookup failed"
-                        );
-                        return Ok(false);
-                    }
-                }
-            }
-        }
+        // Validate the session identified by the SSH username (R4.9).
+        let Some(uname) = username else {
+            tracing::warn!("direct-tcpip rejected: no SSH username");
+            return Ok(false);
+        };
+        let Ok(session_id) = SessionId::parse_str(&uname) else {
+            tracing::warn!(value = %uname, "direct-tcpip rejected: username not a uuid");
+            return Ok(false);
+        };
+        let mngr = serv.sessions_manager().await;
+        let session_handle = match mngr.get_session(SessionKeyPredicate::Id(session_id)).await {
+            Ok(Some(h)) => h,
+            Ok(None) => {
+                tracing::warn!(%session_id, "direct-tcpip rejected: session not found");
+                return Ok(false);
+            }
+            Err(e) => {
+                tracing::warn!(%session_id, error = %e, "direct-tcpip rejected: session lookup failed");
+                return Ok(false);
+            }
+        };
+        // TODO(R4.9): also verify (host_to_connect, port_to_connect) is the
+        // switch address/port owned by `session_handle`, rejecting otherwise.

Confirm there is no host/port-to-session enforcement elsewhere that this handler relies on:

#!/bin/bash
# Look for any switch-address / allowed-target accessor on the session API.
rg -nP --type=rust -C3 '\b(switch_addr|switch_ip|switch_socket|allowed_(host|port|target)|forward)\b' crates/minimald/src
ast-grep outline crates/minimald/src/session.rs --view expanded 2>/dev/null | head -80

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/connection.rs` around lines 491 - 554, The direct-tcpip
authorization logic has two fail-open vulnerabilities. First, the session
validation inside the nested if blocks is entirely skipped when username is None
or cannot be parsed as a SessionId, allowing unauthenticated bypasses.
Restructure the validation so that if a username is provided, failure to parse
it as a valid SessionId causes the channel to be rejected rather than proceeding
to the TcpStream::connect call. Second, the code only checks that a session
exists but never validates whether the host_to_connect and port_to_connect are
actually authorized for that session. After successfully retrieving the session
in the mngr.get_session call, add validation to ensure the target host and port
match an allowed or expected address associated with that session (such as a
switch_addr property) before proceeding to establish the upstream connection.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes