From a5ad7c656120719c1396054b3a7888258e88a2e3 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 12 Sep 2026 11:30:35 +0700 Subject: [PATCH 1/3] fleetd #519: test policy probe guards --- scripts/probe-member-credentials.sh | 102 ++++++++++++--------- scripts/test-probe-member-credentials.sh | 109 +++++++++++++++++++++++ 2 files changed, 168 insertions(+), 43 deletions(-) create mode 100755 scripts/test-probe-member-credentials.sh diff --git a/scripts/probe-member-credentials.sh b/scripts/probe-member-credentials.sh index b92971d..5108cad 100755 --- a/scripts/probe-member-credentials.sh +++ b/scripts/probe-member-credentials.sh @@ -64,6 +64,59 @@ # The two outputs side by side are the finding: any name whose hash matches between them is a # credential the member holds in full. # +parse_policy_fields() { + if command -v jq >/dev/null 2>&1; then + _FIELDS_RAW="$(printf '%s' "$POLICY_JSON" | jq -r ' + (.present | tostring), + (.policy // ""), + (.knownCount // 0 | tostring), + (.allowedCount // 0 | tostring), + (.blockedCount // 0 | tostring), + (.known[]? // empty)')" + _PARSE_STATUS=$? + _PARSER_NAME="jq" + else + _FIELDS_RAW="$(printf '%s' "$POLICY_JSON" | python3 - <<'PY' +import json, sys +data = json.load(sys.stdin) +print(str(data.get("present"))) +print(data.get("policy") or "") +print(data.get("knownCount") if data.get("knownCount") is not None else 0) +print(data.get("allowedCount") if data.get("allowedCount") is not None else 0) +print(data.get("blockedCount") if data.get("blockedCount") is not None else 0) +for n in (data.get("known") or []): + print(n) +PY + )" + _PARSE_STATUS=$? + _PARSER_NAME="python3" + fi + + if [ "$_PARSE_STATUS" -ne 0 ]; then + echo "refusing to run: could not parse the policy fetched from $POLICY_URL — $_PARSER_NAME exited" \ + "non-zero (status $_PARSE_STATUS). That is a parser failure, not a claim about the policy" \ + "itself; the policy response has not been read." >&2 + return 4 + fi + + # A herestring adds a newline, so mapfile would turn an empty parser result into one empty field. + # Keep that case separate so the refusal reports what the parser actually returned: zero fields. + if [ -z "$_FIELDS_RAW" ]; then + _FIELDS=() + else + mapfile -t _FIELDS <<< "$_FIELDS_RAW" + fi + + if (( ${#_FIELDS[@]} < 5 )); then + echo "refusing to run: the policy parser ($_PARSER_NAME) returned ${#_FIELDS[@]} field(s); at" \ + "least 5 are required (present, policy mode, knownCount, allowedCount, blockedCount). The" \ + "parse ran but its shape is wrong — this is not a claim about how many names the policy" \ + "knows." >&2 + return 5 + fi +} + +main() { set -uo pipefail # `pipefail` is not what catches the parser failure handled below (fleetd #500): in # `printf '%s' "$POLICY_JSON" | jq -r '...'`, jq is the LAST element of the pipe, so the pipeline's @@ -159,42 +212,6 @@ fi # materialises the whole string in memory first, where `< <(...)` would stream it. That only # matters for a large producer; this one is a short credential-name policy response, so the # tradeoff is irrelevant here — noted because it would not be for every producer. -if command -v jq >/dev/null 2>&1; then - _FIELDS_RAW="$(printf '%s' "$POLICY_JSON" | jq -r ' - (.present | tostring), - (.policy // ""), - (.knownCount // 0 | tostring), - (.allowedCount // 0 | tostring), - (.blockedCount // 0 | tostring), - (.known[]? // empty)')" - _PARSE_STATUS=$? - _PARSER_NAME="jq" -else - _FIELDS_RAW="$(printf '%s' "$POLICY_JSON" | python3 - <<'PY' -import json, sys -data = json.load(sys.stdin) -print(str(data.get("present"))) -print(data.get("policy") or "") -print(data.get("knownCount") if data.get("knownCount") is not None else 0) -print(data.get("allowedCount") if data.get("allowedCount") is not None else 0) -print(data.get("blockedCount") if data.get("blockedCount") is not None else 0) -for n in (data.get("known") or []): - print(n) -PY - )" - _PARSE_STATUS=$? - _PARSER_NAME="python3" -fi - -if [ "$_PARSE_STATUS" -ne 0 ]; then - echo "refusing to run: could not parse the policy fetched from $POLICY_URL — $_PARSER_NAME exited" \ - "non-zero (status $_PARSE_STATUS). That is a parser failure, not a claim about the policy" \ - "itself; the policy response has not been read." >&2 - exit 4 -fi - -mapfile -t _FIELDS <<< "$_FIELDS_RAW" - # Arity check — the CORRECTNESS fix (fleetd #500). A parser that exits 0 can still return fewer # than the 5 fixed fields (present, policy mode, 3 counts) that every line below this expects, # whatever the reason: a producer that printed nothing, malformed JSON that jq/python3 still @@ -203,13 +220,7 @@ mapfile -t _FIELDS <<< "$_FIELDS_RAW" # without this check a short `_FIELDS` reaches the "0 known names" guard further down with the # same look as a policy that genuinely has 0 names. Check the count here, at the one point the # fact is still present, before the slice consumes it. -if (( ${#_FIELDS[@]} < 5 )); then - echo "refusing to run: the policy parser ($_PARSER_NAME) returned ${#_FIELDS[@]} field(s); at" \ - "least 5 are required (present, policy mode, knownCount, allowedCount, blockedCount). The" \ - "parse ran but its shape is wrong — this is not a claim about how many names the policy" \ - "knows." >&2 - exit 5 -fi +parse_policy_fields || exit $? PRESENT="${_FIELDS[0]:-null}" POLICY_MODE="${_FIELDS[1]:-}" @@ -317,3 +328,8 @@ How to read this: hardcoded list did. If the daemon's policy changes, the next run of this script reflects it with no edit to this file. EOF +} + +if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then + main "$@" +fi diff --git a/scripts/test-probe-member-credentials.sh b/scripts/test-probe-member-credentials.sh new file mode 100755 index 0000000..477c60b --- /dev/null +++ b/scripts/test-probe-member-credentials.sh @@ -0,0 +1,109 @@ +#!/usr/bin/env bash +# Self-contained checks for the policy parsing guards in probe-member-credentials.sh. + +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +PROBE="$ROOT/scripts/probe-member-credentials.sh" +TMP="$(mktemp -d "$ROOT/.probe-member-credentials-test.XXXXXX")" +trap 'rm -rf "$TMP"' EXIT + +# The SOURCED guard exposes this pure parser without contacting POLICY_URL. +source "$PROBE" + +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" +} + +make_jq() { + local body="$1" + mkdir -p "$TMP/bin" + printf '%s\n' '#!/usr/bin/env bash' "$body" > "$TMP/bin/jq" + chmod +x "$TMP/bin/jq" +} + +run_parser() { + local output rc=0 + POLICY_JSON="$(< "$TMP/policy.json")" + POLICY_URL="fixture://member-credentials" + output="$(PATH="$TMP/bin:$PATH" parse_policy_fields 2>&1)" || rc=$? + PARSER_OUTPUT="$output" + PARSER_RC="$rc" +} + +test_bash_older_than_four_refuses() { + local output rc=0 version + version="$(/bin/bash -c 'printf %s "$BASH_VERSION"')" + output="$(/bin/bash "$PROBE" 2>&1)" || rc=$? + assert_equals 3 "$rc" "bash 3 refusal status" + assert_contains 'This shell is bash' "$output" "bash 3 refusal" + assert_contains "$version" "$output" "bash 3 refusal version" +} + +test_parser_non_zero_refuses() { + make_jq 'exit 17' + run_parser + assert_equals 4 "$PARSER_RC" "parser failure status" + assert_contains 'jq exited non-zero (status 17)' "$PARSER_OUTPUT" "parser failure message" +} + +test_short_parser_output_refuses() { + make_jq "printf '%s\\n' true enforce 3 2" + run_parser + assert_equals 5 "$PARSER_RC" "short parser output status" + assert_contains 'jq) returned 4 field(s)' "$PARSER_OUTPUT" "short parser output count" +} + +test_empty_parser_output_reports_zero_fields() { + make_jq ':' + run_parser + assert_equals 5 "$PARSER_RC" "empty parser output status" + assert_contains 'jq) returned 0 field(s)' "$PARSER_OUTPUT" "empty parser output count" +} + +test_well_formed_policy_prints_name_table() { + local output rc=0 + make_jq "cat '$TMP/policy.fields'" + # Shell functions cannot be passed in an environment assignment. Run the executable through bash. + output="$(BRIDGED_MEMBER=1 FIXTURE="$TMP/policy.json" PROBE="$PROBE" PATH="$TMP/bin:$PATH" bash -c ' + curl() { cat "$FIXTURE"; } + export -f curl + exec "$PROBE" + ' 2>&1)" || rc=$? + assert_equals 0 "$rc" "well-formed policy status" + assert_contains 'ALPHA_TOKEN' "$output" "name table" + assert_contains 'BETA_TOKEN' "$output" "name table" + assert_contains 'GAMMA_TOKEN' "$output" "name table" +} + +cat > "$TMP/policy.json" <<'JSON' +{"present":true,"policy":"enforce","knownCount":3,"allowedCount":2,"blockedCount":1,"known":["ALPHA_TOKEN","BETA_TOKEN","GAMMA_TOKEN"]} +JSON +cat > "$TMP/policy.fields" <<'FIELDS' +true +enforce +3 +2 +1 +ALPHA_TOKEN +BETA_TOKEN +GAMMA_TOKEN +FIELDS + +test_bash_older_than_four_refuses +test_parser_non_zero_refuses +test_short_parser_output_refuses +test_empty_parser_output_reports_zero_fields +test_well_formed_policy_prints_name_table +printf 'PASS: probe member credentials guards\n' -- 2.52.0 From 5b1e13ca3d4ff0ea85ace7fec390ab80b26434cf Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 12 Sep 2026 11:47:16 +0700 Subject: [PATCH 2/3] fleetd #519 review fix: move the parser comments with the code they explain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #523 extracted parse_policy_fields() but left about 25 lines of explanatory comments at the old parse site in main(). That is the same defect class as fleetd #500 itself — a stated fact that no longer matches the code next to it — in the very file whose ticket history is about it. Three blocks moved, no code touched: * "One parse pass" + the mapfile/process-substitution reasoning now sits above parse_policy_fields(), which is what it describes. * The arity-check block now sits inside the function, directly above `if (( ${#_FIELDS[@]} < 5 ))`. At the old site it said "the slice just below this" and "every line below this expects", both pointing at a function call rather than the check. Reworded to name main() and its slice explicitly. * The pipefail note said the parser failure was "handled below"; the handling is now above it, in the function. The call site keeps a three-line pointer saying where the reasoning went. Checked myself, on this revision: * suite exit 0, "PASS: probe member credentials guards" * bash -n rc=0 under /bin/bash 3.2.57 and bash 5.3.9 * two mutations killed, each proven applied two ways (mutant present AND original gone), restored byte-identical, green control after each: - dropping the empty-parse special case -> FAIL: empty parser output count - arity threshold 5 -> 0 -> FAIL: short parser output status --- scripts/probe-member-credentials.sh | 55 +++++++++++++++-------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/scripts/probe-member-credentials.sh b/scripts/probe-member-credentials.sh index 5108cad..f65a8a5 100755 --- a/scripts/probe-member-credentials.sh +++ b/scripts/probe-member-credentials.sh @@ -64,6 +64,23 @@ # The two outputs side by side are the finding: any name whose hash matches between them is a # credential the member holds in full. # +# One parse pass: line 1 = present (true/false/null), line 2 = policy mode (possibly blank), +# lines 3-5 = knownCount/allowedCount/blockedCount, remaining lines = the known[] names. A single +# pass avoids re-parsing (and re-risking a truthiness bug) five separate times. +# +# This used to feed the parser straight into `mapfile -t _FIELDS < <(producer)`. That form cannot +# see the producer fail: `<` `<(...)` is a process substitution, not a pipeline, so `set -o +# pipefail` does not reach inside it, and mapfile's own exit status reports whether the BUILTIN +# ran, not whether the command substituted into it succeeded — a failing jq or python3 there still +# leaves mapfile at rc=0 with an empty array, read as a parse that genuinely found nothing (fleetd +# #500). Capturing the parser's output with command substitution first, and checking ITS exit +# status, reports the producer's real failure while the fact still exists — before it is handed to +# mapfile at all. +# +# mapfile then reads from that captured string with `<<<` (a herestring), not `< <(...)`: `<<<` +# materialises the whole string in memory first, where `< <(...)` would stream it. That only +# matters for a large producer; this one is a short credential-name policy response, so the +# tradeoff is irrelevant here — noted because it would not be for every producer. parse_policy_fields() { if command -v jq >/dev/null 2>&1; then _FIELDS_RAW="$(printf '%s' "$POLICY_JSON" | jq -r ' @@ -107,6 +124,14 @@ PY mapfile -t _FIELDS <<< "$_FIELDS_RAW" fi + # Arity check — the CORRECTNESS fix (fleetd #500). A parser that exits 0 can still return fewer + # than the 5 fixed fields (present, policy mode, 3 counts) that every fixed-field read in main() expects, + # whatever the reason: a producer that printed nothing, malformed JSON that jq/python3 still + # accepted, or a schema change upstream. main()'s slice (`_FIELDS[@]:5`) does not fire + # `set -u` on an unset OR a short array, and every fixed-field read there used a `:-` default, so + # without this check a short `_FIELDS` reaches the "0 known names" guard further down with the + # same look as a policy that genuinely has 0 names. Check the count here, at the one point the + # fact is still present, before the slice consumes it. if (( ${#_FIELDS[@]} < 5 )); then echo "refusing to run: the policy parser ($_PARSER_NAME) returned ${#_FIELDS[@]} field(s); at" \ "least 5 are required (present, policy mode, knownCount, allowedCount, blockedCount). The" \ @@ -118,7 +143,7 @@ PY main() { set -uo pipefail -# `pipefail` is not what catches the parser failure handled below (fleetd #500): in +# `pipefail` is not what catches the parser failure handled in parse_policy_fields() above (fleetd #500): in # `printf '%s' "$POLICY_JSON" | jq -r '...'`, jq is the LAST element of the pipe, so the pipeline's # own exit status is already jq's status, with or without pipefail. It is kept as insurance for if # a post-processing stage is ever appended after the parser (e.g. `| tail -n +2`) — at that point @@ -195,31 +220,9 @@ EOF exit 1 fi -# One parse pass: line 1 = present (true/false/null), line 2 = policy mode (possibly blank), -# lines 3-5 = knownCount/allowedCount/blockedCount, remaining lines = the known[] names. A single -# pass avoids re-parsing (and re-risking a truthiness bug) five separate times. -# -# This used to feed the parser straight into `mapfile -t _FIELDS < <(producer)`. That form cannot -# see the producer fail: `<` `<(...)` is a process substitution, not a pipeline, so `set -o -# pipefail` does not reach inside it, and mapfile's own exit status reports whether the BUILTIN -# ran, not whether the command substituted into it succeeded — a failing jq or python3 there still -# leaves mapfile at rc=0 with an empty array, read as a parse that genuinely found nothing (fleetd -# #500). Capturing the parser's output with command substitution first, and checking ITS exit -# status, reports the producer's real failure while the fact still exists — before it is handed to -# mapfile at all. -# -# mapfile then reads from that captured string with `<<<` (a herestring), not `< <(...)`: `<<<` -# materialises the whole string in memory first, where `< <(...)` would stream it. That only -# matters for a large producer; this one is a short credential-name policy response, so the -# tradeoff is irrelevant here — noted because it would not be for every producer. -# Arity check — the CORRECTNESS fix (fleetd #500). A parser that exits 0 can still return fewer -# than the 5 fixed fields (present, policy mode, 3 counts) that every line below this expects, -# whatever the reason: a producer that printed nothing, malformed JSON that jq/python3 still -# accepted, or a schema change upstream. The slice just below this (`_FIELDS[@]:5`) does not fire -# `set -u` on an unset OR a short array, and every fixed-field read above used a `:-` default, so -# without this check a short `_FIELDS` reaches the "0 known names" guard further down with the -# same look as a policy that genuinely has 0 names. Check the count here, at the one point the -# fact is still present, before the slice consumes it. +# Parse the policy in one pass and refuse on any of the three failure causes. The decision, the +# three refusals and the reasoning behind each live in parse_policy_fields() above — kept there +# with the code rather than here, so the explanation cannot drift away from what it explains. parse_policy_fields || exit $? PRESENT="${_FIELDS[0]:-null}" -- 2.52.0 From b5843ab43f2f3453f3f1ae07332adfda5f7e2bb8 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 12 Sep 2026 11:50:11 +0700 Subject: [PATCH 3/3] fleetd #519 review fix: widen two needles to the whole parenthetical MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both arity assertions matched on `jq) returned N field(s)` — a needle that starts in the middle of the script's `(parser name)` parenthetical. On a real failure the harness prints `missing `, so the line came out as: FAIL: empty parser output count: missing jq) returned 0 field(s) which reads as if the script's own message had an unbalanced paren. It does not; the needle was just sliced. Matching on `policy parser (jq) returned N field(s)` makes the failure readable and also pins that the refusal names the parser it used, which the narrower needle did not. make_jq() PATH-prefixes a fake jq, so `_PARSER_NAME` is deterministically "jq" in both tests; the wider needle cannot flake on a host without jq. Re-proved on this revision, because a disproof is about a revision and not a file: * suite exit 0, "PASS: probe member credentials guards" * bash -n rc=0 on the test under /bin/bash 3.2.57 and bash 5.3.9 * dropping the empty-parse special case -> FAIL: empty parser output count: missing policy parser (jq) returned 0 field(s) * arity threshold 5 -> 0 -> FAIL: short parser output status * script restored byte-identical after each, green control after both --- scripts/test-probe-member-credentials.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/test-probe-member-credentials.sh b/scripts/test-probe-member-credentials.sh index 477c60b..613e1c8 100755 --- a/scripts/test-probe-member-credentials.sh +++ b/scripts/test-probe-member-credentials.sh @@ -62,14 +62,14 @@ test_short_parser_output_refuses() { make_jq "printf '%s\\n' true enforce 3 2" run_parser assert_equals 5 "$PARSER_RC" "short parser output status" - assert_contains 'jq) returned 4 field(s)' "$PARSER_OUTPUT" "short parser output count" + assert_contains 'policy parser (jq) returned 4 field(s)' "$PARSER_OUTPUT" "short parser output count" } test_empty_parser_output_reports_zero_fields() { make_jq ':' run_parser assert_equals 5 "$PARSER_RC" "empty parser output status" - assert_contains 'jq) returned 0 field(s)' "$PARSER_OUTPUT" "empty parser output count" + assert_contains 'policy parser (jq) returned 0 field(s)' "$PARSER_OUTPUT" "empty parser output count" } test_well_formed_policy_prints_name_table() { -- 2.52.0