fleet/fleetd
3
0
Fork 0
Code Issues 80 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleetd #638: mask userinfo in the daemon verdict line #685

Closed
agent wants to merge 0 commits from worker/638-a7b391-1 into main
pull from: worker/638-a7b391-1
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-10-03 22:07:18 +02:00
Member
Copy Link
Copy Source

Fixes fleetd #638.

Applies the userinfo-only rewrite sed -E 's#://[^@]*@#://<redacted>@#g' to every path in scripts/config-edit.sh that prints the daemon's verdict line ($VERDICT_LINE / the last-verdict-in-log read-back), via one new helper mask_verdict_userinfo. Not routed through the existing redact() -- that function's key:value masking does not match this line's prose, and masking more than the userinfo would remove the diagnostic detail (e.g. the pattern quoted in a parse-failure refusal) that an operator needs to fix the refusal.

Print sites found and fixed (6 total, by tracing both verdict-acquisition entry points -- wait_for_verdict and last_verdict_line -- to every caller, not just grepping VERDICT_LINE):

  • restore_and_confirm: the refused-restore warn and the restore-confirmed ok (2 sites, both read $VERDICT_LINE directly)
  • report_outcome: the clean / needs-restart / refused branches, all reading one local copy of $VERDICT_LINE (3 sites, 1 rewrite call)
  • check_mode: "last verdict in log", read via last_verdict_line rather than the $VERDICT_LINE global (1 site) -- this one is not named in the ticket's comment and would be missed by grep -n VERDICT_LINE

No current refusal message echoes a URI, token or password (confirmed by the ticket's own measurement against a live daemon), so this is a guard against a future validator doing so, not a fix for an observed leak.

Tests added to scripts/test-config-edit.sh:

  • test_verdict_userinfo_is_masked_with_positive_control: a verdict line carrying amqp://user:hunter2@host/vhost has the userinfo masked, with a positive control proving the surrounding diagnostic prose still reaches the output.
  • test_ordinary_refusal_line_passes_through_unchanged: the real captured refusal shape (quoting ("[unclosed")) passes through byte for byte, unchanged.

Test count: 22 before, 24 after. Full suite (bash scripts/test-config-edit.sh) passes, exit 0, both before and after.

Mutation check: broke the rewrite's pattern so it cannot match (prepended a literal that never occurs) and reran the suite -- test_verdict_userinfo_is_masked_with_positive_control went RED ("must NOT contain [user:hunter2], but it does"), confirming the test actually exercises the rewrite. Reverted; git diff on the mutation site came back clean.

Scope: scripts/config-edit.sh and scripts/test-config-edit.sh only, no Java touched.

Fixes fleetd #638. Applies the userinfo-only rewrite `sed -E 's#://[^@]*@#://<redacted>@#g'` to every path in `scripts/config-edit.sh` that prints the daemon's verdict line ($VERDICT_LINE / the last-verdict-in-log read-back), via one new helper `mask_verdict_userinfo`. Not routed through the existing `redact()` -- that function's key:value masking does not match this line's prose, and masking more than the userinfo would remove the diagnostic detail (e.g. the pattern quoted in a parse-failure refusal) that an operator needs to fix the refusal. **Print sites found and fixed (6 total, by tracing both verdict-acquisition entry points -- `wait_for_verdict` and `last_verdict_line` -- to every caller, not just grepping `VERDICT_LINE`):** - `restore_and_confirm`: the refused-restore warn and the restore-confirmed ok (2 sites, both read $VERDICT_LINE directly) - `report_outcome`: the clean / needs-restart / refused branches, all reading one local copy of $VERDICT_LINE (3 sites, 1 rewrite call) - `check_mode`: "last verdict in log", read via `last_verdict_line` rather than the $VERDICT_LINE global (1 site) -- this one is not named in the ticket's comment and would be missed by `grep -n VERDICT_LINE` No current refusal message echoes a URI, token or password (confirmed by the ticket's own measurement against a live daemon), so this is a guard against a future validator doing so, not a fix for an observed leak. **Tests added to `scripts/test-config-edit.sh`:** - `test_verdict_userinfo_is_masked_with_positive_control`: a verdict line carrying `amqp://user:hunter2@host/vhost` has the userinfo masked, with a positive control proving the surrounding diagnostic prose still reaches the output. - `test_ordinary_refusal_line_passes_through_unchanged`: the real captured refusal shape (quoting `("[unclosed")`) passes through byte for byte, unchanged. Test count: 22 before, 24 after. Full suite (`bash scripts/test-config-edit.sh`) passes, exit 0, both before and after. Mutation check: broke the rewrite's pattern so it cannot match (prepended a literal that never occurs) and reran the suite -- `test_verdict_userinfo_is_masked_with_positive_control` went RED ("must NOT contain [user:hunter2], but it does"), confirming the test actually exercises the rewrite. Reverted; `git diff` on the mutation site came back clean. Scope: `scripts/config-edit.sh` and `scripts/test-config-edit.sh` only, no Java touched.
agent added 1 commit 2026-10-03 22:07:18 +02:00
fleetd #638: mask userinfo in the daemon verdict line before it prints
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 1m7s
Details
CI / build (pull_request) Failing after 2m4s
Details
28b45d97e5 
Apply the userinfo-only rewrite (sed -E 's#://[^@]*@#://<redacted>@#g') to every
path that prints config-edit.sh's $VERDICT_LINE: restore_and_confirm's two
prints, report_outcome's shared local copy (covering its clean/needs-restart/
refused branches), and check_mode's "last verdict in log" line via
last_verdict_line. Deliberately not routed through redact() — that function's
key:value masking does not match this line's prose, and the rest of the line
(e.g. the pattern quoted in a parse-failure refusal) is the detail an operator
needs to fix the refusal.

