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/PgServerConfigScopeRungTests.cs b/Darling/Darling.Tests/PgServerConfigScopeRungTests.cs index 17a69d720a..5dbf5ee10c 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/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..194831b94b --- /dev/null +++ b/Darling/Darling.Tests/PgSettingRedactorTests.cs @@ -0,0 +1,241 @@ +// 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=********")] + // 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'", + "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")] + // 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 ********")] + // 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")] + // 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", + "********")] + [InlineData("ssl_passphrase_command", + "", + "")] + // 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/ + // 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")] + // 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) + { + 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); + } + + // 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 new file mode 100644 index 0000000000..cc3a3d72df --- /dev/null +++ b/Darling/Darling.Tests/PgSettingScrubLiveTests.cs @@ -0,0 +1,413 @@ +/* + * 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.IO; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Collectors; +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 +{ + /// + /// 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"); + } + + /// + /// 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"); + + 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); + } + + /// + /// 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); + + /* 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; + + 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"); + + // 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 = "********"; + + 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 6a2dfea76f..6443cfb39d 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -1819,6 +1819,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 @@ -3079,6 +3086,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"); } @@ -3416,6 +3434,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..af856c182a --- /dev/null +++ b/Darling/PerformanceMonitor.Darling.Service/PgSettingScrub.cs @@ -0,0 +1,446 @@ +/* + * 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 (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 +/// 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 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 +/// 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/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. + /// + 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 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 '%credential%' OR name ILIKE '%pwd%' OR name ILIKE '%pass%'))"; + + 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 +AND t.server_id = $11"; + + 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)) + { + 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), + 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))); + } + } + +/* 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 key = (c.ServerId, c.CollectionTime.Date); + if (!byServerDay.TryGetValue(key, out var list)) + { + list = new List(); + byServerDay[key] = list; + } + list.Add(c); + } + + var rowsUpdated = 0; + var changedRowCount = 0; + var daysTouched = new HashSet(); + var failedServerIds = new SortedSet(); + 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. + 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; + } + + 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; + } + + changedRowCount += changed.Count; + 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) + { + /* 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); + failedServerIds.Add(serverId); + continue; + } + + daysTouched.Add(day); + rowsUpdated += dayUpdated; + } + + 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 + 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.Count); + } + + private static async Task RunBatchAsync( + NpgsqlConnection connection, List changed, List newSettings, List newBootVals, + List newResetVals, int offset, int count, DateTime day, int serverId, 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]; + } + + /* 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 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( + "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); + } + + 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 }); + 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) }); + /* 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 }); + + var updated = await update.ExecuteNonQueryAsync(cancellationToken); + await transaction.CommitAsync(cancellationToken); + return updated; + } + + 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); + } +} diff --git a/PerformanceMonitor.Collectors/PgServerConfigCollector.cs b/PerformanceMonitor.Collectors/PgServerConfigCollector.cs index 738c682908..c732018e87 100644 --- a/PerformanceMonitor.Collectors/PgServerConfigCollector.cs +++ b/PerformanceMonitor.Collectors/PgServerConfigCollector.cs @@ -322,16 +322,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 new file mode 100644 index 0000000000..b791e10021 --- /dev/null +++ b/PerformanceMonitor.Collectors/PgSettingRedactor.cs @@ -0,0 +1,219 @@ +/* + * 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 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 +/// 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. +/// +/// +/// 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), 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", + "passwd", + "passphrase", + "secret", + "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 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); + + /// 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. 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\u00A0]*):[^@/]*@", + 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.CultureInvariant); + + /// 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.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(?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.-]*(?:PASS|SECRET|TOKEN|CREDENTIAL|PWD|(? + /// 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); + redacted = OptionSecretSpaced.Replace(redacted, static m => m.Groups["opt"].Value + " " + Mask); + + 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)) + { + return false; + } + + if (string.Equals(name, "ssl_passphrase_command", StringComparison.OrdinalIgnoreCase)) + { + 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 + // tested as a substring rather than slicing off just the last part. + var dot = name.IndexOf('.'); + if (dot < 0) + { + return false; + } + + foreach (var marker in WholeValueNameMarkers) + { + if (name.Contains(marker, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; + } +}