From 63fc54edc52f758965f71b79be4e859046fc3bbe Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 17 Aug 2026 19:22:27 +0200 Subject: [PATCH] refactor(#3393): stop restarting authelia from swarmctl authelia now watches the users file, so the restart is redundant -- and it was the wrong shape twice over. It could fail: a login was refused for a user whose record was already correct on disk, with nothing in either log implicating the reload. And it only ever worked for this writer -- swarm-authelia-bridge writes the same file and cannot restart anything, since running unprivileged inside the container is the whole reason it may write it at all. A reload that depends on which process did the writing is not a reload. --machine/--unit and their two env vars existed solely to name a systemctl -M target, so they go with it. That drops two required settings from the operator surface. The three objections previously recorded against watch are all answered now, and are kept next to the decision rather than deleted: the key is verified against the pinned build (validate-config accepts it and rejects a misspelling), the watch is on the directory so a rename is observed, and partial reads are structurally impossible because every writer of this file goes through write_atomic. --- docs/tools/swarmctl-cli.md | 2 - nix/host-modules/swarm-controller.nix | 5 +- swarmctl/src/main.rs | 67 +++++++++++---------------- swarmctl/src/users.rs | 2 +- 4 files changed, 32 insertions(+), 44 deletions(-) diff --git a/docs/tools/swarmctl-cli.md b/docs/tools/swarmctl-cli.md index d38368fc..68acc6ce 100644 --- a/docs/tools/swarmctl-cli.md +++ b/docs/tools/swarmctl-cli.md @@ -26,8 +26,6 @@ swarm-level operator CLI * `--authelia-bin ` — authelia binary used to hash passwords. The argon2 parameters must match the verifier's, so this has to be the *configured* package rather than whatever is on `PATH` * `--users-file ` — Host-side path of authelia's users database — i.e. the path inside the container, prefixed with the container's root -* `--machine ` — Machine name of the authelia container, for `systemctl -M` -* `--unit ` — authelia's systemd unit inside that container * `--store ` — Canonical user store diff --git a/nix/host-modules/swarm-controller.nix b/nix/host-modules/swarm-controller.nix index 3e28e43a..06c42b5f 100644 --- a/nix/host-modules/swarm-controller.nix +++ b/nix/host-modules/swarm-controller.nix @@ -29,8 +29,9 @@ let # parameters baked into a hash have to match the verifier's. SWARMCTL_AUTHELIA_BIN = "${autheliaCfg.package}/bin/authelia"; SWARMCTL_AUTHELIA_USERS_FILE = autheliaCfg.hostUsersFile; - SWARMCTL_AUTHELIA_MACHINE = autheliaCfg.machine; - SWARMCTL_AUTHELIA_UNIT = autheliaCfg.unit; + # No MACHINE/UNIT here any more: `swarmctl` no longer restarts authelia, + # because authelia watches the users file itself. Those two values existed + # solely to name a `systemctl -M` target. }; natsCfg = config.services.hyperhive.swarm.nats; diff --git a/swarmctl/src/main.rs b/swarmctl/src/main.rs index b3ff4dc2..fd0b88b3 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -69,12 +69,6 @@ struct PathArgs { /// the container, prefixed with the container's root. #[arg(long, value_name = "PATH")] users_file: Option, - /// Machine name of the authelia container, for `systemctl -M`. - #[arg(long, value_name = "NAME")] - machine: Option, - /// authelia's systemd unit inside that container. - #[arg(long, value_name = "UNIT")] - unit: Option, /// Canonical user store. #[arg(long, value_name = "PATH")] store: Option, @@ -83,8 +77,6 @@ struct PathArgs { struct Paths { authelia_bin: PathBuf, users_file: PathBuf, - machine: String, - unit: String, store: PathBuf, } @@ -93,8 +85,6 @@ impl PathArgs { Ok(Paths { authelia_bin: path_from(self.authelia_bin, "SWARMCTL_AUTHELIA_BIN")?, users_file: path_from(self.users_file, "SWARMCTL_AUTHELIA_USERS_FILE")?, - machine: string_from(self.machine, "SWARMCTL_AUTHELIA_MACHINE")?, - unit: string_from(self.unit, "SWARMCTL_AUTHELIA_UNIT")?, store: self .store .or_else(|| std::env::var_os("SWARMCTL_STORE").map(PathBuf::from)) @@ -108,11 +98,6 @@ fn path_from(flag: Option, env: &str) -> Result { .with_context(|| missing(env)) } -fn string_from(flag: Option, env: &str) -> Result { - flag.or_else(|| std::env::var(env).ok()) - .with_context(|| missing(env)) -} - fn missing(env: &str) -> String { format!( "{env} is unset and no flag was given — swarmctl is installed and \ @@ -358,7 +343,34 @@ fn publish(paths: &Paths, store: &UserStore) -> Result<()> { write_atomic(&paths.store, &format!("{store_json}\n"))?; write_atomic(&paths.users_file, &rendered)?; - restart_authelia(&paths.machine, &paths.unit) + // No restart. Authelia is configured with + // `authentication_backend.file.watch`, so it re-reads this file itself. + // + // This used to shell out to `systemctl -M restart `. That + // was the wrong shape twice over: it could fail (it did — a login refused + // for a user whose record was already correct on disk, with nothing in + // either log implicating the reload), and it only ever worked for THIS + // writer. `swarm-authelia-bridge` writes the same file to create agent + // identities and cannot restart anything: running unprivileged inside the + // container is the whole reason it may write the file at all. A reload + // that depends on which process did the writing is not a reload. + // + // The three objections that previously kept `watch` out of this path are + // all now answered, and they were good objections — recorded here so the + // next reader does not have to re-earn them: + // + // - "could not be verified against the pinned build" — it is now: + // authelia v4.39.20's own `validate-config` accepts `watch` and + // *rejects* a deliberate misspelling of it, so the key is recognised + // rather than silently swallowed. + // - "does the watch survive the rename(2) used above" — yes: authelia + // watches the containing *directory*, so a rename into it is observed. + // A watch on the old inode alone would indeed have missed it. + // - "can it observe a partially written file" — it cannot, and that is + // structural rather than a debounce we are trusting: every writer of + // this file goes through `write_atomic` below, so no partial content is + // ever visible under the final name. (Authelia debounces as well.) + Ok(()) } /// Load the canonical store, or start an empty one if this deployment has @@ -456,29 +468,6 @@ fn parse_field(stdout: &str, marker: &str) -> Option { .filter(|value| !value.is_empty()) } -/// authelia re-reads its file backend at startup, so a users change needs -/// a restart. -/// -/// The file watcher (`authentication_backend.file.watch`) would remove -/// this step entirely, and is deliberately not relied on: it could not be -/// verified against the pinned build, and it carries two unknowns — -/// whether the watch survives the `rename(2)` used above, and whether it -/// can observe a partially written file. An explicit restart assumes -/// nothing. -fn restart_authelia(machine: &str, unit: &str) -> Result<()> { - let status = Command::new("systemctl") - .args(["-M", machine, "restart", unit]) - .status() - .context("running systemctl")?; - if !status.success() { - bail!( - "restarting {unit} in {machine} failed ({status}); the users file is \ - already written, so re-running the restart by hand completes the change" - ); - } - Ok(()) -} - /// Replace `path`'s contents atomically, preserving the existing owner /// and mode. /// diff --git a/swarmctl/src/users.rs b/swarmctl/src/users.rs index 8d38e911..400d89a3 100644 --- a/swarmctl/src/users.rs +++ b/swarmctl/src/users.rs @@ -495,7 +495,7 @@ mod tests { ..UserUpdate::default() }, ) - .expect_err("a no-op must not restart authelia"); + .expect_err("a no-op must not rewrite the user database"); assert!(err.to_string().contains("nothing to change"), "{err}"); }