From be744d45b9583aede873771e90a007b380df9c49 Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 17 Oct 2026 14:03:57 +0200 Subject: [PATCH 1/2] swarmctl and the authelia bridge serialise users.yml writes with a shared flock --- Cargo.lock | 1 + swarm-authelia-bridge/Cargo.toml | 2 + swarm-authelia-bridge/README.md | 10 +- swarm-authelia-bridge/src/main.rs | 64 +++++++- swarm-authelia-bridge/src/store.rs | 133 ++++++++++++++++- swarmctl/README.md | 2 + swarmctl/src/main.rs | 229 +++++++++++++++++++++++++++++ 7 files changed, 436 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d40b8333..3eb1fa1f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4779,6 +4779,7 @@ dependencies = [ "axum", "hive-types", "reqwest", + "rustix", "serde", "serde_json", "serde_norway", diff --git a/swarm-authelia-bridge/Cargo.toml b/swarm-authelia-bridge/Cargo.toml index 2d257cfa..01812e22 100644 --- a/swarm-authelia-bridge/Cargo.toml +++ b/swarm-authelia-bridge/Cargo.toml @@ -13,6 +13,8 @@ hive-types.workspace = true anyhow.workspace = true axum.workspace = true reqwest.workspace = true +# `flock` on `users.yml`'s lock file, shared with `swarmctl`. +rustix.workspace = true serde.workspace = true serde_json.workspace = true serde_norway = "0.9.42" diff --git a/swarm-authelia-bridge/README.md b/swarm-authelia-bridge/README.md index ce81bfa2..5fa9cc6e 100644 --- a/swarm-authelia-bridge/README.md +++ b/swarm-authelia-bridge/README.md @@ -51,8 +51,14 @@ which is exactly what a hive with existing users hit. What still has to hold, since two processes share the file: -- **Read before write.** Both do, so neither can drop a user the other - added between operations. +- **One lock around read → write.** Both take an exclusive `flock` on + `users.yml.lock`, a sibling file in the same directory, before loading and + hold it until the rename lands, waiting at most 10 seconds. Reading the + file first is not enough: both writers spend about a second minting a + password between load and write, and the second rename would drop the + first writer's user. The lock file is opened `O_NOFOLLOW`, so neither side + can be pointed at another file through it. `swarmctl` is root and hands + the file it creates to the directory's owner, so the bridge can open it. - **Unknown keys survive.** Both carry unmodelled top-level and per-user fields through a round-trip, or whichever writes second would silently delete what the first added. diff --git a/swarm-authelia-bridge/src/main.rs b/swarm-authelia-bridge/src/main.rs index 0068fb7a..e07252a6 100644 --- a/swarm-authelia-bridge/src/main.rs +++ b/swarm-authelia-bridge/src/main.rs @@ -98,7 +98,8 @@ struct AppState { /// whichever `publish`es second silently drops the first agent's entry — /// the store is a plain file, not a database with its own concurrency /// control. `tokio::sync::Mutex`, not `std`'s: held across the `.await`s - /// in `generate_password` and `publish`. + /// in `generate_password` and `publish`. It covers this process only; + /// `store::lock_users_file` is the lock against `swarmctl`. write_lock: tokio::sync::Mutex<()>, } @@ -316,6 +317,10 @@ async fn handle( // doc comment on `AppState::write_lock` for why this is reachable in // practice, not just theoretically. let _write_guard = write_lock.lock().await; + // The in-process lock does not cover `swarmctl`, which rewrites the same + // file from the host: the `flock` does, and is held to the end of the + // function like the guard above. + let _file_lock = store::lock_users_file(&cfg.users_file, store::USERS_LOCK_WAIT).await?; let mut user_store = store::load_store(&cfg.users_file)?; if user_store.users.contains_key(&name) { @@ -443,7 +448,7 @@ async fn generate_password(bin: &std::path::Path) -> Result { #[cfg(test)] mod tests { use super::{ - BridgeRequest, Config, Refusal, StatusCode, classify, introspect::Verdict, respond, + BridgeRequest, Config, Refusal, StatusCode, classify, handle, introspect::Verdict, respond, }; const CONTROLLER: &str = "swarm-controller"; @@ -586,4 +591,59 @@ mod tests { ); let _ = std::fs::remove_dir_all(config.users_file.parent().unwrap()); } + + /// `handle` waits for the `flock` `swarmctl` takes, not only for the + /// in-process lock: it is held here while `handle` runs with a lock of + /// its own that nothing contends. + #[tokio::test] + async fn an_identity_request_waits_for_the_users_file_lock() { + use std::os::unix::fs::PermissionsExt as _; + use std::time::Duration; + + let mut config = config_with_one_operator(); + let dir = config.users_file.parent().unwrap().to_owned(); + let authelia = dir.join("authelia"); + std::fs::write( + &authelia, + "#!/bin/sh\nprintf 'Random Password: pw\\nDigest: $argon2id$x\\n'\n", + ) + .unwrap(); + std::fs::set_permissions(&authelia, std::fs::Permissions::from_mode(0o755)).unwrap(); + config.authelia_bin = authelia; + + let held = crate::store::lock_users_file(&config.users_file, Duration::from_secs(1)) + .await + .unwrap(); + let write_lock = std::sync::Arc::new(tokio::sync::Mutex::new(())); + let request = { + let config = std::sync::Arc::new(config); + let write_lock = std::sync::Arc::clone(&write_lock); + let users_file = config.users_file.clone(); + ( + tokio::spawn(async move { + handle(&config, &write_lock, "atlas".to_owned()) + .await + .map(|_| ()) + }), + users_file, + ) + }; + let (mut request, users_file) = request; + + assert!( + tokio::time::timeout(Duration::from_millis(300), &mut request) + .await + .is_err(), + "the request finished while another process held the lock" + ); + drop(held); + request.await.unwrap().expect("request after release"); + assert!( + crate::store::load_store(&users_file) + .unwrap() + .users + .contains_key("atlas") + ); + let _ = std::fs::remove_dir_all(users_file.parent().unwrap()); + } } diff --git a/swarm-authelia-bridge/src/store.rs b/swarm-authelia-bridge/src/store.rs index 88c71ee1..d38dbe29 100644 --- a/swarm-authelia-bridge/src/store.rs +++ b/swarm-authelia-bridge/src/store.rs @@ -19,8 +19,9 @@ use std::collections::BTreeMap; use std::fs::{self, File, Permissions}; use std::io::Write as _; -use std::os::unix::fs::PermissionsExt as _; +use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; use std::path::Path; +use std::time::Duration; use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; @@ -277,6 +278,72 @@ fn fill_missing_emails(store: &mut UserStore) { } } +/// How long a writer waits for the other process's `users.yml` update. +pub const USERS_LOCK_WAIT: Duration = Duration::from_secs(10); + +/// Take the exclusive `flock` on `.lock`, held until the +/// returned file drops. +/// +/// `swarmctl` rewrites the same file from the host and takes the same lock on +/// the same inode. `handle` holds it across load → modify → [`publish`]: +/// the in-process `write_lock` does not cover that other process, and the +/// rename alone would not stop two writers loading the same snapshot. +/// +/// The lock file is a sibling of `users_file`, not `users_file` itself, +/// because `write_atomic` replaces that inode on every write. It is opened +/// `O_NOFOLLOW` and `O_NONBLOCK` (a planted symlink or FIFO cannot redirect or +/// hang it) and must be a regular file; no ownership is touched by path. +pub async fn lock_users_file(users_file: &Path, wait: Duration) -> Result { + let dir = users_file + .parent() + .with_context(|| format!("{} has no parent directory", users_file.display()))?; + fs::create_dir_all(dir).with_context(|| format!("creating {}", dir.display()))?; + let name = users_file + .file_name() + .with_context(|| format!("{} has no file name", users_file.display()))?; + let mut lock_name = name.to_os_string(); + lock_name.push(".lock"); + let lock_path = dir.join(lock_name); + + let flags = rustix::fs::OFlags::NOFOLLOW | rustix::fs::OFlags::NONBLOCK; + let file = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .mode(0o600) + .custom_flags(flags.bits().cast_signed()) + .open(&lock_path) + .with_context(|| format!("opening {}", lock_path.display()))?; + if !file + .metadata() + .with_context(|| format!("stat {}", lock_path.display()))? + .is_file() + { + bail!("{} is not a regular file", lock_path.display()); + } + + let deadline = tokio::time::Instant::now() + wait; + loop { + match rustix::fs::flock(&file, rustix::fs::FlockOperation::NonBlockingLockExclusive) { + Ok(()) => return Ok(file), + Err(e) + if e == rustix::io::Errno::WOULDBLOCK && tokio::time::Instant::now() < deadline => + { + tokio::time::sleep(Duration::from_millis(50)).await; + } + Err(e) if e == rustix::io::Errno::WOULDBLOCK => bail!( + "timed out after {wait:?} waiting for {}: another process (`swarmctl user`) \ + is updating {}", + lock_path.display(), + users_file.display() + ), + Err(e) => { + return Err(e).with_context(|| format!("locking {}", lock_path.display())); + } + } + } +} + /// Replace `path`'s contents atomically. Unlike `swarmctl::write_atomic`, /// this does **not** need to preserve a foreign owner via `chown` — this /// process runs as `users_file`'s own owning user (see the module doc: @@ -637,6 +704,70 @@ some_future_top_level_key: 7 fs::remove_dir_all(&dir).ok(); } + #[tokio::test] + async fn the_users_lock_excludes_a_second_holder_until_it_is_released() { + let dir = tempdir(); + let users_file = dir.join("users.yml"); + let held = lock_users_file(&users_file, Duration::from_secs(1)) + .await + .expect("first lock"); + + let err = lock_users_file(&users_file, Duration::from_millis(200)) + .await + .expect_err("a held lock must not be granted twice"); + assert!(err.to_string().contains("timed out"), "{err}"); + + drop(held); + lock_users_file(&users_file, Duration::from_millis(200)) + .await + .expect("a released lock is granted"); + + fs::remove_dir_all(&dir).ok(); + } + + #[tokio::test] + async fn the_users_lock_refuses_a_symlink_planted_at_the_lock_path() { + let dir = tempdir(); + let users_file = dir.join("users.yml"); + let target = dir.join("victim"); + fs::write(&target, "keep").unwrap(); + std::os::unix::fs::symlink(&target, dir.join("users.yml.lock")).unwrap(); + + lock_users_file(&users_file, Duration::from_secs(1)) + .await + .expect_err("a symlinked lock path must be refused"); + assert_eq!(fs::read_to_string(&target).unwrap(), "keep"); + + fs::remove_dir_all(&dir).ok(); + } + + /// Two read-modify-writes that each sit between load and publish long + /// enough for the other to load: without the lock the second `publish` + /// drops the first user. + #[tokio::test] + async fn interleaved_read_modify_writes_both_land() { + let dir = tempdir(); + let users_file = dir.join("users.yml"); + let add = |name: &'static str| { + let users_file = users_file.clone(); + tokio::spawn(async move { + let _lock = lock_users_file(&users_file, Duration::from_secs(5)).await?; + let mut store = load_store(&users_file)?; + tokio::time::sleep(Duration::from_millis(300)).await; + store.users.insert(name.to_owned(), user("$argon2id$x")); + publish(&users_file, &mut store) + }) + }; + let (a, b) = (add("alice"), add("bob")); + a.await.unwrap().expect("alice"); + b.await.unwrap().expect("bob"); + + let names: Vec<_> = load_store(&users_file).unwrap().users.into_keys().collect(); + assert_eq!(names, ["alice", "bob"]); + + fs::remove_dir_all(&dir).ok(); + } + /// Test-only temp dir under the OS temp root — disposable scratch for /// one test's lifetime, not durable state (see `state-not-tmp`: this /// is exactly the legitimate use, not a place we persist anything). diff --git a/swarmctl/README.md b/swarmctl/README.md index 566ab2ac..4497148f 100644 --- a/swarmctl/README.md +++ b/swarmctl/README.md @@ -31,6 +31,8 @@ controller can queue (see below). Nothing here becomes reachable by that. `swarmctl` reads and writes `users.yml` — authelia's own users database — directly. `swarm-authelia-bridge` writes agent subjects into the same file. +Every verb that writes holds an exclusive `flock` on `users.yml.lock` from +before the load until the rename, as the bridge does. ⚠️ The file is round-tripped, so **comments and hand-formatting do not survive a write**. Values do, and so do keys this binary does not model. diff --git a/swarmctl/src/main.rs b/swarmctl/src/main.rs index 2d46a83d..0b3eeecd 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -39,6 +39,7 @@ use std::io::{self, Write as _}; use std::os::unix::fs::{MetadataExt as _, OpenOptionsExt as _, PermissionsExt as _}; use std::path::{Path, PathBuf}; use std::process::Command; +use std::time::{Duration, Instant}; use anyhow::{Context, Result, bail}; use clap::{Args, Parser, Subcommand}; @@ -758,6 +759,7 @@ fn schedule(remote: &RemoteArgs, command: ScheduleVerb) -> Result<()> { fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { users::validate_username(&args.username)?; + let _lock = lock_users_file(&paths.users_file)?; let mut store = load_store(&paths.users_file)?; if store.users.contains_key(&args.username) { bail!( @@ -789,6 +791,7 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { } fn user_reset_password(paths: &Paths, args: &ResetPasswordArgs) -> Result<()> { + let _lock = lock_users_file(&paths.users_file)?; let mut store = load_store(&paths.users_file)?; let Some(user) = store.users.get_mut(&args.username) else { bail!( @@ -814,6 +817,7 @@ fn user_reset_password(paths: &Paths, args: &ResetPasswordArgs) -> Result<()> { } fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { + let _lock = lock_users_file(&paths.users_file)?; let mut store = load_store(&paths.users_file)?; let Some(user) = store.users.get_mut(&args.username) else { bail!( @@ -981,6 +985,81 @@ fn parse_field(stdout: &str, marker: &str) -> Option { .filter(|value| !value.is_empty()) } +/// How long a writer waits for the other process's `users.yml` update. +const USERS_LOCK_WAIT: Duration = Duration::from_secs(10); + +/// Take the exclusive `flock` on `.lock`, held until the +/// returned file drops. +/// +/// `swarm-authelia-bridge` rewrites the same file from inside the +/// `swarm-authelia` container and takes the same lock on the same inode. +/// Every verb holds it across load → modify → [`publish`]: the rename alone +/// would not stop two writers loading the same snapshot, and the second +/// rename would drop the first one's user. +/// +/// The lock file is a sibling of `users_file`, not `users_file` itself, +/// because `write_atomic` replaces that inode on every write. The directory +/// belongs to a less-privileged uid, so the lock file is opened `O_NOFOLLOW` +/// (a planted symlink fails the open) and `O_NONBLOCK` (a planted FIFO cannot +/// hang it), and must be a regular file. A file created here is handed to the +/// directory's owner by descriptor so that uid can open it too. +fn lock_users_file(users_file: &Path) -> Result { + lock_users_file_within(users_file, USERS_LOCK_WAIT) +} + +fn lock_users_file_within(users_file: &Path, wait: Duration) -> Result { + let dir = users_file + .parent() + .with_context(|| format!("{} has no parent directory", users_file.display()))?; + fs::create_dir_all(dir).with_context(|| format!("creating {}", dir.display()))?; + let name = users_file + .file_name() + .with_context(|| format!("{} has no file name", users_file.display()))?; + let mut lock_name = name.to_os_string(); + lock_name.push(".lock"); + let lock_path = dir.join(lock_name); + + let flags = rustix::fs::OFlags::NOFOLLOW | rustix::fs::OFlags::NONBLOCK; + let file = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .mode(0o600) + .custom_flags(flags.bits().cast_signed()) + .open(&lock_path) + .with_context(|| format!("opening {}", lock_path.display()))?; + let meta = file + .metadata() + .with_context(|| format!("stat {}", lock_path.display()))?; + if !meta.is_file() { + bail!("{} is not a regular file", lock_path.display()); + } + let dir_meta = fs::metadata(dir).with_context(|| format!("stat {}", dir.display()))?; + if (meta.uid(), meta.gid()) != (dir_meta.uid(), dir_meta.gid()) { + std::os::unix::fs::fchown(&file, Some(dir_meta.uid()), Some(dir_meta.gid())) + .with_context(|| format!("setting owner on {}", lock_path.display()))?; + } + + let deadline = Instant::now() + wait; + loop { + match rustix::fs::flock(&file, rustix::fs::FlockOperation::NonBlockingLockExclusive) { + Ok(()) => return Ok(file), + Err(e) if e == rustix::io::Errno::WOULDBLOCK && Instant::now() < deadline => { + std::thread::sleep(Duration::from_millis(50)); + } + Err(e) if e == rustix::io::Errno::WOULDBLOCK => bail!( + "timed out after {wait:?} waiting for {}: another process (the \ + authelia bridge or another `swarmctl user` verb) is updating {}", + lock_path.display(), + users_file.display() + ), + Err(e) => { + return Err(e).with_context(|| format!("locking {}", lock_path.display())); + } + } + } +} + /// Replace `path`'s contents atomically, preserving the existing owner /// and mode. /// @@ -1388,6 +1467,156 @@ mod tests { fs::remove_dir_all(&dir).ok(); } + /// A scratch directory holding a `users.yml` with one user, `carol`, and + /// a stand-in `authelia` that takes `delay` seconds to mint a password. + fn locked_users_fixture(tag: &str, delay: &str) -> Paths { + let dir = std::env::temp_dir().join(format!("swarmctl-lock-{tag}-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).expect("temp dir"); + let authelia_bin = dir.join("authelia"); + fs::write( + &authelia_bin, + format!( + "#!/bin/sh\nsleep {delay}\nprintf 'Random Password: pw\\nDigest: $argon2id$x\\n'\n" + ), + ) + .expect("fake authelia"); + fs::set_permissions(&authelia_bin, Permissions::from_mode(0o755)).expect("chmod"); + let users_file = dir.join("users.yml"); + fs::write( + &users_file, + "users:\n carol:\n displayname: carol\n password: x\n groups: []\n", + ) + .expect("seed"); + Paths { + authelia_bin, + users_file, + } + } + + fn add_args(username: &str) -> AddArgs { + AddArgs { + username: username.to_owned(), + display_name: None, + email: None, + groups: Vec::new(), + } + } + + #[test] + fn the_users_lock_excludes_a_second_holder_until_it_is_released() { + let paths = locked_users_fixture("exclusive", "0"); + let held = lock_users_file(&paths.users_file).expect("first lock"); + + let err = lock_users_file_within(&paths.users_file, Duration::from_millis(200)) + .expect_err("a held lock must not be granted twice"); + assert!(err.to_string().contains("timed out"), "{err}"); + + drop(held); + lock_users_file_within(&paths.users_file, Duration::from_millis(200)) + .expect("a released lock is granted"); + + fs::remove_dir_all(paths.users_file.parent().unwrap()).ok(); + } + + #[test] + fn the_users_lock_refuses_a_symlink_planted_at_the_lock_path() { + let paths = locked_users_fixture("symlink", "0"); + let dir = paths.users_file.parent().unwrap(); + let target = dir.join("victim"); + fs::write(&target, "keep").expect("victim"); + std::os::unix::fs::symlink(&target, dir.join("users.yml.lock")).expect("plant"); + + lock_users_file(&paths.users_file).expect_err("a symlinked lock path must be refused"); + assert_eq!(fs::read_to_string(&target).expect("read"), "keep"); + + fs::remove_dir_all(dir).ok(); + } + + /// Both adds load the file before either publishes (each spends 0.5s in + /// authelia), so without the lock the second rename drops the first user. + #[test] + fn two_concurrent_user_adds_both_land() { + let paths = std::sync::Arc::new(locked_users_fixture("adds", "0.5")); + let handles: Vec<_> = ["alice", "bob"] + .into_iter() + .map(|name| { + let paths = std::sync::Arc::clone(&paths); + std::thread::spawn(move || user_add(&paths, add_args(name))) + }) + .collect(); + for handle in handles { + handle.join().expect("thread").expect("user add"); + } + + let store = load_store(&paths.users_file).expect("reload"); + let names: Vec<_> = store.users.keys().map(String::as_str).collect(); + assert_eq!(names, ["alice", "bob", "carol"]); + + fs::remove_dir_all(paths.users_file.parent().unwrap()).ok(); + } + + #[test] + fn every_user_verb_waits_for_the_users_lock() { + let paths = std::sync::Arc::new(locked_users_fixture("verbs", "0")); + let held = lock_users_file(&paths.users_file).expect("lock"); + + let (add_tx, add_rx) = std::sync::mpsc::channel(); + let (reset_tx, reset_rx) = std::sync::mpsc::channel(); + let (update_tx, update_rx) = std::sync::mpsc::channel(); + let p = std::sync::Arc::clone(&paths); + std::thread::spawn(move || add_tx.send(user_add(&p, add_args("dave")))); + let p = std::sync::Arc::clone(&paths); + std::thread::spawn(move || { + let args = ResetPasswordArgs { + username: "carol".to_owned(), + }; + reset_tx.send(user_reset_password(&p, &args)) + }); + let p = std::sync::Arc::clone(&paths); + std::thread::spawn(move || { + let args = UpdateArgs { + username: "carol".to_owned(), + display_name: None, + email: None, + add_groups: vec!["ops".to_owned()], + remove_groups: Vec::new(), + }; + update_tx.send(user_update(&p, args)) + }); + + let wait = Duration::from_millis(300); + assert!(add_rx.recv_timeout(wait).is_err(), "user add ran unlocked"); + assert!( + reset_rx.recv_timeout(wait).is_err(), + "user reset-password ran unlocked" + ); + assert!( + update_rx.recv_timeout(wait).is_err(), + "user update ran unlocked" + ); + + drop(held); + let done = Duration::from_secs(5); + add_rx + .recv_timeout(done) + .expect("add finishes") + .expect("add"); + reset_rx + .recv_timeout(done) + .expect("reset finishes") + .expect("reset"); + update_rx + .recv_timeout(done) + .expect("update finishes") + .expect("update"); + let store = load_store(&paths.users_file).expect("reload"); + assert!(store.users.contains_key("dave")); + assert_eq!(store.users["carol"].groups, ["ops"]); + + fs::remove_dir_all(paths.users_file.parent().unwrap()).ok(); + } + #[test] fn parses_authelia_hash_output() { let out = "Random Password: hunter2\nDigest: $argon2id$v=19$m=65536$abc\n"; From 6b9383929090c1002a3a5c62acb4749eeadda1be Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 17 Oct 2026 14:56:39 +0200 Subject: [PATCH 2/2] chown only a lock file swarmctl just created and refuse a hard-linked one --- swarm-authelia-bridge/README.md | 9 +- swarm-authelia-bridge/src/store.rs | 41 ++++++--- swarmctl/src/main.rs | 133 ++++++++++++++++++++++++----- 3 files changed, 149 insertions(+), 34 deletions(-) diff --git a/swarm-authelia-bridge/README.md b/swarm-authelia-bridge/README.md index 5fa9cc6e..8a189756 100644 --- a/swarm-authelia-bridge/README.md +++ b/swarm-authelia-bridge/README.md @@ -56,9 +56,12 @@ What still has to hold, since two processes share the file: hold it until the rename lands, waiting at most 10 seconds. Reading the file first is not enough: both writers spend about a second minting a password between load and write, and the second rename would drop the - first writer's user. The lock file is opened `O_NOFOLLOW`, so neither side - can be pointed at another file through it. `swarmctl` is root and hands - the file it creates to the directory's owner, so the bridge can open it. + first writer's user. The lock file is opened `O_NOFOLLOW`, so a symlink + cannot point either side at another file. `swarmctl` is root: it refuses an + existing lock file with more than one hard link, and it changes the owner + of a lock file only when it created that file in the same call, handing it + to the directory's owner so the bridge can open it. The bridge opens the + lock read-only, which is all `flock` needs. - **Unknown keys survive.** Both carry unmodelled top-level and per-user fields through a round-trip, or whichever writes second would silently delete what the first added. diff --git a/swarm-authelia-bridge/src/store.rs b/swarm-authelia-bridge/src/store.rs index d38dbe29..474eb99f 100644 --- a/swarm-authelia-bridge/src/store.rs +++ b/swarm-authelia-bridge/src/store.rs @@ -19,7 +19,7 @@ use std::collections::BTreeMap; use std::fs::{self, File, Permissions}; use std::io::Write as _; -use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; +use std::os::unix::fs::PermissionsExt as _; use std::path::Path; use std::time::Duration; @@ -305,15 +305,19 @@ pub async fn lock_users_file(users_file: &Path, wait: Duration) -> Result lock_name.push(".lock"); let lock_path = dir.join(lock_name); - let flags = rustix::fs::OFlags::NOFOLLOW | rustix::fs::OFlags::NONBLOCK; - let file = fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(false) - .mode(0o600) - .custom_flags(flags.bits().cast_signed()) - .open(&lock_path) - .with_context(|| format!("opening {}", lock_path.display()))?; + // Read-only: `flock` needs no write access, and a lock file `swarmctl` + // (root) left behind readable but not writable must still be lockable. + let fd = rustix::fs::open( + &lock_path, + rustix::fs::OFlags::RDONLY + | rustix::fs::OFlags::CREATE + | rustix::fs::OFlags::NOFOLLOW + | rustix::fs::OFlags::NONBLOCK + | rustix::fs::OFlags::CLOEXEC, + rustix::fs::Mode::RUSR | rustix::fs::Mode::WUSR, + ) + .with_context(|| format!("opening {}", lock_path.display()))?; + let file = File::from(fd); if !file .metadata() .with_context(|| format!("stat {}", lock_path.display()))? @@ -741,6 +745,23 @@ some_future_top_level_key: 7 fs::remove_dir_all(&dir).ok(); } + /// A lock file left behind without write access (root-owned after a crash + /// in `swarmctl`) can still be locked: `flock` needs only a read fd. + #[tokio::test] + async fn a_read_only_lock_file_can_still_be_locked() { + let dir = tempdir(); + let users_file = dir.join("users.yml"); + let lock_path = dir.join("users.yml.lock"); + fs::write(&lock_path, "").unwrap(); + fs::set_permissions(&lock_path, Permissions::from_mode(0o444)).unwrap(); + + lock_users_file(&users_file, Duration::from_secs(1)) + .await + .expect("lock a read-only lock file"); + + fs::remove_dir_all(&dir).ok(); + } + /// Two read-modify-writes that each sit between load and publish long /// enough for the other to load: without the lock the second `publish` /// drops the first user. diff --git a/swarmctl/src/main.rs b/swarmctl/src/main.rs index 0b3eeecd..a4f475c3 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -999,14 +999,69 @@ const USERS_LOCK_WAIT: Duration = Duration::from_secs(10); /// /// The lock file is a sibling of `users_file`, not `users_file` itself, /// because `write_atomic` replaces that inode on every write. The directory -/// belongs to a less-privileged uid, so the lock file is opened `O_NOFOLLOW` -/// (a planted symlink fails the open) and `O_NONBLOCK` (a planted FIFO cannot -/// hang it), and must be a regular file. A file created here is handed to the -/// directory's owner by descriptor so that uid can open it too. +/// belongs to a less-privileged uid, so see [`open_lock_file`] for how the +/// open resists a planted link. Only a file this call created is handed to +/// the directory's owner (`fchown` by descriptor), so that uid can open it +/// too; an existing entry is never chowned. fn lock_users_file(users_file: &Path) -> Result { lock_users_file_within(users_file, USERS_LOCK_WAIT) } +/// Open `.lock`, reporting whether this call created it. +/// +/// `O_EXCL` first, so a file that is ours is told apart from one that was +/// already there. An existing entry is reopened read-only: `flock` needs no +/// write access, and a lock file left behind owned by root (a crash between +/// create and `fchown`) stays openable by the bridge. An existing entry must +/// be a regular file with one link; a hard link planted by the directory's +/// owner to some other file is refused. +fn open_lock_file(lock_path: &Path) -> Result<(File, bool)> { + let nofollow = (rustix::fs::OFlags::NOFOLLOW | rustix::fs::OFlags::NONBLOCK) + .bits() + .cast_signed(); + for _ in 0..2 { + // Mode 0644 so the lock stays readable by the bridge if we die before + // `fchown`; the directory is 0700, so nobody else reaches it. + match fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o644) + .custom_flags(nofollow) + .open(lock_path) + { + Ok(file) => return Ok((file, true)), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {} + Err(e) => return Err(e).with_context(|| format!("creating {}", lock_path.display())), + } + match fs::OpenOptions::new() + .read(true) + .custom_flags(nofollow) + .open(lock_path) + { + Ok(file) => { + let meta = file + .metadata() + .with_context(|| format!("stat {}", lock_path.display()))?; + if !meta.is_file() { + bail!("{} is not a regular file", lock_path.display()); + } + if meta.nlink() != 1 { + bail!( + "{} has {} hard links; refusing to lock a file that is not ours", + lock_path.display(), + meta.nlink() + ); + } + return Ok((file, false)); + } + // Removed between the two opens: create it again. + Err(e) if e.kind() == io::ErrorKind::NotFound => {} + Err(e) => return Err(e).with_context(|| format!("opening {}", lock_path.display())), + } + } + bail!("{} keeps changing under us", lock_path.display()) +} + fn lock_users_file_within(users_file: &Path, wait: Duration) -> Result { let dir = users_file .parent() @@ -1019,23 +1074,9 @@ fn lock_users_file_within(users_file: &Path, wait: Duration) -> Result { lock_name.push(".lock"); let lock_path = dir.join(lock_name); - let flags = rustix::fs::OFlags::NOFOLLOW | rustix::fs::OFlags::NONBLOCK; - let file = fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(false) - .mode(0o600) - .custom_flags(flags.bits().cast_signed()) - .open(&lock_path) - .with_context(|| format!("opening {}", lock_path.display()))?; - let meta = file - .metadata() - .with_context(|| format!("stat {}", lock_path.display()))?; - if !meta.is_file() { - bail!("{} is not a regular file", lock_path.display()); - } - let dir_meta = fs::metadata(dir).with_context(|| format!("stat {}", dir.display()))?; - if (meta.uid(), meta.gid()) != (dir_meta.uid(), dir_meta.gid()) { + let (file, created) = open_lock_file(&lock_path)?; + if created { + let dir_meta = fs::metadata(dir).with_context(|| format!("stat {}", dir.display()))?; std::os::unix::fs::fchown(&file, Some(dir_meta.uid()), Some(dir_meta.gid())) .with_context(|| format!("setting owner on {}", lock_path.display()))?; } @@ -1533,6 +1574,56 @@ mod tests { fs::remove_dir_all(dir).ok(); } + #[test] + fn an_existing_lock_file_is_opened_not_created_or_chowned() { + let paths = locked_users_fixture("existing", "0"); + let lock_path = paths.users_file.with_file_name("users.yml.lock"); + + let (_, created) = open_lock_file(&lock_path).expect("first open"); + assert!(created, "a missing lock file is created by this call"); + let (_, created) = open_lock_file(&lock_path).expect("second open"); + assert!( + !created, + "an existing lock file must be reported as not ours" + ); + + fs::remove_dir_all(paths.users_file.parent().unwrap()).ok(); + } + + /// The directory's owner can plant a hard link; a root-side `fchown` on it + /// would re-own the link's target. + #[test] + fn the_users_lock_refuses_a_hard_link_planted_at_the_lock_path() { + let paths = locked_users_fixture("hardlink", "0"); + let dir = paths.users_file.parent().unwrap(); + let target = dir.join("victim"); + fs::write(&target, "keep").expect("victim"); + fs::hard_link(&target, dir.join("users.yml.lock")).expect("plant"); + let before = fs::metadata(&target).expect("stat"); + + let err = lock_users_file(&paths.users_file).expect_err("a linked lock path is refused"); + assert!(err.to_string().contains("hard links"), "{err}"); + let after = fs::metadata(&target).expect("stat"); + assert_eq!((after.uid(), after.gid()), (before.uid(), before.gid())); + assert_eq!(fs::read_to_string(&target).expect("read"), "keep"); + + fs::remove_dir_all(dir).ok(); + } + + /// A lock file left behind without write access (root-owned after a crash + /// before `fchown`) can still be locked: `flock` needs only a read fd. + #[test] + fn a_read_only_lock_file_can_still_be_locked() { + let paths = locked_users_fixture("readonly", "0"); + let lock_path = paths.users_file.with_file_name("users.yml.lock"); + fs::write(&lock_path, "").expect("seed"); + fs::set_permissions(&lock_path, Permissions::from_mode(0o444)).expect("chmod"); + + lock_users_file(&paths.users_file).expect("lock a read-only lock file"); + + fs::remove_dir_all(paths.users_file.parent().unwrap()).ok(); + } + /// Both adds load the file before either publishes (each spends 0.5s in /// authelia), so without the lock the second rename drops the first user. #[test]