From 6b9383929090c1002a3a5c62acb4749eeadda1be Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 17 Oct 2026 14:56:39 +0200 Subject: [PATCH] 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]