diff --git a/CLAUDE.md b/CLAUDE.md index ecc18dcc..55fccb33 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 **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 + `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 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 edbb6a33..ad8ee01a 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", @@ -4648,7 +4662,7 @@ dependencies = [ "clap-markdown", "clap_complete", "serde", - "serde_json", + "serde_norway", ] [[package]] @@ -5236,6 +5250,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/docs/swarm/secrets.md b/docs/swarm/secrets.md index 8c670efc..23547b7c 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` | `users.json` (canonical) → `users.yml` (rendered) | `swarmctl`, on the host that runs authelia | +| authelia subject store | `swarmctl` and `swarm-authelia-bridge` | `users.yml` — one file, read and written by both | `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 c858428f..c78dab5e 100644 --- a/docs/swarm/sso.md +++ b/docs/swarm/sso.md @@ -48,9 +48,14 @@ this password is stored nowhere — record it now ``` The password is generated, hashed, and printed once; only the hash is -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). +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. This step stays manual on purpose. Bootstrapping an identity provider non-interactively means a secret arriving from somewhere — a file, an @@ -125,8 +130,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.json` canonical, -`users.yml` a rendered artifact). +same way it already owns authelia's user store (`users.yml`, read and +written in place). ### 3. A hive elsewhere diff --git a/docs/swarm/ui.md b/docs/swarm/ui.md index 006e5b22..f292993e 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` treats the existing entry as the canonical store, so the group -is what changes. +`swarmctl` reads the existing entry out of `users.yml`, 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 68acc6ce..1da3e791 100644 --- a/docs/tools/swarmctl-cli.md +++ b/docs/tools/swarmctl-cli.md @@ -25,8 +25,9 @@ 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 -* `--store ` — Canonical user store +* `--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. @@ -40,7 +41,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 the canonical store +* `list` — List every user in authelia's users database @@ -85,9 +86,9 @@ Every flag is optional and they compose, so one call can set several things at o ## `swarmctl user list` -List every user in the canonical store. +List every user in authelia's users database. -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). +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). **Usage:** `swarmctl user list` diff --git a/nix/host-modules/swarm-authelia.nix b/nix/host-modules/swarm-authelia.nix index e8f684ff..c25cf13a 100644 --- a/nix/host-modules/swarm-authelia.nix +++ b/nix/host-modules/swarm-authelia.nix @@ -920,14 +920,12 @@ in }; environment = { SWARM_AUTHELIA_BRIDGE_BIND = "127.0.0.1:${toString cfg.bridgePort}"; - # 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"; + # 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. 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 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/README.md b/swarm-authelia-bridge/README.md index c9d040e6..cb80a825 100644 --- a/swarm-authelia-bridge/README.md +++ b/swarm-authelia-bridge/README.md @@ -32,10 +32,29 @@ 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. -## Known limitation +## One store, two writers -`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. +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. diff --git a/swarm-authelia-bridge/src/main.rs b/swarm-authelia-bridge/src/main.rs index da061911..1561ca7a 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, &mut 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..1d6ad3b4 100644 --- a/swarm-authelia-bridge/src/store.rs +++ b/swarm-authelia-bridge/src/store.rs @@ -1,27 +1,22 @@ -//! The canonical user store this bridge owns, and the authelia users -//! database rendered from it. +//! Authelia's users database — **the** store, not a rendering of one. //! -//! 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. +//! 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. //! -//! ⚠️ **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. +//! ⚠️ 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`. 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 +25,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 +51,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,102 +90,103 @@ 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: &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. 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: @@ -225,29 +229,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] @@ -275,21 +326,78 @@ mod tests { /// `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 publish_writes_both_files_and_the_store_reloads() { + 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() { 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, &mut 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, &mut 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..58095c99 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_json.workspace = true +serde_norway = "0.9.42" [lints] workspace = true diff --git a/swarmctl/README.md b/swarmctl/README.md index 18d64166..604a8e65 100644 --- a/swarmctl/README.md +++ b/swarmctl/README.md @@ -23,21 +23,24 @@ 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. -## Two files, one of them authoritative +## One file, two writers -- `users.json` — canonical, ours, JSON. -- `users.yml` — a **rendered artifact** for authelia. Written, never read - back. +`users.yml` — authelia's own users database — is read and written +directly. There is no second store. -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. +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 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. +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. ## Configuration @@ -53,7 +56,6 @@ 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 925605f6..515dc78b 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -34,13 +34,6 @@ 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 { @@ -67,17 +60,22 @@ 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 { @@ -85,10 +83,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")?, - store: self - .store - .or_else(|| std::env::var_os("SWARMCTL_STORE").map(PathBuf::from)) - .unwrap_or_else(|| PathBuf::from(DEFAULT_STORE)), }) } } @@ -157,12 +151,12 @@ enum UserVerb { /// editing an attribute, and folded together an attribute edit can /// invalidate a login by accident. Update(UpdateArgs), - /// List every user in the canonical store. + /// List every user in authelia's users database. /// - /// 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). + /// 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). List, } @@ -238,12 +232,12 @@ fn main() -> Result<()> { fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { users::validate_username(&args.username)?; - let mut store = load_store(&paths.store, &paths.users_file)?; + let mut store = load_store(&paths.users_file)?; if store.users.contains_key(&args.username) { bail!( "user {:?} already exists in {}", args.username, - paths.store.display() + paths.users_file.display() ); } @@ -256,10 +250,11 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { password: generated.digest, email: args.email, groups: args.groups, + extra: std::collections::BTreeMap::new(), }, ); - publish(paths, &store)?; + publish(paths, &mut store)?; println!("added {} to {}", args.username, paths.users_file.display()); println!("password: {}", generated.password); @@ -268,12 +263,12 @@ fn user_add(paths: &Paths, args: AddArgs) -> Result<()> { } fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { - let mut store = load_store(&paths.store, &paths.users_file)?; + let mut store = load_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.store.display() + paths.users_file.display() ); }; @@ -292,7 +287,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, &store)?; + publish(paths, &mut store)?; for change in &changes { println!("{change}"); @@ -301,14 +296,14 @@ fn user_update(paths: &Paths, args: UpdateArgs) -> Result<()> { Ok(()) } -/// `swarmctl user list` — read-only, never touches authelia's users file -/// or restarts it. Prints one line per user from the canonical store. +/// `swarmctl user list` — read-only: it loads authelia's users file and +/// writes nothing. One line per user, agent identities included. fn user_list(paths: &Paths) -> Result<()> { use std::fmt::Write as _; - let store = load_store(&paths.store, &paths.users_file)?; + let store = load_store(&paths.users_file)?; if store.users.is_empty() { - println!("no users in {}", paths.store.display()); + println!("no users in {}", paths.users_file.display()); return Ok(()); } for (username, user) in &store.users { @@ -324,23 +319,24 @@ fn user_list(paths: &Paths) -> Result<()> { Ok(()) } -/// Write the store + the rendered users file, then restart authelia. +/// Write the users database. /// -/// 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")?; +/// 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"); + } - // 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"))?; + // Render before writing: a value this refuses to emit should stop the + // whole operation rather than land a partial file. + let rendered = users::render_yaml(store)?; write_atomic(&paths.users_file, &rendered)?; // No restart: authelia watches this file @@ -351,38 +347,25 @@ fn publish(paths: &Paths, store: &UserStore) -> Result<()> { Ok(()) } -/// Load the canonical store, or start an empty one if this deployment has -/// never had a user added. +/// Load the users database, or start empty when it does not exist yet. /// -/// 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())), +/// ⚠️ **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())), } } @@ -562,25 +545,40 @@ mod tests { fs::remove_dir_all(&dir).ok(); } - /// The guard that stands between a missing store and an overwritten - /// user database. + /// 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. #[test] - fn load_store_refuses_to_take_over_a_populated_users_file() { - let dir = std::env::temp_dir().join(format!("swarmctl-guard-{}", std::process::id())); + fn a_populated_users_file_is_read_rather_than_refused() { + let dir = std::env::temp_dir().join(format!("swarmctl-takeover-{}", 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 password: \"x\"\n").expect("write"); - let err = load_store(&store, &users_file).expect_err("must refuse"); + 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"); assert!( - err.to_string().contains("refusing to overwrite"), - "unexpected error: {err}" + store.users.contains_key("mara"), + "a user this process did not write must be read, not refused" ); fs::write(&users_file, "users: {}\n").expect("seed"); - let taken = load_store(&store, &users_file).expect("the seed is takeable"); - assert!(taken.users.is_empty()); + assert!( + load_store(&users_file) + .expect("the seed loads") + .users + .is_empty(), + "the first-boot seed is an empty store, with no special case" + ); fs::remove_dir_all(&dir).ok(); } diff --git a/swarmctl/src/users.rs b/swarmctl/src/users.rs index a423d10d..e6a3b99b 100644 --- a/swarmctl/src/users.rs +++ b/swarmctl/src/users.rs @@ -1,49 +1,46 @@ -//! The swarm's SSO user store, and the authelia users database rendered -//! from it. +//! The swarm's SSO user store — authelia's own `users.yml`, read and +//! written directly. //! -//! **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. +//! **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. //! -//! 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. +//! 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*. //! -//! 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. +//! ⚠️ 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. use std::collections::BTreeMap; -use std::fmt::Write as _; -use anyhow::{Result, bail}; +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. +/// The canonical store — this *is* `users.yml`, deserialised. /// -/// 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. +/// `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. #[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)] @@ -57,6 +54,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, } /// Usernames are map **keys** in a YAML document, and they also reach @@ -100,28 +101,6 @@ 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 @@ -145,52 +124,66 @@ const SYNTHETIC_EMAIL_DOMAIN: &str = "hyperhive.local"; /// implement, and the login dies with `InternalError` rather than anything /// naming the missing field. /// -/// 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. +/// 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. fn synthetic_email(username: &str) -> String { format!("{username}@{SYNTHETIC_EMAIL_DOMAIN}") } -/// Render the store as authelia's block-style YAML users database. +/// Give every user an address before the store is written. /// -/// 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 { - if store.users.is_empty() { - return Ok(format!("{SEED_USERS_FILE}\n")); +/// 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 +} - let mut out = String::from("users:\n"); +/// 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. +pub fn render_yaml(store: &UserStore) -> Result { + validate(store)?; + serde_norway::to_string(store).context("serialising the users database") +} + +/// Everything [`render_yaml`] refuses to write. Separate so the rule can be +/// tested directly, 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))?; - // 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))?; - } + if let Some(email) = &user.email { + reject_control_chars("email", email)?; + } + for group in &user.groups { + reject_control_chars("group", group)?; } } - Ok(out) + Ok(()) } /// A requested change to an existing user. @@ -304,17 +297,6 @@ 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::*; @@ -325,39 +307,91 @@ mod tests { password: password.to_owned(), email: None, groups: Vec::new(), + extra: BTreeMap::new(), } } #[test] - fn an_empty_store_renders_the_seed_document() { + fn an_empty_store_round_trips_as_an_empty_store() { let out = render_yaml(&UserStore::default()).expect("empty store renders"); - assert_eq!(out, "users: {}\n"); + let back: UserStore = serde_norway::from_str(&out).expect("re-reads"); assert!( - 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" + back.users.is_empty(), + "removing the last user must leave a file the next run can load, got:\n{out}" ); } /// The digest is the value most likely to break a naive emitter: it /// carries `$`, `=`, `,` and `/`, and `,` in particular terminates a - /// YAML flow scalar. + /// 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. #[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("mara".to_owned(), user(digest)); let out = render_yaml(&store).expect("renders"); - assert!( - out.contains(&format!("password: \"{digest}\"")), - "digest must be emitted verbatim inside double quotes, got:\n{out}" + 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}" ); } + /// 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 quoting_escapes_backslash_and_quote() { - assert_eq!(quote(r#"a"b\c"#), r#""a\"b\\c""#); + 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()); } #[test] @@ -372,22 +406,9 @@ mod tests { ); } - /// 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}" - ); - } - + /// 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. #[test] fn a_supplied_email_is_never_replaced_by_the_synthetic_one() { let mut u = user("$argon2id$x"); @@ -395,11 +416,16 @@ mod tests { let mut store = UserStore::default(); store.users.insert("mara".to_owned(), u); - let out = render_yaml(&store).expect("renders"); assert!( - out.contains(r#"email: "real@elsewhere.example""#), - "the supplied address must win:\n{out}" + 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("mara@hyperhive.local"), "the synthetic address must not also appear:\n{out}" @@ -416,8 +442,13 @@ mod tests { 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 groups_render_as_a_block_sequence() { + fn a_users_groups_survive_a_round_trip_in_order() { let mut u = user("$argon2id$x"); u.email = Some("mara@example.com".to_owned()); u.groups = vec!["admins".to_owned(), "operators".to_owned()]; @@ -425,19 +456,12 @@ mod tests { store.users.insert("mara".to_owned(), u); let out = render_yaml(&store).expect("renders"); - 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", - ) - ); + 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"); } /// Ordering is a property of the artifact, not an accident: an @@ -608,17 +632,27 @@ mod tests { assert_eq!(u.displayname, "Test User", "the user must be untouched"); } - /// The seed check gates an irreversible overwrite, so it has to be - /// tolerant of trailing whitespace and intolerant of everything else. + /// 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. #[test] - 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")); + 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. assert!( - 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" + 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" ); } }