Skip to main content

max / makenotwork

sando: fix pre-swap config-check env parsing; enable the gate on both nodes The pre-swap config-drift gate (check_target_config) sourced the node's env file with `. env_file`, which runs it as a script — so any shell metacharacter in a value (a DB password, a URL) is expanded or word-split. On our real nodes this silently dropped DATABASE_URL to empty, so MNW_CHECK_CONFIG reported it missing and the gate would fail EVERY deploy. That is why the gate, though implemented, was never enabled and incident #2 (prod crash-loop on a missing CDN_BASE_URL) slipped through un-gated. Load the env file with systemd EnvironmentFile= semantics instead: read each line and `export "$line"` verbatim, no re-expansion or splitting. Validated end-to-end against testnot's live 0.10.14 binary (was "DATABASE_URL required", now "MNW_CONFIG_CHECK: ok"). Enable config_check_env_file = /etc/mnw/makenotwork.env on both serving nodes (testnot-1, prod-1) now that the check is correct. The gate is fail-closed and runs before the symlink swap, so a missing required var fails the promote with the running service intact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-10 13:53 UTC
Commit: 907a1f7589e9b39298c37899c609b9bd3a9e332e
Parent: 93f0493
2 files changed, +104 insertions, -12 deletions
M sando/daemon/src/deploy.rs +94 -12
@@ -205,10 +205,11 @@ async fn deploy_remote(
205 205 Ok(PathBuf::from(release_root).join("releases").join(version))
206 206 }
207 207
208 - /// Pre-swap config-drift check: source the node's env file, then run the
209 - /// freshly-deployed binary in `MNW_CHECK_CONFIG=1` mode (loads config, exits
210 - /// 0/1, no DB/migrations/bind). A non-zero exit — a required var missing — is
211 - /// surfaced by `run_checked` as an error, failing the promote before the swap.
208 + /// Pre-swap config-drift check: load the node's env file the way systemd loads
209 + /// it, then run the freshly-deployed binary in `MNW_CHECK_CONFIG=1` mode (loads
210 + /// config, exits 0/1, no DB/migrations/bind). A non-zero exit — a required var
211 + /// missing — is surfaced by `run_checked` as an error, failing the promote
212 + /// before the swap.
212 213 ///
213 214 /// Bounded by a timeout as a backstop: a binary predating `MNW_CHECK_CONFIG`
214 215 /// would ignore the var and try to start normally, which must not hang the
@@ -220,14 +221,7 @@ async fn check_target_config(
220 221 deployed_bin: &str,
221 222 env_file: &str,
222 223 ) -> Result<()> {
223 - // `set -a` exports every var the env file assigns so Config::from_env sees
224 - // them; the binary then validates and exits. Quoted paths; the env file is
225 - // operator-controlled config, not user input.
226 - let script = format!(
227 - "set -eu; set -a; . {env}; set +a; MNW_CHECK_CONFIG=1 {bin}",
228 - env = sh_quote(env_file),
229 - bin = sh_quote(deployed_bin),
230 - );
224 + let script = config_check_script(env_file, deployed_bin);
231 225 let fut = run_checked(executor, &script, "pre-swap config check");
232 226 match tokio::time::timeout(std::time::Duration::from_secs(20), fut).await {
233 227 Ok(result) => result.map(|_| ()),
@@ -238,6 +232,34 @@ async fn check_target_config(
238 232 }
239 233 }
240 234
235 + /// Shell that loads `env_file` with systemd `EnvironmentFile=` semantics, then
236 + /// runs `bin` under `MNW_CHECK_CONFIG=1`.
237 + ///
238 + /// Load the file line by line and `export` each `KEY=VALUE` verbatim rather than
239 + /// `. env_file`. Dot-sourcing runs the file as a script, so any shell
240 + /// metacharacter in a value (`$`, backticks, `;`, `&`, a glob, whitespace) is
241 + /// expanded or word-split — a DB URL carrying a password silently dropped
242 + /// `DATABASE_URL` to empty on our nodes, which would fail the check (and thus
243 + /// every deploy) even though systemd starts the service fine. `export "$line"`
244 + /// assigns the already-expanded word literally, matching systemd's "no variable
245 + /// expansion" rule. Comments and blank lines are skipped; the `|| [ -n "$line" ]`
246 + /// guard processes a final line with no trailing newline. (Quoted values —
247 + /// `KEY="v"` — aren't unquoted here the way systemd would, but our env files use
248 + /// bare `KEY=VALUE`, and a stray quote can only make the check stricter, never
249 + /// wave a bad config through.)
250 + fn config_check_script(env_file: &str, bin: &str) -> String {
251 + format!(
252 + "set -eu\n\
253 + while IFS= read -r __sando_l || [ -n \"$__sando_l\" ]; do\n\
254 + \tcase \"$__sando_l\" in ''|'#'*) continue ;; esac\n\
255 + \texport \"$__sando_l\"\n\
256 + done < {env}\n\
257 + MNW_CHECK_CONFIG=1 {bin}\n",
258 + env = sh_quote(env_file),
259 + bin = sh_quote(bin),
260 + )
261 + }
262 +
241 263 /// Build the swap-and-restart shell script for a remote node.
242 264 ///
243 265 /// The symlink swap is atomic via `mv -T` of a freshly-created symlink over the
@@ -626,6 +648,66 @@ mod tests {
626 648 assert_eq!(target.to_string_lossy(), "releases/new", "no prev existed to roll back to");
627 649 }
628 650
651 + // ---- config_check_script: systemd-faithful env loading ----
652 +
653 + #[tokio::test]
654 + async fn config_check_script_loads_values_with_shell_metachars() {
655 + // The bug: `. env_file` expands/word-splits values, so a URL or a
656 + // password containing a shell metacharacter is mangled — it dropped
657 + // DATABASE_URL to empty on a real node, which would fail every deploy.
658 + // The export-loop must load such a value intact. The "binary" is a
659 + // checker script (a real path, like a deployed binary) that exits 0 only
660 + // if the var arrived byte-for-byte — it compares against the expected
661 + // value read from a file, so nothing re-interprets the metacharacters.
662 + let tricky = "postgres://u:p$ss;w&rd@h/db `x` $(y)";
663 + // Plain files in a tempdir: no lingering write fd, so the checker can be
664 + // exec'd (a NamedTempFile stays open and would ETXTBSY).
665 + let dir = tempfile::tempdir().unwrap();
666 + let expected_path = dir.path().join("expected");
667 + std::fs::write(&expected_path, tricky).unwrap(); // no trailing newline
668 +
669 + let env_path = dir.path().join("node.env");
670 + std::fs::write(
671 + &env_path,
672 + format!(
673 + "# a comment\n\nDATABASE_URL={tricky}\nOTHER=plain\nEXPECTED_FILE={ef}\n",
674 + ef = expected_path.display(),
675 + ),
676 + )
677 + .unwrap();
678 +
679 + let checker_path = dir.path().join("checker.sh");
680 + std::fs::write(
681 + &checker_path,
682 + "#!/bin/sh\nwant=$(cat \"$EXPECTED_FILE\")\n\
683 + [ \"$DATABASE_URL\" = \"$want\" ] || { echo \"DB [$DATABASE_URL] != [$want]\" >&2; exit 1; }\n\
684 + [ \"$OTHER\" = plain ] || { echo \"OTHER [$OTHER]\" >&2; exit 1; }\n",
685 + )
686 + .unwrap();
687 + std::fs::set_permissions(&checker_path, std::os::unix::fs::PermissionsExt::from_mode(0o755)).unwrap();
688 +
689 + let script = config_check_script(
690 + &env_path.to_string_lossy(),
691 + &checker_path.to_string_lossy(),
692 + );
693 + let out = run_script(&script).await;
694 + assert!(
695 + out.status.success(),
696 + "value with shell metachars must load intact; stderr: {}",
697 + String::from_utf8_lossy(&out.stderr),
698 + );
699 + }
700 +
701 + #[tokio::test]
702 + async fn config_check_script_propagates_binary_failure() {
703 + // A required var missing (the binary exits non-zero) must fail the check.
704 + let env = tempfile::NamedTempFile::new().unwrap();
705 + std::fs::write(env.path(), "FOO=bar\n").unwrap();
706 + let script = config_check_script(&env.path().to_string_lossy(), "false");
707 + let out = run_script(&script).await;
708 + assert!(!out.status.success(), "a non-zero MNW_CHECK_CONFIG exit must fail the check");
709 + }
710 +
629 711 #[tokio::test]
630 712 async fn deploy_node_denied_when_executor_lacks_deploy_grant() {
631 713 // Defense in depth: an executor without the deploy grant refuses the
M sando/sando.toml +10
@@ -57,6 +57,12 @@ name = "testnot-1"
57 57 ssh_target = "deploy@testnot" # tailnet name; never the public IP / testnot.work hostname
58 58 release_root = "/opt/mnw"
59 59 service_name = "makenotwork.service"
60 + # Pre-swap config-drift guard: source this node's env and run the freshly-rsynced
61 + # binary in MNW_CHECK_CONFIG=1 mode BEFORE the symlink swap. A required var
62 + # missing here (how prod crash-looped on CDN_BASE_URL, postmortem 2026-07-09 #2)
63 + # fails the promote with the running service intact. The node runs 0.10.14, which
64 + # supports the mode; validated end-to-end 2026-07-10.
65 + config_check_env_file = "/etc/mnw/makenotwork.env"
60 66 # Set health_url to add an HTTP readiness probe on top of `systemctl is-active`,
61 67 # e.g. health_url = "http://127.0.0.1:8080/health" (the local port the service
62 68 # binds). Unset = unit-active check only.
@@ -78,6 +84,10 @@ name = "prod-1"
78 84 ssh_target = "makenotwork@alpha-west-1"
79 85 release_root = "/opt/mnw"
80 86 service_name = "makenotwork.service"
87 + # Pre-swap config-drift guard (see testnot-1 above). This node is the one the
88 + # 0.10.14 deploy crash-looped on a missing CDN_BASE_URL — the exact miss this
89 + # gate closes. First real exercise is the next prod promote.
90 + config_check_env_file = "/etc/mnw/makenotwork.env"
81 91
82 92 # ---- C: prod-2 (declared, not yet provisioned) ----
83 93 [[tier]]