diff --git a/CLAUDE.md b/CLAUDE.md index 55fccb33..ecc18dcc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -153,9 +153,9 @@ hand-maintained per-file tree drifts out of sync with the code. **root and acts directly** — no socket, no HTTP route, no priv helper; the crate's README records why the rootless shape was examined and rejected. Does not link `swarm-controller`, mirroring `hivectl` ÷ - `hive-c0re`. ⚠️ Its user store is authelia's own **`users.yml`, read and - written in place** — one file, shared with `swarm-authelia-bridge`; see - that crate's README for what both writers must uphold. Full, always-current + `hive-c0re`. ⚠️ Its user store is **two files, one authoritative**: + `users.json` is canonical, authelia's `users.yml` is a *rendered + artifact* that is written and never read back. Full, always-current verb reference (CI-enforced against the clap tree, same pattern as `hivectl`'s — see `docs/conventions.md`): [`docs/tools/swarmctl-cli.md`](docs/tools/swarmctl-cli.md). diff --git a/Cargo.lock b/Cargo.lock index ad8ee01a..edbb6a33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4324,19 +4324,6 @@ 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" @@ -4577,7 +4564,6 @@ dependencies = [ "reqwest 0.13.1", "serde", "serde_json", - "serde_norway", "swarm-authelia-bridge-sock", "tokio", "tracing", @@ -4662,7 +4648,7 @@ dependencies = [ "clap-markdown", "clap_complete", "serde", - "serde_norway", + "serde_json", ] [[package]] @@ -5250,12 +5236,6 @@ 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/docs/swarm/secrets.md b/docs/swarm/secrets.md index 23547b7c..8c670efc 100644 --- a/docs/swarm/secrets.md +++ b/docs/swarm/secrets.md @@ -47,7 +47,7 @@ Every row below is read against one of these. | authelia OIDC issuer key (RSA) | same unit | `/var/lib/authelia-swarm/oidc-issuer.key` | same — relying parties verify against the **public** half at `/jwks.json` | | OIDC client secret, plaintext half | `authelia crypto hash generate --random` | `/var/lib/authelia-swarm/oidc-clients/.secret` | operator provides the file and names it in the service's `sso.clientSecretFile` | | OIDC client secret, digest half | the same mint | `oidc-clients/.digest` | authelia's own half; merged at runtime via `settingsFiles` | -| authelia subject store | `swarmctl` and `swarm-authelia-bridge` | `users.yml` — one file, read and written by both | `swarmctl`, on the host that runs authelia | +| authelia subject store | `swarmctl` | `users.json` (canonical) → `users.yml` (rendered) | `swarmctl`, on the host that runs authelia | | wireguard private key | **the operator** — `wg genkey` | whatever `swarm.wireguard.privateKeyFile` names | always operator-provided; nothing generates this for you | | queue auth-callout nkeys (user seed + account seed) | `swarm-nats-callout-keys` first-boot unit, when `nats.autoGenerateCallout` is set | `/var/lib/swarm-nats-callout/{callout-user,issuer}.seed`, `0600` | operator mints both with `nk` and names them in `nats.calloutUserSeedFile` / `nats.calloutIssuerSeedFile` | diff --git a/docs/swarm/sso.md b/docs/swarm/sso.md index c78dab5e..c858428f 100644 --- a/docs/swarm/sso.md +++ b/docs/swarm/sso.md @@ -48,14 +48,9 @@ this password is stored nowhere — record it now ``` The password is generated, hashed, and printed once; only the hash is -kept. `swarmctl` reads and writes authelia's `users.yml` directly — it is -the one user store, shared with `swarm-authelia-bridge`, which creates -agent identities in the same file. No restart: authelia watches it. Full -reference: [`../tools/swarmctl-cli.md`](../tools/swarmctl-cli.md). - -You can edit `users.yml` by hand, and `swarmctl` will read what you -wrote. ⚠️ It rewrites the whole file on every change, so **comments and -formatting do not survive**; values and unrecognised keys do. +kept. `swarmctl` writes its canonical `users.json`, re-renders authelia's +`users.yml` from it, and restarts authelia. Full reference: +[`../tools/swarmctl-cli.md`](../tools/swarmctl-cli.md). This step stays manual on purpose. Bootstrapping an identity provider non-interactively means a secret arriving from somewhere — a file, an @@ -130,8 +125,8 @@ presenting as "the forge is broken", several layers from its cause. ### 2. Swarm-managed services The controller side owns provisioning: `swarmctl` writes both halves, the -same way it already owns authelia's user store (`users.yml`, read and -written in place). +same way it already owns authelia's user store (`users.json` canonical, +`users.yml` a rendered artifact). ### 3. A hive elsewhere diff --git a/docs/swarm/ui.md b/docs/swarm/ui.md index f292993e..006e5b22 100644 --- a/docs/swarm/ui.md +++ b/docs/swarm/ui.md @@ -45,8 +45,8 @@ would have meant those accounts silently failing a check they were supposed to pass. An account created without any group needs re-adding with the flag — -`swarmctl` reads the existing entry out of `users.yml`, so the group is -what changes. +`swarmctl` treats the existing entry as the canonical store, so the group +is what changes. Why a group and not a list of usernames: agents are getting authelia accounts of their own (matrix SSO), and *authenticated* would then diff --git a/docs/tools/swarmctl-cli.md b/docs/tools/swarmctl-cli.md index 1da3e791..68acc6ce 100644 --- a/docs/tools/swarmctl-cli.md +++ b/docs/tools/swarmctl-cli.md @@ -25,9 +25,8 @@ swarm-level operator CLI ###### **Options:** * `--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. - - This is the only user store: it is read before every change and written in place, and `swarm-authelia-bridge` writes the same file. +* `--users-file ` — Host-side path of authelia's users database — i.e. the path inside the container, prefixed with the container's root +* `--store ` — Canonical user store @@ -41,7 +40,7 @@ Manage subjects in the swarm's SSO provider * `add` — Add a user, generating a password for them * `update` — Change an existing user's attributes -* `list` — List every user in authelia's users database +* `list` — List every user in the canonical store @@ -86,9 +85,9 @@ Every flag is optional and they compose, so one call can set several things at o ## `swarmctl user list` -List every user in authelia's users database. +List every user in the canonical store. -Read-only: it never writes the file. Shows every subject in it, including agent identities `swarm-authelia-bridge` created — one line per user: username, display name, email (if set), groups (if any). +Reads `store` only — never authelia's rendered `users.yml`, which is a derived artifact this crate writes and never reads back (see the crate doc comment). One line per user: username, display name, email (if set), groups (if any). **Usage:** `swarmctl user list` diff --git a/nix/host-modules/swarm-authelia.nix b/nix/host-modules/swarm-authelia.nix index c25cf13a..e8f684ff 100644 --- a/nix/host-modules/swarm-authelia.nix +++ b/nix/host-modules/swarm-authelia.nix @@ -920,12 +920,14 @@ in }; environment = { SWARM_AUTHELIA_BRIDGE_BIND = "127.0.0.1:${toString cfg.bridgePort}"; - # ONE file, and this is it. The bridge used to carry a - # second, private `…-users.json` it treated as canonical - # while writing `users.yml` as a rendering of it — two - # canonical stores for one physical file, which is what - # made `swarm agent create` refuse to start on a hive whose - # `users.yml` already held users. + # Own canonical store, alongside authelia's own state — + # NOT `swarm-controller`'s state dir: the two processes + # aren't guaranteed to be on the same host, and this store + # has to live wherever its writer (this bridge) does. See + # `swarm-authelia-bridge/README.md`'s "known limitation" + # section for the resulting `swarmctl`-owns-a-second-store + # seam. + SWARM_AUTHELIA_BRIDGE_STORE = "${stateDir}/swarm-authelia-bridge-users.json"; SWARM_AUTHELIA_BRIDGE_USERS_FILE = cfg.usersFile; # The CONFIGURED authelia, not whatever is on `PATH`: the # argon2 parameters baked into a hash have to match the diff --git a/swarm-authelia-bridge/Cargo.toml b/swarm-authelia-bridge/Cargo.toml index cb177d95..4e7fb493 100644 --- a/swarm-authelia-bridge/Cargo.toml +++ b/swarm-authelia-bridge/Cargo.toml @@ -14,7 +14,6 @@ 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/README.md b/swarm-authelia-bridge/README.md index cb80a825..c9d040e6 100644 --- a/swarm-authelia-bridge/README.md +++ b/swarm-authelia-bridge/README.md @@ -32,29 +32,10 @@ simply owns the file it writes. No elevated privilege anywhere. `swarm-controller` (loopback) and a split-host one (bind wider + firewall) with no separate transport. -## One store, two writers +## Known limitation -This bridge and `swarmctl` both read and write authelia's `users.yml` -directly. That is the whole arrangement — neither keeps a private copy it -considers canonical. - -It used to be otherwise, and the seam was real: each side had its own -`users.json` treated as authoritative, rendering the *same* physical -`users.yml`. A writer whose own JSON was missing could not tell "nothing -here yet" from "someone else's users", so it refused to write at all — -which is exactly what a hive with existing users hit (#3422). - -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. -- **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. -- **Every user gets an email.** A relying party asking for the claim fails - rather than degrades, so both write paths fill in a synthetic address - when none was supplied. - -Neither restarts authelia: it watches the file. `swarmctl` *could* (it is -root); this bridge deliberately cannot, and a reload that depends on which -process wrote is not a reload. +`swarmctl` still writes an independent `users.json`/`users.yml` for human +accounts, assuming co-location with `swarm-controller`'s host. Two +canonical stores for the same physical file is a real seam, not solved +here — routing `swarmctl` through this bridge too is a plausible follow-up, +out of scope for the agent-identity slice this crate shipped with. diff --git a/swarm-authelia-bridge/src/main.rs b/swarm-authelia-bridge/src/main.rs index 1561ca7a..da061911 100644 --- a/swarm-authelia-bridge/src/main.rs +++ b/swarm-authelia-bridge/src/main.rs @@ -47,6 +47,7 @@ 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, @@ -58,6 +59,7 @@ 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")?, @@ -263,7 +265,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.users_file)?; + let mut user_store = store::load_store(&cfg.store_path, &cfg.users_file)?; if user_store.users.contains_key(&name) { return Ok(BridgeResponse::AlreadyExists); } @@ -275,10 +277,9 @@ async fn handle(state: &AppState, name: String) -> Result { password: generated, email: None, groups: Vec::new(), - extra: std::collections::BTreeMap::new(), }, ); - store::publish(&cfg.users_file, &mut user_store)?; + store::publish(&cfg.store_path, &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 1d6ad3b4..15e8ec9b 100644 --- a/swarm-authelia-bridge/src/store.rs +++ b/swarm-authelia-bridge/src/store.rs @@ -1,22 +1,27 @@ -//! Authelia's users database — **the** store, not a rendering of one. +//! The canonical user store this bridge owns, and the authelia users +//! database rendered from it. //! -//! There used to be a private `users.json` here, canonical, with `users.yml` -//! rendered from it, while `swarmctl` kept its own pair against the *same* -//! physical file. Two canonical stores for one file is a seam, and it bit: a -//! writer whose own JSON was absent refused to write at all, unable to 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. +//! Deliberately its own copy, not shared code with `swarmctl::users` even +//! though the YAML shape is identical (same "factor out later if +//! duplication actually bites" reasoning as `swarm-controller::forge` +//! mirroring `hive-c0re::forge` — see the agent-identity-at-swarm-level +//! design thread). One real difference from `swarmctl`'s copy: this +//! store lives **wherever this +//! bridge runs** (inside the `swarm-authelia` container, alongside +//! `authelia-swarm`'s own state), not under `swarm-controller`'s state +//! dir — the two are not guaranteed to be the same host once +//! `swarm-authelia` and `swarm-controller` split across hosts, and this +//! bridge only ever runs where `swarm-authelia` does. //! -//! ⚠️ The file is **round-tripped**, so comments and hand-formatting do not -//! survive a write; values and unmodelled keys do (see `extra`). -//! -//! Deliberately its own copy of the shape rather than code shared with -//! `swarmctl::users` — same "factor out if the duplication actually bites" -//! reasoning as `swarm-controller::forge` mirroring `hive-c0re::forge`. What -//! both copies must uphold is in `../README.md`. +//! ⚠️ **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. 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 _; @@ -25,21 +30,17 @@ use std::path::Path; use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; -/// 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. +/// 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`. #[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)] @@ -51,10 +52,6 @@ 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 @@ -90,103 +87,102 @@ fn reject_control_chars(field: &str, value: &str) -> Result<()> { Ok(()) } -/// 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") +/// 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 } -/// 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<()> { +/// 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"); 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))?; } - for group in &user.groups { - reject_control_chars("group", group)?; + if !user.groups.is_empty() { + writeln!(out, " groups:")?; + for group in &user.groups { + reject_control_chars("group", group)?; + writeln!(out, " - {}", quote(group))?; + } } } - Ok(()) + Ok(out) } -/// 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())), +/// 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())), } } +/// 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(users_file: &Path, store: &mut UserStore) -> Result<()> { - fill_missing_emails(store); - // 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. +pub fn publish(store_path: &Path, users_file: &Path, store: &UserStore) -> Result<()> { 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) } -/// Domain for an address this bridge invents — the same one -/// `swarmctl::users` uses, and the same one `hive-c0re` already gives every -/// agent's forge account. Never routable, deliberately: nothing sends mail -/// here, the address exists so a relying party asking for an `email` claim -/// gets one. -const SYNTHETIC_EMAIL_DOMAIN: &str = "hyperhive.local"; - -/// Give every user an address before the file is written. -/// -/// 🩸 **This bridge did not do it, and `swarmctl` did.** The fix for the -/// missing-email defect landed in `swarmctl` only, so every identity created -/// *here* landed in `users.yml` with no `email` — and a relying party that -/// asks for the claim does not degrade, it fails (grafana's OIDC login is -/// the measured case: it falls through to an endpoint authelia does not -/// implement and dies with an internal error). Two writers of one file -/// disagreeing about a required field is not a difference worth keeping, so -/// the rule now lives on both write paths. -fn fill_missing_emails(store: &mut UserStore) { - for (name, user) in &mut store.users { - if user.email.is_none() { - user.email = Some(format!("{name}@{SYNTHETIC_EMAIL_DOMAIN}")); - } - } -} - /// 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: @@ -229,76 +225,29 @@ 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 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()); + 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)); } - /// 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_a_round_trip() { + fn an_argon2_digest_survives_quoting() { 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"); - 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}" - ); + assert!(out.contains(&format!("password: \"{digest}\""))); } #[test] @@ -326,78 +275,21 @@ some_future_top_level_key: 7 /// `write_atomic` round-trips through a real temp dir — the property /// under test is the rename-into-place, not just the render. - /// 🩸 The asymmetry this fixes: the earlier missing-email fix gave - /// `swarmctl`-created humans a synthetic address and left bridge-created - /// **agents** with none, so two writers of one file disagreed about a - /// field a relying party treats as required — grafana's OIDC login fails - /// outright without it rather than degrading. - /// - /// Asserted through the real write path, because that is where the rule - /// lives: `handle` inserts a user with `email: None` and nothing between - /// there and the file would notice. #[test] - fn an_agent_identity_reaches_the_file_with_an_email() { - let dir = tempdir(); - let users_file = dir.join("users.yml"); - - let mut store = UserStore::default(); - store.users.insert("atlas".to_owned(), user("$argon2id$x")); - assert!( - store.users["atlas"].email.is_none(), - "the fixture must start with the state `handle` creates" - ); - publish(&users_file, &mut store).expect("publish"); - - let reloaded = load_store(&users_file).expect("reload"); - assert_eq!( - reloaded.users["atlas"].email.as_deref(), - Some("atlas@hyperhive.local"), - "an agent must not land in the file without an email" - ); - - fs::remove_dir_all(&dir).ok(); - } - - #[test] - fn publish_then_load_round_trips_through_the_real_file() { + fn publish_writes_both_files_and_the_store_reloads() { 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(&users_file, &mut store).expect("publish"); + publish(&store_path, &users_file, &store).expect("publish"); - 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, &mut store).expect("publish"); - let reloaded = load_store(&users_file).expect("reload"); - assert!(reloaded.users.contains_key("mara"), "existing user lost"); + let reloaded = load_store(&store_path, &users_file).expect("reload"); 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 58095c99..f9865229 100644 --- a/swarmctl/Cargo.toml +++ b/swarmctl/Cargo.toml @@ -14,7 +14,7 @@ clap.workspace = true clap-markdown = "0.1" clap_complete.workspace = true serde.workspace = true -serde_norway = "0.9.42" +serde_json.workspace = true [lints] workspace = true diff --git a/swarmctl/README.md b/swarmctl/README.md index 604a8e65..18d64166 100644 --- a/swarmctl/README.md +++ b/swarmctl/README.md @@ -23,24 +23,21 @@ a verb has to run as a non-root user or from another host, the answer is a **group-gated admin socket**, separate from the controller's `0666` gateway-facing one — not a widening of what root does here. -## One file, two writers +## Two files, one of them authoritative -`users.yml` — authelia's own users database — is read and written -directly. There is no second store. +- `users.json` — canonical, ours, JSON. +- `users.yml` — a **rendered artifact** for authelia. Written, never read + back. -There used to be: a private `users.json` here, canonical, with `users.yml` -rendered from it, while `swarm-authelia-bridge` kept its own pair against -the *same* physical file. Two canonical stores for one file is a seam, and -it bit — a writer whose own JSON was missing could not tell "nothing here -yet" from "someone else's users", and refused to write (#3422). +The split is what lets this crate work without a YAML parser: the +workspace has none, and adding one costs a crates.io fetch, a lock update +and a vendor hash for a schema we fully control and only ever emit. -The argument for the split was that it let this crate work without a YAML -parser. It didn't: the JSON was read back on every run, so the round-trip -was already being paid — the two files differed only in *format*. - -⚠️ 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. -See `swarm-authelia-bridge/README.md` for what both writers must uphold. +The shortcut of writing JSON into the `.yml` (JSON being a subset of +YAML) is deliberately not taken: authelia refuses to start on a users file +it cannot parse, so that file fronts the whole SSO provider's boot, and +"almost certainly parses" is not a claim worth betting a boot on without +running it. ## Configuration @@ -56,6 +53,7 @@ an error. | `SWARMCTL_AUTHELIA_USERS_FILE` | host-side path of the users database | | `SWARMCTL_AUTHELIA_MACHINE` | container name, for `systemctl -M` | | `SWARMCTL_AUTHELIA_UNIT` | authelia's unit inside that container | +| `SWARMCTL_STORE` | canonical store (defaults to the controller's state dir) | ## Usage diff --git a/swarmctl/src/main.rs b/swarmctl/src/main.rs index 515dc78b..925605f6 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -34,6 +34,13 @@ use clap::{Args, Parser, Subcommand}; use users::{User, UserStore}; +/// Canonical user store. A compiled-in default is legitimate here for the +/// same reason it is on the daemon's socket path: this is a path this +/// process **creates**, not an address it hopes to find something at. It +/// lives under the controller's state directory because the controller is +/// this store's eventual reader. +const DEFAULT_STORE: &str = "/var/lib/swarm-controller/users.json"; + #[derive(Parser)] #[command(name = "swarmctl", version, about = "swarm-level operator CLI")] struct Cli { @@ -60,22 +67,17 @@ struct PathArgs { authelia_bin: Option, /// Host-side path of authelia's users database — i.e. the path inside /// the container, prefixed with the container's root. - /// - /// This is the only user store: it is read before every change and - /// written in place, and `swarm-authelia-bridge` writes the same file. - // - // The `--store` flag that named a second, private JSON store is gone - // rather than deprecated — a flag whose only remaining effect would be - // nothing reads as accepted and does nothing, where an unknown-argument - // error is loud. Not in the doc comment: `--help` is an operator - // surface, and the removal's reasoning belongs in the README. #[arg(long, value_name = "PATH")] users_file: Option, + /// Canonical user store. + #[arg(long, value_name = "PATH")] + store: Option, } struct Paths { authelia_bin: PathBuf, users_file: PathBuf, + store: PathBuf, } impl PathArgs { @@ -83,6 +85,10 @@ 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")?, + store: self + .store + .or_else(|| std::env::var_os("SWARMCTL_STORE").map(PathBuf::from)) + .unwrap_or_else(|| PathBuf::from(DEFAULT_STORE)), }) } } @@ -151,12 +157,12 @@ enum UserVerb { /// editing an attribute, and folded together an attribute edit can /// invalidate a login by accident. Update(UpdateArgs), - /// List every user in authelia's users database. + /// List every user in the canonical store. /// - /// Read-only: it never writes the file. Shows every subject in it, - /// including agent identities `swarm-authelia-bridge` created — one - /// line per user: username, display name, email (if set), groups (if - /// any). + /// Reads `store` only — never authelia's rendered `users.yml`, which + /// is a derived artifact this crate writes and never reads back (see + /// the crate doc comment). One line per user: username, display name, + /// email (if set), groups (if any). List, } @@ -232,12 +238,12 @@ fn main() -> Result<()> { fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { users::validate_username(&args.username)?; - let mut store = load_store(&paths.users_file)?; + let mut store = load_store(&paths.store, &paths.users_file)?; if store.users.contains_key(&args.username) { bail!( "user {:?} already exists in {}", args.username, - paths.users_file.display() + paths.store.display() ); } @@ -250,11 +256,10 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { password: generated.digest, email: args.email, groups: args.groups, - extra: std::collections::BTreeMap::new(), }, ); - publish(paths, &mut store)?; + publish(paths, &store)?; println!("added {} to {}", args.username, paths.users_file.display()); println!("password: {}", generated.password); @@ -263,12 +268,12 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { } fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { - let mut store = load_store(&paths.users_file)?; + let mut store = load_store(&paths.store, &paths.users_file)?; let Some(user) = store.users.get_mut(&args.username) else { bail!( "no user {:?} in {} — `swarmctl user add` creates one", args.username, - paths.users_file.display() + paths.store.display() ); }; @@ -287,7 +292,7 @@ fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { // list is the only way to notice. let groups = users::fmt_groups(&user.groups); - publish(paths, &mut store)?; + publish(paths, &store)?; for change in &changes { println!("{change}"); @@ -296,14 +301,14 @@ fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { Ok(()) } -/// `swarmctl user list` — read-only: it loads authelia's users file and -/// writes nothing. One line per user, agent identities included. +/// `swarmctl user list` — read-only, never touches authelia's users file +/// or restarts it. Prints one line per user from the canonical store. fn user_list(paths: &Paths) -> Result<()> { use std::fmt::Write as _; - let store = load_store(&paths.users_file)?; + let store = load_store(&paths.store, &paths.users_file)?; if store.users.is_empty() { - println!("no users in {}", paths.users_file.display()); + println!("no users in {}", paths.store.display()); return Ok(()); } for (username, user) in &store.users { @@ -319,24 +324,23 @@ fn user_list(paths: &Paths) -> Result<()> { Ok(()) } -/// Write the users database. +/// Write the store + the rendered users file, then restart authelia. /// -/// Shared by every verb that mutates it, so the rules below hold for all of -/// them rather than for whichever one was written first. -fn publish(paths: &Paths, store: &mut UserStore) -> Result<()> { - // Before rendering, not at creation: a user can also arrive by being - // *read* — from a file the bridge wrote, or one an operator edited — - // and an authelia subject with no `email` breaks any relying party that - // asks for the claim (grafana's OIDC login is the measured case: it - // fails outright rather than degrading). Filling it here is the only - // place no entry point can skip. - for name in users::fill_missing_emails(store) { - println!("note: {name} had no email; set to a synthetic address"); - } - - // Render before writing: a value this refuses to emit should stop the - // whole operation rather than land a partial file. +/// Shared by every verb that mutates the store, so the ordering rules +/// below hold for all of them rather than for whichever one was written +/// first. +fn publish(paths: &Paths, store: &UserStore) -> Result<()> { + // Render before writing anything: a value this refuses to emit should + // stop the whole operation, not leave the canonical store one user + // ahead of the file authelia reads. let rendered = users::render_yaml(store)?; + let store_json = serde_json::to_string_pretty(store).context("serialising the user store")?; + + // Store first, and the order matters. If the store lands and the + // users file does not, the next run re-renders and repairs it. The + // other order loses a user: the store would not know about someone + // authelia does, and the next render would silently drop them. + write_atomic(&paths.store, &format!("{store_json}\n"))?; write_atomic(&paths.users_file, &rendered)?; // No restart: authelia watches this file @@ -347,25 +351,38 @@ fn publish(paths: &Paths, store: &mut UserStore) -> Result<()> { Ok(()) } -/// Load the users database, or start empty when it does not exist yet. +/// Load the canonical store, or start an empty one if this deployment has +/// never had a user added. /// -/// ⚠️ **The overwrite guard that used to live here is gone, and its absence -/// is the fix rather than a regression.** It refused to write when this -/// crate's own JSON store was missing but `users.yml` held users — it -/// existed to police *two* stores that could disagree, and there is now one. -/// 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. -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())), +/// The guard exists because starting empty means the next write +/// **overwrites** authelia's users file. That is only safe when the file +/// is the untouched first-boot seed; anything else is a user database +/// somebody meant to be there, and losing it is the one unrecoverable +/// mistake this tool can make. +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 !users::is_untouched_seed(&existing) => bail!( + "no user store at {} but {} already holds users — refusing to \ + overwrite it. Reconstruct the store, or move the file aside if \ + it is disposable.", + store_path.display(), + users_file.display() + ), + Ok(_) => Ok(UserStore::default()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(UserStore::default()), + Err(e) => Err(e).with_context(|| { + format!( + "reading {} to check it is safe to take over", + users_file.display() + ) + }), + } + } + Err(e) => Err(e).with_context(|| format!("reading {}", store_path.display())), } } @@ -545,40 +562,25 @@ mod tests { fs::remove_dir_all(&dir).ok(); } - /// Replaces `load_store_refuses_to_take_over_a_populated_users_file`, - /// which pinned the guard this change removes. - /// - /// The guard refused to write when this crate's own JSON store was - /// missing but `users.yml` held users — the state every real hive is in, - /// and the reason `swarm agent create` failed. Deleting the test with - /// the guard would have left the *new* behaviour unpinned, so it is - /// inverted rather than dropped: a populated file must now be **read**, - /// and users somebody else wrote must survive. + /// The guard that stands between a missing store and an overwritten + /// user database. #[test] - fn a_populated_users_file_is_read_rather_than_refused() { - let dir = std::env::temp_dir().join(format!("swarmctl-takeover-{}", std::process::id())); + fn load_store_refuses_to_take_over_a_populated_users_file() { + let dir = std::env::temp_dir().join(format!("swarmctl-guard-{}", std::process::id())); fs::create_dir_all(&dir).expect("temp dir"); + let store = dir.join("absent.json"); let users_file = dir.join("users.yml"); - fs::write( - &users_file, - "users:\n mara:\n displayname: \"mara\"\n password: \"x\"\n", - ) - .expect("write"); - let store = load_store(&users_file).expect("a populated file is readable"); + fs::write(&users_file, "users:\n mara:\n password: \"x\"\n").expect("write"); + let err = load_store(&store, &users_file).expect_err("must refuse"); assert!( - store.users.contains_key("mara"), - "a user this process did not write must be read, not refused" + err.to_string().contains("refusing to overwrite"), + "unexpected error: {err}" ); fs::write(&users_file, "users: {}\n").expect("seed"); - assert!( - load_store(&users_file) - .expect("the seed loads") - .users - .is_empty(), - "the first-boot seed is an empty store, with no special case" - ); + let taken = load_store(&store, &users_file).expect("the seed is takeable"); + assert!(taken.users.is_empty()); fs::remove_dir_all(&dir).ok(); } diff --git a/swarmctl/src/users.rs b/swarmctl/src/users.rs index e6a3b99b..a423d10d 100644 --- a/swarmctl/src/users.rs +++ b/swarmctl/src/users.rs @@ -1,46 +1,49 @@ -//! The swarm's SSO user store — authelia's own `users.yml`, read and -//! written directly. +//! The swarm's SSO user store, and the authelia users database rendered +//! from it. //! -//! **One file.** There used to be a private `users.json` here, canonical, -//! with `users.yml` rendered from it — and `swarm-authelia-bridge` kept its -//! own pair against the *same* physical `users.yml`. Two canonical stores -//! for one file is a seam, and it bit: a writer whose own store was missing -//! could not tell *"nothing here yet"* from *"someone else's users"*, and -//! refused to write at all. +//! **Two files, and only one of them is authoritative.** `users.json` is +//! ours and canonical; `users.yml` is a rendered artifact for authelia, +//! written and never read back. //! -//! The JSON bought nothing. The old module doc argued it let this crate -//! exist without a YAML parser — but `load_store` read the JSON back on -//! every run, so the round-trip was already being paid; the two files -//! differed only in *format*. +//! That split is not a preference, it is what lets this crate exist +//! without a YAML parser: the workspace has none, and adding one costs a +//! crates.io fetch, a lock update and a vendor hash — for a schema we +//! fully control and only ever emit. Rendering in one direction needs no +//! parser at all. //! -//! ⚠️ Consequence, deliberate: this file is **round-tripped**, so comments -//! and hand-formatting do not survive a write. An operator editing it -//! directly keeps their *values* and loses their *comments*. Unknown keys -//! survive (see `extra`), so a field this binary does not model is not -//! deleted by it. +//! The tempting shortcut — *"JSON is a subset of YAML, so just write JSON +//! into the `.yml` and read it back with `serde_json`"* — is rejected on +//! purpose. authelia **refuses to start** on a users file it cannot +//! parse, so it fronts the whole SSO provider's boot, and "go-yaml +//! almost certainly accepts flow style" is not a claim worth betting that +//! on without running it. Block style is what upstream's own examples +//! show. use std::collections::BTreeMap; +use std::fmt::Write as _; -use anyhow::{Context, Result, bail}; +use anyhow::{Result, bail}; use serde::{Deserialize, Serialize}; -/// The canonical store — this *is* `users.yml`, deserialised. +/// What the swarm-authelia module's first-boot unit writes into +/// `users.yml` when there is no database yet. /// -/// `BTreeMap` rather than `HashMap` so the file is stable between runs: a -/// diffable artifact is worth more than the ordering being meaningless, and -/// a random-order rewrite makes every change look like a whole-file change. +/// Matched (after trimming) so a first `user add` can take the file over +/// without a flag, while anything *else* already in place is treated as +/// content somebody meant to be there. Overwriting a live SSO user +/// database because a canonical store happened to be missing is the one +/// unrecoverable mistake available here. +pub const SEED_USERS_FILE: &str = "users: {}"; + +/// The canonical store, serialised as JSON. +/// +/// `BTreeMap` rather than `HashMap` so the rendered YAML is stable +/// between runs: a diffable artifact is worth more than the ordering +/// being meaningless, and a random-order rewrite makes every change look +/// like a whole-file change. #[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 whatever 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)] @@ -54,10 +57,6 @@ 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, } /// Usernames are map **keys** in a YAML document, and they also reach @@ -101,6 +100,28 @@ 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: the +/// argon2 digest alone contains `$`, `=`, `,` and `/`, and deciding +/// per-value which of those need quoting is exactly the kind of judgement +/// that is right until one day it isn't. Control characters are excluded +/// upstream by [`reject_control_chars`], 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 +} + /// Domain for an address this crate invents. /// /// The same one `hive-c0re` already gives every agent's forge account @@ -124,66 +145,52 @@ const SYNTHETIC_EMAIL_DOMAIN: &str = "hyperhive.local"; /// implement, and the login dies with `InternalError` rather than anything /// naming the missing field. /// -/// Synthesised on the **write path** and stored, because the file *is* the -/// store now: "rendered but not persisted" no longer has anywhere to live. -/// mara ruled it directly — *"email in yml is what is already there and -/// correct"* — so a synthesised address is simply the user's address, and -/// the next `user update --email` overwrites it like any other value. +/// Synthesised in the **renderer**, never written to the store: `users.json` +/// stays honest that no address was supplied, so an operator who later sets a +/// real one is not fighting a value swarmctl invented, and every existing user +/// is fixed by the next render with no migration step. fn synthetic_email(username: &str) -> String { format!("{username}@{SYNTHETIC_EMAIL_DOMAIN}") } -/// Give every user an address before the store is written. +/// Render the store as authelia's block-style YAML users database. /// -/// Returns the names it filled, so a caller can say what it did rather than -/// changing the file silently. -/// -/// On the write path rather than at creation, because users also arrive by -/// being *read* — from a file another writer produced, or one an operator -/// edited. Fixing them where they enter would need every entry point to -/// remember; fixing them where they leave cannot be forgotten. -pub fn fill_missing_emails(store: &mut UserStore) -> Vec { - let mut filled = Vec::new(); - for (name, user) in &mut store.users { - if user.email.is_none() { - user.email = Some(synthetic_email(name)); - filled.push(name.clone()); - } - } - filled -} - -/// Serialise 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-runs -/// validation over every value it is about to emit, which is what makes -/// *"no control character ever reaches this file"* a property of the **one -/// path that writes it** rather than a rule every call site must remember. -/// A bare `to_string(&store)` serialises perfectly and drops that silently. +/// Fallible because it re-runs validation over every value it is about to +/// emit. That is not belt-and-braces: it makes "no control character ever +/// reaches the file" a property of the *only* path that writes it, rather +/// than a rule the call sites have to remember. pub fn render_yaml(store: &UserStore) -> Result { - validate(store)?; - serde_norway::to_string(store).context("serialising the users database") -} + if store.users.is_empty() { + return Ok(format!("{SEED_USERS_FILE}\n")); + } -/// Everything [`render_yaml`] refuses to write. Separate so the rule can be -/// tested directly, without going through serialisation. -fn validate(store: &UserStore) -> Result<()> { + let mut out = String::from("users:\n"); for (name, user) in &store.users { validate_username(name)?; reject_control_chars("displayname", &user.displayname)?; reject_control_chars("password digest", &user.password)?; - if let Some(email) = &user.email { - reject_control_chars("email", email)?; - } - for group in &user.groups { - reject_control_chars("group", group)?; + + writeln!(out, " {name}:")?; + writeln!(out, " displayname: {}", quote(&user.displayname))?; + writeln!(out, " password: {}", quote(&user.password))?; + // Unconditional: an absent email is the failure mode, not a valid + // rendering. The synthetic address is validated on the same path as + // a supplied one so neither can smuggle a control character. + let email = match &user.email { + Some(supplied) => supplied.clone(), + None => synthetic_email(name), + }; + 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))?; + } } } - Ok(()) + Ok(out) } /// A requested change to an existing user. @@ -297,6 +304,17 @@ pub fn fmt_groups(groups: &[String]) -> String { } } +/// Whether an existing `users.yml` is safe to take over when no canonical +/// store exists yet — i.e. it is the untouched first-boot seed. +/// +/// Empty counts: a zero-byte file holds no users to destroy, and authelia +/// would refuse to start on it anyway, so treating it as untouched turns +/// a dead deployment into a working one instead of demanding a flag. +pub fn is_untouched_seed(contents: &str) -> bool { + let trimmed = contents.trim(); + trimmed.is_empty() || trimmed == SEED_USERS_FILE +} + #[cfg(test)] mod tests { use super::*; @@ -307,91 +325,39 @@ mod tests { password: password.to_owned(), email: None, groups: Vec::new(), - extra: BTreeMap::new(), } } #[test] - fn an_empty_store_round_trips_as_an_empty_store() { + fn an_empty_store_renders_the_seed_document() { let out = render_yaml(&UserStore::default()).expect("empty store renders"); - let back: UserStore = serde_norway::from_str(&out).expect("re-reads"); + assert_eq!(out, "users: {}\n"); assert!( - back.users.is_empty(), - "removing the last user must leave a file the next run can load, got:\n{out}" + is_untouched_seed(&out), + "the empty rendering must itself read as the seed, or removing the \ + last user would produce a file the next run refuses to take over" ); } /// The digest is the value most likely to break a naive emitter: it /// carries `$`, `=`, `,` and `/`, and `,` in particular terminates a - /// YAML flow scalar. Asserted through a **round-trip** rather than - /// against a literal quoting style: which scalars a real emitter chooses - /// to quote is its business, and pinning the spelling would make this - /// test fail on a serializer upgrade that broke nothing. + /// YAML flow scalar. #[test] - fn an_argon2_digest_survives_a_round_trip() { + fn an_argon2_digest_survives_quoting() { let digest = "$argon2id$v=19$m=65536,t=3,p=4$c29tZXNhbHQ$aGFzaA+/w=="; let mut store = UserStore::default(); store.users.insert("mara".to_owned(), user(digest)); let out = render_yaml(&store).expect("renders"); - let back: UserStore = serde_norway::from_str(&out).expect("re-reads"); - assert_eq!( - back.users["mara"].password, digest, - "digest must survive verbatim, got:\n{out}" + assert!( + out.contains(&format!("password: \"{digest}\"")), + "digest must be emitted verbatim inside double quotes, got:\n{out}" ); } - /// Two processes write this file and neither knows the other's fields, - /// so anything unmodelled has to survive being read and written back — - /// otherwise the second writer silently deletes the first's work, which - /// is the same class of bug as the two-canonical-stores seam this change - /// removed, one level down. #[test] - fn unknown_keys_survive_a_round_trip() { - let raw = "\ -theme: dark -users: - mara: - displayname: \"mara\" - password: \"$argon2id$x\" - disabled: true -"; - let mut store: UserStore = serde_norway::from_str(raw).expect("parses"); - let out = render_yaml(&store).expect("renders"); - assert!( - out.contains("theme"), - "a top-level key this binary does not model was dropped:\n{out}" - ); - assert!( - out.contains("disabled"), - "a per-user key this binary does not model was dropped:\n{out}" - ); - - // ...and it must still survive once we have touched the entry the - // way a real verb does. - fill_missing_emails(&mut store); - let out = render_yaml(&store).expect("renders"); - assert!(out.contains("disabled"), "dropped after a write:\n{out}"); - } - - /// mara: *"email in yml is what is already there and correct"* — with the - /// file as the store, an address is filled in on the write path and kept, - /// rather than invented afresh by each render. - #[test] - fn a_user_with_no_email_gets_one_stored() { - let mut store = UserStore::default(); - store.users.insert("mara".to_owned(), user("$argon2id$x")); - - let filled = fill_missing_emails(&mut store); - assert_eq!(filled, vec!["mara".to_owned()]); - assert_eq!( - store.users["mara"].email.as_deref(), - Some("mara@hyperhive.local") - ); - - // Idempotent: a second pass must not report a change, or every - // publish would print a note about a user it did not touch. - assert!(fill_missing_emails(&mut store).is_empty()); + fn quoting_escapes_backslash_and_quote() { + assert_eq!(quote(r#"a"b\c"#), r#""a\"b\\c""#); } #[test] @@ -406,9 +372,22 @@ users: ); } - /// The earlier missing-email property, moved from the renderer to the - /// write path along with the synthesis itself: a user who supplied an - /// address keeps it, and no invented one appears beside it. + /// Email is deliberately NOT in the test above any more. It used to + /// assert that an absent one is omitted, which pinned the behaviour that + /// broke grafana's login: authelia serves no `email` claim, and a relying + /// party that wants one fails rather than degrading. + #[test] + fn a_user_with_no_email_still_renders_one_from_the_shared_domain() { + let mut store = UserStore::default(); + store.users.insert("mara".to_owned(), user("$argon2id$x")); + + let out = render_yaml(&store).expect("renders"); + assert!( + out.contains(r#"email: "mara@hyperhive.local""#), + "a user with no email must still render one:\n{out}" + ); + } + #[test] fn a_supplied_email_is_never_replaced_by_the_synthetic_one() { let mut u = user("$argon2id$x"); @@ -416,16 +395,11 @@ users: let mut store = UserStore::default(); store.users.insert("mara".to_owned(), u); - assert!( - fill_missing_emails(&mut store).is_empty(), - "a user with an address is not missing one" - ); - assert_eq!( - store.users["mara"].email.as_deref(), - Some("real@elsewhere.example"), - "the supplied address must win" - ); let out = render_yaml(&store).expect("renders"); + assert!( + out.contains(r#"email: "real@elsewhere.example""#), + "the supplied address must win:\n{out}" + ); assert!( !out.contains("mara@hyperhive.local"), "the synthetic address must not also appear:\n{out}" @@ -442,13 +416,8 @@ users: assert_eq!(synthetic_email("mara"), "mara@hyperhive.local"); } - /// Was `groups_render_as_a_block_sequence`, a byte-for-byte assertion on - /// the hand-rolled emitter's output — including which scalars it chose to - /// quote. That is the serializer's business now, and pinning it would - /// fail on an upgrade that broke nothing. What has to hold is that the - /// values come back, in order. #[test] - fn a_users_groups_survive_a_round_trip_in_order() { + fn groups_render_as_a_block_sequence() { let mut u = user("$argon2id$x"); u.email = Some("mara@example.com".to_owned()); u.groups = vec!["admins".to_owned(), "operators".to_owned()]; @@ -456,12 +425,19 @@ users: store.users.insert("mara".to_owned(), u); let out = render_yaml(&store).expect("renders"); - let back: UserStore = serde_norway::from_str(&out).expect("re-reads"); - let mara = &back.users["mara"]; - assert_eq!(mara.displayname, "Test User"); - assert_eq!(mara.password, "$argon2id$x"); - assert_eq!(mara.email.as_deref(), Some("mara@example.com")); - assert_eq!(mara.groups, ["admins", "operators"], "order is meaningful"); + assert_eq!( + out, + concat!( + "users:\n", + " mara:\n", + " displayname: \"Test User\"\n", + " password: \"$argon2id$x\"\n", + " email: \"mara@example.com\"\n", + " groups:\n", + " - \"admins\"\n", + " - \"operators\"\n", + ) + ); } /// Ordering is a property of the artifact, not an accident: an @@ -632,27 +608,17 @@ users: assert_eq!(u.displayname, "Test User", "the user must be untouched"); } - /// Replaces `only_the_untouched_seed_reads_as_takeable`. That test - /// pinned the seed check, which existed to decide whether overwriting - /// `users.yml` was safe — a question that only arose because a *second* - /// store claimed to be canonical. There is no overwrite to gate now: the - /// file is read before it is written. - /// - /// What still has to hold is that the first-boot seed and a zero-byte - /// file both mean "no users yet" rather than an error, so a fresh - /// deployment is not stranded. + /// The seed check gates an irreversible overwrite, so it has to be + /// tolerant of trailing whitespace and intolerant of everything else. #[test] - fn the_first_boot_seed_and_an_empty_file_both_mean_no_users() { - let seeded: UserStore = serde_norway::from_str("users: {}").expect("the seed parses"); - assert!(seeded.users.is_empty()); - - // The empty-file case is handled before deserialisation (an empty - // document is not valid YAML for this type), so it is asserted where - // it lives — `main::load_store` — rather than reproduced here. + fn only_the_untouched_seed_reads_as_takeable() { + assert!(is_untouched_seed("users: {}")); + assert!(is_untouched_seed("users: {}\n")); + assert!(!is_untouched_seed("users:\n mara:\n")); assert!( - serde_norway::from_str::("").is_err(), - "if this ever starts parsing, load_store's empty-file arm is \ - redundant rather than load-bearing, and should be revisited" + is_untouched_seed(""), + "a zero-byte file has no users to lose, and authelia will not start \ + on it — refusing here would strand the deployment" ); } }