chown only a lock file swarmctl just created and refuse a hard-linked one

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
+  first writer's user. The lock file is opened `O_NOFOLLOW`, so a symlink
-  can be pointed at another file through it. `swarmctl` is root and hands
+  cannot point either side at another file. `swarmctl` is root: it refuses an
-  the file it creates to the directory's owner, so the bridge can open it.
+  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.

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<File>
     lock_name.push(".lock");
     let lock_path = dir.join(lock_name);
 
-    let flags = rustix::fs::OFlags::NOFOLLOW | rustix::fs::OFlags::NONBLOCK;
+    // Read-only: `flock` needs no write access, and a lock file `swarmctl`
-    let file = fs::OpenOptions::new()
+    // (root) left behind readable but not writable must still be lockable.
-        .write(true)
+    let fd = rustix::fs::open(
-        .create(true)
+        &lock_path,
-        .truncate(false)
+        rustix::fs::OFlags::RDONLY
-        .mode(0o600)
+            | rustix::fs::OFlags::CREATE
-        .custom_flags(flags.bits().cast_signed())
+            | rustix::fs::OFlags::NOFOLLOW
-        .open(&lock_path)
+            | rustix::fs::OFlags::NONBLOCK
-        .with_context(|| format!("opening {}", lock_path.display()))?;
+            | 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.

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`
+/// belongs to a less-privileged uid, so see [`open_lock_file`] for how the
-/// (a planted symlink fails the open) and `O_NONBLOCK` (a planted FIFO cannot
+/// open resists a planted link. Only a file this call created is handed to
-/// hang it), and must be a regular file. A file created here is handed to the
+/// the directory's owner (`fchown` by descriptor), so that uid can open it
-/// directory's owner by descriptor so that uid can open it too.
+/// too; an existing entry is never chowned.
 fn lock_users_file(users_file: &Path) -> Result<File> {
     lock_users_file_within(users_file, USERS_LOCK_WAIT)
 }
 
+/// Open `<users_file>.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<File> {
     let dir = users_file
         .parent()
@@ -1019,23 +1074,9 @@ fn lock_users_file_within(users_file: &Path, wait: Duration) -> Result<File> {
     lock_name.push(".lock");
     let lock_path = dir.join(lock_name);
 
-    let flags = rustix::fs::OFlags::NOFOLLOW | rustix::fs::OFlags::NONBLOCK;
+    let (file, created) = open_lock_file(&lock_path)?;
-    let file = fs::OpenOptions::new()
+    if created {
-        .write(true)
+        let dir_meta = fs::metadata(dir).with_context(|| format!("stat {}", dir.display()))?;
-        .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()))?;
     }
@@ -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]