From 7e6e6b0175572abd7c0f10c810d19f05f7eb1bcc Mon Sep 17 00:00:00 2001 From: codewiththiha Date: Wed, 23 Sep 2026 09:53:13 +0000 Subject: [PATCH] fix: repair linux database access, drop scan sudo, and improve responsiveness - share one sqlite database across sudo elevation on Linux and hand root files back - drop sudo requirement for scan and let save-defaults run without elevating - eliminate lag on candidate switching, key handling, and Ctrl+C shutdown - enforce CI warnings as errors and run checks across Linux and macOS - fix minor formatting inconsistencies in command summaries and doctor checks --- .github/workflows/ci.yml | 33 +++- Cargo.lock | 4 +- Cargo.toml | 10 +- README.md | 32 +++- RELEASE_NOTES.md | 75 ++++---- crates/vmate-cli/Cargo.toml | 3 + crates/vmate-cli/src/commands/all.rs | 16 +- crates/vmate-cli/src/commands/connect.rs | 16 +- crates/vmate-cli/src/commands/doctor.rs | 45 ++++- crates/vmate-cli/src/commands/recent.rs | 2 + crates/vmate-cli/src/commands/scan.rs | 25 ++- crates/vmate-cli/src/ui/connect.rs | 43 +++-- crates/vmate-cli/src/ui/term.rs | 52 +++++- crates/vmate-cli/tests/cli.rs | 17 ++ crates/vmate-cli/tests/scan.rs | 26 +++ crates/vmate-core/Cargo.toml | 3 + crates/vmate-core/src/connect/service.rs | 45 ++++- crates/vmate-core/src/db/pool.rs | 33 +++- crates/vmate-core/src/ovpn/process.rs | 14 ++ crates/vmate-core/src/paths.rs | 122 ++++++++++++- crates/vmate-core/src/settings.rs | 15 +- crates/vmate-core/src/system/killer.rs | 131 ++++++++++++-- crates/vmate-core/src/system/mod.rs | 11 +- crates/vmate-core/src/system/root.rs | 219 ++++++++++++++++++++++- 24 files changed, 863 insertions(+), 129 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d5ba13c..bd316ed 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,8 +2,11 @@ # # The README documents `cargo fmt --check`, `cargo clippy --all-targets # -- -D warnings`, and `cargo test` as the quality bar; this job enforces it -# so nothing lands that doesn't pass its own checks. Releases are handled -# separately by `release.yml` (tag-triggered). +# so nothing lands that doesn't pass its own checks. Warnings are errors two +# ways: clippy runs with `-D warnings` (so a rustc warning fails too), and the +# workspace lints table promotes the rustc `unused` group to `deny`, so even a +# plain `cargo check` — the step below — fails on dead code and unused imports. +# Releases are handled separately by `release.yml` (tag-triggered). name: ci @@ -17,8 +20,16 @@ permissions: jobs: check: - name: fmt + clippy + test - runs-on: ubuntu-latest + name: ${{ matrix.os }} + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + # Linux and macOS both: vmate-cli ships for both, and they differ + # exactly where it hurts (sudo resets HOME on Linux, process groups and + # `killall` behave differently), so a green Linux alone proves little. + os: [ubuntu-latest, macos-latest] + steps: - uses: actions/checkout@v4 @@ -31,8 +42,16 @@ jobs: - name: Format run: cargo fmt --check - - name: Clippy - run: cargo clippy --workspace --all-targets -- -D warnings + - name: Clippy (warnings as errors) + run: cargo clippy --workspace --all-targets --all-features --locked -- -D warnings + + - name: Warnings (workspace lints deny unused code and unsafe) + run: cargo check --workspace --all-targets --locked + + - name: Docs (broken links as errors) + env: + RUSTDOCFLAGS: -D warnings + run: cargo doc --workspace --no-deps --locked - name: Test - run: cargo test --workspace + run: cargo test --workspace --locked diff --git a/Cargo.lock b/Cargo.lock index c6f1007..a506ac2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2995,7 +2995,7 @@ checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" [[package]] name = "vmate-cli" -version = "1.2.0" +version = "1.2.1" dependencies = [ "anyhow", "arboard", @@ -3023,7 +3023,7 @@ dependencies = [ [[package]] name = "vmate-core" -version = "1.2.0" +version = "1.2.1" dependencies = [ "anyhow", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index 9f362c7..b0dd167 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,10 +8,18 @@ members = [ [workspace.package] edition = "2024" rust-version = "1.85" -version = "1.2.0" +version = "1.2.1" license = "MIT" authors = ["codewiththiha"] +# Warnings are errors for everything in this workspace (never for +# dependencies). `unused` covers unused imports/variables and dead code, so a +# plain `cargo check` — not just clippy — fails on them, and `unsafe_code` +# keeps the process/signal code honest. +[workspace.lints.rust] +unused = "deny" +unsafe_code = "deny" + [workspace.dependencies] # Async runtime. Deliberately NOT `full`: vmate only needs process spawning, # timers, signals, mpsc, DNS lookup, and the runtime itself — trimming the diff --git a/README.md b/README.md index c125fe7..a88aba5 100644 --- a/README.md +++ b/README.md @@ -31,9 +31,10 @@ from a keyboard-driven terminal UI backed by SQLite. - Rust **1.85+** (edition 2024) - [OpenVPN](https://openvpn.net/) — `openvpn` on `PATH`, or pass `--openvpn-bin` -- Root/sudo for `scan`, `connect` and `all` (vmate-cli re-executes under `sudo` - automatically on an interactive terminal; set `VMATE_NO_ELEVATE=1` to run - without elevation — OpenVPN will likely fail) +- Root/sudo for `connect` and `all`, which configure routes and the tunnel + interface. `scan` **never asks for a password** — it only probes configs — and + picks up root silently when your sudo credentials are already cached. Set + `VMATE_NO_ELEVATE=1` to skip elevation entirely (OpenVPN will likely fail) - `killall` (optional) only if you pass `--killall` for the global OpenVPN sweep ## Build & Test @@ -244,7 +245,9 @@ instead. vmate-cli doctor ``` -Checks for OpenVPN, root access, and database health. +Checks OpenVPN, `killall`, the database (journal mode and counts), storage +ownership, and the platform/sudo environment — including whether an elevated +run resolves the same config directory as a normal one. #### `completions` — shell completion scripts @@ -338,9 +341,16 @@ failures, a `removed from recent list` notice is shown briefly, and the Config line shows the `.ovpn` file name (not the full path). Pressing `n` gracefully kills the current OpenVPN process group (SIGTERM, then -SIGKILL after the grace period), runs `killall -9 openvpn` too when `--killall` -is enabled, marks the config as skipped (it is **not** deleted from history), -and moves it to the end of a shuffled deferred queue. +SIGKILL after a short switch grace period), runs `killall -9 openvpn` too when +`--killall` is enabled, marks the config as skipped (it is **not** deleted from +history), and moves it to the end of a shuffled deferred queue. A switch or a +quit never waits out the full teardown grace period, so `n` and Ctrl+C hand the +terminal back immediately. + +Keys are polled while a connection is being established as well as while it is +up, so `n`, `r`, `c` and `q` respond during the handshake. Ctrl+C (and a real +SIGINT/SIGTERM when the TUI is not in raw mode) restores the terminal and kills +only the OpenVPN processes vmate spawned. ## Export @@ -367,6 +377,14 @@ migrations run automatically on startup: sqlite3 ~/.config/vmate-cli/vmate.db "PRAGMA journal_mode;" # → wal ``` +One history, elevated or not: `sudo` on Linux resets `HOME` to `/root`, which +would otherwise give `sudo vmate-cli scan` and a plain `vmate-cli recent` two +different databases. vmate re-applies your home and config environment across +the `sudo` re-exec and hands any root-created database files back to you, so +both halves of a session — and both platforms — see the same history. Any file +that is already root-owned from an older release is reported by +`vmate-cli doctor` together with the `chown` that fixes it. + ## Architecture ``` diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 2ff23fe..e92fb2f 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,37 +1,48 @@ -# vmate-cli v1.2.0 +# vmate-cli v1.2.1 Prebuilt binaries for macOS (Apple Silicon and Intel) and Linux (x86_64 and arm64). Each zip contains the `vmate-cli` binary and `install.sh`, which copies the binary onto your PATH. -## Built-in VPN configs - -- `vmate-cli scan` with no directory scans the built-in vpn-gate remotes — the - shared key is embedded once and each config is built on the fly, so there are - no files to download. `--provider` selects the provider (default `vpn-gate`) - and `--proto udp|tcp` picks the transport (default `udp`); re-scan with - `--proto tcp` to try the other. -- Built-in configs appear in `recent` by their remote (`host-port`), cache - their country like normal configs, and export as - `provider_host-port_COUNTRY.ovpn`. - -## Persistent defaults - -- `vmate-cli scan --save-defaults --max 500 --timeout 20s` saves those as the - defaults for future sessions; `vmate-cli connect --save-defaults - --retry-count 5 --connect-timeout 10s --cooldown 60s --stability-grace 8s` - does the same for the connect tunables. `--save-defaults` writes the new - defaults and exits — it does not scan or connect. -- Each value resolves as `explicit flag → persisted setting → built-in - default`, so a plain `vmate-cli scan` uses what you saved and a one-off - `--max 200` still overrides for that run. Settings live in - `vmate-cli/settings.json` inside your config directory and are - human-editable. - -## Reliability - -- A CI gate now runs `cargo fmt --check`, `cargo clippy`, and `cargo test` on - every push, so the documented quality bar is enforced. -- Internal architecture pass: the retry/drop policy, built-in identity, and - process-teardown logic were consolidated, and the process-registry isolation - that had made the test suite flaky was fixed. +## Linux: the database works again + +- **Fixed: scan results disappeared on Linux.** `sudo` on Linux resets `HOME` + to `/root`, so an elevated `vmate-cli scan` or `connect` stored its history in + `/root/.config/vmate-cli/vmate.db` while `vmate-cli recent` — which never + elevates — read `~/.config/vmate-cli/vmate.db` and reported nothing. macOS + `sudo` keeps `HOME`, which is why the same build behaved differently per + platform. vmate now re-applies your home, `XDG_*` and `VMATE_*` environment + across the `sudo` re-exec and hands every root-created config directory, + database, WAL sidecar and settings file back to your user. +- `vmate-cli doctor` reports the platform, your home, the resolved config + directory and whether the database is really writable, and prints the + `chown` that repairs a database left root-owned by an older release. +- The database is opened by path instead of through a `sqlite://` URL, so a + database path containing `?` can no longer be parsed as query parameters. + +## `scan` no longer asks for a password + +- Scanning only probes configs — it never rewrites routes or interfaces — so it + never blocks on a sudo prompt. When your sudo credentials are already cached + the run is elevated for free; otherwise it runs with the privileges it has + and hints at `sudo vmate-cli scan ...` only if every probe failed. +- `--save-defaults` is a pure write, so it never elevates on any command. + +## Snappier switching and quitting + +- `n` (next config) and Ctrl+C no longer wait out the 3s teardown grace period: + a user-initiated switch escalates after 400ms, and shutdown cleanup polls for + the processes to exit instead of sleeping a fixed second. +- Keys are polled every 50ms, so `n`, `r`, `c` and `q` land immediately and the + uptime clock keeps ticking smoothly. +- A real SIGINT/SIGTERM — raw mode turns an interactive Ctrl+C into a key event + — now kills only the OpenVPN processes vmate spawned and restores the + terminal instead of leaving it in raw mode. + +## Housekeeping + +- CI runs on Linux *and* macOS, treats warnings as errors twice over (clippy + with `-D warnings`, plus a workspace lints table that promotes `unused` and + `unsafe_code` to errors) and fails on broken doc links. +- Result blocks are separated by a consistent blank line, so a scan report + followed by an export summary no longer runs together. diff --git a/crates/vmate-cli/Cargo.toml b/crates/vmate-cli/Cargo.toml index 204518f..0ab67c0 100644 --- a/crates/vmate-cli/Cargo.toml +++ b/crates/vmate-cli/Cargo.toml @@ -35,3 +35,6 @@ vmate-core = { path = "../vmate-core" } assert_cmd.workspace = true predicates.workspace = true tempfile.workspace = true + +[lints] +workspace = true diff --git a/crates/vmate-cli/src/commands/all.rs b/crates/vmate-cli/src/commands/all.rs index afaf794..fd113e0 100644 --- a/crates/vmate-cli/src/commands/all.rs +++ b/crates/vmate-cli/src/commands/all.rs @@ -12,17 +12,18 @@ use vmate_core::settings::UserSettings; use vmate_core::system::{ProcessKiller, RealProcessKiller, require_root_for}; pub async fn run(settings: &Settings, args: &AllArgs, verbose: &Verbosity) -> Result<()> { - // Elevate like a real run: the config dir may need sudo (e.g. left over - // from a previous elevated run), so --save-defaults must not skip it. - require_root_for("run OpenVPN tests and connections", settings.no_elevate)?; - - // --save-defaults persists and exits without scanning or connecting. + // --save-defaults persists and exits without scanning or connecting: it + // writes one file, so it must not block on a password prompt. if settings.save_defaults { crate::commands::scan::persist_scan_defaults(&args.scan)?; persist_connect_defaults(&args.connect)?; return Ok(()); } + // `all` connects at the end, and connecting reconfigures the system, so it + // needs root: elevate up front rather than re-running the scan afterwards. + require_root_for("run OpenVPN tests and connections", settings.no_elevate)?; + let us = UserSettings::load(); let connect = resolve_connect(&us, &args.connect); @@ -36,6 +37,8 @@ pub async fn run(settings: &Settings, args: &AllArgs, verbose: &Verbosity) -> Re } if report.matched_configs.is_empty() { + // Blank line so this reads as its own block after the scan report. + println!(); println!("No successful configs matched filter: {}", settings.filter); return Ok(()); } @@ -57,6 +60,9 @@ pub async fn run(settings: &Settings, args: &AllArgs, verbose: &Verbosity) -> Re let queue = ConnectQueue::new(candidates); let registry = Arc::new(vmate_core::system::ProcessRegistry::new()); + // Catches a real signal (raw mode turns Ctrl+C into a key event) so an + // interrupted session still cleans up and restores the terminal. + let _signal_guard = crate::ui::term::SignalGuard::install(registry.clone()); let runner = Arc::new(RealOpenVpnRunner { bin: settings.openvpn_bin.clone(), registry: registry.clone(), diff --git a/crates/vmate-cli/src/commands/connect.rs b/crates/vmate-cli/src/commands/connect.rs index d881711..3f00a72 100644 --- a/crates/vmate-cli/src/commands/connect.rs +++ b/crates/vmate-cli/src/commands/connect.rs @@ -53,16 +53,17 @@ pub(crate) fn persist_connect_defaults(args: &ConnectArgs) -> Result<()> { } pub async fn run(settings: &Settings, args: &ConnectArgs, verbose: &Verbosity) -> Result<()> { - // Elevate like a real run: the config dir may need sudo (e.g. left over - // from a previous elevated run), so --save-defaults must not skip it. - require_root_for("run OpenVPN connections", settings.no_elevate)?; - - // --save-defaults persists and exits without connecting (no OpenVPN, no DB). + // --save-defaults persists and exits without connecting: it writes a single + // file, so it needs no elevation and must not block on a password prompt. if settings.save_defaults { persist_connect_defaults(args)?; return Ok(()); } + // Connecting really does reconfigure the system (routes and the tunnel + // interface), so it needs root: elevate like a normal run. + require_root_for("run OpenVPN connections", settings.no_elevate)?; + let us = UserSettings::load(); let resolved = resolve_connect(&us, args); @@ -91,6 +92,11 @@ pub async fn run(settings: &Settings, args: &ConnectArgs, verbose: &Verbosity) - killall_enabled: settings.killall_enabled, }); let registry = Arc::new(vmate_core::system::ProcessRegistry::new()); + // Raw mode turns Ctrl+C into a key event, so this only catches a real + // signal (--no-interactive, or one delivered before the TUI came up): it + // kills what we spawned and restores the terminal instead of leaving both + // behind. + let _signal_guard = crate::ui::term::SignalGuard::install(registry.clone()); let runner = Arc::new(RealOpenVpnRunner { bin: settings.openvpn_bin.clone(), registry: registry.clone(), diff --git a/crates/vmate-cli/src/commands/doctor.rs b/crates/vmate-cli/src/commands/doctor.rs index 14d352a..1aacd34 100644 --- a/crates/vmate-cli/src/commands/doctor.rs +++ b/crates/vmate-cli/src/commands/doctor.rs @@ -4,10 +4,10 @@ use crate::settings::Settings; use anyhow::Result; use comfy_table::Table; use std::io::IsTerminal; +use std::path::Path; use vmate_core::db::ConfigRepo; use vmate_core::db::models::ConfigStatus; use vmate_core::db::pool::init_pool; -use vmate_core::system::is_root; pub async fn run(settings: &Settings) -> Result<()> { let mut table = Table::new(); @@ -54,10 +54,20 @@ pub async fn run(settings: &Settings) -> Result<()> { } } + table.add_row(["Root".to_string(), vmate_core::system::root_summary()]); table.add_row([ - "Root".to_string(), - if is_root() { "yes" } else { "no" }.to_string(), + "Platform".to_string(), + format!("{} / {}", std::env::consts::OS, std::env::consts::ARCH), ]); + table.add_row([ + "Home".to_string(), + std::env::var("HOME").unwrap_or_else(|_| "-".to_string()), + ]); + match vmate_core::paths::config_dir() { + Ok(dir) => table.add_row(["Config dir".to_string(), dir.display().to_string()]), + Err(err) => table.add_row(["Config dir".to_string(), format!("error: {err}")]), + }; + table.add_row(["DB access".to_string(), storage_status(&settings.db_path)]); table.add_row([ "Terminal".to_string(), if std::io::stdout().is_terminal() { @@ -96,6 +106,35 @@ pub async fn run(settings: &Settings) -> Result<()> { Ok(()) } +/// Whether the database — and the directory holding it — can be used by the +/// current user. +/// +/// This is the check that makes the classic Linux failure visible: `sudo` +/// resets `HOME` there, so an older elevated run could create (and later leave +/// behind) a root-owned database that every normal run failed to open. +fn storage_status(db_path: &Path) -> String { + let dir = match db_path.parent() { + Some(dir) if !dir.as_os_str().is_empty() => dir, + _ => Path::new("."), + }; + if !vmate_core::paths::writable_by_current_user(dir) { + return format!("blocked ({} is not writable)", dir.display()); + } + if !db_path.exists() { + return "ok (not created yet)".to_string(); + } + if vmate_core::paths::writable_by_current_user(db_path) { + return "ok".to_string(); + } + let owner = vmate_core::paths::owner_uid(db_path) + .map(|uid| uid.to_string()) + .unwrap_or_else(|| "unknown".to_string()); + format!( + "read-only (owned by uid {owner}) — fix with: sudo chown -R \"$(id -u):$(id -g)\" {}", + dir.display() + ) +} + fn status_text(ok: bool) -> String { if ok { "ok".to_string() diff --git a/crates/vmate-cli/src/commands/recent.rs b/crates/vmate-cli/src/commands/recent.rs index 0e0c1b4..e694c9f 100644 --- a/crates/vmate-cli/src/commands/recent.rs +++ b/crates/vmate-cli/src/commands/recent.rs @@ -49,6 +49,8 @@ pub async fn run(settings: &Settings, args: &RecentArgs) -> Result<()> { if let Some(export_dir) = &args.export { let dest = vmate_core::paths::expand_path(export_dir); let result = vmate_core::export::export_configs(&repo, &settings.filter, &dest).await?; + // Blank line so the summary reads as its own block after the listing. + println!(); println!( "Exported {} of {} configs to {}", result.exported, diff --git a/crates/vmate-cli/src/commands/scan.rs b/crates/vmate-cli/src/commands/scan.rs index c625bb8..2a0ef18 100644 --- a/crates/vmate-cli/src/commands/scan.rs +++ b/crates/vmate-cli/src/commands/scan.rs @@ -15,13 +15,16 @@ use vmate_core::ovpn::process::{RealVpnTester, VpnTester}; use vmate_core::scan::{ScanOptions, ScanProgress, ScanReport, ScanService}; use vmate_core::settings::UserSettings; use vmate_core::system::{ - CleanupGuard, ProcessKiller, RealProcessKiller, require_root_for, shutdown_signal, + CleanupGuard, ProcessKiller, RealProcessKiller, elevate_without_prompt, is_root, + shutdown_signal, }; pub async fn run(settings: &Settings, args: &ScanArgs, verbose: &Verbosity) -> Result<()> { - // Elevate like a real run: the config dir may need sudo (e.g. left over - // from a previous elevated run), so --save-defaults must not skip it. - require_root_for("run OpenVPN tests", settings.no_elevate)?; + // Scanning only *probes* configs — it never rewrites the system's routes or + // interfaces — so it must never block on a password prompt. When sudo + // credentials are already cached the run is elevated for free; otherwise it + // simply runs with the privileges it has. + elevate_without_prompt(settings.no_elevate); // --save-defaults persists and exits without scanning (no OpenVPN, no DB). if settings.save_defaults { @@ -29,7 +32,16 @@ pub async fn run(settings: &Settings, args: &ScanArgs, verbose: &Verbosity) -> R return Ok(()); } - scan_pipeline(settings, args, verbose).await?; + let (report, _repo) = scan_pipeline(settings, args, verbose).await?; + + // Every probe failed and we were not root: on Linux OpenVPN needs root to + // open a tun device, so say so instead of leaving a silent wall of misses. + if !is_root() && !settings.no_elevate && report.scanned > 0 && report.success == 0 { + eprintln!( + "note: no config connected and vmate is not running as root\n\ + hint: OpenVPN needs root to open a tun device — try `sudo vmate-cli scan ...`" + ); + } Ok(()) } @@ -107,6 +119,9 @@ pub(crate) async fn scan_pipeline( let dest = vmate_core::paths::expand_path(export_dir); let result = vmate_core::export::export_configs_from_matches(&report.matched_configs, &dest).await?; + // Blank line so the export summary reads as its own block, not as + // another row of the report above it. + println!(); println!( "Exported {} of {} configs to {}", result.exported, diff --git a/crates/vmate-cli/src/ui/connect.rs b/crates/vmate-cli/src/ui/connect.rs index c9f7323..2ac1c39 100644 --- a/crates/vmate-cli/src/ui/connect.rs +++ b/crates/vmate-cli/src/ui/connect.rs @@ -28,6 +28,11 @@ type Term = Terminal>; /// before reverting to the connected status. const MESSAGE_TTL: Duration = Duration::from_secs(3); +/// How long `poll_command` blocks waiting for a key before re-rendering and +/// yielding to the runtime. Short enough that `n`/`q`/Ctrl+C feel instant, +/// long enough to keep redraws (and CPU) cheap. +const KEY_POLL: Duration = Duration::from_millis(50); + /// Whether a transient overlay came from the connect status (the fading /// "Connected successfully to X" confirmation) or from a user-facing notice /// (`notify`/`copy`, e.g. "removed ... from recent list"). A confirmation is @@ -217,26 +222,34 @@ impl ConnectTui { if self.no_interactive { return None; } - if !event::poll(Duration::from_millis(200)).ok()? { + // Short poll: the connect loop redraws (and so updates the uptime + // clock) on every call, and a keypress must be picked up straight + // away instead of waiting out a long block. + if !event::poll(KEY_POLL).ok()? { return None; } match event::read().ok()? { - Event::Key(k) if k.kind == KeyEventKind::Press => match k.code { - // Ctrl+C quits (raw mode swallows SIGINT). - KeyCode::Char('c') if k.modifiers.contains(KeyModifiers::CONTROL) => { - Some(UserCommand::Quit) + Event::Key(k) if k.kind == KeyEventKind::Press => { + // Ctrl+C quits (raw mode swallows SIGINT, so an interactive + // Ctrl+C can only ever be seen here). + if k.modifiers.contains(KeyModifiers::CONTROL) + && matches!(k.code, KeyCode::Char('c') | KeyCode::Char('C')) + { + return Some(UserCommand::Quit); } - KeyCode::Char('n') => Some(UserCommand::Next), - KeyCode::Char('r') => Some(UserCommand::Reconnect), - KeyCode::Char('v') => { - self.verbose = !self.verbose; - Some(UserCommand::ToggleVerbose) + match k.code { + KeyCode::Char('n') => Some(UserCommand::Next), + KeyCode::Char('r') => Some(UserCommand::Reconnect), + KeyCode::Char('v') => { + self.verbose = !self.verbose; + Some(UserCommand::ToggleVerbose) + } + KeyCode::Char('?') => Some(UserCommand::Help), + KeyCode::Char('q') | KeyCode::Esc => Some(UserCommand::Quit), + KeyCode::Char('c') => Some(UserCommand::CopyPath), + _ => None, } - KeyCode::Char('?') => Some(UserCommand::Help), - KeyCode::Char('q') | KeyCode::Esc => Some(UserCommand::Quit), - KeyCode::Char('c') => Some(UserCommand::CopyPath), - _ => None, - }, + } _ => None, } } diff --git a/crates/vmate-cli/src/ui/term.rs b/crates/vmate-cli/src/ui/term.rs index 4a72644..06adac5 100644 --- a/crates/vmate-cli/src/ui/term.rs +++ b/crates/vmate-cli/src/ui/term.rs @@ -7,6 +7,19 @@ pub fn stdout_is_tty() -> bool { std::io::stdout().is_terminal() } +/// Put the terminal back the way it was found: raw mode off, alternate screen +/// left, mouse capture disabled. +/// +/// Safe to call more than once and on a terminal that was never touched, so it +/// can be used both by the RAII guard below and by the emergency signal path. +pub fn restore_terminal() { + use crossterm::event::DisableMouseCapture; + use crossterm::execute; + use crossterm::terminal::{LeaveAlternateScreen, disable_raw_mode}; + let _ = disable_raw_mode(); + let _ = execute!(std::io::stdout(), LeaveAlternateScreen, DisableMouseCapture); +} + /// RAII guard that restores the terminal when dropped. /// /// Used by the interactive TUIs so that raw mode and the alternate screen are @@ -15,10 +28,39 @@ pub struct TuiGuard; impl Drop for TuiGuard { fn drop(&mut self) { - use crossterm::event::DisableMouseCapture; - use crossterm::execute; - use crossterm::terminal::{LeaveAlternateScreen, disable_raw_mode}; - let _ = disable_raw_mode(); - let _ = execute!(std::io::stdout(), LeaveAlternateScreen, DisableMouseCapture); + restore_terminal(); + } +} + +/// Keeps an emergency shutdown handler alive for one interactive session. +/// +/// Raw mode turns Ctrl+C into a key event, so this only sees a real signal: +/// `--no-interactive` runs, or a SIGINT/SIGTERM that arrives before the TUI +/// takes over. Without it such a signal would kill vmate mid-session, leaving +/// the terminal in raw mode and the tunnel process behind. +pub struct SignalGuard(Option>); + +impl SignalGuard { + /// Install the handler for the processes registered in `registry`. + pub fn install(registry: std::sync::Arc) -> Self { + let handle = tokio::spawn(async move { + let reason = vmate_core::system::shutdown_signal().await; + registry.kill_all_immediate(); + restore_terminal(); + // 130 is the shell convention for SIGINT, 143 for SIGTERM. + std::process::exit(match reason { + vmate_core::system::ShutdownReason::CtrlC => 130, + vmate_core::system::ShutdownReason::Term => 143, + }); + }); + Self(Some(handle)) + } +} + +impl Drop for SignalGuard { + fn drop(&mut self) { + if let Some(handle) = self.0.take() { + handle.abort(); + } } } diff --git a/crates/vmate-cli/tests/cli.rs b/crates/vmate-cli/tests/cli.rs index 0c86e34..fdfc021 100644 --- a/crates/vmate-cli/tests/cli.rs +++ b/crates/vmate-cli/tests/cli.rs @@ -91,6 +91,23 @@ fn invalid_filter_is_rejected() { .stderr(predicate::str::contains("invalid country code")); } +/// `doctor` is how a user diagnoses a database that cannot be written to — the +/// Linux failure this release fixes — so the rows that explain it must stay. +#[test] +fn doctor_reports_platform_config_dir_and_storage() { + let dir = tempfile::tempdir().unwrap(); + let db = dir.path().join("vmate.db"); + let mut cmd = Command::cargo_bin("vmate-cli").unwrap(); + cmd.args(["doctor"]) + .env("VMATE_DB", &db) + .assert() + .success() + .stdout(predicate::str::contains("Platform")) + .stdout(predicate::str::contains("Config dir")) + .stdout(predicate::str::contains("DB access")) + .stdout(predicate::str::contains("Home")); +} + #[test] fn repeated_filter_flags_work() { let mut cmd = Command::cargo_bin("vmate-cli").unwrap(); diff --git a/crates/vmate-cli/tests/scan.rs b/crates/vmate-cli/tests/scan.rs index 771ac76..c906f5c 100644 --- a/crates/vmate-cli/tests/scan.rs +++ b/crates/vmate-cli/tests/scan.rs @@ -25,6 +25,32 @@ fn scan_empty_dir_succeeds() { .stdout(predicate::str::contains("Found matched: 0")); } +/// Scanning only probes configs, so it must not demand a password. The absence +/// of `VMATE_NO_ELEVATE` here is the point: an older release refused to run at +/// all without it unless the process was already root. +#[test] +fn scan_does_not_require_root() { + let (dir, db) = tmp_db(); + let mut cmd = Command::cargo_bin("vmate-cli").unwrap(); + cmd.args(["scan", dir.path().to_str().unwrap(), "--no-save"]) + .env("VMATE_DB", &db) + .assert() + .success() + .stdout(predicate::str::contains("Found matched: 0")); +} + +/// `--save-defaults` writes one file: it must not elevate on any command. +#[test] +fn scan_save_defaults_does_not_require_root() { + let home = tempfile::tempdir().expect("tempdir"); + let mut cmd = Command::cargo_bin("vmate-cli").unwrap(); + cmd.args(["scan", "--save-defaults", "--max", "42"]) + .env("HOME", home.path()) + .assert() + .success() + .stdout(predicate::str::contains("Saved scan defaults")); +} + #[test] fn scan_missing_dir_fails_cleanly() { let (_, db) = tmp_db(); diff --git a/crates/vmate-core/Cargo.toml b/crates/vmate-core/Cargo.toml index 068eadf..78d296b 100644 --- a/crates/vmate-core/Cargo.toml +++ b/crates/vmate-core/Cargo.toml @@ -30,3 +30,6 @@ walkdir.workspace = true [dev-dependencies] tokio.workspace = true + +[lints] +workspace = true diff --git a/crates/vmate-core/src/connect/service.rs b/crates/vmate-core/src/connect/service.rs index b47ed40..00ea042 100644 --- a/crates/vmate-core/src/connect/service.rs +++ b/crates/vmate-core/src/connect/service.rs @@ -9,8 +9,8 @@ //! isn't dropped for two long-session crashes; a crash right after the //! handshake still counts, so a connect-then-crash config can't loop forever. //! Manual `n`/Next only defers the config (row stays in the DB); `r`/Reconnect -//! is penalty-free. The per-candidate helpers live in [`super::session`]; the -//! [`ConnectHost`] trait lives in [`super::host`]. +//! is penalty-free. The per-candidate helpers live in the session helper module; +//! the [`ConnectHost`] trait is re-exported from [`crate::connect`]. use crate::connect::host::ConnectHost; use crate::connect::queue::ConnectQueue; @@ -94,12 +94,34 @@ pub struct ConnectService { impl ConnectService { pub async fn run(&self, mut queue: ConnectQueue, host: &mut dyn ConnectHost) -> Result<()> { + let cancel = CancellationToken::new(); + + // Ctrl+C / SIGTERM must break the loop even when the TUI cannot turn + // them into key events: raw mode swallows SIGINT, so with + // `--no-interactive` (or a signal that lands before the UI is up) this + // is the only way out that is not "wait for the current handshake". + let cancel_task = cancel.clone(); + let signal_task = tokio::spawn(async move { + let _ = crate::system::shutdown_signal().await; + cancel_task.cancel(); + }); + + let result = self.run_queue(&mut queue, host, &cancel).await; + signal_task.abort(); + result + } + + async fn run_queue( + &self, + queue: &mut ConnectQueue, + host: &mut dyn ConnectHost, + cancel: &CancellationToken, + ) -> Result<()> { let _guard = CleanupGuard::new( self.killer.clone(), self.registry.clone(), self.options.killall_enabled, ); - let cancel = CancellationToken::new(); // Go parity: try once, reconnect once, then drop from history. The // budget is configurable via ConnectOptions::retry_count. while let Some(first) = queue.next_candidate() { @@ -169,14 +191,16 @@ impl ConnectService { match outcome { ConnectOutcome::Cancelled => { - handle.kill_graceful(self.killer.as_ref()).await; + // The user is leaving now: do not make them wait out + // the teardown grace period. + handle.kill_now(self.killer.as_ref()).await; host.finish().await?; return Ok(()); } ConnectOutcome::Next => { // User pressed `n` during the handshake: KEEP in DB, // defer in-session, move to the next candidate. - handle.kill_graceful(self.killer.as_ref()).await; + handle.kill_now(self.killer.as_ref()).await; let _ = self.repo.mark_skipped(candidate.id).await; queue.skip(candidate); break; @@ -186,9 +210,16 @@ impl ConnectService { last_reason = None; let connected_at = std::time::Instant::now(); let exit = self - .monitor_connected(&candidate, &mut handle, host, &cancel) + .monitor_connected(&candidate, &mut handle, host, cancel) .await?; - handle.kill_graceful(self.killer.as_ref()).await; // targeted kill only + // A switch or a quit escalates immediately; a crash or + // a timeout keeps the graceful teardown. + match exit { + Phase2Exit::Quit | Phase2Exit::Next => { + handle.kill_now(self.killer.as_ref()).await; // targeted kill only + } + _ => handle.kill_graceful(self.killer.as_ref()).await, + } match exit { Phase2Exit::Quit => { host.finish().await?; diff --git a/crates/vmate-core/src/db/pool.rs b/crates/vmate-core/src/db/pool.rs index 1e69238..6579195 100644 --- a/crates/vmate-core/src/db/pool.rs +++ b/crates/vmate-core/src/db/pool.rs @@ -5,7 +5,6 @@ use sqlx::Row; use sqlx::SqlitePool; use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; use std::path::Path; -use std::str::FromStr; /// The type of the shared SQLite connection pool. pub type DbPool = SqlitePool; @@ -20,12 +19,11 @@ pub async fn init_pool(db_path: &Path) -> Result { } } - // SQLite treats a single leading slash specially, so `sqlite:///abs/path` - // is produced for absolute paths and `sqlite://rel/path` for relative ones. - let url = format!("sqlite://{}", db_path.display()); - - let options = SqliteConnectOptions::from_str(&url) - .with_context(|| format!("invalid SQLite URL: {url}"))? + // The database is opened by path, not through a `sqlite://` URL: URL + // parsing treats everything after the first `?` as query parameters, so a + // database path containing one would silently resolve somewhere else. + let options = SqliteConnectOptions::new() + .filename(db_path) .create_if_missing(true) .pragma("journal_mode", "WAL") .pragma("synchronous", "NORMAL") @@ -36,7 +34,7 @@ pub async fn init_pool(db_path: &Path) -> Result { .max_connections(8) .connect_with(options) .await - .context("failed to open SQLite database")?; + .with_context(|| format!("failed to open SQLite database {}", db_path.display()))?; // Embed and run the migrations shipped with the crate. sqlx::migrate!("../../migrations") @@ -44,6 +42,11 @@ pub async fn init_pool(db_path: &Path) -> Result { .await .context("failed to run database migrations")?; + // An elevated run creates root-owned database files. Handing them back + // keeps one history shared between `sudo vmate-cli` and a normal run — + // on Linux, where `sudo` resets HOME, that is exactly what used to break. + repair_db_ownership(db_path); + Ok(pool) } @@ -55,3 +58,17 @@ pub async fn journal_mode(pool: &DbPool) -> Result { .context("journal_mode check failed")?; Ok(row.get::("journal_mode")) } + +/// Hand the database — and its WAL sidecars — back to the user who invoked +/// sudo. Best effort: a failed chown must never fail a scan or a connect. +fn repair_db_ownership(db_path: &Path) { + crate::system::repair_ownership(db_path); + if let Some(parent) = db_path.parent() { + crate::system::repair_ownership(parent); + } + for suffix in ["-wal", "-shm"] { + let mut sidecar = db_path.as_os_str().to_os_string(); + sidecar.push(suffix); + crate::system::repair_ownership(Path::new(&sidecar)); + } +} diff --git a/crates/vmate-core/src/ovpn/process.rs b/crates/vmate-core/src/ovpn/process.rs index 276599a..12cb944 100644 --- a/crates/vmate-core/src/ovpn/process.rs +++ b/crates/vmate-core/src/ovpn/process.rs @@ -69,6 +69,20 @@ impl OpenVpnHandle { ) .await; } + + /// Stop the process tree without waiting out the full grace period. + /// + /// Used when the user asked to move on — `n` (next config) or Ctrl+C — + /// where waiting on a process they just abandoned reads as a frozen UI. + pub async fn kill_now(&mut self, killer: &dyn ProcessKiller) { + crate::system::killer::kill_process_tree_with_grace( + killer, + self.child.id().unwrap_or(0), + &mut self.child, + crate::system::killer::SWITCH_GRACE, + ) + .await; + } } /// Spawn OpenVPN in a new process group, registering the pid in `registry`. diff --git a/crates/vmate-core/src/paths.rs b/crates/vmate-core/src/paths.rs index 3e8bbcc..fadd014 100644 --- a/crates/vmate-core/src/paths.rs +++ b/crates/vmate-core/src/paths.rs @@ -1,6 +1,8 @@ -//! Path helpers: `~` expansion and default config paths. +//! Path helpers: `~` expansion, default config paths and storage ownership. -use anyhow::{Context, Result, anyhow}; +use anyhow::{Context, Result, anyhow, bail}; +use nix::unistd::{Uid, User, getgid, getuid}; +use std::os::unix::fs::MetadataExt; use std::path::{Path, PathBuf}; /// Expand a leading `~` to the current user's home directory. @@ -24,7 +26,15 @@ pub fn canonicalize_dir>(path: P) -> Result { } /// The user configuration directory (`~/.config/vmate-cli` on Unix). +/// +/// Under `sudo` the *invoking* user's directory is returned, so an elevated run +/// and a normal run share one database. Linux `sudo` resets `HOME` to `/root` +/// (`always_set_home`) while macOS `sudo` keeps it, which is exactly why the +/// directory is derived from `SUDO_UID` instead of from the environment. pub fn config_dir() -> Result { + if let Some(dir) = sudo_user_config_dir() { + return Ok(dir); + } let dirs = directories::ProjectDirs::from("", "", "vmate-cli") .ok_or_else(|| anyhow!("cannot determine config directory"))?; Ok(dirs.config_dir().to_path_buf()) @@ -38,9 +48,84 @@ pub fn builtin_dir() -> Result { /// The default SQLite database path, creating the config directory if needed. pub fn default_db_path() -> Result { let dir = config_dir()?; - std::fs::create_dir_all(&dir) - .with_context(|| format!("cannot create config directory {}", dir.display()))?; - Ok(dir.join("vmate.db")) + if !dir.exists() { + std::fs::create_dir_all(&dir) + .with_context(|| format!("cannot create config directory {}", dir.display()))?; + } + // A directory created by an elevated run belongs to root: hand it back so + // the next unprivileged run can still write to it. + crate::system::repair_ownership(&dir); + + let db = dir.join("vmate.db"); + ensure_writable(&db, &dir)?; + Ok(db) +} + +/// Refuse early — and with a fix — when the database cannot be written to. +/// +/// The usual cause is an older elevated run: on Linux `sudo` resets `HOME`, so +/// vmate stored its history under `/root` and, when the directory was shared, +/// left root-owned files behind that the normal user cannot open. +fn ensure_writable(db: &Path, dir: &Path) -> Result<()> { + let target = if db.exists() { db } else { dir }; + if writable_by_current_user(target) { + return Ok(()); + } + let owner = owner_uid(target) + .map(|uid| uid.to_string()) + .unwrap_or_else(|| "unknown".to_string()); + bail!( + "cannot write to {} (owned by uid {owner})\n\ + hint: it was created by an elevated run — hand it back with\n\ + \x20 `sudo chown -R \"$(id -u):$(id -g)\" {}`", + target.display(), + dir.display() + ); +} + +/// Whether `path` can be written by the current (real) user. +/// +/// Root bypasses the check: it can write regardless of the mode bits. +pub fn writable_by_current_user(path: &Path) -> bool { + if crate::system::is_root() { + return true; + } + let Ok(meta) = std::fs::metadata(path) else { + // Missing: whether it can be created is decided by the parent. + return path.parent().is_some_and(writable_by_current_user); + }; + + // A directory needs write *and* search (x) permission to be usable. + let (owner_bits, group_bits, other_bits) = if meta.is_dir() { + (0o300, 0o030, 0o003) + } else { + (0o200, 0o020, 0o002) + }; + let required = if meta.uid() == getuid().as_raw() { + owner_bits + } else if meta.gid() == getgid().as_raw() { + group_bits + } else { + other_bits + }; + (meta.mode() & required) == required +} + +/// The owning uid of `path`, when its metadata can be read. +pub fn owner_uid(path: &Path) -> Option { + std::fs::metadata(path).ok().map(|meta| meta.uid()) +} + +/// The invoking user's config directory when this process is the elevated half +/// of a `sudo` run. +fn sudo_user_config_dir() -> Option { + let (uid, _gid) = crate::system::sudo_uid_gid()?; + let user = User::from_uid(Uid::from_raw(uid)).ok()??; + let base = match std::env::var_os("XDG_CONFIG_HOME") { + Some(dir) if !dir.is_empty() => PathBuf::from(dir), + _ => user.dir.join(".config"), + }; + Some(base.join("vmate-cli")) } #[cfg(test)] @@ -60,4 +145,31 @@ mod tests { assert_eq!(expand_path("/etc/openvpn"), PathBuf::from("/etc/openvpn")); assert_eq!(expand_path("relative/path"), PathBuf::from("relative/path")); } + + #[test] + fn config_dir_is_absolute_and_named_vmate_cli() { + let dir = config_dir().expect("config dir"); + assert!(dir.is_absolute(), "config dir must be absolute: {dir:?}"); + assert_eq!(dir.file_name().and_then(|n| n.to_str()), Some("vmate-cli")); + } + + #[test] + fn the_config_dir_itself_is_writable() { + let dir = config_dir().expect("config dir"); + assert!( + writable_by_current_user(&dir), + "the config dir must be writable: {dir:?}" + ); + } + + #[test] + fn missing_paths_fall_back_to_their_parent() { + let dir = std::env::temp_dir().join("vmate-missing-parent-probe"); + assert!(writable_by_current_user(&dir)); + if !crate::system::is_root() { + assert!(!writable_by_current_user(Path::new( + "/proc/definitely-missing-vmate.db" + ))); + } + } } diff --git a/crates/vmate-core/src/settings.rs b/crates/vmate-core/src/settings.rs index ad08bc2..bad190e 100644 --- a/crates/vmate-core/src/settings.rs +++ b/crates/vmate-core/src/settings.rs @@ -15,6 +15,7 @@ //! built-in defaults. use crate::paths; +use anyhow::Context; use std::path::{Path, PathBuf}; use std::time::Duration; @@ -80,7 +81,19 @@ impl UserSettings { std::fs::create_dir_all(parent)?; } let json = serde_json::to_string_pretty(self)?; - std::fs::write(path, json)?; + std::fs::write(path, json).with_context(|| { + format!( + "cannot write settings to {}\nhint: on Linux an earlier `sudo vmate-cli ...` run \ + may own it — remove the file or re-run with sudo once", + path.display() + ) + })?; + // Keep an elevated `--save-defaults` from leaving a root-owned settings + // file behind for the next unprivileged run. + crate::system::repair_ownership(path); + if let Some(parent) = path.parent() { + crate::system::repair_ownership(parent); + } Ok(()) } diff --git a/crates/vmate-core/src/system/killer.rs b/crates/vmate-core/src/system/killer.rs index e004339..6609ff6 100644 --- a/crates/vmate-core/src/system/killer.rs +++ b/crates/vmate-core/src/system/killer.rs @@ -12,11 +12,23 @@ use nix::sys::signal::{Signal, kill}; use nix::unistd::Pid; use std::process::Stdio; use std::sync::{Arc, Mutex}; -use std::time::Duration; +use std::time::{Duration, Instant}; /// Grace period between SIGTERM and SIGKILL when killing a process tree. pub const KILL_GRACE: Duration = Duration::from_secs(3); +/// Grace period used when the *user* asked to move on — `n` (next config) or +/// Ctrl+C. Waiting out [`KILL_GRACE`] there feels like the UI hung, so a switch +/// escalates to SIGKILL far sooner; OpenVPN has nothing worth flushing on a +/// config the user just abandoned. +pub const SWITCH_GRACE: Duration = Duration::from_millis(400); + +/// Upper bound on the wait between SIGTERM and SIGKILL during shutdown cleanup. +pub const CLEANUP_GRACE: Duration = Duration::from_millis(750); + +/// How often shutdown cleanup re-checks whether the processes are gone. +const REAP_POLL: Duration = Duration::from_millis(25); + /// PIDs of every OpenVPN process spawned in one session. /// /// Owned per run — and per test — so concurrent sessions never interfere: @@ -54,23 +66,62 @@ impl ProcessRegistry { self.pids.lock().unwrap_or_else(|e| e.into_inner()).clear(); } - /// Kill every registered process group: SIGTERM all of them, allow one - /// grace period, then SIGKILL anything still alive, then clear. + /// Kill every registered process group: SIGTERM all of them, wait for them + /// to disappear (or [`CLEANUP_GRACE`]), then SIGKILL anything still alive. /// - /// Sync and best-effort: individual kill errors are ignored, and the grace - /// period is a fixed sleep because there is no live handle to wait on. This - /// is the last-resort safety net used by [`CleanupGuard::drop`]. + /// Sync and best-effort: individual kill errors are ignored. The wait polls + /// instead of sleeping a fixed second, so a shutdown whose processes exited + /// on SIGTERM returns in milliseconds rather than pausing every single run. + /// This is the last-resort safety net used by [`CleanupGuard::drop`]. pub fn kill_all_graceful(&self) { let pids = self.registered(); for &pid in &pids { let _ = kill_process_group(pid); } - std::thread::sleep(Duration::from_secs(1)); + self.wait_until_gone(&pids, CLEANUP_GRACE); for &pid in &pids { let _ = force_kill_process_group(pid); } self.clear(); } + + /// SIGTERM then immediately SIGKILL every registered process group. + /// + /// Used when the process is exiting *now* (Ctrl+C / SIGTERM outside raw + /// mode): a grace period there only delays the prompt coming back. + pub fn kill_all_immediate(&self) { + let pids = self.registered(); + for &pid in &pids { + let _ = kill_process_group(pid); + let _ = force_kill_process_group(pid); + } + self.clear(); + } + + /// Block until every pid is gone or `grace` elapses — whichever comes + /// first. Returns as soon as the processes have actually exited. + fn wait_until_gone(&self, pids: &[u32], grace: Duration) { + let deadline = Instant::now() + grace; + loop { + if !pids.iter().copied().any(process_exists) { + return; + } + if Instant::now() >= deadline { + return; + } + std::thread::sleep(REAP_POLL); + } + } +} + +/// Whether a process with this pid still exists. +/// +/// Signal 0 is a permission/existence probe: `ESRCH` means the pid is gone. +fn process_exists(pid: u32) -> bool { + if pid == 0 { + return false; + } + kill(Pid::from_raw(pid as i32), None::).is_ok() } /// SIGTERM the process group whose leader has the given pid. The child is @@ -163,12 +214,23 @@ pub async fn kill_process_tree_graceful( killer: &dyn ProcessKiller, pid: u32, child: &mut tokio::process::Child, +) { + kill_process_tree_with_grace(killer, pid, child, KILL_GRACE).await; +} + +/// Kill a process tree with an explicit grace period: SIGTERM the group, wait +/// up to `grace` for the child to exit, then SIGKILL the group. +/// +/// [`KILL_GRACE`] is right for a normal teardown; [`SWITCH_GRACE`] keeps a +/// user-initiated switch or quit from feeling like a hang. +pub async fn kill_process_tree_with_grace( + killer: &dyn ProcessKiller, + pid: u32, + child: &mut tokio::process::Child, + grace: Duration, ) { let _ = killer.kill_process_group(pid); - if tokio::time::timeout(KILL_GRACE, child.wait()) - .await - .is_err() - { + if tokio::time::timeout(grace, child.wait()).await.is_err() { let _ = killer.force_kill_process_group(pid); let _ = child.wait().await; } @@ -291,4 +353,51 @@ mod tests { assert!(killer.force_kill_process_group(0).is_ok()); assert!(killer.killall_openvpn().is_ok()); // disabled -> no-op } + + /// Shutdown must not pay a fixed sleep when the OpenVPN processes have + /// already exited — a stalled teardown is what makes Ctrl+C feel like a + /// hang. + #[tokio::test] + async fn cleanup_returns_as_soon_as_the_processes_are_gone() { + use crate::ovpn::process::spawn_openvpn; + + let registry = ProcessRegistry::new(); + let args = ["-c".to_string(), "exit 0".to_string()]; + let mut handle = spawn_openvpn("sh", &args, ®istry).unwrap(); + handle.child.wait().await.unwrap(); + + let start = std::time::Instant::now(); + registry.kill_all_graceful(); + let elapsed = start.elapsed(); + + assert!( + elapsed < Duration::from_millis(500), + "cleanup waited {elapsed:?} for a process that had already exited" + ); + assert!(registry.registered().is_empty()); + } + + /// A switch (`n`) or a quit escalates on `SWITCH_GRACE`, not `KILL_GRACE`, + /// even when the child ignores SIGTERM and has to be SIGKILLed. + #[tokio::test] + async fn switch_grace_escalates_faster_than_the_default_grace() { + use crate::ovpn::process::spawn_openvpn; + + let registry = ProcessRegistry::new(); + let args = ["-c".to_string(), "trap '' TERM; sleep 30".to_string()]; + let mut handle = spawn_openvpn("sh", &args, ®istry).unwrap(); + let pid = handle.child.id().unwrap_or(0); + let killer = RealProcessKiller { + killall_enabled: false, + }; + + let start = std::time::Instant::now(); + kill_process_tree_with_grace(&killer, pid, &mut handle.child, SWITCH_GRACE).await; + let elapsed = start.elapsed(); + + assert!( + elapsed < KILL_GRACE, + "switching took {elapsed:?}; it must not wait out the full teardown grace" + ); + } } diff --git a/crates/vmate-core/src/system/mod.rs b/crates/vmate-core/src/system/mod.rs index 7b6cdff..31c8f6d 100644 --- a/crates/vmate-core/src/system/mod.rs +++ b/crates/vmate-core/src/system/mod.rs @@ -5,8 +5,13 @@ pub mod root; pub mod signal; pub use killer::{ - CleanupGuard, ProcessKiller, ProcessRegistry, RealProcessKiller, force_kill_process_group, - kill_process_group, kill_process_tree_graceful, killall_openvpn, + CleanupGuard, ProcessKiller, ProcessRegistry, RealProcessKiller, SWITCH_GRACE, + force_kill_process_group, kill_process_group, kill_process_tree_graceful, + kill_process_tree_with_grace, killall_openvpn, +}; +pub use root::{ + carried_environment, elevate_with_sudo, elevate_without_prompt, interactive, is_elevated, + is_root, repair_ownership, require_root_for, root_summary, sudo_can_elevate_without_prompt, + sudo_uid_gid, sudo_user, }; -pub use root::{elevate_with_sudo, is_root, require_root_for}; pub use signal::{ShutdownReason, shutdown_signal}; diff --git a/crates/vmate-core/src/system/root.rs b/crates/vmate-core/src/system/root.rs index 23bad5f..fd600af 100644 --- a/crates/vmate-core/src/system/root.rs +++ b/crates/vmate-core/src/system/root.rs @@ -1,15 +1,128 @@ -//! Root privilege handling. +//! Root privilege handling, sudo elevation and ownership repair. -use anyhow::Result; +use anyhow::{Result, bail}; use nix::unistd::getuid; use std::io::IsTerminal; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; +use std::process::Stdio; + +/// Environment carried across a `sudo` re-exec so the elevated half resolves +/// the same home, config directory, database and settings as the invoking +/// user's run does. +/// +/// Linux `sudo` sets `HOME=/root` (`always_set_home`), which used to split +/// vmate's storage in two: an elevated `scan`/`connect` wrote +/// `/root/.config/vmate-cli/vmate.db` while `recent` — which never elevates — +/// read `~/.config/vmate-cli/vmate.db` and reported an empty history. macOS +/// `sudo` keeps `HOME`, so the split only ever showed up on Linux. +const CARRIED_ENV: &[&str] = &[ + "HOME", + "USER", + "LOGNAME", + "SHELL", + "TERM", + "LANG", + "LC_ALL", + "XDG_CONFIG_HOME", + "XDG_DATA_HOME", + "XDG_CACHE_HOME", + "VMATE_DB", + "VMATE_OPENVPN_BIN", + "IPINFO_TOKEN", + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "NO_PROXY", +]; + +/// Marker proving this process is already the elevated half of a re-exec. A +/// sudo policy that leaves us unprivileged must never loop on sudo. +const ELEVATED_MARKER: &str = "VMATE_ELEVATED"; /// Whether the current process runs as uid 0. pub fn is_root() -> bool { getuid().is_root() } +/// Whether stdin and stdout are both terminals (i.e. prompting is possible). +pub fn interactive() -> bool { + std::io::stdin().is_terminal() && std::io::stdout().is_terminal() +} + +/// The user who invoked `sudo`, when this process is the elevated half. +pub fn sudo_user() -> Option { + std::env::var("SUDO_USER").ok().filter(|s| !s.is_empty()) +} + +/// The invoking user's uid/gid, when this process is the elevated half. +pub fn sudo_uid_gid() -> Option<(u32, u32)> { + if !is_root() { + return None; + } + let uid = std::env::var("SUDO_UID").ok()?.parse::().ok()?; + let gid = std::env::var("SUDO_GID").ok()?.parse::().ok()?; + Some((uid, gid)) +} + +/// Whether this process is the elevated half of a sudo re-exec. +pub fn is_elevated() -> bool { + std::env::var_os(ELEVATED_MARKER).is_some() +} + +/// Hand a root-created path back to the user who invoked sudo. +/// +/// Without this an elevated run leaves `~/.config/vmate-cli` — and the database +/// inside it — owned by root, and the next unprivileged run cannot open it. +/// Best effort by design: a failed chown must never abort a scan. +pub fn repair_ownership(path: &Path) { + let Some((uid, gid)) = sudo_uid_gid() else { + return; + }; + if !path.exists() { + return; + } + #[cfg(unix)] + if let Err(err) = std::os::unix::fs::chown(path, Some(uid), Some(gid)) { + tracing::debug!( + path = %path.display(), + error = %err, + "could not hand ownership back to the invoking user" + ); + } +} + +/// `KEY=value` assignments that re-create the user's identity after `sudo`. +pub fn carried_environment() -> Vec { + let mut assignments: Vec = CARRIED_ENV + .iter() + .filter_map(|key| { + std::env::var_os(*key).map(|value| format!("{key}={}", value.to_string_lossy())) + }) + .collect(); + assignments.push(format!("{ELEVATED_MARKER}=1")); + assignments +} + +/// Whether `sudo` can elevate without asking for a password — either the +/// credential cache is warm or the user has a `NOPASSWD` rule. +/// +/// `sudo -n` never prompts: it succeeds from the cache or fails outright. +/// Commands that work unprivileged use this to pick up root for free instead +/// of blocking the terminal on a password prompt. +pub fn sudo_can_elevate_without_prompt() -> bool { + if is_root() { + return false; + } + std::process::Command::new("sudo") + .args(["-n", "-v"]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .map(|status| status.success()) + .unwrap_or(false) +} + /// Ensure the process has root privileges for `context`. /// /// * Already root → returns. @@ -27,10 +140,18 @@ pub fn require_root_for(context: &str, no_elevate: bool) -> Result<()> { return Ok(()); } - let interactive = std::io::stdin().is_terminal() && std::io::stdout().is_terminal(); - if !interactive { - anyhow::bail!( - "vmate-cli needs root to {context}.\n\ + // We already went through sudo once and are somehow still unprivileged: + // bailing is the only way to avoid re-executing forever. + if is_elevated() { + bail!( + "vmate-cli was elevated with sudo but is still not root, so it cannot {context}\n\ + hint: check `sudo -v`, or run `sudo vmate-cli ...` yourself" + ); + } + + if !interactive() { + bail!( + "vmate-cli needs root to {context}\n\ hint: run `sudo vmate-cli ...`" ); } @@ -38,6 +159,24 @@ pub fn require_root_for(context: &str, no_elevate: bool) -> Result<()> { elevate_with_sudo() } +/// Elevate only when it costs the user nothing. +/// +/// Used by commands that do not need root to do their job (`scan`): when sudo +/// credentials are already cached the run silently becomes root, and when they +/// are not the command simply runs unprivileged. It never prompts. +pub fn elevate_without_prompt(no_elevate: bool) { + if is_root() || no_elevate || is_elevated() { + return; + } + if std::env::var_os("VMATE_NO_ELEVATE").is_some() { + return; + } + if !interactive() || !sudo_can_elevate_without_prompt() { + return; + } + elevate_with_sudo() +} + /// Re-execute the current binary with `sudo`, preserving arguments. /// /// This function never returns: the child runs under sudo and this process @@ -46,7 +185,13 @@ pub fn elevate_with_sudo() -> ! { let exe = std::env::current_exe().unwrap_or_else(|_| PathBuf::from("vmate-cli")); let args: Vec = std::env::args_os().skip(1).collect(); + // `sudo env HOME=... `: `sudo` resets the environment (and, + // on Linux, resets HOME to /root), so the user's identity is re-applied by + // `env` *after* sudo. That works with a stock sudoers file, where `-E` + // would be refused. let status = std::process::Command::new("sudo") + .arg("env") + .args(carried_environment()) .arg(&exe) .args(&args) .status(); @@ -59,3 +204,63 @@ pub fn elevate_with_sudo() -> ! { } } } + +/// One-line privilege status for `vmate-cli doctor`. +pub fn root_summary() -> String { + if !is_root() { + return "no".to_string(); + } + match sudo_user() { + Some(user) => format!("yes (elevated via sudo as {user})"), + None => "yes".to_string(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn carried_environment_restores_home_and_marks_elevation() { + // HOME is set in every environment we run in, so the pair must appear. + let assignments = carried_environment(); + let home = std::env::var("HOME").unwrap_or_else(|_| "/root".to_string()); + assert!( + assignments.iter().any(|a| a == &format!("HOME={home}")), + "HOME must survive sudo: {assignments:?}" + ); + assert!( + assignments + .iter() + .any(|a| a == &format!("{ELEVATED_MARKER}=1")), + "the elevated half must be marked: {assignments:?}" + ); + } + + #[test] + fn carried_environment_has_no_duplicate_keys() { + let mut keys: Vec = carried_environment() + .iter() + .filter_map(|a| a.split_once('=').map(|(k, _)| k.to_string())) + .collect(); + let total = keys.len(); + keys.sort(); + keys.dedup(); + assert_eq!(keys.len(), total, "duplicate assignment: {keys:?}"); + } + + #[test] + fn repair_ownership_ignores_missing_paths() { + // A no-op rather than an error: ownership repair is best effort. + repair_ownership(Path::new("/nonexistent/vmate-ownership-probe")); + } + + #[test] + fn unprivileged_processes_have_no_sudo_identity() { + // Guards the is_root() short circuit: without it a normal run would + // try to chown user-owned files to a bogus uid. + if !is_root() { + assert!(sudo_uid_gid().is_none()); + } + } +}