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 new file mode 100755 index 0000000..7d47d63 --- /dev/null +++ b/scripts/config-edit.sh @@ -0,0 +1,726 @@ +#!/usr/bin/env bash +# +# The one auditable way to edit the live fleetd.yaml. +# +# fleetd ticket #635 — why this exists at all: fleetd.yaml is gitignored and holds the live +# fleet's settings. A bad raw edit reaches a daemon that is already serving, so a direct `Edit` +# on it is refused by policy. This script is the allow-listed alternative, and it is not just +# convenience — it is the thing a raw file write can never give you: a backup, a parse check +# BEFORE the file is installed, and the daemon's own reload verdict read back afterwards. An +# edit to a live config is not finished when the bytes are written. It is finished when the +# daemon has said what it did with them. +# +# What the daemon says, and how this script finds it — measured against `ConfigRef.java` on +# fleetd commit 158a2a8, 2026-10-01: +# +# 1. `ConfigRef` re-reads fleetd.yaml only when the WATCHER sees the mtime move (every 10s by +# default — read the real interval out of the daemon's own startup line, "config watch: ... +# re-read when it changes (every Ns)"). So a verdict never appears before the next tick. +# 2. `ConfigRef.Outcome.summary()` logs exactly one of five strings (ConfigRef.java:371-391): +# config reload refused — +# config reload refused — these keys cannot change under a running daemon: . ... +# config reloaded +# config reloaded; these changes need a restart to take effect: +# config reloaded; partially live — +# A parse/validation failure logs a DIFFERENT line instead, before any summary ever runs +# (ConfigRef.java:425): "config reload from refused, keeping the running config: +# ". This script recognises both shapes of refusal. +# 3. The em dash in those strings is a real multi-byte character — match the stable prefix +# "config reload refused" (or "...refused, keeping the running config" for the parse-failure +# shape), never the dash itself. +# 4. A cold-key change (bind/herdrSocket/memberHerdrSocket/broker/auth) throws away the WHOLE +# reload — the running config keeps every old value, not only the cold one. +# 5. A deferred/split change IS applied (current.set(fresh) runs) — "needs a restart" is a +# SUCCESS with a follow-up, never a failure. +# +# Four outcomes, and they stay four (see the exit code table below). The one most likely to be +# gotten wrong is "cannot tell" (exit 5): the daemon may be down, or the watcher may be stalled, +# and folding that into either "refused" or "applied" is worse than never checking at all, +# because a caller then acts on a verdict nobody actually read. So exit 5 never restores — a +# visible, recoverable edit beats an invisible revert of a GOOD edit. +# +# Usage: +# scripts/config-edit.sh --check +# scripts/config-edit.sh --set = [--set ...] +# scripts/config-edit.sh --from +# 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 +# --wait-seconds default: 4x the watch interval this script reads out of --log (10 -> 40) +# +# Exit codes (the --check/--restore/usage-error paths are reported separately, see below): +# 0 applied; verdict read; clean +# 3 applied; verdict read; needs a restart (deferred or split keys named) +# 4 REFUSED by the daemon; backup restored (and the restore's own verdict reported if seen) +# 5 CANNOT TELL — no verdict line inside the wait window. Nothing is restored. +# +# Never prints a secret. fleetd.yaml keeps credentials out by indirection (broker.uriEnv, +# gitTokenEnv) but this script does not rely on that staying true: every diff it prints is piped +# through `redact`, which (a) blanks the userinfo of any `scheme://user:pass@host` and (b) masks +# the whole value on any line whose key looks like a credential. See `redact` below. + +set -euo pipefail + +REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +SELF="$REPO/scripts/config-edit.sh" + +CONFIG="$REPO/fleetd/fleetd.yaml" +LOG="$REPO/fleetd/fleetd.out" +WAIT_SECONDS_OVERRIDE="" +FALLBACK_PORT=8765 + +MODE="" +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' "$*"; } +die() { printf '\n FAIL %s\n\n' "$*" >&2; exit 1; } + +set_mode() { + local new="$1" + if [ -n "$MODE" ] && [ "$MODE" != "$new" ]; then + die "cannot combine --$MODE and --$new in one invocation" + fi + MODE="$new" +} + +while [ $# -gt 0 ]; do + case "$1" in + --check) set_mode check; shift ;; + --restore) set_mode restore; shift ;; + --set) + [ $# -ge 2 ] || die "--set requires =" + set_mode set + SETS+=("$2") + shift 2 ;; + --from) + [ $# -ge 2 ] || die "--from requires a candidate file path" + set_mode from + FROM_FILE="$2" + shift 2 ;; + --dry-run) DRY_RUN=1; shift ;; + --config) + [ $# -ge 2 ] || die "--config requires a path" + CONFIG="$2"; shift 2 ;; + --log) + [ $# -ge 2 ] || die "--log requires a path" + LOG="$2"; shift 2 ;; + --wait-seconds) + [ $# -ge 2 ] || die "--wait-seconds requires a number of seconds" + WAIT_SECONDS_OVERRIDE="$2"; shift 2 ;; + -h|--help) sed -n '3,70p' "$SELF"; exit 0 ;; + *) echo "unknown option: $1 (try --help)" >&2; exit 2 ;; + esac +done + +[ -n "$MODE" ] || die "no action given — use --check, --set, --from, or --restore (see --help)" + +# ------------------------------------------------------------------------------------- redaction +# +# Two independent passes, applied to every diff this script ever prints: +# 1. `scheme://user:pass@host` -> `scheme://@host`, globally (the `g` flag matters — +# a line can carry more than one URI). +# 2. Any line whose key looks like TOKEN|SECRET|PASSWORD|PASSWD|PASSPHRASE|CREDENTIAL|URI|_KEY, +# matched case-insensitively against the key text (uriEnv, gitTokenEnv, ... are camelCase, +# not SCREAMING_CASE) has its whole value blanked, diff marker and indentation kept so the +# shape of the change is still visible. Deliberately conservative: a false-positive +# redaction on an unrelated line costs nothing, an unredacted secret is a security defect +# (acceptance criterion 7). +# +# fleetd #635 follow-up (ticket comment 17670, defect 7) — a masked key line is not the whole +# story: a YAML block scalar (`|`, `|-`, `>`, `>-`, ...) puts the VALUE on the lines that follow +# the key, each indented deeper than it. The key-name match above only ever sees the key line +# itself, so those continuation lines used to flow straight through unredacted while the key line +# right above them printed a reassuring "" — an incomplete redactor that looks complete +# is worse than one that visibly does nothing, because it stops a reviewer from looking further. +# The fix is structural, not another name to match: once a key line is masked, every following +# line indented STRICTLY DEEPER than that key is masked too, by indentation alone, until the +# indentation returns to the key's own level or shallower. This needs no knowledge of the key's +# name and so protects a block scalar under any masked key, present or future. +# +# `redact` is always fed `diff -u` output, and every line of a unified diff starts with exactly +# one of ' ', '+', '-' (the three body markers; '@'/'-'/'+' for the three header-line kinds too). +# That one leading character is NOT part of the YAML indentation, and must be stripped before +# indentation is measured or a key is matched — otherwise a changed ('+' or '-') line reads one +# column shallower than it really is, and either wrongly escapes a continuation mask or wrongly +# ends one early. Tabs are out of scope: YAML forbids them for indentation, and this is a bounded +# fix, not a YAML parser. +redact() { + local line prefix content indent lead key + local masked=0 masked_indent=0 saved_nocasematch=0 + shopt -q nocasematch && saved_nocasematch=1 + shopt -s nocasematch + sed -E 's#://[^@]*@#://@#g' | while IFS= read -r line || [ -n "$line" ]; do + case "$line" in + [\ +-]*) prefix="${line:0:1}"; content="${line:1}" ;; + *) prefix=""; content="$line" ;; + esac + + indent=0 + while [ "${content:$indent:1}" = " " ]; do indent=$((indent + 1)); done + + if [ "$masked" = 1 ] && [ "$indent" -gt "$masked_indent" ]; then + printf '%s%*s\n' "$prefix" "$indent" "" + continue + fi + masked=0 + + if [[ "$content" =~ ^([[:space:]]*)([A-Za-z0-9_.-]+:) ]]; then + lead="${BASH_REMATCH[1]}" + key="${BASH_REMATCH[2]}" + if [[ "$key" =~ (TOKEN|SECRET|PASSWORD|PASSWD|PASSPHRASE|CREDENTIAL|URI|_KEY) ]]; then + printf '%s%s%s \n' "$prefix" "$lead" "$key" + masked=1 + masked_indent="$indent" + continue + fi + fi + printf '%s\n' "$line" + done + [ "$saved_nocasematch" = 1 ] || shopt -u nocasematch +} + +# ------------------------------------------------------------------------------------- the probe +# +# Probe the SOCKET, never `pgrep`/`ps -f` — both print argv, and argv holds `NAME=value`, making +# either a credential channel. The port comes from the config's own `bind.port`; 8765 is only a +# fallback when that key is absent or the file does not parse yet. +resolve_port() { + local file="$1" port + if [ -f "$file" ] && command -v yq >/dev/null 2>&1; then + port="$(yq eval '.bind.port' "$file" 2>/dev/null || true)" + else + port="" + fi + case "$port" in + ''|null) echo "$FALLBACK_PORT" ;; + *) echo "$port" ;; + esac +} + +daemon_listening() { + local port="$1" + if command -v nc >/dev/null 2>&1; then + nc -z -w1 127.0.0.1 "$port" 2>/dev/null + else + ( exec 3<>"/dev/tcp/127.0.0.1/$port" ) 2>/dev/null + fi +} + +# --------------------------------------------------------------------------------- the log marker +# +# Take the log's line count BEFORE touching anything. Every later read of "what did the daemon +# say" starts strictly after this mark, so a refusal from hours ago can never be mistaken for +# this edit's verdict. Same approach as scripts/redeploy-fleetd.sh's RESTART_MARK. +log_mark() { + local file="$1" + if [ -f "$file" ]; then + wc -l < "$file" 2>/dev/null || echo 0 + else + echo 0 + fi +} + +read_verdict_after_marker() { + local file="$1" mark="$2" + [ -f "$file" ] || return 0 + tail -n "+$((mark + 1))" "$file" 2>/dev/null || true +} + +# Classifies one log LINE. Echoes one of: refused | clean | needs-restart | none. Always +# succeeds (every branch ends in `echo`), so it is safe to call from inside `$( )`. +classify_verdict_line() { + local line="$1" + case "$line" in + *'config reload refused'*) echo refused ;; + *'config reload from '*'refused, keeping the running config'*) echo refused ;; + *'config reloaded'*) + case "$line" in + *'need a restart'*|*'partially live'*) echo needs-restart ;; + *) echo clean ;; + esac ;; + *) echo none ;; + esac + return 0 +} + +scan_region_for_verdict() { + local region="$1" line kind + [ -n "$region" ] || return 1 + while IFS= read -r line || [ -n "$line" ]; do + kind="$(classify_verdict_line "$line")" + if [ "$kind" != "none" ]; then + VERDICT_KIND="$kind" + VERDICT_LINE="$line" + return 0 + fi + done <<< "$region" + return 1 +} + +# Sets VERDICT_KIND/VERDICT_LINE and returns 0 on the first verdict line found after $mark; +# returns 1 (VERDICT_KIND=none) if none appeared inside $wait_s seconds. Checks once before each +# sleep AND once more after the last sleep, the same boundary idiom +# scripts/redeploy-fleetd.sh's wait_for_daemon_exit/wait_for_new_pid already use. +wait_for_verdict() { + local log="$1" mark="$2" wait_s="$3" _i region + VERDICT_KIND="none" + VERDICT_LINE="" + for _i in $(seq "$wait_s"); do + region="$(read_verdict_after_marker "$log" "$mark")" + scan_region_for_verdict "$region" && return 0 + sleep 1 + done + region="$(read_verdict_after_marker "$log" "$mark")" + scan_region_for_verdict "$region" && return 0 + return 1 +} + +last_verdict_line() { + local file="$1" line out="" + [ -f "$file" ] || return 0 + while IFS= read -r line || [ -n "$line" ]; do + if [ "$(classify_verdict_line "$line")" != "none" ]; then + out="$line" + fi + done < "$file" + printf '%s' "$out" +} + +default_wait_seconds() { + local log="$1" interval="" + if [ -f "$log" ]; then + interval="$(grep -F 'config watch:' "$log" 2>/dev/null | tail -1 \ + | sed -E 's/.*\(every ([0-9]+)s\).*/\1/' || true)" + fi + case "$interval" in + ''|*[!0-9]*) interval=10 ;; + esac + echo $((interval * 4)) +} + +# ----------------------------------------------------------------------------------- the backup +# +# 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 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" + 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" 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 +# +# 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 + for kv in "$@"; do + case "$kv" in + *=*) : ;; + *) die "--set expects =, got: '$kv'" ;; + esac + 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 + # fleetd #635 follow-up (ticket comment 17673, defect 8) — these two failure messages used to + # echo the full "$kv" (path=value, exactly as the operator typed it), unredacted. The operator + # already has the value, so a terminal is not where this leaks — the risk is where the output + # goes NEXT: this fleet pastes command output into tickets, PRs and fleet_reply bodies, and a + # failure is exactly when someone copies it to ask for help. Print the PATH, which is what's + # needed to fix the command, and never the value. $kv is not key:value-shaped YAML, so piping + # it through redact would just pass it straight through — a false sense of coverage, the same + # mistake as defect 7. + if [ "$value" = "null" ]; then + yq eval -i ".${path} = null" "$cand" \ + || die "yq could not clear --set '.${path}=null' — 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 '.${path}=' — nothing was installed. The live config is unchanged." + done +} + +build_from_set() { + local cand="$1" + cp "$CONFIG" "$cand" + apply_set_pairs "$cand" "${SETS[@]}" +} + +build_from_file() { + local cand="$1" + [ -f "$FROM_FILE" ] || die "--from file not found: $FROM_FILE" + cp "$FROM_FILE" "$cand" +} + +parse_check() { + yq eval '.' "$1" >/dev/null 2>&1 +} + +install_candidate() { + local cand="$1" live="$2" + mv -f "$cand" "$live" +} + +# -------------------------------------------------------------------------------- the report path +# +# Prints the literal command the operator (or a test) can run to restore the backup by hand — the +# absolute path to THIS script plus the overrides actually in force, so it works from any cwd. +restore_command_line() { + printf '%q --restore --config %q --log %q --wait-seconds %q' "$SELF" "$CONFIG" "$LOG" "$WAIT_SECONDS" +} + +# State 4 only: restore the pre-edit backup, then wait for a SECOND verdict confirming the +# 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 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 + refused) warn "the RESTORE was also refused by the daemon: $VERDICT_LINE" ;; + *) ok "restore confirmed: $VERDICT_LINE" ;; + esac + else + warn "the restore is on disk, but no confirming verdict line appeared within ${WAIT_SECONDS}s" + warn "cannot confirm the restore reloaded cleanly — check $LOG by hand" + fi + return 0 +} + +# The four-outcome decision. Echoed as a function so run_edit/restore_mode share one place that +# can return 0/3/4/5 — never duplicated, never re-worded between the two callers. +report_outcome() { + local mark="$1" backup="$2" kind line + + say "waiting for the daemon's verdict (up to ${WAIT_SECONDS}s)" + if wait_for_verdict "$LOG" "$mark" "$WAIT_SECONDS"; then + kind="$VERDICT_KIND"; line="$VERDICT_LINE" + else + kind="none" + fi + + case "$kind" in + clean) + ok "daemon verdict: $line" + say "result: applied cleanly" + return 0 ;; + needs-restart) + ok "daemon verdict: $line" + say "result: applied — a restart is needed for the change(s) named above" + return 3 ;; + refused) + warn "daemon verdict: $line" + say "result: REFUSED — restoring the backup" + restore_and_confirm "$backup" + return 4 ;; + none) + warn "no verdict line appeared within ${WAIT_SECONDS}s after $LOG line $mark" + warn "CANNOT TELL whether the daemon applied this edit, refused it, or is simply down." + warn "Nothing was restored — the edit is still on disk at $CONFIG." + echo + echo " backup: $backup" + echo " to restore it by hand:" + echo " $(restore_command_line)" + return 5 ;; + esac +} + +# ------------------------------------------------------------------------------------- the modes +check_mode() { + say "config-edit --check" + if [ -f "$CONFIG" ]; then + if parse_check "$CONFIG"; then + ok "config parses: $CONFIG" + else + warn "config does NOT parse as valid YAML: $CONFIG" + fi + else + warn "no config file at $CONFIG" + fi + + local port + port="$(resolve_port "$CONFIG")" + if daemon_listening "$port"; then + ok "daemon is listening on 127.0.0.1:$port" + else + warn "no daemon detected listening on 127.0.0.1:$port" + fi + + ok "watch interval assumed: $(( $(default_wait_seconds "$LOG") / 4 ))s (derives --wait-seconds default of $(default_wait_seconds "$LOG")s)" + + local verdict + verdict="$(last_verdict_line "$LOG")" + if [ -n "$verdict" ]; then + ok "last verdict in log: $verdict" + else + warn "no reload verdict line found in $LOG" + fi + + local backup + backup="$(newest_backup "$CONFIG")" + if [ -n "$backup" ]; then + ok "newest backup: $backup" + else + warn "no backups found for $CONFIG" + fi + + if command -v yq >/dev/null 2>&1; then + ok "yq: $(yq --version 2>&1)" + else + warn "yq not found on PATH" + fi + + return 0 +} + +# Shared by --set and --from: backup, build, parse-check, redacted diff, install, await verdict. +run_edit() { + local builder="$1" + [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to edit" + + local mark orig_mode + mark="$(log_mark "$LOG")" + orig_mode="$(file_mode "$CONFIG")" + + say "probe" + local port + port="$(resolve_port "$CONFIG")" + if daemon_listening "$port"; then + ok "daemon appears to be listening on 127.0.0.1:$port" + else + warn "no daemon detected listening on 127.0.0.1:$port — a verdict may never appear" + fi + + say "backup" + local backup + backup="$(backup_config "$CONFIG")" + ok "backup: $backup" + + say "candidate" + local cand + 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"; 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"; 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 + report_outcome "$mark" "$backup" || rc=$? + return "$rc" +} + +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")" \ + || die "could not create a candidate temp file next to $CONFIG" + cand="$CAND" + if ! "$builder" "$cand"; then + 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"; 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"; CAND="" + return 0 +} + +restore_mode() { + [ -f "$CONFIG" ] || die "no config at $CONFIG to restore onto" + local backup dir base + backup="$(newest_backup "$CONFIG")" + if [ -z "$backup" ]; then + # fleetd #635 follow-up (ticket comment 17664) — this message must name the directory the + # code actually searches (backup_dir_for, same as newest_backup), not the old beside-the- + # config glob. A backup written the OLD way is real and NOT searched any more — say so and + # give the one-line recovery command — but do NOT make the search itself look there; that + # would be a behaviour change nobody asked for. The message is the only thing being fixed. + dir="$(backup_dir_for "$CONFIG")" + base="$(basename "$CONFIG")" + die "no backup found matching ${dir}/${base}.bak.* — nothing to restore. + A backup written the OLD way, directly beside the config (${CONFIG}.bak.*), is NOT + searched — that location was retired so a backup of a file that must never be committed + cannot sit next to a tracked directory. If one exists there, recover it by hand: + cp ${CONFIG}.bak.. $CONFIG" + fi + [ -f "$backup" ] || die "backup candidate $backup vanished" + + say "restore" + ok "restoring $backup onto $CONFIG" + 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 + report_outcome "$mark" "$backup" || rc=$? + return "$rc" +} + +# -------------------------------------------------------------------------------------- dispatch + +if [ -n "$WAIT_SECONDS_OVERRIDE" ]; then + WAIT_SECONDS="$WAIT_SECONDS_OVERRIDE" +else + WAIT_SECONDS="$(default_wait_seconds "$LOG")" +fi + +RC=0 +case "$MODE" in + check) + check_mode || RC=$? + ;; + set) + [ "${#SETS[@]}" -gt 0 ] || die "--set requires at least one =" + if [ "$DRY_RUN" = 1 ]; then + dry_run_diff build_from_set || RC=$? + else + run_edit build_from_set || RC=$? + fi + ;; + from) + [ -n "$FROM_FILE" ] || die "--from requires a candidate file path" + if [ "$DRY_RUN" = 1 ]; then + dry_run_diff build_from_file || RC=$? + else + run_edit build_from_file || RC=$? + fi + ;; + restore) + restore_mode || RC=$? + ;; +esac + +exit "$RC" diff --git a/scripts/test-config-edit.sh b/scripts/test-config-edit.sh new file mode 100755 index 0000000..3518771 --- /dev/null +++ b/scripts/test-config-edit.sh @@ -0,0 +1,585 @@ +#!/usr/bin/env bash +# Self-contained checks for scripts/config-edit.sh — fleetd ticket #635. +# +# Drives the REAL config-edit.sh as a subprocess against a FIXTURE config and a FIXTURE log in a +# throwaway temp directory this file creates and removes. Never touches fleetd/fleetd.yaml or +# fleetd/fleetd.out, and never starts, stops, or contacts a daemon — there is no daemon here, so +# each test PLAYS the daemon: it starts config-edit.sh in the background (it is waiting on the +# log), appends the verdict line it wants, then collects the real exit code. + +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +EDIT="$ROOT/scripts/config-edit.sh" +TMP="$(mktemp -d "$ROOT/.config-edit-test.XXXXXX")" +trap 'rm -rf "$TMP"' EXIT + +fail() { + printf 'FAIL: %s\n' "$*" >&2 + return 1 +} + +assert_equals() { + local expected="$1" actual="$2" description="$3" + [ "$expected" = "$actual" ] || fail "$description: expected $expected, got $actual" +} + +assert_contains() { + local needle="$1" text="$2" description="$3" + printf '%s' "$text" | grep -qF -- "$needle" || fail "$description: missing [$needle]" +} + +assert_not_contains() { + local needle="$1" text="$2" description="$3" + if printf '%s' "$text" | grep -qF -- "$needle"; then + fail "$description: must NOT contain [$needle], but it does" + fi + return 0 +} + +# A fresh fixture pair per test: $1/fleetd.yaml (the config) and $1/fleetd.out (the log), plus a +# small wait-seconds budget so no test takes long. Returns the fixture dir via stdout. +new_fixture() { + local dir + dir="$(mktemp -d "$TMP/fixture.XXXXXX")" + cat > "$dir/fleetd.yaml" <<'YAML' +bind: + host: 127.0.0.1 + port: 19999 +broker: + uri: amqp://user:hunter2@host/vhost +profiles: + sonnet: + weight: 3 + maxLoad: 5 +YAML + : > "$dir/fleetd.out" + printf '%s' "$dir" +} + +# Runs config-edit.sh in the background against $dir's fixtures, with the given extra args, and +# a short --wait-seconds. Sets RUN_PID. Caller appends to $dir/fleetd.out (or not, for the +# silence test) and then calls collect_run to block for the exit code. +start_run() { + local dir="$1" wait_s="$2"; shift 2 + ( + # config-edit.sh deliberately exits 3/4/5 on several of these tests. This subshell inherits + # the parent's `set -e`, and without disabling it here the FIRST nonzero exit would kill the + # subshell before the `echo $? > rc` line ever ran — the real code would never reach the file. + set +e + "$EDIT" --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds "$wait_s" "$@" \ + > "$dir/stdout.log" 2>&1 + echo $? > "$dir/rc" + ) & + RUN_PID=$! +} + +collect_run() { + local dir="$1" + # wait echoes back the backgrounded subshell's own exit status (here, deliberately 3/4/5 on + # several tests) — under `set -e` a bare nonzero `wait` would abort this whole test script, so + # it is neutralized with `|| true`; the real code is read from $dir/rc right after. + wait "$RUN_PID" || true + RUN_OUTPUT="$(cat "$dir/stdout.log")" + RUN_RC="$(cat "$dir/rc")" +} + +# -------------------------------------------------------------- acceptance criterion 1: refusal +test_refusal_restores_byte_for_byte() { + local dir + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + + start_run "$dir" 5 --set '.broker.uri=amqp://changed@host/x' + sleep 1 + printf 'config reload refused — these keys cannot change under a running daemon: broker. Restart fleetd to apply them.\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 4 "$RUN_RC" "refusal exit code" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "refusal must restore the config byte for byte onto the pre-edit backup" +} + +# -------------------------------------------------------------- acceptance criterion 2: clean +test_clean_reload_keeps_the_edit() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=7' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "clean reload exit code" + assert_equals "7" "$(yq eval '.profiles.sonnet.weight' "$dir/fleetd.yaml")" "clean reload live value" +} + +# ----------------------------------------------------- acceptance criterion 3: deferred != clean +test_deferred_reload_is_told_apart_from_clean() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=9' + sleep 1 + printf 'config reloaded; these changes need a restart to take effect: profiles\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 3 "$RUN_RC" "deferred reload exit code" + [ "$RUN_RC" != 0 ] || fail "deferred reload must not report exit 0" + assert_contains "profiles" "$RUN_OUTPUT" "deferred reload names the key" + assert_contains "restart" "$RUN_OUTPUT" "deferred reload says a restart is needed" +} + +# -------------------------------------------------------------- acceptance criterion 4: silence +test_silence_is_its_own_answer() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 2 --set '.profiles.sonnet.weight=11' + # Feed the log nothing. + collect_run "$dir" + + assert_equals 5 "$RUN_RC" "silence exit code" + assert_equals "11" "$(yq eval '.profiles.sonnet.weight' "$dir/fleetd.yaml")" "the edited value must still be on disk" + assert_contains '--restore' "$RUN_OUTPUT" "silence prints the --restore command" + + local backup restore_cmd + 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:]]*//')" + [ -n "$restore_cmd" ] || fail "could not find the printed --restore invocation in the output" + # Running this --restore invocation installs the backup, then itself waits for a confirming + # verdict that this fixture never feeds — so it legitimately exits 5 ("cannot tell") here, same + # as any edit with no daemon on the other end. Only a usage/internal error (1 or 2) is a real + # failure of the command itself; the actual assertion is the byte-for-byte cmp below. + local restore_rc=0 + eval "$restore_cmd" > "$dir/restore.log" 2>&1 || restore_rc=$? + case "$restore_rc" in + 0|3|4|5) : ;; + *) fail "the printed --restore command errored out (exit $restore_rc): $(cat "$dir/restore.log")" ;; + esac + + cmp -s "$dir/fleetd.yaml" "$backup" \ + || fail "running the printed --restore command must put the file back to the original backup" +} + +# --------------------------------------------------------- acceptance criterion 5: bad candidate +test_broken_candidate_never_reaches_live_path() { + local dir rc=0 + dir="$(new_fixture)" + printf 'foo: [unclosed\n' > "$dir/broken.yaml" + + "$EDIT" --from "$dir/broken.yaml" --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 || rc=$? + + [ "$rc" -ne 0 ] || fail "a broken --from candidate must exit non-zero" + cmp -s "$dir/fleetd.yaml" <(new_fixture_yaml) \ + || fail "the broken candidate must never reach the live fixture config" +} +new_fixture_yaml() { + cat <<'YAML' +bind: + host: 127.0.0.1 + port: 19999 +broker: + uri: amqp://user:hunter2@host/vhost +profiles: + sonnet: + weight: 3 + maxLoad: 5 +YAML +} + +# -------------------------------------------------------------------- acceptance criterion 6 +test_marker_skips_lines_before_it() { + local dir + dir="$(new_fixture)" + printf 'config reload refused — something ancient\n' > "$dir/fleetd.out" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=5' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "a stale refusal before the marker must not be read as this edit's verdict" +} + +# ------------------------------------------------------------------- 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)" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=4' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + 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 +} + +# ----------------------------------------- acceptance criterion 14: restore message names the real directory +# fleetd #635 follow-up (ticket comment 17664, defect 6) — the --restore "no backup found" +# message used to print the OLD beside-the-config glob even though newest_backup had already +# moved to searching the managed directory. Proves BOTH directions: the not-found message names +# the directory actually searched (not merely that it says SOMETHING), and that a real backup +# sitting in that directory still lets --restore succeed — otherwise the fix could regress into +# a message that is always printed regardless of whether a backup exists. +test_restore_message_names_the_searched_directory() { + local dir rc=0 + + # Direction 1: no backup anywhere — the message must name .config-backups/, not the bare + # beside-the-config glob the OLD code printed. + dir="$(new_fixture)" + "$EDIT" --restore --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 || rc=$? + RUN_OUTPUT="$(cat "$dir/stdout.log")" + + assert_equals 1 "$rc" "--restore with no backup anywhere exit code" + assert_contains ".config-backups/fleetd.yaml.bak.*" "$RUN_OUTPUT" \ + "the not-found message must name the directory actually searched, not the old beside-the-config glob" + + # Direction 2: a real backup IS present in .config-backups/ — --restore must still succeed, so + # the message fix cannot have turned into one that prints regardless of whether a backup exists. + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + start_run "$dir" 5 --set '.profiles.sonnet.weight=77' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "setup edit exit code for criterion 14's second half" + + start_run "$dir" 5 --restore + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "--restore with a real backup present must still succeed" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "--restore with a real backup present must put the file back byte for byte" +} + +# ------------------------- acceptance criterion 15a: block-scalar continuation lines are redacted +# fleetd #635 follow-up (ticket comment 17670, defect 7) — redact() used to look only AT the key +# line. A YAML block scalar (`|`) puts its value on the lines that FOLLOW the key, each indented +# deeper than it, so the real secret flowed through untouched while the key line right above it +# printed a reassuring "" — worse than no redaction, because the marker stops a reader +# from looking further. The edited key here ("retries") sits directly next to the block scalar, +# well inside diff -u's default 3-line context window, so the printed hunk is GUARANTEED to +# include the secret's lines — placing the edit further away would let this pass today even +# without the fix, proving nothing (the ticket comment's own warning, from the lead's first +# reproduction attempt). The positive control runs FIRST: without it, "the secret never entered +# the diff at all" would pass identically to "it entered and was correctly redacted". +new_fixture_block_scalar() { + local dir + dir="$(mktemp -d "$TMP/fixture.XXXXXX")" + cat > "$dir/fleetd.yaml" <<'YAML' +bind: + host: 127.0.0.1 + port: 19999 +broker: + uri: amqp://user:hunter2@host/vhost +auth: + token: | + FAKELEAK-BLOCK-SCALAR + retries: 1 +profiles: + sonnet: + weight: 3 + maxLoad: 5 +YAML + : > "$dir/fleetd.out" + printf '%s' "$dir" +} + +test_block_scalar_continuation_is_redacted() { + local dir + dir="$(new_fixture_block_scalar)" + + start_run "$dir" 5 --set '.auth.retries=2' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "block-scalar case reload exit code" + # Positive control FIRST: the key's own (masked) line must really be in the printed diff, or the + # negative assertion right after proves nothing — see the comment above this test. + assert_contains "token:" "$RUN_OUTPUT" "block-scalar case: the key's line must be in the printed diff" + assert_contains "" "$RUN_OUTPUT" "block-scalar case: redaction must be proven to have run on real content" + assert_not_contains "FAKELEAK-BLOCK-SCALAR" "$RUN_OUTPUT" "block-scalar case: the block scalar's VALUE must never leak" +} + +# ------------------------------------- acceptance criterion 15b: "passphrase" is also recognised +# "passphrase" was in none of TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY (ticket comment +# 17670). This is a plain key:value line, not a block scalar — kept in its OWN fixture and OWN +# function, separate from criterion 15a, so that a failure in one case can never mask a failure in +# the other (a single combined test would abort under `set -e` at its first failing assertion, +# and the second case would then never even run). +new_fixture_passphrase() { + local dir + dir="$(mktemp -d "$TMP/fixture.XXXXXX")" + cat > "$dir/fleetd.yaml" <<'YAML' +bind: + host: 127.0.0.1 + port: 19999 +broker: + uri: amqp://user:hunter2@host/vhost +auth: + passphrase: FAKELEAK-PASSPHRASE + retries: 1 +profiles: + sonnet: + weight: 3 + maxLoad: 5 +YAML + : > "$dir/fleetd.out" + printf '%s' "$dir" +} + +test_passphrase_key_is_redacted() { + local dir + dir="$(new_fixture_passphrase)" + + start_run "$dir" 5 --set '.auth.retries=2' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "passphrase case reload exit code" + assert_contains "passphrase:" "$RUN_OUTPUT" "passphrase case: the key's line must be in the printed diff" + assert_contains "" "$RUN_OUTPUT" "passphrase case: redaction must be proven to have run on real content" + assert_not_contains "FAKELEAK-PASSPHRASE" "$RUN_OUTPUT" "passphrase case: the passphrase VALUE must never leak" +} + +# ----------------------------------- acceptance criterion 16: a failing --set must not echo value +# fleetd #635 follow-up (ticket comment 17673, defect 8) — apply_set_pairs used to echo the FULL +# "$kv" (path=value, exactly as typed) in its yq-failure messages, so a broken --set with a +# secret-looking value printed that value right back out. The path alone is what the positive +# control proves is still there — it is what the operator needs to fix their command — and the +# negative assertion proves the value itself never appears. Kept to exactly this one failure +# shape (an invalid yq path/expression), matching the ticket's own reproduction. +test_failing_set_does_not_echo_its_value() { + local dir rc=0 + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + + "$EDIT" --dry-run --set '.broker.["bad=FAKELEAK-SETVALUE' \ + --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 "a --set with an invalid yq expression must exit non-zero, got 0" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "a failing --set must install nothing — the live fixture changed" + # Positive control FIRST: the path must still be in the message, or the negative assertion right + # after proves nothing (the message could simply have disappeared entirely). + assert_contains '.broker.["bad' "$RUN_OUTPUT" "the failure message must still name the PATH" + assert_not_contains "FAKELEAK-SETVALUE" "$RUN_OUTPUT" "the failure message must NEVER echo the VALUE" +} + +# dry-run must never touch the live file and must still redact. +test_dry_run_never_installs_and_redacts() { + local dir before + dir="$(new_fixture)" + before="$(cat "$dir/fleetd.yaml")" + + "$EDIT" --dry-run --set '.profiles.sonnet.weight=99' \ + --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 + local rc=$? + RUN_OUTPUT="$(cat "$dir/stdout.log")" + + assert_equals 0 "$rc" "dry-run exit code" + 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". +test_check_is_read_only_and_exits_zero() { + local dir before rc=0 + dir="$(new_fixture)" + before="$(cat "$dir/fleetd.yaml")" + + "$EDIT" --check --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" \ + > "$dir/stdout.log" 2>&1 || rc=$? + + assert_equals 0 "$rc" "--check exit code" + assert_equals "$before" "$(cat "$dir/fleetd.yaml")" "--check must never modify the config" +} + +test_refusal_shape_from_parse_failure_wording_is_recognised() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=6' + sleep 1 + printf 'config reload from %s refused, keeping the running config: boom\n' "$dir/fleetd.yaml" >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 4 "$RUN_RC" "the parse-failure refusal shape must also exit 4, not be read as silence" +} + +echo "== acceptance criterion 1: refusal restores byte for byte ==" +test_refusal_restores_byte_for_byte +echo "== acceptance criterion 2: clean reload keeps the edit ==" +test_clean_reload_keeps_the_edit +echo "== acceptance criterion 3: deferred reload told apart from clean ==" +test_deferred_reload_is_told_apart_from_clean +echo "== acceptance criterion 4: silence is its own answer ==" +test_silence_is_its_own_answer +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 (+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 "== acceptance criterion 14: the restore message names the directory actually searched ==" +test_restore_message_names_the_searched_directory +echo "== acceptance criterion 15a: a block scalar's continuation lines are redacted ==" +test_block_scalar_continuation_is_redacted +echo "== acceptance criterion 15b: a passphrase key is also recognised ==" +test_passphrase_key_is_redacted +echo "== acceptance criterion 16: a failing --set must not echo its value ==" +test_failing_set_does_not_echo_its_value +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 ==" +test_check_is_read_only_and_exits_zero +echo "== extra: the parse-failure refusal shape is also recognised ==" +test_refusal_shape_from_parse_failure_wording_is_recognised + +printf 'PASS: config-edit acceptance criteria\n'