From f56ee03d17f26b1321e84481a42fdd4ae464e24d Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 16:21:59 -0400 Subject: [PATCH 01/11] Add PgSettingRedactor for #4348 The redactor S1b's scrub and this lane's collector wiring both call. Masks libpq password/sslpassword keywords, URI userinfo/query passwords, PASSWORD/PASSWD/SECRET/TOKEN assignments inside a value, and whole-value masks for ssl_passphrase_command and dotted extension settings whose last name segment names a secret. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../PgSettingRedactor.cs | 162 ++++++++++++++++++ 1 file changed, 162 insertions(+) create mode 100644 PerformanceMonitor.Collectors/PgSettingRedactor.cs diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs new file mode 100644 index 0000000000..e0404d8b20 --- /dev/null +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -0,0 +1,162 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Text.RegularExpressions; + +namespace PerformanceMonitor.Collectors; + +/// +/// Masks secrets out of a pg_settings / pg_db_role_setting value before it is stored (#4348). +/// +/// The monitoring role is granted pg_monitor, which includes pg_read_all_settings, so it +/// reads GUC_SUPERUSER_ONLY settings — including primary_conninfo, which on a standby carries the +/// replication password in plain text. Nothing upstream of storage was redacting it, so a stored row, a backup +/// of the store, or an export of it, all held a live credential. +/// +/// Rules (applied to every name/value pair the collector reads): +/// +/// Inside any value, whatever the setting's name, the secret is masked and the rest of the value is kept: +/// +/// the value of a libpq password or sslpassword keyword, quoted or not, honoring libpq's +/// quoting and \\ / \' escapes; +/// the password in a URI's user info (scheme://user:secret@host), and a password query +/// parameter; +/// the value of an assignment or option whose name contains PASSWORD, PASSWD, SECRET +/// or TOKEN (case-insensitive), such as PGPASSWORD=... or --password=.... +/// +/// Host, port, user, dbname and application_name are never touched. +/// ssl_passphrase_command has its whole value masked (an empty value stays empty — there is +/// nothing to mask). +/// An extension setting — a name with a dot, such as anon.salt — has its whole value masked when +/// the part of the name AFTER THE LAST DOT contains (as a substring, case-insensitive) password, +/// passwd, passphrase, secret, salt, token or key. This is a plain +/// substring test, not a whole-word one: myext.api_key matches (ends in key) and so would a name +/// that merely happens to contain those letters together, such as myext.turkey_interval — the false +/// positive it can produce only over-masks, never leaks, and myext.keep_alive is the negative case that +/// proves it (no marker word is a substring of keep_alive). +/// Nothing else changes: a core setting whose name merely contains one of those words, such as +/// password_encryption, keeps its value — only the DOTTED, extension-scoped form triggers a whole-value +/// mask by name. +/// +/// +/// Every rule maps the already-masked marker back onto itself, so is idempotent — +/// running it twice on the same value is the same as running it once, which matters because the collector and +/// the one-time scrub of pre-existing rows (S1b of #4348) both call it, and the scrub may run over rows a newer +/// collector has already redacted. +/// +public static class PgSettingRedactor +{ + /// + /// Bumped whenever the rules above change in a way that would redact a previously-unmasked value + /// differently. The one-time scrub (#4348) records the version it ran under, so a later rules change can + /// tell which stored rows were scrubbed under an older, narrower rule set. + /// + public const int RulesVersion = 1; + + private const string Mask = "********"; + + /// Names whose value is masked in full when the setting is extension-scoped (#4348). + private static readonly string[] WholeValueNameMarkers = + { + "password", + "passwd", + "passphrase", + "secret", + "salt", + "token", + "key", + }; + + /// A libpq password/sslpassword keyword inside a conninfo-shaped value. The keyword + /// must sit at the start of the string or after whitespace, exactly like every other libpq keyword=value + /// pair, so it does not fire on an unrelated keyword such as passfile. The value is either a + /// libpq-quoted string (backslash escapes \\ and \', closing on the first unescaped ') + /// or an unquoted run of non-whitespace. + private static readonly Regex LibpqPasswordKeyword = new( + @"(?<=^|\s)(?sslpassword|password)\s*=\s*(?:'(?:\\.|[^'\\])*'|\S*)", + RegexOptions.IgnoreCase | RegexOptions.Compiled); + + /// The password half of a URI's user info: scheme://user:secret@host. A bare + /// user@host, with no password, does not match and the username is left alone. + private static readonly Regex UriUserInfoPassword = new( + @"://(?[^:@/\s]+):[^@/\s]*@", + RegexOptions.Compiled); + + /// A password query parameter in a URI's query string. + private static readonly Regex UriQueryPassword = new( + @"(?<=[?&])(?password)=[^&#\s'""]*", + RegexOptions.IgnoreCase | RegexOptions.Compiled); + + /// A shell-style or option-style assignment (NAME=value, --name=value) whose name + /// contains PASSWORD, PASSWD, SECRET or TOKEN. Covers PGPASSWORD=x, AWS_SECRET_ACCESS_KEY=x, + /// --password=x inside archive_command/restore_command-shaped values. The value side + /// accepts the same libpq-style quoting as , plus double quotes, so a + /// quoted value with an = or space inside it is not mistaken for the start of the next token. + private static readonly Regex AssignmentSecretName = new( + @"(?[\w.-]*(?:PASSWORD|PASSWD|SECRET|TOKEN)[\w.-]*)=(?:""(?:\\.|[^""\\])*""|'(?:\\.|[^'\\])*'|\S*)", + RegexOptions.IgnoreCase | RegexOptions.Compiled); + + /// + /// Masks the secret out of per the rules on this type, given the setting's + /// . Never throws. in, out; an empty + /// string is returned unchanged, since there is nothing in it to mask. + /// + public static string? Redact(string? name, string? value) + { + if (string.IsNullOrEmpty(value)) + { + return value; + } + + if (IsWholeValueMasked(name)) + { + return Mask; + } + + var redacted = LibpqPasswordKeyword.Replace(value, static m => m.Groups["kw"].Value + "=" + Mask); + redacted = UriUserInfoPassword.Replace(redacted, static m => "://" + m.Groups["user"].Value + ":" + Mask + "@"); + redacted = UriQueryPassword.Replace(redacted, static m => m.Groups["key"].Value + "=" + Mask); + redacted = AssignmentSecretName.Replace(redacted, static m => m.Groups["name"].Value + "=" + Mask); + + return redacted; + } + + private static bool IsWholeValueMasked(string? name) + { + if (string.IsNullOrEmpty(name)) + { + return false; + } + + if (string.Equals(name, "ssl_passphrase_command", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + // Extension-scoped settings only: a bare core GUC such as password_encryption never matches here, + // no matter what its name contains — only the part after the LAST dot is tested, and only when + // there is a dot at all. + var dot = name.LastIndexOf('.'); + if (dot < 0 || dot == name.Length - 1) + { + return false; + } + + var lastPart = name.AsSpan(dot + 1); + foreach (var marker in WholeValueNameMarkers) + { + if (lastPart.Contains(marker, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; + } +} From 1c13ea4ae5d83fe4f6111bc267f29351e2560f0f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 16:33:31 -0400 Subject: [PATCH 02/11] Wire PgSettingRedactor into the PostgreSQL config collector (#4348) Every row PgServerConfigCollector.ReadAsync produces now passes setting, boot_val and reset_val through PgSettingRedactor.Redact before a Row is built, in both UNION ALL arms (they share one read loop). Adds the unit corpus pinning the redactor's rules directly, and a live test that plants a secret in primary_conninfo, reads it back through the real collector under a pg_monitor-only role, and proves the test fails with the wiring reverted. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../PgSettingRedactionLivePostgresTests.cs | 163 ++++++++++++++++++ .../Darling.Tests/PgSettingRedactorTests.cs | 146 ++++++++++++++++ .../PgServerConfigCollector.cs | 16 +- .../PgSettingRedactor.cs | 8 +- 4 files changed, 325 insertions(+), 8 deletions(-) create mode 100644 Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs create mode 100644 Darling/Darling.Tests/PgSettingRedactorTests.cs diff --git a/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs b/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs new file mode 100644 index 0000000000..93ac9a5ea2 --- /dev/null +++ b/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs @@ -0,0 +1,163 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Collectors; +using Xunit; + +namespace Darling.Tests; + +/// +/// #4348, end to end, against a REAL catalog: the monitoring role is granted only pg_monitor — never +/// superuser — and pg_monitor includes pg_read_all_settings, so that role sees +/// GUC_SUPERUSER_ONLY settings such as primary_conninfo. Its raw text on a standby can carry a +/// replication password in plain text. This plants that value with ALTER SYSTEM SET, reads it back +/// through the REAL (not a hand-rolled column read, so a revert of the +/// collector's redaction call sites fails this test), and asserts the secret never reaches a Row while +/// host, port, user and application_name survive. +/// +/// Cluster-wide state (ALTER SYSTEM is not per-database), so this is serialized with the rest of +/// the live-postgres collection and restores the setting in a finally that runs even on failure. +/// +[Collection("live-postgres")] +public sealed class PgSettingRedactionLivePostgresTests +{ + private const string RoleName = "s1a_4348_redact_probe"; + private const string RolePassword = "S1aRedactProbe4348Only"; + private const string SecretConninfo = "host=127.0.0.1 port=1 user=replicator password=hunter2 application_name=s1a"; + + private static string? ConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + + [Fact] + public async Task PrimaryConninfosPassword_IsRedactedByTheRealCollector_UnderAPgMonitorOnlyRole() + { + var cs = ConnectionString; + Assert.SkipWhen(string.IsNullOrEmpty(cs), "Set DARLING_TEST_PG to a Postgres connection string to run the #4348 redaction round trip."); + + var ct = TestContext.Current.CancellationToken; + await using var owner = new NpgsqlConnection(cs); + await owner.OpenAsync(ct); + + var bodySucceeded = false; + try + { + await ExecAsync(owner, $"ALTER SYSTEM SET primary_conninfo = '{SecretConninfo}'", ct); + await ExecAsync(owner, "SELECT pg_reload_conf()", ct); + + await ExecAsync(owner, $"DROP ROLE IF EXISTS {RoleName}", ct); + await ExecAsync(owner, $"CREATE ROLE {RoleName} LOGIN PASSWORD '{RolePassword}'", ct); + await ExecAsync(owner, $"GRANT pg_monitor TO {RoleName}", ct); + + var probeConnectionString = new NpgsqlConnectionStringBuilder(cs) + { + Username = RoleName, + Password = RolePassword, + Pooling = false, + }.ConnectionString; + + await using var probe = new NpgsqlConnection(probeConnectionString); + await probe.OpenAsync(ct); + + /* The vulnerability itself: a pg_monitor-only login, no superuser, reads the raw secret straight + off pg_settings. Nothing in PostgreSQL's own permission model stops it — that is why the + collector has to. */ + await using (var raw = new NpgsqlCommand("SELECT setting FROM pg_settings WHERE name = 'primary_conninfo'", probe)) + { + var rawValue = (string?)await raw.ExecuteScalarAsync(ct); + Assert.Contains("hunter2", rawValue, StringComparison.Ordinal); + } + + var context = new CollectorContext + { + ServerId = -4348, + ServerName = "pg-4348-redaction-probe", + CollectionTime = DateTime.UtcNow, + Deltas = new CollectorDeltaCalculator(), + Target = new CollectorTargetInfo { Engine = CollectorTargetEngine.PostgreSql, PostgresMajorVersion = 17 }, + ExcludedDatabases = Array.Empty(), + CurrentDatabaseName = probe.Database, + }; + + var sql = PgServerConfigCollector.Instance.BuildQuery(context).Text; + List rows; + await using (var read = new NpgsqlCommand(sql, probe)) + await using (var reader = await read.ExecuteReaderAsync(ct)) + { + rows = await PgServerConfigCollector.Instance.ReadAsync(reader, context, ct); + } + + PgServerConfigCollector.Row? conninfoRow = null; + foreach (var row in rows) + { + if (row.Name == "primary_conninfo" && row.DatabaseName is null && row.RoleName is null) + { + conninfoRow = row; + } + + /* NO column of ANY row — not just primary_conninfo's — carries the secret. boot_val and + reset_val go through the same redactor as setting, so this also covers them. */ + AssertNoSecret(row.Name); + AssertNoSecret(row.Setting); + AssertNoSecret(row.Unit); + AssertNoSecret(row.Category); + AssertNoSecret(row.Context); + AssertNoSecret(row.VarType); + AssertNoSecret(row.Source); + AssertNoSecret(row.BootValue); + AssertNoSecret(row.ResetValue); + AssertNoSecret(row.SourceFile); + AssertNoSecret(row.ShortDescription); + AssertNoSecret(row.DatabaseName); + AssertNoSecret(row.RoleName); + } + + Assert.NotNull(conninfoRow); + Assert.NotNull(conninfoRow!.Value.Setting); + var setting = conninfoRow.Value.Setting!; + Assert.Contains("password=********", setting, StringComparison.Ordinal); + Assert.Contains("host=127.0.0.1", setting, StringComparison.Ordinal); + Assert.Contains("port=1", setting, StringComparison.Ordinal); + Assert.Contains("user=replicator", setting, StringComparison.Ordinal); + Assert.Contains("application_name=s1a", setting, StringComparison.Ordinal); + + bodySucceeded = true; + } + finally + { + /* RunOwnedAsync, not RunAsync (#1902): the reset has to run on THIS session — primary_conninfo + and the probe role are cluster-wide, not store rows a fresh connection could reach just as + well, and there is nothing to reconnect to that a new connection would do differently. */ + await LiveStoreCleanup.RunOwnedAsync(bodySucceeded, async () => + { + await ExecAsync(owner, "ALTER SYSTEM SET primary_conninfo = ''", ct); + await ExecAsync(owner, "SELECT pg_reload_conf()", ct); + await ExecAsync(owner, $"DROP ROLE IF EXISTS {RoleName}", ct); + }); + } + } + + private static void AssertNoSecret(string? value) + { + if (value is null) + { + return; + } + + Assert.DoesNotContain("hunter2", value, StringComparison.Ordinal); + } + + private static async Task ExecAsync(NpgsqlConnection connection, string sql, CancellationToken ct) + { + await using var cmd = new NpgsqlCommand(sql, connection); + await cmd.ExecuteNonQueryAsync(ct); + } +} diff --git a/Darling/Darling.Tests/PgSettingRedactorTests.cs b/Darling/Darling.Tests/PgSettingRedactorTests.cs new file mode 100644 index 0000000000..49790d238f --- /dev/null +++ b/Darling/Darling.Tests/PgSettingRedactorTests.cs @@ -0,0 +1,146 @@ +// Copyright (c) Erik Darling Data. All rights reserved. +// Licensed under the terms in the LICENSE file in the repository root. + +using PerformanceMonitor.Collectors; +using Xunit; + +namespace Darling.Tests; + +/// +/// #4348 — the monitoring role's pg_monitor grant includes pg_read_all_settings, so +/// PgServerConfigCollector sees GUC_SUPERUSER_ONLY settings such as a standby's +/// primary_conninfo, which can carry a replication password in plain text. These pin +/// 's rules directly, without a database, so the corpus runs on every build. +/// +public sealed class PgSettingRedactorTests +{ + [Fact] + public void RulesVersionIsPinned() + { + Assert.Equal(1, PgSettingRedactor.RulesVersion); + } + + [Fact] + public void NullValue_StaysNull() + { + Assert.Null(PgSettingRedactor.Redact("primary_conninfo", null)); + } + + /// + /// Every case both matches its expected redaction and is idempotent — redacting an already-redacted + /// value must be a no-op, because the collector (new rows) and the S1b scrub (old rows, #4348) call + /// the same redactor, and the scrub can run over rows a newer collector already redacted. + /// + [Theory] + // libpq password keyword: unquoted, first / middle / last, spaces around '='. + [InlineData("primary_conninfo", + "password=hunter2 host=127.0.0.1 port=5432 user=replicator application_name=s1a", + "password=******** host=127.0.0.1 port=5432 user=replicator application_name=s1a")] + [InlineData("primary_conninfo", + "host=127.0.0.1 password=hunter2 port=5432", + "host=127.0.0.1 password=******** port=5432")] + [InlineData("primary_conninfo", + "host=127.0.0.1 port=5432 password=hunter2", + "host=127.0.0.1 port=5432 password=********")] + [InlineData("primary_conninfo", + "host=a password = hunter2 user=b", + "host=a password=******** user=b")] + // libpq password keyword: quoted, with '\'' and '\\' escapes inside the quotes. + [InlineData("primary_conninfo", + "password='hunter2' host=foo", + "password=******** host=foo")] + [InlineData("primary_conninfo", + @"password='it\'s\\a secret' host=foo", + "password=******** host=foo")] + // sslpassword — a distinct libpq keyword, not a substring match on "password". + [InlineData("primary_conninfo", + "sslpassword=hunter2 sslmode=require", + "sslpassword=******** sslmode=require")] + [InlineData("primary_conninfo", + "sslpassword='hunter2' sslmode=require", + "sslpassword=******** sslmode=require")] + // URI form: user info with and without a password, several hosts, percent-encoding, a query parameter. + [InlineData("primary_conninfo", + "postgresql://alice@host1:5432,host2:5432/db", + "postgresql://alice@host1:5432,host2:5432/db")] + [InlineData("primary_conninfo", + "postgresql://alice:hunter2@host1:5432,host2:5432/db", + "postgresql://alice:********@host1:5432,host2:5432/db")] + [InlineData("primary_conninfo", + "postgresql://alice:hun%40ter2@host/db", + "postgresql://alice:********@host/db")] + [InlineData("primary_conninfo", + "postgresql://alice@host/db?sslmode=require&password=hunter2", + "postgresql://alice@host/db?sslmode=require&password=********")] + // assignment / option whose NAME contains PASSWORD, PASSWD, SECRET or TOKEN. + [InlineData("archive_command", + "PGPASSWORD=hunter2 psql -c 'select 1'", + "PGPASSWORD=******** psql -c 'select 1'")] + [InlineData("archive_command", + "AWS_SECRET_ACCESS_KEY=hunter2secret wal-g wal-push %p", + "AWS_SECRET_ACCESS_KEY=******** wal-g wal-push %p")] + [InlineData("restore_command", + "pg_dump --password=hunter2 --host=foo", + "pg_dump --password=******** --host=foo")] + [InlineData("restore_command", + @"pg_dump --password=""hunter 2"" --host=foo", + "pg_dump --password=******** --host=foo")] + // ssl_passphrase_command: whole value masked; empty stays empty. + [InlineData("ssl_passphrase_command", + "/usr/bin/cat /etc/ssl/passphrase-hunter2.txt", + "********")] + [InlineData("ssl_passphrase_command", + "", + "")] + // extension setting (dotted name): whole value masked when the LAST segment names a secret. + [InlineData("anon.salt", "s0mesalt", "********")] + [InlineData("myext.api_key", "AKIAABCDEFG", "********")] + // the substring decision, pinned: "key" matches wherever it appears in the last segment, not only as + // its own word — myext.turkey_interval is not about a key at all, and is still masked in full. + [InlineData("myext.turkey_interval", "anything", "********")] + // negative: myext.keep_alive does not contain any marker (password/passwd/passphrase/secret/salt/token/key) + // as a substring of "keep_alive", so its value is left alone. + [InlineData("myext.keep_alive", "30s", "30s")] + // must not change at all. + [InlineData("password_encryption", "scram-sha-256", "scram-sha-256")] + [InlineData("unix_socket_directories", "passfile=/x/.pgpass", "passfile=/x/.pgpass")] + [InlineData("primary_conninfo", "host=a user=b", "host=a user=b")] + [InlineData("primary_conninfo", "", "")] + public void RedactsPerTheRuling_AndIsIdempotent(string name, string value, string expected) + { + var actual = PgSettingRedactor.Redact(name, value); + + Assert.Equal(expected, actual); + + // f(f(x)) == f(x): re-running the redactor over its own output changes nothing further. + Assert.Equal(actual, PgSettingRedactor.Redact(name, actual)); + } + + /// + /// Every rule applies wherever the value's own text says so — not only to primary_conninfo. + /// A password can hide behind an archive_command or restore_command just as easily. + /// + [Fact] + public void EmbeddedSecretRulesAreNotGatedOnTheSettingName() + { + Assert.Equal( + "PGPASSWORD=******** psql", + PgSettingRedactor.Redact("restore_command", "PGPASSWORD=hunter2 psql")); + } + + [Fact] + public void NeverThrows_OnEmptyName() + { + var result = PgSettingRedactor.Redact(string.Empty, "password=hunter2"); + + Assert.Equal("password=********", result); + } + + [Fact] + public void NeverThrows_OnNullName() + { + var result = PgSettingRedactor.Redact(null, "password=hunter2"); + + Assert.Equal("password=********", result); + } +} diff --git a/PerformanceMonitor.Collectors/PgServerConfigCollector.cs b/PerformanceMonitor.Collectors/PgServerConfigCollector.cs index 511779e4df..88633c5b3c 100644 --- a/PerformanceMonitor.Collectors/PgServerConfigCollector.cs +++ b/PerformanceMonitor.Collectors/PgServerConfigCollector.cs @@ -217,16 +217,24 @@ public override async ValueTask> ReadAsync(DbDataReader reader, Collec while (await reader.ReadAsync(cancellationToken)) { + var name = reader.GetString(0); + + /* #4348: pg_monitor includes pg_read_all_settings, so this connection sees GUC_SUPERUSER_ONLY + settings too — primary_conninfo on a standby carries a replication password in plain text. + Every value that can hold a secret is redacted here, before a Row is ever built, in BOTH + arms of the UNION ALL (they share this one read loop): setting, boot_val and reset_val. The + query text and every other column are untouched — this is the only place a stored row is + shaped, so it is the only place that needs to change. */ rows.Add(new Row( - Name: reader.GetString(0), - Setting: reader.IsDBNull(1) ? null : reader.GetString(1), + Name: name, + Setting: PgSettingRedactor.Redact(name, reader.IsDBNull(1) ? null : reader.GetString(1)), Unit: reader.IsDBNull(2) ? null : reader.GetString(2), Category: reader.IsDBNull(3) ? null : reader.GetString(3), Context: reader.IsDBNull(4) ? null : reader.GetString(4), VarType: reader.IsDBNull(5) ? null : reader.GetString(5), Source: reader.IsDBNull(6) ? null : reader.GetString(6), - BootValue: reader.IsDBNull(7) ? null : reader.GetString(7), - ResetValue: reader.IsDBNull(8) ? null : reader.GetString(8), + BootValue: PgSettingRedactor.Redact(name, reader.IsDBNull(7) ? null : reader.GetString(7)), + ResetValue: PgSettingRedactor.Redact(name, reader.IsDBNull(8) ? null : reader.GetString(8)), SourceFile: reader.IsDBNull(9) ? null : reader.GetString(9), SourceLine: reader.IsDBNull(10) ? 0 : reader.GetInt32(10), PendingRestart: !reader.IsDBNull(11) && reader.GetBoolean(11), diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs index e0404d8b20..0889d183dd 100644 --- a/PerformanceMonitor.Collectors/PgSettingRedactor.cs +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -80,18 +80,18 @@ public static class PgSettingRedactor /// or an unquoted run of non-whitespace. private static readonly Regex LibpqPasswordKeyword = new( @"(?<=^|\s)(?sslpassword|password)\s*=\s*(?:'(?:\\.|[^'\\])*'|\S*)", - RegexOptions.IgnoreCase | RegexOptions.Compiled); + RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); /// The password half of a URI's user info: scheme://user:secret@host. A bare /// user@host, with no password, does not match and the username is left alone. private static readonly Regex UriUserInfoPassword = new( @"://(?[^:@/\s]+):[^@/\s]*@", - RegexOptions.Compiled); + RegexOptions.Compiled | RegexOptions.CultureInvariant); /// A password query parameter in a URI's query string. private static readonly Regex UriQueryPassword = new( @"(?<=[?&])(?password)=[^&#\s'""]*", - RegexOptions.IgnoreCase | RegexOptions.Compiled); + RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); /// A shell-style or option-style assignment (NAME=value, --name=value) whose name /// contains PASSWORD, PASSWD, SECRET or TOKEN. Covers PGPASSWORD=x, AWS_SECRET_ACCESS_KEY=x, @@ -100,7 +100,7 @@ public static class PgSettingRedactor /// quoted value with an = or space inside it is not mistaken for the start of the next token. private static readonly Regex AssignmentSecretName = new( @"(?[\w.-]*(?:PASSWORD|PASSWD|SECRET|TOKEN)[\w.-]*)=(?:""(?:\\.|[^""\\])*""|'(?:\\.|[^'\\])*'|\S*)", - RegexOptions.IgnoreCase | RegexOptions.Compiled); + RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); /// /// Masks the secret out of per the rules on this type, given the setting's From 8faac3966b6173b9821c9009152961d32237c72f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:11:05 -0400 Subject: [PATCH 03/11] Scrub pre-existing unredacted PostgreSQL settings (#4348 S1b) One-time background job that redacts secrets an older collector build stored in plain text in collect.pg_server_config (a standby's primary_conninfo password, most notably), using S1a's PgSettingRedactor. Batches by calendar day with a literal time-range predicate, required because a join-only UPDATE against a compressed hypertable cannot exclude chunks and decompresses the whole table. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../PgServerConfigScopeRungTests.cs | 15 +- .../Darling.Tests/PgSettingScrubLiveTests.cs | 154 ++++++++ .../DarlingWorker.cs | 55 +++ .../PgSettingScrub.cs | 337 ++++++++++++++++++ 4 files changed, 558 insertions(+), 3 deletions(-) create mode 100644 Darling/Darling.Tests/PgSettingScrubLiveTests.cs create mode 100644 Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs diff --git a/Darling/Darling.Tests/PgServerConfigScopeRungTests.cs b/Darling/Darling.Tests/PgServerConfigScopeRungTests.cs index 8f4b7bfc52..d2a4623512 100644 --- a/Darling/Darling.Tests/PgServerConfigScopeRungTests.cs +++ b/Darling/Darling.Tests/PgServerConfigScopeRungTests.cs @@ -386,7 +386,15 @@ predicate and are not reads. */ var excludes = statement.Contains("database_name IS NULL", StringComparison.Ordinal) && statement.Contains("role_name IS NULL", StringComparison.Ordinal); var selects = statement.Contains("database_name IS NOT NULL OR", StringComparison.Ordinal); - if (!excludes && !selects) + + /* #4348 S1b: the one-time secret scrub is the census's third answer, not a missed choice. + A secret can land in a per-database or per-role override exactly as easily as in the + server-wide setting (the ruling's own test shape seeds both), so the scrub's candidate + read must carry NEITHER arm — it wants every row, whichever scope it is, and decides + row-by-row in C# via PgSettingRedactor rather than in this SQL. */ + var scrubsEveryScopeDeliberately = Path.GetFileName(file) == "PgSettingScrub.cs"; + + if (!excludes && !selects && !scrubsEveryScopeDeliberately) { undecided.Add($"{Path.GetFileName(file)} @ {match.Index}"); } @@ -394,8 +402,9 @@ predicate and are not reads. */ } /* 10 -> 12 (#3937): ScopedConfigChangesSql's `snapshots` subquery and `overrides` CTE, both aliased - `FROM pg_server_config AS c`, both classified "selects" above. */ - Assert.Equal(12, reads); + `FROM pg_server_config AS c`, both classified "selects" above. 12 -> 13 (#4348 S1b): the scrub's + one candidate read, classified above as the deliberate third answer. */ + Assert.Equal(13, reads); Assert.True(undecided.Count == 0, "a pg_server_config read carries neither arm of the V138 scope split, so it will see per-database " + "and per-role override rows as if they were the server's settings: [" diff --git a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs new file mode 100644 index 0000000000..c66b710a46 --- /dev/null +++ b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs @@ -0,0 +1,154 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Darling.Service; +using PerformanceMonitor.Darling.Storage; +using Xunit; + +namespace Darling.Tests; + +/// +/// Live, end-to-end proof of the #4348 S1b one-time scrub against a REAL compressed TimescaleDB chunk. +/// +/// #1776 own-store — mints its own scratch database () rather than +/// sharing the live fixture, so it is deliberately NOT in the live-postgres collection: it creates its +/// own hypertable/compression shape on collect.pg_server_config, which the shared fixture must never +/// inherit from a test. +/// +public sealed class PgSettingScrubLiveTests +{ + private const int ServerId = -444444; + private const string ServerName = "darling-setting-scrub-e2e"; + private const string Secret = "host=replica1.internal port=5432 user=replicator password=hunter2 sslmode=require"; + private static readonly DateTime OldTime = DateTime.SpecifyKind(new DateTime(2026, 1, 1, 0, 0, 0), DateTimeKind.Unspecified); + private static readonly DateTime NewTime = DateTime.SpecifyKind(new DateTime(2026, 1, 1, 1, 0, 0), DateTimeKind.Unspecified); + + [Fact] + public async Task TheScrubRedactsOldRows_LeavesHistoryUnchanged_AndIsIdempotent() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), + "Set DARLING_TEST_PG to a Postgres connection string (with TimescaleDB installed) to run the live #4348 scrub test (it mints its own scratch database)."); + + var ct = TestContext.Current.CancellationToken; + + await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + await using var connection = new NpgsqlConnection(scratch.ConnectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + + Assert.True(await TimescaleSupport.TryEnableAsync(connection, null, ct), + "the dev fixture is expected to have TimescaleDB installed"); + await ExecAsync(connection, "SELECT create_hypertable('collect.pg_server_config', by_range('collection_time', INTERVAL '1 days'), if_not_exists => true)", ct); + await ExecAsync(connection, "ALTER TABLE collect.pg_server_config SET (timescaledb.compress, timescaledb.compress_segmentby = 'server_id')", ct); + + /* Arm 1: a server-wide pg_settings row (database_name/role_name NULL), the OLD unredacted shape. */ + await InsertRowAsync(connection, ServerId, OldTime, "primary_conninfo", Secret, databaseName: null, roleName: null, ct); + /* Arm 2: a database_name override row (V138) — the standby's replication setting scoped to one database. */ + await InsertRowAsync(connection, ServerId, OldTime, "primary_conninfo", Secret, databaseName: "appdb", roleName: null, ct); + /* A NEWER row S1a's redactor already masked — same underlying value, already-redacted shape, server-wide. */ + await InsertRowAsync(connection, ServerId, NewTime, "primary_conninfo", "host=replica1.internal port=5432 user=replicator password=******** sslmode=require", databaseName: null, roleName: null, ct); + /* A control row with no secret at all — must come out byte-identical. */ + await InsertRowAsync(connection, ServerId, OldTime, "shared_buffers", "128MB", databaseName: null, roleName: null, ct); + + await ExecAsync(connection, "SELECT count(compress_chunk(c, if_not_compressed => true)) FROM show_chunks('collect.pg_server_config') c", ct); + + /* ── Prove the assertion below actually depends on the job: the secret is really there, compressed, before it runs. ── */ + Assert.True(await ContainsSecretAsync(connection, ct), "seeding failed to plant the secret this test exists to catch"); + + await using var postgres = NpgsqlDataSource.Create(scratch.ConnectionString); + + var first = await PgSettingScrub.RunAsync(postgres, logger: null, ct); + Assert.False(first.AlreadyDone); + Assert.True(first.RowsUpdated >= 2, $"expected at least the two secret-bearing rows to be updated, got {first.RowsUpdated}"); + + /* ── No column of any row contains the secret. ── */ + Assert.False(await ContainsSecretAsync(connection, ct), "the raw password survived the scrub"); + + /* ── The control row is byte-identical. ── */ + var controlValue = await ScalarTextAsync(connection, + "SELECT setting FROM collect.pg_server_config WHERE server_id = $1 AND collection_time = $2 AND name = 'shared_buffers'", + ServerId, OldTime, ct); + Assert.Equal("128MB", controlValue); + + /* ── The change history shows no CHANGE between the old (now-scrubbed) row and the newer (already-redacted) row. ── */ + await using (var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString)) + { + var changes = await DarlingPgServerConfigReader.GetConfigChangesAsync( + dataSource, ServerId, OldTime.AddHours(-1), NewTime.AddHours(1), 50, ct); + foreach (var change in changes) + { + if (change.Name == "primary_conninfo" && change.DatabaseName is null) + { + Assert.NotEqual(DarlingPgServerConfigReader.PgConfigChangeKind.Changed, change.ChangeKind); + } + } + } + + /* ── A second start does nothing: the marker holds. ── */ + var second = await PgSettingScrub.RunAsync(postgres, logger: null, ct); + Assert.True(second.AlreadyDone); + + /* ── An older marker (standing in for a future RulesVersion bump) runs the scrub again. ── */ + await ExecAsync(connection, + "UPDATE collect.collector_state SET state_value = '0' WHERE collector_name = 'pg_setting_scrub' AND state_key = 'rules_version'", ct); + var third = await PgSettingScrub.RunAsync(postgres, logger: null, ct); + Assert.False(third.AlreadyDone); + } + + private static async Task ContainsSecretAsync(NpgsqlConnection connection, CancellationToken ct) + { + await using var cmd = new NpgsqlCommand( + "SELECT count(*) FROM collect.pg_server_config WHERE setting LIKE '%hunter2%' OR boot_val LIKE '%hunter2%' OR reset_val LIKE '%hunter2%'", + connection) + { CommandTimeout = 60 }; + var count = (long)(await cmd.ExecuteScalarAsync(ct))!; + return count > 0; + } + + private static async Task ScalarTextAsync(NpgsqlConnection connection, string sql, int serverId, DateTime collectionTime, CancellationToken ct) + { + await using var cmd = new NpgsqlCommand(sql, connection) { CommandTimeout = 60 }; + cmd.Parameters.AddWithValue(serverId); + cmd.Parameters.AddWithValue(collectionTime); + return (await cmd.ExecuteScalarAsync(ct)) as string; + } + + private static async Task InsertRowAsync( + NpgsqlConnection connection, int serverId, DateTime collectionTime, string name, string value, + string? databaseName, string? roleName, CancellationToken ct) + { + await using var cmd = new NpgsqlCommand(@" +INSERT INTO collect.pg_server_config + (collection_id, collection_time, server_id, server_name, name, setting, unit, category, context, vartype, + source, boot_val, reset_val, sourcefile, sourceline, pending_restart, short_desc, database_name, role_name) +VALUES + ($1, $2, $3, $4, $5, $6, NULL, 'category', 'sighup', 'string', 'configuration file', $6, $6, NULL, NULL, false, 'desc', $7, $8)", connection) + { CommandTimeout = 60 }; + cmd.Parameters.AddWithValue((long)(collectionTime.Ticks ^ (long)name.GetHashCode() ^ (databaseName ?? "").GetHashCode())); + cmd.Parameters.AddWithValue(collectionTime); + cmd.Parameters.AddWithValue(serverId); + cmd.Parameters.AddWithValue(ServerName); + cmd.Parameters.AddWithValue(name); + cmd.Parameters.AddWithValue(value); + cmd.Parameters.AddWithValue((object?)databaseName ?? DBNull.Value); + cmd.Parameters.AddWithValue((object?)roleName ?? DBNull.Value); + await cmd.ExecuteNonQueryAsync(ct); + } + + private static async Task ExecAsync(NpgsqlConnection connection, string sql, CancellationToken ct) + { + await using var cmd = new NpgsqlCommand(sql, connection) { CommandTimeout = 120 }; + await cmd.ExecuteNonQueryAsync(ct); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs index 2f42dd038d..1b0a132041 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -1817,6 +1817,13 @@ message an operator greps for. */ } } + /* #4348 S1b: the one-time scrub of secrets an older collector build stored in plain text (a + standby's replication password inside primary_conninfo, most notably). Launched here, right + after migrations confirm collect.pg_server_config and collect.collector_state exist, and NOT + gated on TimescaleDB — the scrub works the same on plain PostgreSQL, it just has fewer chunks to + reason about. Drained with the other background startup work below. */ + var settingScrub = RunPgSettingScrubAsync(postgres, stoppingToken); + /* #4214 ruling 9: the once-per-start store host/settings profile log — host facts, pg_settings and the managed conf files only, never the store-size/chunk-total reads --check-settings and the MCP read own. Its own short deadline and its own catch: a bad conf file, a permission problem or a @@ -3077,6 +3084,17 @@ of what this start meant to do. */ } } + /* And the setting scrub (#4348 S1b): a run cut short by shutdown left its marker unwritten (the + marker is only written after every batch completes), so the next start tries again from the top. */ + try + { + await settingScrub; + } + catch (OperationCanceledException) + { + /* Expected on shutdown. */ + } + _logger.LogInformation("PerformanceMonitor Darling collection loop stopped"); } @@ -3414,6 +3432,43 @@ private async Task RunMaterializationHoleRepairAsync(NpgsqlDataSource postgres, } } + /// + /// Runs once (#4348 S1b), concurrently with the rest of startup. + /// Its own connection and its own catch, the same isolation as : + /// a store this cannot reach degrades to whatever plaintext it already had, never to a service that did + /// not start. One INFORMATION line whatever the run found, so "nothing to do" (already scrubbed) is + /// visibly different from "never ran" in the log. + /// + private async Task RunPgSettingScrubAsync(NpgsqlDataSource postgres, CancellationToken stoppingToken) + { + try + { + var summary = await PgSettingScrub.RunAsync(postgres, _logger, stoppingToken); + if (summary.AlreadyDone) + { + _logger.LogInformation("Postgres setting scrub (#4348): already scrubbed at the current rules version — nothing to do."); + } + else + { + _logger.LogInformation( + "Postgres setting scrub (#4348): {Candidates} candidate row(s) read, {Updated} row(s) redacted across {Days} day(s).", + summary.CandidatesRead, summary.RowsUpdated, summary.DaysTouched); + } + } + catch (OperationCanceledException) + { + _logger.LogInformation( + "Postgres setting scrub (#4348) was cancelled before it could report — at shutdown that is expected, and the next start retries from the top because the marker is only written after every batch completes."); + throw; + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + _logger.LogWarning( + "Postgres setting scrub (#4348) could not run — any row an older collector build stored unredacted stays as it is until the next start retries: {Message}", + ex.Message); + } + } + /// /// Moves the sweep gate to concurrent servers (#2170). The gate is built at /// and its width is expressed as how many permits are held OUT of diff --git a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs new file mode 100644 index 0000000000..59d91aec9e --- /dev/null +++ b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs @@ -0,0 +1,337 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Npgsql; +using NpgsqlTypes; +using PerformanceMonitor.Collectors; + +namespace PerformanceMonitor.Darling.Service; + +/// +/// One-time scrub of secrets would now mask but an older collector build +/// stored in plain text — most notably a standby's replication password inside primary_conninfo +/// (#4348 S1b; S1a is the redactor and the collector wiring that stops new rows from ever carrying one). +/// +/// A background job, not a migration. The ruling (issuecomment-5838869407) is explicit: this +/// reads candidate rows with a coarse SQL filter, redacts them in C# with the same +/// the collector uses, and updates only the rows whose text actually +/// changes, by their full key, in small batches. It runs after startup, concurrently with the rest of the +/// startup sequence exactly like DarlingWorker.RunMaterializationHoleRepairAsync and +/// RunBaselineBackfillAsync, and it never holds up collection. +/// +/// The marker reuses collect.collector_state (V44) — the store's general key/value state +/// table — under the fleet-sentinel server_id () and +/// an owner name of its own, exactly the shape PgSelfAlertDeliveryStampStore and +/// QueryStoreBackfill already use for store-wide (not per-server) one-shot state. No migration rung: +/// the table already holds one short row per (server, collector, key). The stored value is +/// as text, so a later rules change — one that would redact a +/// previously-unmasked value differently — is a version bump the marker compares against, and the scrub runs +/// again rather than trusting a stale "done". +/// +/// Batches are grouped by calendar day, and every UPDATE carries that day as a literal range +/// predicate — measured, not assumed (#4348 issue comment). collect.pg_server_config compresses +/// with compress_segmentby = server_id on 1-day chunks. An UPDATE that reaches a compressed chunk only +/// through a JOIN against an unnested key array, with no CONSTANT collection_time predicate the +/// planner can evaluate at plan time, cannot exclude any chunk — confirmed live: a 2-row batch shaped that +/// way decompressed 8,846,250 of about 9.2M tuples and tripped TimescaleDB's +/// timescaledb.max_tuples_decompressed_per_dml_transaction safety limit (default 100,000, error +/// 53400) almost immediately. Adding the day's literal >= / < range alongside the join — redundant +/// with the join equality, but load-bearing — lets TimescaleDB exclude every other chunk, and the same shape +/// of batch then succeeds at once. A day's candidates rarely near (a target +/// with 350 settings has at most 350 candidates on its worst day), but a day is still sub-batched past that +/// cap, so one target's answer never determines the batch size. +/// +/// No explicit recompress step. Measured live on TimescaleDB 2.30.1: after an UPDATE scoped by +/// the day's range predicate, the touched chunk's is_compressed flag stayed true throughout — +/// this build's compressed-chunk DML path decompresses and recompresses the affected segment inline. Forcing +/// a manual compress_chunk afterward would be redundant work, not a correctness requirement; the +/// standing compression policy has nothing left to do. +/// +/// Failure is isolated and retried, never fatal. Any exception — the connection, the candidate +/// read, a batch — is caught, logged once at WARNING, and the marker is left unwritten, so the next service +/// start tries again from the top. A store whose scrub cannot run degrades to the plaintext it already had, +/// never to a service that did not start. +/// +public static class PgSettingScrub +{ + /// The owner name in collect.collector_state, deliberately not a collector definition's — + /// the QueryStoreBackfill / self_alert precedent, so no collector's declared-key read or + /// per-database prune can reach it. + public const string StateCollectorName = "pg_setting_scrub"; + + /// The marker's one key: the the store was last + /// scrubbed under, as text. + public const string RulesVersionStateKey = "rules_version"; + + /// The candidate read's deadline — generous against the measured 14.6 s over a synthetic 9.2M-row + /// / 3-target / 365-day table (#4348 issue comment), because a fleet's real store can hold far more + /// history than that and this runs once, off the collection path. + internal const int CandidateReadTimeoutSeconds = 300; + + /// Each day-batch UPDATE's deadline — small on purpose, since a batch is bounded to one chunk's + /// worth of a day's candidates. + internal const int UpdateBatchTimeoutSeconds = 60; + + /// The marker read/write's deadline — one small keyed row. + internal const int MarkerTimeoutSeconds = 30; + + /// Sub-batch cap WITHIN a single day, so one day whose candidate count is unusually large still + /// keeps each UPDATE's key-array small. See the type remarks for why a day is the outer grouping. + internal const int MaxKeysPerUpdate = 500; + + /// What one run of the scrub found and did, for the caller's summary log line. + public sealed class Summary + { + public static readonly Summary NoOp = new(alreadyDone: true, 0, 0, 0); + + internal Summary(bool alreadyDone, int candidatesRead, int rowsUpdated, int daysTouched) + { + AlreadyDone = alreadyDone; + CandidatesRead = candidatesRead; + RowsUpdated = rowsUpdated; + DaysTouched = daysTouched; + } + + /// True when the marker already named the current + /// and the run did nothing else. + public bool AlreadyDone { get; } + + /// Rows the coarse filter returned (an over-inclusive superset; not all of them changed). + public int CandidatesRead { get; } + + /// Rows actually written, because at least one of setting/boot_val/reset_val + /// differed from its redacted form. + public int RowsUpdated { get; } + + /// Distinct calendar days that needed at least one UPDATE. + public int DaysTouched { get; } + } + + private const string MarkerGetSql = @" +SELECT state_value +FROM collect.collector_state +WHERE server_id = $1 AND collector_name = $2 AND state_key = $3"; + + private const string MarkerUpsertSql = @" +INSERT INTO collect.collector_state (server_id, collector_name, state_key, state_value, updated_at) +VALUES ($1, $2, $3, $4, $5) +ON CONFLICT (server_id, collector_name, state_key) +DO UPDATE SET state_value = EXCLUDED.state_value, updated_at = EXCLUDED.updated_at"; + + /// + /// The coarse filter: a superset of every row could possibly + /// change, cheap enough to run over a compressed hypertable (ILIKE substring tests, no regex). See the + /// type remarks and PgSettingRedactor's own remarks for the rule this mirrors rule-for-rule: + /// a libpq/URI password or an assignment naming PASSWORD/PASSWD/SECRET/TOKEN anywhere in a value; + /// ssl_passphrase_command by name; a dotted extension setting whose name contains one of the + /// whole-value markers. Over-inclusive on purpose — the fine-grained decision is + /// , run per candidate row below. + /// + private const string CandidateSql = @" +SELECT server_id, collection_time, name, database_name, role_name, setting, boot_val, reset_val +FROM collect.pg_server_config +WHERE + setting ILIKE '%password%' OR reset_val ILIKE '%password%' OR boot_val ILIKE '%password%' + OR setting ILIKE '%passwd%' OR reset_val ILIKE '%passwd%' OR boot_val ILIKE '%passwd%' + OR setting ILIKE '%secret%' OR reset_val ILIKE '%secret%' OR boot_val ILIKE '%secret%' + OR setting ILIKE '%token%' OR reset_val ILIKE '%token%' OR boot_val ILIKE '%token%' + OR setting ILIKE '%://%@%' OR reset_val ILIKE '%://%@%' OR boot_val ILIKE '%://%@%' + OR name = 'ssl_passphrase_command' + OR (name LIKE '%.%' AND ( + name ILIKE '%password%' OR name ILIKE '%passwd%' OR name ILIKE '%passphrase%' + OR name ILIKE '%secret%' OR name ILIKE '%salt%' OR name ILIKE '%token%' OR name ILIKE '%key%'))"; + + private const string BatchUpdateSql = @" +WITH batch AS ( + SELECT * FROM unnest($1::int[], $2::timestamp[], $3::text[], $4::text[], $5::text[], $6::text[], $7::text[], $8::text[]) + AS k(server_id, collection_time, name, database_name, role_name, new_setting, new_boot_val, new_reset_val) +) +UPDATE collect.pg_server_config t +SET setting = b.new_setting, boot_val = b.new_boot_val, reset_val = b.new_reset_val +FROM batch b +WHERE t.server_id = b.server_id +AND t.collection_time = b.collection_time +AND t.name = b.name +AND t.database_name IS NOT DISTINCT FROM b.database_name +AND t.role_name IS NOT DISTINCT FROM b.role_name +AND t.collection_time >= $9 AND t.collection_time < $10"; + + private sealed record CandidateRow( + int ServerId, DateTime CollectionTime, string Name, string? DatabaseName, string? RoleName, + string? Setting, string? BootVal, string? ResetVal); + + /// + /// Runs the scrub once against , or returns + /// at once if the marker already names the current rules version. Propagates a failure to the caller, + /// which isolates it exactly like DarlingWorker.RunMaterializationHoleRepairAsync — see the type + /// remarks. + /// + public static async Task RunAsync(NpgsqlDataSource postgres, ILogger? logger, CancellationToken cancellationToken) + { + await using var connection = await postgres.OpenConnectionAsync(cancellationToken); + + var currentVersion = PgSettingRedactor.RulesVersion.ToString(CultureInfo.InvariantCulture); + var marker = await ReadMarkerAsync(connection, cancellationToken); + if (marker == currentVersion) + { + return Summary.NoOp; + } + + var candidates = new List(); + await using (var read = new NpgsqlCommand(CandidateSql, connection) { CommandTimeout = CandidateReadTimeoutSeconds }) + await using (var reader = await read.ExecuteReaderAsync(cancellationToken)) + { + while (await reader.ReadAsync(cancellationToken)) + { + candidates.Add(new CandidateRow( + reader.GetInt32(0), + reader.GetDateTime(1), + reader.GetString(2), + reader.IsDBNull(3) ? null : reader.GetString(3), + reader.IsDBNull(4) ? null : reader.GetString(4), + reader.IsDBNull(5) ? null : reader.GetString(5), + reader.IsDBNull(6) ? null : reader.GetString(6), + reader.IsDBNull(7) ? null : reader.GetString(7))); + } + } + + var byDay = new SortedDictionary>(); + foreach (var c in candidates) + { + var day = c.CollectionTime.Date; + if (!byDay.TryGetValue(day, out var list)) + { + list = new List(); + byDay[day] = list; + } + list.Add(c); + } + + var rowsUpdated = 0; + var daysTouched = 0; + foreach (var (day, dayCandidates) in byDay) + { + var changed = new List(); + var newSettings = new List(); + var newBootVals = new List(); + var newResetVals = new List(); + + foreach (var c in dayCandidates) + { + var newSetting = PgSettingRedactor.Redact(c.Name, c.Setting); + var newBootVal = PgSettingRedactor.Redact(c.Name, c.BootVal); + var newResetVal = PgSettingRedactor.Redact(c.Name, c.ResetVal); + + if (newSetting == c.Setting && newBootVal == c.BootVal && newResetVal == c.ResetVal) + { + continue; + } + + changed.Add(c); + newSettings.Add(newSetting); + newBootVals.Add(newBootVal); + newResetVals.Add(newResetVal); + } + + if (changed.Count == 0) + { + continue; + } + + daysTouched++; + for (var i = 0; i < changed.Count; i += MaxKeysPerUpdate) + { + var take = Math.Min(MaxKeysPerUpdate, changed.Count - i); + rowsUpdated += await RunBatchAsync( + connection, changed, newSettings, newBootVals, newResetVals, i, take, day, cancellationToken); + } + } + + await WriteMarkerAsync(connection, currentVersion, cancellationToken); + + return new Summary(alreadyDone: false, candidates.Count, rowsUpdated, daysTouched); + } + + private static async Task RunBatchAsync( + NpgsqlConnection connection, List changed, List newSettings, List newBootVals, + List newResetVals, int offset, int count, DateTime day, CancellationToken cancellationToken) + { + var serverIds = new int[count]; + var times = new DateTime[count]; + var names = new string[count]; + var databaseNames = new string?[count]; + var roleNames = new string?[count]; + var settings = new string?[count]; + var bootVals = new string?[count]; + var resetVals = new string?[count]; + + for (var j = 0; j < count; j++) + { + var c = changed[offset + j]; + serverIds[j] = c.ServerId; + times[j] = c.CollectionTime; + names[j] = c.Name; + databaseNames[j] = c.DatabaseName; + roleNames[j] = c.RoleName; + settings[j] = newSettings[offset + j]; + bootVals[j] = newBootVals[offset + j]; + resetVals[j] = newResetVals[offset + j]; + } + + await using var update = new NpgsqlCommand(BatchUpdateSql, connection) { CommandTimeout = UpdateBatchTimeoutSeconds }; + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Integer, Value = serverIds }); + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Timestamp, Value = times }); + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = names }); + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = databaseNames }); + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = roleNames }); + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = settings }); + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = bootVals }); + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = resetVals }); + /* The literal day range is what lets TimescaleDB exclude every chunk but this one — see the type + remarks. Redundant with the join equality on collection_time, and load-bearing anyway. */ + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Timestamp, Value = day }); + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Timestamp, Value = day.AddDays(1) }); + + return await update.ExecuteNonQueryAsync(cancellationToken); + } + + private static async Task ReadMarkerAsync(NpgsqlConnection connection, CancellationToken cancellationToken) + { + await using var command = new NpgsqlCommand(MarkerGetSql, connection) { CommandTimeout = MarkerTimeoutSeconds }; + command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Integer, Value = DarlingObservability.FleetServerId }); + command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Text, Value = StateCollectorName }); + command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Text, Value = RulesVersionStateKey }); + + var result = await command.ExecuteScalarAsync(cancellationToken); + return result as string; + } + + private static async Task WriteMarkerAsync(NpgsqlConnection connection, string rulesVersion, CancellationToken cancellationToken) + { + await using var command = new NpgsqlCommand(MarkerUpsertSql, connection) { CommandTimeout = MarkerTimeoutSeconds }; + command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Integer, Value = DarlingObservability.FleetServerId }); + command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Text, Value = StateCollectorName }); + command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Text, Value = RulesVersionStateKey }); + command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Text, Value = rulesVersion }); + command.Parameters.Add(new NpgsqlParameter + { + NpgsqlDbType = NpgsqlDbType.Timestamp, + /* Naive UTC, Kind-Unspecified — the product-wide PG `timestamp` discipline (PgAlertStateStore. + NaiveUtcNow, PgSelfAlertDeliveryStampStore.RecordDeliveredAtUtcAsync). */ + Value = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified), + }); + + await command.ExecuteNonQueryAsync(cancellationToken); + } +} From 74c475b4e4aa7d2f1ad6d4c9e175b284a1c76fc5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 19:27:17 -0400 Subject: [PATCH 04/11] Batch the #4348 scrub by (server, day), correct its docs, and close three small leaks (#4351) Lane 1, the scrub (PgSettingScrub.cs), on ruling comment-5840560804: - H1: a day-only batch join has no constant server_id predicate, so TimescaleDB cannot exclude other servers' rows in that day's chunk and decompresses the whole chunk for every server on the fleet. Past about 11 targets that trips the default timescaledb.max_tuples_decompressed_per_dml_transaction. Group batches by (server, day) and add a literal server_id predicate alongside the day range in the UPDATE. - M1: corrected the "recompresses inline" remark - a touched chunk stays partly compressed (chunk.status = compressed + partial) until the standing compression policy's next run, not immediately. - L4: skip writing the marker (and log a warning) when the updated row count differs from the changed row count, so a systematic key mismatch retries on the next start instead of being marked done with plaintext left. - L5: skip empty ssl_passphrase_command candidate rows; guard a NULL name in the candidate read; use the logger parameter for the L4 warning. - Amendment to the ruling (scrub side only): added '%pass%', '%key%', '%credential%' and '%pwd%' to the three value columns' candidate filter and to the dotted-name filter. RulesVersion stays 1. Lane 2 (PgSettingRedactor.cs) is a separate commit on this branch. --- .../PgSettingScrub.cs | 78 ++++++++++++++----- 1 file changed, 59 insertions(+), 19 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs index 59d91aec9e..27d9c9f641 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs @@ -52,11 +52,14 @@ namespace PerformanceMonitor.Darling.Service; /// with 350 settings has at most 350 candidates on its worst day), but a day is still sub-batched past that /// cap, so one target's answer never determines the batch size. /// -/// No explicit recompress step. Measured live on TimescaleDB 2.30.1: after an UPDATE scoped by -/// the day's range predicate, the touched chunk's is_compressed flag stayed true throughout — -/// this build's compressed-chunk DML path decompresses and recompresses the affected segment inline. Forcing -/// a manual compress_chunk afterward would be redundant work, not a correctness requirement; the -/// standing compression policy has nothing left to do. +/// No explicit recompress step, and the touched chunk stays partly compressed until the standing +/// compression policy next runs. Measured live on TimescaleDB 2.30.1: after an UPDATE scoped by the +/// day's range predicate, the touched chunk's _timescaledb_catalog.chunk.status read "compressed + +/// partial" — the chunk's is_compressed flag stays true, but the rows this batch touched moved +/// into that chunk's uncompressed heap and stay there, growing the store, until the compression policy's +/// next run recompresses them. A manual compress_chunk right after each batch would avoid that window, +/// but the scrub deliberately leaves it to the standing policy rather than adding its own compression step — +/// see the release note below for the space and retention consequence. /// /// Failure is isolated and retried, never fatal. Any exception — the connection, the candidate /// read, a batch — is caught, logged once at WARNING, and the marker is left unwritten, so the next service @@ -147,10 +150,15 @@ OR setting ILIKE '%passwd%' OR reset_val ILIKE '%passwd%' OR boot_val ILIKE '%pa OR setting ILIKE '%secret%' OR reset_val ILIKE '%secret%' OR boot_val ILIKE '%secret%' OR setting ILIKE '%token%' OR reset_val ILIKE '%token%' OR boot_val ILIKE '%token%' OR setting ILIKE '%://%@%' OR reset_val ILIKE '%://%@%' OR boot_val ILIKE '%://%@%' - OR name = 'ssl_passphrase_command' + OR setting ILIKE '%pass%' OR reset_val ILIKE '%pass%' OR boot_val ILIKE '%pass%' + OR setting ILIKE '%key%' OR reset_val ILIKE '%key%' OR boot_val ILIKE '%key%' + OR setting ILIKE '%credential%' OR reset_val ILIKE '%credential%' OR boot_val ILIKE '%credential%' + OR setting ILIKE '%pwd%' OR reset_val ILIKE '%pwd%' OR boot_val ILIKE '%pwd%' + OR (name = 'ssl_passphrase_command' AND (setting <> '' OR boot_val <> '' OR reset_val <> '')) OR (name LIKE '%.%' AND ( name ILIKE '%password%' OR name ILIKE '%passwd%' OR name ILIKE '%passphrase%' - OR name ILIKE '%secret%' OR name ILIKE '%salt%' OR name ILIKE '%token%' OR name ILIKE '%key%'))"; + OR name ILIKE '%secret%' OR name ILIKE '%salt%' OR name ILIKE '%token%' OR name ILIKE '%key%' + OR name ILIKE '%credential%' OR name ILIKE '%pwd%' OR name ILIKE '%pass%'))"; private const string BatchUpdateSql = @" WITH batch AS ( @@ -165,7 +173,8 @@ FROM batch b AND t.name = b.name AND t.database_name IS NOT DISTINCT FROM b.database_name AND t.role_name IS NOT DISTINCT FROM b.role_name -AND t.collection_time >= $9 AND t.collection_time < $10"; +AND t.collection_time >= $9 AND t.collection_time < $10 +AND t.server_id = $11"; private sealed record CandidateRow( int ServerId, DateTime CollectionTime, string Name, string? DatabaseName, string? RoleName, @@ -194,6 +203,14 @@ public static async Task RunAsync(NpgsqlDataSource postgres, ILogger? l { while (await reader.ReadAsync(cancellationToken)) { + if (reader.IsDBNull(2)) + { + /* V102's name column is nullable; a NULL name cannot be redacted (PgSettingRedactor keys + its decision off the name) and cannot be batched (it is a join/predicate key), so skip + it rather than throw and fail the whole scrub at every start (R1 L5). */ + continue; + } + candidates.Add(new CandidateRow( reader.GetInt32(0), reader.GetDateTime(1), @@ -206,21 +223,28 @@ public static async Task RunAsync(NpgsqlDataSource postgres, ILogger? l } } - var byDay = new SortedDictionary>(); +/* Grouped by (server, day), not day alone — see the type remarks' H1 fix. A day-only batch joins many + servers' rows through an unnested array with no constant server_id predicate, so TimescaleDB cannot + exclude any other server's rows in that day's chunk and decompresses the whole chunk for every + server on the fleet; past about 11 targets that blows the default + timescaledb.max_tuples_decompressed_per_dml_transaction. A constant server_id predicate per batch, + alongside the day range, lets the planner exclude everything but this one server's segment. */ + var byServerDay = new SortedDictionary<(int ServerId, DateTime Day), List>(); foreach (var c in candidates) { - var day = c.CollectionTime.Date; - if (!byDay.TryGetValue(day, out var list)) + var key = (c.ServerId, c.CollectionTime.Date); + if (!byServerDay.TryGetValue(key, out var list)) { list = new List(); - byDay[day] = list; + byServerDay[key] = list; } list.Add(c); } var rowsUpdated = 0; - var daysTouched = 0; - foreach (var (day, dayCandidates) in byDay) + var changedRowCount = 0; + var daysTouched = new HashSet(); + foreach (var ((serverId, day), dayCandidates) in byServerDay) { var changed = new List(); var newSettings = new List(); @@ -249,23 +273,36 @@ public static async Task RunAsync(NpgsqlDataSource postgres, ILogger? l continue; } - daysTouched++; + daysTouched.Add(day); + changedRowCount += changed.Count; for (var i = 0; i < changed.Count; i += MaxKeysPerUpdate) { var take = Math.Min(MaxKeysPerUpdate, changed.Count - i); rowsUpdated += await RunBatchAsync( - connection, changed, newSettings, newBootVals, newResetVals, i, take, day, cancellationToken); + connection, changed, newSettings, newBootVals, newResetVals, i, take, day, serverId, cancellationToken); } } - await WriteMarkerAsync(connection, currentVersion, cancellationToken); + if (rowsUpdated == changedRowCount) + { + await WriteMarkerAsync(connection, currentVersion, cancellationToken); + } + else + { + /* R1 L4: a systematic key mismatch would otherwise mark the store scrubbed while plaintext rows + remain. Skipping the marker means the next start re-reads and re-tries; a day dropped by + retention mid-run (benign) just means those rows are gone by then. */ + logger?.LogWarning( + "pg_setting_scrub: updated {RowsUpdated} of {ChangedRowCount} changed rows; leaving the marker unwritten so the next start retries", + rowsUpdated, changedRowCount); + } - return new Summary(alreadyDone: false, candidates.Count, rowsUpdated, daysTouched); + return new Summary(alreadyDone: false, candidates.Count, rowsUpdated, daysTouched.Count); } private static async Task RunBatchAsync( NpgsqlConnection connection, List changed, List newSettings, List newBootVals, - List newResetVals, int offset, int count, DateTime day, CancellationToken cancellationToken) + List newResetVals, int offset, int count, DateTime day, int serverId, CancellationToken cancellationToken) { var serverIds = new int[count]; var times = new DateTime[count]; @@ -302,6 +339,9 @@ private static async Task RunBatchAsync( remarks. Redundant with the join equality on collection_time, and load-bearing anyway. */ update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Timestamp, Value = day }); update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Timestamp, Value = day.AddDays(1) }); + /* The constant server_id predicate is what lets TimescaleDB exclude every other server's segment in + this day's chunk — see the H1 fix note above RunAsync's grouping. */ + update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Integer, Value = serverId }); return await update.ExecuteNonQueryAsync(cancellationToken); } From d2b7ccea421200846cdead8da7dfb3ae078d5cbb Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 19:42:31 -0400 Subject: [PATCH 05/11] Close the redactor's remaining #4348 review-round-1 leaks (M2-M5, L1, L2) M2/M3/M4: an empty-user URI password, a space-separated option (--password/--secret-access-key with no '='), and idempotence for a quoted value glued to trailing punctuation (a ';' or another keyword) are now redacted and stay redacted on a second pass. M5/L1 (ruling amendment on comment-5838869407): the assignment name part becomes PASS|SECRET|TOKEN|CREDENTIAL|PWD|KEY (KEY bounded to its own segment so libpq's sslkey= is untouched), covering WAL-G, Azure and pgBackRest secret variable names. credential and pwd join WholeValueNameMarkers, and a marker in any dot-separated segment of an extension setting's name counts, not only the last. L2: a raw space or NBSP inside a URI password, a quoted value followed by whatever comes after its closing quote, a backslash- newline inside a quote, and an unterminated quote all mask through to the end. Added the classifier pin that a reload's `parameter "..." changed to "..."` line matches no parser and is never stored. Every new rule's test corpus carries both a redaction case and a surviving-neighbour case, plus a second-pass-changes-nothing assertion over the whole corpus. #4351 --- .../Darling.Tests/PgLogEventsPipelineTests.cs | 15 ++++ .../Darling.Tests/PgSettingRedactorTests.cs | 69 +++++++++++++++++-- .../PgSettingRedactor.cs | 63 ++++++++++++----- 3 files changed, 125 insertions(+), 22 deletions(-) diff --git a/Darling/Darling.Tests/PgLogEventsPipelineTests.cs b/Darling/Darling.Tests/PgLogEventsPipelineTests.cs index 441edd56df..aada52cc30 100644 --- a/Darling/Darling.Tests/PgLogEventsPipelineTests.cs +++ b/Darling/Darling.Tests/PgLogEventsPipelineTests.cs @@ -292,6 +292,21 @@ public void TheLockWaitFamily_KeepsPidsModesAndDurations_AndFingerprintsTheState Assert.NotEqual(waits[0].RawLineHash, waits[1].RawLineHash); } + /// #4348's review round 1: a reload's parameter "..." changed to "..." line can carry a + /// setting's live value in plain text — including primary_conninfo's password — but it matches no + /// parser's shape (not a WARNING+, not one of the five recognised LOG shapes), so + /// never emits an event for it and + /// never stores it. The redactor () only ever sees the config-settings + /// collector's rows, never this line, so this pins that the line truly is dropped rather than relying on + /// the redactor to catch it after the fact. + [Fact] + public void ReloadParameterChangedLine_IsNotClassified_AndSoIsNeverStored() + { + var events = Classify(P + "[3001] LOG: parameter \"primary_conninfo\" changed to \"password=hunter2 host=standby\"\n"); + + Assert.Empty(events); + } + [Fact] public void TheRecognisedOnlyFamily_IsStoredUnderItsOwnName_WithNothingLifted() { diff --git a/Darling/Darling.Tests/PgSettingRedactorTests.cs b/Darling/Darling.Tests/PgSettingRedactorTests.cs index 49790d238f..41164e1838 100644 --- a/Darling/Darling.Tests/PgSettingRedactorTests.cs +++ b/Darling/Darling.Tests/PgSettingRedactorTests.cs @@ -72,6 +72,10 @@ public void NullValue_StaysNull() [InlineData("primary_conninfo", "postgresql://alice@host/db?sslmode=require&password=hunter2", "postgresql://alice@host/db?sslmode=require&password=********")] + // M2 (review round 1): an empty user name in the URI's user info still carries a password to libpq. + [InlineData("primary_conninfo", + "postgresql://:hunter2@primary:5432/db", + "postgresql://:********@primary:5432/db")] // assignment / option whose NAME contains PASSWORD, PASSWD, SECRET or TOKEN. [InlineData("archive_command", "PGPASSWORD=hunter2 psql -c 'select 1'", @@ -85,6 +89,40 @@ public void NullValue_StaysNull() [InlineData("restore_command", @"pg_dump --password=""hunter 2"" --host=foo", "pg_dump --password=******** --host=foo")] + // M3 (review round 1): a space-separated option, no '=' at all. + [InlineData("archive_command", + "mycmd --password hunter2 %p", + "mycmd --password ******** %p")] + [InlineData("archive_command", + "aws s3 cp s3://b/%f %p --secret-access-key hunter2secret", + "aws s3 cp s3://b/%f %p --secret-access-key ********")] + [InlineData("archive_command", + "mycmd --no-password -h x %p", + "mycmd --no-password -h x %p")] + // M4 (review round 1): a quoted value glued to a trailing ';' rather than whitespace. + [InlineData("archive_command", + "export PGPASSWORD='hunter 2'; psql", + "export PGPASSWORD=******** psql")] + [InlineData("primary_conninfo", + "password='x'host=y", + "password=********")] + // M5 (ruling amendment): backup-tool secret variable names the assignment list did not cover. + [InlineData("archive_command", + "WALG_LIBSODIUM_KEY=hunter2 wal-g wal-push %p", + "WALG_LIBSODIUM_KEY=******** wal-g wal-push %p")] + [InlineData("archive_command", + "AZURE_STORAGE_ACCESS_KEY=hunter2 wal-g wal-push %p", + "AZURE_STORAGE_ACCESS_KEY=******** wal-g wal-push %p")] + [InlineData("archive_command", + "WALG_PGP_KEY_PASSPHRASE=hunter2 wal-g wal-push %p", + "WALG_PGP_KEY_PASSPHRASE=******** wal-g wal-push %p")] + [InlineData("restore_command", + "PGBACKREST_REPO1_CIPHER_PASS=hunter2 pgbackrest restore", + "PGBACKREST_REPO1_CIPHER_PASS=******** pgbackrest restore")] + // KEY is bounded to its own segment, so libpq's sslkey keyword is never touched. + [InlineData("primary_conninfo", + "sslkey=/path/to/client.key sslmode=require", + "sslkey=/path/to/client.key sslmode=require")] // ssl_passphrase_command: whole value masked; empty stays empty. [InlineData("ssl_passphrase_command", "/usr/bin/cat /etc/ssl/passphrase-hunter2.txt", @@ -92,18 +130,41 @@ public void NullValue_StaysNull() [InlineData("ssl_passphrase_command", "", "")] - // extension setting (dotted name): whole value masked when the LAST segment names a secret. + // extension setting (dotted name): whole value masked when a segment names a secret. [InlineData("anon.salt", "s0mesalt", "********")] [InlineData("myext.api_key", "AKIAABCDEFG", "********")] // the substring decision, pinned: "key" matches wherever it appears in the last segment, not only as // its own word — myext.turkey_interval is not about a key at all, and is still masked in full. [InlineData("myext.turkey_interval", "anything", "********")] - // negative: myext.keep_alive does not contain any marker (password/passwd/passphrase/secret/salt/token/key) - // as a substring of "keep_alive", so its value is left alone. + // negative: myext.keep_alive does not contain any marker (password/passwd/passphrase/secret/salt/token/key/ + // credential/pwd) as a substring of "keep_alive", so its value is left alone. [InlineData("myext.keep_alive", "30s", "30s")] + // L1 (review round 1): "credential" and "pwd" join the whole-value markers, and a marker in ANY + // dot-separated segment counts, not only the last one. + [InlineData("myext.api_credentials", "AKIAABCDEFG", "********")] + [InlineData("app.db_pwd", "hunter2", "********")] + [InlineData("vault.secret.value", "hunter2", "********")] + // L2 (review round 1): edge forms, probe-confirmed. + [InlineData("primary_conninfo", + "postgresql://u:pa S8@h/db", + "postgresql://u:********@h/db")] + [InlineData("restore_command", + "password = S16 host=foo", + "password=******** host=foo")] + [InlineData("restore_command", + "curl -u admin:S22", + "curl -u admin:S22")] + [InlineData("primary_conninfo", + "password='S5 unterminated", + "password=********")] + [InlineData("primary_conninfo", + "password='it\\\nhas a newline' host=foo", + "password=******** host=foo")] // must not change at all. [InlineData("password_encryption", "scram-sha-256", "scram-sha-256")] - [InlineData("unix_socket_directories", "passfile=/x/.pgpass", "passfile=/x/.pgpass")] + // PASS is a deliberately unbounded substring (unlike the bounded KEY segment test), so a libpq + // passfile keyword now over-masks rather than leaking — the ruling's own tradeoff, never a leak. + [InlineData("unix_socket_directories", "passfile=/x/.pgpass", "passfile=********")] [InlineData("primary_conninfo", "host=a user=b", "host=a user=b")] [InlineData("primary_conninfo", "", "")] public void RedactsPerTheRuling_AndIsIdempotent(string name, string value, string expected) diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs index 0889d183dd..b2b26eee44 100644 --- a/PerformanceMonitor.Collectors/PgSettingRedactor.cs +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -61,7 +61,8 @@ public static class PgSettingRedactor private const string Mask = "********"; - /// Names whose value is masked in full when the setting is extension-scoped (#4348). + /// Names whose value is masked in full when the setting is extension-scoped (#4348), or when a + /// marker sits in ANY dot-separated segment of the name, not only the last (review round 1, M5/L1). private static readonly string[] WholeValueNameMarkers = { "password", @@ -71,21 +72,30 @@ public static class PgSettingRedactor "salt", "token", "key", + "credential", + "pwd", }; /// A libpq password/sslpassword keyword inside a conninfo-shaped value. The keyword /// must sit at the start of the string or after whitespace, exactly like every other libpq keyword=value /// pair, so it does not fire on an unrelated keyword such as passfile. The value is either a - /// libpq-quoted string (backslash escapes \\ and \', closing on the first unescaped ') - /// or an unquoted run of non-whitespace. + /// libpq-quoted string (backslash escapes \\ and \', closing on the first unescaped ', + /// or running to the end of the value when the closing quote never arrives — round 1's L2), plus whatever + /// non-whitespace immediately follows the closing quote (round 1's M4: a quoted value glued to a trailing + /// ; or another token with no space), or an unquoted run of non-whitespace. private static readonly Regex LibpqPasswordKeyword = new( - @"(?<=^|\s)(?sslpassword|password)\s*=\s*(?:'(?:\\.|[^'\\])*'|\S*)", + @"(?<=^|\s)(?sslpassword|password)\s*=\s*(?:'(?:\\[\s\S]|[^'\\])*(?:'|$)\S*|\S*)", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); /// The password half of a URI's user info: scheme://user:secret@host. A bare - /// user@host, with no password, does not match and the username is left alone. + /// user@host, with no password, does not match and the username is left alone. The user name may + /// be empty (round 1's M2: postgresql://:secret@host is valid libpq and still carries a + /// password). The password itself may contain a raw space or an NBSP (round 1's L2 — libpq's own + /// isspace is not Unicode-aware, so a literal space can sit inside the password up to the @), + /// so the password class excludes only @, / and the URI's own delimiters, not all + /// whitespace. private static readonly Regex UriUserInfoPassword = new( - @"://(?[^:@/\s]+):[^@/\s]*@", + @"://(?[^:@/\s\u00A0]*):[^@/]*@", RegexOptions.Compiled | RegexOptions.CultureInvariant); /// A password query parameter in a URI's query string. @@ -93,13 +103,29 @@ public static class PgSettingRedactor @"(?<=[?&])(?password)=[^&#\s'""]*", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); - /// A shell-style or option-style assignment (NAME=value, --name=value) whose name - /// contains PASSWORD, PASSWD, SECRET or TOKEN. Covers PGPASSWORD=x, AWS_SECRET_ACCESS_KEY=x, - /// --password=x inside archive_command/restore_command-shaped values. The value side - /// accepts the same libpq-style quoting as , plus double quotes, so a - /// quoted value with an = or space inside it is not mistaken for the start of the next token. + /// A shell-style or option-style assignment (NAME=value) whose name contains PASSWORD, + /// PASSWD, PASSPHRASE, SECRET, TOKEN, CREDENTIAL, PWD, or a standalone KEY segment (round 1's M5 + /// amendment: common WAL-G/Azure/pgBackRest variable names such as WALG_LIBSODIUM_KEY and + /// AZURE_STORAGE_ACCESS_KEY). PASS covers PASSWORD, PASSWD and PASSPHRASE as substrings; KEY + /// is bounded on both sides by (?<![A-Za-z0-9])/(?![A-Za-z0-9]) so it matches only as its + /// own word or delimited segment, never as a substring — which is what keeps libpq's sslkey=/path + /// unmasked. Covers PGPASSWORD=x, AWS_SECRET_ACCESS_KEY=x, WALG_PGP_KEY_PASSPHRASE=x + /// inside archive_command/restore_command-shaped values. The value side accepts the same + /// libpq-style quoting as , plus double quotes, plus whatever + /// non-whitespace immediately follows the closing quote (round 1's M4), so a quoted value with an + /// =, space, or trailing punctuation inside or after it is not mistaken for the start of the next + /// token. private static readonly Regex AssignmentSecretName = new( - @"(?[\w.-]*(?:PASSWORD|PASSWD|SECRET|TOKEN)[\w.-]*)=(?:""(?:\\.|[^""\\])*""|'(?:\\.|[^'\\])*'|\S*)", + @"(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?A space-separated option whose name contains PASSWORD, PASSWD, SECRET or TOKEN, taking its + /// value from the next whitespace-delimited token rather than an = (round 1's M3: --password + /// hunter2 and --secret-access-key hunter2 have no = at all, so + /// never fires on them). (?!-) keeps a value-less flag such as + /// --no-password -h x from swallowing the next option as its value. + private static readonly Regex OptionSecretSpaced = new( + @"(?<=^|\s)(?--?[\w.-]*(?:PASSWORD|PASSWD|SECRET|TOKEN)[\w.-]*)\s+(?!-)(?:""(?:\\[\s\S]|[^""\\])*(?:""|$)\S*|'(?:\\[\s\S]|[^'\\])*(?:'|$)\S*|\S+)", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); /// @@ -123,6 +149,7 @@ public static class PgSettingRedactor redacted = UriUserInfoPassword.Replace(redacted, static m => "://" + m.Groups["user"].Value + ":" + Mask + "@"); redacted = UriQueryPassword.Replace(redacted, static m => m.Groups["key"].Value + "=" + Mask); redacted = AssignmentSecretName.Replace(redacted, static m => m.Groups["name"].Value + "=" + Mask); + redacted = OptionSecretSpaced.Replace(redacted, static m => m.Groups["opt"].Value + " " + Mask); return redacted; } @@ -140,18 +167,18 @@ private static bool IsWholeValueMasked(string? name) } // Extension-scoped settings only: a bare core GUC such as password_encryption never matches here, - // no matter what its name contains — only the part after the LAST dot is tested, and only when - // there is a dot at all. - var dot = name.LastIndexOf('.'); - if (dot < 0 || dot == name.Length - 1) + // no matter what its name contains — only a dotted name is tested at all. Round 1's L1: a marker in + // ANY dot-separated segment counts, not only the last one (vault.secret.value), so the whole name is + // tested as a substring rather than slicing off just the last part. + var dot = name.IndexOf('.'); + if (dot < 0) { return false; } - var lastPart = name.AsSpan(dot + 1); foreach (var marker in WholeValueNameMarkers) { - if (lastPart.Contains(marker, StringComparison.OrdinalIgnoreCase)) + if (name.Contains(marker, StringComparison.OrdinalIgnoreCase)) { return true; } From bd483dcaff48fee4c5789f6c93e42beef818e8b5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 20:38:23 -0400 Subject: [PATCH 06/11] M1: extend OptionSecretSpaced name part to match AssignmentSecretName (#4351) --?[\w.-]*(?:PASSWORD|PASSWD|SECRET|TOKEN) missed gpg --passphrase, openssl -pass, --encryption-key, --credentials. Swap in the same name part the assignment path uses: PASS|SECRET|TOKEN|CREDENTIAL|PWD|standalone KEY. Adds redacting tests for those four inputs plus a --no-password negative. --- Darling/Darling.Tests/PgSettingRedactorTests.cs | 13 +++++++++++++ PerformanceMonitor.Collectors/PgSettingRedactor.cs | 11 +++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/Darling/Darling.Tests/PgSettingRedactorTests.cs b/Darling/Darling.Tests/PgSettingRedactorTests.cs index 41164e1838..ab86bd287b 100644 --- a/Darling/Darling.Tests/PgSettingRedactorTests.cs +++ b/Darling/Darling.Tests/PgSettingRedactorTests.cs @@ -96,6 +96,19 @@ public void NullValue_StaysNull() [InlineData("archive_command", "aws s3 cp s3://b/%f %p --secret-access-key hunter2secret", "aws s3 cp s3://b/%f %p --secret-access-key ********")] + // M1 (review round 2): the spaced-option path had fallen behind the assignment path's name list. + [InlineData("archive_command", + "gpg --passphrase S1 --decrypt %p", + "gpg --passphrase ******** --decrypt %p")] + [InlineData("archive_command", + "openssl enc -pass pass:S3 -d", + "openssl enc -pass ******** -d")] + [InlineData("archive_command", + "mycmd --encryption-key S4 %p", + "mycmd --encryption-key ******** %p")] + [InlineData("archive_command", + "mycmd --credentials S5 %p", + "mycmd --credentials ******** %p")] [InlineData("archive_command", "mycmd --no-password -h x %p", "mycmd --no-password -h x %p")] diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs index b2b26eee44..29b6c208ed 100644 --- a/PerformanceMonitor.Collectors/PgSettingRedactor.cs +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -119,13 +119,16 @@ public static class PgSettingRedactor @"(?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?A space-separated option whose name contains PASSWORD, PASSWD, SECRET or TOKEN, taking its - /// value from the next whitespace-delimited token rather than an = (round 1's M3: --password - /// hunter2 and --secret-access-key hunter2 have no = at all, so + /// A space-separated option whose name contains PASS, SECRET, TOKEN, CREDENTIAL, PWD, or a + /// standalone KEY segment -- the same name part as (review round 2's + /// M1: the option path had fallen behind the assignment path's name list, so gpg --passphrase x, + /// openssl enc -pass pass:x, --encryption-key x and --credentials x still leaked), + /// taking its value from the next whitespace-delimited token rather than an = (round 1's M3: + /// --password hunter2 and --secret-access-key hunter2 have no = at all, so /// never fires on them). (?!-) keeps a value-less flag such as /// --no-password -h x from swallowing the next option as its value. private static readonly Regex OptionSecretSpaced = new( - @"(?<=^|\s)(?--?[\w.-]*(?:PASSWORD|PASSWD|SECRET|TOKEN)[\w.-]*)\s+(?!-)(?:""(?:\\[\s\S]|[^""\\])*(?:""|$)\S*|'(?:\\[\s\S]|[^'\\])*(?:'|$)\S*|\S+)", + @"(?<=^|\s)(?--?[\w.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(? From 6a4bbd53e837e2299174c03604e7c021b007e9b0 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 20:40:26 -0400 Subject: [PATCH 07/11] L1: fix doc comments that drifted from the code (#4351) Redactor: the option/assignment name list is PASS/SECRET/TOKEN/CREDENTIAL/ PWD/KEY on both paths, not PASSWORD/PASSWD/SECRET/TOKEN; the dotted-name rule tests ANY segment, not only the last; passfile is masked by the assignment regex's PASS substring, not the libpq keyword regex. Scrub: batches group by (server, day) since H1, not day alone; the candidate SQL comment lists the current name set; the missing release note pointer now points at the CHANGELOG entry. --- .../PgSettingScrub.cs | 33 ++++++++++--------- .../PgSettingRedactor.cs | 33 +++++++++++-------- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs index 27d9c9f641..19057dc2dd 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs @@ -39,18 +39,21 @@ namespace PerformanceMonitor.Darling.Service; /// previously-unmasked value differently — is a version bump the marker compares against, and the scrub runs /// again rather than trusting a stale "done". /// -/// Batches are grouped by calendar day, and every UPDATE carries that day as a literal range -/// predicate — measured, not assumed (#4348 issue comment). collect.pg_server_config compresses -/// with compress_segmentby = server_id on 1-day chunks. An UPDATE that reaches a compressed chunk only -/// through a JOIN against an unnested key array, with no CONSTANT collection_time predicate the -/// planner can evaluate at plan time, cannot exclude any chunk — confirmed live: a 2-row batch shaped that -/// way decompressed 8,846,250 of about 9.2M tuples and tripped TimescaleDB's -/// timescaledb.max_tuples_decompressed_per_dml_transaction safety limit (default 100,000, error -/// 53400) almost immediately. Adding the day's literal >= / < range alongside the join — redundant -/// with the join equality, but load-bearing — lets TimescaleDB exclude every other chunk, and the same shape -/// of batch then succeeds at once. A day's candidates rarely near (a target -/// with 350 settings has at most 350 candidates on its worst day), but a day is still sub-batched past that -/// cap, so one target's answer never determines the batch size. +/// Batches are grouped by (server, day), and every UPDATE carries both as literal predicates — +/// measured, not assumed (#4348 issue comment; the server predicate is round 2's H1 fix). +/// collect.pg_server_config compresses with compress_segmentby = server_id on 1-day chunks. An +/// UPDATE that reaches a compressed chunk only through a JOIN against an unnested key array, with no CONSTANT +/// collection_time/server_id predicate the planner can evaluate at plan time, cannot exclude any +/// chunk or segment — confirmed live: a 2-row batch shaped that way decompressed 8,846,250 of about 9.2M +/// tuples and tripped TimescaleDB's timescaledb.max_tuples_decompressed_per_dml_transaction safety +/// limit (default 100,000, error 53400) almost immediately; day-only grouping repeated the same failure once +/// a fleet passed about 11 targets, because every target's rows for that day still shared the chunk. Adding +/// the day's literal >= / < range AND a constant server_id predicate alongside the join — +/// redundant with the join equality, but load-bearing — lets TimescaleDB exclude every other chunk and every +/// other server's segment, and the same shape of batch then succeeds at once. A target's candidates rarely +/// near (a target with 350 settings has at most 350 candidates on its worst +/// day), but one (server, day) group is still sub-batched past that cap, so one target's answer never +/// determines the batch size. /// /// No explicit recompress step, and the touched chunk stays partly compressed until the standing /// compression policy next runs. Measured live on TimescaleDB 2.30.1: after an UPDATE scoped by the @@ -59,7 +62,7 @@ namespace PerformanceMonitor.Darling.Service; /// into that chunk's uncompressed heap and stay there, growing the store, until the compression policy's /// next run recompresses them. A manual compress_chunk right after each batch would avoid that window, /// but the scrub deliberately leaves it to the standing policy rather than adding its own compression step — -/// see the release note below for the space and retention consequence. +/// see the CHANGELOG entry for #4351 for the space and retention consequence. /// /// Failure is isolated and retried, never fatal. Any exception — the connection, the candidate /// read, a batch — is caught, logged once at WARNING, and the marker is left unwritten, so the next service @@ -136,8 +139,8 @@ ON CONFLICT (server_id, collector_name, state_key) /// The coarse filter: a superset of every row could possibly /// change, cheap enough to run over a compressed hypertable (ILIKE substring tests, no regex). See the /// type remarks and PgSettingRedactor's own remarks for the rule this mirrors rule-for-rule: - /// a libpq/URI password or an assignment naming PASSWORD/PASSWD/SECRET/TOKEN anywhere in a value; - /// ssl_passphrase_command by name; a dotted extension setting whose name contains one of the + /// a libpq/URI password or an assignment/option naming PASS/SECRET/TOKEN/CREDENTIAL/PWD/KEY anywhere in a + /// value; ssl_passphrase_command by name; a dotted extension setting whose name contains one of the /// whole-value markers. Over-inclusive on purpose — the fine-grained decision is /// , run per candidate row below. /// diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs index 29b6c208ed..13c007ce62 100644 --- a/PerformanceMonitor.Collectors/PgSettingRedactor.cs +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -27,19 +27,23 @@ namespace PerformanceMonitor.Collectors; /// quoting and \\ / \' escapes; /// the password in a URI's user info (scheme://user:secret@host), and a password query /// parameter; -/// the value of an assignment or option whose name contains PASSWORD, PASSWD, SECRET -/// or TOKEN (case-insensitive), such as PGPASSWORD=... or --password=.... +/// the value of an assignment or option whose name contains PASS, SECRET, TOKEN, +/// CREDENTIAL, PWD, or a standalone KEY segment (case-insensitive), such as +/// PGPASSWORD=..., --password=..., --passphrase ... or --encryption-key ... — the +/// same name part on both the =-assignment path and the space-separated option path. /// /// Host, port, user, dbname and application_name are never touched. /// ssl_passphrase_command has its whole value masked (an empty value stays empty — there is /// nothing to mask). /// An extension setting — a name with a dot, such as anon.salt — has its whole value masked when -/// the part of the name AFTER THE LAST DOT contains (as a substring, case-insensitive) password, -/// passwd, passphrase, secret, salt, token or key. This is a plain -/// substring test, not a whole-word one: myext.api_key matches (ends in key) and so would a name -/// that merely happens to contain those letters together, such as myext.turkey_interval — the false -/// positive it can produce only over-masks, never leaks, and myext.keep_alive is the negative case that -/// proves it (no marker word is a substring of keep_alive). +/// ANY dot-separated segment of the name contains (as a substring, case-insensitive) password, +/// passwd, passphrase, secret, salt, token, key, credential or +/// pwd — not only the last segment (round 1's L1: vault.secret.value matches on the middle +/// segment). This is a plain substring test, not a whole-word one: myext.api_key matches (ends in +/// key) and so would a name that merely happens to contain those letters together, such as +/// myext.turkey_interval — the false positive it can produce only over-masks, never leaks, and +/// myext.keep_alive is the negative case that proves it (no marker word is a substring of +/// keep_alive). /// Nothing else changes: a core setting whose name merely contains one of those words, such as /// password_encryption, keeps its value — only the DOTTED, extension-scoped form triggers a whole-value /// mask by name. @@ -78,11 +82,14 @@ public static class PgSettingRedactor /// A libpq password/sslpassword keyword inside a conninfo-shaped value. The keyword /// must sit at the start of the string or after whitespace, exactly like every other libpq keyword=value - /// pair, so it does not fire on an unrelated keyword such as passfile. The value is either a - /// libpq-quoted string (backslash escapes \\ and \', closing on the first unescaped ', - /// or running to the end of the value when the closing quote never arrives — round 1's L2), plus whatever - /// non-whitespace immediately follows the closing quote (round 1's M4: a quoted value glued to a trailing - /// ; or another token with no space), or an unquoted run of non-whitespace. + /// pair, so THIS regex does not fire on an unrelated keyword such as passfile — but + /// 's broader name part (round 2's M1) treats PASS as a + /// substring, so passfile=/x/.pgpass is still masked whole, by that regex, not this one; it is an + /// over-mask the ruling accepts, never a leak. The value is either a libpq-quoted string (backslash + /// escapes \\ and \', closing on the first unescaped ', or running to the end of the + /// value when the closing quote never arrives — round 1's L2), plus whatever non-whitespace immediately + /// follows the closing quote (round 1's M4: a quoted value glued to a trailing ; or another token + /// with no space), or an unquoted run of non-whitespace. private static readonly Regex LibpqPasswordKeyword = new( @"(?<=^|\s)(?sslpassword|password)\s*=\s*(?:'(?:\\[\s\S]|[^'\\])*(?:'|$)\S*|\S*)", RegexOptions.IgnoreCase | RegexOptions.Compiled | RegexOptions.CultureInvariant); From d574f45684b4e548eef29469cd7de06b03a74574 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 20:44:59 -0400 Subject: [PATCH 08/11] M2: pin H1's (server, day) batching with a live test (#4351) Two servers on one day, in one compressed chunk, with Options=-c timescaledb.max_tuples_decompressed_per_dml_transaction=1 (one target's own row count). RunAsync must update both rows without tripping 53400. Verified RED against the pre-H1 day-only-batch shape (checked out from 8faac396, ran, restored) and GREEN at head; docker timescale/timescaledb:2.30.1-pg18 on 55451, container removed after. --- .../Darling.Tests/PgSettingScrubLiveTests.cs | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs index c66b710a46..7a514ec335 100644 --- a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs +++ b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs @@ -20,13 +20,78 @@ namespace Darling.Tests; /// /// Live, end-to-end proof of the #4348 S1b one-time scrub against a REAL compressed TimescaleDB chunk. /// -/// #1776 own-store — mints its own scratch database () rather than +/// #4348 own-store — mints its own scratch database () rather than /// sharing the live fixture, so it is deliberately NOT in the live-postgres collection: it creates its /// own hypertable/compression shape on collect.pg_server_config, which the shared fixture must never /// inherit from a test. /// public sealed class PgSettingScrubLiveTests { + /// + /// Pins round 2's H1 fix: batches are grouped by (server, day), not day alone, and every UPDATE carries a + /// constant server_id predicate alongside the day range — not just the join equality — so + /// TimescaleDB can exclude every other server's compressed segment in that day's chunk. Two servers each + /// get a secret-bearing row on the SAME day, in the SAME compressed chunk. The connection's + /// Options=-c timescaledb.max_tuples_decompressed_per_dml_transaction=N is set to one target's row + /// count plus one: a day-only batch (the pre-H1 shape) would decompress BOTH servers' rows in the shared + /// chunk and trip the limit with 53400; the (server, day) batch decompresses only the one server's + /// segment and stays under it. + /// + [Fact] + public async Task TheScrubBatchesPerServerAndDay_NotDayAlone() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), + "Set DARLING_TEST_PG to a Postgres connection string (with TimescaleDB installed) to run the live #4351 H1 batching pin (it mints its own scratch database)."); + + var ct = TestContext.Current.CancellationToken; + + await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + + /* Seed and shape the chunk using an UNCAPPED connection: capping max_tuples_decompressed at + connection scope would also block the seeding compress_chunk() call itself. */ + await using (var setupConnection = new NpgsqlConnection(scratch.ConnectionString)) + { + await setupConnection.OpenAsync(ct); + await PgMigrations.MigrateAsync(setupConnection, ct); + + Assert.True(await TimescaleSupport.TryEnableAsync(setupConnection, null, ct), + "the dev fixture is expected to have TimescaleDB installed"); + await ExecAsync(setupConnection, "SELECT create_hypertable('collect.pg_server_config', by_range('collection_time', INTERVAL '1 days'), if_not_exists => true)", ct); + await ExecAsync(setupConnection, "ALTER TABLE collect.pg_server_config SET (timescaledb.compress, timescaledb.compress_segmentby = 'server_id')", ct); + + const int serverA = -444445; + const int serverB = -444446; + + /* One secret-bearing row per server, same day, same chunk. */ + await InsertRowAsync(setupConnection, serverA, OldTime, "primary_conninfo", Secret, databaseName: null, roleName: null, ct); + await InsertRowAsync(setupConnection, serverB, OldTime, "primary_conninfo", Secret, databaseName: null, roleName: null, ct); + + await ExecAsync(setupConnection, "SELECT count(compress_chunk(c, if_not_compressed => true)) FROM show_chunks('collect.pg_server_config') c", ct); + + Assert.True(await ContainsSecretAsync(setupConnection, ct), "seeding failed to plant the secret this test exists to catch"); + } + + /* One target's own day has 1 candidate row. Cap at 1 (target-row-count) so a day-only batch, which + would decompress both servers' rows sharing this chunk, trips 53400 with room to spare; a + (server, day) batch touches only 1 row and stays at the cap. */ + var cappedConnectionString = new NpgsqlConnectionStringBuilder(scratch.ConnectionString) + { + Options = "-c timescaledb.max_tuples_decompressed_per_dml_transaction=1", + }.ConnectionString; + + await using var postgres = NpgsqlDataSource.Create(cappedConnectionString); + + var summary = await PgSettingScrub.RunAsync(postgres, logger: null, ct); + + Assert.False(summary.AlreadyDone); + Assert.Equal(2, summary.RowsUpdated); + + await using var verifyConnection = new NpgsqlConnection(scratch.ConnectionString); + await verifyConnection.OpenAsync(ct); + Assert.False(await ContainsSecretAsync(verifyConnection, ct), "the raw password survived the scrub under a tight decompress cap"); + } + private const int ServerId = -444444; private const string ServerName = "darling-setting-scrub-e2e"; private const string Secret = "host=replica1.internal port=5432 user=replicator password=hunter2 sslmode=require"; From 58c2f68099847090c419e0b3dd7726fefc2f9aef Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:03:22 -0400 Subject: [PATCH 09/11] 4351: M3 batch-safe decompress + per-server retry, L3 non-secret allowlist M3(a): SET LOCAL timescaledb.max_tuples_decompressed_per_dml_transaction = 0 inside each batch's own transaction, keeping H1's (literal server + literal day range) UPDATE shape. Confirmed live on TimescaleDB 2.30.1 that the GUC's pg_settings context is 'user', so the store's own non-superuser role may set it. M3(b): failures are now caught per server; a failing server's remaining days are skipped for the rest of the run, the other servers still get scrubbed, and the marker is withheld unless every server passed. Only the server id and SQLSTATE are logged, never the exception text. Live pins added: one target's one day exceeding the decompress limit completes; a forced per-server failure (real constraint violation) leaves the other target scrubbed and the marker unset, and a retried run after the cause is removed completes and sets it. L3: added an explicit case-insensitive allowlist of non-secret rds./passwordcheck. settings that would otherwise be masked whole by the dotted-name rule, with pins for each name plus a negative proving a real secret next to them is still masked. L4: grepped the collector/service tree for a durable buffer, spool, or outbox around pg_server_config writes; none exists. No migration needed. --- .../Darling.Tests/PgSettingRedactorTests.cs | 21 +++ .../Darling.Tests/PgSettingScrubLiveTests.cs | 135 ++++++++++++++++++ .../PgSettingScrub.cs | 79 ++++++++-- .../PgSettingRedactor.cs | 20 +++ 4 files changed, 247 insertions(+), 8 deletions(-) diff --git a/Darling/Darling.Tests/PgSettingRedactorTests.cs b/Darling/Darling.Tests/PgSettingRedactorTests.cs index ab86bd287b..194831b94b 100644 --- a/Darling/Darling.Tests/PgSettingRedactorTests.cs +++ b/Darling/Darling.Tests/PgSettingRedactorTests.cs @@ -217,4 +217,25 @@ public void NeverThrows_OnNullName() Assert.Equal("password=********", result); } + + // L3, round 2: these three names are password POLICY settings, not secrets — the allowlist keeps them + // unmasked even though "password" sits in the dotted name. + [Theory] + [InlineData("rds.accepted_password_auth_method", "md5+password")] + [InlineData("RDS.ACCEPTED_PASSWORD_AUTH_METHOD", "md5+password")] + [InlineData("rds.restrict_password_commands", "on")] + [InlineData("passwordcheck.min_password_length", "8")] + public void AllowlistedPolicyNames_AreNotMasked(string name, string value) + { + Assert.Equal(value, PgSettingRedactor.Redact(name, value)); + } + + // L3 negative: a REAL secret next to an allowlisted name in the same batch is still masked — the + // allowlist is name-exact, not a blanket "don't touch anything dotted with .password. in it" escape. + [Fact] + public void AllowlistedPolicyName_DoesNotShieldARealSecretElsewhere() + { + Assert.Equal("md5+password", PgSettingRedactor.Redact("rds.accepted_password_auth_method", "md5+password")); + Assert.Equal("********", PgSettingRedactor.Redact("app.db_password", "fake-secret-value")); + } } diff --git a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs index 7a514ec335..633faa2ab0 100644 --- a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs +++ b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs @@ -92,6 +92,141 @@ connection scope would also block the seeding compress_chunk() call itself. */ Assert.False(await ContainsSecretAsync(verifyConnection, ct), "the raw password survived the scrub under a tight decompress cap"); } + /// + /// M3 (a): a single target's single day exceeds the decompress limit on its own — no cross-server + /// sharing needed. 200 secret-bearing rows for ONE server on ONE day, capped at + /// max_tuples_decompressed_per_dml_transaction=50 (well under 200, and also under + /// so the cap alone is what would fail without the SET + /// LOCAL override). Before the fix this trips 53400 on the very first batch of its own day; after the + /// fix, SET LOCAL = 0 inside each batch's transaction lets it complete regardless of the cap. + /// + [Fact] + public async Task TheScrubCompletes_WhenOneTargetsOneDayExceedsTheDecompressLimit() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), + "Set DARLING_TEST_PG to a Postgres connection string (with TimescaleDB installed) to run the live #4351 M3(a) decompress-limit pin (it mints its own scratch database)."); + + var ct = TestContext.Current.CancellationToken; + + await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + + const int serverId = -444447; + const int rowCount = 200; + + await using (var setupConnection = new NpgsqlConnection(scratch.ConnectionString)) + { + await setupConnection.OpenAsync(ct); + await PgMigrations.MigrateAsync(setupConnection, ct); + + Assert.True(await TimescaleSupport.TryEnableAsync(setupConnection, null, ct), + "the dev fixture is expected to have TimescaleDB installed"); + await ExecAsync(setupConnection, "SELECT create_hypertable('collect.pg_server_config', by_range('collection_time', INTERVAL '1 days'), if_not_exists => true)", ct); + await ExecAsync(setupConnection, "ALTER TABLE collect.pg_server_config SET (timescaledb.compress, timescaledb.compress_segmentby = 'server_id')", ct); + + for (var i = 0; i < rowCount; i++) + { + await InsertRowAsync(setupConnection, serverId, OldTime, $"custom.setting_{i}", Secret, databaseName: null, roleName: null, ct); + } + + await ExecAsync(setupConnection, "SELECT count(compress_chunk(c, if_not_compressed => true)) FROM show_chunks('collect.pg_server_config') c", ct); + + Assert.True(await ContainsSecretAsync(setupConnection, ct), "seeding failed to plant the secret this test exists to catch"); + } + + var cappedConnectionString = new NpgsqlConnectionStringBuilder(scratch.ConnectionString) + { + Options = "-c timescaledb.max_tuples_decompressed_per_dml_transaction=50", + }.ConnectionString; + + await using var postgres = NpgsqlDataSource.Create(cappedConnectionString); + + var summary = await PgSettingScrub.RunAsync(postgres, logger: null, ct); + + Assert.False(summary.AlreadyDone); + Assert.Equal(rowCount, summary.RowsUpdated); + + await using var verifyConnection = new NpgsqlConnection(scratch.ConnectionString); + await verifyConnection.OpenAsync(ct); + Assert.False(await ContainsSecretAsync(verifyConnection, ct), "the raw password survived the scrub under a tight decompress cap on a single target's single day"); + } + + /// + /// M3 (b): a forced failure on one target leaves the OTHER targets scrubbed and the marker unset; after + /// the cause is removed, the next run completes and sets it. The failure is injected through a real + /// database error the scrub's own UPDATE would hit — a CHECK constraint that rejects only + /// badServer's redacted value, so the scrub's catch-per-server path sees an honest 23514 from + /// Postgres, not a mocked exception. Dropping the constraint after the first run is "the cause removed"; + /// the second run then completes badServer too and sets the marker. + /// + [Fact] + public async Task TheScrub_IsolatesAFailingServer_AndRetriesOnTheNextRun() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), + "Set DARLING_TEST_PG to a Postgres connection string (with TimescaleDB installed) to run the live #4351 M3(b) per-server isolation pin (it mints its own scratch database)."); + + var ct = TestContext.Current.CancellationToken; + + await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + + const int goodServer = -444448; + const int badServer = -444449; + + await using var setupConnection = new NpgsqlConnection(scratch.ConnectionString); + await setupConnection.OpenAsync(ct); + await PgMigrations.MigrateAsync(setupConnection, ct); + + Assert.True(await TimescaleSupport.TryEnableAsync(setupConnection, null, ct), + "the dev fixture is expected to have TimescaleDB installed"); + await ExecAsync(setupConnection, "SELECT create_hypertable('collect.pg_server_config', by_range('collection_time', INTERVAL '1 days'), if_not_exists => true)", ct); + + /* Rejects only badServer's post-redaction row — an honest constraint violation (23514) the scrub's + batch UPDATE actually hits, standing in for any real per-row failure without touching the + product's own logic. */ + await ExecAsync(setupConnection, + $"ALTER TABLE collect.pg_server_config ADD CONSTRAINT chk_4351_forced_failure CHECK (server_id <> {badServer} OR setting LIKE '%hunter2%')", ct); + + await ExecAsync(setupConnection, "ALTER TABLE collect.pg_server_config SET (timescaledb.compress, timescaledb.compress_segmentby = 'server_id')", ct); + + await InsertRowAsync(setupConnection, goodServer, OldTime, "primary_conninfo", Secret, databaseName: null, roleName: null, ct); + await InsertRowAsync(setupConnection, badServer, OldTime, "primary_conninfo", Secret, databaseName: null, roleName: null, ct); + + await ExecAsync(setupConnection, "SELECT count(compress_chunk(c, if_not_compressed => true)) FROM show_chunks('collect.pg_server_config') c", ct); + Assert.True(await ContainsSecretAsync(setupConnection, ct), "seeding failed to plant the secret this test exists to catch"); + + await using var postgres = NpgsqlDataSource.Create(scratch.ConnectionString); + + var first = await PgSettingScrub.RunAsync(postgres, logger: null, ct); + Assert.False(first.AlreadyDone); + + // goodServer's row is scrubbed even though badServer's failed; the marker is withheld. + var goodValue = await ScalarTextAsync(setupConnection, + "SELECT setting FROM collect.pg_server_config WHERE server_id = $1 AND collection_time = $2 AND name = 'primary_conninfo'", + goodServer, OldTime, ct); + Assert.NotNull(goodValue); + Assert.DoesNotContain("hunter2", goodValue); + + var markerValue = await ScalarTextAsync(setupConnection, + "SELECT state_value FROM collect.collector_state WHERE collector_name = 'pg_setting_scrub' AND state_key = 'rules_version'", + 0, DateTime.MinValue, ct); + Assert.Null(markerValue); + + var stillSecretBad = await ScalarTextAsync(setupConnection, + "SELECT setting FROM collect.pg_server_config WHERE server_id = $1 AND collection_time = $2 AND name = 'primary_conninfo'", + badServer, OldTime, ct); + Assert.Contains("hunter2", stillSecretBad); + + // Remove the cause, then the next run completes badServer too and sets the marker. + await ExecAsync(setupConnection, "ALTER TABLE collect.pg_server_config DROP CONSTRAINT chk_4351_forced_failure", ct); + + var second = await PgSettingScrub.RunAsync(postgres, logger: null, ct); + Assert.False(second.AlreadyDone); + Assert.False(await ContainsSecretAsync(setupConnection, ct), "the raw password survived the retried run"); + } + + private const string Mask = "********"; + private const int ServerId = -444444; private const string ServerName = "darling-setting-scrub-e2e"; private const string Secret = "host=replica1.internal port=5432 user=replicator password=hunter2 sslmode=require"; diff --git a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs index 19057dc2dd..b3029ec2d0 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs @@ -247,8 +247,26 @@ public static async Task RunAsync(NpgsqlDataSource postgres, ILogger? l var rowsUpdated = 0; var changedRowCount = 0; var daysTouched = new HashSet(); + var failedServerIds = new SortedSet(); + int? currentServerId = null; + var currentServerFailed = false; foreach (var ((serverId, day), dayCandidates) in byServerDay) { + /* M3 (b): catch per server and continue — see the type remarks. Once this server's batch has + failed, skip its remaining days this run (they share the same failure mode) and move on to the + next server; the marker withheld below (rowsUpdated < changedRowCount) makes the WHOLE run + retry every server, including ones that already succeeded, on the next start — cheap and + correct, since a server whose rows are already redacted contributes nothing on a re-run. */ + if (serverId != currentServerId) + { + currentServerId = serverId; + currentServerFailed = false; + } + else if (currentServerFailed) + { + continue; + } + var changed = new List(); var newSettings = new List(); var newBootVals = new List(); @@ -276,20 +294,45 @@ public static async Task RunAsync(NpgsqlDataSource postgres, ILogger? l continue; } - daysTouched.Add(day); changedRowCount += changed.Count; - for (var i = 0; i < changed.Count; i += MaxKeysPerUpdate) + var dayUpdated = 0; + try + { + for (var i = 0; i < changed.Count; i += MaxKeysPerUpdate) + { + var take = Math.Min(MaxKeysPerUpdate, changed.Count - i); + dayUpdated += await RunBatchAsync( + connection, changed, newSettings, newBootVals, newResetVals, i, take, day, serverId, cancellationToken); + } + } + catch (NpgsqlException ex) { - var take = Math.Min(MaxKeysPerUpdate, changed.Count - i); - rowsUpdated += await RunBatchAsync( - connection, changed, newSettings, newBootVals, newResetVals, i, take, day, serverId, cancellationToken); + /* Never the exception TEXT (it can carry the value that tripped it) — just the server id and + the SQLSTATE, e.g. 53400 when a target's day still exceeds the decompress limit even under + SET LOCAL = 0 (which cannot happen on its own, but a future change to the limit's floor + should still be caught here rather than stopping every later target). */ + logger?.LogWarning( + "pg_setting_scrub: server {ServerId} failed on {Day:yyyy-MM-dd} with SQLSTATE {SqlState}; skipping this server for the rest of the run, will retry on the next start", + serverId, day, ex.SqlState); + currentServerFailed = true; + failedServerIds.Add(serverId); + continue; } + + daysTouched.Add(day); + rowsUpdated += dayUpdated; } - if (rowsUpdated == changedRowCount) + if (rowsUpdated == changedRowCount && failedServerIds.Count == 0) { await WriteMarkerAsync(connection, currentVersion, cancellationToken); } + else if (failedServerIds.Count > 0) + { + logger?.LogWarning( + "pg_setting_scrub: {FailedCount} server(s) failed this run ({FailedServerIds}); leaving the marker unwritten so the next start retries them", + failedServerIds.Count, string.Join(",", failedServerIds)); + } else { /* R1 L4: a systematic key mismatch would otherwise mark the store scrubbed while plaintext rows @@ -329,7 +372,25 @@ private static async Task RunBatchAsync( resetVals[j] = newResetVals[offset + j]; } - await using var update = new NpgsqlCommand(BatchUpdateSql, connection) { CommandTimeout = UpdateBatchTimeoutSeconds }; + /* M3 (a): SET LOCAL, not a batch-per-collection_time split — it keeps H1's shape (one literal target + filter, one literal day range, per UPDATE) instead of adding a third grouping key. The GUC's + pg_settings context is `user`, so the store's own non-superuser role may set it (confirmed live + against TimescaleDB 2.30.1 on docker: SET SESSION AUTHORIZATION to an ordinary LOGIN role, then + SET LOCAL timescaledb.max_tuples_decompressed_per_dml_transaction = 0 succeeds and reads back as + 0). Scoped to this one batch's transaction only — LOCAL means it reverts at COMMIT/ROLLBACK, so it + never leaks into any other batch, any other server, or the marker read/write, which do not run + inside this transaction. 0 disables the limit entirely for this transaction, which is safe here + because MaxKeysPerUpdate already bounds how many rows a single batch's chunk segment can touch — + the risk the GUC guards against (an unbounded decompress) cannot occur at this batch size. */ + await using var transaction = await connection.BeginTransactionAsync(cancellationToken); + await using (var setLocal = new NpgsqlCommand( + "SET LOCAL timescaledb.max_tuples_decompressed_per_dml_transaction = 0", connection, transaction) + { CommandTimeout = UpdateBatchTimeoutSeconds }) + { + await setLocal.ExecuteNonQueryAsync(cancellationToken); + } + + await using var update = new NpgsqlCommand(BatchUpdateSql, connection, transaction) { CommandTimeout = UpdateBatchTimeoutSeconds }; update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Integer, Value = serverIds }); update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Timestamp, Value = times }); update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = names }); @@ -346,7 +407,9 @@ private static async Task RunBatchAsync( this day's chunk — see the H1 fix note above RunAsync's grouping. */ update.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Integer, Value = serverId }); - return await update.ExecuteNonQueryAsync(cancellationToken); + var updated = await update.ExecuteNonQueryAsync(cancellationToken); + await transaction.CommitAsync(cancellationToken); + return updated; } private static async Task ReadMarkerAsync(NpgsqlConnection connection, CancellationToken cancellationToken) diff --git a/PerformanceMonitor.Collectors/PgSettingRedactor.cs b/PerformanceMonitor.Collectors/PgSettingRedactor.cs index 13c007ce62..b791e10021 100644 --- a/PerformanceMonitor.Collectors/PgSettingRedactor.cs +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -164,6 +164,18 @@ public static class PgSettingRedactor return redacted; } + /// Names that would otherwise trip 's dotted-name test but + /// carry no secret — a password POLICY setting, not a password (review round 2's L3). Exact match, + /// case-insensitive; anything ELSE with a marker in a dot-separated segment is still masked whole, + /// including a real secret sitting in the same batch as one of these (the negative case the pin + /// covers). + private static readonly string[] NonSecretAllowlist = + { + "rds.accepted_password_auth_method", + "rds.restrict_password_commands", + "passwordcheck.min_password_length", + }; + private static bool IsWholeValueMasked(string? name) { if (string.IsNullOrEmpty(name)) @@ -176,6 +188,14 @@ private static bool IsWholeValueMasked(string? name) return true; } + foreach (var allowed in NonSecretAllowlist) + { + if (string.Equals(name, allowed, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + } + // Extension-scoped settings only: a bare core GUC such as password_encryption never matches here, // no matter what its name contains — only a dotted name is tested at all. Round 1's L1: a marker in // ANY dot-separated segment counts, not only the last one (vault.secret.value), so the whole name is From 732589aba78ce74a487fe0d532e6e055fedd59df Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:26:50 -0400 Subject: [PATCH 10/11] 4351: W1 guard SET LOCAL, W2 fix decompress-bound comment, S1 simplify skip, S3 tighten pins --- .../Darling.Tests/PgSettingScrubLiveTests.cs | 59 +++++++++++++++++++ .../PgSettingScrub.cs | 29 +++++---- 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs index 633faa2ab0..7429bab701 100644 --- a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs +++ b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs @@ -8,9 +8,12 @@ using System; using System.Collections.Generic; +using System.Globalization; +using System.IO; using System.Threading; using System.Threading.Tasks; using Npgsql; +using PerformanceMonitor.Collectors; using PerformanceMonitor.Darling.Service; using PerformanceMonitor.Darling.Storage; using Xunit; @@ -149,6 +152,11 @@ public async Task TheScrubCompletes_WhenOneTargetsOneDayExceedsTheDecompressLimi await using var verifyConnection = new NpgsqlConnection(scratch.ConnectionString); await verifyConnection.OpenAsync(ct); Assert.False(await ContainsSecretAsync(verifyConnection, ct), "the raw password survived the scrub under a tight decompress cap on a single target's single day"); + + var markerValue = await ScalarTextAsync(verifyConnection, + "SELECT state_value FROM collect.collector_state WHERE collector_name = 'pg_setting_scrub' AND state_key = 'rules_version'", + 0, DateTime.MinValue, ct); + Assert.Equal(PgSettingRedactor.RulesVersion.ToString(CultureInfo.InvariantCulture), markerValue); } /// @@ -170,6 +178,11 @@ public async Task TheScrub_IsolatesAFailingServer_AndRetriesOnTheNextRun() await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + /* badServer < goodServer is deliberate: SortedDictionary orders byServerDay by server_id first, so + badServer's batch runs FIRST on the connection and rolls back (see "What holds": await using + disposes an uncompleted transaction). goodServer's batch then runs right after that rollback on + the SAME connection, pinning that a failed-and-rolled-back batch never leaves the connection in a + state (e.g. a failed 25P02 transaction) that would poison the next server's batch. */ const int goodServer = -444448; const int badServer = -444449; @@ -223,6 +236,52 @@ await ExecAsync(setupConnection, var second = await PgSettingScrub.RunAsync(postgres, logger: null, ct); Assert.False(second.AlreadyDone); Assert.False(await ContainsSecretAsync(setupConnection, ct), "the raw password survived the retried run"); + + // Retry half of the claim: badServer's one row is what the second run picks up; the marker is now set. + Assert.Equal(1, first.RowsUpdated); + Assert.Equal(1, second.RowsUpdated); + + var markerValueAfterRetry = await ScalarTextAsync(setupConnection, + "SELECT state_value FROM collect.collector_state WHERE collector_name = 'pg_setting_scrub' AND state_key = 'rules_version'", + 0, DateTime.MinValue, ct); + Assert.Equal(PgSettingRedactor.RulesVersion.ToString(CultureInfo.InvariantCulture), markerValueAfterRetry); + } + + /// + /// W1 text pin: a pre-2.14 TimescaleDB store (the GUC arrived in timescale/timescaledb PR #6566) has no + /// timescaledb.max_tuples_decompressed_per_dml_transaction setting at all. A bare + /// SET LOCAL ... = 0 would raise 42704 on every batch on such a store, so the SET must be guarded + /// by a current_setting(name, true) IS NOT NULL check — true makes it non-throwing, and + /// set_config(..., true) is the SET LOCAL equivalent, scoped to the same transaction. + /// + [Fact] + public void TheSetLocalIsGuardedForAPre214TimescaleDbStore() + { + var source = File.ReadAllText(Path.Combine(RepoRoot(), + "Darling", "PerformanceMonitor.Darling.Service", "PgSettingScrub.cs")); + + Assert.Contains( + "SELECT set_config('timescaledb.max_tuples_decompressed_per_dml_transaction', '0', true)", + source, StringComparison.Ordinal); + Assert.Contains( + "WHERE current_setting('timescaledb.max_tuples_decompressed_per_dml_transaction', true) IS NOT NULL", + source, StringComparison.Ordinal); + + /* The un-guarded shape this replaces — must not come back. */ + Assert.DoesNotContain( + "\"SET LOCAL timescaledb.max_tuples_decompressed_per_dml_transaction = 0\"", + source, StringComparison.Ordinal); + } + + private static string RepoRoot() + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "PerformanceMonitor.sln"))) + { + directory = directory.Parent; + } + + return directory?.FullName ?? throw new InvalidOperationException("PerformanceMonitor.sln not found above the test output directory."); } private const string Mask = "********"; diff --git a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs index b3029ec2d0..af856c182a 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs @@ -248,21 +248,16 @@ public static async Task RunAsync(NpgsqlDataSource postgres, ILogger? l var changedRowCount = 0; var daysTouched = new HashSet(); var failedServerIds = new SortedSet(); - int? currentServerId = null; - var currentServerFailed = false; foreach (var ((serverId, day), dayCandidates) in byServerDay) { /* M3 (b): catch per server and continue — see the type remarks. Once this server's batch has failed, skip its remaining days this run (they share the same failure mode) and move on to the next server; the marker withheld below (rowsUpdated < changedRowCount) makes the WHOLE run retry every server, including ones that already succeeded, on the next start — cheap and - correct, since a server whose rows are already redacted contributes nothing on a re-run. */ - if (serverId != currentServerId) - { - currentServerId = serverId; - currentServerFailed = false; - } - else if (currentServerFailed) + correct, since a server whose rows are already redacted contributes nothing on a re-run. + failedServerIds already records the failure, so the skip does not depend on byServerDay's + iteration order grouping a server's days contiguously. */ + if (failedServerIds.Contains(serverId)) { continue; } @@ -314,7 +309,6 @@ public static async Task RunAsync(NpgsqlDataSource postgres, ILogger? l logger?.LogWarning( "pg_setting_scrub: server {ServerId} failed on {Day:yyyy-MM-dd} with SQLSTATE {SqlState}; skipping this server for the rest of the run, will retry on the next start", serverId, day, ex.SqlState); - currentServerFailed = true; failedServerIds.Add(serverId); continue; } @@ -380,11 +374,20 @@ private static async Task RunBatchAsync( 0). Scoped to this one batch's transaction only — LOCAL means it reverts at COMMIT/ROLLBACK, so it never leaks into any other batch, any other server, or the marker read/write, which do not run inside this transaction. 0 disables the limit entirely for this transaction, which is safe here - because MaxKeysPerUpdate already bounds how many rows a single batch's chunk segment can touch — - the risk the GUC guards against (an unbounded decompress) cannot occur at this batch size. */ + because the literal server_id + day-range predicates confine decompression to ONE target's + segment in ONE day's chunk (settings × collections that day — ~8k rows hourly, ~500k at a + 1-minute cadence), not the whole chunk; MaxKeysPerUpdate bounds the key array, not the + decompress. */ await using var transaction = await connection.BeginTransactionAsync(cancellationToken); + /* Guarded, not a bare SET LOCAL: a bring-your-own store on TimescaleDB older than 2.14 (the GUC + arrived in timescale/timescaledb PR #6566) has no such setting. current_setting(name, true) + returns NULL rather than raising when the GUC is absent, so the WHERE makes this a no-op on those + stores instead of a 42704 on every batch, every start, for good — set_config(..., true) is the + SET LOCAL equivalent (transaction-scoped, reverts at COMMIT/ROLLBACK). */ await using (var setLocal = new NpgsqlCommand( - "SET LOCAL timescaledb.max_tuples_decompressed_per_dml_transaction = 0", connection, transaction) + "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", + connection, transaction) { CommandTimeout = UpdateBatchTimeoutSeconds }) { await setLocal.ExecuteNonQueryAsync(cancellationToken); From 377894cedccc1c62dda98a1bc19138ce609d26ca Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:45:50 -0400 Subject: [PATCH 11/11] 4351: fix own-store marker to #1776 for hygiene test --- Darling/Darling.Tests/PgSettingScrubLiveTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs index 7429bab701..cc3a3d72df 100644 --- a/Darling/Darling.Tests/PgSettingScrubLiveTests.cs +++ b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs @@ -23,7 +23,7 @@ namespace Darling.Tests; /// /// Live, end-to-end proof of the #4348 S1b one-time scrub against a REAL compressed TimescaleDB chunk. /// -/// #4348 own-store — mints its own scratch database () rather than +/// #1776 own-store — mints its own scratch database () rather than /// sharing the live fixture, so it is deliberately NOT in the live-postgres collection: it creates its /// own hypertable/compression shape on collect.pg_server_config, which the shared fixture must never /// inherit from a test.