From fc3c1d51b1be9db6ba6ad286584fbc34bb305f38 Mon Sep 17 00:00:00 2001 From: Andrew Guschin Date: Fri, 30 Oct 2026 03:32:00 +0400 Subject: Fix protected activation ordering and diagnostic review findings --- Cargo.lock | 82 ++++++++++++++++++++++++++- Cargo.toml | 1 + assets/roentgen-xray.service | 1 + docs/implementation-review.md | 33 +++++++++++ docs/validation/linux-vm.txt | 67 ++++++++++++++++++++++ src/controller.rs | 97 ++++++++++++++++++++++++++------ src/diagnostics.rs | 8 ++- src/linux/firewall.rs | 18 ++++-- src/linux/mod.rs | 53 +++++++++++++++--- src/linux/routes.rs | 3 + src/main.rs | 3 + src/model.rs | 19 ++++++- src/runtime.rs | 7 +++ src/storage.rs | 65 ++++++++++++++++++---- src/subscription.rs | 10 ++-- src/tui.rs | 9 ++- src/xray.rs | 39 +++++++------ tests/vm_controller_acceptance.py | 114 +++++++++++++++++++++++++++++++++++++- tests/vm_guard_acceptance.py | 16 ++++++ 19 files changed, 575 insertions(+), 70 deletions(-) create mode 100644 docs/implementation-review.md create mode 100644 docs/validation/linux-vm.txt diff --git a/Cargo.lock b/Cargo.lock index d06b956..5536432 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -133,6 +133,12 @@ dependencies = [ "pkg-config", ] +[[package]] +name = "base64" +version = "0.22.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6" + [[package]] name = "base64" version = "0.23.1" @@ -370,6 +376,12 @@ dependencies = [ "syn 3.0.9", ] +[[package]] +name = "deranged" +version = "0.5.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9e9de72ce2ad1f90dc62fa25f0f430ef85eb4b0d8fa0be4f30373bc40a21d28e" + [[package]] name = "displaydoc" version = "0.2.7" @@ -733,7 +745,7 @@ version = "0.1.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ddc03d96684f9226b8a787cdb71488417b53ab5ea8fdb1dac946cb9431cc8bff" dependencies = [ - "base64", + "base64 0.23.1", "bytes", "futures-channel", "futures-util", @@ -1213,6 +1225,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "num-conv" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "521739c6d2bac4aa25192232afe6841231376b2b26d4d9fae5ecf8ca5772e441" + [[package]] name = "once_cell" version = "1.21.4" @@ -1276,6 +1294,16 @@ version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" +[[package]] +name = "pem" +version = "3.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d30c53c26bc5b31a98cd02d20f25a7c8567146caf63ed593a9d87b2775291be" +dependencies = [ + "base64 0.22.1", + "serde_core", +] + [[package]] name = "percent-encoding" version = "2.3.2" @@ -1303,6 +1331,12 @@ dependencies = [ "zerovec", ] +[[package]] +name = "powerfmt" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4a6394b9e965e73d0a289ee54f589087e2c676aedf60885baf52c76b771e4958" + [[package]] name = "proc-macro-crate" version = "3.5.0" @@ -1440,6 +1474,19 @@ dependencies = [ "unicode-width 0.2.0", ] +[[package]] +name = "rcgen" +version = "0.13.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75e669e5202259b5314d1ea5397316ad400819437857b90861765f24c4cf80a2" +dependencies = [ + "pem", + "ring", + "rustls-pki-types", + "time", + "yasna", +] + [[package]] name = "redox_syscall" version = "0.5.18" @@ -1455,7 +1502,7 @@ version = "0.13.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "16a1cfa75cc186dd73d5818e510e042e40927bccc9c236b061cea97e1eb08029" dependencies = [ - "base64", + "base64 0.23.1", "bytes", "futures-core", "futures-util", @@ -1507,7 +1554,7 @@ version = "0.1.0" dependencies = [ "anyhow", "async-trait", - "base64", + "base64 0.23.1", "clap", "crossterm", "futures-util", @@ -1517,6 +1564,7 @@ dependencies = [ "nftnl", "percent-encoding", "ratatui", + "rcgen", "reqwest", "rtnetlink", "rusqlite", @@ -2050,6 +2098,25 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "time" +version = "0.3.55" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cdb87b95ec50ddfa440816d227a17b2ccbdda963a316a727fda0fc4334f7d134" +dependencies = [ + "deranged", + "num-conv", + "powerfmt", + "serde_core", + "time-core", +] + +[[package]] +name = "time-core" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9e1c906769ad99c88eaa54e728060edef082f8e358ff32030cb7c7d315e81109" + [[package]] name = "tinystr" version = "0.8.5" @@ -2643,6 +2710,15 @@ version = "0.6.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3ad82d2a33cdc9674dc7465672f271e096168fcdbe0f799d9e6db8c5892679dc" +[[package]] +name = "yasna" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e17bb3549cc1321ae1296b9cdc2698e2b6cb1992adfa19a8c72e5b7a738f44cd" +dependencies = [ + "time", +] + [[package]] name = "yoke" version = "0.8.4" diff --git a/Cargo.toml b/Cargo.toml index c449417..4af2171 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,6 +16,7 @@ mnl = { version = "0.3.1", optional = true } nftnl = { version = "0.9.4", optional = true } percent-encoding = "2.3.2" ratatui = "0.29" +rcgen = "0.13" reqwest = { version = "0.13", default-features = false, features = ["rustls", "stream", "socks"] } rtnetlink = "0.23" rusqlite = { version = "0.41.1", features = ["bundled"] } diff --git a/assets/roentgen-xray.service b/assets/roentgen-xray.service index 7cbdaea..17b4dec 100644 --- a/assets/roentgen-xray.service +++ b/assets/roentgen-xray.service @@ -7,6 +7,7 @@ After=roentgen-guard.service Type=simple User=roentgen-xray Group=roentgen-xray +ExecStartPre=/usr/local/bin/roentgen xray-preflight ExecStart=/usr/bin/xray run -config /run/roentgen/xray.json Restart=on-failure RestartSec=5 diff --git a/docs/implementation-review.md b/docs/implementation-review.md new file mode 100644 index 0000000..0bf25c0 --- /dev/null +++ b/docs/implementation-review.md @@ -0,0 +1,33 @@ +# First-release implementation review + +The implementation was reviewed against `40cf3276cdc74bf6f1795990ed600891a637d1fe`. Independent standards and specification reviews inspected candidate `d608ffe`. Corrections followed in the same branch. Review findings do not establish that the release acceptance matrix has passed. + +## Standards + +The standards review found no hard repository-rule violations and three design observations: + +- Numeric TUI screen state allowed a subscription row to be clamped against the configuration list. Refresh now uses the current screen's row bounds. +- Entry validation used free-form strings. Saved entries now use a serialized status enum, and the storage boundary rejects contradictory validation/connection data. +- The controller created Xray probes directly while activation used the runtime boundary. Probe creation now goes through that boundary as well. + +Standards findings: three design observations, zero hard violations. The row-selection bug was the most consequential observation and is corrected. + +## Spec + +The specification review found five correctness issues: + +- Retry could bypass a failed activation preflight. Runtime retries now require successful compatibility checks, and each managed Xray service start repeats the version check. +- An unset selection could reuse a TUN left by the previous controller. Startup first blocks forwarding, and no-selection reconciliation stops the owned core and removes capture. Enabled selection changes block forwarding before committing new state. +- An untrusted local listener could impersonate the probe's SOCKS endpoint. The adapter now generates a private TLS identity for an authenticated HTTPS proxy. Diagnostic requests authenticate that identity before using the proxy, so a reused TCP port cannot authorize a different route. +- Manual diagnostic requests ignored provider cooldowns. Retry-After now prevents further diagnostic requests to that provider, including manual Ping and Speed. +- Competing HTTP and measurement timeouts could turn a completed duration-limited Speed observation into a generic error. Diagnostics now use one measurement deadline and report consumed bytes when the duration cap ends a nonempty measurement. + +Spec findings: five correctness issues, corrected. The stale-selection forwarding path was the most serious finding. VM CLI regressions cover failed preflight retries and startup with stale capture and unset selection. The probe TLS changes and duration/rate-limit behavior still require end-to-end validation with the required current Xray runtime. + +## Validation boundary + +After the user's VM-only instruction, all test execution used the supplied ALT VM. Compilation and formatting used the local checkout. The VM checkpoint precedes package/test preparation and is restored after validation. + +The current package available from that VM's refreshed Sisyphus repository is Xray 26.3.27. The implementation rejects activation with it. The tests therefore distinguish actual guard/controller/polkit/editor/recovery behavior from unverified native-TUN and diagnostic operation with the researched newer Xray contract. The product installer is not counted as a successful installation in this environment; system-bus tests stage their integration fixtures explicitly. + +The final available suite passed on 2026-10-30: 10 Rust CLI/controller acceptance tests and 21 VM workflow checks. The [captured output](validation/linux-vm.txt) records the tested executable digest and each result. Checks include TCP and UDP DNS destination policy, IPv4/IPv6 blocking, retained incoming connections, denial/allow authorization, ordinary-user TUI restoration, editor privacy, failed-preflight retries, stale capture with unset selection, malformed TOML recovery, missing durable state, and uninstall retention/purge. diff --git a/docs/validation/linux-vm.txt b/docs/validation/linux-vm.txt new file mode 100644 index 0000000..604a6de --- /dev/null +++ b/docs/validation/linux-vm.txt @@ -0,0 +1,67 @@ +Candidate SHA-256: 1da493619f430722743b1013faf70da0298d71df620662b6589ccc48e6a5873e +Running: cli_acceptance-8ac5b3df5e4cd10a + +running 2 tests +test subscription_url_cannot_be_passed_on_the_command_line ... ok +test help_describes_the_controller_commands_without_contacting_services ... ok + +test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s + +Running: roentgen-2037e66e0f348630 + +running 0 tests + +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s + +Running: roentgen-0d6b8b6ef4b4cfd3 + +running 0 tests + +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s + +Running: controller_acceptance-7693ccc2191fcbcc + +running 8 tests +test importing_requires_explicit_selection_and_enabling ... ok +test denied_operations_leave_durable_state_unchanged ... ok +test losing_a_selection_keeps_enabled_intent_and_blocks_without_replacement ... ok +test mixed_base64_import_keeps_all_rows_and_prohibits_invalid_selection ... ok +test failed_fetch_preserves_selection_but_an_empty_received_list_clears_it ... ok +test persisted_intent_restores_under_protection_after_controller_restart ... ok +test label_changes_keep_identity_but_duplicate_candidates_clear_selection ... ok +test explicit_unset_and_deletion_never_disable_or_select_another_entry ... ok + +test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s + +Running: vm_guard_acceptance.py +{ + "passed": [ + "private IPv4/IPv6 destinations stay direct on ports 53 and 18080", + "public IPv4/IPv6 destinations are blocked on ports 53 and 18080", + "UDP DNS follows the same IPv4/IPv6 private/public destination rule", + "classified incoming TCP connection survives guard replacement and toggles", + "preexisting outgoing TCP connection receives no established-flow exemption", + "explicit disabled guard restores ordinary direct routing" + ] +} +Running: vm_controller_acceptance.py +{ + "passed": [ + "real HTTP import remains disabled/unselected and public CLI output omits credentials", + "noninteractive non-root mutation is denied with durable state unchanged", + "polkit site allow policy authorizes the actual ordinary bus caller", + "mixed update retains ordered invalid/unsupported rows and normalized identity", + "whole-document failure preserves selection while received empty list clears it", + "unsupported packaged Xray activation is rejected before enabled intent changes", + "service restart retains explicit selection and disabled intent", + "editor receives literal arguments and a user-owned private snapshot that is removed afterward", + "ordinary-user TUI renders, accepts navigation/help/quit, restores terminal modes, and preserves service intent", + "restart and scheduled retry preserve blocked intent after failed compatibility checks", + "restart with unset selection removes stale capture and keeps TUN forwarding blocked", + "explicit recovery persists disabled intent even with malformed editable TOML", + "subscription deletion clears selection without replacement", + "missing durable enabled state fails closed; only explicit recovery initializes disabled state", + "uninstall retains saved data by default, explicit purge removes it, and system Xray remains installed" + ] +} +ALL AVAILABLE VM TESTS PASSED diff --git a/src/controller.rs b/src/controller.rs index 1384ad8..4686118 100644 --- a/src/controller.rs +++ b/src/controller.rs @@ -47,6 +47,7 @@ pub struct Controller { logs: std::collections::VecDeque, next_check: Option, measurements: Vec, + cooldowns: std::collections::HashMap>, } impl Controller { @@ -73,6 +74,7 @@ impl Controller { logs: std::collections::VecDeque::new(), next_check: Some(tokio::time::Instant::now()), measurements: vec![], + cooldowns: std::collections::HashMap::new(), }) } @@ -145,7 +147,7 @@ impl Controller { name: entry.name, host: entry.connection.as_ref().map(|c| c.host.clone()), port: entry.connection.as_ref().map(|c| c.port), - status: entry.status, + status: entry.status.as_str().into(), explanation: entry.explanation, }); } @@ -154,9 +156,13 @@ impl Controller { } pub async fn restore(&mut self) -> Result<()> { - if self.storage.state()?.0 { - self.runtime.arm().await?; - self.reconcile().await?; + let (enabled, selected, _) = self.storage.state()?; + if enabled { + self.runtime.block().await?; + if selected.is_some() { + self.runtime.arm().await?; + self.reconcile().await?; + } } else { self.runtime.disable().await?; } @@ -211,6 +217,16 @@ impl Controller { } Operation::DeleteSubscription { name } => { let id = self.subscription(&name)?.id.clone(); + let (enabled, selected, _) = self.storage.state()?; + if enabled + && self + .storage + .entries(&id)? + .iter() + .any(|entry| Some(&entry.id) == selected.as_ref()) + { + self.runtime.block().await?; + } let mut definitions = self.storage.definitions.clone(); definitions.subscriptions.retain(|s| s.id != id); self.storage.save_definitions(&definitions)?; @@ -241,10 +257,16 @@ impl Controller { } Operation::Select { id } => { self.usable_entry(&id)?; + if self.storage.state()?.0 { + self.runtime.block().await?; + } self.storage.select(Some(&id))?; self.reconcile().await?; } Operation::Unset => { + if self.storage.state()?.0 { + self.runtime.block().await?; + } self.storage.select(None)?; self.reconcile().await?; } @@ -302,21 +324,48 @@ impl Controller { .ok_or_else(|| anyhow!("Select a configuration or supply its ID before testing"))?; let entry = self.usable_entry(id)?; let settings = self.settings(); - let probe = crate::xray::Probe::start( - entry.connection.as_ref().unwrap(), - if speed { - settings.maximum_duration_seconds - } else { - 5 - }, + let url = if speed { + &settings.speed_url + } else { + &settings.ping_url + }; + let provider = url::Url::parse(url)?.origin().ascii_serialization(); + if let Some(deadline) = self.cooldowns.get(&provider) { + ensure!( + deadline.is_some_and(|deadline| tokio::time::Instant::now() >= deadline), + "The diagnostic provider requested a pause; retry after its Retry-After delay" + ); + } + let probe = self + .runtime + .probe( + entry.connection.as_ref().unwrap(), + if speed { + settings.maximum_duration_seconds + } else { + 5 + }, + ) + .await?; + let result = crate::diagnostics::measure( + &settings, + &probe.proxy, + &probe.certificate, + id.into(), + generation, + speed, ) - .await?; - let result = - crate::diagnostics::measure(&settings, &probe.proxy, id.into(), generation, speed) - .await; + .await; probe.stop().await; match result { Ok(observation) => { + if let Some(wait) = observation.retry_after_seconds { + self.cooldowns.insert( + provider, + tokio::time::Instant::now() + .checked_add(std::time::Duration::from_secs(wait)), + ); + } if !speed && Some(id) == selected.as_deref() { let wait = observation.retry_after_seconds.unwrap_or(30).max(30); self.next_check = tokio::time::Instant::now() @@ -421,6 +470,19 @@ impl Controller { } } } + let (enabled, selected, _) = self.storage.state()?; + if enabled { + if let Some(id) = selected { + if let Some(previous) = old.iter().find(|entry| entry.id == id) { + let replacement = entries.iter().find(|entry| entry.id == id); + if replacement.and_then(|entry| entry.connection.as_ref()) + != previous.connection.as_ref() + { + self.runtime.block().await?; + } + } + } + } let affected: std::collections::HashSet<_> = old.iter().map(|e| e.id.as_str()).collect(); self.measurements .retain(|o| !affected.contains(o.configuration_id.as_str())); @@ -443,7 +505,7 @@ impl Controller { .find_entry(id)? .ok_or_else(|| anyhow!("Configuration not found; run configuration list"))?; ensure!( - entry.connection.is_some(), + entry.status == EntryStatus::Supported && entry.connection.is_some(), "Configuration is invalid or unsupported; inspect its explanation" ); Ok(entry) @@ -452,6 +514,9 @@ impl Controller { async fn reconcile(&mut self) -> Result<()> { let (enabled, selected, generation) = self.storage.state()?; if enabled { + if selected.is_none() { + return self.runtime.block().await; + } let entry = selected.map(|id| self.usable_entry(&id)).transpose()?; if let Err(error) = self .runtime diff --git a/src/diagnostics.rs b/src/diagnostics.rs index a2052ec..432a801 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -18,11 +18,12 @@ pub struct Observation { pub retry_after_seconds: Option, } -/// The caller supplies an authenticated, service-owned SOCKS endpoint. This client +/// The caller supplies an authenticated, service-owned HTTPS proxy endpoint. This client /// never inherits environment proxies or creates a direct route to the target. pub async fn measure( settings: &Settings, proxy: &str, + certificate: &str, id: String, generation: u64, speed: bool, @@ -42,11 +43,14 @@ pub async fn measure( }; let proxy = reqwest::Proxy::all(proxy).map_err(|_| anyhow!("Invalid protected probe endpoint"))?; + let certificate = reqwest::Certificate::from_pem(certificate.as_bytes()) + .map_err(|_| anyhow!("Invalid private probe certificate"))?; let client = reqwest::Client::builder() .no_proxy() .proxy(proxy) .redirect(reqwest::redirect::Policy::none()) - .timeout(Duration::from_secs(duration)) + .tls_certs_merge([certificate]) + .retry(reqwest::retry::never()) .user_agent(concat!("Roentgen/", env!("CARGO_PKG_VERSION"))) .build() .map_err(|_| anyhow!("Could not initialize diagnostic TLS client"))?; diff --git a/src/linux/firewall.rs b/src/linux/firewall.rs index 79731b7..8a0dd43 100644 --- a/src/linux/firewall.rs +++ b/src/linux/firewall.rs @@ -84,6 +84,12 @@ pub fn preflight() -> Result<()> { /// Replacing the table is one kernel transaction. Connection marks survive table replacement. pub fn apply(enabled: bool, xray_uid: u32) -> Result<()> { + apply_policy(enabled, xray_uid, false) +} +pub fn forward(xray_uid: u32) -> Result<()> { + apply_policy(true, xray_uid, true) +} +fn apply_policy(enabled: bool, xray_uid: u32, allow_tun: bool) -> Result<()> { let existing = tables()?; let table = Table::new(TABLE, ProtoFamily::Inet); let mut batch = Batch::new(); @@ -124,11 +130,13 @@ pub fn apply(enabled: bool, xray_uid: u32) -> Result<()> { guard.set_policy(Policy::Drop); batch.add(&guard, MsgType::Add); exceptions(&mut batch, &guard, xray_uid); - let mut tun = Rule::new(&guard); - tun.add_expr(&nft_expr!(meta oifname)); - tun.add_expr(&nft_expr!(cmp == &b"roentgen0\0"[..])); - tun.add_expr(&nft_expr!(verdict accept)); - batch.add(&tun, MsgType::Add); + if allow_tun { + let mut tun = Rule::new(&guard); + tun.add_expr(&nft_expr!(meta oifname)); + tun.add_expr(&nft_expr!(cmp == &b"roentgen0\0"[..])); + tun.add_expr(&nft_expr!(verdict accept)); + batch.add(&tun, MsgType::Add); + } } send(batch.finalize()).context("Could not apply Roentgen firewall transaction") } diff --git a/src/linux/mod.rs b/src/linux/mod.rs index 68bb3a8..fd9c471 100644 --- a/src/linux/mod.rs +++ b/src/linux/mod.rs @@ -9,12 +9,14 @@ use std::{path::Path, time::Duration}; pub struct LinuxRuntime { routing: String, applied: Option, + armed: bool, } impl Default for LinuxRuntime { fn default() -> Self { Self { routing: "unknown".into(), applied: None, + armed: false, } } } @@ -25,17 +27,39 @@ impl Runtime for LinuxRuntime { xray::preflight().await?; firewall::preflight()?; routes::preflight().await?; + anyhow::ensure!( + self.applied.is_some() || routes::tun_index().await?.is_none(), + "roentgen0 already exists outside the active owned configuration; resolve the interface conflict before enabling" + ); let (uid, _) = xray::identity()?; firewall::apply(true, uid)?; // Persist the boot guard input before recording enabled intent in SQLite. write_guard(true)?; self.routing = "blocked".into(); + self.armed = true; + Ok(()) + } + async fn block(&mut self) -> Result<()> { + firewall::apply(true, xray::identity()?.0)?; + write_guard(true)?; + self.routing = "blocked".into(); + self.applied = None; + xray::systemctl(&["stop", "roentgen-xray.service"]).await?; + routes::remove().await?; Ok(()) } async fn apply(&mut self, connection: Option<&Trojan>, _: u64) -> Result<()> { - if self.applied.as_ref() == connection { + if connection.is_none() { + return self.block().await; + } + anyhow::ensure!( + self.armed, + "Activation requires successful compatibility checks and an armed guard" + ); + if connection.is_some() && self.applied.as_ref() == connection { if let Some(index) = routes::tun_index().await? { routes::capture(index).await?; + firewall::forward(xray::identity()?.0)?; self.routing = "tun".into(); return Ok(()); } @@ -45,14 +69,22 @@ impl Runtime for LinuxRuntime { xray::systemctl(&["stop", "roentgen-xray.service"]).await?; routes::remove().await?; if let Some(connection) = connection { + anyhow::ensure!( + routes::tun_index().await?.is_none(), + "roentgen0 remained after stopping the owned Xray unit; refusing to take over another interface" + ); + xray::preflight().await?; + firewall::preflight()?; + routes::preflight().await?; xray::write_config( Path::new("/run/roentgen/xray.json"), - &xray::configuration(connection, true, None), + &xray::configuration(connection, true), )?; xray::systemctl(&["start", "roentgen-xray.service"]).await?; for _ in 0..100 { if let Some(index) = routes::tun_index().await? { routes::capture(index).await?; + firewall::forward(xray::identity()?.0)?; self.routing = "tun".into(); self.applied = Some(connection.clone()); return Ok(()); @@ -64,14 +96,16 @@ impl Runtime for LinuxRuntime { Ok(()) } async fn maintain(&mut self, connection: Option<&Trojan>, generation: u64) -> Result<()> { - match routes::tun_index().await? { - Some(index) if connection.is_some() => { - routes::capture(index).await?; - self.routing = "tun".into(); - Ok(()) - } - _ => self.apply(connection, generation).await, + if connection.is_none() { + return self.block().await; + } + if !self.armed { + self.arm().await?; } + self.apply(connection, generation).await + } + async fn probe(&mut self, connection: &Trojan, duration: u64) -> Result { + xray::Probe::start(connection, duration).await } async fn disable(&mut self) -> Result<()> { xray::systemctl(&["stop", "roentgen-xray.service"]).await?; @@ -80,6 +114,7 @@ impl Runtime for LinuxRuntime { write_guard(false)?; firewall::apply(false, xray::identity()?.0)?; self.routing = "direct".into(); + self.armed = false; Ok(()) } fn routing(&self) -> &str { diff --git a/src/linux/routes.rs b/src/linux/routes.rs index 07083bb..4212183 100644 --- a/src/linux/routes.rs +++ b/src/linux/routes.rs @@ -84,6 +84,9 @@ fn owned_rule(rule: &rtnetlink::packet_route::rule::RuleMessage) -> bool { } pub async fn capture(interface: u32) -> Result<()> { + // Only the product-owned TUN changes this setting. Loose reverse-path + // validation accepts proxied replies whose ordinary reverse route is uplink. + std::fs::write("/proc/sys/net/ipv4/conf/roentgen0/rp_filter", "2")?; let handle = connection()?; for route in [ RouteMessageBuilder::::new() diff --git a/src/main.rs b/src/main.rs index 7ad2d81..f4c5373 100644 --- a/src/main.rs +++ b/src/main.rs @@ -59,6 +59,8 @@ enum Command { Daemon, #[command(hide = true)] Guard, + #[command(hide = true)] + XrayPreflight, } #[derive(Subcommand)] @@ -114,6 +116,7 @@ async fn run(cli: Cli) -> Result<()> { Command::Recover { .. } => return roentgen::installation::recover().await, Command::Daemon => return roentgen::daemon::run().await, Command::Guard => return roentgen::daemon::guard().await, + Command::XrayPreflight => return roentgen::xray::preflight().await, _ => {} } let client = Client::system().await?; diff --git a/src/model.rs b/src/model.rs index 8966956..8229207 100644 --- a/src/model.rs +++ b/src/model.rs @@ -16,11 +16,28 @@ impl Trojan { } } +#[derive(Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum EntryStatus { + Supported, + Unsupported, + Invalid, +} +impl EntryStatus { + pub fn as_str(self) -> &'static str { + match self { + Self::Supported => "supported", + Self::Unsupported => "unsupported", + Self::Invalid => "invalid", + } + } +} + #[derive(Clone, Serialize, Deserialize)] pub struct Entry { pub id: String, pub name: String, - pub status: String, + pub status: EntryStatus, pub explanation: Option, pub connection: Option, } diff --git a/src/runtime.rs b/src/runtime.rs index 96ffd0d..2d5552d 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -7,9 +7,16 @@ pub trait Runtime: Send { /// Protection must be installed before durable enabled intent changes. async fn arm(&mut self) -> Result<()>; async fn apply(&mut self, connection: Option<&Trojan>, generation: u64) -> Result<()>; + async fn block(&mut self) -> Result<()> { + self.arm().await?; + self.apply(None, 0).await + } async fn maintain(&mut self, connection: Option<&Trojan>, generation: u64) -> Result<()> { self.apply(connection, generation).await } + async fn probe(&mut self, _: &Trojan, _: u64) -> Result { + bail!("Protected diagnostic probes are unavailable in this runtime") + } /// Called only after disabled intent has been durably recorded. async fn disable(&mut self) -> Result<()>; fn routing(&self) -> &str; diff --git a/src/storage.rs b/src/storage.rs index cb7fcd9..93cb98c 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -1,4 +1,4 @@ -use crate::model::{Definitions, Entry}; +use crate::model::{Definitions, Entry, EntryStatus}; use anyhow::{Context, Result, ensure}; use rusqlite::{Connection, OptionalExtension, params}; use std::{ @@ -70,7 +70,30 @@ impl Storage { ); crate::subscription::subscription_url(&subscription.url)?; } + #[derive(serde::Deserialize)] + #[serde(deny_unknown_fields)] + struct GuardInput { + version: u32, + enabled: bool, + } + let guard_path = config_dir.join("guard.toml"); + let guard_enabled = if guard_path.exists() { + let guard: GuardInput = toml::from_str(&fs::read_to_string(&guard_path)?) + .context("Corrupt guard state; use explicit recover --disable")?; + ensure!( + guard.version == 1, + "Unsupported guard state; use explicit recover --disable" + ); + guard.enabled + } else { + false + }; let path = data_dir.join("state.sqlite3"); + let database_existed = path.exists(); + ensure!( + database_existed || !guard_enabled, + "Durable state is missing while enabled protection is requested; use explicit recover --disable" + ); if path.exists() { ensure!( fs::symlink_metadata(&path)?.is_file(), @@ -84,6 +107,10 @@ impl Storage { let version: u32 = db.query_row("PRAGMA user_version", [], |r| r.get(0))?; ensure!(version <= 1, "Unsupported database version"); if version == 0 { + ensure!( + !database_existed, + "Existing database has no state schema; use explicit recover --disable" + ); db.execute_batch("BEGIN IMMEDIATE; CREATE TABLE entries (subscription_id TEXT NOT NULL, ordinal INTEGER NOT NULL, id TEXT PRIMARY KEY, data TEXT NOT NULL); CREATE TABLE state (singleton INTEGER PRIMARY KEY CHECK(singleton=1), enabled INTEGER NOT NULL CHECK(enabled IN (0,1)), selected TEXT, generation INTEGER NOT NULL); @@ -156,13 +183,7 @@ impl Storage { let values = query .query_map([subscription_id], |r| r.get::<_, String>(0))? .collect::>>()?; - values - .iter() - .map(|v| { - serde_json::from_str(v) - .context("Corrupt saved configuration; explicit recovery is required") - }) - .collect() + values.iter().map(|v| decode_entry(v)).collect() } pub fn find_entry(&self, id: &str) -> Result> { @@ -171,7 +192,7 @@ impl Storage { r.get::<_, String>(0) }) .optional()? - .map(|v| serde_json::from_str(&v).context("Corrupt saved configuration")) + .map(|v| decode_entry(&v)) .transpose() } @@ -254,7 +275,17 @@ pub fn persist_recovery_disabled(data_dir: &Path) -> Result<()> { let db = Connection::open(&path)?; fs::set_permissions(&path, fs::Permissions::from_mode(0o600))?; db.busy_timeout(std::time::Duration::from_secs(5))?; - db.execute_batch("PRAGMA synchronous=FULL; BEGIN EXCLUSIVE;")?; + db.execute_batch("PRAGMA synchronous=FULL;")?; + let version: u32 = db.query_row("PRAGMA user_version", [], |row| row.get(0))?; + ensure!( + version <= 1, + "Unknown database schema; repair it before explicit recovery" + ); + db.execute_batch("BEGIN EXCLUSIVE; + CREATE TABLE IF NOT EXISTS entries (subscription_id TEXT NOT NULL, ordinal INTEGER NOT NULL, id TEXT PRIMARY KEY, data TEXT NOT NULL); + CREATE TABLE IF NOT EXISTS state (singleton INTEGER PRIMARY KEY CHECK(singleton=1), enabled INTEGER NOT NULL CHECK(enabled IN (0,1)), selected TEXT, generation INTEGER NOT NULL); + INSERT OR IGNORE INTO state VALUES (1,0,NULL,0); + PRAGMA user_version=1;")?; db.execute( "UPDATE state SET enabled=0,generation=generation+1 WHERE singleton=1", [], @@ -262,3 +293,17 @@ pub fn persist_recovery_disabled(data_dir: &Path) -> Result<()> { db.execute_batch("COMMIT")?; Ok(()) } + +fn decode_entry(value: &str) -> Result { + let entry: Entry = serde_json::from_str(value).context("Corrupt saved configuration")?; + ensure!( + entry.status == EntryStatus::Supported + && entry.connection.is_some() + && entry.explanation.is_none() + || entry.status != EntryStatus::Supported + && entry.connection.is_none() + && entry.explanation.is_some(), + "Saved entry has contradictory validation state; explicit recovery is required" + ); + Ok(entry) +} diff --git a/src/subscription.rs b/src/subscription.rs index df5b318..8abcb1c 100644 --- a/src/subscription.rs +++ b/src/subscription.rs @@ -1,4 +1,4 @@ -use crate::model::{Entry, Trojan, display_text}; +use crate::model::{Entry, EntryStatus, Trojan, display_text}; use anyhow::{Result, anyhow, bail, ensure}; use async_trait::async_trait; use base64::{Engine, engine::general_purpose}; @@ -191,7 +191,7 @@ fn parse_entry(raw: &str) -> Entry { let mut entry = Entry { id: Uuid::new_v4().to_string(), name: "Invalid entry".into(), - status: "invalid".into(), + status: EntryStatus::Invalid, explanation: None, connection: None, }; @@ -205,7 +205,7 @@ fn parse_entry(raw: &str) -> Entry { entry.name = display_text(&decode(label)?); } if url.scheme() != "trojan" { - entry.status = "unsupported".into(); + entry.status = EntryStatus::Unsupported; bail!("Only Trojan TCP + TLS connections are supported"); } ensure!( @@ -276,7 +276,7 @@ fn parse_entry(raw: &str) -> Entry { "allowInsecure" if value == "0" || value == "false" => {} "headerType" if value == "none" => {} _ => { - entry.status = "unsupported".into(); + entry.status = EntryStatus::Unsupported; bail!("Unsupported transport, TLS, or connection option"); } } @@ -296,7 +296,7 @@ fn parse_entry(raw: &str) -> Entry { })(); match result { Ok(connection) => { - entry.status = "supported".into(); + entry.status = EntryStatus::Supported; entry.connection = Some(connection); } Err(error) => entry.explanation = Some(error.to_string()), diff --git a/src/tui.rs b/src/tui.rs index 9cb69a7..4f8f489 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -65,7 +65,14 @@ impl Ui { self.entries = client.configurations(self.viewed.as_deref()).await?; self.settings = client.read("Settings").await?; self.logs = client.read("Logs").await?; - self.row = self.row.min(self.visible().len().saturating_sub(1)); + self.row = self.row.min( + if self.tab == 1 { + self.subscriptions.len() + } else { + self.visible().len() + } + .saturating_sub(1), + ); Ok(()) } fn visible(&self) -> Vec<&ConfigurationSummary> { diff --git a/src/xray.rs b/src/xray.rs index 5ac8b62..c57147c 100644 --- a/src/xray.rs +++ b/src/xray.rs @@ -69,14 +69,11 @@ pub async fn preflight() -> Result<()> { Ok(()) } -pub fn configuration(connection: &Trojan, tun: bool, probe: Option<(u16, &str)>) -> Value { +pub fn configuration(connection: &Trojan, tun: bool) -> Value { let mut inbounds = vec![]; if tun { inbounds.push(json!({"tag":"host-tun","protocol":"tun","settings":{"name":"roentgen0","MTU":1500,"gateway":["198.18.0.1/30","fdfe:726f:656e::1/126"],"autoSystemRoutingTable":[],"autoOutboundsInterface":"","autoSystemDnsToGateway":false}})); } - if let Some((port, password)) = probe { - inbounds.push(json!({"tag":"probe","listen":"127.0.0.1","port":port,"protocol":"socks","settings":{"auth":"password","accounts":[{"user":"roentgen","pass":password}],"udp":false}})); - } let mut tls = json!({"serverName":connection.server_name,"allowInsecure":false}); if !connection.alpn.is_empty() { tls["alpn"] = json!(connection.alpn); @@ -121,6 +118,7 @@ pub struct Probe { unit: String, path: std::path::PathBuf, pub proxy: String, + pub certificate: String, active: bool, } impl Probe { @@ -131,18 +129,33 @@ impl Probe { let token = uuid::Uuid::new_v4().simple().to_string(); let unit = format!("roentgen-probe-{token}"); let path = Path::new(RUNTIME_DIR).join(format!("{unit}.json")); - write_config( - &path, - &configuration(connection, false, Some((port, &token))), - )?; + let certified = rcgen::generate_simple_self_signed(vec!["127.0.0.1".into()])?; + let certificate = certified.cert.pem(); + let key = certified.key_pair.serialize_pem(); + let mut configuration = configuration(connection, false); + configuration["inbounds"] = json!([{ + "tag":"probe", "listen":"127.0.0.1", "port":port, "protocol":"http", + "settings":{"accounts":[{"user":"roentgen","pass":token}]}, + "streamSettings":{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{ + "certificate":certificate.lines().collect::>(), "key":key.lines().collect::>() + }]}} + }]); + write_config(&path, &configuration)?; drop(listener); let runtime_limit = format!("RuntimeMaxSec={}", duration.saturating_add(10)); + let probe = Self { + unit, + path, + proxy: format!("https://roentgen:{token}@127.0.0.1:{port}"), + certificate, + active: true, + }; let result = Command::new("/usr/bin/systemd-run") .args([ "--quiet", "--collect", "--unit", - &unit, + &probe.unit, "--uid=roentgen-xray", "--property=NoNewPrivileges=yes", "--property=PrivateTmp=yes", @@ -154,17 +167,11 @@ impl Probe { "run", "-config", ]) - .arg(&path) + .arg(&probe.path) .stdout(Stdio::null()) .stderr(Stdio::null()) .status() .await; - let probe = Self { - unit, - path, - proxy: format!("socks5h://roentgen:{token}@127.0.0.1:{port}"), - active: true, - }; if !result?.success() { probe.stop().await; anyhow::bail!("Could not launch protected Xray probe"); diff --git a/tests/vm_controller_acceptance.py b/tests/vm_controller_acceptance.py index d39b16f..1d3405a 100644 --- a/tests/vm_controller_acceptance.py +++ b/tests/vm_controller_acceptance.py @@ -6,6 +6,13 @@ import json import os from pathlib import Path import socket +import sqlite3 +import pty +import select +import termios +import fcntl +import struct +import pwd import subprocess import threading import time @@ -126,19 +133,122 @@ def main(): results.append('service restart retains explicit selection and disabled intent') editor = ROOT / 'check-editor.py' editor.write_text('import os,pathlib,sys\np=pathlib.Path(sys.argv[-1])\nassert p.suffix==".txt" and (p.stat().st_mode & 0o777)==0o600\nassert p.stat().st_uid==os.getuid()\nassert sys.argv[1:3]==["--literal","two words"]\npathlib.Path("/var/tmp/roentgen-editor-path").write_text(str(p))\n') - env = dict(os.environ, EDITOR=f'python3 {editor} --literal "two words"') + env = dict(os.environ, TMPDIR='/var/tmp', EDITOR=f'python3 {editor} --literal "two words"') cli('logs', '--editor', user='rustacean', env=env) snapshot = Path('/var/tmp/roentgen-editor-path').read_text() assert not Path(snapshot).exists() results.append('editor receives literal arguments and a user-owned private snapshot that is removed afterward') + before = status() + process, terminal = pty.fork() + if process == 0: + fcntl.ioctl(0, termios.TIOCSWINSZ, struct.pack('HHHH', 32, 120, 0, 0)) + user = pwd.getpwnam('rustacean') + os.initgroups(user.pw_name, user.pw_gid) + os.setgid(user.pw_gid); os.setuid(user.pw_uid) + os.environ['TMPDIR'] = '/var/tmp' + os.execv(BIN, [BIN, 'tui']) + output = b'' + deadline = time.monotonic() + 10 + while b'Connections' not in output and time.monotonic() < deadline: + if select.select([terminal], [], [], 0.2)[0]: output += os.read(terminal, 65536) + assert b'Connections' in output, 'TUI did not render under the invoking ordinary user' + os.write(terminal, b'l?') + time.sleep(0.3) + os.write(terminal, b'\x1b') + time.sleep(0.1) + os.write(terminal, b'q') + done = False + while time.monotonic() < deadline: + if select.select([terminal], [], [], 0.1)[0]: + with contextlib.suppress(OSError): output += os.read(terminal, 65536) + pid, exit_status = os.waitpid(process, os.WNOHANG) + if pid: + assert os.waitstatus_to_exitcode(exit_status) == 0 + done = True + break + if not done: + os.kill(process, 9); os.waitpid(process, 0) + raise AssertionError('TUI did not exit after q') + attributes = termios.tcgetattr(terminal) + assert attributes[3] & termios.ICANON and attributes[3] & termios.ECHO + assert b'\x1b[?1049l' in output + os.close(terminal) + assert status()['selected_configuration'] == before['selected_configuration'] + assert status()['requested_enabled'] == before['requested_enabled'] + results.append('ordinary-user TUI renders, accepts navigation/help/quit, restores terminal modes, and preserves service intent') + # Persisted enabled intent must not bypass a rejected preflight on retry. + command('systemctl', 'stop', 'roentgen.service') + with sqlite3.connect('/var/lib/roentgen/state.sqlite3') as database: + database.execute('UPDATE state SET enabled=1 WHERE singleton=1') + Path('/etc/roentgen/guard.toml').write_text('version = 1\nenabled = true\n') + cli('guard') + command('systemctl', 'start', 'roentgen.service') + time.sleep(6) + current = status() + assert current['requested_enabled'] and current['routing'] == 'blocked' + assert 'Unsupported Xray release' in current['last_error'] + assert 'roentgen0' not in command('ip', 'link', 'show').stdout + results.append('restart and scheduled retry preserve blocked intent after failed compatibility checks') + # An unset selection must discard stale capture even if a TUN survived. + command('systemctl', 'stop', 'roentgen.service') + command('ip', 'tuntap', 'add', 'dev', 'roentgen0', 'mode', 'tun') + command('ip', 'link', 'set', 'roentgen0', 'up') + command('ip', 'route', 'add', 'default', 'dev', 'roentgen0', 'table', '20260', 'proto', '186') + command('ip', 'rule', 'add', 'priority', '10260', 'fwmark', '0x02000000/0x02000000', 'lookup', '20260') + with sqlite3.connect('/var/lib/roentgen/state.sqlite3') as database: + database.execute('UPDATE state SET selected=NULL WHERE singleton=1') + command('systemctl', 'start', 'roentgen.service') + assert status()['routing'] == 'blocked' and status()['selected_configuration'] is None + assert '10260' not in command('ip', 'rule', 'show').stdout + assert 'oifname "roentgen0" accept' not in command('nft', 'list', 'table', 'inet', 'roentgen').stdout + command('ip', 'link', 'del', 'roentgen0') + results.append('restart with unset selection removes stale capture and keeps TUN forwarding blocked') + # Recovery does not depend on parsing a broken editable definition file. + config = Path('/etc/roentgen/config.toml') + saved = config.read_bytes() + config.write_text('broken = [') + cli('recover', '--disable') + with sqlite3.connect('/var/lib/roentgen/state.sqlite3') as database: + assert database.execute('SELECT enabled FROM state').fetchone()[0] == 0 + config.write_bytes(saved) + command('systemctl', 'start', 'roentgen.service') + assert not status()['requested_enabled'] and status()['routing'] == 'direct' + results.append('explicit recovery persists disabled intent even with malformed editable TOML') cli('subscription', 'delete', 'Personal') assert status()['selected_configuration'] is None results.append('subscription deletion clears selection without replacement') + command('systemctl', 'stop', 'roentgen.service') + with sqlite3.connect('/var/lib/roentgen/state.sqlite3') as database: + database.execute('UPDATE state SET enabled=1 WHERE singleton=1') + Path('/etc/roentgen/guard.toml').write_text('version = 1\nenabled = true\n') + cli('guard') + database = Path('/var/lib/roentgen/state.sqlite3') + database.replace('/var/lib/roentgen/state.sqlite3.missing-fixture') + command('systemctl', 'start', 'roentgen.service', ok=False) + assert not database.exists() + assert 'policy drop' in command('nft', 'list', 'table', 'inet', 'roentgen').stdout + cli('recover', '--disable') + with sqlite3.connect(database) as connection: + assert connection.execute('SELECT enabled FROM state').fetchone()[0] == 0 + results.append('missing durable enabled state fails closed; only explicit recovery initializes disabled state') + cli('uninstall') + assert Path('/etc/roentgen/config.toml').exists() and Path('/var/lib/roentgen/state.sqlite3').exists() + assert not target.exists() and not Path('/etc/systemd/system/roentgen.service').exists() + assert 'roentgen' not in command('nft', 'list', 'tables').stdout + # Purge is exercised after re-staging the service assets as a VM fixture. + for source, destination in destinations.items(): + Path(destination).write_bytes((assets / source).read_bytes()) + target.write_bytes(Path(BIN).read_bytes()); target.chmod(0o755) + command('systemctl', 'daemon-reload') + cli('uninstall', '--purge') + assert not Path('/etc/roentgen').exists() and not Path('/var/lib/roentgen').exists() + assert command('/usr/bin/xray', 'version').stdout.startswith('Xray 26.3.27') + results.append('uninstall retains saved data by default, explicit purge removes it, and system Xray remains installed') print(json.dumps({'passed': results}, indent=2)) finally: server.shutdown() if policy.exists(): policy.unlink() - command('systemctl', 'stop', 'roentgen.service', ok=True) + subprocess.run(['systemctl', 'stop', 'roentgen.service'], capture_output=True) if __name__ == '__main__': main() diff --git a/tests/vm_guard_acceptance.py b/tests/vm_guard_acceptance.py index ff99c33..8886d7b 100644 --- a/tests/vm_guard_acceptance.py +++ b/tests/vm_guard_acceptance.py @@ -80,8 +80,13 @@ def serve(host,port): c.sendall(data) while True: c,_=s.accept();threading.Thread(target=handle,args=(c,),daemon=True).start() +def udp(host): + s=socket.socket(socket.AF_INET6 if ':' in host else socket.AF_INET,socket.SOCK_DGRAM);s.bind((host,53)) + while True: + data,peer=s.recvfrom(1024);s.sendto(data,peer) for host in ['198.51.100.2','10.254.0.2','2001:db8:726f::2','fded:726f::2']: for port in [53,18080]:threading.Thread(target=serve,args=(host,port),daemon=True).start() + threading.Thread(target=udp,args=(host,),daemon=True).start() time.sleep(120) ''' children.append(subprocess.Popen(['ip', 'netns', 'exec', 'roentgen-peer', 'python3', '-u', '-c', server], stdout=subprocess.DEVNULL, stderr=subprocess.PIPE)) @@ -123,6 +128,17 @@ for line in sys.stdin: else: raise AssertionError(f'Public direct fallback leaked to {host}:{port}') results.append('public IPv4/IPv6 destinations are blocked on ports 53 and 18080') + for host, allowed in [('10.254.0.2', True), ('fded:726f::2', True), ('198.51.100.2', False), ('2001:db8:726f::2', False)]: + with socket.socket(socket.AF_INET6 if ':' in host else socket.AF_INET, socket.SOCK_DGRAM) as udp: + udp.settimeout(1) + try: + udp.sendto(b'dns-port-observation', (host, 53)) + data, _ = udp.recvfrom(1024) + except OSError: + assert not allowed, f'Private UDP DNS was blocked: {host}' + else: + assert allowed and data == b'dns-port-observation', f'Public UDP DNS leaked: {host}' + results.append('UDP DNS follows the same IPv4/IPv6 private/public destination rule') incoming.stdin.write('enabled\n'); incoming.stdin.flush() assert incoming.stdout.readline().strip() == 'ok' guard(False) -- cgit v1.2.3