fleetd #635 follow-up: redact() masks block-scalar continuations + passphrase; --set failures stop echoing the value (defects 7 and 8, criteria 15a/15b/16)
Defect 7 (comment 17670): redact() only masked a line that itself started with a secret-looking key, so a YAML block scalar's value leaked on the lines that followed the key while the key line right above it printed a reassuring "<redacted>". Fixed by tracking the masked key's own indentation and masking every following line indented deeper than it, stopping once indentation returns to the key's level or shallower; the diff's leading +/-/space marker is stripped before indentation is measured, per the comment's own pitfall. "passphrase" is now also in the key-name backstop. Defect 8 (comment 17673): apply_set_pairs echoed the operator's full "path=value" input, unredacted, in both of its yq-failure die messages — a failing --set with a secret-looking value printed that value right back. Fixed to print only the path; deliberately not routed through redact, which would pass a non-"key: value"-shaped string straight through. Adds acceptance criteria 15a (block-scalar continuation), 15b (passphrase key), and 16 (failing --set never echoes its value) to scripts/test-config-edit.sh, each with a positive control proving the relevant line really was in the printed output before asserting the secret is absent. All three confirmed RED against the pre-fix code and GREEN after, in isolation, before being folded into the full suite (16 criteria + 3 extras, exit 0). Also updates PR #636's description per comment 17671: the redact() sentence now names the continuation-masking rule and says plainly that the key-name list is a backstop, never a complete list.
This commit is contained in:
+58
-13
@@ -143,21 +143,58 @@ done
|
||||
# Two independent passes, applied to every diff this script ever prints:
|
||||
# 1. `scheme://user:pass@host` -> `scheme://<redacted>@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 "<redacted>" — 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#://[^@]*@#://<redacted>@#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 <redacted>\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<redacted>\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 <redacted>\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}=<value>' — nothing was installed. The live config is unchanged."
|
||||
done
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user