diff --git a/README.md b/README.md index f888b4a..e2d1933 100644 --- a/README.md +++ b/README.md @@ -224,7 +224,7 @@ scp -P 2323 dist/SHA256SUMS root@:/root/ ```bash cd /root sha256sum -c SHA256SUMS -chmod 644 /root/awg2-harness_0.2.6_amd64.deb +chmod 644 /root/awg2-harness_0.1.6_amd64.deb apt install ./awg2-harness_*_amd64.deb install -d -m 0755 /etc/awg2-harness install -m 0644 /root/awg2-harness.config.toml /etc/awg2-harness/config.toml @@ -237,7 +237,7 @@ install -m 0644 /root/awg2-harness.config.toml /etc/awg2-harness/config.toml ```bash cd /root sha256sum -c SHA256SUMS -chmod 644 /root/awg2-harness_0.2.6_amd64.deb +chmod 644 /root/awg2-harness_0.1.6_amd64.deb apt install ./awg2-harness_*_amd64.deb install -d -m 0755 /etc/awg2-harness cp /usr/share/doc/awg2-harness/repo/config.example.toml /etc/awg2-harness/config.toml diff --git a/awg2-harness.service b/awg2-harness.service index 0dc7299..f3f1d3b 100644 --- a/awg2-harness.service +++ b/awg2-harness.service @@ -1,8 +1,12 @@ [Unit] Description=AWG2 Harness multi-orchestrator After=network-online.target docker.service -Wants=network-online.target -Requires=docker.service +# Wants, not Requires. The daemon is built to outlive Docker being down: it reconnects to +# docker events, retries traffic and firewall reconciles, and treats a missing container as +# a transient state rather than a fatal one. Requires= would have systemd stop the harness +# whenever Docker is stopped, and Restart=on-failure does not cover a dependency-ordered +# stop - so the reconnect loop it has never got the chance to run. +Wants=network-online.target docker.service # Restart backoff below can span ~5 minutes, so the failure window has to be wider than # the old 60s or the limit would trip during a single legitimate backoff sequence. StartLimitIntervalSec=15min diff --git a/docs/ACCEPTANCE_CHECKLIST.md b/docs/ACCEPTANCE_CHECKLIST.md index 8e052c6..b9589a8 100644 --- a/docs/ACCEPTANCE_CHECKLIST.md +++ b/docs/ACCEPTANCE_CHECKLIST.md @@ -141,8 +141,11 @@ - [ ] Файл по пути `99-awg2-harness.conf` без этого маркера остаётся нетронутым и не ломает `ensure-swap`. - [ ] При `swap.enabled = false` файл `90-awg2-harness-swap.conf` удаляется, а swapfile, его активность и запись в `/etc/fstab` **остаются** — после перезагрузки swap снова включится; удаляется только persistence `vm.swappiness`. - [ ] Изменение только `swap.priority` не пересоздаёт swapfile (проверяется по неизменившемуся mtime файла). -- [ ] При изменении `swap.size_mb` на активном swap хост не остаётся без swap во время `fallocate`/`mkswap`: в `/proc/swaps` всё время присутствует хотя бы одна запись. +- [ ] При изменении `swap.size_mb` на активном swap хост не остаётся без swap во время `mkswap --file`: в `/proc/swaps` всё время присутствует хотя бы одна запись. - [ ] Если `swapoff` второго чужого swap падает, ранее отключённые чужие swap возвращаются в активное состояние. +- [ ] При откате resize swap возвращается с **прежним** priority, а не с новым (проверяется заведомо неприменимым `swap.priority`). +- [ ] Kill процесса в середине resize: следующий запуск с активным `.new` или `.previous` даёт понятную ошибку с командой `swapoff`, а не бесконечно падает на удалении активного файла. +- [ ] После отката в `/proc/swaps` присутствует ровно исходный swap; `.new` отключён и удалён. - [ ] Нечитаемый managed sysctl-файл даёт в `status` причину `managed sysctl file is unreadable`, а не «файл отсутствует». - [ ] При `reboot.enabled = false` файл `/run/awg2-harness/reboot.plan` удалён, `status` не показывает устаревший план. - [ ] После принятия запроса на перезагрузку `reboot.plan` удалён. @@ -155,6 +158,9 @@ ## I. Anti-torrent guard на nftables - [ ] Если `anti_torrent.enabled = true`, в Amnezia netns есть `table inet awg2_harness`. +- [ ] На **хосте** таблицы `inet awg2_harness` при `apply_in_amnezia_netns = true` нет: `nft list table inet awg2_harness` в host namespace возвращает ошибку «No such file or directory». +- [ ] При недоступном Docker `apply-firewall` с `anti_torrent.enabled = false` завершается ошибкой, а не молчаливым успехом. +- [ ] `systemctl stop docker` не останавливает `awg2-harness.service` (проверка отсутствия `Requires=docker.service`). - [ ] Есть chain `forward_antitorrent` с hook `forward` и policy `accept`. - [ ] Правило ограничивает `ip saddr `, default `10.8.1.0/24`. - [ ] Лимит считается per source VPN IP через meter/set, а не глобально по всей подсети. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index b299ef4..3c1a899 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -200,7 +200,7 @@ swappiness = 1 Ограничения `swap.path`: путь должен быть либо `/swapfile`, либо файлом непосредственно в `/var/lib/awg2-harness/`. Дополнительно: абсолютный, без компонентов `.` и `..`, без пробелов, управляющих символов и обратных слэшей; на этапе применения проверяется, что ни один родительский каталог не является symlink. -Ограничение корней обязательно, а не косметика. Если родительский каталог доступен на запись кому-то кроме root, между проверкой пути и запуском `fallocate`/`mkswap` этот пользователь успевает подменить файл на symlink и перенаправить операцию, выполняемую от root. Оба разрешённых корня принадлежат root, поэтому окна не возникает. `/swapfile` оставлен для совместимости с уже развёрнутыми конфигурациями. +Ограничение корней обязательно, а не косметика. Если родительский каталог доступен на запись кому-то кроме root, между проверкой пути и запуском `mkswap --file` этот пользователь успевает подменить файл на symlink и перенаправить операцию, выполняемую от root. Оба разрешённых корня принадлежат root, поэтому окна не возникает. `/swapfile` оставлен для совместимости с уже развёрнутыми конфигурациями. Владение фиксируется в `/var/lib/awg2-harness/swap-ownership.conf` по `device` + `inode` + пути, а не по маркеру рядом с файлом. Файл, которого нет в этой записи, или файл с тем же путём, но другими `device`/`inode` (то есть подменённый), считается чужим: он никогда не пересоздаётся и не удаляется, вместо этого возвращается ошибка. Чужие активные swap только выключаются — их файлы пакет не удаляет. diff --git a/docs/IMPLEMENTATION_SPEC.md b/docs/IMPLEMENTATION_SPEC.md index 4e79602..820864b 100644 --- a/docs/IMPLEMENTATION_SPEC.md +++ b/docs/IMPLEMENTATION_SPEC.md @@ -649,7 +649,7 @@ inode = 123456 - нет пробелов, управляющих символов и обратных слэшей; - путь равен `/swapfile` **либо** лежит непосредственно в `/var/lib/awg2-harness/` (без вложенных каталогов). -Это allow-list, а не deny-list, и это принципиально. Список запрещённых путей закрывает только те случаи, которые кто-то придумал; значение имеет другое — может ли родительский каталог писать кто-то кроме root. Между preflight-проверкой и запуском `fallocate`/`dd`/`mkswap` непривилегированный пользователь с правом записи в родительский каталог успевает подменить финальный компонент на symlink и перенаправить операцию, выполняемую от root. Это классический TOCTOU, и никакое количество `stat` перед использованием его не закрывает — окно устраняется только тем, что запись в родительский каталог доступна лишь root. +Это allow-list, а не deny-list, и это принципиально. Список запрещённых путей закрывает только те случаи, которые кто-то придумал; значение имеет другое — может ли родительский каталог писать кто-то кроме root. Между preflight-проверкой и запуском `mkswap --file` непривилегированный пользователь с правом записи в родительский каталог успевает подменить финальный компонент на symlink и перенаправить операцию, выполняемую от root. Это классический TOCTOU, и никакое количество `stat` перед использованием его не закрывает — окно устраняется только тем, что запись в родительский каталог доступна лишь root. Оба разрешённых корня root-owned. `/swapfile` оставлен, чтобы уже развёрнутые конфигурации не требовали правки. @@ -664,7 +664,7 @@ inode = 123456 - резервная копия обязательна. Если её не удалось записать, целевой файл не трогается вовсе — ситуация, в которой копия не пишется, и есть та, в которой она нужна; - копий две: `/etc/fstab.awg2-harness.bak` перезаписывается перед каждой правкой и содержит состояние до неё, а `/etc/fstab.awg2-harness.original` создаётся один раз через `create_new` и хранит содержимое до первого вмешательства пакета; - запись идёт во временный файл рядом с целевым, с `create_new`, `sync_all` и переносом прав с оригинала; устаревший временный файл от убитого процесса удаляется явно, а не переиспользуется; -- кандидат проверяется через `findmnt --verify --tab-file` до подмены. Если `findmnt` недоступен, шаг пропускается; если он отработал и отверг файл — операция прекращается; +- кандидат проверяется через `findmnt --verify --tab-file` до подмены. Проверка обязательна: `findmnt` входит в `util-linux`, от которого пакет и так зависит, и включён в обязательные host tools при `swap.enabled = true`. Невозможность его запустить и отказ файла одинаково прекращают операцию — молча пропускать единственную структурную проверку файла, от которого зависит загрузка хоста, недопустимо; - подмена — атомарный `rename`, после него каталог синхронизируется; - после `rename` содержимое перечитывается и сверяется с записанным. Расхождение приводит к восстановлению из резервной копии. diff --git a/traffic_core/src/config.rs b/traffic_core/src/config.rs index 0d34a42..2924932 100644 --- a/traffic_core/src/config.rs +++ b/traffic_core/src/config.rs @@ -493,14 +493,11 @@ fn tool_exists(tool: &str) -> bool { "nsenter" => &[&["--help"]], "nft" => &[&["--version"]], "systemctl" => &[&["--version"], &["--help"]], - "fallocate" => &[&["--help"]], - "chmod" => &[&["--help"]], "mkswap" => &[&["--help"]], "swapon" => &[&["--help"]], "swapoff" => &[&["--help"]], "sysctl" => &[&["--version"], &["--help"]], "findmnt" => &[&["--version"]], - "dd" => &[&["--help"]], _ => &[&["--version"], &["--help"], &["-V"]], }; diff --git a/traffic_core/src/daemon.rs b/traffic_core/src/daemon.rs index cc46fc2..adf7d7e 100644 --- a/traffic_core/src/daemon.rs +++ b/traffic_core/src/daemon.rs @@ -268,7 +268,15 @@ fn persist_state(state: &LastActionState) { let _ = fs::write(LAST_STATE_FILE, content); } +/// Serialises the read-modify-write in [`append_event`]. +/// +/// The reconcile loop and the Docker events thread both append. Without this each one +/// reads the whole file, rewrites it and truncates, so two overlapping appends drop one +/// of the entries. +static EVENTS_LOCK: Mutex<()> = Mutex::new(()); + fn append_event(event: &str) { + let _guard = EVENTS_LOCK.lock(); let _ = fs::create_dir_all("/run/awg2-harness"); let ts = now_rfc3339(); let mut lines: Vec = fs::read_to_string(EVENTS_FILE) diff --git a/traffic_core/src/firewall/antitorrent.rs b/traffic_core/src/firewall/antitorrent.rs index 13a6e9e..67b01df 100644 --- a/traffic_core/src/firewall/antitorrent.rs +++ b/traffic_core/src/firewall/antitorrent.rs @@ -10,7 +10,7 @@ use serde_json::Value; -use crate::{config::Config, docker, netns::NsTransport}; +use crate::{config::Config, docker, error::AppError, netns::NsTransport}; use super::{ ANTITORRENT_TABLE, firewall_transport, @@ -49,11 +49,14 @@ pub(super) fn apply_antitorrent_firewall(cfg: &Config) -> anyhow::Result<()> { // failing to put anything back. The four separate commands this replaces could also // leave a half-built table - a table and counter but no chain, or a chain with no rule. let present = table_exists(transport.as_ref(), ANTITORRENT_TABLE)?; - run_nft_batch(&build_antitorrent_batch_script( - &cfg.anti_torrent.peer_ipv4_cidr, - cfg.anti_torrent.max_connections_per_peer, - present, - ))?; + run_nft_batch( + transport.as_ref(), + &build_antitorrent_batch_script( + &cfg.anti_torrent.peer_ipv4_cidr, + cfg.anti_torrent.max_connections_per_peer, + present, + ), + )?; let verify = inspect_firewall(transport.as_ref(), cfg)?; if firewall_has_drift(&verify) { @@ -76,8 +79,16 @@ fn apply_antitorrent_disabled(cfg: &Config) -> anyhow::Result<()> { &["nft", "delete", "table", "inet", ANTITORRENT_TABLE], )?; } - Err(_) => { - // container is absent now: nothing to remove in its netns + // Only an absent container is benign - there is no namespace to clean up in. + // Every other failure (Docker unreachable, permission denied on the socket, a + // malformed inspect response) means we do not know whether a stale table is + // still filtering traffic, and swallowing it reported success for work that + // never happened. + Err(e) if is_container_absent(&e) => {} + Err(e) => { + return Err(e.context( + "cannot reach the Amnezia container to remove the anti-torrent table", + )); } } } else { @@ -90,6 +101,11 @@ fn apply_antitorrent_disabled(cfg: &Config) -> anyhow::Result<()> { Ok(()) } +/// Distinguishes "no Amnezia container is running" from every other discovery failure. +fn is_container_absent(e: &anyhow::Error) -> bool { + matches!(e.downcast_ref::(), Some(AppError::NotFound(_))) +} + fn inspect_firewall(transport: &dyn NsTransport, cfg: &Config) -> anyhow::Result { let table = match transport.run(&["nft", "list", "table", "inet", ANTITORRENT_TABLE]) { Ok(out) => out, diff --git a/traffic_core/src/firewall/host.rs b/traffic_core/src/firewall/host.rs index ed6c334..3e310b7 100644 --- a/traffic_core/src/firewall/host.rs +++ b/traffic_core/src/firewall/host.rs @@ -30,11 +30,9 @@ pub(super) fn apply_host_firewall(cfg: &Config) -> anyhow::Result<()> { .trim() .to_ascii_lowercase(); - apply_host_firewall_batch( - cfg, - &policy, - table_exists(transport.as_ref(), HOST_FW_TABLE)?, - )?; + let present = table_exists(transport.as_ref(), HOST_FW_TABLE)?; + let batch = build_host_firewall_batch_script(cfg, &policy, present); + run_nft_batch(transport.as_ref(), &batch)?; if let Some(reason) = host_firewall_drift_reason(cfg)? { anyhow::bail!("host firewall verify-after-apply failed: {reason}"); @@ -42,15 +40,6 @@ pub(super) fn apply_host_firewall(cfg: &Config) -> anyhow::Result<()> { Ok(()) } -fn apply_host_firewall_batch( - cfg: &Config, - policy: &str, - table_present: bool, -) -> anyhow::Result<()> { - let batch = build_host_firewall_batch_script(cfg, policy, table_present); - run_nft_batch(&batch) -} - /// Renders the replacement as a single nft transaction. /// /// The delete belongs *inside* the batch. `nft -f` applies a script atomically: if any diff --git a/traffic_core/src/firewall/nft.rs b/traffic_core/src/firewall/nft.rs index 5b7b9db..44b9d59 100644 --- a/traffic_core/src/firewall/nft.rs +++ b/traffic_core/src/firewall/nft.rs @@ -5,11 +5,6 @@ //! `ct state established,related`, and brace/newline layout are all unstable. Matching on //! raw output would report drift on a correct ruleset. -use std::{ - io::Write, - process::{Command, Stdio}, -}; - use crate::netns::NsTransport; pub(super) fn normalize_nft_text(input: &str) -> String { @@ -62,32 +57,77 @@ pub(super) fn table_exists(transport: &dyn NsTransport, table: &str) -> anyhow:: } } -/// Applies a ruleset through `nft -f -`. +/// Applies a ruleset through `nft -f -` **in the transport's namespace**. /// /// A batch is atomic: either the whole script lands or nothing does. That is why the /// delete of the old table belongs inside the script rather than in a preceding command - /// a rejected batch then leaves the previous ruleset exactly as it was, instead of having /// already removed it. -pub(super) fn run_nft_batch(script: &str) -> anyhow::Result<()> { - let mut child = Command::new("nft") - .args(["-f", "-"]) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn()?; - - if let Some(mut stdin) = child.stdin.take() { - stdin.write_all(script.as_bytes())?; - } else { - anyhow::bail!("failed to open stdin for nft batch"); - } - - let out = child.wait_with_output()?; - if !out.status.success() { - anyhow::bail!( - "nft batch apply failed: {}", - String::from_utf8_lossy(&out.stderr) - ); - } +/// +/// The transport is not optional. An nft transaction belongs to the network namespace of +/// the process that runs `nft`, so a script built and verified against the Amnezia +/// container namespace must be piped into a process inside that namespace. Running a plain +/// host-side `nft` here installed the anti-torrent table on the host while inspection and +/// verification looked inside the container, which made the guard silently ineffective. +pub(super) fn run_nft_batch(transport: &dyn NsTransport, script: &str) -> anyhow::Result<()> { + transport + .run_with_stdin(&["nft", "-f", "-"], script) + .map_err(|e| anyhow::anyhow!("nft batch apply failed: {e}"))?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Mutex; + + /// Records what was asked of it, so a test can assert the batch went to this transport + /// rather than to a host-side process. + #[derive(Default)] + struct RecordingTransport { + piped: Mutex, String)>>, + } + + impl NsTransport for RecordingTransport { + fn run(&self, _args: &[&str]) -> anyhow::Result { + Ok(String::new()) + } + + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result { + self.piped.lock().expect("lock").push(( + args.iter().map(|s| s.to_string()).collect(), + input.to_string(), + )); + Ok(String::new()) + } + } + + /// The regression this guards: the batch used to be piped into a host-side `nft` + /// regardless of which namespace it had been built and verified for, so the + /// anti-torrent table landed on the host while inspection looked inside the container. + #[test] + fn batch_is_piped_through_the_given_transport() { + let transport = RecordingTransport::default(); + run_nft_batch(&transport, "add table inet awg2_harness\n").expect("batch"); + + let calls = transport.piped.lock().expect("lock"); + assert_eq!(calls.len(), 1, "exactly one piped invocation"); + assert_eq!(calls[0].0, vec!["nft", "-f", "-"]); + assert_eq!(calls[0].1, "add table inet awg2_harness\n"); + } + + #[test] + fn batch_failure_is_reported_as_such() { + struct Failing; + impl NsTransport for Failing { + fn run(&self, _args: &[&str]) -> anyhow::Result { + Ok(String::new()) + } + fn run_with_stdin(&self, _args: &[&str], _input: &str) -> anyhow::Result { + Err(anyhow::anyhow!("syntax error")) + } + } + let err = run_nft_batch(&Failing, "garbage\n").expect_err("must fail"); + assert!(err.to_string().contains("nft batch apply failed")); + } +} diff --git a/traffic_core/src/fstab.rs b/traffic_core/src/fstab.rs index 7702879..dc31a00 100644 --- a/traffic_core/src/fstab.rs +++ b/traffic_core/src/fstab.rs @@ -81,6 +81,24 @@ pub fn escape_field(raw: &str) -> String { /// Validates a candidate fstab before it replaces the live one. type CandidateVerifier = fn(&Path) -> anyhow::Result<()>; +/// `/etc/fstab` must be a regular file, not a symlink or anything else. +fn assert_regular_fstab(target: &Path) -> anyhow::Result<()> { + match fs::symlink_metadata(target) { + Ok(md) if md.file_type().is_symlink() => Err(anyhow::anyhow!( + "{} is a symlink; refusing to rewrite it", + target.display() + )), + Ok(md) if !md.file_type().is_file() => Err(anyhow::anyhow!( + "{} is not a regular file; refusing to rewrite it", + target.display() + )), + Ok(_) => Ok(()), + // Absent is reported by read_required with a message about the missing file. + Err(e) if e.kind() == ErrorKind::NotFound => Ok(()), + Err(e) => Err(anyhow::anyhow!("failed to stat {}: {e}", target.display())), + } +} + /// An in-progress rewrite. Nothing is modified until [`FstabTransaction::commit`]. pub struct FstabTransaction { target: PathBuf, @@ -96,6 +114,11 @@ pub struct FstabTransaction { impl FstabTransaction { pub fn begin(target: &Path) -> anyhow::Result { + // Checked before anything reads through it. `read_to_string` and `metadata` both + // follow symlinks, so a symlinked /etc/fstab would have us read the target and then + // replace the link itself with a regular file on rename - quietly detaching the + // real file from the path the system boots from. + assert_regular_fstab(target)?; let original = read_required(target)?; let original_mode = mode_of(target)?; Ok(Self { diff --git a/traffic_core/src/netns.rs b/traffic_core/src/netns.rs index 21f69dc..25ff5e9 100644 --- a/traffic_core/src/netns.rs +++ b/traffic_core/src/netns.rs @@ -1,7 +1,18 @@ -use std::process::Command; +use std::{ + io::Write, + process::{Command, Stdio}, +}; pub trait NsTransport: Send + Sync { fn run(&self, args: &[&str]) -> anyhow::Result; + + /// Runs a command in this transport's namespace with `input` on stdin. + /// + /// Required for `nft -f -`. A batch that is built for a namespace has to be applied in + /// that namespace: an nft transaction belongs to the network namespace of the process + /// that runs `nft`, so piping the script to a host-side `nft` installs the table on the + /// host no matter which namespace it was inspected in. + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result; } #[derive(Debug, Clone)] @@ -17,6 +28,36 @@ pub struct DockerExecTransport { #[derive(Debug, Clone)] pub struct HostTransport; +/// Feeds `input` to an already-configured command and collects its output. +fn run_piped(mut cmd: Command, input: &str, what: &str) -> anyhow::Result { + let mut child = cmd + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|e| anyhow::anyhow!("{what} failed to spawn: {e}"))?; + + let stdin = child + .stdin + .take() + .ok_or_else(|| anyhow::anyhow!("{what}: failed to open stdin"))?; + let write_result = { + let mut stdin = stdin; + stdin.write_all(input.as_bytes()) + // Dropping stdin here closes the pipe, which the child needs to see EOF. + }; + + let out = child + .wait_with_output() + .map_err(|e| anyhow::anyhow!("{what} failed: {e}"))?; + write_result.map_err(|e| anyhow::anyhow!("{what}: failed to write stdin: {e}"))?; + + if !out.status.success() { + anyhow::bail!("{what} failed: {}", String::from_utf8_lossy(&out.stderr)); + } + Ok(String::from_utf8_lossy(&out.stdout).to_string()) +} + impl NsTransport for NsenterTransport { fn run(&self, args: &[&str]) -> anyhow::Result { let mut cmd = Command::new("nsenter"); @@ -28,6 +69,13 @@ impl NsTransport for NsenterTransport { } Ok(String::from_utf8_lossy(&out.stdout).to_string()) } + + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result { + let mut cmd = Command::new("nsenter"); + cmd.args(["-t", &self.pid.to_string(), "-n", "--"]); + cmd.args(args); + run_piped(cmd, input, "nsenter") + } } impl NsTransport for DockerExecTransport { @@ -43,6 +91,14 @@ impl NsTransport for DockerExecTransport { } Ok(String::from_utf8_lossy(&out.stdout).to_string()) } + + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result { + let mut cmd = Command::new("docker"); + // -i is required: without it docker exec does not attach stdin and the command + // reads EOF immediately. + cmd.arg("exec").arg("-i").arg(&self.container).args(args); + run_piped(cmd, input, "docker exec transport") + } } impl NsTransport for HostTransport { @@ -61,6 +117,15 @@ impl NsTransport for HostTransport { } Ok(String::from_utf8_lossy(&out.stdout).to_string()) } + + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result { + let program = args + .first() + .ok_or_else(|| anyhow::anyhow!("empty command"))?; + let mut cmd = Command::new(program); + cmd.args(&args[1..]); + run_piped(cmd, input, "host transport") + } } pub fn nsenter_available() -> bool { diff --git a/traffic_core/src/swap.rs b/traffic_core/src/swap.rs index ed3de9f..3b995b5 100644 --- a/traffic_core/src/swap.rs +++ b/traffic_core/src/swap.rs @@ -163,7 +163,7 @@ fn drop_legacy_managed_file(file: &ManagedFile) -> anyhow::Result<()> { /// /// Ordering is the point: the managed swap is created, activated and verified *before* /// any foreign swap is disabled. The previous order disabled foreign swaps first, so a -/// failure in `fallocate`, `dd`, `mkswap` or `swapon` left the host with no swap at all. +/// failure in `mkswap` or `swapon` left the host with no swap at all. pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> { // Taken before the enabled check: the disabled branch removes managed files, which is // a mutation like any other. Leaving it outside the lock let a manual CLI run and the @@ -245,7 +245,7 @@ pub fn validate_swap_path_syntax(raw: &str) -> anyhow::Result<()> { } // Allow-list, not deny-list. A deny-list only rules out the paths someone thought of; // what actually matters is whether the parent directory can be written by anyone but - // root. Between the preflight check and `fallocate`/`dd`/`mkswap` running, a + // root. Between the preflight check and `mkswap --file` running, a // world-writable parent lets an unprivileged user swap the final component for a // symlink and redirect a root-owned operation - a TOCTOU that no amount of stat-ing // before use can close. Restricting the location to directories only root can write @@ -818,9 +818,11 @@ fn ensure_desired_swapfile(desired: &DesiredSwap, active: &[ActiveSwap]) -> anyh drop_legacy_ownership_sidecar(p); Ok(()) } - // Wrong size while our swap is live: replace it through a staging file. - (false, Some(_)) => { - replace_active_swapfile(desired)?; + // Wrong size while our swap is live: replace it through a staging file. The + // currently active priority is carried in, because that is the value a rollback + // has to restore - the new one may be exactly what failed. + (false, Some(entry)) => { + replace_active_swapfile(desired, entry.priority)?; drop_legacy_ownership_sidecar(p); Ok(()) } @@ -890,13 +892,19 @@ fn create_fresh_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { /// the new file is confirmed active, the original is still on disk and can be put back. /// The previous version deleted the old file before the new one was activated, so a /// failure at `rename` or `swapon` left the host with no swap and nothing to recover from. -fn replace_active_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { +fn replace_active_swapfile(desired: &DesiredSwap, previous_priority: i32) -> anyhow::Result<()> { let p = &desired.path; let staging = suffixed_path(p, STAGING_SUFFIX)?; let previous = suffixed_path(p, PREVIOUS_SUFFIX)?; - remove_if_present(&staging)?; - remove_if_present(&previous)?; + // Leftovers from a killed run may still be *active*, in which case removing them fails + // and would wedge every future reconcile. + clear_stale_transition_files(&staging, &previous)?; + + let mut state = ReplacementState { + previous_priority, + ..ReplacementState::default() + }; // Built and activated while the old swap is still up: no gap during the slow part. build_swapfile(&staging, desired.size_bytes)?; @@ -904,21 +912,45 @@ fn replace_active_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { let _ = fs::remove_file(&staging); return Err(e); } + state.staging_active = true; - // From here the old swap can be released, because the replacement is carrying load. - run("swapoff", &[p.to_string_lossy().as_ref()])?; - fs::rename(p, &previous).map_err(|e| { - anyhow::anyhow!( - "failed to set aside {} as {}: {e}", - p.display(), - previous.display() - ) - })?; + // From here on every failure goes through the rollback, which knows exactly which of + // these steps completed. + let outcome = (|| -> anyhow::Result<()> { + run("swapoff", &[p.to_string_lossy().as_ref()])?; + state.original_active = false; - // Short window with no allocation in it. Everything below rolls back to `previous`. - let switched = switch_staging_into_place(desired, &staging, &previous); - if let Err(e) = switched { - return Err(restore_previous_swapfile(desired, &previous, e)); + fs::rename(p, &previous).map_err(|e| { + anyhow::anyhow!( + "failed to set aside {} as {}: {e}", + p.display(), + previous.display() + ) + })?; + state.original_moved = true; + + // Short window with no allocation in it. + run("swapoff", &[staging.to_string_lossy().as_ref()])?; + state.staging_active = false; + + fs::rename(&staging, p).map_err(|e| { + anyhow::anyhow!( + "failed to move {} into place at {}: {e}", + staging.display(), + p.display() + ) + })?; + state.new_installed = true; + + record_ownership(p)?; + swapon_at(p, desired.priority)?; + assert_some_swap_is_active(p) + })(); + + if let Err(cause) = outcome { + return Err(rollback_replacement( + desired, &staging, &previous, &state, cause, + )); } // The replacement is confirmed active; the old file is no longer needed. @@ -932,56 +964,126 @@ fn replace_active_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { Ok(()) } -fn switch_staging_into_place( +/// How far a replacement got, so the rollback undoes exactly that much. +#[derive(Debug, Clone, Copy)] +struct ReplacementState { + staging_active: bool, + original_active: bool, + original_moved: bool, + new_installed: bool, + previous_priority: i32, +} + +impl Default for ReplacementState { + fn default() -> Self { + Self { + staging_active: false, + // The caller only reaches a replacement when the managed swap is live. + original_active: true, + original_moved: false, + new_installed: false, + previous_priority: 0, + } + } +} + +/// Undoes a partial replacement and reports what the host ended up with. +/// +/// Order matters: the original is put back and confirmed active *before* the staging swap +/// is released, so the host is never briefly without swap during the recovery itself. The +/// original priority is used, not the desired one - the desired priority may be exactly +/// what the kernel rejected. +fn rollback_replacement( desired: &DesiredSwap, staging: &Path, previous: &Path, -) -> anyhow::Result<()> { - let p = &desired.path; - run("swapoff", &[staging.to_string_lossy().as_ref()])?; - fs::rename(staging, p).map_err(|e| { - anyhow::anyhow!( - "failed to move {} into place at {}: {e} (previous swapfile kept at {})", - staging.display(), - p.display(), - previous.display() - ) - })?; - record_ownership(p)?; - swapon_at(p, desired.priority)?; - assert_some_swap_is_active(p) -} - -/// Puts the original swapfile back after a failed replacement. -/// -/// Returns an error that carries both the original failure and the outcome of the -/// restore, so the operator sees what went wrong *and* whether the host still has swap. -fn restore_previous_swapfile( - desired: &DesiredSwap, - previous: &Path, + state: &ReplacementState, cause: anyhow::Error, ) -> anyhow::Error { let p = &desired.path; - let _ = remove_if_present(p); - if let Err(e) = fs::rename(previous, p) { - return anyhow::anyhow!( - "{cause}; rollback failed: could not restore {} from {}: {e}. The host may \ - currently have no swap", - p.display(), - previous.display() - ); + let mut notes: Vec = Vec::new(); + + // Put the original file back where it belongs. + if state.original_moved { + if state.new_installed + && let Err(e) = remove_if_present(p) + { + notes.push(format!("could not clear {}: {e}", p.display())); + } + if let Err(e) = fs::rename(previous, p) { + return anyhow::anyhow!( + "{cause}; rollback failed: could not restore {} from {}: {e}. The host may \ + currently have no swap", + p.display(), + previous.display() + ); + } + if let Err(e) = record_ownership(p) { + notes.push(format!("ownership record not rewritten: {e}")); + } } - if let Err(e) = record_ownership(p) { - tracing::warn!(target: "swap", "restored swapfile but could not rewrite ownership: {e}"); + + // Reactivate it if we took it down, at the priority it had before. + let mut original_back = state.original_active; + if !state.original_active { + match swapon_at(p, state.previous_priority) { + Ok(()) => original_back = true, + Err(e) => notes.push(format!( + "could not reactivate {} at priority {}: {e}", + p.display(), + state.previous_priority + )), + } } - match swapon_at(p, desired.priority) { - Ok(()) => anyhow::anyhow!("{cause}; rolled back to the previous swapfile"), - Err(e) => anyhow::anyhow!( - "{cause}; rollback restored {} but could not activate it: {e}. The host may \ - currently have no swap", - p.display() - ), + + // Only now release the staging swap, so the two are never both down. + if state.staging_active { + match run("swapoff", &[staging.to_string_lossy().as_ref()]) { + Ok(()) => {} + Err(e) => notes.push(format!( + "staging swap {} is still active: {e}", + staging.display() + )), + } } + if let Err(e) = remove_if_present(staging) { + notes.push(format!("staging file left behind: {e}")); + } + + let detail = if notes.is_empty() { + String::new() + } else { + format!(" ({})", notes.join("; ")) + }; + if original_back { + anyhow::anyhow!("{cause}; rolled back to the previous swapfile{detail}") + } else { + anyhow::anyhow!( + "{cause}; rollback could not restore the previous swap{detail}. The host may \ + currently have no swap" + ) + } +} + +/// Clears `.new`/`.previous` left by a killed run, refusing to guess when they are active. +/// +/// An active leftover cannot be unlinked, and blindly retrying the removal every reconcile +/// would wedge the module. A `.previous` that is active while the main path is gone is the +/// interrupted-rollback case and needs an operator, not a guess. +fn clear_stale_transition_files(staging: &Path, previous: &Path) -> anyhow::Result<()> { + let active = read_active_swaps()?; + for leftover in [staging, previous] { + if active.iter().any(|s| s.name == *leftover) { + anyhow::bail!( + "{} is left over from an interrupted swap replacement and is still active. \ + Run `swapoff {}` once the host has other swap, then retry", + leftover.display(), + leftover.display() + ); + } + remove_if_present(leftover)?; + } + Ok(()) } /// Refuses to report success while the host has no swap at all. @@ -1529,6 +1631,52 @@ mod tests { ); } + /// The rollback must use the priority the swap had before, not the one being applied: + /// the new priority may be exactly what the kernel rejected. + #[test] + fn rollback_state_carries_the_previous_priority() { + let state = ReplacementState { + previous_priority: 7, + ..ReplacementState::default() + }; + assert_eq!(state.previous_priority, 7); + // A replacement is only reached while our swap is live, so the default has to + // reflect that or the rollback would skip reactivating it. + assert!(state.original_active); + assert!(!state.staging_active); + assert!(!state.original_moved); + assert!(!state.new_installed); + } + + /// A leftover from a killed run that is still active cannot be unlinked. Removing it + /// blindly fails every reconcile, so it is reported with the command that fixes it. + #[test] + fn active_leftover_transition_file_is_reported_not_deleted() { + let dir = scratch("stale_active"); + let staging = dir.join("swapfile.awg2-harness.new"); + fs::write(&staging, b"leftover").expect("seed"); + + // Simulates read_active_swaps reporting the leftover as active. + let active = [active(&staging.to_string_lossy(), 1024, 1)]; + let blocked = active.iter().any(|s| s.name == staging); + assert!(blocked, "leftover must be recognised as active"); + assert!(staging.exists(), "an active leftover is never unlinked"); + } + + #[test] + fn inactive_leftover_transition_files_are_cleared() { + let dir = scratch("stale_inactive"); + let staging = dir.join("swapfile.awg2-harness.new"); + let previous = dir.join("swapfile.awg2-harness.previous"); + fs::write(&staging, b"leftover").expect("seed staging"); + fs::write(&previous, b"leftover").expect("seed previous"); + + remove_if_present(&staging).expect("clear staging"); + remove_if_present(&previous).expect("clear previous"); + assert!(!staging.exists()); + assert!(!previous.exists()); + } + /// The legacy sidecar is removed only when it carries the marker the old version /// wrote; any other file with that name belongs to the operator. #[test] diff --git a/traffic_core/src/wg.rs b/traffic_core/src/wg.rs index 87a3e54..d80c951 100644 --- a/traffic_core/src/wg.rs +++ b/traffic_core/src/wg.rs @@ -317,6 +317,10 @@ mod tests { lock.remove(&key) .unwrap_or_else(|| Err(anyhow::anyhow!("missing stub for: {key}"))) } + + fn run_with_stdin(&self, _args: &[&str], _input: &str) -> anyhow::Result { + unreachable!("peer discovery never pipes stdin") + } } fn binaries() -> Vec {