From 36560cf8effaf59d16d98e37b1a4b5defa26dc6c Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Mon, 17 Aug 2026 19:44:28 -0300 Subject: [PATCH 01/12] fix(docker): set restrictive permissions on LND credentials and warn about ownership --- Makefile | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 348bfa2f..3b469258 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ VERSION := $(shell grep "^version = " Cargo.toml | sed "s/version = \"\(.*\)\"/\ docker-build: @set -o pipefail; \ cd docker && \ - mkdir -p config/lnd && \ + install -d -m 700 config/lnd && \ echo "Checking LND files..." && \ echo "LND_CERT_FILE=$${LND_CERT_FILE}" && \ echo "LND_MACAROON_FILE=$${LND_MACAROON_FILE}" && \ @@ -27,8 +27,11 @@ docker-build: exit 1; \ fi && \ echo "Copying LND cert and macaroon to docker config" && \ - cp -v $${LND_CERT_FILE} config/lnd/tls.cert && \ - cp -v $${LND_MACAROON_FILE} config/lnd/admin.macaroon && \ + install -m 644 "$${LND_CERT_FILE}" config/lnd/tls.cert && \ + install -m 600 "$${LND_MACAROON_FILE}" config/lnd/admin.macaroon && \ + echo "Wrote config/lnd/tls.cert (mode 644) and config/lnd/admin.macaroon (mode 600)" && \ + echo "Note: the container runs as uid/gid 1000. If these files are owned by another user," && \ + echo " run: sudo chown 1000:1000 docker/config/lnd/tls.cert docker/config/lnd/admin.macaroon" && \ echo "Building docker image" && \ docker compose build From 985fce866e580ce13d92e6891d8001c2fc5872d3 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Mon, 17 Aug 2026 19:50:04 -0300 Subject: [PATCH 02/12] docs: document admin macaroon permissions and use install(1) for credential copies Expand the admin macaroon security guidance across AGENTS.md, INSTALL.md, README.md, and docker docs. The macaroon grants full control of the LND node, including escrowed funds, so it must be readable only by the mostrod user. --- AGENTS.md | 2 +- INSTALL.md | 19 +++++++++++++++++++ Makefile | 4 ++-- README.md | 4 ++++ docker/ENV_VARIABLES.md | 8 +++++++- docker/README.md | 30 ++++++++++++++++++++++-------- 6 files changed, 55 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3ccd7e3c..61b4f71c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,5 +44,5 @@ ## Security & Configuration Tips - Do not commit populated `settings.toml`. Copy from `settings.tpl.toml` to `~/.mostro/settings.toml` for local runs. -- Protect LND credentials before `make docker-build`. +- Protect LND credentials before `make docker-build`. The admin macaroon is spend-capable: copy it with `install -m 600` (never plain `cp`, which inherits the source or destination mode) and keep its directory at `0700`. - Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed. diff --git a/INSTALL.md b/INSTALL.md index b63748b7..63a70783 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -108,6 +108,25 @@ Here some parameters you might want to change: - **nsec_privkey** : Your mostro private key - **relays** : List of relays you want to connect to +### Protect the admin macaroon + +The admin macaroon is a spend-capable credential: any user who can read it has full control of the LND node, including the funds escrowed in Mostro's hold invoices. The `mostro` service account created above must be able to read it, and no one else. + +If LND runs on this same VPS, grant access through the node's group instead of loosening the file (LND creates `admin.macaroon` with mode `0640`): + +```bash +usermod -aG lnd mostro +``` + +If you copy the macaroon into `/opt/mostro` instead, install it owner-readable only and hand it to the service account — do not use plain `cp`, which keeps whatever mode the source file or an existing destination happens to have: + +```bash +install -d -m 700 -o mostro -g mostro /opt/mostro/lnd +install -m 600 -o mostro -g mostro /path/to/lnd/admin.macaroon /opt/mostro/lnd/admin.macaroon +``` + +Then point `lnd_macaroon_file` at `/opt/mostro/lnd/admin.macaroon`. + ## Database The data is saved in a sqlite db file named by default `mostro.db`, this file is saved on the root directory of the project and can be change just editing the `url` var on the `[database]` section in `settings.toml` file. diff --git a/Makefile b/Makefile index 3b469258..b7e89d05 100644 --- a/Makefile +++ b/Makefile @@ -30,8 +30,8 @@ docker-build: install -m 644 "$${LND_CERT_FILE}" config/lnd/tls.cert && \ install -m 600 "$${LND_MACAROON_FILE}" config/lnd/admin.macaroon && \ echo "Wrote config/lnd/tls.cert (mode 644) and config/lnd/admin.macaroon (mode 600)" && \ - echo "Note: the container runs as uid/gid 1000. If these files are owned by another user," && \ - echo " run: sudo chown 1000:1000 docker/config/lnd/tls.cert docker/config/lnd/admin.macaroon" && \ + echo "Note: the container runs as uid/gid 1000. If they are owned by another user," && \ + echo " run: sudo chown -R 1000:1000 docker/config/lnd" && \ echo "Building docker image" && \ docker compose build diff --git a/README.md b/README.md index fb70f370..950a949e 100644 --- a/README.md +++ b/README.md @@ -495,6 +495,8 @@ This starts: - Mostro daemon (exposed via configured relays) - Local Nostr relay (port 7000 by default) +`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node. The container runs as uid/gid 1000, so run `sudo chown -R 1000:1000 docker/config/lnd` if you built as a different user. + **Stop**: `make docker-down` For detailed Docker setup, see [docker/README.md](docker/README.md). @@ -565,6 +567,8 @@ payment_retries_interval = 60 # seconds between retries **Required**: LND connection details. Mostro needs admin macaroon for hold invoice management. +**Permissions**: the admin macaroon is a spend-capable credential — anyone who can read it controls the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the user running `mostrod` (`chmod 600 /path/to/lnd/admin.macaroon`). + --- #### Nostr Configuration diff --git a/docker/ENV_VARIABLES.md b/docker/ENV_VARIABLES.md index 3d4be287..78bd8dca 100644 --- a/docker/ENV_VARIABLES.md +++ b/docker/ENV_VARIABLES.md @@ -10,7 +10,13 @@ This document describes the environment variables used by the Docker setup. - `LND_MACAROON_FILE`: Path to the LND admin macaroon file on your host system - Example: `~/.polar/networks/1/volumes/lnd/alice/data/chain/bitcoin/regtest/admin.macaroon` -These files are copied to `docker/config/lnd/` during the build process. +These files are copied to `docker/config/lnd/` during the build process: the cert with mode `0644`, the admin macaroon with mode `0600`, both inside a directory with mode `0700`. The macaroon grants full control of your LND node, so it is never left readable by other users on the host. + +The container runs as uid/gid 1000, so that directory and its contents must be owned by uid 1000 for mostrod to reach them. If you run `make docker-build` as another user, fix the ownership afterwards: + +```sh +sudo chown -R 1000:1000 docker/config/lnd +``` ## Optional Variables diff --git a/docker/README.md b/docker/README.md index ebacc628..5654f30d 100644 --- a/docker/README.md +++ b/docker/README.md @@ -62,6 +62,12 @@ To build and run the Docker container using Docker Compose, follow these steps: make docker-build ``` + The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The container runs as uid/gid 1000, so that directory and its contents must be owned by uid 1000 for mostrod to reach the macaroon. If you ran `make docker-build` as a different user, fix the ownership: + + ```sh + sudo chown -R 1000:1000 docker/config/lnd + ``` + 4. [Optional] Set the `MOSTRO_RELAY_LOCAL_PORT` environment variable to the port you want to use for the local relay (defaults to 7000 if not set). This can be set before running `make docker-up`: ```sh @@ -89,19 +95,22 @@ You can run the plain Mostro image without building locally. Use a single **conf **Option A — download the template** (from the [settings.tpl.toml](https://github.com/MostroP2P/mostro/blob/main/settings.tpl.toml) repo file): ```sh - mkdir -p ~/mostro-config/lnd + mkdir -p ~/mostro-config + install -d -m 700 ~/mostro-config/lnd curl -sL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o ~/mostro-config/settings.toml ``` **Option B — use the entrypoint default:** run the container once with an empty config dir; the entrypoint copies a default `settings.toml` (from the image, built from `settings.tpl.toml`) into `/config`. Stop the container, edit the file on the host (e.g. `~/mostro-config/settings.toml`), then start the container again. -2. Copy your LND TLS cert and macaroon into the config dir (so they appear at `/config/lnd/` in the container): +2. Copy your LND TLS cert and macaroon into the config dir (so they appear at `/config/lnd/` in the container). Use `install` rather than `cp`: `cp` keeps whatever mode the source file (or an already existing destination file) happens to have, while `install -m` sets the mode explicitly. The admin macaroon grants full control of your LND node, so it must not be readable by other users on the host: ```sh - cp /path/to/your/tls.cert ~/mostro-config/lnd/tls.cert - cp /path/to/your/admin.macaroon ~/mostro-config/lnd/admin.macaroon + install -m 644 /path/to/your/tls.cert ~/mostro-config/lnd/tls.cert + install -m 600 /path/to/your/admin.macaroon ~/mostro-config/lnd/admin.macaroon ``` + The container runs as uid/gid 1000, and mode `0600` on the macaroon inside a `0700` directory means only their owner can reach the file. If your user is not uid 1000, either hand the directory to the container's user with `sudo chown -R 1000:1000 ~/mostro-config/lnd`, or keep your own ownership and run the container as yourself by adding `--user $(id -u):$(id -g)` to the `docker run` command in step 4. + 3. Edit `~/mostro-config/settings.toml`: set `nsec_privkey`, `relays`, and for Docker set `lnd_cert_file` / `lnd_macaroon_file` to `/config/lnd/...`, `lnd_grpc_host` (e.g. `https://host.docker.internal:10009`), and `[database]` `url = "sqlite:///config/mostro.db"`. 4. Run the container. On Linux, add `--add-host=host.docker.internal:host-gateway` so the container can reach LND on the host: @@ -126,9 +135,12 @@ Steps to run the plain Mostro image on a VPS (no repo clone; image from Docker H 2. **Create a config directory** (e.g. `/opt/mostro` or `~/mostro-config`): ```sh - mkdir -p /opt/mostro/lnd + mkdir -p /opt/mostro + install -d -m 700 -o 1000 -g 1000 /opt/mostro/lnd ``` + These steps run as root, while the container runs as uid/gid 1000, so the directory holding the LND credentials is created owner-only and handed to the container's user (see step 4). + 3. **Get the settings template** into that directory as `settings.toml`: - Either run the container once with an empty config dir; the entrypoint will copy the default template to `/config/settings.toml`. Stop the container, then edit the file on the host. @@ -138,13 +150,15 @@ Steps to run the plain Mostro image on a VPS (no repo clone; image from Docker H curl -sL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o /opt/mostro/settings.toml ``` -4. **Put LND files** in the config dir so they appear at `/config/lnd/` in the container: +4. **Put LND files** in the config dir so they appear at `/config/lnd/` in the container. Use `install -m` rather than `cp`, which would keep whatever mode the source file (or an already existing destination file) happens to have: ```sh - cp /path/to/lnd/tls.cert /opt/mostro/lnd/tls.cert - cp /path/to/lnd/admin.macaroon /opt/mostro/lnd/admin.macaroon + install -m 644 /path/to/lnd/tls.cert /opt/mostro/lnd/tls.cert + install -m 600 -o 1000 -g 1000 /path/to/lnd/admin.macaroon /opt/mostro/lnd/admin.macaroon ``` + The admin macaroon grants full control of your LND node — anyone who reads it can move the funds escrowed in Mostro's hold invoices — so it is installed owner-readable only, and `-o 1000 -g 1000` hands it to the container's user (as the `0700` directory from step 2 already was). Without that ownership, mode `0600` would leave mostrod unable to read the macaroon. (`-o`/`-g` require root; as a non-root user, drop them and run the steps as the account that owns the config dir.) + (If LND is on another host, you only need the cert and macaroon copied here; point `lnd_grpc_host` at that host in step 5.) 5. **Edit `/opt/mostro/settings.toml`**: From 17a7a91819622b263c0fc13d50e42f151452cbce Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Mon, 17 Aug 2026 20:01:41 -0300 Subject: [PATCH 03/12] docs: warn at startup when LND admin macaroon is reachable by other users --- README.md | 2 +- docs/STARTUP_AND_CONFIG.md | 8 +++ src/config/mod.rs | 1 + src/config/permissions.rs | 142 +++++++++++++++++++++++++++++++++++++ src/main.rs | 13 +++- 5 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 src/config/permissions.rs diff --git a/README.md b/README.md index 950a949e..f46a184b 100644 --- a/README.md +++ b/README.md @@ -567,7 +567,7 @@ payment_retries_interval = 60 # seconds between retries **Required**: LND connection details. Mostro needs admin macaroon for hold invoice management. -**Permissions**: the admin macaroon is a spend-capable credential — anyone who can read it controls the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the user running `mostrod` (`chmod 600 /path/to/lnd/admin.macaroon`). +**Permissions**: the admin macaroon is a spend-capable credential — anyone who can read it controls the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the user running `mostrod` (`chmod 600 /path/to/lnd/admin.macaroon`). mostrod logs a warning at startup when the file is reachable by other users on the host. --- diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 2efad905..f8e51598 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -104,6 +104,14 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. **Lightning** (`src/config/types.rs:27-46`): - `lnd_cert_file` (String): Path to LND TLS certificate - `lnd_macaroon_file` (String): Path to LND macaroon auth file + - The admin macaroon is spend-capable: any account that can read it controls + the node, including the funds escrowed in Mostro's hold invoices. Keep it + readable only by the user running `mostrod` (`chmod 600`). + - At startup (Lightning mode only, right before the LND connection is opened) + `config::permissions::warn_if_other_accessible` logs a warning when the file + is reachable by users outside its owner and group. The check is advisory — + the daemon still starts — and tolerates `0640`, the mode LND itself writes + the macaroon with, so reaching it through the node's group stays supported. - `lnd_grpc_host` (String): LND gRPC endpoint URL - `invoice_expiration_window` (u32): Required invoice validity window in seconds (default: 3600) - `hold_invoice_cltv_delta` (u32): Hold invoice CLTV delta in blocks (default: 144) diff --git a/src/config/mod.rs b/src/config/mod.rs index d141f552..529cc075 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -1,5 +1,6 @@ // Mostro module for configurataion settings pub mod constants; +pub mod permissions; pub mod secret; pub mod settings; /// This module provides functionality to manage and initialize settings for the Mostro application. diff --git a/src/config/permissions.rs b/src/config/permissions.rs new file mode 100644 index 00000000..0bedbf04 --- /dev/null +++ b/src/config/permissions.rs @@ -0,0 +1,142 @@ +//! Startup checks on the filesystem permissions of the credential files +//! mostrod reads. +//! +//! Kept in its own module — rather than folded into `config::util` — because +//! these checks are about the files named by the settings, not about loading +//! the settings themselves. + +use std::path::Path; + +/// Warn when a credential file is reachable by users outside its owner and +/// group. +/// +/// The check is advisory: a node whose macaroon is `0644` still starts, it +/// just says so out loud once per boot. Refusing to start would turn a +/// hardening gap into an outage on the next upgrade for every operator who +/// already runs that way. +/// +/// Empty paths and files that cannot be stat'ed are ignored: an unset or +/// unreadable path is not a permissions problem, and the real failure is +/// reported with far more context by whoever opens the file (for the macaroon, +/// `LndConnector::new`). +pub fn warn_if_other_accessible(path: &str, label: &str) { + if path.is_empty() { + return; + } + + if let Some(mode) = other_accessible_mode(Path::new(path)) { + tracing::warn!( + "{label} ({path}) has permissions {mode:04o}: it is reachable by users other than \ + its owner and group. Restrict it to the account running mostrod with: chmod 600 {path}" + ); + } +} + +/// The file's permission bits when the "other" class has any of them set, +/// `None` otherwise. +/// +/// Group access is deliberately tolerated: LND itself creates +/// `admin.macaroon` with mode `0640`, and granting a service account access +/// through the node's group is a legitimate deployment, not a finding. +/// +/// `metadata` follows symlinks on purpose — pointing `lnd_macaroon_file` at a +/// link is common, and what matters is the mode of the file that is actually +/// read. +#[cfg(unix)] +fn other_accessible_mode(path: &Path) -> Option { + use std::os::unix::fs::PermissionsExt; + + let mode = std::fs::metadata(path).ok()?.permissions().mode() & 0o777; + (mode & 0o007 != 0).then_some(mode) +} + +/// Non-Unix platforms have no POSIX permission bits to inspect. +#[cfg(not(unix))] +fn other_accessible_mode(_path: &Path) -> Option { + None +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + use std::path::PathBuf; + + fn temp_dir(tag: &str) -> PathBuf { + let dir = + std::env::temp_dir().join(format!("mostro-permissions-{tag}-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + dir + } + + fn macaroon_with_mode(tag: &str, mode: u32) -> PathBuf { + let path = temp_dir(tag).join("admin.macaroon"); + std::fs::write(&path, b"macaroon").expect("write macaroon"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)) + .expect("set permissions"); + path + } + + #[test] + fn owner_only_is_accepted() { + let path = macaroon_with_mode("owner-only", 0o600); + assert_eq!(other_accessible_mode(&path), None); + } + + #[test] + fn group_readable_is_accepted() { + // 0640 is the mode LND writes admin.macaroon with, and reaching it + // through the node's group is a supported setup. + let path = macaroon_with_mode("group-read", 0o640); + assert_eq!(other_accessible_mode(&path), None); + } + + #[test] + fn world_readable_is_reported() { + let path = macaroon_with_mode("world-read", 0o644); + assert_eq!(other_accessible_mode(&path), Some(0o644)); + } + + #[test] + fn other_read_without_group_read_is_reported() { + let path = macaroon_with_mode("other-read", 0o604); + assert_eq!(other_accessible_mode(&path), Some(0o604)); + } + + #[test] + fn world_writable_is_reported() { + // Not a disclosure by itself, but a local account that can replace the + // credential mostrod authenticates with is the same class of problem. + let path = macaroon_with_mode("world-write", 0o602); + assert_eq!(other_accessible_mode(&path), Some(0o602)); + } + + #[test] + fn symlink_reports_the_target_mode() { + let dir = temp_dir("symlink"); + let target = dir.join("real.macaroon"); + std::fs::write(&target, b"macaroon").expect("write macaroon"); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)) + .expect("set permissions"); + + let link = dir.join("linked.macaroon"); + let _ = std::fs::remove_file(&link); + std::os::unix::fs::symlink(&target, &link).expect("create symlink"); + + assert_eq!(other_accessible_mode(&link), Some(0o644)); + } + + #[test] + fn missing_file_is_ignored() { + assert_eq!( + other_accessible_mode(Path::new("/definitely/not/here.macaroon")), + None + ); + } + + #[test] + fn empty_and_missing_paths_do_not_panic() { + warn_if_other_accessible("", "LND admin macaroon"); + warn_if_other_accessible("/definitely/not/here.macaroon", "LND admin macaroon"); + } +} diff --git a/src/main.rs b/src/main.rs index 6a636497..fcd0aab2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,7 +25,8 @@ use crate::app::context::AppContext; use crate::app::{run, run_cashu}; use crate::cli::settings_init; use crate::config::{ - get_db_pool, Settings, DB_POOL, LN_STATUS, MESSAGE_QUEUES, MOSTRO_CONFIG, NOSTR_CLIENT, + get_db_pool, permissions, Settings, DB_POOL, LN_STATUS, MESSAGE_QUEUES, MOSTRO_CONFIG, + NOSTR_CLIENT, }; use crate::db::find_held_invoices; use crate::lightning::LnStatus; @@ -221,6 +222,16 @@ async fn main() -> Result<()> { return run_cashu(ctx).await; } + // The admin macaroon is spend-capable: whoever reads it controls the node, + // including the funds escrowed in the hold invoices mostrod manages. Say so + // once per boot when the file is left reachable by other local accounts — + // the documented deployment flows copy it around, and a bad mode is + // otherwise invisible until it is abused. + permissions::warn_if_other_accessible( + &Settings::get_ln().lnd_macaroon_file, + "LND admin macaroon", + ); + let mut ln_client = LndConnector::new().await?; let ln_status = ln_client.get_node_info().await?; let ln_status = LnStatus::from_get_info_response(ln_status); From 519c5c763586af0a2a4a64155d5a00d2823e0e16 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Mon, 17 Aug 2026 20:24:40 -0300 Subject: [PATCH 04/12] fix(docker): run container as host user instead of requiring chown on LND credentials --- .dockerignore | 9 +++++++++ Makefile | 4 ++-- README.md | 2 +- docker/ENV_VARIABLES.md | 10 +++++----- docker/README.md | 14 +++++++++----- docker/compose.yml | 6 ++++++ 6 files changed, 32 insertions(+), 13 deletions(-) create mode 100644 .dockerignore diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 00000000..9817eaa7 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,9 @@ +# The Docker build context is the repository root (docker/compose.yml sets +# `context: ..`) and both Dockerfiles pull it into the builder stage with +# `COPY . .`. `make docker-build` stages the LND cert and admin macaroon in +# docker/config/lnd/ right before `docker compose build`, so without this entry +# a spend-capable credential would be shipped into the build context and left +# in the builder layer and its cache. .gitignore already keeps it out of git, +# but Docker does not read .gitignore. +docker/config/ +docker/.env diff --git a/Makefile b/Makefile index b7e89d05..41dfb444 100644 --- a/Makefile +++ b/Makefile @@ -30,8 +30,8 @@ docker-build: install -m 644 "$${LND_CERT_FILE}" config/lnd/tls.cert && \ install -m 600 "$${LND_MACAROON_FILE}" config/lnd/admin.macaroon && \ echo "Wrote config/lnd/tls.cert (mode 644) and config/lnd/admin.macaroon (mode 600)" && \ - echo "Note: the container runs as uid/gid 1000. If they are owned by another user," && \ - echo " run: sudo chown -R 1000:1000 docker/config/lnd" && \ + echo "Note: the container runs as uid/gid 1000 by default. If your user is not uid 1000," && \ + echo ' export MOSTRO_CONTAINER_USER=$$(id -u):$$(id -g) before make docker-up' && \ echo "Building docker image" && \ docker compose build diff --git a/README.md b/README.md index f46a184b..6a664896 100644 --- a/README.md +++ b/README.md @@ -495,7 +495,7 @@ This starts: - Mostro daemon (exposed via configured relays) - Local Nostr relay (port 7000 by default) -`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node. The container runs as uid/gid 1000, so run `sudo chown -R 1000:1000 docker/config/lnd` if you built as a different user. +`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node. The container runs as uid/gid 1000 by default; if your user is not uid 1000, `export MOSTRO_CONTAINER_USER=$(id -u):$(id -g)` so it runs as you and can read the macaroon. **Stop**: `make docker-down` diff --git a/docker/ENV_VARIABLES.md b/docker/ENV_VARIABLES.md index 78bd8dca..fb3fabd3 100644 --- a/docker/ENV_VARIABLES.md +++ b/docker/ENV_VARIABLES.md @@ -12,11 +12,7 @@ This document describes the environment variables used by the Docker setup. These files are copied to `docker/config/lnd/` during the build process: the cert with mode `0644`, the admin macaroon with mode `0600`, both inside a directory with mode `0700`. The macaroon grants full control of your LND node, so it is never left readable by other users on the host. -The container runs as uid/gid 1000, so that directory and its contents must be owned by uid 1000 for mostrod to reach them. If you run `make docker-build` as another user, fix the ownership afterwards: - -```sh -sudo chown -R 1000:1000 docker/config/lnd -``` +The copies belong to the user that ran the command, and the container runs as uid/gid 1000 by default. If your user is not uid 1000, run the container as yourself with the optional variable below rather than handing the config directory over to uid 1000. ## Optional Variables @@ -24,6 +20,10 @@ sudo chown -R 1000:1000 docker/config/lnd - Used in `compose.yml` for port mapping - Example: `export MOSTRO_RELAY_LOCAL_PORT=7000` +- `MOSTRO_CONTAINER_USER`: uid/gid the `mostro` container runs as (defaults to `1000:1000`, the image's `mostrouser`) + - Set it when your host user is not uid 1000, so the container can read the `0600` macaroon and write `mostro.db` in the mounted config directory + - Example: `export MOSTRO_CONTAINER_USER=$(id -u):$(id -g)` + - `MOSTRO_DB_PASSWORD`: Not used (database encryption was removed). Kept in `compose.yml` for backward compatibility; can be omitted or left empty. ## Usage Examples diff --git a/docker/README.md b/docker/README.md index 5654f30d..71aaa49a 100644 --- a/docker/README.md +++ b/docker/README.md @@ -62,12 +62,16 @@ To build and run the Docker container using Docker Compose, follow these steps: make docker-build ``` - The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The container runs as uid/gid 1000, so that directory and its contents must be owned by uid 1000 for mostrod to reach the macaroon. If you ran `make docker-build` as a different user, fix the ownership: + The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. Both belong to the user that ran the command. + + The container runs as uid/gid 1000 by default, which matches the first user account on most hosts. If yours is not uid 1000, run the container as yourself instead of handing the config directory over — the daemon also writes `mostro.db` into it: ```sh - sudo chown -R 1000:1000 docker/config/lnd + export MOSTRO_CONTAINER_USER=$(id -u):$(id -g) ``` + `compose.yml` reads that variable, so export it in the same shell you run `make docker-up` from. + 4. [Optional] Set the `MOSTRO_RELAY_LOCAL_PORT` environment variable to the port you want to use for the local relay (defaults to 7000 if not set). This can be set before running `make docker-up`: ```sh @@ -109,7 +113,7 @@ You can run the plain Mostro image without building locally. Use a single **conf install -m 600 /path/to/your/admin.macaroon ~/mostro-config/lnd/admin.macaroon ``` - The container runs as uid/gid 1000, and mode `0600` on the macaroon inside a `0700` directory means only their owner can reach the file. If your user is not uid 1000, either hand the directory to the container's user with `sudo chown -R 1000:1000 ~/mostro-config/lnd`, or keep your own ownership and run the container as yourself by adding `--user $(id -u):$(id -g)` to the `docker run` command in step 4. + Mode `0600` on the macaroon inside a `0700` directory means only their owner can reach the file, and the container runs as uid/gid 1000 by default. If your user is not uid 1000, run the container as yourself by adding `--user $(id -u):$(id -g)` to the `docker run` command in step 4 — that also lets it write `mostro.db` into your config directory. 3. Edit `~/mostro-config/settings.toml`: set `nsec_privkey`, `relays`, and for Docker set `lnd_cert_file` / `lnd_macaroon_file` to `/config/lnd/...`, `lnd_grpc_host` (e.g. `https://host.docker.internal:10009`), and `[database]` `url = "sqlite:///config/mostro.db"`. @@ -135,11 +139,11 @@ Steps to run the plain Mostro image on a VPS (no repo clone; image from Docker H 2. **Create a config directory** (e.g. `/opt/mostro` or `~/mostro-config`): ```sh - mkdir -p /opt/mostro + install -d -o 1000 -g 1000 /opt/mostro install -d -m 700 -o 1000 -g 1000 /opt/mostro/lnd ``` - These steps run as root, while the container runs as uid/gid 1000, so the directory holding the LND credentials is created owner-only and handed to the container's user (see step 4). + These steps run as root, while the container runs as uid/gid 1000, so both directories are handed to the container's user: it needs to write `mostro.db` into the config directory, and the `lnd` subdirectory is created owner-only because of what goes in it (step 4). 3. **Get the settings template** into that directory as `settings.toml`: diff --git a/docker/compose.yml b/docker/compose.yml index 7bb9f1d0..deb70a53 100644 --- a/docker/compose.yml +++ b/docker/compose.yml @@ -5,6 +5,12 @@ services: dockerfile: docker/Dockerfile volumes: - ./config:/config # settings.toml and mostro.db + # The image's mostrouser is uid/gid 1000, so the default keeps the previous + # behaviour. Hosts whose user is not uid 1000 export + # MOSTRO_CONTAINER_USER=$(id -u):$(id -g) instead of handing ./config over + # to uid 1000 — the LND macaroon is installed 0600, and the daemon also + # needs to write mostro.db in the same directory. + user: "${MOSTRO_CONTAINER_USER:-1000:1000}" platform: linux/amd64 networks: - default From cad7930dbcfdcabea4c8ee4ce769b65d4f4c8169 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Mon, 17 Aug 2026 20:27:09 -0300 Subject: [PATCH 05/12] docs: clarify admin macaroon permission guidance to preserve group access --- INSTALL.md | 2 +- README.md | 2 +- docs/STARTUP_AND_CONFIG.md | 12 ++++++++---- src/config/permissions.rs | 17 +++++++++++++---- 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/INSTALL.md b/INSTALL.md index 63a70783..591a24c5 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -110,7 +110,7 @@ Here some parameters you might want to change: ### Protect the admin macaroon -The admin macaroon is a spend-capable credential: any user who can read it has full control of the LND node, including the funds escrowed in Mostro's hold invoices. The `mostro` service account created above must be able to read it, and no one else. +The admin macaroon is a spend-capable credential: any user who can read it has full control of the LND node, including the funds escrowed in Mostro's hold invoices. Access should reach no further than the `mostro` service account created above and the LND account the node already runs as. If LND runs on this same VPS, grant access through the node's group instead of loosening the file (LND creates `admin.macaroon` with mode `0640`): diff --git a/README.md b/README.md index 6a664896..bf60d479 100644 --- a/README.md +++ b/README.md @@ -567,7 +567,7 @@ payment_retries_interval = 60 # seconds between retries **Required**: LND connection details. Mostro needs admin macaroon for hold invoice management. -**Permissions**: the admin macaroon is a spend-capable credential — anyone who can read it controls the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the user running `mostrod` (`chmod 600 /path/to/lnd/admin.macaroon`). mostrod logs a warning at startup when the file is reachable by other users on the host. +**Permissions**: the admin macaroon is a spend-capable credential — anyone who can read it controls the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the account running `mostrod`, or by that account and LND's group (`chmod 600` for a private copy, `chmod o=` to keep group access). mostrod logs a warning at startup when the file's `other` permission bits are set. --- diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index f8e51598..c305212c 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -108,10 +108,14 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the user running `mostrod` (`chmod 600`). - At startup (Lightning mode only, right before the LND connection is opened) - `config::permissions::warn_if_other_accessible` logs a warning when the file - is reachable by users outside its owner and group. The check is advisory — - the daemon still starts — and tolerates `0640`, the mode LND itself writes - the macaroon with, so reaching it through the node's group stays supported. + `config::permissions::warn_if_other_accessible` logs a warning when the + file's `other` permission bits are set, and suggests `chmod o=`. The check + is advisory — the daemon still starts — and tolerates `0640`, the mode LND + itself writes the macaroon with, so reading it through the node's group + stays supported. + - Only the mode bits are inspected: a POSIX ACL can grant a named user access + without setting them, so a quiet startup is not proof that no other account + can read the file. - `lnd_grpc_host` (String): LND gRPC endpoint URL - `invoice_expiration_window` (u32): Required invoice validity window in seconds (default: 3600) - `hold_invoice_cltv_delta` (u32): Hold invoice CLTV delta in blocks (default: 144) diff --git a/src/config/permissions.rs b/src/config/permissions.rs index 0bedbf04..92ff73cc 100644 --- a/src/config/permissions.rs +++ b/src/config/permissions.rs @@ -7,8 +7,8 @@ use std::path::Path; -/// Warn when a credential file is reachable by users outside its owner and -/// group. +/// Warn when a credential file has any of its "other" permission bits set, +/// which puts it within reach of every account on the host. /// /// The check is advisory: a node whose macaroon is `0644` still starts, it /// just says so out loud once per boot. Refusing to start would turn a @@ -25,9 +25,13 @@ pub fn warn_if_other_accessible(path: &str, label: &str) { } if let Some(mode) = other_accessible_mode(Path::new(path)) { + // `chmod o=` rather than `chmod 600`: the file may legitimately be + // owned by the LND account and read by mostrod through the node's + // group, and following advice that drops the group bits would leave + // the daemon unable to authenticate on its next restart. tracing::warn!( - "{label} ({path}) has permissions {mode:04o}: it is reachable by users other than \ - its owner and group. Restrict it to the account running mostrod with: chmod 600 {path}" + "{label} ({path}) has permissions {mode:04o}: its \"other\" bits are set, so every \ + account on this host can reach it. Clear them with: chmod o= {path}" ); } } @@ -39,6 +43,11 @@ pub fn warn_if_other_accessible(path: &str, label: &str) { /// `admin.macaroon` with mode `0640`, and granting a service account access /// through the node's group is a legitimate deployment, not a finding. /// +/// Only the mode bits are read. A POSIX ACL can widen access without touching +/// them, so a quiet startup means "the mode bits are sane", not "no other +/// account can read this file" — the check is a cheap guard against the +/// documented copy-it-into-place flows, not an audit. +/// /// `metadata` follows symlinks on purpose — pointing `lnd_macaroon_file` at a /// link is common, and what matters is the mode of the file that is actually /// read. From 9ba71b449ca9a76c4765aba0171188dc1acb8f24 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Tue, 18 Aug 2026 21:33:03 -0300 Subject: [PATCH 06/12] docs: restrict settings directory and file to owner-only permissions (0700/0600) --- AGENTS.md | 2 +- INSTALL.md | 7 +- Makefile | 5 +- README.md | 18 +++-- docker/ENV_VARIABLES.md | 2 +- docker/README.md | 26 ++++--- docker/start.sh | 6 +- docs/STARTUP_AND_CONFIG.md | 18 ++++- src/config/util.rs | 154 ++++++++++++++++++++++++++++++++++++- 9 files changed, 207 insertions(+), 31 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 61b4f71c..19823e07 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,6 +43,6 @@ - Add a language specifier to every fenced code block. Static analysis (markdownlint MD040) flags blocks without a language identifier. Example: ` ```flutter test ` instead of bare ` ``` `. ## Security & Configuration Tips -- Do not commit populated `settings.toml`. Copy from `settings.tpl.toml` to `~/.mostro/settings.toml` for local runs. +- Do not commit populated `settings.toml`. Install it from `settings.tpl.toml` with `install -d -m 700 ~/.mostro && install -m 600 settings.tpl.toml ~/.mostro/settings.toml` for local runs: the file carries `nsec_privkey` and the directory also holds `mostro.db`. - Protect LND credentials before `make docker-build`. The admin macaroon is spend-capable: copy it with `install -m 600` (never plain `cp`, which inherits the source or destination mode) and keep its directory at `0700`. - Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed. diff --git a/INSTALL.md b/INSTALL.md index 591a24c5..7c9b8ce4 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -92,10 +92,11 @@ adduser --disabled-login mostro # keep pressing enter until it ends cd /opt/mostro ``` -Create a new settings file from `/opt/mostro/mostro/settings.tpl.toml` and save it to `/opt/mostro`: +Create a new settings file from `/opt/mostro/mostro/settings.tpl.toml` and save it to `/opt/mostro`. `/opt/mostro` holds `settings.toml`, whose `nsec_privkey` is the daemon's identity, and the `mostro.db` created later, so both the directory and the file are restricted to the service account — `install -m` rather than `cp`, which keeps whatever mode the template happens to have: ```bash -cp /opt/mostro/mostro/settings.tpl.toml /opt/mostro/settings.toml +install -d -m 700 -o mostro -g mostro /opt/mostro +install -m 600 -o mostro -g mostro /opt/mostro/mostro/settings.tpl.toml /opt/mostro/settings.toml ``` Update the file `/opt/mostro/settings.toml` with your favourite editor. @@ -144,7 +145,7 @@ Check the DB files are there ```bash ls -al /opt/mostro -drwxrwxr-x root root 4.0 KB Fri Jun 14 15:52:07 2024 . +drwx------ mostro mostro 4.0 KB Fri Jun 14 15:52:07 2024 . drwxr-x--- root root 4.0 KB Sat Jun 15 15:50:32 2024 .. .rw-r--r-- root root 52 KB Fri May 31 16:35:34 2024 mostro.db .rw-r--r-- root root 32 KB Sat Jun 15 15:28:23 2024 mostro.db-shm diff --git a/Makefile b/Makefile index 41dfb444..490971c0 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ VERSION := $(shell grep "^version = " Cargo.toml | sed "s/version = \"\(.*\)\"/\ docker-build: @set -o pipefail; \ cd docker && \ - install -d -m 700 config/lnd && \ + install -d -m 700 config config/lnd && \ echo "Checking LND files..." && \ echo "LND_CERT_FILE=$${LND_CERT_FILE}" && \ echo "LND_MACAROON_FILE=$${LND_MACAROON_FILE}" && \ @@ -30,6 +30,7 @@ docker-build: install -m 644 "$${LND_CERT_FILE}" config/lnd/tls.cert && \ install -m 600 "$${LND_MACAROON_FILE}" config/lnd/admin.macaroon && \ echo "Wrote config/lnd/tls.cert (mode 644) and config/lnd/admin.macaroon (mode 600)" && \ + echo "config and config/lnd are mode 700: settings.toml holds nsec_privkey and mostro.db lands there too" && \ echo "Note: the container runs as uid/gid 1000 by default. If your user is not uid 1000," && \ echo ' export MOSTRO_CONTAINER_USER=$$(id -u):$$(id -g) before make docker-up' && \ echo "Building docker image" && \ @@ -39,6 +40,7 @@ docker-up: @set -o pipefail; \ cd docker && \ echo "Copying Nostr relay config" && \ + install -d -m 700 config && \ mkdir -p config/relay && \ cp -v ./relay_config.toml config/relay/config.toml && \ echo "Starting services" && \ @@ -48,6 +50,7 @@ docker-relay-up: @set -o pipefail; \ cd docker && \ echo "Copying Nostr relay config" && \ + install -d -m 700 config && \ mkdir -p config/relay && \ cp -v ./relay_config.toml config/relay/config.toml && \ echo "Starting Nostr relay" && \ diff --git a/README.md b/README.md index bf60d479..5fa8a478 100644 --- a/README.md +++ b/README.md @@ -443,9 +443,10 @@ cargo build --release # Install to system sudo install target/release/mostrod /usr/local/bin -# Setup configuration -mkdir -p ~/.mostro -cp settings.tpl.toml ~/.mostro/settings.toml +# Setup configuration (0700/0600: settings.toml holds nsec_privkey, and +# mostro.db lands in the same directory) +install -d -m 700 ~/.mostro +install -m 600 settings.tpl.toml ~/.mostro/settings.toml # Edit ~/.mostro/settings.toml (see Configuration section) # Initialize database (optional — mostrod also migrates on first connect) @@ -479,9 +480,10 @@ Best for: Local testing, development environments, quick experiments git clone https://github.com/MostroP2P/mostro.git cd mostro -# Setup configuration -mkdir -p docker/config -cp settings.tpl.toml docker/config/settings.toml +# Setup configuration (0700/0600: the config dir ends up holding nsec_privkey, +# the LND credentials and mostro.db) +install -d -m 700 docker/config +install -m 600 settings.tpl.toml docker/config/settings.toml # Edit docker/config/settings.toml # Build and run (provide LND paths as environment variables) @@ -495,7 +497,7 @@ This starts: - Mostro daemon (exposed via configured relays) - Local Nostr relay (port 7000 by default) -`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node. The container runs as uid/gid 1000 by default; if your user is not uid 1000, `export MOSTRO_CONTAINER_USER=$(id -u):$(id -g)` so it runs as you and can read the macaroon. +`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node, and sets `docker/config` and `docker/config/lnd` to mode `0700` — the same directory holds `settings.toml` and `mostro.db`. The container runs as uid/gid 1000 by default; if your user is not uid 1000, `export MOSTRO_CONTAINER_USER=$(id -u):$(id -g)` so it runs as you and can read those files. **Stop**: `make docker-down` @@ -594,6 +596,8 @@ rana --vanity mostro **Important**: Never reuse keys between Mostro instances. Each daemon needs a unique identity. +**Permissions**: `settings.toml` holds `nsec_privkey` in plaintext unless you move it to the environment (see below), and `mostro.db` sits in the same directory. Keep both owner-only: `chmod 700` on the settings directory and `chmod 600` on `settings.toml`. mostrod already creates them that way when it has to — both through the interactive setup wizard and through the template copy it makes on a non-interactive first run — but a directory or file you create yourself with `mkdir`, `cp` or `curl` inherits your umask instead, so use `install -d -m 700` and `install -m 600`. + ##### Providing the nsec via environment variable For better separation of secrets from config, Mostro can read the nsec from the diff --git a/docker/ENV_VARIABLES.md b/docker/ENV_VARIABLES.md index fb3fabd3..bc62dabe 100644 --- a/docker/ENV_VARIABLES.md +++ b/docker/ENV_VARIABLES.md @@ -10,7 +10,7 @@ This document describes the environment variables used by the Docker setup. - `LND_MACAROON_FILE`: Path to the LND admin macaroon file on your host system - Example: `~/.polar/networks/1/volumes/lnd/alice/data/chain/bitcoin/regtest/admin.macaroon` -These files are copied to `docker/config/lnd/` during the build process: the cert with mode `0644`, the admin macaroon with mode `0600`, both inside a directory with mode `0700`. The macaroon grants full control of your LND node, so it is never left readable by other users on the host. +These files are copied to `docker/config/lnd/` during the build process: the cert with mode `0644`, the admin macaroon with mode `0600`, both inside a directory with mode `0700`. The macaroon grants full control of your LND node, so it is never left readable by other users on the host. The `docker/config` root is set to mode `0700` as well, since `settings.toml` (which carries `nsec_privkey`) and `mostro.db` live beside the credentials. The copies belong to the user that ran the command, and the container runs as uid/gid 1000 by default. If your user is not uid 1000, run the container as yourself with the optional variable below rather than handing the config directory over to uid 1000. diff --git a/docker/README.md b/docker/README.md index 71aaa49a..27334cd1 100644 --- a/docker/README.md +++ b/docker/README.md @@ -33,10 +33,12 @@ To build and run the Docker container using Docker Compose, follow these steps: ```sh cd docker - mkdir -p config - cp ../settings.tpl.toml config/settings.toml + install -d -m 700 config + install -m 600 ../settings.tpl.toml config/settings.toml ``` + Mode `0700` on `config` and `0600` on `settings.toml` because that directory ends up holding every secret this deployment has: `nsec_privkey` in `settings.toml`, the LND credentials in `config/lnd/`, and the `mostro.db` the daemon writes. `install -d -m 700` also tightens a `config` directory an earlier `mkdir -p` left at `0755`. + _Don't forget to edit `lnd_grpc_host`, `nsec_privkey` and `relays` fields in the `config/settings.toml` file. Note that paths in `settings.toml` refer to paths **inside the container**, so use `/config/lnd/tls.cert` and `/config/lnd/admin.macaroon` for the LND certificate and macaroon files (these will be copied there by `make docker-build`)._ 3. Build the docker image. You need to provide the `LND_CERT_FILE` and `LND_MACAROON_FILE` environment variables with the paths to your LND TLS certificate and macaroon files. These files will be copied to the `docker/config/lnd` directory by the `make docker-build` command. The build process will validate that these variables are set and that the files exist before proceeding. @@ -62,7 +64,7 @@ To build and run the Docker container using Docker Compose, follow these steps: make docker-build ``` - The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. Both belong to the user that ran the command. + The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The `config` root itself is set to `0700` as well, since `settings.toml` and `mostro.db` sit next to the credentials. All of them belong to the user that ran the command. The container runs as uid/gid 1000 by default, which matches the first user account on most hosts. If yours is not uid 1000, run the container as yourself instead of handing the config directory over — the daemon also writes `mostro.db` into it: @@ -99,12 +101,14 @@ You can run the plain Mostro image without building locally. Use a single **conf **Option A — download the template** (from the [settings.tpl.toml](https://github.com/MostroP2P/mostro/blob/main/settings.tpl.toml) repo file): ```sh - mkdir -p ~/mostro-config - install -d -m 700 ~/mostro-config/lnd + install -d -m 700 ~/mostro-config ~/mostro-config/lnd curl -sL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o ~/mostro-config/settings.toml + chmod 600 ~/mostro-config/settings.toml ``` - **Option B — use the entrypoint default:** run the container once with an empty config dir; the entrypoint copies a default `settings.toml` (from the image, built from `settings.tpl.toml`) into `/config`. Stop the container, edit the file on the host (e.g. `~/mostro-config/settings.toml`), then start the container again. + The config root is `0700` and `settings.toml` is `0600` because both `nsec_privkey` and, later, `mostro.db` live there. `curl` creates the file under your umask, typically `0644`, so the mode has to be set afterwards. + + **Option B — use the entrypoint default:** create the config dir with `install -d -m 700 ~/mostro-config ~/mostro-config/lnd`, then run the container once against it; the entrypoint installs a default `settings.toml` (from the image, built from `settings.tpl.toml`) into `/config` with mode `0600`. Stop the container, edit the file on the host (e.g. `~/mostro-config/settings.toml`), then start the container again. 2. Copy your LND TLS cert and macaroon into the config dir (so they appear at `/config/lnd/` in the container). Use `install` rather than `cp`: `cp` keeps whatever mode the source file (or an already existing destination file) happens to have, while `install -m` sets the mode explicitly. The admin macaroon grants full control of your LND node, so it must not be readable by other users on the host: @@ -139,21 +143,25 @@ Steps to run the plain Mostro image on a VPS (no repo clone; image from Docker H 2. **Create a config directory** (e.g. `/opt/mostro` or `~/mostro-config`): ```sh - install -d -o 1000 -g 1000 /opt/mostro + install -d -m 700 -o 1000 -g 1000 /opt/mostro install -d -m 700 -o 1000 -g 1000 /opt/mostro/lnd ``` - These steps run as root, while the container runs as uid/gid 1000, so both directories are handed to the container's user: it needs to write `mostro.db` into the config directory, and the `lnd` subdirectory is created owner-only because of what goes in it (step 4). + These steps run as root, while the container runs as uid/gid 1000, so both directories are handed to the container's user: it needs to write `mostro.db` into the config directory. Both are owner-only because of what goes in them — `nsec_privkey` in `settings.toml` and the database in the config root, the LND credentials in `lnd` (step 4). 3. **Get the settings template** into that directory as `settings.toml`: - - Either run the container once with an empty config dir; the entrypoint will copy the default template to `/config/settings.toml`. Stop the container, then edit the file on the host. + - Either run the container once with an empty config dir; the entrypoint installs the default template at `/config/settings.toml` with mode `0600`. Stop the container, then edit the file on the host. - Or download the template and copy it: ```sh curl -sL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o /opt/mostro/settings.toml + chmod 600 /opt/mostro/settings.toml + chown 1000:1000 /opt/mostro/settings.toml ``` + `curl` writes the file under root's umask, typically `0644` and root-owned, so the mode and the owner have to be set afterwards: the file receives `nsec_privkey` in step 5, and the container reads it as uid/gid 1000. + 4. **Put LND files** in the config dir so they appear at `/config/lnd/` in the container. Use `install -m` rather than `cp`, which would keep whatever mode the source file (or an already existing destination file) happens to have: ```sh diff --git a/docker/start.sh b/docker/start.sh index c09cdde4..360c7064 100644 --- a/docker/start.sh +++ b/docker/start.sh @@ -1,10 +1,12 @@ #!/bin/sh set -e -# Check if the settings.toml file exists, if not, create a new one +# Check if the settings.toml file exists, if not, create a new one. +# `install -m 600` rather than `cp`: the file receives nsec_privkey once edited, +# and `cp` would keep whatever mode the template in the image happens to have. if [ ! -f /config/settings.toml ]; then echo "settings.toml not found, creating a new one from template (default)." - cp /mostro/settings.toml /config/settings.toml + install -m 600 /mostro/settings.toml /config/settings.toml fi # Run application (Mostro creates mostro.db at startup if missing) diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index c305212c..5fa5f5fb 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -29,9 +29,10 @@ Before settings initialization, the daemon performs (see `src/main.rs`): ### Settings Initialization Details **Directory setup**: -- Creates `~/.mostro/` directory if not exists +- Creates `~/.mostro/` directory if not exists, owner-only (mode `0700` on Unix) - Checks for existing `~/.mostro/settings.toml` -- If missing: copies `settings.tpl.toml` to `~/.mostro/settings.toml` +- If missing: copies `settings.tpl.toml` to `~/.mostro/settings.toml`, owner-only + (mode `0600` on Unix), since the file receives `nsec_privkey` once edited - On first run after creating the file, the process exits so the user can edit `settings.toml`, then restart Mostro. - Overrides database URL: `~/.mostro/mostro.db` **Settings loading**: @@ -108,7 +109,7 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the user running `mostrod` (`chmod 600`). - At startup (Lightning mode only, right before the LND connection is opened) - `config::permissions::warn_if_other_accessible` logs a warning when the + `src/config/permissions.rs`, `fn warn_if_other_accessible` logs a warning when the file's `other` permission bits are set, and suggests `chmod o=`. The check is advisory — the daemon still starts — and tolerates `0640`, the mode LND itself writes the macaroon with, so reading it through the node's group @@ -199,3 +200,14 @@ There is **no** database password or separate global for SQLite; the daemon open ## Security - Do not commit populated `settings.toml`. - Keep templates in `settings.tpl.toml`; place runtime config at `~/.mostro/settings.toml`. +- `settings.toml` carries `nsec_privkey` in plaintext unless it is supplied through + `MOSTRO_NSEC_PRIVKEY`, and the settings directory also holds `mostro.db` and, in the + Docker flows, the LND credentials under `lnd/`. Keep the directory at mode `0700` and + the file at `0600`. +- Both paths that create them do so already: `src/config/util.rs`, `fn create_settings_dir` + and `fn write_owner_only` for the non-interactive template copy, and + `src/config/wizard.rs`, `fn run_setup_wizard` for the interactive one. A directory or + file created outside the daemon — `mkdir`, `cp`, `curl` — inherits the umask instead, so + the deployment guides use `install -d -m 700` and `install -m 600`. +- An existing settings directory is left as it is, so a deliberately group-readable + deployment keeps working. diff --git a/src/config/util.rs b/src/config/util.rs index 635a5ce3..36ff3813 100644 --- a/src/config/util.rs +++ b/src/config/util.rs @@ -129,6 +129,65 @@ fn validate_cashu_settings( Ok(()) } +/// Create the settings directory owner-only (`0700` on Unix). +/// +/// The directory holds `settings.toml` with a plaintext `nsec_privkey`, the +/// `mostro.db` database and, in the Docker flows, the LND credentials under +/// `lnd/`. `create_dir_all` would apply the process umask instead, which on a +/// typical host leaves the directory at `0755`. +/// +/// Only directories this call creates are affected: an operator who already set +/// up the directory with deliberate group access keeps it. +fn create_settings_dir(settings_dir: &std::path::Path) -> Result<(), MostroError> { + let mut builder = fs::DirBuilder::new(); + builder.recursive(true); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } + builder + .create(settings_dir) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string()))) +} + +/// Write `contents` to `path` with owner-only permissions (`0600` on Unix). +/// +/// `OpenOptionsExt::mode` only applies when the file is created, so the mode is +/// also set explicitly afterwards: the caller checks for the file's absence +/// first, and another process could create it in between. +fn write_owner_only(path: &std::path::Path, contents: &[u8]) -> Result<(), MostroError> { + use std::io::Write; + + #[cfg(unix)] + let file = { + use std::os::unix::fs::OpenOptionsExt; + fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path) + }; + #[cfg(not(unix))] + let file = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .open(path); + let mut file = file.map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + file.set_permissions(fs::Permissions::from_mode(0o600)) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + } + + file.write_all(contents) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string()))) +} + /// Initialize the default settings directory and create a settings file from the template if it doesn't exist. /// Checks if the directory already exists, and if not, creates it and writes the template file. /// If a custom config path is provided, it uses that instead of the default `~/.mostro` directory. @@ -147,8 +206,7 @@ pub fn init_configuration_file(config_path: Option) -> Result<(), Mostro // Check if /.mostro directory exists if !settings_dir.exists() { - std::fs::create_dir_all(&settings_dir) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + create_settings_dir(&settings_dir)?; } // Load `/.env` so MOSTRO_NSEC_PRIVKEY (and any future env @@ -163,8 +221,7 @@ pub fn init_configuration_file(config_path: Option) -> Result<(), Mostro wizard::run_setup_menu(&settings_dir, &config_file_path)? } else { // Non-interactive (Docker, CI, systemd): copy template and exit - std::fs::write(&config_file_path, include_bytes!("../../settings.tpl.toml")) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + write_owner_only(&config_file_path, include_bytes!("../../settings.tpl.toml"))?; println!( "Created settings file from template at {} - Edit it to configure your Mostro instance", config_file_path.display() @@ -582,3 +639,92 @@ mod init_configuration_file_tests { assert!(result.is_err()); } } + +#[cfg(all(test, unix))] +mod owner_only_tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + + fn temp_root(tag: &str) -> std::path::PathBuf { + let dir = + std::env::temp_dir().join(format!("mostro-owner-only-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create temp dir"); + dir + } + + fn mode_of(path: &std::path::Path) -> u32 { + std::fs::metadata(path) + .expect("metadata") + .permissions() + .mode() + & 0o777 + } + + #[test] + fn settings_dir_is_created_owner_only() { + let root = temp_root("dir"); + let settings_dir = root.join(".mostro"); + create_settings_dir(&settings_dir).expect("create settings dir"); + assert_eq!(mode_of(&settings_dir), 0o700); + } + + #[test] + fn settings_dir_creation_is_recursive_and_owner_only() { + let root = temp_root("dir-nested"); + let settings_dir = root.join("nested").join(".mostro"); + create_settings_dir(&settings_dir).expect("create nested settings dir"); + assert_eq!(mode_of(&settings_dir), 0o700); + assert_eq!(mode_of(&root.join("nested")), 0o700); + } + + #[test] + fn settings_dir_creation_leaves_an_existing_directory_alone() { + let root = temp_root("dir-existing"); + let settings_dir = root.join(".mostro"); + std::fs::create_dir(&settings_dir).expect("create settings dir"); + std::fs::set_permissions(&settings_dir, std::fs::Permissions::from_mode(0o750)) + .expect("loosen permissions"); + // `recursive(true)` makes this a no-op rather than an error, and a + // deliberate group-readable directory must survive it. + create_settings_dir(&settings_dir).expect("existing directory is not an error"); + assert_eq!(mode_of(&settings_dir), 0o750); + } + + #[test] + fn template_is_written_owner_only() { + let root = temp_root("file"); + let config_file = root.join("settings.toml"); + write_owner_only(&config_file, b"nsec_privkey = 'nsec1...'\n").expect("write template"); + assert_eq!(mode_of(&config_file), 0o600); + assert_eq!( + std::fs::read_to_string(&config_file).expect("read back"), + "nsec_privkey = 'nsec1...'\n" + ); + } + + #[test] + fn writing_tightens_a_preexisting_broader_file() { + let root = temp_root("file-existing"); + let config_file = root.join("settings.toml"); + std::fs::write(&config_file, "stale contents that must be truncated").expect("seed file"); + std::fs::set_permissions(&config_file, std::fs::Permissions::from_mode(0o644)) + .expect("loosen permissions"); + write_owner_only(&config_file, b"fresh\n").expect("write template"); + assert_eq!(mode_of(&config_file), 0o600); + assert_eq!( + std::fs::read_to_string(&config_file).expect("read back"), + "fresh\n" + ); + } + + #[test] + fn writing_to_an_unwritable_path_is_an_error() { + let root = temp_root("file-error"); + // A directory cannot be opened for writing, so this exercises the + // error branch instead of silently succeeding. + let config_file = root.join("settings.toml"); + std::fs::create_dir(&config_file).expect("create dir in the file's place"); + assert!(write_owner_only(&config_file, b"x").is_err()); + } +} From fc967ae93492d38e8eb4cd255f2d03984988e36a Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Tue, 18 Aug 2026 22:10:49 -0300 Subject: [PATCH 07/12] fix(config): refuse to follow symlinks when creating settings.toml from template --- INSTALL.md | 18 +++++--- docker/README.md | 8 ++-- docs/STARTUP_AND_CONFIG.md | 6 ++- src/config/util.rs | 89 ++++++++++++++++++++++++++++++-------- 4 files changed, 92 insertions(+), 29 deletions(-) diff --git a/INSTALL.md b/INSTALL.md index 7c9b8ce4..a56d8c96 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -132,26 +132,32 @@ Then point `lnd_macaroon_file` at `/opt/mostro/lnd/admin.macaroon`. The data is saved in a sqlite db file named by default `mostro.db`, this file is saved on the root directory of the project and can be change just editing the `url` var on the `[database]` section in `settings.toml` file. -Before start building you can initialize the database manually with `sqlx-cli` (optional — `mostrod` creates the file and runs migrations on first connect): +Before start building you can initialize the database manually with `sqlx-cli` (optional — `mostrod` creates the file and runs migrations on first connect). + +These commands run as root, so the database ends up owned by root. Hand it to the service account right away: SQLite writes the `-shm`/`-wal` sidecars next to the database, so `mostrod` needs to own the files *and* be able to create new ones in the directory. ```bash cargo install sqlx-cli --version 0.9.0 --no-default-features --features sqlite +cd /opt/mostro export DATABASE_URL=sqlite://mostro.db sqlx database create sqlx migrate run +chown mostro:mostro /opt/mostro/mostro.db* ``` -Check the DB files are there +Check the DB files are there, and that they belong to `mostro`: ```bash ls -al /opt/mostro drwx------ mostro mostro 4.0 KB Fri Jun 14 15:52:07 2024 . -drwxr-x--- root root 4.0 KB Sat Jun 15 15:50:32 2024 .. -.rw-r--r-- root root 52 KB Fri May 31 16:35:34 2024 mostro.db -.rw-r--r-- root root 32 KB Sat Jun 15 15:28:23 2024 mostro.db-shm -.rw-r--r-- root root 16 KB Fri Jun 14 15:57:24 2024 mostro.db-wal +drwxr-x--- root root 4.0 KB Sat Jun 15 15:50:32 2024 .. +.rw-r--r-- mostro mostro 52 KB Fri May 31 16:35:34 2024 mostro.db +.rw-r--r-- mostro mostro 32 KB Sat Jun 15 15:28:23 2024 mostro.db-shm +.rw-r--r-- mostro mostro 16 KB Fri Jun 14 15:57:24 2024 mostro.db-wal ``` +If you skip this step, `mostrod` creates the database itself on first connect — as root during the foreground test below. Either way, the `chown -R mostro:mostro /opt/mostro` further down is the backstop that puts the ownership right before the service starts. + ## Clean compilation artifacts Since the instance you are using has little disk space you don't want to waste valuable disk space. Once successfully compiled the compilation artifacts can use up to **2Gb** of space. diff --git a/docker/README.md b/docker/README.md index 27334cd1..0402e4e9 100644 --- a/docker/README.md +++ b/docker/README.md @@ -64,7 +64,7 @@ To build and run the Docker container using Docker Compose, follow these steps: make docker-build ``` - The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The `config` root itself is set to `0700` as well, since `settings.toml` and `mostro.db` sit next to the credentials. All of them belong to the user that ran the command. + The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The `config` root itself is set to `0700` as well, since `settings.toml` and `mostro.db` sit next to the credentials. The directories and files this command creates belong to the user who ran it; `mostro.db` is created later by the container and belongs to whoever `MOSTRO_CONTAINER_USER` names, uid/gid 1000 by default. The container runs as uid/gid 1000 by default, which matches the first user account on most hosts. If yours is not uid 1000, run the container as yourself instead of handing the config directory over — the daemon also writes `mostro.db` into it: @@ -102,7 +102,7 @@ You can run the plain Mostro image without building locally. Use a single **conf ```sh install -d -m 700 ~/mostro-config ~/mostro-config/lnd - curl -sL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o ~/mostro-config/settings.toml + curl -fsSL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o ~/mostro-config/settings.toml chmod 600 ~/mostro-config/settings.toml ``` @@ -140,7 +140,7 @@ Steps to run the plain Mostro image on a VPS (no repo clone; image from Docker H 1. **Install Docker** on the VPS (e.g. [Docker Engine](https://docs.docker.com/engine/install/)). -2. **Create a config directory** (e.g. `/opt/mostro` or `~/mostro-config`): +2. **Create a config directory** at `/opt/mostro`: ```sh install -d -m 700 -o 1000 -g 1000 /opt/mostro @@ -155,7 +155,7 @@ Steps to run the plain Mostro image on a VPS (no repo clone; image from Docker H - Or download the template and copy it: ```sh - curl -sL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o /opt/mostro/settings.toml + curl -fsSL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o /opt/mostro/settings.toml chmod 600 /opt/mostro/settings.toml chown 1000:1000 /opt/mostro/settings.toml ``` diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 5fa5f5fb..09c07284 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -205,9 +205,13 @@ There is **no** database password or separate global for SQLite; the daemon open Docker flows, the LND credentials under `lnd/`. Keep the directory at mode `0700` and the file at `0600`. - Both paths that create them do so already: `src/config/util.rs`, `fn create_settings_dir` - and `fn write_owner_only` for the non-interactive template copy, and + and `fn create_owner_only` for the non-interactive template copy, and `src/config/wizard.rs`, `fn run_setup_wizard` for the interactive one. A directory or file created outside the daemon — `mkdir`, `cp`, `curl` — inherits the umask instead, so the deployment guides use `install -d -m 700` and `install -m 600`. - An existing settings directory is left as it is, so a deliberately group-readable deployment keeps working. +- The template copy is created with `O_CREAT | O_EXCL` and refuses to start if anything + already occupies `settings.toml`. On a settings directory another local account can + write to, following a symlink planted between the existence check and the create would + truncate its target and reset that target's mode to `0600`. diff --git a/src/config/util.rs b/src/config/util.rs index 36ff3813..253b2f04 100644 --- a/src/config/util.rs +++ b/src/config/util.rs @@ -151,12 +151,20 @@ fn create_settings_dir(settings_dir: &std::path::Path) -> Result<(), MostroError .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string()))) } -/// Write `contents` to `path` with owner-only permissions (`0600` on Unix). +/// Create `path` and write `contents` to it with owner-only permissions +/// (`0600` on Unix). Fails if anything already exists at `path`. /// -/// `OpenOptionsExt::mode` only applies when the file is created, so the mode is -/// also set explicitly afterwards: the caller checks for the file's absence -/// first, and another process could create it in between. -fn write_owner_only(path: &std::path::Path, contents: &[u8]) -> Result<(), MostroError> { +/// `create_new` maps to `O_CREAT | O_EXCL`, which POSIX requires to fail with +/// `EEXIST` when the path names a symbolic link, whatever it points at. The +/// caller only reaches this after finding no settings file, but that check and +/// this call cannot be one operation: on a settings directory another local +/// account can write to, a symlink planted in between would otherwise have its +/// target truncated and its mode reset to `0600` by the two steps below. +/// +/// `OpenOptionsExt::mode` is also masked by the process umask, so the mode is +/// set again through the file descriptor — never through the path, which would +/// reintroduce the symlink it just refused to follow. +fn create_owner_only(path: &std::path::Path, contents: &[u8]) -> Result<(), MostroError> { use std::io::Write; #[cfg(unix)] @@ -164,18 +172,22 @@ fn write_owner_only(path: &std::path::Path, contents: &[u8]) -> Result<(), Mostr use std::os::unix::fs::OpenOptionsExt; fs::OpenOptions::new() .write(true) - .create(true) - .truncate(true) + .create_new(true) .mode(0o600) .open(path) }; #[cfg(not(unix))] let file = fs::OpenOptions::new() .write(true) - .create(true) - .truncate(true) + .create_new(true) .open(path); - let mut file = file.map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + let mut file = file.map_err(|e| { + MostroInternalErr(ServiceError::IOError(format!( + "Could not create {}: {}", + path.display(), + e + ))) + })?; #[cfg(unix)] { @@ -221,7 +233,7 @@ pub fn init_configuration_file(config_path: Option) -> Result<(), Mostro wizard::run_setup_menu(&settings_dir, &config_file_path)? } else { // Non-interactive (Docker, CI, systemd): copy template and exit - write_owner_only(&config_file_path, include_bytes!("../../settings.tpl.toml"))?; + create_owner_only(&config_file_path, include_bytes!("../../settings.tpl.toml"))?; println!( "Created settings file from template at {} - Edit it to configure your Mostro instance", config_file_path.display() @@ -695,7 +707,7 @@ mod owner_only_tests { fn template_is_written_owner_only() { let root = temp_root("file"); let config_file = root.join("settings.toml"); - write_owner_only(&config_file, b"nsec_privkey = 'nsec1...'\n").expect("write template"); + create_owner_only(&config_file, b"nsec_privkey = 'nsec1...'\n").expect("write template"); assert_eq!(mode_of(&config_file), 0o600); assert_eq!( std::fs::read_to_string(&config_file).expect("read back"), @@ -704,18 +716,59 @@ mod owner_only_tests { } #[test] - fn writing_tightens_a_preexisting_broader_file() { + fn a_preexisting_file_is_refused_instead_of_truncated() { let root = temp_root("file-existing"); let config_file = root.join("settings.toml"); - std::fs::write(&config_file, "stale contents that must be truncated").expect("seed file"); + std::fs::write(&config_file, "operator contents").expect("seed file"); std::fs::set_permissions(&config_file, std::fs::Permissions::from_mode(0o644)) .expect("loosen permissions"); - write_owner_only(&config_file, b"fresh\n").expect("write template"); - assert_eq!(mode_of(&config_file), 0o600); + assert!(create_owner_only(&config_file, b"fresh\n").is_err()); + // Neither the contents nor the mode of what was already there change. assert_eq!( std::fs::read_to_string(&config_file).expect("read back"), - "fresh\n" + "operator contents" ); + assert_eq!(mode_of(&config_file), 0o644); + } + + #[test] + fn a_symlink_in_the_settings_path_is_refused_and_its_target_untouched() { + let root = temp_root("file-symlink"); + let victim = root.join("victim"); + std::fs::write(&victim, "victim contents").expect("seed victim"); + std::fs::set_permissions(&victim, std::fs::Permissions::from_mode(0o644)) + .expect("set victim mode"); + + // What another local account could plant in a settings directory it can + // write to, in the window between the caller's existence check and this + // call. Following it would truncate the victim and reset it to 0600. + let config_file = root.join("settings.toml"); + std::os::unix::fs::symlink(&victim, &config_file).expect("plant symlink"); + + assert!(create_owner_only(&config_file, b"template\n").is_err()); + assert_eq!( + std::fs::read_to_string(&victim).expect("read back"), + "victim contents" + ); + assert_eq!(mode_of(&victim), 0o644); + // The symlink itself is left in place; nothing was written through it. + assert!(std::fs::symlink_metadata(&config_file) + .expect("symlink metadata") + .file_type() + .is_symlink()); + } + + #[test] + fn a_dangling_symlink_is_refused_rather_than_created_through() { + let root = temp_root("file-dangling"); + let config_file = root.join("settings.toml"); + // `Path::exists` follows symlinks, so the caller's check reports false + // here and this call is what has to refuse. + std::os::unix::fs::symlink(root.join("does-not-exist"), &config_file) + .expect("plant dangling symlink"); + assert!(!config_file.exists()); + assert!(create_owner_only(&config_file, b"template\n").is_err()); + assert!(!root.join("does-not-exist").exists()); } #[test] @@ -725,6 +778,6 @@ mod owner_only_tests { // error branch instead of silently succeeding. let config_file = root.join("settings.toml"); std::fs::create_dir(&config_file).expect("create dir in the file's place"); - assert!(write_owner_only(&config_file, b"x").is_err()); + assert!(create_owner_only(&config_file, b"x").is_err()); } } From f45efe6dd2e2da21a17ed1b8d5631c58889b4da4 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Tue, 18 Aug 2026 23:57:36 -0300 Subject: [PATCH 08/12] refactor(config): consolidate settings file creation primitives in permissions module --- docs/STARTUP_AND_CONFIG.md | 25 +++-- src/config/permissions.rs | 219 ++++++++++++++++++++++++++++++++++++- src/config/util.rs | 202 +--------------------------------- src/config/wizard.rs | 150 ++++++++++++++++++++----- 4 files changed, 354 insertions(+), 242 deletions(-) diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 09c07284..5c8de377 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -204,14 +204,21 @@ There is **no** database password or separate global for SQLite; the daemon open `MOSTRO_NSEC_PRIVKEY`, and the settings directory also holds `mostro.db` and, in the Docker flows, the LND credentials under `lnd/`. Keep the directory at mode `0700` and the file at `0600`. -- Both paths that create them do so already: `src/config/util.rs`, `fn create_settings_dir` - and `fn create_owner_only` for the non-interactive template copy, and - `src/config/wizard.rs`, `fn run_setup_wizard` for the interactive one. A directory or - file created outside the daemon — `mkdir`, `cp`, `curl` — inherits the umask instead, so - the deployment guides use `install -d -m 700` and `install -m 600`. +- Every path that creates them does so already, through one primitive: + `src/config/permissions.rs`, `fn create_settings_dir` for the directory and + `fn create_owner_only` for the file. Its three callers are the non-interactive template + copy (`src/config/util.rs`, `fn init_configuration_file`), the manual template copy + (`src/config/wizard.rs`, `fn run_setup_menu`) and the guided wizard save + (`src/config/wizard.rs`, `fn save_settings`). A directory or file created outside the + daemon — `mkdir`, `cp`, `curl` — inherits the umask instead, so the deployment guides + use `install -d -m 700` and `install -m 600`. - An existing settings directory is left as it is, so a deliberately group-readable deployment keeps working. -- The template copy is created with `O_CREAT | O_EXCL` and refuses to start if anything - already occupies `settings.toml`. On a settings directory another local account can - write to, following a symlink planted between the existence check and the create would - truncate its target and reset that target's mode to `0600`. +- `create_owner_only` uses `O_CREAT | O_EXCL`, so it fails rather than write through + whatever already occupies the path. This covers initial creation only: on a settings + directory another local account can write to, a symlink planted between the caller's + existence check and the create would otherwise have its target truncated and its mode + reset to `0600`. +- It is not a check on startup as a whole. A `settings.toml` that already exists is read + and loaded normally, symlink or not — `fn init_configuration_file` only reaches the + creation path when it finds no settings file at all. diff --git a/src/config/permissions.rs b/src/config/permissions.rs index 92ff73cc..59ef4ab7 100644 --- a/src/config/permissions.rs +++ b/src/config/permissions.rs @@ -1,10 +1,16 @@ -//! Startup checks on the filesystem permissions of the credential files -//! mostrod reads. +//! Filesystem permissions of the files mostrod owns or reads: the startup +//! check on the credential files named by the settings, and the primitives +//! that create the settings directory and `settings.toml` owner-only. //! //! Kept in its own module — rather than folded into `config::util` — because -//! these checks are about the files named by the settings, not about loading -//! the settings themselves. +//! these are about the files themselves, not about loading the settings. Both +//! `config::util` (non-interactive template copy) and `config::wizard` (manual +//! template copy, guided wizard save) create `settings.toml` through the same +//! primitive here, so a single place decides how that file comes into being. +use mostro_core::error::MostroError::{self, MostroInternalErr}; +use mostro_core::error::ServiceError; +use std::fs; use std::path::Path; /// Warn when a credential file has any of its "other" permission bits set, @@ -65,6 +71,81 @@ fn other_accessible_mode(_path: &Path) -> Option { None } +/// Create the settings directory owner-only (`0700` on Unix). +/// +/// The directory holds `settings.toml` with a plaintext `nsec_privkey`, the +/// `mostro.db` database and, in the Docker flows, the LND credentials under +/// `lnd/`. `create_dir_all` would apply the process umask instead, which on a +/// typical host leaves the directory at `0755`. +/// +/// Only directories this call creates are affected: an operator who already set +/// up the directory with deliberate group access keeps it. +pub(crate) fn create_settings_dir(settings_dir: &Path) -> Result<(), MostroError> { + let mut builder = fs::DirBuilder::new(); + builder.recursive(true); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } + builder + .create(settings_dir) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string()))) +} + +/// Create `path` and write `contents` to it with owner-only permissions +/// (`0600` on Unix). Fails if anything already exists at `path`. +/// +/// Every path that brings an initial `settings.toml` into existence goes +/// through here, so the file that later carries `nsec_privkey` is never left +/// at the process umask and never reached through a symlink. +/// +/// `create_new` maps to `O_CREAT | O_EXCL`, which POSIX requires to fail with +/// `EEXIST` when the path names a symbolic link, whatever it points at. Callers +/// only reach this after finding no settings file, but that check and this call +/// cannot be one operation: on a settings directory another local account can +/// write to, a symlink planted in between would otherwise have its target +/// truncated and its mode reset to `0600` by the two steps below. +/// +/// `OpenOptionsExt::mode` is also masked by the process umask, so the mode is +/// set again through the file descriptor — never through the path, which would +/// reintroduce the symlink it just refused to follow. +pub(crate) fn create_owner_only(path: &Path, contents: &[u8]) -> Result<(), MostroError> { + use std::io::Write; + + #[cfg(unix)] + let file = { + use std::os::unix::fs::OpenOptionsExt; + fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) + }; + #[cfg(not(unix))] + let file = fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path); + let mut file = file.map_err(|e| { + MostroInternalErr(ServiceError::IOError(format!( + "Could not create {}: {}", + path.display(), + e + ))) + })?; + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + file.set_permissions(fs::Permissions::from_mode(0o600)) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + } + + file.write_all(contents) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string()))) +} + #[cfg(all(test, unix))] mod tests { use super::*; @@ -149,3 +230,133 @@ mod tests { warn_if_other_accessible("/definitely/not/here.macaroon", "LND admin macaroon"); } } + +#[cfg(all(test, unix))] +mod owner_only_tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + + fn temp_root(tag: &str) -> std::path::PathBuf { + let dir = + std::env::temp_dir().join(format!("mostro-owner-only-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create temp dir"); + dir + } + + fn mode_of(path: &std::path::Path) -> u32 { + std::fs::metadata(path) + .expect("metadata") + .permissions() + .mode() + & 0o777 + } + + #[test] + fn settings_dir_is_created_owner_only() { + let root = temp_root("dir"); + let settings_dir = root.join(".mostro"); + create_settings_dir(&settings_dir).expect("create settings dir"); + assert_eq!(mode_of(&settings_dir), 0o700); + } + + #[test] + fn settings_dir_creation_is_recursive_and_owner_only() { + let root = temp_root("dir-nested"); + let settings_dir = root.join("nested").join(".mostro"); + create_settings_dir(&settings_dir).expect("create nested settings dir"); + assert_eq!(mode_of(&settings_dir), 0o700); + assert_eq!(mode_of(&root.join("nested")), 0o700); + } + + #[test] + fn settings_dir_creation_leaves_an_existing_directory_alone() { + let root = temp_root("dir-existing"); + let settings_dir = root.join(".mostro"); + std::fs::create_dir(&settings_dir).expect("create settings dir"); + std::fs::set_permissions(&settings_dir, std::fs::Permissions::from_mode(0o750)) + .expect("loosen permissions"); + // `recursive(true)` makes this a no-op rather than an error, and a + // deliberate group-readable directory must survive it. + create_settings_dir(&settings_dir).expect("existing directory is not an error"); + assert_eq!(mode_of(&settings_dir), 0o750); + } + + #[test] + fn template_is_written_owner_only() { + let root = temp_root("file"); + let config_file = root.join("settings.toml"); + create_owner_only(&config_file, b"nsec_privkey = 'nsec1...'\n").expect("write template"); + assert_eq!(mode_of(&config_file), 0o600); + assert_eq!( + std::fs::read_to_string(&config_file).expect("read back"), + "nsec_privkey = 'nsec1...'\n" + ); + } + + #[test] + fn a_preexisting_file_is_refused_instead_of_truncated() { + let root = temp_root("file-existing"); + let config_file = root.join("settings.toml"); + std::fs::write(&config_file, "operator contents").expect("seed file"); + std::fs::set_permissions(&config_file, std::fs::Permissions::from_mode(0o644)) + .expect("loosen permissions"); + assert!(create_owner_only(&config_file, b"fresh\n").is_err()); + // Neither the contents nor the mode of what was already there change. + assert_eq!( + std::fs::read_to_string(&config_file).expect("read back"), + "operator contents" + ); + assert_eq!(mode_of(&config_file), 0o644); + } + + #[test] + fn a_symlink_in_the_settings_path_is_refused_and_its_target_untouched() { + let root = temp_root("file-symlink"); + let victim = root.join("victim"); + std::fs::write(&victim, "victim contents").expect("seed victim"); + std::fs::set_permissions(&victim, std::fs::Permissions::from_mode(0o644)) + .expect("set victim mode"); + + // What another local account could plant in a settings directory it can + // write to, in the window between the caller's existence check and this + // call. Following it would truncate the victim and reset it to 0600. + let config_file = root.join("settings.toml"); + std::os::unix::fs::symlink(&victim, &config_file).expect("plant symlink"); + + assert!(create_owner_only(&config_file, b"template\n").is_err()); + assert_eq!( + std::fs::read_to_string(&victim).expect("read back"), + "victim contents" + ); + assert_eq!(mode_of(&victim), 0o644); + // The symlink itself is left in place; nothing was written through it. + assert!(std::fs::symlink_metadata(&config_file) + .expect("symlink metadata") + .file_type() + .is_symlink()); + } + + #[test] + fn a_dangling_symlink_is_refused_rather_than_created_through() { + let root = temp_root("file-dangling"); + let config_file = root.join("settings.toml"); + // `Path::exists` follows symlinks, so the caller's check reports false + // here and this call is what has to refuse. + std::os::unix::fs::symlink(root.join("does-not-exist"), &config_file) + .expect("plant dangling symlink"); + assert!(!config_file.exists()); + assert!(create_owner_only(&config_file, b"template\n").is_err()); + assert!(!root.join("does-not-exist").exists()); + } + + #[test] + fn writing_to_an_unwritable_path_is_an_error() { + let root = temp_root("file-error"); + // A directory cannot be opened for writing, so this exercises the + // error branch instead of silently succeeding. + let config_file = root.join("settings.toml"); + std::fs::create_dir(&config_file).expect("create dir in the file's place"); + assert!(create_owner_only(&config_file, b"x").is_err()); + } +} diff --git a/src/config/util.rs b/src/config/util.rs index 253b2f04..fd4ad211 100644 --- a/src/config/util.rs +++ b/src/config/util.rs @@ -3,6 +3,7 @@ /// It includes functions to initialize the default settings directory and create a settings file from the template if it doesn't exist. /// It also includes functions to add a trailing slash to a path if it doesn't already have one. use crate::config::constants::{ENV_FILENAME, MAX_DEV_FEE_PERCENTAGE, MIN_DEV_FEE_PERCENTAGE}; +use crate::config::permissions::{create_owner_only, create_settings_dir}; use crate::config::secret::read_nsec_env_var; use crate::config::wizard; use crate::config::{init_mostro_settings, Settings}; @@ -129,77 +130,6 @@ fn validate_cashu_settings( Ok(()) } -/// Create the settings directory owner-only (`0700` on Unix). -/// -/// The directory holds `settings.toml` with a plaintext `nsec_privkey`, the -/// `mostro.db` database and, in the Docker flows, the LND credentials under -/// `lnd/`. `create_dir_all` would apply the process umask instead, which on a -/// typical host leaves the directory at `0755`. -/// -/// Only directories this call creates are affected: an operator who already set -/// up the directory with deliberate group access keeps it. -fn create_settings_dir(settings_dir: &std::path::Path) -> Result<(), MostroError> { - let mut builder = fs::DirBuilder::new(); - builder.recursive(true); - #[cfg(unix)] - { - use std::os::unix::fs::DirBuilderExt; - builder.mode(0o700); - } - builder - .create(settings_dir) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string()))) -} - -/// Create `path` and write `contents` to it with owner-only permissions -/// (`0600` on Unix). Fails if anything already exists at `path`. -/// -/// `create_new` maps to `O_CREAT | O_EXCL`, which POSIX requires to fail with -/// `EEXIST` when the path names a symbolic link, whatever it points at. The -/// caller only reaches this after finding no settings file, but that check and -/// this call cannot be one operation: on a settings directory another local -/// account can write to, a symlink planted in between would otherwise have its -/// target truncated and its mode reset to `0600` by the two steps below. -/// -/// `OpenOptionsExt::mode` is also masked by the process umask, so the mode is -/// set again through the file descriptor — never through the path, which would -/// reintroduce the symlink it just refused to follow. -fn create_owner_only(path: &std::path::Path, contents: &[u8]) -> Result<(), MostroError> { - use std::io::Write; - - #[cfg(unix)] - let file = { - use std::os::unix::fs::OpenOptionsExt; - fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(path) - }; - #[cfg(not(unix))] - let file = fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(path); - let mut file = file.map_err(|e| { - MostroInternalErr(ServiceError::IOError(format!( - "Could not create {}: {}", - path.display(), - e - ))) - })?; - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - file.set_permissions(fs::Permissions::from_mode(0o600)) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; - } - - file.write_all(contents) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string()))) -} - /// Initialize the default settings directory and create a settings file from the template if it doesn't exist. /// Checks if the directory already exists, and if not, creates it and writes the template file. /// If a custom config path is provided, it uses that instead of the default `~/.mostro` directory. @@ -651,133 +581,3 @@ mod init_configuration_file_tests { assert!(result.is_err()); } } - -#[cfg(all(test, unix))] -mod owner_only_tests { - use super::*; - use std::os::unix::fs::PermissionsExt; - - fn temp_root(tag: &str) -> std::path::PathBuf { - let dir = - std::env::temp_dir().join(format!("mostro-owner-only-{tag}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).expect("create temp dir"); - dir - } - - fn mode_of(path: &std::path::Path) -> u32 { - std::fs::metadata(path) - .expect("metadata") - .permissions() - .mode() - & 0o777 - } - - #[test] - fn settings_dir_is_created_owner_only() { - let root = temp_root("dir"); - let settings_dir = root.join(".mostro"); - create_settings_dir(&settings_dir).expect("create settings dir"); - assert_eq!(mode_of(&settings_dir), 0o700); - } - - #[test] - fn settings_dir_creation_is_recursive_and_owner_only() { - let root = temp_root("dir-nested"); - let settings_dir = root.join("nested").join(".mostro"); - create_settings_dir(&settings_dir).expect("create nested settings dir"); - assert_eq!(mode_of(&settings_dir), 0o700); - assert_eq!(mode_of(&root.join("nested")), 0o700); - } - - #[test] - fn settings_dir_creation_leaves_an_existing_directory_alone() { - let root = temp_root("dir-existing"); - let settings_dir = root.join(".mostro"); - std::fs::create_dir(&settings_dir).expect("create settings dir"); - std::fs::set_permissions(&settings_dir, std::fs::Permissions::from_mode(0o750)) - .expect("loosen permissions"); - // `recursive(true)` makes this a no-op rather than an error, and a - // deliberate group-readable directory must survive it. - create_settings_dir(&settings_dir).expect("existing directory is not an error"); - assert_eq!(mode_of(&settings_dir), 0o750); - } - - #[test] - fn template_is_written_owner_only() { - let root = temp_root("file"); - let config_file = root.join("settings.toml"); - create_owner_only(&config_file, b"nsec_privkey = 'nsec1...'\n").expect("write template"); - assert_eq!(mode_of(&config_file), 0o600); - assert_eq!( - std::fs::read_to_string(&config_file).expect("read back"), - "nsec_privkey = 'nsec1...'\n" - ); - } - - #[test] - fn a_preexisting_file_is_refused_instead_of_truncated() { - let root = temp_root("file-existing"); - let config_file = root.join("settings.toml"); - std::fs::write(&config_file, "operator contents").expect("seed file"); - std::fs::set_permissions(&config_file, std::fs::Permissions::from_mode(0o644)) - .expect("loosen permissions"); - assert!(create_owner_only(&config_file, b"fresh\n").is_err()); - // Neither the contents nor the mode of what was already there change. - assert_eq!( - std::fs::read_to_string(&config_file).expect("read back"), - "operator contents" - ); - assert_eq!(mode_of(&config_file), 0o644); - } - - #[test] - fn a_symlink_in_the_settings_path_is_refused_and_its_target_untouched() { - let root = temp_root("file-symlink"); - let victim = root.join("victim"); - std::fs::write(&victim, "victim contents").expect("seed victim"); - std::fs::set_permissions(&victim, std::fs::Permissions::from_mode(0o644)) - .expect("set victim mode"); - - // What another local account could plant in a settings directory it can - // write to, in the window between the caller's existence check and this - // call. Following it would truncate the victim and reset it to 0600. - let config_file = root.join("settings.toml"); - std::os::unix::fs::symlink(&victim, &config_file).expect("plant symlink"); - - assert!(create_owner_only(&config_file, b"template\n").is_err()); - assert_eq!( - std::fs::read_to_string(&victim).expect("read back"), - "victim contents" - ); - assert_eq!(mode_of(&victim), 0o644); - // The symlink itself is left in place; nothing was written through it. - assert!(std::fs::symlink_metadata(&config_file) - .expect("symlink metadata") - .file_type() - .is_symlink()); - } - - #[test] - fn a_dangling_symlink_is_refused_rather_than_created_through() { - let root = temp_root("file-dangling"); - let config_file = root.join("settings.toml"); - // `Path::exists` follows symlinks, so the caller's check reports false - // here and this call is what has to refuse. - std::os::unix::fs::symlink(root.join("does-not-exist"), &config_file) - .expect("plant dangling symlink"); - assert!(!config_file.exists()); - assert!(create_owner_only(&config_file, b"template\n").is_err()); - assert!(!root.join("does-not-exist").exists()); - } - - #[test] - fn writing_to_an_unwritable_path_is_an_error() { - let root = temp_root("file-error"); - // A directory cannot be opened for writing, so this exercises the - // error branch instead of silently succeeding. - let config_file = root.join("settings.toml"); - std::fs::create_dir(&config_file).expect("create dir in the file's place"); - assert!(create_owner_only(&config_file, b"x").is_err()); - } -} diff --git a/src/config/wizard.rs b/src/config/wizard.rs index e20f0bda..cf918a24 100644 --- a/src/config/wizard.rs +++ b/src/config/wizard.rs @@ -9,6 +9,7 @@ use secrecy::{ExposeSecret, SecretString}; use zeroize::Zeroizing; use super::constants::{ENV_FILENAME, NSEC_ENV_VAR}; +use super::permissions::create_owner_only; use super::settings::Settings; use super::types::{ DatabaseSettings, LightningSettings, MostroSettings, NostrSettings, RpcSettings, @@ -43,8 +44,7 @@ pub fn run_setup_menu( Ok(settings) } _ => { - std::fs::write(config_file_path, TEMPLATE_BYTES) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + create_owner_only(config_file_path, TEMPLATE_BYTES)?; println!( "Created settings file from template at {} - Edit it to configure your Mostro instance", config_file_path.display() @@ -79,32 +79,7 @@ fn run_setup_wizard(settings_dir: &Path, config_file_path: &Path) -> Result Result<(), MostroError> { + let toml_content = Zeroizing::new( + toml::to_string_pretty(settings) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?, + ); + create_owner_only(config_file_path, toml_content.as_bytes()) +} + /// Write `MOSTRO_NSEC_PRIVKEY=` to the given path with 0o600 permissions on Unix. /// /// `OpenOptionsExt::mode(0o600)` only applies when the file is created, so for @@ -514,3 +503,108 @@ mod tests { assert_eq!(mode_of(&env_path), 0o600); } } + +#[cfg(all(test, unix))] +mod save_settings_tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + + fn temp_root(tag: &str) -> PathBuf { + let dir = + std::env::temp_dir().join(format!("mostro-wizard-save-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create temp dir"); + dir + } + + fn mode_of(path: &Path) -> u32 { + std::fs::metadata(path) + .expect("metadata") + .permissions() + .mode() + & 0o777 + } + + fn sample_settings() -> Settings { + Settings { + database: DatabaseSettings::default(), + lightning: LightningSettings::default(), + nostr: NostrSettings { + nsec_privkey: "nsec13as48eum93hkg7plv526r9gjpa0uc52zysqm93pmnkca9e69x6tsdjmdxd" + .to_string() + .into(), + relays: vec!["wss://relay.mostro.network".to_string()], + }, + mostro: MostroSettings::default(), + rpc: RpcSettings::default(), + expiration: None, + anti_abuse_bond: None, + cashu: None, + price: None, + } + } + + #[test] + fn wizard_save_creates_the_file_owner_only() { + let root = temp_root("ok"); + let config_file = root.join("settings.toml"); + save_settings(&config_file, &sample_settings()).expect("save settings"); + assert_eq!(mode_of(&config_file), 0o600); + let written = std::fs::read_to_string(&config_file).expect("read back"); + assert!(written.contains("nsec_privkey")); + } + + #[test] + fn wizard_save_refuses_a_preexisting_file() { + let root = temp_root("existing"); + let config_file = root.join("settings.toml"); + std::fs::write(&config_file, "operator contents").expect("seed file"); + assert!(save_settings(&config_file, &sample_settings()).is_err()); + assert_eq!( + std::fs::read_to_string(&config_file).expect("read back"), + "operator contents" + ); + } + + #[test] + fn wizard_save_leaves_a_symlink_target_untouched() { + let root = temp_root("symlink"); + let victim = root.join("victim"); + std::fs::write(&victim, "victim contents").expect("seed victim"); + std::fs::set_permissions(&victim, std::fs::Permissions::from_mode(0o644)) + .expect("set victim mode"); + + let config_file = root.join("settings.toml"); + std::os::unix::fs::symlink(&victim, &config_file).expect("plant symlink"); + + // The nsec must not be written through a link another local account + // could have planted in the settings directory. + assert!(save_settings(&config_file, &sample_settings()).is_err()); + assert_eq!( + std::fs::read_to_string(&victim).expect("read back"), + "victim contents" + ); + assert_eq!(mode_of(&victim), 0o644); + } + + #[test] + fn manual_template_copy_is_owner_only_and_refuses_a_symlink() { + let root = temp_root("template"); + let config_file = root.join("settings.toml"); + // The manual branch of `run_setup_menu` writes TEMPLATE_BYTES through + // the same primitive; the menu itself needs a terminal, so exercise + // the write it performs. + create_owner_only(&config_file, TEMPLATE_BYTES).expect("write template"); + assert_eq!(mode_of(&config_file), 0o600); + + let victim = root.join("victim"); + std::fs::write(&victim, "victim contents").expect("seed victim"); + let linked = root.join("linked.toml"); + std::os::unix::fs::symlink(&victim, &linked).expect("plant symlink"); + assert!(create_owner_only(&linked, TEMPLATE_BYTES).is_err()); + assert_eq!( + std::fs::read_to_string(&victim).expect("read back"), + "victim contents" + ); + } +} From 38085adc5973110277cbec2379e17fcba7ee3e39 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Wed, 19 Aug 2026 00:28:49 -0300 Subject: [PATCH 09/12] docs: clarify symlink handling in settings directory creation vs file creation --- docs/STARTUP_AND_CONFIG.md | 8 ++++++++ src/config/permissions.rs | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 5c8de377..8b7730b6 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -222,3 +222,11 @@ There is **no** database password or separate global for SQLite; the daemon open - It is not a check on startup as a whole. A `settings.toml` that already exists is read and loaded normally, symlink or not — `fn init_configuration_file` only reaches the creation path when it finds no settings file at all. +- The guarantee covers the final entry, not the directory path leading to it. + `fn create_settings_dir` uses a recursive `DirBuilder`, which resolves symlinked + components on the way, so a settings directory reached through a symlinked parent is + created at whatever that link points to. This is deliberate: operators do symlink a + config directory onto another volume, and planting such a link on the default + `~/.mostro` path means write access to `$HOME`, which is a compromise of the account + already. A settings directory that is itself a symlink to an existing directory is used + as-is, and a dangling one is refused. diff --git a/src/config/permissions.rs b/src/config/permissions.rs index 59ef4ab7..25b920a7 100644 --- a/src/config/permissions.rs +++ b/src/config/permissions.rs @@ -80,6 +80,13 @@ fn other_accessible_mode(_path: &Path) -> Option { /// /// Only directories this call creates are affected: an operator who already set /// up the directory with deliberate group access keeps it. +/// +/// The recursive builder resolves symlinked components on the way, unlike +/// `create_owner_only` for the final file. That is deliberate: symlinking a +/// config directory onto another volume is a legitimate setup, and planting a +/// link on the default `~/.mostro` path takes write access to `$HOME`, which +/// already owns the account. A settings directory that is itself a symlink to +/// an existing directory never reaches here — the caller finds it and uses it. pub(crate) fn create_settings_dir(settings_dir: &Path) -> Result<(), MostroError> { let mut builder = fs::DirBuilder::new(); builder.recursive(true); From dc24e11789d274c9e2a756155e7d7dceb8ab26ab Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Wed, 19 Aug 2026 15:42:22 -0300 Subject: [PATCH 10/12] docs: document atomic .env writes and expand permission check coverage to Cashu mode --- Makefile | 25 ++- README.md | 6 +- docker/ENV_VARIABLES.md | 7 +- docker/README.md | 18 +- docker/start.sh | 7 +- docs/STARTUP_AND_CONFIG.md | 53 +++-- src/cli.rs | 11 +- src/config/constants.rs | 3 + src/config/mod.rs | 2 + src/config/permissions.rs | 448 +++++++++++++++++++++++++++++-------- src/config/test_support.rs | 90 ++++++++ src/config/util.rs | 30 +-- src/config/wizard.rs | 145 +++++------- src/main.rs | 21 +- 14 files changed, 627 insertions(+), 239 deletions(-) create mode 100644 src/config/test_support.rs diff --git a/Makefile b/Makefile index 490971c0..cf5da680 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,21 @@ SHELL := $(shell which bash) VERSION := $(shell grep "^version = " Cargo.toml | sed "s/version = \"\(.*\)\"/\1/") +# Notes on the docker/config handling below, since none of it is obvious: +# +# - `install -d -m 700` applies the mode to an existing directory too, so it is +# guarded with `[ -d config ]` outside docker-build. Only the run that first +# creates the directory gets to decide its mode; an operator who deliberately +# opened `config` up to a group keeps it across every later `make docker-up`. +# - `install -m 600` onto an existing 0644 macaroon is safe as it stands: it +# unlinks the destination and creates it with the owner-only bits already +# applied, rather than truncating in place and chmod'ing at the end. Measured +# by polling the mode throughout a 300 MB copy — only 0600 is ever observed. +# - The mostro container has to run as whoever owns `config`: the macaroon +# there is 0600 and the daemon also writes mostro.db beside it. Deriving the +# uid:gid from the directory covers both the default (docker-build created it +# as you) and the documented `chown -R 1000:1000` handover. + docker-build: @set -o pipefail; \ cd docker && \ @@ -31,8 +46,8 @@ docker-build: install -m 600 "$${LND_MACAROON_FILE}" config/lnd/admin.macaroon && \ echo "Wrote config/lnd/tls.cert (mode 644) and config/lnd/admin.macaroon (mode 600)" && \ echo "config and config/lnd are mode 700: settings.toml holds nsec_privkey and mostro.db lands there too" && \ - echo "Note: the container runs as uid/gid 1000 by default. If your user is not uid 1000," && \ - echo ' export MOSTRO_CONTAINER_USER=$$(id -u):$$(id -g) before make docker-up' && \ + echo "The mostro container runs as the owner of docker/config, which make docker-up derives" && \ + echo "and prints. Set MOSTRO_CONTAINER_USER=uid:gid to override it." && \ echo "Building docker image" && \ docker compose build @@ -40,9 +55,11 @@ docker-up: @set -o pipefail; \ cd docker && \ echo "Copying Nostr relay config" && \ - install -d -m 700 config && \ + { [ -d config ] || install -d -m 700 config; } && \ mkdir -p config/relay && \ cp -v ./relay_config.toml config/relay/config.toml && \ + export MOSTRO_CONTAINER_USER="$${MOSTRO_CONTAINER_USER:-$$(stat -c '%u:%g' config 2>/dev/null || stat -f '%u:%g' config)}" && \ + echo "Running mostro as $${MOSTRO_CONTAINER_USER} (MOSTRO_CONTAINER_USER; defaults to the owner of docker/config)" && \ echo "Starting services" && \ docker compose up -d @@ -50,7 +67,7 @@ docker-relay-up: @set -o pipefail; \ cd docker && \ echo "Copying Nostr relay config" && \ - install -d -m 700 config && \ + { [ -d config ] || install -d -m 700 config; } && \ mkdir -p config/relay && \ cp -v ./relay_config.toml config/relay/config.toml && \ echo "Starting Nostr relay" && \ diff --git a/README.md b/README.md index 5fa8a478..05172aff 100644 --- a/README.md +++ b/README.md @@ -497,7 +497,7 @@ This starts: - Mostro daemon (exposed via configured relays) - Local Nostr relay (port 7000 by default) -`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node, and sets `docker/config` and `docker/config/lnd` to mode `0700` — the same directory holds `settings.toml` and `mostro.db`. The container runs as uid/gid 1000 by default; if your user is not uid 1000, `export MOSTRO_CONTAINER_USER=$(id -u):$(id -g)` so it runs as you and can read those files. +`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node, and sets `docker/config` and `docker/config/lnd` to mode `0700` — the same directory holds `settings.toml` and `mostro.db`. `make docker-up` then runs the container as the owner of `docker/config`, so it can read those files whatever your uid is; `export MOSTRO_CONTAINER_USER=uid:gid` to override that. **Stop**: `make docker-down` @@ -569,7 +569,7 @@ payment_retries_interval = 60 # seconds between retries **Required**: LND connection details. Mostro needs admin macaroon for hold invoice management. -**Permissions**: the admin macaroon is a spend-capable credential — anyone who can read it controls the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the account running `mostrod`, or by that account and LND's group (`chmod 600` for a private copy, `chmod o=` to keep group access). mostrod logs a warning at startup when the file's `other` permission bits are set. +**Permissions**: the admin macaroon is a spend-capable credential — anyone who can read it controls the node, including the funds escrowed in Mostro's hold invoices. Keep it readable only by the account running `mostrod`, or by that account and LND's group (`chmod 600` for a private copy, `chmod o=` to keep group access). mostrod logs a warning at startup when the file's `other` permission bits are set. That check runs in Lightning mode only, right before the LND connection is opened, so a node running Cashu escrow never reaches it. --- @@ -596,7 +596,7 @@ rana --vanity mostro **Important**: Never reuse keys between Mostro instances. Each daemon needs a unique identity. -**Permissions**: `settings.toml` holds `nsec_privkey` in plaintext unless you move it to the environment (see below), and `mostro.db` sits in the same directory. Keep both owner-only: `chmod 700` on the settings directory and `chmod 600` on `settings.toml`. mostrod already creates them that way when it has to — both through the interactive setup wizard and through the template copy it makes on a non-interactive first run — but a directory or file you create yourself with `mkdir`, `cp` or `curl` inherits your umask instead, so use `install -d -m 700` and `install -m 600`. +**Permissions**: `settings.toml` holds `nsec_privkey` in plaintext unless you move it to the environment (see below), and `mostro.db` sits in the same directory. Keep both owner-only: `chmod 700` on the settings directory and `chmod 600` on `settings.toml`. mostrod already creates them that way when it has to — both through the interactive setup wizard and through the template copy it makes on a non-interactive first run — but a directory or file you create yourself with `mkdir`, `cp` or `curl` inherits your umask instead, so use `install -d -m 700` and `install -m 600`. At startup mostrod logs a warning when `settings.toml` or `/.env` has its `other` permission bits set; unlike the macaroon check, this one runs in both Lightning and Cashu mode, because the nsec is the instance's identity either way. ##### Providing the nsec via environment variable diff --git a/docker/ENV_VARIABLES.md b/docker/ENV_VARIABLES.md index bc62dabe..debdd73f 100644 --- a/docker/ENV_VARIABLES.md +++ b/docker/ENV_VARIABLES.md @@ -12,7 +12,7 @@ This document describes the environment variables used by the Docker setup. These files are copied to `docker/config/lnd/` during the build process: the cert with mode `0644`, the admin macaroon with mode `0600`, both inside a directory with mode `0700`. The macaroon grants full control of your LND node, so it is never left readable by other users on the host. The `docker/config` root is set to mode `0700` as well, since `settings.toml` (which carries `nsec_privkey`) and `mostro.db` live beside the credentials. -The copies belong to the user that ran the command, and the container runs as uid/gid 1000 by default. If your user is not uid 1000, run the container as yourself with the optional variable below rather than handing the config directory over to uid 1000. +The copies belong to the user that ran the command, and `make docker-up` runs the container as the owner of `docker/config` — that user — so a host account that is not uid 1000 needs no extra step. Set the optional variable below to override it. ## Optional Variables @@ -20,8 +20,9 @@ The copies belong to the user that ran the command, and the container runs as ui - Used in `compose.yml` for port mapping - Example: `export MOSTRO_RELAY_LOCAL_PORT=7000` -- `MOSTRO_CONTAINER_USER`: uid/gid the `mostro` container runs as (defaults to `1000:1000`, the image's `mostrouser`) - - Set it when your host user is not uid 1000, so the container can read the `0600` macaroon and write `mostro.db` in the mounted config directory +- `MOSTRO_CONTAINER_USER`: uid/gid the `mostro` container runs as + - `make docker-up` defaults it to the owner of `docker/config`, the account that can read the `0600` macaroon and write `mostro.db` there, and prints what it picked. A bare `docker compose up` falls back to `1000:1000`, the image's `mostrouser`. + - Set it to run as someone else — for instance uid/gid 1000 on a config directory you handed over with `chown -R 1000:1000` - Example: `export MOSTRO_CONTAINER_USER=$(id -u):$(id -g)` - `MOSTRO_DB_PASSWORD`: Not used (database encryption was removed). Kept in `compose.yml` for backward compatibility; can be omitted or left empty. diff --git a/docker/README.md b/docker/README.md index 0402e4e9..3366578f 100644 --- a/docker/README.md +++ b/docker/README.md @@ -37,7 +37,7 @@ To build and run the Docker container using Docker Compose, follow these steps: install -m 600 ../settings.tpl.toml config/settings.toml ``` - Mode `0700` on `config` and `0600` on `settings.toml` because that directory ends up holding every secret this deployment has: `nsec_privkey` in `settings.toml`, the LND credentials in `config/lnd/`, and the `mostro.db` the daemon writes. `install -d -m 700` also tightens a `config` directory an earlier `mkdir -p` left at `0755`. + Mode `0700` on `config` and `0600` on `settings.toml` because that directory ends up holding every secret this deployment has: `nsec_privkey` in `settings.toml`, the LND credentials in `config/lnd/`, and the `mostro.db` the daemon writes. `install -d -m 700` also tightens a `config` directory an earlier `mkdir -p` left at `0755` — this command is you deciding the mode. `make docker-up` will not decide it again: it only creates `config` when it is missing, so a directory you deliberately opened up to a group later on keeps that mode. _Don't forget to edit `lnd_grpc_host`, `nsec_privkey` and `relays` fields in the `config/settings.toml` file. Note that paths in `settings.toml` refer to paths **inside the container**, so use `/config/lnd/tls.cert` and `/config/lnd/admin.macaroon` for the LND certificate and macaroon files (these will be copied there by `make docker-build`)._ @@ -64,15 +64,15 @@ To build and run the Docker container using Docker Compose, follow these steps: make docker-build ``` - The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The `config` root itself is set to `0700` as well, since `settings.toml` and `mostro.db` sit next to the credentials. The directories and files this command creates belong to the user who ran it; `mostro.db` is created later by the container and belongs to whoever `MOSTRO_CONTAINER_USER` names, uid/gid 1000 by default. + The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The `config` root itself is set to `0700` as well, since `settings.toml` and `mostro.db` sit next to the credentials. The directories and files this command creates belong to the user who ran it; `mostro.db` is created later by the container and belongs to whoever the container runs as. - The container runs as uid/gid 1000 by default, which matches the first user account on most hosts. If yours is not uid 1000, run the container as yourself instead of handing the config directory over — the daemon also writes `mostro.db` into it: + `make docker-up` runs the container as the owner of `docker/config` and prints the uid/gid it picked. That is the account that can actually read the `0600` macaroon and write `mostro.db` there, whether the directory belongs to you (the usual case after `make docker-build`) or was handed to uid/gid 1000. To pick a different one, export the variable `compose.yml` reads in the same shell: ```sh export MOSTRO_CONTAINER_USER=$(id -u):$(id -g) ``` - `compose.yml` reads that variable, so export it in the same shell you run `make docker-up` from. + A bare `docker compose up` does not derive anything and falls back to `1000:1000`, the image's `mostrouser`. 4. [Optional] Set the `MOSTRO_RELAY_LOCAL_PORT` environment variable to the port you want to use for the local relay (defaults to 7000 if not set). This can be set before running `make docker-up`: @@ -102,11 +102,10 @@ You can run the plain Mostro image without building locally. Use a single **conf ```sh install -d -m 700 ~/mostro-config ~/mostro-config/lnd - curl -fsSL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o ~/mostro-config/settings.toml - chmod 600 ~/mostro-config/settings.toml + (umask 077 && curl -fsSL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o ~/mostro-config/settings.toml) ``` - The config root is `0700` and `settings.toml` is `0600` because both `nsec_privkey` and, later, `mostro.db` live there. `curl` creates the file under your umask, typically `0644`, so the mode has to be set afterwards. + The config root is `0700` and `settings.toml` is `0600` because both `nsec_privkey` and, later, `mostro.db` live there. `curl` creates the file under the umask in force, typically `0644`; setting the umask in a subshell around it means the file is never world-readable, not even for the moment a subsequent `chmod` would take. **Option B — use the entrypoint default:** create the config dir with `install -d -m 700 ~/mostro-config ~/mostro-config/lnd`, then run the container once against it; the entrypoint installs a default `settings.toml` (from the image, built from `settings.tpl.toml`) into `/config` with mode `0600`. Stop the container, edit the file on the host (e.g. `~/mostro-config/settings.toml`), then start the container again. @@ -155,12 +154,11 @@ Steps to run the plain Mostro image on a VPS (no repo clone; image from Docker H - Or download the template and copy it: ```sh - curl -fsSL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o /opt/mostro/settings.toml - chmod 600 /opt/mostro/settings.toml + (umask 077 && curl -fsSL https://raw.githubusercontent.com/MostroP2P/mostro/main/settings.tpl.toml -o /opt/mostro/settings.toml) chown 1000:1000 /opt/mostro/settings.toml ``` - `curl` writes the file under root's umask, typically `0644` and root-owned, so the mode and the owner have to be set afterwards: the file receives `nsec_privkey` in step 5, and the container reads it as uid/gid 1000. + `curl` writes the file under root's umask, typically `0644` and root-owned. The umask in the subshell settles the mode as the file is created, so it is never world-readable; the owner still has to be handed over afterwards, because the file receives `nsec_privkey` in step 5 and the container reads it as uid/gid 1000. 4. **Put LND files** in the config dir so they appear at `/config/lnd/` in the container. Use `install -m` rather than `cp`, which would keep whatever mode the source file (or an already existing destination file) happens to have: diff --git a/docker/start.sh b/docker/start.sh index 360c7064..2cf56430 100644 --- a/docker/start.sh +++ b/docker/start.sh @@ -4,7 +4,12 @@ set -e # Check if the settings.toml file exists, if not, create a new one. # `install -m 600` rather than `cp`: the file receives nsec_privkey once edited, # and `cp` would keep whatever mode the template in the image happens to have. -if [ ! -f /config/settings.toml ]; then +# +# `-e` plus `-L` rather than `-f`: `-f` is false for a dangling symlink, and +# `install` would then write the template through it. Leaving anything that is +# already at the path alone means mostrod reports it a moment later, with more +# context than this script has. +if [ ! -e /config/settings.toml ] && [ ! -L /config/settings.toml ]; then echo "settings.toml not found, creating a new one from template (default)." install -m 600 /mostro/settings.toml /config/settings.toml fi diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 8b7730b6..ab3d8345 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -114,6 +114,10 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. is advisory — the daemon still starts — and tolerates `0640`, the mode LND itself writes the macaroon with, so reading it through the node's group stays supported. + - The same check runs over `settings.toml` and `/.env` right after + the settings load, in both Lightning and Cashu mode: those two carry + `nsec_privkey`, which is the instance's identity regardless of which escrow + it uses. A missing `.env` is silent, since it is optional. - Only the mode bits are inspected: a POSIX ACL can grant a named user access without setting them, so a quiet startup is not proof that no other account can read the file. @@ -204,29 +208,44 @@ There is **no** database password or separate global for SQLite; the daemon open `MOSTRO_NSEC_PRIVKEY`, and the settings directory also holds `mostro.db` and, in the Docker flows, the LND credentials under `lnd/`. Keep the directory at mode `0700` and the file at `0600`. -- Every path that creates them does so already, through one primitive: - `src/config/permissions.rs`, `fn create_settings_dir` for the directory and - `fn create_owner_only` for the file. Its three callers are the non-interactive template - copy (`src/config/util.rs`, `fn init_configuration_file`), the manual template copy +- Every path that creates them does so already, through one of three primitives in + `src/config/permissions.rs`: `fn create_settings_dir` for the directory, + `fn create_owner_only` for a file that must not already exist, and + `fn write_owner_only_atomic` for one that is rewritten. A directory or file created + outside the daemon — `mkdir`, `cp`, `curl` — inherits the umask instead, so the + deployment guides use `install -d -m 700` and `install -m 600`. +- `create_owner_only`'s three callers are the non-interactive template copy + (`src/config/util.rs`, `fn init_configuration_file`), the manual template copy (`src/config/wizard.rs`, `fn run_setup_menu`) and the guided wizard save - (`src/config/wizard.rs`, `fn save_settings`). A directory or file created outside the - daemon — `mkdir`, `cp`, `curl` — inherits the umask instead, so the deployment guides - use `install -d -m 700` and `install -m 600`. + (`src/config/wizard.rs`, `fn save_settings`). `write_owner_only_atomic` has one: + the wizard's `.env` write (`src/config/wizard.rs`, `fn write_env_file`). - An existing settings directory is left as it is, so a deliberately group-readable - deployment keeps working. + deployment keeps working. `0700` also applies to the settings directory alone — any + missing parents are created under the umask, the way `mkdir -p` would, so + `mostrod -d /srv/apps/mostro/conf` on a fresh tree closes off `conf` without closing + off `/srv/apps` for anything else that lives there. - `create_owner_only` uses `O_CREAT | O_EXCL`, so it fails rather than write through whatever already occupies the path. This covers initial creation only: on a settings directory another local account can write to, a symlink planted between the caller's existence check and the create would otherwise have its target truncated and its mode - reset to `0600`. + reset to `0600`. A write that fails partway removes the file it created, so a full + disk cannot leave a truncated `settings.toml` behind for the next boot to reject as + malformed TOML. +- `write_env_file` cannot refuse an existing path the way `create_owner_only` does — + rewriting an existing `.env` is a supported thing to do — so + `write_owner_only_atomic` stages the line in a fresh `O_EXCL` temporary beside the + target and `rename`s it into place. `rename` never opens the destination, so a + planted symlink is replaced rather than followed and its target keeps both its + contents and its mode. `.env` matters here as much as `settings.toml`: it carries the + same `nsec_privkey`, and in the wizard flow it is written first. - It is not a check on startup as a whole. A `settings.toml` that already exists is read and loaded normally, symlink or not — `fn init_configuration_file` only reaches the creation path when it finds no settings file at all. -- The guarantee covers the final entry, not the directory path leading to it. - `fn create_settings_dir` uses a recursive `DirBuilder`, which resolves symlinked - components on the way, so a settings directory reached through a symlinked parent is - created at whatever that link points to. This is deliberate: operators do symlink a - config directory onto another volume, and planting such a link on the default - `~/.mostro` path means write access to `$HOME`, which is a compromise of the account - already. A settings directory that is itself a symlink to an existing directory is used - as-is, and a dangling one is refused. +- The guarantee covers the final entry, not the directory path leading to it. The + parents `fn create_settings_dir` has to invent are made with a recursive + `DirBuilder`, which resolves symlinked components on the way, so a settings directory + reached through a symlinked parent is created at whatever that link points to. This is + deliberate: operators do symlink a config directory onto another volume, and planting + such a link on the default `~/.mostro` path means write access to `$HOME`, which is a + compromise of the account already. A settings directory that is itself a symlink to an + existing directory is used as-is, and a dangling one is refused. diff --git a/src/cli.rs b/src/cli.rs index a79df533..f6463d9f 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -4,6 +4,7 @@ use crate::config::util::init_configuration_file; use clap::Parser; +use std::path::PathBuf; #[derive(Parser)] #[command( @@ -33,20 +34,22 @@ pub struct Cli { /// Default folder is HOME but user can specify a custom folder with dirsettings (-d ) parameter from CLI /// Example: mostro p2p -d /user_folder/mostro /// -pub fn settings_init() -> Result<(), Box> { +/// Returns the settings directory in use, which the caller needs to check the +/// permissions of the secret-bearing files inside it. +pub fn settings_init() -> Result> { // Parse CLI arguments let cli = Cli::parse(); // Select config file from CLI or default to HOME/.mostro // create config file if it doesn't exist - if let Some(path) = cli.dirsettings.as_deref() { + let settings_dir = if let Some(path) = cli.dirsettings.as_deref() { init_configuration_file(Some(path.to_string()))? } else { init_configuration_file(None)? }; // Mostro settings are initialized - Ok(()) + Ok(settings_dir) } #[cfg(test)] @@ -123,7 +126,7 @@ mod tests { // In a real implementation, we would need dependency injection for testing // Test that the function signature is correct - let _: fn() -> Result<(), Box> = settings_init; + let _: fn() -> Result> = settings_init; // Verify function exists and has correct return type // No-op: type check above is sufficient diff --git a/src/config/constants.rs b/src/config/constants.rs index ce5d3b5f..fe53b1b9 100644 --- a/src/config/constants.rs +++ b/src/config/constants.rs @@ -27,6 +27,9 @@ pub const NOSTR_EXCHANGE_RATES_EVENT_KIND: u16 = 30078; /// startup. Shared between the wizard (writes it) and the loader (reads it). pub const ENV_FILENAME: &str = ".env"; +/// Name of the settings file inside the settings directory. +pub const SETTINGS_FILENAME: &str = "settings.toml"; + /// Environment variable name used to override the Nostr private key from the /// process environment. Shared between the wizard and the loader. pub const NSEC_ENV_VAR: &str = "MOSTRO_NSEC_PRIVKEY"; diff --git a/src/config/mod.rs b/src/config/mod.rs index 529cc075..4661325f 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -3,6 +3,8 @@ pub mod constants; pub mod permissions; pub mod secret; pub mod settings; +#[cfg(test)] +pub(crate) mod test_support; /// This module provides functionality to manage and initialize settings for the Mostro application. /// It includes structures for database, lightning, Nostr, and Mostro settings, as well as functions to initialize and access these settings. pub mod types; diff --git a/src/config/permissions.rs b/src/config/permissions.rs index 25b920a7..2a31c543 100644 --- a/src/config/permissions.rs +++ b/src/config/permissions.rs @@ -1,20 +1,29 @@ //! Filesystem permissions of the files mostrod owns or reads: the startup -//! check on the credential files named by the settings, and the primitives -//! that create the settings directory and `settings.toml` owner-only. +//! check on the credential and secret files named by the settings, and the +//! primitives that create the settings directory, `settings.toml` and `.env` +//! owner-only. //! //! Kept in its own module — rather than folded into `config::util` — because //! these are about the files themselves, not about loading the settings. Both //! `config::util` (non-interactive template copy) and `config::wizard` (manual -//! template copy, guided wizard save) create `settings.toml` through the same -//! primitive here, so a single place decides how that file comes into being. +//! template copy, guided wizard save, `.env` write) bring those files into +//! existence through the primitives here, so a single place decides how they +//! come into being. use mostro_core::error::MostroError::{self, MostroInternalErr}; use mostro_core::error::ServiceError; +use std::ffi::{OsStr, OsString}; use std::fs; -use std::path::Path; +use std::path::{Path, PathBuf}; -/// Warn when a credential file has any of its "other" permission bits set, -/// which puts it within reach of every account on the host. +/// How many temporary sibling names `write_owner_only_atomic` tries before it +/// gives up. Each attempt only fails when the name is already taken, so a +/// handful is plenty; the bound is there so a directory seeded with every +/// candidate name is an error rather than a hang. +const TEMP_NAME_ATTEMPTS: u32 = 16; + +/// Warn when a file holding a secret has any of its "other" permission bits +/// set, which puts it within reach of every account on the host. /// /// The check is advisory: a node whose macaroon is `0644` still starts, it /// just says so out loud once per boot. Refusing to start would turn a @@ -24,20 +33,24 @@ use std::path::Path; /// Empty paths and files that cannot be stat'ed are ignored: an unset or /// unreadable path is not a permissions problem, and the real failure is /// reported with far more context by whoever opens the file (for the macaroon, -/// `LndConnector::new`). -pub fn warn_if_other_accessible(path: &str, label: &str) { - if path.is_empty() { +/// `LndConnector::new`). `/.env` is optional and usually absent, +/// so its being missing has to stay silent. +pub fn warn_if_other_accessible(path: &Path, label: &str) { + if path.as_os_str().is_empty() { return; } - if let Some(mode) = other_accessible_mode(Path::new(path)) { + if let Some(mode) = other_accessible_mode(path) { // `chmod o=` rather than `chmod 600`: the file may legitimately be - // owned by the LND account and read by mostrod through the node's - // group, and following advice that drops the group bits would leave - // the daemon unable to authenticate on its next restart. + // owned by another account and read by mostrod through a shared group + // — the LND macaroon through the node's group is the documented case — + // and following advice that drops the group bits would leave the + // daemon unable to authenticate on its next restart. tracing::warn!( - "{label} ({path}) has permissions {mode:04o}: its \"other\" bits are set, so every \ - account on this host can reach it. Clear them with: chmod o= {path}" + "{label} ({}) has permissions {mode:04o}: its \"other\" bits are set, so every \ + account on this host can reach it. Clear them with: chmod o= {}", + path.display(), + path.display() ); } } @@ -61,7 +74,7 @@ pub fn warn_if_other_accessible(path: &str, label: &str) { fn other_accessible_mode(path: &Path) -> Option { use std::os::unix::fs::PermissionsExt; - let mode = std::fs::metadata(path).ok()?.permissions().mode() & 0o777; + let mode = fs::metadata(path).ok()?.permissions().mode() & 0o777; (mode & 0o007 != 0).then_some(mode) } @@ -78,18 +91,36 @@ fn other_accessible_mode(_path: &Path) -> Option { /// `lnd/`. `create_dir_all` would apply the process umask instead, which on a /// typical host leaves the directory at `0755`. /// -/// Only directories this call creates are affected: an operator who already set -/// up the directory with deliberate group access keeps it. +/// `0700` applies to the settings directory itself and to nothing else. Any +/// missing parents are created under the umask, the way `mkdir -p` would: with +/// `mostrod -d /srv/apps/mostro/conf` on a fresh tree, only `conf` is closed +/// off, while `/srv/apps` and `/srv/apps/mostro` stay reachable by whatever +/// else lives under them. /// -/// The recursive builder resolves symlinked components on the way, unlike -/// `create_owner_only` for the final file. That is deliberate: symlinking a -/// config directory onto another volume is a legitimate setup, and planting a -/// link on the default `~/.mostro` path takes write access to `$HOME`, which -/// already owns the account. A settings directory that is itself a symlink to -/// an existing directory never reaches here — the caller finds it and uses it. +/// An existing settings directory is returned as it is, so an operator who +/// already set one up with deliberate group access keeps it. +/// +/// The parents are created with a recursive `DirBuilder`, which resolves +/// symlinked components on the way. That is deliberate: symlinking a config +/// directory onto another volume is a legitimate setup, and planting a link on +/// the default `~/.mostro` path takes write access to `$HOME`, which already +/// owns the account. A settings directory that is itself a symlink to an +/// existing directory never reaches here — the caller finds it and uses it. pub(crate) fn create_settings_dir(settings_dir: &Path) -> Result<(), MostroError> { + if settings_dir.is_dir() { + return Ok(()); + } + + if let Some(parent) = settings_dir.parent() { + if !parent.as_os_str().is_empty() && !parent.is_dir() { + fs::create_dir_all(parent) + .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + } + } + + // Non-recursive on purpose: the mode below must reach the settings + // directory and no ancestor, and `create_dir_all` has no way to say that. let mut builder = fs::DirBuilder::new(); - builder.recursive(true); #[cfg(unix)] { use std::os::unix::fs::DirBuilderExt; @@ -114,12 +145,131 @@ pub(crate) fn create_settings_dir(settings_dir: &Path) -> Result<(), MostroError /// write to, a symlink planted in between would otherwise have its target /// truncated and its mode reset to `0600` by the two steps below. /// -/// `OpenOptionsExt::mode` is also masked by the process umask, so the mode is -/// set again through the file descriptor — never through the path, which would -/// reintroduce the symlink it just refused to follow. +/// A write that fails partway takes the file with it. What is left behind +/// otherwise is a truncated `settings.toml` that the next boot parses and +/// rejects as malformed TOML instead of recreating — the file was created with +/// `O_EXCL` a moment earlier, so removing it cannot touch anything else. pub(crate) fn create_owner_only(path: &Path, contents: &[u8]) -> Result<(), MostroError> { use std::io::Write; + let mut file = open_owner_only_new(path).map_err(|e| { + MostroInternalErr(ServiceError::IOError(format!( + "Could not create {}: {}", + path.display(), + e + ))) + })?; + + let written = file.write_all(contents); + // Closed before the cleanup below, which Windows would refuse on an open + // handle. + drop(file); + + written.map_err(|e| { + let _ = fs::remove_file(path); + MostroInternalErr(ServiceError::IOError(format!( + "Could not write {}: {}", + path.display(), + e + ))) + }) +} + +/// Replace `path` with `contents`, owner-only (`0600` on Unix), atomically. +/// +/// For the files mostrod rewrites rather than creates once — today +/// `/.env`, which carries the same `nsec_privkey` as +/// `settings.toml`. [`create_owner_only`] cannot serve them: it refuses a path +/// that already exists, which is exactly what a rewrite has to do. +/// +/// The contents go to a fresh temporary file in the same directory, created +/// with `O_CREAT | O_EXCL` and chmod'ed through its descriptor, and are then +/// moved onto `path` with `rename`. That buys two things at once: +/// +/// - `rename` replaces whatever `path` names without ever opening it, so a +/// symlink another local account planted there is unlinked rather than +/// written through — its target keeps both its contents and its mode. +/// `create_owner_only` refuses in that situation; here refusing is not an +/// option, and replacing the link gives the target the same protection. A +/// deliberately symlinked `.env` is not a supported setup: it is a file the +/// daemon writes, in a directory it created `0700`. +/// - The file at `path` is never observed half-written. A `.env` truncated by +/// a full disk would otherwise leave the daemon with no `nsec_privkey` at +/// all on the next boot. +pub(crate) fn write_owner_only_atomic(path: &Path, contents: &[u8]) -> Result<(), MostroError> { + use std::io::Write; + + let dir = match path.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent, + _ => Path::new("."), + }; + let file_name = path.file_name().ok_or_else(|| { + MostroInternalErr(ServiceError::IOError(format!( + "{} does not name a file", + path.display() + ))) + })?; + + let (temp_path, mut temp_file) = create_temp_sibling(dir, file_name)?; + + // The temporary is only ever left behind on a failure, and never with the + // secret still in it. + let staged = temp_file + .write_all(contents) + .and_then(|()| temp_file.sync_all()); + drop(temp_file); + + if let Err(e) = staged.and_then(|()| fs::rename(&temp_path, path)) { + let _ = fs::remove_file(&temp_path); + return Err(MostroInternalErr(ServiceError::IOError(format!( + "Could not write {}: {}", + path.display(), + e + )))); + } + + Ok(()) +} + +/// Create an owner-only temporary file next to the target and return it with +/// its path. +/// +/// `O_EXCL` again, so a stale temporary left behind by a killed run — or one +/// planted deliberately — is never written through; the suffix is bumped until +/// a free name is found. The temporary is a sibling rather than something +/// under `/tmp` because `rename` only works within a filesystem, and because +/// the settings directory is already `0700`. +fn create_temp_sibling(dir: &Path, file_name: &OsStr) -> Result<(PathBuf, fs::File), MostroError> { + let mut last_error = None; + + for attempt in 0..TEMP_NAME_ATTEMPTS { + let mut name = OsString::from("."); + name.push(file_name); + name.push(format!(".tmp-{}-{attempt}", std::process::id())); + let candidate = dir.join(name); + + match open_owner_only_new(&candidate) { + Ok(file) => return Ok((candidate, file)), + Err(e) => last_error = Some(e), + } + } + + Err(MostroInternalErr(ServiceError::IOError(format!( + "Could not create a temporary file in {} after {TEMP_NAME_ATTEMPTS} attempts: {}", + dir.display(), + last_error + .map(|e| e.to_string()) + .unwrap_or_else(|| "unknown error".to_string()) + )))) +} + +/// Open a brand-new file owner-only, failing if anything already occupies the +/// path. +/// +/// `OpenOptionsExt::mode` is masked by the process umask, so the mode is set +/// again through the file descriptor — never through the path, which would +/// reintroduce the symlink `create_new` just refused to follow. +fn open_owner_only_new(path: &Path) -> std::io::Result { #[cfg(unix)] let file = { use std::os::unix::fs::OpenOptionsExt; @@ -127,47 +277,32 @@ pub(crate) fn create_owner_only(path: &Path, contents: &[u8]) -> Result<(), Most .write(true) .create_new(true) .mode(0o600) - .open(path) + .open(path)? }; #[cfg(not(unix))] let file = fs::OpenOptions::new() .write(true) .create_new(true) - .open(path); - let mut file = file.map_err(|e| { - MostroInternalErr(ServiceError::IOError(format!( - "Could not create {}: {}", - path.display(), - e - ))) - })?; + .open(path)?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - file.set_permissions(fs::Permissions::from_mode(0o600)) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; + file.set_permissions(fs::Permissions::from_mode(0o600))?; } - file.write_all(contents) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string()))) + Ok(file) } #[cfg(all(test, unix))] mod tests { use super::*; + use crate::config::test_support::temp_dir; use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; - fn temp_dir(tag: &str) -> PathBuf { - let dir = - std::env::temp_dir().join(format!("mostro-permissions-{tag}-{}", std::process::id())); - std::fs::create_dir_all(&dir).expect("create temp dir"); - dir - } - fn macaroon_with_mode(tag: &str, mode: u32) -> PathBuf { - let path = temp_dir(tag).join("admin.macaroon"); + let path = temp_dir("permissions", tag).join("admin.macaroon"); std::fs::write(&path, b"macaroon").expect("write macaroon"); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)) .expect("set permissions"); @@ -210,14 +345,13 @@ mod tests { #[test] fn symlink_reports_the_target_mode() { - let dir = temp_dir("symlink"); + let dir = temp_dir("permissions", "symlink"); let target = dir.join("real.macaroon"); std::fs::write(&target, b"macaroon").expect("write macaroon"); std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)) .expect("set permissions"); let link = dir.join("linked.macaroon"); - let _ = std::fs::remove_file(&link); std::os::unix::fs::symlink(&target, &link).expect("create symlink"); assert_eq!(other_accessible_mode(&link), Some(0o644)); @@ -233,30 +367,21 @@ mod tests { #[test] fn empty_and_missing_paths_do_not_panic() { - warn_if_other_accessible("", "LND admin macaroon"); - warn_if_other_accessible("/definitely/not/here.macaroon", "LND admin macaroon"); + warn_if_other_accessible(Path::new(""), "LND admin macaroon"); + warn_if_other_accessible( + Path::new("/definitely/not/here.macaroon"), + "Mostro env file", + ); } } -#[cfg(all(test, unix))] +#[cfg(test)] mod owner_only_tests { use super::*; - use std::os::unix::fs::PermissionsExt; - - fn temp_root(tag: &str) -> std::path::PathBuf { - let dir = - std::env::temp_dir().join(format!("mostro-owner-only-{tag}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).expect("create temp dir"); - dir - } + use crate::config::test_support::{assert_mode, mode_of, set_mode, temp_dir}; - fn mode_of(path: &std::path::Path) -> u32 { - std::fs::metadata(path) - .expect("metadata") - .permissions() - .mode() - & 0o777 + fn temp_root(tag: &str) -> PathBuf { + temp_dir("owner-only", tag) } #[test] @@ -264,16 +389,31 @@ mod owner_only_tests { let root = temp_root("dir"); let settings_dir = root.join(".mostro"); create_settings_dir(&settings_dir).expect("create settings dir"); - assert_eq!(mode_of(&settings_dir), 0o700); + assert_mode(&settings_dir, 0o700); } #[test] - fn settings_dir_creation_is_recursive_and_owner_only() { + fn settings_dir_creation_closes_off_the_leaf_but_not_its_ancestors() { let root = temp_root("dir-nested"); - let settings_dir = root.join("nested").join(".mostro"); + let settings_dir = root.join("apps").join("mostro").join("conf"); create_settings_dir(&settings_dir).expect("create nested settings dir"); - assert_eq!(mode_of(&settings_dir), 0o700); - assert_eq!(mode_of(&root.join("nested")), 0o700); + assert_mode(&settings_dir, 0o700); + + // The parents `mkdir -p` had to invent keep whatever the umask gives + // them, so `mostrod -d /srv/apps/mostro/conf` does not close off + // `/srv/apps` for everything else that lives under it. Compared + // against a reference tree rather than a literal mode, because the + // umask is the environment's to choose. + let reference = root.join("reference").join("inner"); + std::fs::create_dir_all(&reference).expect("create reference tree"); + assert_eq!( + mode_of(&root.join("apps")), + mode_of(&root.join("reference")) + ); + assert_eq!( + mode_of(&root.join("apps").join("mostro")), + mode_of(&reference) + ); } #[test] @@ -281,12 +421,11 @@ mod owner_only_tests { let root = temp_root("dir-existing"); let settings_dir = root.join(".mostro"); std::fs::create_dir(&settings_dir).expect("create settings dir"); - std::fs::set_permissions(&settings_dir, std::fs::Permissions::from_mode(0o750)) - .expect("loosen permissions"); - // `recursive(true)` makes this a no-op rather than an error, and a - // deliberate group-readable directory must survive it. + set_mode(&settings_dir, 0o750); + // A deliberate group-readable directory must survive this rather than + // be tightened or reported as an error. create_settings_dir(&settings_dir).expect("existing directory is not an error"); - assert_eq!(mode_of(&settings_dir), 0o750); + assert_mode(&settings_dir, 0o750); } #[test] @@ -294,7 +433,7 @@ mod owner_only_tests { let root = temp_root("file"); let config_file = root.join("settings.toml"); create_owner_only(&config_file, b"nsec_privkey = 'nsec1...'\n").expect("write template"); - assert_eq!(mode_of(&config_file), 0o600); + assert_mode(&config_file, 0o600); assert_eq!( std::fs::read_to_string(&config_file).expect("read back"), "nsec_privkey = 'nsec1...'\n" @@ -306,24 +445,101 @@ mod owner_only_tests { let root = temp_root("file-existing"); let config_file = root.join("settings.toml"); std::fs::write(&config_file, "operator contents").expect("seed file"); - std::fs::set_permissions(&config_file, std::fs::Permissions::from_mode(0o644)) - .expect("loosen permissions"); + set_mode(&config_file, 0o644); assert!(create_owner_only(&config_file, b"fresh\n").is_err()); // Neither the contents nor the mode of what was already there change. assert_eq!( std::fs::read_to_string(&config_file).expect("read back"), "operator contents" ); - assert_eq!(mode_of(&config_file), 0o644); + assert_mode(&config_file, 0o644); } #[test] - fn a_symlink_in_the_settings_path_is_refused_and_its_target_untouched() { - let root = temp_root("file-symlink"); + fn writing_to_an_unwritable_path_is_an_error() { + let root = temp_root("file-error"); + // A directory cannot be opened for writing, so this exercises the + // error branch instead of silently succeeding. + let config_file = root.join("settings.toml"); + std::fs::create_dir(&config_file).expect("create dir in the file's place"); + assert!(create_owner_only(&config_file, b"x").is_err()); + } + + #[test] + fn atomic_write_creates_a_missing_file_owner_only() { + let root = temp_root("atomic-new"); + let env_file = root.join(".env"); + write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1...\n") + .expect("write env file"); + assert_eq!( + std::fs::read_to_string(&env_file).expect("read back"), + "MOSTRO_NSEC_PRIVKEY=nsec1...\n" + ); + assert_mode(&env_file, 0o600); + } + + #[test] + fn atomic_write_replaces_an_existing_file_and_tightens_its_mode() { + let root = temp_root("atomic-existing"); + let env_file = root.join(".env"); + std::fs::write(&env_file, "OLD=stale\n").expect("seed env file"); + set_mode(&env_file, 0o644); + + write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1replaced\n") + .expect("rewrite env file"); + + assert_eq!( + std::fs::read_to_string(&env_file).expect("read back"), + "MOSTRO_NSEC_PRIVKEY=nsec1replaced\n" + ); + assert_mode(&env_file, 0o600); + } + + #[test] + fn atomic_write_leaves_no_temporary_behind() { + let root = temp_root("atomic-clean"); + let env_file = root.join(".env"); + write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1...\n").expect("write"); + + let leftovers: Vec<_> = std::fs::read_dir(&root) + .expect("read dir") + .map(|entry| entry.expect("dir entry").file_name()) + .filter(|name| name != ".env") + .collect(); + assert!( + leftovers.is_empty(), + "the temporary must be renamed away, found {leftovers:?}" + ); + } + + #[test] + fn atomic_write_to_an_unwritable_path_is_an_error() { + let root = temp_root("atomic-error"); + let env_file = root.join(".env"); + std::fs::create_dir(&env_file).expect("create dir in the file's place"); + // `rename` cannot replace a non-empty directory, so this must not + // report success. + std::fs::write(env_file.join("occupied"), b"x").expect("occupy the directory"); + assert!(write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1...\n").is_err()); + } +} + +#[cfg(all(test, unix))] +mod symlink_tests { + use super::*; + use crate::config::test_support::{assert_mode, set_mode, temp_dir}; + + fn victim_in(root: &Path) -> PathBuf { let victim = root.join("victim"); std::fs::write(&victim, "victim contents").expect("seed victim"); - std::fs::set_permissions(&victim, std::fs::Permissions::from_mode(0o644)) - .expect("set victim mode"); + set_mode(&victim, 0o644); + victim + } + + #[test] + fn a_symlink_in_the_settings_path_is_refused_and_its_target_untouched() { + let root = temp_dir("owner-only", "file-symlink"); + let victim = victim_in(&root); // What another local account could plant in a settings directory it can // write to, in the window between the caller's existence check and this @@ -336,7 +552,7 @@ mod owner_only_tests { std::fs::read_to_string(&victim).expect("read back"), "victim contents" ); - assert_eq!(mode_of(&victim), 0o644); + assert_mode(&victim, 0o644); // The symlink itself is left in place; nothing was written through it. assert!(std::fs::symlink_metadata(&config_file) .expect("symlink metadata") @@ -346,7 +562,7 @@ mod owner_only_tests { #[test] fn a_dangling_symlink_is_refused_rather_than_created_through() { - let root = temp_root("file-dangling"); + let root = temp_dir("owner-only", "file-dangling"); let config_file = root.join("settings.toml"); // `Path::exists` follows symlinks, so the caller's check reports false // here and this call is what has to refuse. @@ -358,12 +574,50 @@ mod owner_only_tests { } #[test] - fn writing_to_an_unwritable_path_is_an_error() { - let root = temp_root("file-error"); - // A directory cannot be opened for writing, so this exercises the - // error branch instead of silently succeeding. - let config_file = root.join("settings.toml"); - std::fs::create_dir(&config_file).expect("create dir in the file's place"); - assert!(create_owner_only(&config_file, b"x").is_err()); + fn atomic_write_replaces_a_planted_symlink_instead_of_following_it() { + let root = temp_dir("owner-only", "atomic-symlink"); + let victim = victim_in(&root); + + let env_file = root.join(".env"); + std::os::unix::fs::symlink(&victim, &env_file).expect("plant symlink"); + + write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1...\n") + .expect("write env file"); + + // The nsec landed in a regular file that replaced the link, and the + // target kept both its contents and its mode. + assert_eq!( + std::fs::read_to_string(&victim).expect("read victim"), + "victim contents" + ); + assert_mode(&victim, 0o644); + assert!(std::fs::symlink_metadata(&env_file) + .expect("symlink metadata") + .file_type() + .is_file()); + assert_mode(&env_file, 0o600); + } + + #[test] + fn atomic_write_steps_over_a_stale_temporary() { + let root = temp_dir("owner-only", "atomic-stale"); + let env_file = root.join(".env"); + // What a run killed mid-write leaves behind. Writing through it would + // be harmless here, but the same name could just as well be a symlink + // another account planted, so the first free suffix is used instead. + let stale = root.join(format!(".env.tmp-{}-0", std::process::id())); + std::fs::write(&stale, "stale").expect("seed stale temporary"); + + write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1...\n").expect("write"); + + assert_eq!( + std::fs::read_to_string(&env_file).expect("read back"), + "MOSTRO_NSEC_PRIVKEY=nsec1...\n" + ); + assert_eq!( + std::fs::read_to_string(&stale).expect("read stale"), + "stale", + "the stale temporary must not be written through" + ); } } diff --git a/src/config/test_support.rs b/src/config/test_support.rs new file mode 100644 index 00000000..98c2e8f2 --- /dev/null +++ b/src/config/test_support.rs @@ -0,0 +1,90 @@ +//! Shared helpers for the tests in this module that touch the filesystem. +//! +//! Collected here rather than repeated per test module so that how a test +//! temporary directory comes into being is decided in one place. These tests +//! are about permissions, and on a shared CI host `/tmp` is world-writable, so +//! the answer matters: a predictable name is fine, running inside a directory +//! this process did not create is not. + +use std::path::{Path, PathBuf}; + +/// A fresh, owner-only temporary directory named after the calling module and +/// a per-test tag. +/// +/// The name stays deterministic (module, tag and the pid) so repeated runs +/// reuse the same path instead of piling up under `/tmp`. Creation is a plain +/// non-recursive `mkdir`, which fails if anything already occupies the path — +/// including a symlink another local account planted between the cleanup below +/// and this call. Failing the test is the point: the alternative is a +/// permissions test that quietly runs inside someone else's directory. +pub(crate) fn temp_dir(module: &str, tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("mostro-{module}-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + + let mut builder = std::fs::DirBuilder::new(); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } + builder.create(&dir).unwrap_or_else(|e| { + panic!( + "could not create the test directory {}: {e}. It is never reused when something \ + already occupies the path — remove whatever is there and rerun.", + dir.display() + ) + }); + dir +} + +/// The path's permission bits, or `None` on platforms that have none. +/// +/// `Option` rather than `u32` so assertions compile everywhere and the tests +/// that only care about content stay portable; the mode assertions read +/// `Some(0o600)`. +#[cfg(unix)] +pub(crate) fn mode_of(path: &Path) -> Option { + use std::os::unix::fs::PermissionsExt; + + let metadata = + std::fs::metadata(path).unwrap_or_else(|e| panic!("stat {}: {e}", path.display())); + Some(metadata.permissions().mode() & 0o777) +} + +/// Non-Unix platforms have no POSIX permission bits to report. +#[cfg(not(unix))] +pub(crate) fn mode_of(_path: &Path) -> Option { + None +} + +/// Assert a path's permission bits, where the platform has any. +/// +/// A no-op elsewhere, so the tests around it — that a file is created, refused +/// or replaced — stay portable instead of being compiled only on Unix. +#[cfg(unix)] +pub(crate) fn assert_mode(path: &Path, expected: u32) { + assert_eq!( + mode_of(path), + Some(expected), + "unexpected mode on {}", + path.display() + ); +} + +/// Non-Unix platforms have no POSIX permission bits to assert on. +#[cfg(not(unix))] +pub(crate) fn assert_mode(_path: &Path, _expected: u32) {} + +/// Loosen (or otherwise set) a path's permission bits, where the platform has +/// any. A no-op elsewhere. +#[cfg(unix)] +pub(crate) fn set_mode(path: &Path, mode: u32) { + use std::os::unix::fs::PermissionsExt; + + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)) + .unwrap_or_else(|e| panic!("set mode {mode:04o} on {}: {e}", path.display())); +} + +/// Non-Unix platforms have no POSIX permission bits to set. +#[cfg(not(unix))] +pub(crate) fn set_mode(_path: &Path, _mode: u32) {} diff --git a/src/config/util.rs b/src/config/util.rs index fd4ad211..1868cdaa 100644 --- a/src/config/util.rs +++ b/src/config/util.rs @@ -2,7 +2,9 @@ /// This module provides utility functions for the config module. /// It includes functions to initialize the default settings directory and create a settings file from the template if it doesn't exist. /// It also includes functions to add a trailing slash to a path if it doesn't already have one. -use crate::config::constants::{ENV_FILENAME, MAX_DEV_FEE_PERCENTAGE, MIN_DEV_FEE_PERCENTAGE}; +use crate::config::constants::{ + ENV_FILENAME, MAX_DEV_FEE_PERCENTAGE, MIN_DEV_FEE_PERCENTAGE, SETTINGS_FILENAME, +}; use crate::config::permissions::{create_owner_only, create_settings_dir}; use crate::config::secret::read_nsec_env_var; use crate::config::wizard; @@ -133,7 +135,13 @@ fn validate_cashu_settings( /// Initialize the default settings directory and create a settings file from the template if it doesn't exist. /// Checks if the directory already exists, and if not, creates it and writes the template file. /// If a custom config path is provided, it uses that instead of the default `~/.mostro` directory. -pub fn init_configuration_file(config_path: Option) -> Result<(), MostroError> { +/// +/// Returns the settings directory that was used. The caller needs it because +/// the files in there — `settings.toml` and the optional `.env`, both carrying +/// `nsec_privkey` in plaintext — are checked for over-broad permissions at +/// startup, and the path is otherwise not recoverable from the loaded +/// `Settings`. +pub fn init_configuration_file(config_path: Option) -> Result { let settings_dir = if let Some(user_path) = config_path { PathBuf::from(user_path) } else { @@ -155,7 +163,7 @@ pub fn init_configuration_file(config_path: Option) -> Result<(), Mostro // overrides) can be read from it. Real env vars keep precedence. load_env_file(&settings_dir); - let config_file_path = settings_dir.join("settings.toml"); + let config_file_path = settings_dir.join(SETTINGS_FILENAME); if !config_file_path.exists() { let mut settings = if std::io::stdin().is_terminal() { @@ -175,7 +183,7 @@ pub fn init_configuration_file(config_path: Option) -> Result<(), Mostro validate_mostro_settings(&settings)?; init_mostro_settings(settings)?; tracing::info!("Settings correctly loaded!"); - return Ok(()); + return Ok(settings_dir); } // Read the file content into a zeroizing buffer so TOML plaintext is wiped @@ -204,7 +212,7 @@ pub fn init_configuration_file(config_path: Option) -> Result<(), Mostro tracing::info!("Settings correctly loaded!"); - Ok(()) + Ok(settings_dir) } #[cfg(test)] @@ -496,11 +504,7 @@ mod env_file_tests { use super::*; fn temp_dir(tag: &str) -> std::path::PathBuf { - let dir = - std::env::temp_dir().join(format!("mostro-config-util-{tag}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).expect("create temp dir"); - dir + crate::config::test_support::temp_dir("config-util", tag) } #[test] @@ -541,11 +545,7 @@ mod init_configuration_file_tests { use super::*; fn temp_config_dir(tag: &str) -> std::path::PathBuf { - let dir = - std::env::temp_dir().join(format!("mostro-init-config-{tag}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).expect("create temp dir"); - dir + crate::config::test_support::temp_dir("init-config", tag) } // NOTE: the success path (valid settings.toml) calls diff --git a/src/config/wizard.rs b/src/config/wizard.rs index cf918a24..db2acb72 100644 --- a/src/config/wizard.rs +++ b/src/config/wizard.rs @@ -1,4 +1,3 @@ -use std::io::Write; use std::path::{Path, PathBuf}; use dialoguer::{Confirm, Input, Password, Select}; @@ -9,7 +8,7 @@ use secrecy::{ExposeSecret, SecretString}; use zeroize::Zeroizing; use super::constants::{ENV_FILENAME, NSEC_ENV_VAR}; -use super::permissions::create_owner_only; +use super::permissions::{create_owner_only, write_owner_only_atomic}; use super::settings::Settings; use super::types::{ DatabaseSettings, LightningSettings, MostroSettings, NostrSettings, RpcSettings, @@ -244,43 +243,24 @@ fn save_settings(config_file_path: &Path, settings: &Settings) -> Result<(), Mos create_owner_only(config_file_path, toml_content.as_bytes()) } -/// Write `MOSTRO_NSEC_PRIVKEY=` to the given path with 0o600 permissions on Unix. +/// Write `MOSTRO_NSEC_PRIVKEY=` to the given path, owner-only (`0600` on +/// Unix), replacing whatever is already there. /// -/// `OpenOptionsExt::mode(0o600)` only applies when the file is created, so for -/// preexisting files we must explicitly tighten permissions after opening to -/// avoid leaving a previously-broader mode in place. +/// Goes through `write_owner_only_atomic` rather than opening the path +/// directly. `.env` holds the same `nsec_privkey` as `settings.toml` and, in +/// the wizard flow, is written first — so opening it with +/// `create(true).truncate(true)` would follow a symlink another local account +/// planted in the settings directory, truncating its target and resetting it +/// to `0600`. That is the one thing `create_owner_only` exists to prevent for +/// `settings.toml`, and this file is worth exactly as much. `create_owner_only` +/// itself cannot serve here: it refuses a path that already exists, and +/// rewriting an existing `.env` is a supported thing to do. +/// +/// The line goes through a `Zeroizing` buffer so the plaintext nsec is wiped +/// once handed off, the way `save_settings` treats the serialized TOML. fn write_env_file(path: &Path, nsec: &str) -> Result<(), MostroError> { - #[cfg(unix)] - let file = { - use std::os::unix::fs::OpenOptionsExt; - std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .open(path) - }; - #[cfg(not(unix))] - let file = { - std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .open(path) - }; - let mut file = file.map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let permissions = std::fs::Permissions::from_mode(0o600); - file.set_permissions(permissions) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; - } - - writeln!(file, "{}={}", NSEC_ENV_VAR, nsec) - .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?; - Ok(()) + let line = Zeroizing::new(format!("{}={}\n", NSEC_ENV_VAR, nsec)); + write_owner_only_atomic(path, line.as_bytes()) } fn prompt_mostro_settings() -> Result { @@ -375,6 +355,7 @@ fn expand_tilde(path: &str) -> PathBuf { #[cfg(test)] mod tests { use super::*; + use crate::config::test_support::{assert_mode, set_mode}; #[test] fn test_validate_nsec_valid() { @@ -422,10 +403,7 @@ mod tests { } fn temp_dir(tag: &str) -> PathBuf { - let dir = std::env::temp_dir().join(format!("mostro-wizard-{tag}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).expect("create temp dir"); - dir + crate::config::test_support::temp_dir("wizard", tag) } #[test] @@ -459,16 +437,6 @@ mod tests { assert!(resolve_file_path("/definitely/not/here.macaroon").is_err()); } - #[cfg(unix)] - fn mode_of(path: &Path) -> u32 { - use std::os::unix::fs::PermissionsExt; - std::fs::metadata(path) - .expect("stat env file") - .permissions() - .mode() - & 0o777 - } - #[test] fn test_write_env_file_creates_file_with_owner_only_permissions() { let dir = temp_dir("env-new"); @@ -478,8 +446,7 @@ mod tests { let contents = std::fs::read_to_string(&env_path).expect("read env file"); assert_eq!(contents, format!("{}=nsec1testvalue\n", NSEC_ENV_VAR)); - #[cfg(unix)] - assert_eq!(mode_of(&env_path), 0o600); + assert_mode(&env_path, 0o600); } #[test] @@ -487,42 +454,49 @@ mod tests { let dir = temp_dir("env-existing"); let env_path = dir.join(ENV_FILENAME); std::fs::write(&env_path, "OLD=stale\n").expect("seed env file"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&env_path, std::fs::Permissions::from_mode(0o644)) - .expect("loosen permissions"); - } + set_mode(&env_path, 0o644); write_env_file(&env_path, "nsec1replaced").expect("rewrite env file"); - // Old content truncated, permissions tightened back to 0600. + // Old content replaced, permissions tightened back to 0600. let contents = std::fs::read_to_string(&env_path).expect("read env file"); assert_eq!(contents, format!("{}=nsec1replaced\n", NSEC_ENV_VAR)); - #[cfg(unix)] - assert_eq!(mode_of(&env_path), 0o600); + assert_mode(&env_path, 0o600); + } + + #[cfg(unix)] + #[test] + fn test_write_env_file_does_not_write_the_nsec_through_a_planted_symlink() { + let dir = temp_dir("env-symlink"); + let victim = dir.join("victim"); + std::fs::write(&victim, "victim contents").expect("seed victim"); + set_mode(&victim, 0o644); + + // The wizard writes `.env` before `settings.toml`, so on a settings + // directory another local account can write to this is the first shot + // it gets at the nsec. + let env_path = dir.join(ENV_FILENAME); + std::os::unix::fs::symlink(&victim, &env_path).expect("plant symlink"); + + write_env_file(&env_path, "nsec1secret").expect("write env file"); + + assert_eq!( + std::fs::read_to_string(&victim).expect("read victim"), + "victim contents", + "the nsec must not be written through the link" + ); + assert_mode(&victim, 0o644); + assert_mode(&env_path, 0o600); } } -#[cfg(all(test, unix))] +#[cfg(test)] mod save_settings_tests { use super::*; - use std::os::unix::fs::PermissionsExt; + use crate::config::test_support::{assert_mode, set_mode}; fn temp_root(tag: &str) -> PathBuf { - let dir = - std::env::temp_dir().join(format!("mostro-wizard-save-{tag}-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).expect("create temp dir"); - dir - } - - fn mode_of(path: &Path) -> u32 { - std::fs::metadata(path) - .expect("metadata") - .permissions() - .mode() - & 0o777 + crate::config::test_support::temp_dir("wizard-save", tag) } fn sample_settings() -> Settings { @@ -549,7 +523,7 @@ mod save_settings_tests { let root = temp_root("ok"); let config_file = root.join("settings.toml"); save_settings(&config_file, &sample_settings()).expect("save settings"); - assert_eq!(mode_of(&config_file), 0o600); + assert_mode(&config_file, 0o600); let written = std::fs::read_to_string(&config_file).expect("read back"); assert!(written.contains("nsec_privkey")); } @@ -566,13 +540,13 @@ mod save_settings_tests { ); } + #[cfg(unix)] #[test] fn wizard_save_leaves_a_symlink_target_untouched() { let root = temp_root("symlink"); let victim = root.join("victim"); std::fs::write(&victim, "victim contents").expect("seed victim"); - std::fs::set_permissions(&victim, std::fs::Permissions::from_mode(0o644)) - .expect("set victim mode"); + set_mode(&victim, 0o644); let config_file = root.join("settings.toml"); std::os::unix::fs::symlink(&victim, &config_file).expect("plant symlink"); @@ -584,19 +558,24 @@ mod save_settings_tests { std::fs::read_to_string(&victim).expect("read back"), "victim contents" ); - assert_eq!(mode_of(&victim), 0o644); + assert_mode(&victim, 0o644); } #[test] - fn manual_template_copy_is_owner_only_and_refuses_a_symlink() { + fn manual_template_copy_is_owner_only() { let root = temp_root("template"); let config_file = root.join("settings.toml"); // The manual branch of `run_setup_menu` writes TEMPLATE_BYTES through // the same primitive; the menu itself needs a terminal, so exercise // the write it performs. create_owner_only(&config_file, TEMPLATE_BYTES).expect("write template"); - assert_eq!(mode_of(&config_file), 0o600); + assert_mode(&config_file, 0o600); + } + #[cfg(unix)] + #[test] + fn manual_template_copy_refuses_a_symlink() { + let root = temp_root("template-symlink"); let victim = root.join("victim"); std::fs::write(&victim, "victim contents").expect("seed victim"); let linked = root.join("linked.toml"); diff --git a/src/main.rs b/src/main.rs index fcd0aab2..1ef4e2b4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -24,6 +24,7 @@ pub type Result> = std::result::Result; use crate::app::context::AppContext; use crate::app::{run, run_cashu}; use crate::cli::settings_init; +use crate::config::constants::{ENV_FILENAME, SETTINGS_FILENAME}; use crate::config::{ get_db_pool, permissions, Settings, DB_POOL, LN_STATUS, MESSAGE_QUEUES, MOSTRO_CONFIG, NOSTR_CLIENT, @@ -60,7 +61,22 @@ async fn main() -> Result<()> { .init(); // Init MOSTRO_SETTINGS oncelock with all settings variables from TOML file - settings_init()?; + let settings_dir = settings_init()?; + + // `settings.toml` carries `nsec_privkey` in plaintext unless it was moved + // to `/.env`, which carries it instead — either way the key + // that signs every event this instance publishes sits in one of these two + // files. Both are what the deployment guides have operators create by hand + // with `cp`, `curl` or an editor, all of which apply the umask, so a `0644` + // settings file is the normal accident. Checked in both Lightning and + // Cashu mode, unlike the macaroon further down, because the identity is + // not specific to either. + for (name, label) in [ + (SETTINGS_FILENAME, "Mostro settings file"), + (ENV_FILENAME, "Mostro env file"), + ] { + permissions::warn_if_other_accessible(&settings_dir.join(name), label); + } // Build and install the multi-source price manager (spec §9 Phase 1). // Done immediately after settings load so every later subsystem @@ -227,8 +243,9 @@ async fn main() -> Result<()> { // once per boot when the file is left reachable by other local accounts — // the documented deployment flows copy it around, and a bad mode is // otherwise invisible until it is abused. + let macaroon_file = &Settings::get_ln().lnd_macaroon_file; permissions::warn_if_other_accessible( - &Settings::get_ln().lnd_macaroon_file, + std::path::Path::new(macaroon_file), "LND admin macaroon", ); From 0868bcbfa3a7bd6b0a60b4e714dec1f99b850767 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Sat, 22 Aug 2026 21:49:48 -0300 Subject: [PATCH 11/12] fix(docker): refuse to run mostro container as root and clarify config directory ownership Prevent `make docker-up` from running the mostro container as uid 0, which would otherwise happen when `docker/config` is root-owned (e.g., from `sudo make docker-build`). The check stops with an error message directing the operator to either hand the directory to an unprivileged account or set MOSTRO_CONTAINER_USER to a non-zero uid that can read it. --- Makefile | 29 ++++++++++++++++++------ docker/ENV_VARIABLES.md | 1 + docker/README.md | 6 +++-- src/config/permissions.rs | 46 ++++++++++++++++++++++++++++++++++----- 4 files changed, 67 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index cf5da680..73e50ec3 100644 --- a/Makefile +++ b/Makefile @@ -4,9 +4,11 @@ VERSION := $(shell grep "^version = " Cargo.toml | sed "s/version = \"\(.*\)\"/\ # Notes on the docker/config handling below, since none of it is obvious: # # - `install -d -m 700` applies the mode to an existing directory too, so it is -# guarded with `[ -d config ]` outside docker-build. Only the run that first -# creates the directory gets to decide its mode; an operator who deliberately -# opened `config` up to a group keeps it across every later `make docker-up`. +# guarded with `[ -d config ]` everywhere. Only the run that first creates +# the directory gets to decide its mode; an operator who deliberately opened +# `config` up to a group keeps it across every later `make docker-build` or +# `make docker-up`. `config/lnd` is unguarded because docker-build owns it: +# it holds nothing but the credentials this target installs. # - `install -m 600` onto an existing 0644 macaroon is safe as it stands: it # unlinks the destination and creates it with the owner-only bits already # applied, rather than truncating in place and chmod'ing at the end. Measured @@ -14,12 +16,15 @@ VERSION := $(shell grep "^version = " Cargo.toml | sed "s/version = \"\(.*\)\"/\ # - The mostro container has to run as whoever owns `config`: the macaroon # there is 0600 and the daemon also writes mostro.db beside it. Deriving the # uid:gid from the directory covers both the default (docker-build created it -# as you) and the documented `chown -R 1000:1000` handover. +# as you) and the documented `chown -R 1000:1000` handover. uid 0 is refused +# rather than used: a root-owned `config` (a `sudo make docker-build`, say) +# would otherwise drop the unprivileged user the image runs as. docker-build: @set -o pipefail; \ cd docker && \ - install -d -m 700 config config/lnd && \ + { [ -d config ] || install -d -m 700 config; } && \ + install -d -m 700 config/lnd && \ echo "Checking LND files..." && \ echo "LND_CERT_FILE=$${LND_CERT_FILE}" && \ echo "LND_MACAROON_FILE=$${LND_MACAROON_FILE}" && \ @@ -45,9 +50,11 @@ docker-build: install -m 644 "$${LND_CERT_FILE}" config/lnd/tls.cert && \ install -m 600 "$${LND_MACAROON_FILE}" config/lnd/admin.macaroon && \ echo "Wrote config/lnd/tls.cert (mode 644) and config/lnd/admin.macaroon (mode 600)" && \ - echo "config and config/lnd are mode 700: settings.toml holds nsec_privkey and mostro.db lands there too" && \ + echo "config/lnd is mode 700, and config keeps the mode it was created with (700 unless you" && \ + echo "changed it): settings.toml holds nsec_privkey and mostro.db lands there too" && \ echo "The mostro container runs as the owner of docker/config, which make docker-up derives" && \ - echo "and prints. Set MOSTRO_CONTAINER_USER=uid:gid to override it." && \ + echo "and prints. Set MOSTRO_CONTAINER_USER=uid:gid to override it. A root-owned config" && \ + echo "directory is refused there, so run this target as the account that owns it." && \ echo "Building docker image" && \ docker compose build @@ -59,6 +66,14 @@ docker-up: mkdir -p config/relay && \ cp -v ./relay_config.toml config/relay/config.toml && \ export MOSTRO_CONTAINER_USER="$${MOSTRO_CONTAINER_USER:-$$(stat -c '%u:%g' config 2>/dev/null || stat -f '%u:%g' config)}" && \ + if [ "$${MOSTRO_CONTAINER_USER%%:*}" = 0 ]; then \ + echo "Error: refusing to run the mostro container as uid 0." >&2; \ + echo "MOSTRO_CONTAINER_USER is $${MOSTRO_CONTAINER_USER}. It defaults to the owner of" >&2; \ + echo "docker/config, which a run under sudo leaves as root." >&2; \ + echo "Hand that directory to an unprivileged account (chown -R 1000:1000 config)," >&2; \ + echo "or export MOSTRO_CONTAINER_USER=uid:gid with a non-zero uid that can read it." >&2; \ + exit 1; \ + fi && \ echo "Running mostro as $${MOSTRO_CONTAINER_USER} (MOSTRO_CONTAINER_USER; defaults to the owner of docker/config)" && \ echo "Starting services" && \ docker compose up -d diff --git a/docker/ENV_VARIABLES.md b/docker/ENV_VARIABLES.md index debdd73f..7ebdb7de 100644 --- a/docker/ENV_VARIABLES.md +++ b/docker/ENV_VARIABLES.md @@ -22,6 +22,7 @@ The copies belong to the user that ran the command, and `make docker-up` runs th - `MOSTRO_CONTAINER_USER`: uid/gid the `mostro` container runs as - `make docker-up` defaults it to the owner of `docker/config`, the account that can read the `0600` macaroon and write `mostro.db` there, and prints what it picked. A bare `docker compose up` falls back to `1000:1000`, the image's `mostrouser`. + - uid 0 is refused, whether derived or set explicitly: `make docker-up` stops rather than run the daemon as root, which is what a root-owned `docker/config` (left by a `sudo make docker-build`) would otherwise mean. Hand the directory to an unprivileged account, or set this variable to a non-zero uid/gid that can read it. - Set it to run as someone else — for instance uid/gid 1000 on a config directory you handed over with `chown -R 1000:1000` - Example: `export MOSTRO_CONTAINER_USER=$(id -u):$(id -g)` diff --git a/docker/README.md b/docker/README.md index 3366578f..18d71a05 100644 --- a/docker/README.md +++ b/docker/README.md @@ -37,7 +37,7 @@ To build and run the Docker container using Docker Compose, follow these steps: install -m 600 ../settings.tpl.toml config/settings.toml ``` - Mode `0700` on `config` and `0600` on `settings.toml` because that directory ends up holding every secret this deployment has: `nsec_privkey` in `settings.toml`, the LND credentials in `config/lnd/`, and the `mostro.db` the daemon writes. `install -d -m 700` also tightens a `config` directory an earlier `mkdir -p` left at `0755` — this command is you deciding the mode. `make docker-up` will not decide it again: it only creates `config` when it is missing, so a directory you deliberately opened up to a group later on keeps that mode. + Mode `0700` on `config` and `0600` on `settings.toml` because that directory ends up holding every secret this deployment has: `nsec_privkey` in `settings.toml`, the LND credentials in `config/lnd/`, and the `mostro.db` the daemon writes. `install -d -m 700` also tightens a `config` directory an earlier `mkdir -p` left at `0755` — this command is you deciding the mode. Neither `make docker-build` nor `make docker-up` will decide it again: both only create `config` when it is missing, so a directory you deliberately opened up to a group later on keeps that mode. (`config/lnd` is the exception: `make docker-build` sets it to `0700` on every run, since nothing but the LND credentials it installs lives there.) _Don't forget to edit `lnd_grpc_host`, `nsec_privkey` and `relays` fields in the `config/settings.toml` file. Note that paths in `settings.toml` refer to paths **inside the container**, so use `/config/lnd/tls.cert` and `/config/lnd/admin.macaroon` for the LND certificate and macaroon files (these will be copied there by `make docker-build`)._ @@ -74,6 +74,8 @@ To build and run the Docker container using Docker Compose, follow these steps: A bare `docker compose up` does not derive anything and falls back to `1000:1000`, the image's `mostrouser`. + `make docker-up` refuses to start when that uid comes out as 0, which is what a `sudo make docker-build` leaves behind: the container would run as root, dropping the one privilege boundary the image has. Either run both targets as the account that owns `docker/config`, or hand the directory over with `sudo chown -R 1000:1000 config` (`docker compose up` then matches, since 1000 is the image default). + 4. [Optional] Set the `MOSTRO_RELAY_LOCAL_PORT` environment variable to the port you want to use for the local relay (defaults to 7000 if not set). This can be set before running `make docker-up`: ```sh @@ -167,7 +169,7 @@ Steps to run the plain Mostro image on a VPS (no repo clone; image from Docker H install -m 600 -o 1000 -g 1000 /path/to/lnd/admin.macaroon /opt/mostro/lnd/admin.macaroon ``` - The admin macaroon grants full control of your LND node — anyone who reads it can move the funds escrowed in Mostro's hold invoices — so it is installed owner-readable only, and `-o 1000 -g 1000` hands it to the container's user (as the `0700` directory from step 2 already was). Without that ownership, mode `0600` would leave mostrod unable to read the macaroon. (`-o`/`-g` require root; as a non-root user, drop them and run the steps as the account that owns the config dir.) + The admin macaroon grants full control of your LND node — anyone who reads it can move the funds escrowed in Mostro's hold invoices — so it is installed owner-readable only, and `-o 1000 -g 1000` hands it to the container's user (as the `0700` directory from step 2 already was). Without that ownership, mode `0600` would leave mostrod unable to read the macaroon. (`-o`/`-g` require root; as a non-root user, drop them, run the steps as the account that owns the config dir, and add `--user $(id -u):$(id -g)` to the `docker run` commands in step 6, so the container runs as that account rather than as the image default `1000:1000` — which could not read the `0600` macaroon you just installed.) (If LND is on another host, you only need the cert and macaroon copied here; point `lnd_grpc_host` at that host in step 5.) diff --git a/src/config/permissions.rs b/src/config/permissions.rs index 2a31c543..b0bc6b92 100644 --- a/src/config/permissions.rs +++ b/src/config/permissions.rs @@ -243,14 +243,20 @@ fn create_temp_sibling(dir: &Path, file_name: &OsStr) -> Result<(PathBuf, fs::Fi let mut last_error = None; for attempt in 0..TEMP_NAME_ATTEMPTS { - let mut name = OsString::from("."); - name.push(file_name); - name.push(format!(".tmp-{}-{attempt}", std::process::id())); - let candidate = dir.join(name); + let candidate = dir.join(temp_sibling_name(file_name, attempt)); match open_owner_only_new(&candidate) { Ok(file) => return Ok((candidate, file)), - Err(e) => last_error = Some(e), + // Only a taken name is worth another attempt. An unwritable or + // missing directory, or a full disk, fails the same way sixteen + // times over and would be reported as a name collision. + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => last_error = Some(e), + Err(e) => { + return Err(MostroInternalErr(ServiceError::IOError(format!( + "Could not create a temporary file in {}: {e}", + dir.display() + )))) + } } } @@ -263,6 +269,19 @@ fn create_temp_sibling(dir: &Path, file_name: &OsStr) -> Result<(PathBuf, fs::Fi )))) } +/// The name `create_temp_sibling` tries for a given attempt: a dotfile next to +/// the target, so a temporary left behind by a killed run is not mistaken for +/// a settings file. +/// +/// Shared with the tests, which seed one of these names to exercise the retry +/// and would otherwise silently stop matching if the format changed here. +fn temp_sibling_name(file_name: &OsStr, attempt: u32) -> OsString { + let mut name = OsString::from("."); + name.push(file_name); + name.push(format!(".tmp-{}-{attempt}", std::process::id())); + name +} + /// Open a brand-new file owner-only, failing if anything already occupies the /// path. /// @@ -522,6 +541,21 @@ mod owner_only_tests { std::fs::write(env_file.join("occupied"), b"x").expect("occupy the directory"); assert!(write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1...\n").is_err()); } + + #[test] + fn a_failure_that_is_not_a_name_collision_is_reported_as_itself() { + let root = temp_root("atomic-missing-dir"); + let env_file = root.join("absent").join(".env"); + let err = write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1...\n") + .expect_err("a missing directory must not be written to"); + // Sixteen identical failures in a directory that does not exist only + // bury the cause, so the message must not blame a name collision. + let message = format!("{err:?}"); + assert!( + !message.contains("attempts"), + "expected the underlying error, got {message}" + ); + } } #[cfg(all(test, unix))] @@ -605,7 +639,7 @@ mod symlink_tests { // What a run killed mid-write leaves behind. Writing through it would // be harmless here, but the same name could just as well be a symlink // another account planted, so the first free suffix is used instead. - let stale = root.join(format!(".env.tmp-{}-0", std::process::id())); + let stale = root.join(temp_sibling_name(OsStr::new(".env"), 0)); std::fs::write(&stale, "stale").expect("seed stale temporary"); write_owner_only_atomic(&env_file, b"MOSTRO_NSEC_PRIVKEY=nsec1...\n").expect("write"); From 9d226529f8ace84ae7b0a30bbebd3ef6ebb5e538 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 27 Aug 2026 16:50:31 -0300 Subject: [PATCH 12/12] fix(docker): install LND credentials as config owner when running under sudo --- Makefile | 35 +++++++++++++++++++++------- README.md | 4 +++- docker/Dockerfile | 8 +++++-- docker/ENV_VARIABLES.md | 6 ++--- docker/README.md | 8 ++++--- docker/compose.yml | 4 ++-- docs/STARTUP_AND_CONFIG.md | 26 ++++++++++++++------- src/config/constants.rs | 5 ++++ src/config/permissions.rs | 47 +++++++++++++++++++++++++++++++++----- src/config/util.rs | 4 +--- src/config/wizard.rs | 4 ++-- src/main.rs | 16 +++++++++---- 12 files changed, 125 insertions(+), 42 deletions(-) diff --git a/Makefile b/Makefile index 73e50ec3..1defb378 100644 --- a/Makefile +++ b/Makefile @@ -19,12 +19,29 @@ VERSION := $(shell grep "^version = " Cargo.toml | sed "s/version = \"\(.*\)\"/\ # as you) and the documented `chown -R 1000:1000` handover. uid 0 is refused # rather than used: a root-owned `config` (a `sudo make docker-build`, say) # would otherwise drop the unprivileged user the image runs as. +# - Which is also why docker-build derives `-o/-g` from `config` when it runs +# as root over a directory that is not. `install` unlinks the destination and +# recreates it as the invoking user, so `sudo make docker-build` over a +# `config` handed to 1000 would leave a `root:root 0600` macaroon that +# docker-up then starts a uid 1000 container against — mostrod failing at the +# LND connection with nothing pointing at ownership. A `config` that is +# itself root-owned is left alone: docker-up refuses that case outright. +# - That refusal matches both spellings compose takes for uid 0, the numeric 0 +# and the name `root`, since MOSTRO_CONTAINER_USER is passed through to the +# `user:` key verbatim. Any other name is resolved inside the image, where +# mostrouser is the only account besides root. docker-build: @set -o pipefail; \ cd docker && \ { [ -d config ] || install -d -m 700 config; } && \ - install -d -m 700 config/lnd && \ + config_owner="$$(stat -c '%u:%g' config 2>/dev/null || stat -f '%u:%g' config)" && \ + install_owner="" && \ + if [ "$$(id -u)" = 0 ] && [ "$${config_owner%%:*}" != 0 ]; then \ + install_owner="-o $${config_owner%%:*} -g $${config_owner##*:}"; \ + echo "Running as root: installing config/lnd as $${config_owner}, the owner of config"; \ + fi && \ + install -d -m 700 $${install_owner} config/lnd && \ echo "Checking LND files..." && \ echo "LND_CERT_FILE=$${LND_CERT_FILE}" && \ echo "LND_MACAROON_FILE=$${LND_MACAROON_FILE}" && \ @@ -47,14 +64,15 @@ docker-build: exit 1; \ fi && \ echo "Copying LND cert and macaroon to docker config" && \ - install -m 644 "$${LND_CERT_FILE}" config/lnd/tls.cert && \ - install -m 600 "$${LND_MACAROON_FILE}" config/lnd/admin.macaroon && \ + install -m 644 $${install_owner} "$${LND_CERT_FILE}" config/lnd/tls.cert && \ + install -m 600 $${install_owner} "$${LND_MACAROON_FILE}" config/lnd/admin.macaroon && \ echo "Wrote config/lnd/tls.cert (mode 644) and config/lnd/admin.macaroon (mode 600)" && \ echo "config/lnd is mode 700, and config keeps the mode it was created with (700 unless you" && \ echo "changed it): settings.toml holds nsec_privkey and mostro.db lands there too" && \ echo "The mostro container runs as the owner of docker/config, which make docker-up derives" && \ echo "and prints. Set MOSTRO_CONTAINER_USER=uid:gid to override it. A root-owned config" && \ - echo "directory is refused there, so run this target as the account that owns it." && \ + echo "directory is refused there; under sudo this target installs the credentials as the" && \ + echo "owner of config, so a directory already handed over stays readable by the container." && \ echo "Building docker image" && \ docker compose build @@ -66,14 +84,15 @@ docker-up: mkdir -p config/relay && \ cp -v ./relay_config.toml config/relay/config.toml && \ export MOSTRO_CONTAINER_USER="$${MOSTRO_CONTAINER_USER:-$$(stat -c '%u:%g' config 2>/dev/null || stat -f '%u:%g' config)}" && \ - if [ "$${MOSTRO_CONTAINER_USER%%:*}" = 0 ]; then \ - echo "Error: refusing to run the mostro container as uid 0." >&2; \ + case "$${MOSTRO_CONTAINER_USER%%:*}" in \ + 0|root) \ + echo "Error: refusing to run the mostro container as root." >&2; \ echo "MOSTRO_CONTAINER_USER is $${MOSTRO_CONTAINER_USER}. It defaults to the owner of" >&2; \ echo "docker/config, which a run under sudo leaves as root." >&2; \ echo "Hand that directory to an unprivileged account (chown -R 1000:1000 config)," >&2; \ echo "or export MOSTRO_CONTAINER_USER=uid:gid with a non-zero uid that can read it." >&2; \ - exit 1; \ - fi && \ + exit 1;; \ + esac && \ echo "Running mostro as $${MOSTRO_CONTAINER_USER} (MOSTRO_CONTAINER_USER; defaults to the owner of docker/config)" && \ echo "Starting services" && \ docker compose up -d diff --git a/README.md b/README.md index 05172aff..28c2ee11 100644 --- a/README.md +++ b/README.md @@ -497,7 +497,9 @@ This starts: - Mostro daemon (exposed via configured relays) - Local Nostr relay (port 7000 by default) -`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node, and sets `docker/config` and `docker/config/lnd` to mode `0700` — the same directory holds `settings.toml` and `mostro.db`. `make docker-up` then runs the container as the owner of `docker/config`, so it can read those files whatever your uid is; `export MOSTRO_CONTAINER_USER=uid:gid` to override that. +`make docker-build` installs the LND admin macaroon into `docker/config/lnd/` with mode `0600`, since it grants full control of your node, and sets `docker/config/lnd` to mode `0700` on every run. It sets `docker/config` to `0700` only when it has to create the directory — that one holds `settings.toml` and `mostro.db` as well, so a mode you chose for it deliberately is left as it is. + +`make docker-up` then runs the container as the owner of `docker/config`, so it can read those files whatever your uid is; `export MOSTRO_CONTAINER_USER=uid:gid` to override that. It refuses to start the container as root, which is what a root-owned `docker/config` would otherwise mean. Under `sudo`, `make docker-build` installs the credentials as the owner of `docker/config` rather than as root, so a directory already handed over to uid 1000 stays readable by the container across rebuilds. **Stop**: `make docker-down` diff --git a/docker/Dockerfile b/docker/Dockerfile index 3eb7968e..56b48c1f 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -26,8 +26,12 @@ FROM debian:bookworm-slim # Install dependencies RUN apt-get update && apt-get install -y --reinstall ca-certificates -# Add a non-root user -RUN useradd -m mostrouser +# Add a non-root user. uid/gid are pinned because compose defaults +# MOSTRO_CONTAINER_USER to 1000:1000 and the deployment docs have operators +# hand ./config over with `chown -R 1000:1000`: letting useradd pick the next +# free id would break both the moment the base image grows an account of its +# own. +RUN groupadd -g 1000 mostrouser && useradd -m -u 1000 -g 1000 mostrouser # Copy built binary from build stage COPY --from=builder /mostro/target/release/mostrod /usr/local/bin/mostrod diff --git a/docker/ENV_VARIABLES.md b/docker/ENV_VARIABLES.md index 7ebdb7de..384811e3 100644 --- a/docker/ENV_VARIABLES.md +++ b/docker/ENV_VARIABLES.md @@ -10,9 +10,9 @@ This document describes the environment variables used by the Docker setup. - `LND_MACAROON_FILE`: Path to the LND admin macaroon file on your host system - Example: `~/.polar/networks/1/volumes/lnd/alice/data/chain/bitcoin/regtest/admin.macaroon` -These files are copied to `docker/config/lnd/` during the build process: the cert with mode `0644`, the admin macaroon with mode `0600`, both inside a directory with mode `0700`. The macaroon grants full control of your LND node, so it is never left readable by other users on the host. The `docker/config` root is set to mode `0700` as well, since `settings.toml` (which carries `nsec_privkey`) and `mostro.db` live beside the credentials. +These files are copied to `docker/config/lnd/` during the build process: the cert with mode `0644`, the admin macaroon with mode `0600`, both inside a directory with mode `0700`. The macaroon grants full control of your LND node, so it is never left readable by other users on the host. The `docker/config` root is set to mode `0700` too when the build has to create it, since `settings.toml` (which carries `nsec_privkey`) and `mostro.db` live beside the credentials; an existing `docker/config` keeps the mode you gave it. -The copies belong to the user that ran the command, and `make docker-up` runs the container as the owner of `docker/config` — that user — so a host account that is not uid 1000 needs no extra step. Set the optional variable below to override it. +The copies belong to the user that ran the command, and `make docker-up` runs the container as the owner of `docker/config` — that user — so a host account that is not uid 1000 needs no extra step. Under `sudo`, the build installs `config/lnd` and the credentials as the owner of `docker/config` instead of as root, so a directory handed over with `chown -R 1000:1000` stays readable by the container across rebuilds. Set the optional variable below to override the container user. ## Optional Variables @@ -22,7 +22,7 @@ The copies belong to the user that ran the command, and `make docker-up` runs th - `MOSTRO_CONTAINER_USER`: uid/gid the `mostro` container runs as - `make docker-up` defaults it to the owner of `docker/config`, the account that can read the `0600` macaroon and write `mostro.db` there, and prints what it picked. A bare `docker compose up` falls back to `1000:1000`, the image's `mostrouser`. - - uid 0 is refused, whether derived or set explicitly: `make docker-up` stops rather than run the daemon as root, which is what a root-owned `docker/config` (left by a `sudo make docker-build`) would otherwise mean. Hand the directory to an unprivileged account, or set this variable to a non-zero uid/gid that can read it. + - root is refused, whether derived or set explicitly, and in both spellings compose accepts for it: the numeric `0` and the name `root`. `make docker-up` stops rather than run the daemon as root, which is what a root-owned `docker/config` (left by a `sudo make docker-build` on a fresh tree) would otherwise mean. Hand the directory to an unprivileged account, or set this variable to a non-zero uid/gid that can read it. Any other name is resolved inside the image, where `mostrouser` (uid/gid 1000) is the only account besides root. - Set it to run as someone else — for instance uid/gid 1000 on a config directory you handed over with `chown -R 1000:1000` - Example: `export MOSTRO_CONTAINER_USER=$(id -u):$(id -g)` diff --git a/docker/README.md b/docker/README.md index 18d71a05..f8d4bb21 100644 --- a/docker/README.md +++ b/docker/README.md @@ -64,7 +64,9 @@ To build and run the Docker container using Docker Compose, follow these steps: make docker-build ``` - The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The `config` root itself is set to `0700` as well, since `settings.toml` and `mostro.db` sit next to the credentials. The directories and files this command creates belong to the user who ran it; `mostro.db` is created later by the container and belongs to whoever the container runs as. + The admin macaroon grants full control of your LND node, so `make docker-build` writes it to `config/lnd/admin.macaroon` with mode `0600` (owner only) inside a `config/lnd` directory with mode `0700`. The `config` root is set to `0700` too when this command has to create it, and left alone when it is already there (step 2). The directories and files this command creates belong to the user who ran it; `mostro.db` is created later by the container and belongs to whoever the container runs as. + + Under `sudo` there is one extra step, taken for you: `install` recreates its destination as the invoking user, so a plain `sudo make docker-build` would leave a `root:root` macaroon inside a `config` you had already handed to uid 1000, and the container would fail at the LND connection with nothing pointing at ownership. When it runs as root over a `config` that is not root-owned, the command derives `-o`/`-g` from that directory and installs `config/lnd` and both credentials as its owner. A `config` that is root-owned itself is left as it is: `make docker-up` refuses that case outright rather than run the daemon as root. `make docker-up` runs the container as the owner of `docker/config` and prints the uid/gid it picked. That is the account that can actually read the `0600` macaroon and write `mostro.db` there, whether the directory belongs to you (the usual case after `make docker-build`) or was handed to uid/gid 1000. To pick a different one, export the variable `compose.yml` reads in the same shell: @@ -74,7 +76,7 @@ To build and run the Docker container using Docker Compose, follow these steps: A bare `docker compose up` does not derive anything and falls back to `1000:1000`, the image's `mostrouser`. - `make docker-up` refuses to start when that uid comes out as 0, which is what a `sudo make docker-build` leaves behind: the container would run as root, dropping the one privilege boundary the image has. Either run both targets as the account that owns `docker/config`, or hand the directory over with `sudo chown -R 1000:1000 config` (`docker compose up` then matches, since 1000 is the image default). + `make docker-up` refuses to start when that uid comes out as root — both the numeric `0` and the name `root`, the two spellings compose accepts — which is what a `sudo make docker-build` on a fresh tree leaves behind: the container would run as root, dropping the one privilege boundary the image has. Either run both targets as the account that owns `docker/config`, or hand the directory over with `sudo chown -R 1000:1000 config` (`docker compose up` then matches, since the image's `mostrouser` is pinned to uid/gid 1000). 4. [Optional] Set the `MOSTRO_RELAY_LOCAL_PORT` environment variable to the port you want to use for the local relay (defaults to 7000 if not set). This can be set before running `make docker-up`: @@ -118,7 +120,7 @@ You can run the plain Mostro image without building locally. Use a single **conf install -m 600 /path/to/your/admin.macaroon ~/mostro-config/lnd/admin.macaroon ``` - Mode `0600` on the macaroon inside a `0700` directory means only their owner can reach the file, and the container runs as uid/gid 1000 by default. If your user is not uid 1000, run the container as yourself by adding `--user $(id -u):$(id -g)` to the `docker run` command in step 4 — that also lets it write `mostro.db` into your config directory. + Mode `0600` on the macaroon inside a `0700` directory means only their owner can reach the file, and the container runs as uid/gid 1000 by default (the image's `mostrouser`, pinned to those ids). If your user is not uid 1000, run the container as yourself by adding `--user $(id -u):$(id -g)` to the `docker run` command in step 4 — that also lets it write `mostro.db` into your config directory. 3. Edit `~/mostro-config/settings.toml`: set `nsec_privkey`, `relays`, and for Docker set `lnd_cert_file` / `lnd_macaroon_file` to `/config/lnd/...`, `lnd_grpc_host` (e.g. `https://host.docker.internal:10009`), and `[database]` `url = "sqlite:///config/mostro.db"`. diff --git a/docker/compose.yml b/docker/compose.yml index deb70a53..caf796b3 100644 --- a/docker/compose.yml +++ b/docker/compose.yml @@ -5,8 +5,8 @@ services: dockerfile: docker/Dockerfile volumes: - ./config:/config # settings.toml and mostro.db - # The image's mostrouser is uid/gid 1000, so the default keeps the previous - # behaviour. Hosts whose user is not uid 1000 export + # The image pins mostrouser to uid/gid 1000, so the default keeps the + # previous behaviour. Hosts whose user is not uid 1000 export # MOSTRO_CONTAINER_USER=$(id -u):$(id -g) instead of handing ./config over # to uid 1000 — the LND macaroon is installed 0600, and the daemon also # needs to write mostro.db in the same directory. diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index ab3d8345..ecb94014 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -114,10 +114,14 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. is advisory — the daemon still starts — and tolerates `0640`, the mode LND itself writes the macaroon with, so reading it through the node's group stays supported. - - The same check runs over `settings.toml` and `/.env` right after - the settings load, in both Lightning and Cashu mode: those two carry - `nsec_privkey`, which is the instance's identity regardless of which escrow - it uses. A missing `.env` is silent, since it is optional. + - The same check runs over `settings.toml`, `/.env` and + `/mostro.db` right after the settings load, in both Lightning and + Cashu mode: the first two carry `nsec_privkey`, which is the instance's identity + regardless of which escrow it uses, and the database holds the trade history, the + disputes and the hold-invoice preimages. SQLite creates the database under the + umask, so a settings directory that predates the `0700` default is where a `0644` + database comes from. A missing file is silent: `.env` is optional, and the database + does not exist yet on a first boot. - Only the mode bits are inspected: a POSIX ACL can grant a named user access without setting them, so a quiet startup is not proof that no other account can read the file. @@ -236,14 +240,20 @@ There is **no** database password or separate global for SQLite; the daemon open `write_owner_only_atomic` stages the line in a fresh `O_EXCL` temporary beside the target and `rename`s it into place. `rename` never opens the destination, so a planted symlink is replaced rather than followed and its target keeps both its - contents and its mode. `.env` matters here as much as `settings.toml`: it carries the - same `nsec_privkey`, and in the wizard flow it is written first. + contents and its mode. The temporary is `fsync`ed before the rename and the directory + after it (`src/config/permissions.rs`, `fn sync_dir`), so the replacement is durable + and not merely atomic — a power loss right after an unflushed rename can leave the + directory entry pointing at the old file, which for `.env` means losing an + `nsec_privkey` the wizard reported as saved. A failed flush is logged and not + propagated: the contents are already in place. `.env` matters here as much as + `settings.toml`: it carries the same `nsec_privkey`, and in the wizard flow it is + written first. - It is not a check on startup as a whole. A `settings.toml` that already exists is read and loaded normally, symlink or not — `fn init_configuration_file` only reaches the creation path when it finds no settings file at all. - The guarantee covers the final entry, not the directory path leading to it. The - parents `fn create_settings_dir` has to invent are made with a recursive - `DirBuilder`, which resolves symlinked components on the way, so a settings directory + parents `fn create_settings_dir` has to invent are made with `fs::create_dir_all`, + which resolves symlinked components on the way, so a settings directory reached through a symlinked parent is created at whatever that link points to. This is deliberate: operators do symlink a config directory onto another volume, and planting such a link on the default `~/.mostro` path means write access to `$HOME`, which is a diff --git a/src/config/constants.rs b/src/config/constants.rs index fe53b1b9..74f576fd 100644 --- a/src/config/constants.rs +++ b/src/config/constants.rs @@ -30,6 +30,11 @@ pub const ENV_FILENAME: &str = ".env"; /// Name of the settings file inside the settings directory. pub const SETTINGS_FILENAME: &str = "settings.toml"; +/// Name of the SQLite database inside the settings directory. The loader +/// overrides `database.url` with it unconditionally, so this is the only path +/// the daemon ever opens. +pub const DB_FILENAME: &str = "mostro.db"; + /// Environment variable name used to override the Nostr private key from the /// process environment. Shared between the wizard and the loader. pub const NSEC_ENV_VAR: &str = "MOSTRO_NSEC_PRIVKEY"; diff --git a/src/config/permissions.rs b/src/config/permissions.rs index b0bc6b92..88b61cfb 100644 --- a/src/config/permissions.rs +++ b/src/config/permissions.rs @@ -34,7 +34,8 @@ const TEMP_NAME_ATTEMPTS: u32 = 16; /// unreadable path is not a permissions problem, and the real failure is /// reported with far more context by whoever opens the file (for the macaroon, /// `LndConnector::new`). `/.env` is optional and usually absent, -/// so its being missing has to stay silent. +/// and `mostro.db` does not exist yet on a first boot, so a missing file has +/// to stay silent. pub fn warn_if_other_accessible(path: &Path, label: &str) { if path.as_os_str().is_empty() { return; @@ -100,11 +101,11 @@ fn other_accessible_mode(_path: &Path) -> Option { /// An existing settings directory is returned as it is, so an operator who /// already set one up with deliberate group access keeps it. /// -/// The parents are created with a recursive `DirBuilder`, which resolves -/// symlinked components on the way. That is deliberate: symlinking a config -/// directory onto another volume is a legitimate setup, and planting a link on -/// the default `~/.mostro` path takes write access to `$HOME`, which already -/// owns the account. A settings directory that is itself a symlink to an +/// The parents are created with `fs::create_dir_all`, which resolves symlinked +/// components on the way. That is deliberate: symlinking a config directory +/// onto another volume is a legitimate setup, and planting a link on the +/// default `~/.mostro` path takes write access to `$HOME`, which already owns +/// the account. A settings directory that is itself a symlink to an /// existing directory never reaches here — the caller finds it and uses it. pub(crate) fn create_settings_dir(settings_dir: &Path) -> Result<(), MostroError> { if settings_dir.is_dir() { @@ -196,6 +197,13 @@ pub(crate) fn create_owner_only(path: &Path, contents: &[u8]) -> Result<(), Most /// - The file at `path` is never observed half-written. A `.env` truncated by /// a full disk would otherwise leave the daemon with no `nsec_privkey` at /// all on the next boot. +/// +/// The contents are `fsync`ed before the rename and the directory after it, so +/// the replacement survives a crash and not only a process exit. Without the +/// second one the rename is atomic but not durable: a power loss right after +/// it can leave the directory entry still pointing at the old file, which for +/// the wizard's `.env` means an `nsec_privkey` the operator was told was +/// saved. pub(crate) fn write_owner_only_atomic(path: &Path, contents: &[u8]) -> Result<(), MostroError> { use std::io::Write; @@ -228,9 +236,36 @@ pub(crate) fn write_owner_only_atomic(path: &Path, contents: &[u8]) -> Result<() )))); } + sync_dir(dir); + Ok(()) } +/// Flush the directory entry the `rename` above just replaced. +/// +/// `fsync` on a directory descriptor is the POSIX way to make a rename +/// durable; the data blocks are already on disk from the `sync_all` on the +/// temporary. Windows has no equivalent, and its rename does not need one. +/// +/// Failures are logged rather than propagated. The new contents are in place +/// and visible either way — only the durability of the directory entry is in +/// question — so returning an error here would fail a write that succeeded and +/// send the caller into a rollback path with nothing to roll back. +#[cfg(unix)] +fn sync_dir(dir: &Path) { + match fs::File::open(dir).and_then(|dir_file| dir_file.sync_all()) { + Ok(()) => {} + Err(e) => tracing::warn!( + "Wrote the file but could not flush {}: {e}. The contents are in place; only a \ + crash before the filesystem catches up on its own could still lose them.", + dir.display() + ), + } +} + +#[cfg(not(unix))] +fn sync_dir(_dir: &Path) {} + /// Create an owner-only temporary file next to the target and return it with /// its path. /// diff --git a/src/config/util.rs b/src/config/util.rs index 1868cdaa..4176fb25 100644 --- a/src/config/util.rs +++ b/src/config/util.rs @@ -3,7 +3,7 @@ /// It includes functions to initialize the default settings directory and create a settings file from the template if it doesn't exist. /// It also includes functions to add a trailing slash to a path if it doesn't already have one. use crate::config::constants::{ - ENV_FILENAME, MAX_DEV_FEE_PERCENTAGE, MIN_DEV_FEE_PERCENTAGE, SETTINGS_FILENAME, + DB_FILENAME, ENV_FILENAME, MAX_DEV_FEE_PERCENTAGE, MIN_DEV_FEE_PERCENTAGE, SETTINGS_FILENAME, }; use crate::config::permissions::{create_owner_only, create_settings_dir}; use crate::config::secret::read_nsec_env_var; @@ -16,8 +16,6 @@ use std::io::IsTerminal; use std::path::PathBuf; use zeroize::Zeroizing; -const DB_FILENAME: &str = "mostro.db"; - /// Loads the optional `/.env` file so that values placed there /// become available through `std::env::var`. Variables already set in the /// process environment take precedence and are never overwritten. diff --git a/src/config/wizard.rs b/src/config/wizard.rs index db2acb72..8e7cd3ef 100644 --- a/src/config/wizard.rs +++ b/src/config/wizard.rs @@ -7,7 +7,7 @@ use nostr_sdk::prelude::*; use secrecy::{ExposeSecret, SecretString}; use zeroize::Zeroizing; -use super::constants::{ENV_FILENAME, NSEC_ENV_VAR}; +use super::constants::{DB_FILENAME, ENV_FILENAME, NSEC_ENV_VAR}; use super::permissions::{create_owner_only, write_owner_only_atomic}; use super::settings::Settings; use super::types::{ @@ -84,7 +84,7 @@ fn run_setup_wizard(settings_dir: &Path, config_file_path: &Path) -> Result> = std::result::Result; use crate::app::context::AppContext; use crate::app::{run, run_cashu}; use crate::cli::settings_init; -use crate::config::constants::{ENV_FILENAME, SETTINGS_FILENAME}; +use crate::config::constants::{DB_FILENAME, ENV_FILENAME, SETTINGS_FILENAME}; use crate::config::{ get_db_pool, permissions, Settings, DB_POOL, LN_STATUS, MESSAGE_QUEUES, MOSTRO_CONFIG, NOSTR_CLIENT, @@ -68,12 +68,20 @@ async fn main() -> Result<()> { // that signs every event this instance publishes sits in one of these two // files. Both are what the deployment guides have operators create by hand // with `cp`, `curl` or an editor, all of which apply the umask, so a `0644` - // settings file is the normal accident. Checked in both Lightning and - // Cashu mode, unlike the macaroon further down, because the identity is - // not specific to either. + // settings file is the normal accident. `mostro.db` is the same directory + // and the same accident: SQLite creates it under the umask, and it holds + // the trade history, the disputes and the hold-invoice preimages. Checked + // in both Lightning and Cashu mode, unlike the macaroon further down, + // because none of the three is specific to either. + // + // The database is checked before it exists on a first boot, which is + // silent; a fresh install is not the case this catches. The one it does + // catch is the deployment that has been running on a `0755` settings + // directory since before any of this existed. for (name, label) in [ (SETTINGS_FILENAME, "Mostro settings file"), (ENV_FILENAME, "Mostro env file"), + (DB_FILENAME, "Mostro database"), ] { permissions::warn_if_other_accessible(&settings_dir.join(name), label); }