From 24f4cd42a9b0b83d82aa1a469fa60d73a70c0458 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 18 Aug 2026 10:31:48 +0200 Subject: [PATCH] fix(#3422): the bridge reads and writes users.yml directly `swarm agent create` failed with "no user store at swarm-authelia-bridge-users.json but users.yml already holds users" on any hive that had users. The guard was correct; what was wrong is that two files both claimed to be canonical for one physical file. The bridge kept a private users.json and rendered users.yml from it, while swarmctl kept its own pair against the same users.yml. A writer whose own JSON was absent could not tell "nothing here yet" from "someone else's users", so it refused to write at all. users.yml becomes the store: read before write, through serde_norway rather than a hand-rolled emitter. Unknown top-level and per-user keys round-trip through `extra`, or whichever process writes second would silently delete what the first added. Validation stays on the write path -- a bare to_string(&store) serialises perfectly and drops the "no control character ever reaches this file" guarantee silently. The seed constant goes with the guard: nothing writes a seed now, an absent file is an empty store, and left as a pub constant it read as if the seed dance were still load-bearing. --- Cargo.lock | 21 +++ swarm-authelia-bridge/Cargo.toml | 1 + swarm-authelia-bridge/src/main.rs | 7 +- swarm-authelia-bridge/src/store.rs | 261 ++++++++++++++++++----------- swarmctl/Cargo.toml | 1 + 5 files changed, 190 insertions(+), 101 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index edbb6a33..e68af4c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4324,6 +4324,19 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_norway" +version = "0.9.42" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e408f29489b5fd500fab51ff1484fc859bb655f32c671f307dcd733b72e8168c" +dependencies = [ + "indexmap", + "itoa", + "ryu", + "serde", + "unsafe-libyaml-norway", +] + [[package]] name = "serde_path_to_error" version = "0.1.20" @@ -4564,6 +4577,7 @@ dependencies = [ "reqwest 0.13.1", "serde", "serde_json", + "serde_norway", "swarm-authelia-bridge-sock", "tokio", "tracing", @@ -4649,6 +4663,7 @@ dependencies = [ "clap_complete", "serde", "serde_json", + "serde_norway", ] [[package]] @@ -5236,6 +5251,12 @@ dependencies = [ "subtle", ] +[[package]] +name = "unsafe-libyaml-norway" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b39abd59bf32521c7f2301b52d05a6a2c975b6003521cbd0c6dc1582f0a22104" + [[package]] name = "untrusted" version = "0.9.0" diff --git a/swarm-authelia-bridge/Cargo.toml b/swarm-authelia-bridge/Cargo.toml index 4e7fb493..cb177d95 100644 --- a/swarm-authelia-bridge/Cargo.toml +++ b/swarm-authelia-bridge/Cargo.toml @@ -14,6 +14,7 @@ axum.workspace = true reqwest.workspace = true serde.workspace = true serde_json.workspace = true +serde_norway = "0.9.42" swarm-authelia-bridge-sock.workspace = true tokio.workspace = true tracing.workspace = true diff --git a/swarm-authelia-bridge/src/main.rs b/swarm-authelia-bridge/src/main.rs index da061911..043daf6c 100644 --- a/swarm-authelia-bridge/src/main.rs +++ b/swarm-authelia-bridge/src/main.rs @@ -47,7 +47,6 @@ use swarm_authelia_bridge_sock::{BridgeRequest, BridgeResponse}; /// that provisions this bridge's own client alongside authelia. struct Config { bind: String, - store_path: std::path::PathBuf, users_file: std::path::PathBuf, authelia_bin: std::path::PathBuf, introspection_url: String, @@ -59,7 +58,6 @@ impl Config { fn from_env() -> Result { Ok(Self { bind: env_var("SWARM_AUTHELIA_BRIDGE_BIND")?, - store_path: env_var("SWARM_AUTHELIA_BRIDGE_STORE")?.into(), users_file: env_var("SWARM_AUTHELIA_BRIDGE_USERS_FILE")?.into(), authelia_bin: env_var("SWARM_AUTHELIA_BRIDGE_AUTHELIA_BIN")?.into(), introspection_url: env_var("SWARM_AUTHELIA_BRIDGE_INTROSPECTION_URL")?, @@ -265,7 +263,7 @@ async fn handle(state: &AppState, name: String) -> Result { // practice, not just theoretically. let _write_guard = state.write_lock.lock().await; - let mut user_store = store::load_store(&cfg.store_path, &cfg.users_file)?; + let mut user_store = store::load_store(&cfg.users_file)?; if user_store.users.contains_key(&name) { return Ok(BridgeResponse::AlreadyExists); } @@ -277,9 +275,10 @@ async fn handle(state: &AppState, name: String) -> Result { password: generated, email: None, groups: Vec::new(), + extra: std::collections::BTreeMap::new(), }, ); - store::publish(&cfg.store_path, &cfg.users_file, &user_store)?; + store::publish(&cfg.users_file, &user_store)?; tracing::info!(agent = %name, "created authelia identity"); Ok(BridgeResponse::Created) } diff --git a/swarm-authelia-bridge/src/store.rs b/swarm-authelia-bridge/src/store.rs index 15e8ec9b..ffc0a6c3 100644 --- a/swarm-authelia-bridge/src/store.rs +++ b/swarm-authelia-bridge/src/store.rs @@ -13,15 +13,27 @@ //! `swarm-authelia` and `swarm-controller` split across hosts, and this //! bridge only ever runs where `swarm-authelia` does. //! -//! ⚠️ **Known limitation, not solved here**: `swarmctl` still writes its -//! own independent `users.json`/`users.yml` for human accounts, assuming -//! co-location with `swarm-controller`'s host. Two independent canonical -//! stores for the same physical `users.yml` is a real seam — tracked as a -//! follow-up (route `swarmctl` through this bridge too), not attempted in -//! this slice, whose scope is agent identities only. +//! # `users.yml` is the store, not a rendering of one +//! +//! There used to be a private `users.json` here, canonical, with `users.yml` +//! rendered from it — and `swarmctl` had its own pair against the *same* +//! physical `users.yml`. Two canonical stores for one file is a seam, and it +//! bit: a bridge whose own JSON was absent refused to write at all, because +//! it could not tell "nothing here yet" from "someone else's users". +//! +//! The JSON bought nothing. It was read back on every load, so the +//! round-trip it seemed to avoid was already being paid — the two files +//! differed only in *format*. One file removes the class: there is no second +//! store to disagree with, and the overwrite guard that policed them has +//! nothing left to do. +//! +//! ⚠️ Consequence, deliberate: this file is now **round-tripped**, so +//! comments and hand-formatting in it do not survive a write. An operator +//! editing it directly gets their *values* kept and their *comments* dropped. +//! Unknown keys are preserved (see `extra` below) so a field this binary does +//! not know about is not deleted by it. use std::collections::BTreeMap; -use std::fmt::Write as _; use std::fs::{self, File, Permissions}; use std::io::Write as _; use std::os::unix::fs::PermissionsExt as _; @@ -30,17 +42,21 @@ use std::path::Path; use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; -/// What the `swarm-authelia` module's first-boot unit writes into -/// `users.yml` when there is no database yet — same constant/shape -/// `swarmctl::users::SEED_USERS_FILE` uses, since both write the same -/// physical file format. -pub const SEED_USERS_FILE: &str = "users: {}"; - -/// The canonical store, serialised as JSON. `BTreeMap` for a stable, -/// diffable render — same rationale as `swarmctl::users::UserStore`. +/// The canonical store — this *is* `users.yml`, deserialised. `BTreeMap` +/// for a stable, diffable file: authelia does not care about key order, but +/// a human reading `git diff` does. #[derive(Debug, Default, Serialize, Deserialize)] pub struct UserStore { pub users: BTreeMap, + /// Top-level keys this binary does not model, carried through a + /// round-trip untouched. + /// + /// Two processes write this file and neither is authoritative about the + /// other's fields. Without this, whichever writes second silently + /// deletes anything the first added — the same shape as the bug that + /// made one file canonical in the first place, one level down. + #[serde(flatten)] + pub extra: BTreeMap, } #[derive(Debug, Serialize, Deserialize)] @@ -52,6 +68,10 @@ pub struct User { pub email: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub groups: Vec, + /// Per-user keys this binary does not model (authelia's `disabled`, for + /// one). Same preservation rule as [`UserStore::extra`]. + #[serde(flatten)] + pub extra: BTreeMap, } /// Same conservative charset `swarmctl::users::validate_username` already @@ -87,99 +107,74 @@ fn reject_control_chars(field: &str, value: &str) -> Result<()> { Ok(()) } -/// A double-quoted YAML scalar — everything is quoted, including values -/// that would be fine bare, since an argon2 digest alone contains `$`, -/// `=`, `,` and `/`. Control characters are excluded upstream, so `"` and -/// `\` are the complete escape set. -fn quote(s: &str) -> String { - let mut out = String::with_capacity(s.len() + 2); - out.push('"'); - for c in s.chars() { - match c { - '"' => out.push_str("\\\""), - '\\' => out.push_str("\\\\"), - _ => out.push(c), - } - } - out.push('"'); - out +/// Render the store as authelia's YAML users database. +/// +/// Serialisation is `serde_norway`'s — quoting an argon2 digest (`$`, `=`, +/// `,`, `/`) is exactly the kind of thing a real emitter gets right and a +/// hand-rolled one gets right until it doesn't. +/// +/// ⚠️ **Still fallible, and that is the load-bearing part.** It re-validates +/// every value it is about to emit, so *"no control character ever reaches +/// this file"* stays a property of the **one path that writes it** rather +/// than a rule every caller has to remember. A bare `to_string(&store)` +/// would serialise perfectly and drop that guarantee silently. +pub fn render_yaml(store: &UserStore) -> Result { + validate(store)?; + serde_norway::to_string(store).context("serialising the users database") } -/// Render the store as authelia's block-style YAML users database. -/// Fallible for the same reason `swarmctl::users::render_yaml` is: it -/// re-validates every value it is about to emit, so "no control character -/// ever reaches the file" is a property of the one path that writes it. -pub fn render_yaml(store: &UserStore) -> Result { - if store.users.is_empty() { - return Ok(format!("{SEED_USERS_FILE}\n")); - } - let mut out = String::from("users:\n"); +/// Everything [`render_yaml`] refuses to write. Separate so the rule can be +/// tested directly, and so a second writer can call it without going +/// through serialisation. +fn validate(store: &UserStore) -> Result<()> { for (name, user) in &store.users { validate_username(name)?; reject_control_chars("displayname", &user.displayname)?; reject_control_chars("password digest", &user.password)?; - writeln!(out, " {name}:")?; - writeln!(out, " displayname: {}", quote(&user.displayname))?; - writeln!(out, " password: {}", quote(&user.password))?; if let Some(email) = &user.email { reject_control_chars("email", email)?; - writeln!(out, " email: {}", quote(email))?; } - if !user.groups.is_empty() { - writeln!(out, " groups:")?; - for group in &user.groups { - reject_control_chars("group", group)?; - writeln!(out, " - {}", quote(group))?; - } + for group in &user.groups { + reject_control_chars("group", group)?; } } - Ok(out) + Ok(()) } -/// Load the canonical store, or start an empty one if this bridge has -/// never written a user. Same overwrite guard as -/// `swarmctl::load_store`: starting empty means the next write -/// **overwrites** `users_file`, which is only safe when that file is -/// still the untouched first-boot seed. -pub fn load_store(store_path: &Path, users_file: &Path) -> Result { - match fs::read_to_string(store_path) { - Ok(raw) => serde_json::from_str(&raw) - .with_context(|| format!("parsing the user store at {}", store_path.display())), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - match fs::read_to_string(users_file) { - Ok(existing) if !is_untouched_seed(&existing) => bail!( - "no user store at {} but {} already holds users — refusing to \ - overwrite it", - store_path.display(), - users_file.display() - ), - Ok(_) | Err(_) => Ok(UserStore::default()), - } - } - Err(e) => Err(e).with_context(|| format!("reading {}", store_path.display())), +/// Load the users database, or start empty when it does not exist yet. +/// +/// ⚠️ **There is deliberately no overwrite guard here any more.** The old +/// one refused to write when a private JSON store was missing but +/// `users.yml` held users — it existed to police *two* stores that could +/// disagree, and with one store there is nothing to disagree with. Reading +/// the same file we are about to write means we cannot clobber users we did +/// not know about: we just read them. +/// +/// An absent or empty file is an empty store, not an error: first boot is a +/// legitimate state, and the seed document authelia's own unit writes +/// (`users: {}`) deserialises to exactly that with no special case. +pub fn load_store(users_file: &Path) -> Result { + match fs::read_to_string(users_file) { + Ok(raw) if raw.trim().is_empty() => Ok(UserStore::default()), + Ok(raw) => serde_norway::from_str(&raw) + .with_context(|| format!("parsing the users database at {}", users_file.display())), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(UserStore::default()), + Err(e) => Err(e).with_context(|| format!("reading {}", users_file.display())), } } -/// Whether an existing `users.yml` is safe to take over — i.e. it is the -/// untouched first-boot seed. Empty counts too: a zero-byte file holds -/// nothing to lose. -fn is_untouched_seed(contents: &str) -> bool { - let trimmed = contents.trim(); - trimmed.is_empty() || trimmed == SEED_USERS_FILE -} - /// Write the store + the rendered users file. **No restart** (the load- /// bearing difference from `swarmctl::publish`): this bridge relies on /// authelia's `authentication_backend.file.watch`, confirmed working /// against the pinned 4.39.20 build during this design work — a /// restart would drop every active SSO session, which mara ruled out for /// agent creation specifically (not a rare, human-initiated event). -pub fn publish(store_path: &Path, users_file: &Path, store: &UserStore) -> Result<()> { +pub fn publish(users_file: &Path, store: &UserStore) -> Result<()> { + // One file, so there is no longer an ordering question between two + // writes — the old version wrote the JSON store first so a crash + // between them left something to repair from. That whole failure mode + // belonged to having two files. let rendered = render_yaml(store)?; - let store_json = serde_json::to_string_pretty(store).context("serialising the user store")?; - // Store first — if the store lands and the users file doesn't, the - // next run re-renders and repairs it. The other order loses a user. - write_atomic(store_path, &format!("{store_json}\n"))?; write_atomic(users_file, &rendered) } @@ -225,29 +220,76 @@ fn write_atomic(path: &Path, contents: &str) -> Result<()> { mod tests { use super::*; + /// What the `swarm-authelia` module's first-boot unit writes into + /// `users.yml` when there is no database yet. + /// + /// A test fixture rather than a production constant: nothing in this + /// binary writes a seed any more — the store deserialises whatever is + /// there and an absent file is an empty store. Keeping it `pub` in the + /// module would be a constant nothing reads, which is exactly the kind + /// of leftover that makes the next reader think the seed dance is still + /// load-bearing. + const SEED_USERS_FILE: &str = "users: {}"; + fn user(password: &str) -> User { User { displayname: "atlas".to_owned(), password: password.to_owned(), email: None, groups: Vec::new(), + extra: BTreeMap::new(), } } + /// The document `swarm-authelia`'s first-boot unit writes must load as + /// an empty store with no special case — otherwise a fresh hive's very + /// first `EnsureAgentIdentity` fails on a file we wrote ourselves. #[test] - fn an_empty_store_renders_the_seed_document() { - let out = render_yaml(&UserStore::default()).expect("renders"); - assert_eq!(out, "users: {}\n"); - assert!(is_untouched_seed(&out)); + fn the_first_boot_seed_loads_as_an_empty_store() { + let store: UserStore = serde_norway::from_str(SEED_USERS_FILE).expect("seed parses"); + assert!(store.users.is_empty()); + assert!(store.extra.is_empty()); } + /// An argon2 digest is `$`, `=`, `,` and `/` — the reason a real + /// emitter replaced the hand-rolled one. Asserted by **round-trip**, + /// not by looking for quotes: how the emitter chooses to quote is its + /// business, that the value survives is ours. #[test] - fn an_argon2_digest_survives_quoting() { + fn an_argon2_digest_survives_a_round_trip() { let digest = "$argon2id$v=19$m=65536,t=3,p=4$c29tZXNhbHQ$aGFzaA+/w=="; let mut store = UserStore::default(); store.users.insert("atlas".to_owned(), user(digest)); + let out = render_yaml(&store).expect("renders"); - assert!(out.contains(&format!("password: \"{digest}\""))); + let back: UserStore = serde_norway::from_str(&out).expect("reparses"); + assert_eq!(back.users["atlas"].password, digest); + } + + /// A key neither writer models must survive the other's write. Without + /// this, two processes sharing one file silently delete each other's + /// fields — the same class of bug as the two stores this replaced. + #[test] + fn unknown_keys_survive_a_round_trip() { + let source = "\ +users: + atlas: + displayname: atlas + password: \"$argon2id$x\" + disabled: true +some_future_top_level_key: 7 +"; + let store: UserStore = serde_norway::from_str(source).expect("parses"); + let out = render_yaml(&store).expect("renders"); + + assert!( + out.contains("disabled"), + "a per-user key this binary does not model was dropped: {out}" + ); + assert!( + out.contains("some_future_top_level_key"), + "a top-level key this binary does not model was dropped: {out}" + ); } #[test] @@ -276,20 +318,45 @@ mod tests { /// `write_atomic` round-trips through a real temp dir — the property /// under test is the rename-into-place, not just the render. #[test] - fn publish_writes_both_files_and_the_store_reloads() { + fn publish_then_load_round_trips_through_the_real_file() { let dir = tempdir(); - let store_path = dir.join("users.json"); let users_file = dir.join("users.yml"); fs::write(&users_file, SEED_USERS_FILE).unwrap(); let mut store = UserStore::default(); store.users.insert("atlas".to_owned(), user("$argon2id$x")); - publish(&store_path, &users_file, &store).expect("publish"); + publish(&users_file, &store).expect("publish"); - let reloaded = load_store(&store_path, &users_file).expect("reload"); + let reloaded = load_store(&users_file).expect("reload"); + assert!(reloaded.users.contains_key("atlas")); + + fs::remove_dir_all(&dir).ok(); + } + + /// The bug this change closes: a `users.yml` holding users that this + /// binary did not write is **loaded**, not refused. The old code kept a + /// private store alongside it and bailed when the two disagreed. + #[test] + fn a_users_file_written_by_someone_else_is_read_not_refused() { + let dir = tempdir(); + let users_file = dir.join("users.yml"); + fs::write( + &users_file, + "users:\n mara:\n displayname: mara\n password: \"$argon2id$x\"\n", + ) + .unwrap(); + + let mut store = load_store(&users_file).expect("must load a foreign users file"); + assert!(store.users.contains_key("mara")); + + // …and adding to it keeps the existing user rather than replacing. + store + .users + .insert("atlas".to_owned(), user("$argon2id$second")); + publish(&users_file, &store).expect("publish"); + let reloaded = load_store(&users_file).expect("reload"); + assert!(reloaded.users.contains_key("mara"), "existing user lost"); assert!(reloaded.users.contains_key("atlas")); - let yaml = fs::read_to_string(&users_file).unwrap(); - assert!(yaml.contains("atlas")); fs::remove_dir_all(&dir).ok(); } diff --git a/swarmctl/Cargo.toml b/swarmctl/Cargo.toml index f9865229..74591b59 100644 --- a/swarmctl/Cargo.toml +++ b/swarmctl/Cargo.toml @@ -15,6 +15,7 @@ clap-markdown = "0.1" clap_complete.workspace = true serde.workspace = true serde_json.workspace = true +serde_norway = "0.9.42" [lints] workspace = true