No current refusal message echoes a URI, token or password, so this is a guard
against a future validator doing so, not a fix for an observed leak.
ltms commented 2026-10-03 22:30:44 +02:00
Owner
Copy Link
Copy Source

Do not merge. One confirmed defect, measured through the real script path. Details and the fix
are on ticket #638, comment 18259.

Short version: the pattern s#://[^@]*@#...#g has a character class that crosses spaces and words,
so a verdict line holding a URI with no userinfo plus any later @ gets the text between them
destroyed. That is the opposite of the leak this unit guards against, and it removes the diagnostic
detail the helper's own comment promises to keep.

The print-path enumeration in this PR is correct and I am keeping it, including the check_mode
site my own earlier comment missed. A replacement PR builds on this commit rather than redoing it.
I will close this one as superseded when that lands.

**Do not merge.** One confirmed defect, measured through the real script path. Details and the fix are on ticket #638, comment 18259. Short version: the pattern `s#://[^@]*@#...#g` has a character class that crosses spaces and words, so a verdict line holding a URI with **no** userinfo plus any later `@` gets the text between them destroyed. That is the opposite of the leak this unit guards against, and it removes the diagnostic detail the helper's own comment promises to keep. The print-path enumeration in this PR is correct and I am keeping it, including the `check_mode` site my own earlier comment missed. A replacement PR builds on this commit rather than redoing it. I will close this one as superseded when that lands.
ltms commented 2026-10-03 22:41:04 +02:00
Owner
Copy Link
Copy Source

Superseded by #690, which is merged. Closing this unmerged.

PR #690 carries this PR's commit (28b45d9) plus the over-masking fix on top, so nothing here is lost. Merged to main as 7dec74f, pushed (2eb2d61..7dec74f).

I closed this by hand because we merge locally, so Gitea never closes a PR by itself.

What I verified myself, not on the worker's word

The pattern, in both directions. The previous review of this PR reported no defect, and that was true but only covered the leak direction (userinfo → masked). The defect was in the other direction. I ran six line shapes through the old pattern and the new one:

input old [^@]* new [^@/[:space:]]*
broker.uri amqp://user:pass@broker.local/vhost refused amqp://<redacted>@broker.local/vhost refused same — masked
amqps://admin:s3cr3t@host:5671/ amqps://<redacted>@host:5671/ same — masked
broker.uri amqp://broker.local/vhost unreachable, contact ops@example.com broker.uri amqp://<redacted>@example.com unchanged, byte for byte
see https://docs.local/guide and mail ops@example.com see https://<redacted>@example.com unchanged, byte for byte
amqp://broker.local/a@b amqp://<redacted>@b unchanged, byte for byte
amqp://u:p@h1/ and amqp://h2/x then ops@example.com amqp://<redacted>@h1/ and amqp://<redacted>@example.com amqp://<redacted>@h1/ and amqp://h2/x then ops@example.com

