diff --git a/.gitignore b/.gitignore index 1aab090..04abae2 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,13 @@ fleetd.out fleetd/fleetd.out logs/ +# fleetd #635 follow-up — scripts/config-edit.sh's backup directory. No leading slash, so this is +# ignored at every depth: the real one lives under fleetd/ (also named in fleetd/.gitignore, next +# to the config it backs up), and scripts/test-config-edit.sh's own throwaway fixtures build one +# under the repo root while the suite runs. --config can point anywhere, so the directory name is +# ignored everywhere rather than only where the live daemon happens to use it. +.config-backups/ + # fleetd #480: the lead rollover handover file. `leadRollover.handoverPath` points here, and the # outgoing lead rewrites it on every rollover. It is a snapshot of one moment's live state — # unpushed branches, running builds, open questions — so it is stale the moment it is written and diff --git a/fleetd/.gitignore b/fleetd/.gitignore index 5b6d1fa..5a25259 100644 --- a/fleetd/.gitignore +++ b/fleetd/.gitignore @@ -7,6 +7,13 @@ dependency-reduced-pom.xml fleetd.yaml bridged.yaml +# fleetd #635 follow-up — scripts/config-edit.sh's backups of fleetd.yaml. A backup of a file +# that must never be committed inherits that requirement. The directory is the real protection +# (it keeps working even if the backup naming changes); the glob is a backstop for a stray +# backup written the old way, directly beside fleetd.yaml, or by an older copy of the script. +.config-backups/ +fleetd.yaml.bak.* + # CB-505 audit trail + daemon stdout/stderr — runtime records, never source logs/ diff --git a/scripts/config-edit.sh b/scripts/config-edit.sh index 2c9ef22..604cf7a 100755 --- a/scripts/config-edit.sh +++ b/scripts/config-edit.sh @@ -46,6 +46,13 @@ # scripts/config-edit.sh --dry-run --set = # scripts/config-edit.sh --restore # +# `--set .a.b=` (an empty value — a forgotten typo) is REFUSED, not accepted as "clear the +# field": a null value falls back to its default rather than erroring, which is silent, not +# safe. To clear a key on purpose, write a literal null: `--set .a.b=null`. Every other value +# is always written as a YAML string (via yq's strenv(), never spliced into the expression), so +# there is currently no --set spelling for the literal three-character STRING "null" itself — use +# --from for that rare case. +# # Overrides (so this is drivable with no daemon — see scripts/test-config-edit.sh): # --config default: fleetd/fleetd.yaml # --log default: fleetd/fleetd.out @@ -77,6 +84,16 @@ DRY_RUN=0 SETS=() FROM_FILE="" +# fleetd #635 follow-up — a signal (or any early exit while a candidate is still uninstalled) must +# not leave a `.config-edit.XXXXXX` file sitting beside the live config forever. CAND is global +# (never a function-local) on purpose: this ONE trap, set once, covers every path that ever +# creates a candidate — run_edit and dry_run_diff both assign it, and clear it back to "" once the +# file is consumed (installed, or explicitly removed), so a later, unrelated exit never retries a +# path that already served its purpose. +CAND="" +cleanup_candidate() { [ -n "$CAND" ] && rm -f "$CAND" 2>/dev/null; return 0; } +trap cleanup_candidate EXIT INT TERM + say() { printf '\n\033[1m== %s\033[0m\n' "$*"; } ok() { printf ' ok %s\n' "$*"; } warn() { printf ' WARN %s\n' "$*"; } @@ -114,7 +131,7 @@ while [ $# -gt 0 ]; do --wait-seconds) [ $# -ge 2 ] || die "--wait-seconds requires a number of seconds" WAIT_SECONDS_OVERRIDE="$2"; shift 2 ;; - -h|--help) sed -n '3,63p' "$SELF"; exit 0 ;; + -h|--help) sed -n '3,70p' "$SELF"; exit 0 ;; *) echo "unknown option: $1 (try --help)" >&2; exit 2 ;; esac done @@ -272,18 +289,62 @@ default_wait_seconds() { # # Timestamped, never pruned — "keep backups" per the ticket. A pid suffix avoids a same-second # collision between two invocations. +# +# fleetd #635 follow-up — lands under a DEDICATED, gitignored directory beside the config +# (/.config-backups/), never beside the config file itself. The whole reason fleetd.yaml is +# gitignored is that it must never be committed, and a backup of it inherits that requirement — a +# bare `fleetd.yaml.bak.*` next to a tracked directory is one `git add -A`/`git add .` away from +# committing the live config. A directory beats a glob on its own: the glob only protects today's +# naming, a location keeps working even if the naming changes later. (See .gitignore for the glob +# kept anyway, as a backstop for a stray backup written the old way.) +BACKUP_DIRNAME=".config-backups" + +backup_dir_for() { + local src="$1" + printf '%s/%s' "$(dirname "$src")" "$BACKUP_DIRNAME" +} + backup_config() { - local src="$1" ts backup + local src="$1" ts backup dir base + dir="$(backup_dir_for "$src")" + mkdir -p "$dir" \ + || die "could not create the backup directory $dir — refusing to edit without a backup. The live config at $src was NOT touched." ts="$(date -u +%Y%m%dT%H%M%S)Z" - backup="${src}.bak.${ts}.$$" + base="$(basename "$src")" + backup="${dir}/${base}.bak.${ts}.$$" cp "$src" "$backup" \ || die "could not create a backup at $backup — refusing to edit without one. The live config at $src was NOT touched." printf '%s' "$backup" } newest_backup() { - local cfg="$1" - ls -t "${cfg}".bak.* 2>/dev/null | head -1 || true + local cfg="$1" dir base + dir="$(backup_dir_for "$cfg")" + base="$(basename "$cfg")" + ls -t "${dir}/${base}".bak.* 2>/dev/null | head -1 || true +} + +# ------------------------------------------------------------------------------------ the file mode +# +# fleetd #635 follow-up — `mv` from a mktemp candidate carries mktemp's 0600 onto the live path +# forever (measured: 644 -> 600 after one --set), and a restore does not undo it either, because +# `cp` onto an EXISTING file keeps the DESTINATION's mode, not the source's. Capture the live +# file's mode before anything touches it, and reapply it to whatever lands on that path +# afterwards — the candidate before install, and the config again after a restore — so an edit +# changes the file's CONTENT only, never its permissions. BSD `stat -f '%Lp'` first (matches this +# project's dev machine), GNU `stat -c '%a'` as the fallback. Prints nothing when the file does +# not exist yet, so apply_mode then does nothing and a first-ever edit falls back to the normal +# umask default rather than inventing a number. +file_mode() { + local file="$1" + [ -f "$file" ] || return 0 + stat -f '%Lp' "$file" 2>/dev/null || stat -c '%a' "$file" 2>/dev/null || true +} + +apply_mode() { + local file="$1" mode="$2" + [ -n "$mode" ] || return 0 + chmod "$mode" "$file" 2>/dev/null || true } # --------------------------------------------------------------------------- candidate builders @@ -291,6 +352,23 @@ newest_backup() { # Never edit the live file in place. Each builder fills $1 (a temp file already sitting in the # SAME directory as the live config, so the later `mv` install is a rename, not a cross-device # copy — see run_edit). +# fleetd #635 follow-up — a forgotten value (`--set .a.b=`, a plausible typo) must never be +# accepted as "clear the field". `*=*` alone cannot tell "--set .a.b=" from "--set .a.b=7" apart +# — both contain an `=` — so the guard has to look at the VALUE, not the shape of the argument. +# An empty value refuses outright: nothing is installed, and the message names the likely cause +# AND the two ways to actually mean it (clear on purpose, or an intentional empty string via +# --from). Measured against the real daemon loader: a quoted empty string reads back as a null +# field (`quoted empty -> OK int=null`), and a null numeric field FALLS BACK TO ITS DEFAULT rather +# than erroring — so this is not a cosmetic nit, it is the one shape of edit that widens capacity +# silently instead of failing loudly, which is exactly what this script exists to catch. +# +# A deliberate clear needs its own spelling, because `""` and YAML `null` are NOT the same value +# to the loader (`""` is a valid empty String; `null` means absent, and an Integer field reads +# either the same way — null — but a String field would keep `""` as a real value). `--set +# .a.b=null` is that spelling: it writes a literal, unquoted `null` via yq, never the string +# "null" through strenv(). One consequence worth knowing: there is currently no --set spelling +# for the three-character STRING "null" itself (it collides with the clear spelling) — use +# --from for that rare case. apply_set_pairs() { local cand="$1" kv path value shift @@ -302,6 +380,17 @@ apply_set_pairs() { path="${kv%%=*}" path="${path#.}" value="${kv#*=}" + if [ -z "$value" ]; then + die "--set '$kv' has an EMPTY value — refusing. Nothing was installed. A forgotten value + would NULL the field, and a null value falls back to its default rather than erroring — + silent, not safe. Did you mean --set .${path}=null to clear it on purpose, or --from a + file if you need a genuinely empty string?" + fi + if [ "$value" = "null" ]; then + yq eval -i ".${path} = null" "$cand" \ + || die "yq could not clear '$kv' — nothing was installed. The live config is unchanged." + continue + fi CONFIG_EDIT_SET_VALUE="$value" yq eval -i ".${path} = strenv(CONFIG_EDIT_SET_VALUE)" "$cand" \ || die "yq could not apply --set '$kv' — nothing was installed. The live config is unchanged." done @@ -340,10 +429,12 @@ restore_command_line() { # restore itself reloaded cleanly. Never claims a restore it did not observe — if the second wait # also times out, it says so plainly rather than reporting "restored" as though confirmed. restore_and_confirm() { - local backup="$1" mark2 + local backup="$1" mark2 orig_mode + orig_mode="$(file_mode "$CONFIG")" mark2="$(log_mark "$LOG")" cp "$backup" "$CONFIG" \ || die "could not restore $backup onto $CONFIG — the live config is left as the REFUSED edit. Fix this by hand immediately: cp \"$backup\" \"$CONFIG\"" + apply_mode "$CONFIG" "$orig_mode" ok "restored from $backup" if wait_for_verdict "$LOG" "$mark2" "$WAIT_SECONDS"; then case "$VERDICT_KIND" in @@ -448,8 +539,9 @@ run_edit() { local builder="$1" [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to edit" - local mark + local mark orig_mode mark="$(log_mark "$LOG")" + orig_mode="$(file_mode "$CONFIG")" say "probe" local port @@ -467,25 +559,29 @@ run_edit() { say "candidate" local cand - cand="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ + CAND="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ || die "could not create a candidate temp file next to $CONFIG" + cand="$CAND" if ! "$builder" "$cand"; then - rm -f "$cand" + rm -f "$cand"; CAND="" die "could not build the candidate — nothing was installed. The live config at $CONFIG is unchanged." fi if ! parse_check "$cand"; then - rm -f "$cand" + rm -f "$cand"; CAND="" die "candidate does not parse as valid YAML — nothing was installed. The live config at $CONFIG is unchanged." fi ok "candidate parses" + apply_mode "$cand" "$orig_mode" + say "change (redacted)" diff -u "$backup" "$cand" | redact || true say "install" install_candidate "$cand" "$CONFIG" \ || die "could not install the candidate onto $CONFIG — the live config was NOT changed. The validated candidate is sitting at $cand; investigate before retrying." + CAND="" ok "installed: $CONFIG" local rc=0 @@ -497,19 +593,20 @@ dry_run_diff() { local builder="$1" [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to diff against" local cand - cand="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ + CAND="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ || die "could not create a candidate temp file next to $CONFIG" + cand="$CAND" if ! "$builder" "$cand"; then - rm -f "$cand" + rm -f "$cand"; CAND="" die "could not build the candidate — this was a --dry-run, nothing would have been installed either" fi if ! parse_check "$cand"; then - rm -f "$cand" + rm -f "$cand"; CAND="" die "candidate does not parse as valid YAML — this was a --dry-run, nothing would have been installed either" fi say "dry run — diff (redacted), nothing installed" diff -u "$CONFIG" "$cand" | redact || true - rm -f "$cand" + rm -f "$cand"; CAND="" return 0 } @@ -522,9 +619,11 @@ restore_mode() { say "restore" ok "restoring $backup onto $CONFIG" - local mark + local mark orig_mode mark="$(log_mark "$LOG")" + orig_mode="$(file_mode "$CONFIG")" cp "$backup" "$CONFIG" || die "could not copy $backup onto $CONFIG" + apply_mode "$CONFIG" "$orig_mode" ok "installed: $CONFIG" local rc=0 diff --git a/scripts/test-config-edit.sh b/scripts/test-config-edit.sh index a6e5b4c..20bd1cd 100755 --- a/scripts/test-config-edit.sh +++ b/scripts/test-config-edit.sh @@ -51,6 +51,7 @@ broker: profiles: sonnet: weight: 3 + maxLoad: 5 YAML : > "$dir/fleetd.out" printf '%s' "$dir" @@ -143,7 +144,7 @@ test_silence_is_its_own_answer() { assert_contains '--restore' "$RUN_OUTPUT" "silence prints the --restore command" local backup restore_cmd - backup="$(ls -t "$dir"/fleetd.yaml.bak.* | head -1)" + backup="$(ls -t "$dir"/.config-backups/fleetd.yaml.bak.* | head -1)" [ -n "$backup" ] || fail "silence must still have taken a backup" restore_cmd="$(printf '%s\n' "$RUN_OUTPUT" | grep -F -- '--restore --config' | sed -E 's/^[[:space:]]*//')" @@ -186,6 +187,7 @@ broker: profiles: sonnet: weight: 3 + maxLoad: 5 YAML } @@ -203,7 +205,14 @@ test_marker_skips_lines_before_it() { assert_equals 0 "$RUN_RC" "a stale refusal before the marker must not be read as this edit's verdict" } -# ------------------------------------------------------------------- acceptance criterion 7 +# ------------------------------------------------------------------- acceptance criterion 7 (+13) +# fleetd #635 follow-up (ticket comment 17659) — the two assertions below this comment were the +# WHOLE test before the follow-up, and both are negative-only: they pass just as happily when the +# diff is never printed at all as when it is printed and correctly redacted. A mutant that deletes +# `diff -u "$backup" "$cand" | redact` from the edit path survives them, because an absent output +# contains neither "hunter2" nor "user:" either — see the mutation-and-revert proof in the reply. +# Criterion 13 is the fix: a LOUD positive control that only passes when a diff was demonstrably +# printed AND the redaction demonstrably ran on real content, not merely that nothing leaked. test_redaction_holds() { local dir dir="$(new_fixture)" @@ -216,6 +225,121 @@ test_redaction_holds() { assert_equals 0 "$RUN_RC" "redaction-case reload exit code" assert_not_contains "hunter2" "$RUN_OUTPUT" "full output must never contain the password" assert_not_contains "user:" "$RUN_OUTPUT" "full output must never contain the userinfo" + # acceptance criterion 13 — positive control: the diff's default 3-line context around the + # changed "weight" key also covers the fixture's "uri:" line, so a genuinely-printed, genuinely- + # redacted diff must contain BOTH the redaction marker and the changed key's name. A test that + # only ever asserts absence cannot tell "redacted" from "never printed" apart; this can. + assert_contains "" "$RUN_OUTPUT" "the redaction must be PROVEN to have run on real content, not merely absent" + assert_contains "weight" "$RUN_OUTPUT" "a diff must have been demonstrably printed at all" +} + +# ------------------------------------------------------- acceptance criterion 9: forgotten value +# `--set .a.b=` is a plausible typo (the value simply forgotten), and it must be refused outright +# rather than silently nulling the field — a null numeric field falls back to its default, which +# widens capacity instead of failing loudly. No background verdict feeder here: a refused --set +# must never even reach the daemon, so this never starts a background run at all. +test_forgotten_value_refuses_and_installs_nothing() { + local dir rc=0 + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + + "$EDIT" --set '.profiles.sonnet.maxLoad=' \ + --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 || rc=$? + RUN_OUTPUT="$(cat "$dir/stdout.log")" + + [ "$rc" -ne 0 ] || fail "an empty --set value must exit non-zero, got 0" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "an empty --set value must install nothing — the live fixture changed" + assert_contains "EMPTY value" "$RUN_OUTPUT" "the refusal must name the empty value" +} + +# ---------------------------------------------------------- acceptance criterion 10: explicit null +# `--set .a.b=null` is the deliberate-clear spelling, and it must write a REAL yaml null, never +# the string "''" — those are different values to the daemon's loader (fleetd ticket #635's +# follow-up comment measured `""` reading back as a null field anyway, which is exactly why the +# two forms must not collapse onto each other: `--set path=` refuses instead of silently reaching +# this same null outcome through the back door). Read the RAW line with grep, never only through +# `yq` — `yq eval` reports `null` for both an actual null and a missing/absent key, so it cannot +# tell "wrote null" apart from "wrote nothing"; only the literal line on disk can. +test_explicit_null_writes_bare_null_not_empty_string() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.maxLoad=null' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "explicit null clear exit code" + local raw_line + raw_line="$(grep -E 'maxLoad' "$dir/fleetd.yaml")" + assert_contains "null" "$raw_line" "the installed line must spell a bare null" + assert_not_contains '""' "$raw_line" "the installed line must NOT be a quoted empty string" +} + +# ------------------------------------------------------- acceptance criterion 11: backup never committable +# A backup of fleetd.yaml inherits fleetd.yaml's own "never commit this" requirement (fleetd #635 +# follow-up, ticket comment 17655). Proves two things: the backup lands somewhere `git +# check-ignore` reports as ignored (equivalently, a path `git status --porcelain` never lists as +# untracked), AND that --restore still finds and uses it from that location. +test_backup_is_never_committable() { + local dir backup + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=55' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "setup edit exit code" + + backup="$(ls -t "$dir"/.config-backups/fleetd.yaml.bak.* 2>/dev/null | head -1)" + [ -n "$backup" ] || fail "no backup found under .config-backups/ — did the location change?" + + git -C "$ROOT" check-ignore -q -- "$backup" \ + || fail "the backup at $backup is NOT gitignored — it would survive a git add -A" + if git -C "$ROOT" status --porcelain -- "$backup" 2>/dev/null | grep -q '^??'; then + fail "git status still lists the backup as untracked: $backup" + fi + + start_run "$dir" 5 --restore + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "--restore after the backup-location change exit code" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "--restore from the new backup location must still put the file back byte for byte" +} + +# --------------------------------------------------------- acceptance criterion 12: file mode +# `mv` from a mktemp candidate carries mktemp's 0600 forever, and a plain `cp` onto an existing +# file keeps the DESTINATION's mode rather than the source's, so a restore does not undo the +# narrowing either (fleetd #635 follow-up, ticket comment 17657). Proves the mode survives an edit +# AND a subsequent restore, from two different starting points — 644 is the common case, 600 +# proves the fix PRESERVES whatever mode was there rather than hardcoding 644. +test_file_mode_survives_edit_and_restore() { + local dir want got + for want in 644 600; do + dir="$(new_fixture)" + chmod "$want" "$dir/fleetd.yaml" + + start_run "$dir" 5 --set ".profiles.sonnet.weight=${want}" + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "mode-preservation setup edit exit code ($want)" + got="$(stat -f '%Lp' "$dir/fleetd.yaml" 2>/dev/null || stat -c '%a' "$dir/fleetd.yaml")" + assert_equals "$want" "$got" "mode must survive a --set ($want)" + + start_run "$dir" 5 --restore + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "mode-preservation restore exit code ($want)" + got="$(stat -f '%Lp' "$dir/fleetd.yaml" 2>/dev/null || stat -c '%a' "$dir/fleetd.yaml")" + assert_equals "$want" "$got" "mode must survive a --restore ($want)" + done } # dry-run must never touch the live file and must still redact. @@ -234,6 +358,8 @@ test_dry_run_never_installs_and_redacts() { assert_equals "$before" "$(cat "$dir/fleetd.yaml")" "dry-run must never write the live config" assert_not_contains "hunter2" "$RUN_OUTPUT" "dry-run diff must also be redacted" assert_contains "99" "$RUN_OUTPUT" "dry-run diff must show the candidate value" + # Same positive-control reasoning as acceptance criterion 13, applied to the dry-run diff path. + assert_contains "" "$RUN_OUTPUT" "the dry-run diff's redaction must be PROVEN to have run, not merely absent" } # --check is read-only and always exits 0, even against a dead "daemon". @@ -273,8 +399,16 @@ echo "== acceptance criterion 5: broken candidate never reaches the live path == test_broken_candidate_never_reaches_live_path echo "== acceptance criterion 6: the marker works ==" test_marker_skips_lines_before_it -echo "== acceptance criterion 7: the redaction holds ==" +echo "== acceptance criterion 7 (+13: redaction is proven to have run) ==" test_redaction_holds +echo "== acceptance criterion 9: a forgotten value refuses and installs nothing ==" +test_forgotten_value_refuses_and_installs_nothing +echo "== acceptance criterion 10: an explicit clear writes a bare null ==" +test_explicit_null_writes_bare_null_not_empty_string +echo "== acceptance criterion 11: a backup is never committable ==" +test_backup_is_never_committable +echo "== acceptance criterion 12: the file mode survives an edit and a restore ==" +test_file_mode_survives_edit_and_restore echo "== extra: dry-run never installs, and redacts ==" test_dry_run_never_installs_and_redacts echo "== extra: --check is read-only and always exits 0 =="