Redact passwords and other secrets from stored PostgreSQL settings (#4348)

[erikdarlingdata]

Closes #4348.

Why

The Darling service's PostgreSQL config collector (PgServerConfigCollector) reads pg_settings plus a
UNION ALL arm over pg_db_role_setting for per-database/per-role overrides, and stores every value it sees.
The README's setup grants the monitoring role pg_monitor, which includes pg_read_all_settings, so that role
reads GUC_SUPERUSER_ONLY settings too — including primary_conninfo, which on a standby carries the
replication password in plain text. Nothing between that read and storage redacted it, so a stored row, a
backup of the store, or an export of it, all held a live credential.

Confirmed live, not just reasoned about: PgSettingRedactionLivePostgresTests creates a role granted only
pg_monitor (no superuser) and reads pg_settings.setting directly with it — the raw value contains the
planted password (Assert.Contains("hunter2", rawValue) passes on unpatched PostgreSQL permissions, which is
expected and correct: this is PostgreSQL's own behavior, not a bug in it). The fix has to sit in front of
storage, which is what this lane builds.

What changes

• New PerformanceMonitor.Collectors/PgSettingRedactor.cs. public static string? Redact(string? name, string? value): null in, null out; never throws; idempotent (redacting an already-redacted value is a
  no-op). RulesVersion = 1 for lane S1b's scrub to record. Four rules, matching the ruling on the issue:
  • a libpq password/sslpassword keyword's value, quoted or not, honoring libpq's \\/\' escapes;
  • the password half of a URI's user info (scheme://user:secret@host) and a password query parameter;
  • an assignment/option whose name contains PASSWORD, PASSWD, SECRET or TOKEN (PGPASSWORD=...,
    AWS_SECRET_ACCESS_KEY=..., --password=...), wherever it appears inside a value's text;
  • a whole-value mask for ssl_passphrase_command (empty stays empty) and for a dotted extension setting
    whose LAST name segment contains password/passwd/passphrase/secret/salt/token/key as a substring — pinned
    as a substring test, not whole-word, with myext.turkey_interval (masked) next to myext.keep_alive
    (left alone, since no marker is a substring of keep_alive) proving the distinction on purpose.
    Host, port, user, dbname and application_name are never touched; a core setting whose name merely contains
    one of those words (password_encryption) keeps its value, since only the dotted, extension-scoped form
    triggers a name-based mask.
• Wired into PgServerConfigCollector.ReadAsync. Every row's setting, boot_val and reset_val pass
  through PgSettingRedactor.Redact(name, ...) before a Row is built — one read loop covers both UNION ALL
  arms, so both the server-wide and the per-database/per-role override population are covered by the same
  change. The query text and every other column are untouched.
• Darling/Darling.Tests/PgSettingRedactorTests.cs (new, 31 tests): 26 [Theory] cases (libpq
  keyword/value at every position with quoting and escapes, sslpassword, URI userinfo/query password with
  percent-encoding and multiple hosts, the three assignment-name examples from the ruling, ssl_passphrase_command
  set and empty, the extension-name substring cases above, and the must-not-change set: password_encryption,
  passfile=/x/.pgpass, a plain host=a user=b, empty string, null) plus a rules-version pin and null/empty-name
  checks. Every case also asserts Redact(name, Redact(name, value)) == Redact(name, value).
• Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs (new, live, [Collection("live-postgres")]):
  plants primary_conninfo with a fake password via ALTER SYSTEM SET + pg_reload_conf(), creates a
  pg_monitor-only role, reads it back through the REAL PgServerConfigCollector (not a hand-rolled column
  read, so a revert of the collector's redaction call sites fails this test — proved by hand: reverted the two
  Redact(...) call sites, rebuilt, reran, watched it fail, restored, reran, watched it pass), and asserts the
  stored primary_conninfo row reads ...password=********... while keeping host, port, user and
  application_name, and that no column of any row contains the raw secret. Cleanup resets the setting, reloads
  and drops the role through LiveStoreCleanup.RunOwnedAsync (the reset has to run on the same session — it's
  cluster state, not a store row a fresh connection would reach just as well).

CHANGELOG entry

SECTION: Fixed
ENTRY:

• Passwords and other secrets in stored PostgreSQL settings are now redacted ([Redact passwords and other secrets from stored PostgreSQL settings (#4348) #4351]) - The config collector stored pg_settings and per-database/per-role override values verbatim, including secrets a pg_monitor-only monitoring role can read but should not retain in plain text, such as a standby's primary_conninfo replication password or a backup tool's key/token in archive_command/restore_command; new collections now mask the secret, and existing stored rows are scrubbed in place, in batches, and the scrub completes at any settings cadence; a target that fails is retried on the next start. Non-secret password-policy settings (rds.accepted_password_auth_method, rds.restrict_password_commands, passwordcheck.min_password_length) are no longer masked. "Keep the rest of the value" is not true for every case: ssl_passphrase_command and a dotted extension setting naming a secret are masked whole. Rotate any password or key that was ever stored this way; the store's WAL archive, replicas and dead row versions can still hold the old plaintext, not only backups and exports, until they age out. Upgrade every service that writes to a store - a service still on an old build keeps storing plaintext after the scrub's marker is set, and nothing re-runs it.
  REF:
  [Redact passwords and other secrets from stored PostgreSQL settings (#4348) #4351]: Redact passwords and other secrets from stored PostgreSQL settings (#4348) #4351

Test plan

• Darling.Tests.PgSettingRedactorTests (new): 31/31 passed.
• Darling.Tests.PgSettingRedactionLivePostgresTests (new, live rig): 1/1 passed; proved by hand that it
  fails with the collector's redaction call sites reverted.
• *PgServerConfig* (PgServerConfigTests, PgServerConfigScopeRungTests,
  PgServerConfigOverrideLivePostgresTests, PgServerConfigToolBoundTests): all passed.
• McpPayloadContractCensusTests, LivePostgresCollectionHygieneTests,
  LiveCleanupConversionRatchetTests (this lane's live test tripped
  NoLiveTestCleansUpOnItsOwnBodysConnection on the first pass until its finally moved to
  LiveStoreCleanup.RunOwnedAsync; green after), DocCommentHygieneTests: all passed.
• dotnet build Darling/Darling.Tests/Darling.Tests.csproj after merging origin/dev: 0 Warning(s), 0
  Error(s).
• Full Darling.Tests suite, once, on a freshly created darlingtest database, after the origin/dev
  merge: Total: 14306, Errors: 0, Failed: 1, Skipped: 55, Not Run: 1.
  - The 1 failure, ServerListAndSummaryPlanShapeTests.TheShippedReads_TouchFarFewerChunks_..., touches
  chunk-exclusion plan shape on collection_log, nothing this PR changed. Re-run alone on the dirty
  post-suite database it still failed; re-run alone on a freshly recreated database it passed
  (Total: 1, Failed: 0) — the cumulative chunk count from the rest of the suite, not this change, is
  why it failed in the full run. Not filed, per the skill's own re-run protocol.
  - The 1 "Not Run" has no exception or entry anywhere in the log I could tie to a test name; it isn't in
  a file this PR touches. Flagging it for the coordinator rather than chasing it further inside this
  lane's scope.
  - 55 skipped are the usual env-gated live/upgrade tests (DARLING_TEST_PGRUNTIME*, etc.) not set here.
• Scrub of stored rows (lane S1b).

For the coordinator

• S1b's scrub should merge into THIS branch (fix/4348-redact-pg-settings) and call
  PgSettingRedactor.Redact/RulesVersion directly — nothing else should reimplement the rules.
• The CHANGELOG entry above assumes S1b's half lands in the same PR/branch before this merges to dev; if the
  scrub ships separately, the entry's "existing stored rows are scrubbed in place" clause needs to move with it.
• myext.turkey_interval / myext.keep_alive in the unit corpus pin a real decision (substring, not
  whole-word, match on the extension name's last segment) — worth a second look given it can also mask a
  value whose name coincidentally contains those letters (over-masks, never leaks).

https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3

Scrub (S1b)

Full measurement table and both live findings: #4348 (comment)

The job

PerformanceMonitor.Darling.Service/PgSettingScrub.cs. A background job launched from DarlingWorker
right after migrations confirm collect.pg_server_config exists (not gated on TimescaleDB — it works the
same on plain PostgreSQL), drained the same way as RunMaterializationHoleRepairAsync /
RunBaselineBackfillAsync: fire-and-forget at startup, awaited at shutdown, never holds up collection.

• Marker: reuses collect.collector_state (V44) — no new migration. Keyed under the fleet sentinel
  server_id = 0 (DarlingObservability.FleetServerId) and collector_name = 'pg_setting_scrub', the same
  shape PgSelfAlertDeliveryStampStore and QueryStoreBackfill already use for store-wide one-shot state.
  The stored value is PgSettingRedactor.RulesVersion as text; the job no-ops when the marker already
  matches, and a future rules bump makes it run again automatically.
• Coarse filter: an ILIKE-only superset of every row PgSettingRedactor.Redact could change (password/
  passwd/secret/token substrings in any value column, ssl_passphrase_command by name, a dotted extension
  name carrying one of the whole-value markers) — cheap, no regex, safe to run over a compressed hypertable.
• Batching: by calendar day (the table's chunk width), sub-batched at 500 keys within a day. Each
  UPDATE carries the day as a literal >= / < range predicate alongside the join, which the measurement
  comment shows is not optional — without it, TimescaleDB can't exclude chunks and decompresses the whole
  table. Redaction runs in C# via PgSettingRedactor.Redact, one call per column
  (setting/boot_val/reset_val), and a row is only written if at least one column actually changed.
• No manual recompress: measured live on TimescaleDB 2.30.1 — the touched chunks' is_compressed flag
  never left true, so this build's compressed-chunk DML path handles it inline. Nothing here forces a
  compress_chunk call.
• Failure isolation: DarlingWorker.RunPgSettingScrubAsync catches everything but
  OperationCanceledException, logs one WARNING, and leaves the marker unwritten so the next start retries
  from the top. It never stops the service.

Step 6 — what else stores a setting's value

Checked every pg_server_config reader (PgTargetFactCollector.Config.cs and friends,
DarlingPgServerConfigReader, the Viewer tabs). The analysis fact collectors turn a setting's value into a
fact through PgSettingValue.ToBytes/ToMs/ToNumber/ToBool — all four take a NUMERIC or boolean-shaped GUC
(memory sizes, timeouts) and none of them ever copies setting/boot_val/reset_val verbatim into fact or
finding text. primary_conninfo and other string-typed secrets never flow through those converters. No
alert history, Custom View or other table copies this table's value columns. Scrubbing
collect.pg_server_config in place is the whole fix.

A census pin update, in-lane

PgServerConfigScopeRungTests.EveryProductReadOfTheConfigTable_EitherExcludesTheOverrides_OrIsTheOneThatSelectsThem
polices every pg_server_config read as either excluding override rows or being the one designated read that
selects them. The scrub's candidate read is a genuine third case — it must catch a secret in either arm, so
it filters on neither database_name/role_name shape — so I named it explicitly in that test (12 → 13
reads) rather than letting a real security read slip through undecided.

Tests

Live, own-store (ScratchPostgres, #1776 own-store): PgSettingScrubLiveTests.cs — seeds an old
unredacted pg_settings row AND a database_name-override row (both carrying the standby password) plus an
already-redacted newer row and a no-secret control row, compresses, proves the secret is really there
pre-scrub, runs the job, and asserts: no column of any row contains it; the control row is byte-identical;
DarlingPgServerConfigReader.GetConfigChangesAsync reports no Changed entry between the scrubbed old row
and the already-redacted new row; a second run is a no-op (AlreadyDone); rolling the marker back to an
older version makes it run again.

Ran: *PgServerConfig*, *PgSettingScrub*, *RollupBackfill*, StorageCommandTimeoutTests,
StartupCommandTimeoutTests, DocCommentHygiene*, LivePostgresCollectionHygieneTests,
LiveCleanupConversionRatchetTests — 179 + 55 total across the two runs, 0 failed after the census fix
above (one failure before it, in-lane fixed). Build: Darling/Darling.Tests/Darling.Tests.csproj, 0
warnings. I did not run the full suite — S1a's lane owns that per the brief.

For the coordinator

• The two branches are merged together on fix/4348-redact-pg-settings (this PR); S1a's PR now carries both
  halves.
• I did not re-run the full suite myself (S1a's job per the brief); please confirm S1a's full run is clean
  after this merge, since the census pin edit is the one change here that touches a file S1a didn't write.
• The measurement's synthetic worst case (one secret setting present on every hourly row for a year) is more
  adversarial than a typical store; real candidate counts and day-touch counts will usually be far smaller.

pm-pr lane report (lane 1, scrub)

Head pushed: 74c475b4e4aa7d2f1ad6d4c9e175b284a1c76fc5 (on top of 8faac3966b6173b9821c9009152961d32237c72f, confirmed still the PR's head before pushing).

H1 (batching). PgSettingScrub.RunAsync now groups candidates by (server_id, day) instead of day alone, and BatchUpdateSql gets an added literal AND t.server_id = $11 alongside the existing day-range predicate, so TimescaleDB can exclude every other server's segment in that day's chunk. RunBatchAsync takes the server id and binds it as a constant NpgsqlDbType.Integer parameter.

RED/GREEN proof, via a throwaway net10.0 console harness (the test project is net10.0-windows and its testhost also needs Microsoft.WindowsDesktop.App, unavailable on this macOS host — same constraint the brief anticipated) referencing PerformanceMonitor.Darling.Service/.Storage directly against docker timescale/timescaledb:2.30.1-pg18 on port 55451:

• Seeded 12 targets x 24 hourly collections x 380 settings/collection = 109,440 rows, one day, primary_conninfo containing a per-server secret; compressed every chunk.
• RED (old code, day-only grouping, no server predicate, reverted via git checkout 8faac396 -- PgSettingScrub.cs in a scratch build, then restored): RunAsync threw PostgresException: 53400: tuple decompression limit exceeded by operation — the same failure mode R1 measured.
• GREEN (fixed code): RunAsync SUCCEEDED: AlreadyDone=False CandidatesRead=288 RowsUpdated=288 DaysTouched=1 — no exception, all 12 targets' primary_conninfo rows scrubbed in one run.

M1/L4/L5.

• M1: corrected the "recompresses inline" remark to say a touched chunk stays partly compressed (chunk.status = compressed+partial) until the compression policy's next run, with the space/retention consequence named.
• L4: RunAsync now tracks changedRowCount vs rowsUpdated; when they differ it skips the marker write and logs a warning via the (previously unused) logger parameter, instead of silently marking the store done.
• L5: the candidate SQL's ssl_passphrase_command branch now requires a non-empty value; the candidate read skips a NULL name row instead of throwing.
• Amendment applied (scrub side only, per orders): added '%pass%', '%key%', '%credential%', '%pwd%' to the three value columns and to the dotted-name filter in CandidateSql. RulesVersion left at 1.

CHANGELOG. Rewrote the one [#4351] line above to fold in R1's three corrections (batched-not-whole-table scrub, whole-value masking for ssl_passphrase_command/dotted names, backup-tool secrets in archive_command/restore_command) plus L3's release note (upgrade every writer service). Kept it one line.

What ran locally: dotnet build Darling/Darling.Tests/Darling.Tests.csproj -p:EnableWindowsTargeting=true — 0 Warning(s), 0 Error(s). The harness RED/GREEN run above is the only executable proof for this lane's own file; PgSettingScrubLiveTests, StorageCommandTimeoutTests, LivePostgresCollectionHygieneTests, DocCommentHygieneTests could not run on macOS (net10.0-windows testhost) and are left to CI.

Left for lane 2 / CI: the redactor fixes (M2-M5, L1, L2) on PgSettingRedactor.cs, the full test suite run, and a merge of origin/dev if it has drifted. Docker rig pmpr-4351-l1 removed after use.

pm-pr lane report (lane 2, redactor)

Head pushed: 9cc40042bc0b70e6763efd2b994b149d95dd340b (merge of origin/dev onto lane 1's 74c475b4 plus this lane's commit d2b7ccea).

Items, per the ruling (comment-5840560804) and review (pullrequestreview-5323293334):

1. M2/M3/M4 (PgSettingRedactor.cs): applied R1's pasted fixes verbatim, adapted to the file's current shape.

   • M2: UriUserInfoPassword allows an empty user ([^:@/\s\u00A0]* instead of +), so postgresql://:secret@host is masked.
   • M3: added OptionSecretSpaced, a new regex/replace pass for a space-separated option (--password hunter2, --secret-access-key hunter2secret), with the (?!-) guard so a value-less flag doesn't swallow the next option.
   • M4: LibpqPasswordKeyword and AssignmentSecretName's quoted branches now use '(?:\\[\s\S]|[^'\\])*(?:'|$)\S* (and the double-quote equivalent) so whatever follows the closing quote — ;, another keyword, anything — is consumed too, restoring idempotence.
   • Added R1's probe inputs as [InlineData] rows, each also run through the existing per-case idempotence assert, plus a second-pass-over-the-whole-corpus loop (see below).

2. M5/L1 (ruling amendment): AssignmentSecretName's name part became (?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?<![A-Za-z0-9])KEY(?![A-Za-z0-9])), case-insensitive. credential and pwd joined WholeValueNameMarkers. IsWholeValueMasked now tests the marker against the whole dotted name (any segment), not just the substring after the last dot — vault.secret.value now masks. All seven R1 probe inputs (WALG_LIBSODIUM_KEY, AZURE_STORAGE_ACCESS_KEY, WALG_PGP_KEY_PASSPHRASE, PGBACKREST_REPO1_CIPHER_PASS, myext.api_credentials, app.db_pwd, vault.secret.value) are tested and redacted; sslkey=/path is tested and kept as-is (KEY is segment-bounded, PASS is not — see the pinned side effect below).

   • Side effect, called out and pinned rather than hidden: because PASS is a deliberately unbounded substring (matching the ruling's own text — "PASS covers PASSWORD, PASSWD and PASSPHRASE"), libpq's passfile= keyword now also gets masked (passfile=/x/.pgpass -> passfile=********). This over-masks a non-secret path; it never leaks. The old test asserting passfile unchanged was updated to assert the new masked form, with a comment explaining why.

3. L2 edge forms: fixed via the same quote-tail regex extension above, plus the URI password class now allows a raw space/NBSP ([^@/]* instead of [^@/\s]*) up to @. Added the classifier pin: PgLogEventsPipelineTests.ReloadParameterChangedLine_IsNotClassified_AndSoIsNeverStored shows a LOG: parameter "primary_conninfo" changed to "password=..." line matches no parser and Classify returns empty, so PgLogEventsCollector never stores it.

RED/GREEN evidence: built a throwaway net10.0 console harness (/tmp/redactor-harness, source copy of PgSettingRedactor.cs) mirroring the test file's full 45-case corpus plus a whole-corpus second-pass idempotence loop, since Darling.Tests targets net10.0-windows and can't run on macOS.

• On lane 1's head (74c475b4, before this lane's fix): 15 of 45 cases RED, including every M5/L1 probe and the M4 second-pass case.
• After this lane's fix: 45/45 GREEN, second-pass idempotence 0 failures.

What ran locally:

• dotnet build Darling/Darling.Tests/Darling.Tests.csproj -p:EnableWindowsTargeting=true — 0 Warnings, 0 Errors, before and after the origin/dev merge.
• The harness (pure logic, no xUnit, no database) — 45/45 pass.
• Darling.Tests, StorageCommandTimeoutTests, LivePostgresCollectionHygieneTests, DocCommentHygiene* did NOT run locally (WPF/net10.0-windows target, no Windows runtime host here) — left to CI.

CI at this head (9cc40042): all test/build jobs pass — Darling Linux build, Darling PG tests (0/1/2), Darling PostgreSQL tests, Darling whole-tree guards, Lite tests (0-3, result), build, check-branches, description-drift, Claude review guard all green. One job, review, failed at 15s — this looks like the review-gate awaiting a fresh reviewer verdict on the new head rather than a test failure; pm-pr should check it by hand before arming.

Locking/batching: no change. This lane touched only PgSettingRedactor.cs (pure regex/string logic, no I/O, no locks) and PgLogEventsPipelineTests.cs/PgSettingRedactorTests.cs. Nothing here changes the scrub's batching or locking shape from lane 1's 74c475b4, so per the ruling this does not require a second review round on its own.

CHANGELOG entry: unchanged — the redactor's fixes close leaks the ruling already scoped under [#4351]; no new user-visible behavior needs its own line beyond what lane 1 wrote.

pm-pr lane report (final-round fixes M1, M2, L1)

lane-4351-rf report: PR #4351 final-round fixes (M1, M2, L1)

New head: 4ba02c19 (was 9cc40042bc0b70e6763efd2b994b149d95dd340b). Branch
fix/4348-redact-pg-settings pushed. PR is still OPEN; head sha was confirmed
unchanged before push.

M1 — OptionSecretSpaced name part (pre-merge)

PgSettingRedactor.cs's space-separated option regex used the stale name part
(?:PASSWORD|PASSWD|SECRET|TOKEN). Swapped in the ruling's name part, the same
one AssignmentSecretName already used: (?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?<![A-Za-z0-9])KEY(?![A-Za-z0-9])),
case-insensitive.

Added redacting tests: gpg --passphrase S1, openssl enc -pass pass:S3,
--encryption-key S4, --credentials S5 — all mask to ********. Kept the
existing --no-password -h x negative (unchanged, still passes: (?!-) after
the option keeps a value-less flag from swallowing the next option).

PgSettingRedactorTests (54 tests, includes all pre-existing plus the 4 new
redacting cases and the M4/M5 corpus): GREEN, 54/54, 0 failed.

M2 — live test pinning H1's (server, day) batching (pre-merge, test only)

Added PgSettingScrubLiveTests.TheScrubBatchesPerServerAndDay_NotDayAlone:
seeds two servers' secret rows on the same day in the same compressed chunk,
runs with Options=-c timescaledb.max_tuples_decompressed_per_dml_transaction=1
(one target's own row count), and asserts RunAsync updates both rows without
53400.

Verified both directions with docker timescale/timescaledb:2.30.1-pg18 on
host port 55451, container pmpr-4351-rf, -c timezone=UTC:

• RED against the pre-H1 shape: checked out PgSettingScrub.cs from the
  S1b commit (8faac396, day-only batching, no server_id predicate) into the
  worktree, built (0 warnings), ran the new test — failed with
  53400: tuple decompression limit exceeded by operation, exactly the
  ruling's predicted failure mode.
• GREEN at head: restored the current PgSettingScrub.cs (git diff
  empty against the commit), rebuilt (0 warnings), ran again — 2/2 passed
  (both the new test and the pre-existing scrub e2e test).

Container removed (docker rm -f pmpr-4351-rf) after both runs.

L1 — doc comments matching current code (pre-merge, text only)

• PgSettingRedactor.cs ~30-31/36-42: assignment/option name list corrected to
  PASS/SECRET/TOKEN/CREDENTIAL/PWD/KEY on both paths; "after the last dot"
  corrected to "any dot-separated segment" (matches IsWholeValueMasked's
  actual substring-over-whole-name test).
• PgSettingRedactor.cs ~79-82 (passfile): clarified that passfile is masked
  by AssignmentSecretName's PASS substring match, not by
  LibpqPasswordKeyword (which deliberately excludes it) — an accepted
  over-mask, not a leak.
• PgSettingScrub.cs ~42-53: "grouped by calendar day" corrected to
  "(server, day)"; the 8,846,250/9.2M decompression figure and the fleet-size
  (~11 targets) trigger for day-only batching are now both stated.
• PgSettingScrub.cs ~62: dead pointer to a nonexistent "release note" now
  points at the CHANGELOG entry for Redact passwords and other secrets from stored PostgreSQL settings (#4348) #4351.
• PgSettingScrub.cs ~138-140 (candidate SQL comment): name list corrected to
  match the SQL's actual ILIKE predicates.

Rebuilt after each doc-only edit: 0 Warning(s), 0 Error(s) both times.

Build and test summary

• dotnet build Darling/Darling.Tests/Darling.Tests.csproj -p:EnableWindowsTargeting=true:
  0 Warning(s), 0 Error(s), both before and after git merge origin/dev
  (merge was clean, no conflicts — 27 files from dev, none overlapping the
  three files this lane touched).
• Ran locally (macOS, WPF framework reference stripped from
  Darling.Tests.runtimeconfig.json post-build, per house pattern):
  • PgSettingRedactorTests: 54/54 passed.
  • PgSettingScrubLiveTests (both methods, live docker): 2/2 passed.
  • StorageCommandTimeoutTests, DocCommentHygiene*: passed (included in
    the combined run below, no [FAIL] for those classes).
  • LivePostgresCollectionHygieneTests.EveryClassUsingTheSharedStore_IsSerializedOrDocumentsWhyNot:
    FAILED locally with ReflectionTypeLoadException / missing
    PresentationFramework — this is the macOS/WPF-desktop-strip
    limitation (the test enumerates all types in the assembly via reflection,
    which needs the WPF runtime this host doesn't have once
    Microsoft.WindowsDesktop.App is removed from the runtimeconfig to run
    at all). Pre-existing environment limitation, not caused by this lane's
    edits — nothing in scope touches that test or its fixture list. CI (real
    Windows) decides it.
  • Combined run total: 161 tests, 1 failed (the above), 0 errors.

Deferred (out of this lane's scope per brief)

M3, M4, L2-L4 are explicitly the coordinator's call per the brief and were not
touched.

CHANGELOG entry

Left to the PR body's existing ## CHANGELOG entry section; this lane did not
edit CHANGELOG.md. The two nits from the review (key/token claim now holds
for --option value forms after M1; "any fleet size" → "at the default
hourly cadence" per M3's caveat) still need to be applied to the PR body text
by whoever edits it next — this lane ran out of budget before doing that
edit itself; flagging it here so it isn't dropped.

pm-pr lane report (M3, L3, L4)

New head: 58c2f680 (parent 4ba02c19, pushed to fix/4348-redact-pg-settings).

M3(a): SET LOCAL, not batch-per-collection_time

Chose SET LOCAL timescaledb.max_tuples_decompressed_per_dml_transaction = 0 inside each batch's own transaction, over splitting batches by exact collection_time. It keeps H1's exact shape (one literal server_id filter + one literal day range per UPDATE) instead of adding a third grouping dimension. Confirmed live on timescale/timescaledb:2.30.1-pg18 (docker, port 55451): the GUC's pg_settings.context is user, so an ordinary non-superuser LOGIN role can SET LOCAL it — no grant needed. 0 disables the limit for that one transaction only; MaxKeysPerUpdate (500) already bounds how much a single batch can touch, so disabling the safety net at that scope is safe.

M3(b): per-server catch, retry, marker semantics

Failures are now caught per (server, day) batch via catch (NpgsqlException ex). On failure: log a WARNING with serverId, day, and ex.SqlState only — never exception text. The failing server's remaining days this run are skipped (same failure mode); other servers continue. The marker (collect.collector_state, existing V44 key/value table — no migration) is written only when rowsUpdated == changedRowCount AND no server failed. A partial run leaves the marker unset, so the whole run — including servers that already succeeded — retries on the next start (cheap: already-redacted rows contribute nothing to changedRowCount on re-run).

Pins — RED on 4ba02c1, GREEN on 58c2f68

Ran Darling.Tests.PgSettingScrubLiveTests against the docker fixture (role darling, port 55451):

• RED (checked out PgSettingScrub.cs from 4ba02c1): Total: 4, Failed: 2 — the two new pins failed.
• GREEN (fix restored): Total: 4, Failed: 0.
• Full PgSettingScrubLiveTests + PgSettingRedactorTests: Total: 63, Failed: 0.

New pins: (i) TheScrubCompletes_WhenOneTargetsOneDayExceedsTheDecompressLimit — 200 rows, one server, one day, cap=50; completes and clears the secret. (ii) TheScrub_IsolatesAFailingServer_AndRetriesOnTheNextRun — a CHECK constraint rejects only the bad server's redacted row (real 23514, not mocked); good server is scrubbed, marker stays unset, bad server keeps its secret; after dropping the constraint the next run completes and clears everything.

L3: allowlist

Added PgSettingRedactor.NonSecretAllowlist (exact, case-insensitive): rds.accepted_password_auth_method, rds.restrict_password_commands, passwordcheck.min_password_length. Checked before the dotted-name marker scan. Pins in PgSettingRedactorTests: each name unchanged (including a case-varied form), plus a negative — app.db_password = 'fake-secret-value' is still masked to ********. sslkey/ssl_key_file/krb_server_keyfile untouched, as required.

L4: no durable buffer/spool found

Grepped Darling/PerformanceMonitor.Darling.Service/ and PerformanceMonitor.Collectors/ for spool/buffer/outbox patterns around config writes. The only hits are in QueryStoreCollector.cs, unrelated SQL Server plan-spill comments. No pre-existing buffered/spooled path can write pg_server_config after the marker is set. Nothing to fix.

CHANGELOG phrases (for pm-pr to apply)

• Replace "completes at the default hourly settings cadence" with: "completes at any settings cadence, and a target that fails is retried on the next start".
• Add: "non-secret password-policy settings (rds.accepted_password_auth_method, rds.restrict_password_commands, passwordcheck.min_password_length) are no longer masked".

Riskiest changed lines (for the narrow Opus M3 check)

1. RunBatchAsync: the new BeginTransactionAsync + SET LOCAL ... = 0 + CommitAsync wrapping — if CommitAsync is ever skipped on an exception path, the transaction leaks open on the shared connection for the rest of the run.
2. RunAsync's new currentServerId/currentServerFailed skip logic — relies on byServerDay being sorted by (ServerId, Day) (it's a SortedDictionary) so a failed server's later days are contiguous; if that iteration order ever changes, a failed server's later day could silently re-attempt or be skipped wrongly.
3. The marker gate rowsUpdated == changedRowCount && failedServerIds.Count == 0 — changedRowCount only counts rows from days that didn't throw before summing, so a genuinely mismatched write count (the old R1 L4 case) and a per-server failure are now conflated in one if; worth confirming the fallback else branch still logs distinctly enough to tell the two apart in practice.

pm-pr lane report (narrow M3 check fixes: W1, W2, S1, S3)

New head: 95829cf3 (previous head 58c2f680, merged origin/dev at ff0373bb on top with git merge, no conflicts). Branch fix/4348-redact-pg-settings.

W1 (pre-merge): guarded SET LOCAL

Replaced the bare SET LOCAL timescaledb.max_tuples_decompressed_per_dml_transaction = 0 in RunBatchAsync with the review's guarded form: SELECT set_config('timescaledb.max_tuples_decompressed_per_dml_transaction', '0', true) WHERE current_setting('timescaledb.max_tuples_decompressed_per_dml_transaction', true) IS NOT NULL. set_config(..., true) is the transaction-local equivalent of SET LOCAL; current_setting(name, true) returns NULL rather than raising when the GUC doesn't exist, so the WHERE turns a pre-2.14 store's batch into a no-op instead of a 42704 on every batch, every start. H1's shape (one literal server_id filter + one literal day range per batch) is unchanged — no third grouping key added.

Pin: added TheSetLocalIsGuardedForAPre214TimescaleDbStore in PgSettingScrubLiveTests.cs — a source-text pin (two Assert.Contains on the SQL text, since it's built by string concatenation) confirming the guarded form is present. The existing M3(a) pin (TheScrubCompletes_WhenOneTargetsOneDayExceedsTheDecompressLimit, 200 rows/one server/one day, cap=50) still passes on 2.30, proving the guard doesn't block the over-limit batch on a store that DOES have the GUC. A live simulation of an absent-GUC store on 2.30 isn't possible (the setting is always present there), so I did not attempt one.

W2: comment fix

Replaced the false "MaxKeysPerUpdate already bounds decompression" claim in RunBatchAsync's comment with the review's corrected text: the literal server_id + day-range predicates bound decompression to one target's segment in one day's chunk (~8k rows hourly, ~500k at a 1-minute cadence), not the whole chunk; MaxKeysPerUpdate bounds the key array, not the decompress.

S1: order-independent skip

Dropped currentServerId/currentServerFailed from RunAsync; the skip now reads failedServerIds.Contains(serverId) directly. failedServerIds.Add(serverId) already recorded the failure, so this is behaviorally identical and no longer depends on byServerDay's (server, day) sort grouping a server's days contiguously.

S3: tightened pins

• TheScrubCompletes_WhenOneTargetsOneDayExceedsTheDecompressLimit: added a marker-is-set assertion after the successful run.
• TheScrub_IsolatesAFailingServer_AndRetriesOnTheNextRun: added RowsUpdated assertions on both runs (first=1, second=1) and a marker-is-set assertion after the retry succeeds.
• Added a comment on badServer < goodServer's ordering: it pins that badServer's rolled-back batch (runs first, since SortedDictionary orders by server_id) doesn't poison the connection for goodServer's batch right after.

Tests

Docker timescale/timescaledb:2.30.1-pg18, container pmpr-4351-w1, host port 55451, -c timezone=UTC. Build: dotnet build Darling/Darling.Tests/Darling.Tests.csproj — 0 Warning(s), 0 Error(s), both before and after the origin/dev merge. Ran via the in-process xUnit v3 runner (WPF framework stripped from Darling.Tests.runtimeconfig.json post-build, per house pattern — dotnet test itself errors on macOS with this project's net10.0-windows target). Had to create role/db darling/darling in the container first (the fixture connects as that role; the previous M3 lane used the same setup).

PgSettingScrubLiveTests + PgSettingRedactorTests: Total: 64, Failed: 0, Errors: 0.

Container removed after the run (docker rm -f pmpr-4351-w1).

Follow-ups left (per brief, not touched)

• W2's timeout suggestion (S2 in the narrow review): a client-side command timeout logs an empty SQLSTATE, and a broken connection after a failed cancel ends the run early (remaining servers not attempted that start). Fails safe, but worth a log fix and an optional explicit connection.State != Open break.
• S2 (same item as above, listed separately in the brief): unchanged.

Push

Confirmed via gh-pm.sh pr view 4351 --json headRefOid,state that the head was still 58c2f680 (OPEN) immediately before pushing. Plain git push (no force). No GitHub-writing gh command was run.

[erikdarlingdata]

Security review, round 1 (lane R1), head 8faac39

Verdict: ship after fixes. The collector wiring is right: every stored value passes through Redact before a Row exists, in both arms and all three columns. H1 must be fixed before merge, because the scrub cannot finish on a fleet of 11 or more PostgreSQL targets. The Medium findings are real leaks or broken claims, and each fix is small.

How I checked: I ran a bypass probe that calls the PR's own PgSettingRedactor.Redact (a .NET 10 file-based app referencing PerformanceMonitor.Collectors, 37 inputs, with a leak check and an f(f(x)) check on each). I also ran a live rig on the repo's runtime zip (TimescaleDB, 1-day chunks, compress_segmentby = server_id, chunks compressed). On it I ran the scrub's BatchUpdateSql verbatim as a prepared statement, so the day range is a bound parameter exactly as Npgsql sends it. The rig is stopped.

High

H1. A day batch decompresses every target's rows for that day, so on 11 or more targets it fails with 53400, forever. PgSettingScrub.cs:155-168 (BatchUpdateSql) and :209 (grouping by day only).

• The join on server_id is not a constant, so TimescaleDB cannot filter compressed batches by segment. The day range admits every batch in the chunk, and the whole day is decompressed for all targets.
• At the default hourly cadence with about 380 settings, one target-day is 9,120 rows, so 11 targets pass the default max_tuples_decompressed_per_dml_transaction of 100,000.
• Rig, 12 targets on one day (109,440 rows), a 24-key batch for one target: ERROR: tuple decompression limit exceeded by operation DETAIL: current limit: 100000, tuples decompressed: 109440.
• Rig, 3 targets (27,360 rows): the batch succeeds, but it decompressed all 27,360 rows to change 24.
• S1b's measurement passed only because it seeded 3 targets (25,200 rows per day).
• Failure scenario: the exception is caught and the marker is never written. Every start repeats the full-table candidate read and fails again, and the plaintext stays. get_pg_server_config_changes then reports each secret-bearing setting as Changed, with the old plaintext beside the new mask. That is the false change ruling point 2 exists to prevent.
• Fix, verified on the same 12-target day (UPDATE 24, 9,120 heap rows, which is one target-day). Group batches by (server, day) and add a constant server predicate:

var byServerDay = new SortedDictionary<(int ServerId, DateTime Day), List<CandidateRow>>();
foreach (var c in candidates)
{
    var key = (c.ServerId, c.CollectionTime.Date);
    if (!byServerDay.TryGetValue(key, out var list)) { list = new List<CandidateRow>(); byServerDay[key] = list; }
    list.Add(c);
}

AND   t.collection_time >= $9 AND t.collection_time < $10
AND   t.server_id = $11"

update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Integer, Value = serverId });

• Pin it with a live test that seeds 2 or more targets on one day and opens the scrub's data source with Options=-c timescaledb.max_tuples_decompressed_per_dml_transaction=<one target-day + 1>. QueryStoreSliceRepairLiveTests already lowers this setting in a session. The test fails on the current shape and passes with the fix. DaysTouched becomes server-days, or keep a separate distinct-day count.

Medium

M1. The "recompresses inline" claim is false: touched chunks are left partially compressed. PgSettingScrub.cs:55-60, and S1b's second finding.

• Rig, after the 3-target batch: _timescaledb_catalog.chunk.status = 9 (compressed plus partial), while is_compressed still read t. SELECT count(*) FROM ONLY <chunk> returned 27,360, the whole day in the uncompressed heap.
• is_compressed cannot see the partial state, which is why S1b's check missed it.
• Scenario: with a standby present every day, the scrub moves one target-day per day into the heap (after H1's fix), about 3.3M rows for one standby over 365 days. Before the fix it moves the whole table. The space comes back only when the compression policy recompresses, and the deleted compressed batches stay as dead tuples until vacuum.
• Fix: correct the remark. Either state the expected temporary growth and leave it to the policy, or run compress_chunk(<chunk>) on each touched chunk after its batches.
• Truth consequence: the old plaintext row versions also survive in dead tuples until VACUUM, and in WAL. See the CHANGELOG note below.

M2. A URI with an empty user name leaks its password. PgSettingRedactor.cs:88.

• Probe: postgresql://:S6@primary:5432/db comes back unchanged.
• libpq accepts an empty user in the user info and still stores the password (conninfo_uri_parse_options). The + requires a user name.
• Fix: @"://(?<user>[^:@/\s]*):[^@/\s]*@", plus an InlineData row.

M3. A space-separated option leaks, although the ruling's "option whose name contains PASSWORD, SECRET..." covers it. PgSettingRedactor.cs:101-103.

• Probe: mycmd --password S21 %p and aws s3 cp s3://b/%f %p --secret-access-key S26 both come back unchanged.
• The candidate SQL already matches these rows, so only the redactor needs to change. Fix:

private static readonly Regex OptionSecretSpaced = new(
    @"(?<=^|\s)(?<opt>--?[\w.-]*(?:PASSWORD|PASSWD|SECRET|TOKEN)[\w.-]*)\s+(?!-)(?:""(?:\\.|[^""\\])*""\S*|'(?:\\.|[^'\\])*'\S*|\S+)",
    RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant);
// in Redact, after AssignmentSecretName:
redacted = OptionSecretSpaced.Replace(redacted, static m => m.Groups["opt"].Value + " " + Mask);

• (?!-) keeps a value-less flag such as --no-password -h x from swallowing the next option. The rule maps --password ******** onto itself.

M4. The idempotence claim fails for a quoted value followed by a character other than whitespace. PgSettingRedactor.cs:48-51, :82 and :102.

• Probe: export PGPASSWORD='S25 y'; psql becomes export PGPASSWORD=********; psql on the first pass. The second pass drops the ;, giving export PGPASSWORD=******** psql, because \S* matches ********;.
• The same happens with libpq's password='x'host=y.
• Scenario: the scrub's candidate read also matches rows the new collector has already redacted, because they still contain PGPASSWORD. Rows collected between the upgrade and the scrub's finish, or before any re-run, get rewritten and then differ from later rows. That produces a false Changed in the change history.
• Fix: let a quoted value take the rest of its token in both regexes, as '(?:\\.|[^'\\])*'\S* and ""(?:\\.|[^""\\])*""\S*. Add export PGPASSWORD='hunter 2'; psql → export PGPASSWORD=******** psql to the corpus, and the existing f(f(x)) assert then pins it.

M5. Common backup-tool secrets in archive_command/restore_command leak, because the assignment list is narrower than the ruling's own whole-value list. PgSettingRedactor.cs:101-103.

• Probe: these all come back unchanged, and all are real WAL-G, Azure and pgBackRest variable names: WALG_LIBSODIUM_KEY=S17, AZURE_STORAGE_ACCESS_KEY=S18, WALG_PGP_KEY_PASSPHRASE=S19, PGBACKREST_REPO1_CIPHER_PASS=S20.
• The ruling lists passphrase, key and salt for extension names but not for assignments. Changing that needs a ruling amendment.
• Proposed name part: (?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?<![A-Za-z0-9])KEY(?![A-Za-z0-9])). PASS covers PASSWORD, PASSWD and PASSPHRASE. The KEY segment test leaves libpq's sslkey=/path alone.
• The candidate SQL needs '%pass%', '%key%', '%credential%' and '%pwd%' on the three value columns.
• RulesVersion can stay 1 if this lands before merge.

Low

• L1. Extension names outside the marker list keep their value: myext.api_credentials (probe: unchanged) and app.db_pwd. So does a marker that sits only in a middle segment, such as vault.secret.value. Fix: add credential and pwd to WholeValueNameMarkers (:65-74) and to the SQL name filter.
• L2. Edge forms that leak (probe-confirmed). Each is rare, and some are invalid libpq:
  • a raw space in a URI password: postgresql://u:pa S8@h/db;
  • a percent-encoded query keyword: ?pass%77ord=S9 (libpq decodes keywords);
  • a backslash-newline inside a quoted value: \\. does not match \n, so use \\[\s\S];
  • an NBSP inside a password: \S is Unicode-aware but libpq's isspace is not;
  • "password = S16 ..." after a double quote;
  • curl -u admin:S22;
  • an unterminated quote, password='S5 unterminated, which masks only the first word. '(?:\\[\s\S]|[^'\\])*(?:'|$) masks to the end.
• L3. Marker per store (collect.collector_state, fleet sentinel): two new-build services running the scrub at once are safe, because the updates are idempotent (after M4) and a lock failure leaves the marker unwritten. But a service still on an old build that writes to the same store keeps storing plaintext after the marker is set, and nothing re-runs. Put this in the release note: upgrade every service that writes to a store.
• L4. RunAsync (:261) writes the marker even when rowsUpdated is smaller than the number of changed rows. Retention dropping a day mid-run is benign, but a systematic key mismatch would mark the store scrubbed with plaintext left. Fix: skip the marker and log a warning when the counts differ. The next start re-reads, and the dropped rows are gone by then.
• L5. Three smaller points:
  • The candidate read returns every target's ssl_passphrase_command row every hour even when it is empty (the default), about 8,760 rows per target-year held in memory for nothing. Fix: OR (name = 'ssl_passphrase_command' AND (setting <> '' OR boot_val <> '' OR reset_val <> '')).
  • reader.GetString(2) (:200) throws on a NULL name. The column is nullable in V102, and one such row would fail the scrub at every start.
  • RunAsync's logger parameter is unused.

Scrub cost and locks

• The candidate read has no time bound, so it decompresses every chunk. At S1b's rate (14.6 s per 9.2M rows, about 630k rows/s), a 927 MB compressed table would take:
  • about 7 s at S1b's synthetic density (about 215 B/row, or 4.3M rows);
  • about 75-150 s for real hourly near-duplicates at 10-20 B/row (45-90M rows).
• Both are inside CandidateReadTimeoutSeconds = 300. StorageCommandTimeoutTests scans only the Storage project, so it does not police PgSettingScrub.cs; the three deadlines are explicit.
• The read holds AccessShareLock on every chunk while it runs, which delays retention's drop_chunks.
• Batch updates take row locks and RowExclusiveLock on past-day chunks. The collector's inserts go to the open chunk, so they do not conflict.
• While H1 stands, the full read repeats at every start.

Coverage (checked, no finding)

• PgServerConfigCollector.ReadAsync is the only writer of collect.pg_server_config. Both arms and setting, boot_val and reset_val are redacted before the Row exists.
• The other collectors' current_setting() reads are fixed names with no secrets.
• The scrub's warning logs ex.Message, which for an Npgsql error carries no DETAIL or row values by default.
• S1b's claim that nothing else copies these values holds for facts, alerts and Custom Views: readers go to the table, and the fact converters take numeric and boolean settings only.
• Not verified: PgLogEventsCollector stores message prose as written. PostgreSQL's reload line parameter "primary_conninfo" changed to "..." is at LOG severity, and it should fall outside the six stored families, but I did not trace the classifier. A one-line classifier test would pin it.

CHANGELOG entry

One line, labelled [#4351], and it tells operators to rotate a primary_conninfo password. Three corrections:

1. "Existing stored rows are scrubbed in place" is false on 11 or more targets until H1 is fixed.
2. "Keep the rest of the value" is not true for ssl_passphrase_command and dotted secret names, which are masked whole.
3. The rotation advice should also cover secrets in archive_command/restore_command, and say that the store's WAL archive, replicas and dead row versions hold the old text, not only backups and exports.

Also noted: the PR's review check is failing (20 s). The build and PG test jobs pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3

[erikdarlingdata]

Ruling on review round 1 (coordinator)

Review: pullrequestreview-5323293334 (lane R1, head 8faac39). Verdict: ship after fixes. Every finding is accepted as written, including R1's pasted fixes. Two fix lanes, three items each, then this PR merges when CI is green. No second review round is needed unless a fix changes the scrub's locking or batching beyond H1's shape.

Amendment to ruling 5838869407 (M5, L1). The assignment and option name test becomes R1's proposed name part:
(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?<![A-Za-z0-9])KEY(?![A-Za-z0-9])), case-insensitive. PASS covers PASSWORD, PASSWD and PASSPHRASE. The KEY segment test leaves libpq's sslkey=/path alone. credential and pwd join WholeValueNameMarkers, and a marker in any segment of a dotted name counts, not only the last one. The scrub's candidate SQL gains '%pass%', '%key%', '%credential%' and '%pwd%' on the three value columns and the matching name tests. RulesVersion stays 1, because nothing has shipped with version 1.

Lane 1, the scrub (PgSettingScrub.cs):

1. H1: batch by target and day, with a literal target filter and the literal day range in each UPDATE. Pin it with R1's 12-target case (109,440 rows in one day) passing under the default timescaledb.max_tuples_decompressed_per_dml_transaction.
2. M1, L4, L5: correct the "recompresses inline" comment (touched chunks stay partly compressed until the compression job's next run); write no marker when the updated count differs from the changed count, and log a warning; skip empty ssl_passphrase_command rows in the candidate read; guard a NULL name; drop the unused logger parameter or use it.
3. The CHANGELOG line: R1's three corrections, plus L3's release note (upgrade every service that writes to a store, or an old build keeps storing plaintext after the scrub). Keep it one line, labelled [#4351].

Lane 2, the redactor (PgSettingRedactor.cs):

1. M2, M3, M4: R1's pasted fixes (the empty user-name URI, space-separated options, idempotence for a quoted value followed by ;), with R1's probe inputs as tests, and a second-pass-changes-nothing assertion over the whole corpus.
2. M5 and L1: the amended name part above, with R1's probe inputs (WALG_LIBSODIUM_KEY, AZURE_STORAGE_ACCESS_KEY, WALG_PGP_KEY_PASSPHRASE, PGBACKREST_REPO1_CIPHER_PASS, myext.api_credentials, app.db_pwd, vault.secret.value) and sslkey=/path kept as is.
3. L2's edge forms, using R1's regex fixes (\\[\s\S] for a backslash-newline, '(?:\\[\s\S]|[^'\\])*(?:'|$) for an unterminated quote); mask a URI password up to its @ even with a raw space or an NBSP in it. Add R1's one-line classifier test that PostgreSQL's reload line (parameter "primary_conninfo" changed to ...) is not stored by PgLogEventsCollector.

Both lanes run every class in each test file they touch, StorageCommandTimeoutTests, LivePostgresCollectionHygieneTests and DocCommentHygiene*. The second lane to finish merges origin/dev and runs the full suite once.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3