fleetd #635: config-edit.sh — the one auditable way to edit fleetd.yaml #636
Reference in New Issue
Block a user
Delete Branch "worker/config-edit-seam-ca8dc1-1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes fleetd #635.
Adds
scripts/config-edit.sh: backs up the livefleetd.yaml, builds a candidate off it (--setviayq, or--froma given file), parse-checks the candidate withyqbefore it ever reaches the live path, installs atomically (mvwithin the same directory), then reads the daemon's ownConfigRefreload verdict back out offleetd.out— marked from before the edit (the log's line count), so a stale verdict line can never be mistaken for this edit's result.Four exit codes, kept to four on purpose:
--restorecommand is printedEvery diff this script prints (the install's own change report, and
--dry-run) is piped throughredact, which blanks anyscheme://user:pass@hostuserinfo, masks the whole value on any line whose key looks like TOKEN/SECRET/PASSWORD/PASSWD/PASSPHRASE/CREDENTIAL/URI/_KEY, and — since a masked key's value is not always on the key's own line — also masks every line that follows a masked key and is indented deeper than it, until the indentation returns to the key's own level (a YAML block scalar's value sits on exactly those lines). The key-name list is a backstop and can never be complete; the indentation-based continuation masking is what holds regardless of the key's name.Fixed during review (defects 7 and 8, both security — ticket comments 17670/17671/17673):
redactused to mask only the key line itself, so a YAML block scalar's value leaked on the lines that followed it while the key line right above printed a reassuring<redacted>— an incomplete redaction that looks complete is worse than none, because the marker stops a reader from looking further. Fixed as described above;passphrasewas also missing from the key-name list entirely. Separately,apply_set_pairs's two yq-failure messages used to echo the operator's fullpath=valueinput verbatim, so a failing--setwith a secret-looking value echoed that value right back — now they print only the path, never the value (and deliberately do not route it throughredact, which would pass a non-key: value-shaped string straight through and give a false sense of coverage).--checkis read-only (reports config parse state, daemon-listening probe, watch interval, last verdict in the log, newest backup, yq version) and always exits 0.Backups now live in a dedicated
<dir>/.config-backups/directory (gitignored at the repo root and infleetd/.gitignore, plus afleetd.yaml.bak.*glob backstop) rather than besidefleetd.yamlitself — a backup of a file that must never be committed inherits that requirement. The live file's permission mode survives both an edit and a restore (amktempcandidate used to carrymktemp's0600onto the live path forever). An empty--set .a.b=value is refused outright rather than silently nulling the field; a deliberate clear is spelled--set .a.b=null, which writes a real YAMLnull.Also adds
scripts/test-config-edit.sh, self-contained, driving the real script against fixtures in a throwaway temp dir with no daemon involved — it plays the daemon by running config-edit.sh in the background and appending the verdict line it wants to a fixture log.Settled during review:
--setalways writes the value as a YAML string scalar (via yq'sstrenv()), so e.g.--set .profiles.sonnet.weight=7ends up asweight: "7"rather than an unquoted7. This was a deliberate simplification to avoid any shell-injection risk from embedding the raw value in a yq expression. Checked against the real daemon loader (ObjectMapper(new YAMLFactory())from the built jar): a quoted scalar coerces fine onto Integer/Boolean fields ("300000" -> Integer,"false" -> Boolean), and a genuinely bad value is refused ("abc" -> InvalidFormatException) — which this script already classifies asrefused, restores the backup, and exits 4. The chain holds end to end; no change needed.Noted but out of scope (per the ticket, not touched):
scripts/redeploy-fleetd.shis the other script that writes a live file (the jar) without the exact same "read back what the consumer did with it" seam this ticket's shape is about — it reads back healthz/process-liveness, which is adjacent but not identical. Not fixing, just naming it as asked. Also out of scope:$VERDICT_LINE(the daemon's own refusal/reload text) is printed raw in several places — the lead is filing this as its own ticket rather than asking for a fix here, since verifying it needs a live daemon and masking the daemon's own refusal text could hide the one thing that explains the refusal.Test plan
scripts/test-config-edit.shrun locally: criteria 1–7 and 9–12, 14, 15a, 15b and 16 (13 folded into criterion 7) plus the 3 extra checks (dry-run never installs,--checkis read-only, the parse-failure refusal wording is also recognised) pass, exit 0.bash -nclean on both scripts.shellcheckwas not available in this worktree — not run; said so rather than claiming a pass.mvnbuild run — shell-only ticket, no Java touched.The --restore "no backup found" message still printed the old beside-the-config glob (${CONFIG}.bak.*) even though newest_backup had already moved to searching the managed .config-backups/ directory. The message was left behind when the search moved — the search itself was already correct (ticket comment 17664). Fix is reporting-only: the message now names the directory actually searched (via backup_dir_for), and separately says that a backup written the old way, directly beside the config, is not searched any more, with the one-line cp to recover one by hand. No search fallback was added — reading backups from outside the managed directory stays unsupported, as instructed. Acceptance criterion 14 proves both directions: the not-found message names the real directory (confirmed red on the pre-fix code, green after), and a restore with a real backup present in .config-backups/ still succeeds (confirmed this catches an "always not-found" regression that direction 1 alone would miss). All 14 criteria plus 3 extras pass in scripts/test-config-edit.sh.Correction to this PR's description, from the lead who merged it.
The description above says:
The second half of that sentence is too strong. Read it as: the continuation masking holds
while the masked key's own line is inside the diff hunk being printed, and not otherwise.
redact()is feddiff -uoutput, which prints three lines of context. A block scalar's bodyoften reaches the function with its key line left out, and then
maskedis never set and the bodyprints in full — with no
<redacted>anywhere to hint that redaction was attempted. A blank lineinside a block scalar drops the anchor the same way, and that one prints a
<redacted>markerdirectly above the leaked text.
Both measured through the real script with
--dry-run, each with a positive control run first toprove the secret's lines actually reached the output.
redact()'s own source comment is corrected onmainby #640(merged).
so
d7f94cais a strict improvement. It is latent rather than live: today'sfleetd.yamlholds5 block scalars and all 5 sit under non-secret keys, and secrets here are referenced by env-var
name rather than written inline.
The rest of the description stands, and the work in this PR is accepted — I re-ran the suite in a
pristine copy of
d7f94ca(16 criteria + 3 extras, exit 0) and read the diff before merging.