diff --git a/scripts/config-edit.sh b/scripts/config-edit.sh index 52e4c0f..7d47d63 100755 --- a/scripts/config-edit.sh +++ b/scripts/config-edit.sh @@ -143,21 +143,58 @@ done # 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|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). +# 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 marker saved_nocasematch=0 + 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 - if [[ "$line" =~ ^([-+\ ]?[[:space:]]*[A-Za-z0-9_.-]+:) ]]; then - marker="${BASH_REMATCH[1]}" - if [[ "$marker" =~ (TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY) ]]; then - printf '%s \n' "$marker" + 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 @@ -386,13 +423,21 @@ apply_set_pairs() { 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 '$kv' — nothing was installed. The live config is unchanged." + || 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 '$kv' — nothing was installed. The live config is unchanged." + || die "yq could not apply --set '.${path}=' — nothing was installed. The live config is unchanged." done } diff --git a/scripts/test-config-edit.sh b/scripts/test-config-edit.sh index 138b0fa..3518771 100755 --- a/scripts/test-config-edit.sh +++ b/scripts/test-config-edit.sh @@ -382,6 +382,124 @@ test_restore_message_names_the_searched_directory() { || 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 @@ -451,6 +569,12 @@ 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 =="