The old pattern did not merely over-redact a token. It deleted whole clauses of the operator's verdict line. The last row is the strongest case: the new pattern masks the URI that has userinfo and leaves the one that does not, in the same line.

The new test is not vacuous. I mutated the merged pattern back to [^@]* in a throwaway worktree and re-ran scripts/test-config-edit.sh: RC=1, exactly one FAIL, and it was test_uri_without_userinfo_survives_a_later_at_sign. Unmutated: RC=0. So that test really pins the narrowed class rather than passing by luck.

Test count went 24 → 25, which I counted on merged main with grep -cE '^test_[a-z_]+\(\) \{' scripts/test-config-edit.sh = 25.

No Java build was run for this merge, on purpose. Nothing Java changed, and grep -rl "config-edit" fleetd/src/test/java fleetd/src/main/java returns nothing, so no Java test reads these scripts and the 1942 total cannot move. That is a measurement, not an assumption.

One thing this PR correctly did not fix

redact() at scripts/config-edit.sh:273 still carries the identical unbounded class, s#://[^@]*@#://<redacted>@#g, on the diff-output path. The worker reported it and left it alone, which was the right call for its scope. I confirmed it on the PR ref myself. Filed as its own ticket.

## Superseded by #690, which is merged. Closing this unmerged. PR #690 carries this PR's commit (`28b45d9`) plus the over-masking fix on top, so nothing here is lost. Merged to `main` as `7dec74f`, pushed (`2eb2d61..7dec74f`). I closed this by hand because we merge locally, so Gitea never closes a PR by itself. ## What I verified myself, not on the worker's word **The pattern, in both directions.** The previous review of this PR reported no defect, and that was true but only covered the leak direction (userinfo → masked). The defect was in the other direction. I ran six line shapes through the old pattern and the new one: | input | old `[^@]*` | new `[^@/[:space:]]*` | |---|---|---| | `broker.uri amqp://user:pass@broker.local/vhost refused` | `amqp://<redacted>@broker.local/vhost refused` | same — masked | | `amqps://admin:s3cr3t@host:5671/` | `amqps://<redacted>@host:5671/` | same — masked | | `broker.uri amqp://broker.local/vhost unreachable, contact ops@example.com` | **`broker.uri amqp://<redacted>@example.com`** | unchanged, byte for byte | | `see https://docs.local/guide and mail ops@example.com` | **`see https://<redacted>@example.com`** | unchanged, byte for byte | | `amqp://broker.local/a@b` | **`amqp://<redacted>@b`** | unchanged, byte for byte | | `amqp://u:p@h1/ and amqp://h2/x then ops@example.com` | **`amqp://<redacted>@h1/ and amqp://<redacted>@example.com`** | `amqp://<redacted>@h1/ and amqp://h2/x then ops@example.com` | The old pattern did not merely over-redact a token. It deleted whole clauses of the operator's verdict line. The last row is the strongest case: the new pattern masks the URI that has userinfo and leaves the one that does not, in the same line. **The new test is not vacuous.** I mutated the merged pattern back to `[^@]*` in a throwaway worktree and re-ran `scripts/test-config-edit.sh`: `RC=1`, exactly one `FAIL`, and it was `test_uri_without_userinfo_survives_a_later_at_sign`. Unmutated: `RC=0`. So that test really pins the narrowed class rather than passing by luck. **Test count** went 24 → 25, which I counted on merged `main` with `grep -cE '^test_[a-z_]+\(\) \{' scripts/test-config-edit.sh` = 25. **No Java build was run for this merge, on purpose.** Nothing Java changed, and `grep -rl "config-edit" fleetd/src/test/java fleetd/src/main/java` returns nothing, so no Java test reads these scripts and the 1942 total cannot move. That is a measurement, not an assumption. ## One thing this PR correctly did not fix `redact()` at `scripts/config-edit.sh:273` still carries the identical unbounded class, `s#://[^@]*@#://<redacted>@#g`, on the diff-output path. The worker reported it and left it alone, which was the right call for its scope. I confirmed it on the PR ref myself. Filed as its own ticket.
ltms closed this pull request 2026-10-03 22:41:12 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 1m7s
Details
CI / build (pull_request) Failing after 2m4s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#685
Delete Branch "worker/638-a7b391-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?