From 2abc4a72a6bd5ca2b03728a0e0c257f218c43350 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 9 Oct 2026 01:23:34 -0400 Subject: [PATCH 1/2] Darling live tests stop depending on PUBLIC's CONNECT on the shared test database (#5618) The password-key live test ran the managed provisioning batch inside a scratch database; its REVOKE ALL ON DATABASE darling FROM PUBLIC landed on the cluster's shared darling database, and the redaction test's own login (CONNECT only through PUBLIC) was then refused with 42501. The managed case now points its database-level statements at its own scratch database, the redaction test grants its probe role CONNECT itself, and a guard on the pg-cluster-roles collection fails the test that leaves PUBLIC without CONNECT. --- .../PasswordKeyTablesLiveTests.cs | 33 +++++- .../Darling.Tests/PgClusterRolesCollection.cs | 5 + .../PgSettingRedactionLivePostgresTests.cs | 20 +++- .../PublicConnectCatalogReadLiveTests.cs | 65 +++++++++++ Darling/Darling.Tests/PublicConnectDelta.cs | 30 +++++ .../PublicConnectGuardAttribute.cs | 108 ++++++++++++++++++ .../Darling.Tests/PublicConnectGuardTests.cs | 46 ++++++++ 7 files changed, 300 insertions(+), 7 deletions(-) create mode 100644 Darling/Darling.Tests/PublicConnectCatalogReadLiveTests.cs create mode 100644 Darling/Darling.Tests/PublicConnectDelta.cs create mode 100644 Darling/Darling.Tests/PublicConnectGuardAttribute.cs create mode 100644 Darling/Darling.Tests/PublicConnectGuardTests.cs diff --git a/Darling/Darling.Tests/PasswordKeyTablesLiveTests.cs b/Darling/Darling.Tests/PasswordKeyTablesLiveTests.cs index 135f268826..231c55d590 100644 --- a/Darling/Darling.Tests/PasswordKeyTablesLiveTests.cs +++ b/Darling/Darling.Tests/PasswordKeyTablesLiveTests.cs @@ -289,10 +289,30 @@ public async Task ASecondCurrentKeyFails_AndTheReadsReturnTheCurrentKeyAndTheNew private static ProvisioningTarget TargetFor(bool managed, NpgsqlConnection owner, ScratchPostgres scratch) => managed ? ProvisioningTarget.Managed : ProvisioningTarget.ComposeStore(OwnerRoleOf(owner), scratch.DatabaseName); - private static string ProvisioningBatch(bool managed, NpgsqlConnection owner, ScratchPostgres scratch) => - DarlingManagedRoles.BuildProvisioningSql( + private static string ProvisioningBatch(bool managed, NpgsqlConnection owner, ScratchPostgres scratch) + { + var batch = DarlingManagedRoles.BuildProvisioningSql( ProvisioningTestSecrets.Admin, ProvisioningTestSecrets.Viewer, ProvisioningTestSecrets.Mcp, 15, PasswordReassert.All, TargetFor(managed, owner, scratch)); + if (!managed) + { + return batch; + } + + /* #5618: the managed target names the cluster's shared "darling" database in its three database-level + statements (REVOKE ALL ... FROM PUBLIC, and the CONNECT grants to admin, viewer and mcp). Database + privileges live in pg_database, which every database in the cluster shares, so running the batch inside a + scratch database still took CONNECT away from PUBLIC on "darling" and never gave it back: the next test + that opened a connection through PUBLIC's CONNECT failed with 42501. The managed shape stays managed (its + owner, its role marker, the PUBLIC revoke), pointed at this test's own database, the way ViewerGrantReplay + points its replays. */ + var shared = "ON DATABASE " + ProvisioningTarget.Managed.DatabaseIdentifier + " "; + var scratchDatabase = "ON DATABASE \"" + scratch.DatabaseName.Replace("\"", "\"\"", StringComparison.Ordinal) + "\" "; + Assert.Contains("REVOKE ALL " + shared + "FROM PUBLIC;", batch, StringComparison.Ordinal); + batch = batch.Replace(shared, scratchDatabase, StringComparison.Ordinal); + Assert.DoesNotContain(shared, batch, StringComparison.Ordinal); + return batch; + } [Theory] [InlineData(true)] @@ -310,9 +330,8 @@ public async Task Provisioning_LeavesNoReadOfThePinTables_AndNoTriggerOrReferenc await ScalarAsync(owner, "SELECT EXISTS (SELECT 1 FROM pg_roles WHERE rolname IN ('admin', 'viewer', 'mcp'))", ct) is true, "A cluster-wide admin/viewer/mcp role already exists on this cluster; the provisioning batch would adopt it."); Assert.SkipWhen( - managed && (OwnerRoleOf(owner) != DarlingManagedPostgres.UserName - || await ScalarAsync(owner, "SELECT NOT EXISTS (SELECT 1 FROM pg_database WHERE datname = 'darling')", ct) is true), - "The managed shape names the owner and database 'darling', which this connection does not have."); + managed && OwnerRoleOf(owner) != DarlingManagedPostgres.UserName, + "The managed shape names the owner 'darling', which this connection is not."); var bodySucceeded = false; try @@ -320,6 +339,10 @@ await ScalarAsync(owner, "SELECT EXISTS (SELECT 1 FROM pg_roles WHERE rolname IN await ExecAsync(owner, ProvisioningBatch(managed, owner, scratch), ct); await SeedRowsAsync(owner, ct); + /* #5618: the managed batch's PUBLIC revoke, proven on the scratch database it now names (the compose shape + leaves PUBLIC's CONNECT alone). The shared "darling" database is not this test's to change. */ + Assert.Equal((object)!managed, await ScalarAsync(owner, "SELECT has_database_privilege('public', current_database(), 'CONNECT')", ct)); + /* The pin tables are read by the store owner only; the key and service-state tables are readable. */ foreach (var (role, password) in new[] { diff --git a/Darling/Darling.Tests/PgClusterRolesCollection.cs b/Darling/Darling.Tests/PgClusterRolesCollection.cs index 2547388e47..5f12142f28 100644 --- a/Darling/Darling.Tests/PgClusterRolesCollection.cs +++ b/Darling/Darling.Tests/PgClusterRolesCollection.cs @@ -32,8 +32,13 @@ namespace Darling.Tests; /// would run its residue check against a store the parallel collections /// are still writing. fails when a live class changes a cluster-wide /// object under a fixed name and is neither here nor on its allow list. +/// +/// (#5618) runs around every test in it and fails the one that leaves +/// PUBLIC without CONNECT on the shared test database. The product's managed batch revokes it, the privilege is +/// cluster-wide, and the tests after the one that did it fail with "permission denied for database". /// [CollectionDefinition("pg-cluster-roles", DisableParallelization = true)] +[PublicConnectGuard] public sealed class PgClusterRolesCollection { } diff --git a/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs b/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs index bfea3b5716..02eb653651 100644 --- a/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs +++ b/Darling/Darling.Tests/PgSettingRedactionLivePostgresTests.cs @@ -25,8 +25,10 @@ namespace Darling.Tests; /// 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. +/// Cluster-wide state (ALTER SYSTEM is not per-database), so this runs in the pg-cluster-roles +/// collection, alone, and restores the setting in a finally that runs even on failure. The probe role also +/// holds its own CONNECT on the database it connects to (#5618), so it passes whether or not an earlier test has +/// taken CONNECT away from PUBLIC. /// [Collection("pg-cluster-roles")] public sealed class PgSettingRedactionLivePostgresTests @@ -56,6 +58,7 @@ public async Task PrimaryConninfosPassword_IsRedactedByTheRealCollector_UnderAPg await using var owner = new NpgsqlConnection(cs); await owner.OpenAsync(ct); + var quotedDatabase = "\"" + owner.Database.Replace("\"", "\"\"", StringComparison.Ordinal) + "\""; var bodySucceeded = false; try @@ -67,6 +70,13 @@ public async Task PrimaryConninfosPassword_IsRedactedByTheRealCollector_UnderAPg await ExecAsync(owner, $"CREATE ROLE {RoleName} LOGIN PASSWORD '{RolePassword}'", ct); await ExecAsync(owner, $"GRANT pg_monitor TO {RoleName}", ct); + /* #5618: the probe role holds CONNECT on this database in its own right. A fresh role gets CONNECT only + through PUBLIC, and the product's provisioning batch takes that away from the cluster's "darling" database + (REVOKE ALL ... FROM PUBLIC), so a test that relied on PUBLIC failed with 42501 whenever an earlier test + had run that batch. Revoked in the cleanup below, before the role is dropped: a role that still holds a + privilege cannot be dropped. */ + await ExecAsync(owner, $"GRANT CONNECT ON DATABASE {quotedDatabase} TO {RoleName}", ct); + var probeConnectionString = new NpgsqlConnectionStringBuilder(cs) { Username = RoleName, @@ -150,6 +160,12 @@ await LiveStoreCleanup.RunOwnedAsync(bodySucceeded, async () => { await ExecAsync(owner, "ALTER SYSTEM SET primary_conninfo = ''", ct); await ExecAsync(owner, "SELECT pg_reload_conf()", ct); + + /* #5618: the grant goes before the role does. The role may not exist when the body failed before + creating it, hence the existence check. */ + await ExecAsync(owner, + $"DO $$ BEGIN IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = '{RoleName}') THEN " + + $"REVOKE CONNECT ON DATABASE {quotedDatabase} FROM {RoleName}; END IF; END $$", ct); await ExecAsync(owner, $"DROP ROLE IF EXISTS {RoleName}", ct); }); } diff --git a/Darling/Darling.Tests/PublicConnectCatalogReadLiveTests.cs b/Darling/Darling.Tests/PublicConnectCatalogReadLiveTests.cs new file mode 100644 index 0000000000..cd5a7bc4dd --- /dev/null +++ b/Darling/Darling.Tests/PublicConnectCatalogReadLiveTests.cs @@ -0,0 +1,65 @@ +/* + * 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.Linq; +using System.Reflection; +using System.Threading.Tasks; +using Npgsql; +using Xunit; + +namespace Darling.Tests; + +/// +/// The PUBLIC-CONNECT guard against a real cluster (#5618): its catalog read sees a database's PUBLIC CONNECT granted, +/// then revoked, and the comparison names the database (the revoke is on this test's own scratch database, never on +/// the shared one); and the collection that needs the guard still carries it. +/// +[Collection("live-postgres")] +public sealed class PublicConnectCatalogReadLiveTests +{ + private static string? ConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + + [Fact] + public void ThePgClusterRolesCollection_CarriesTheGuard() + { + /* The guard only guards what it is attached to. Taking it off the collection would leave the next test that + revokes the privilege to be found the way #5618 was, by the victim's name in a nightly run. */ + Assert.Single(typeof(PgClusterRolesCollection).GetCustomAttributes()); + } + + [Fact] + public async Task TheCatalogRead_SeesAGrantAndARevokeOfPublicConnect() + { + var cs = ConnectionString; + Assert.SkipWhen(string.IsNullOrEmpty(cs), "Set DARLING_TEST_PG to a Postgres connection string to run the #5618 guard read."); + var ct = TestContext.Current.CancellationToken; + + /* #1776 own-store: the revoke below is on this test's own scratch database, never on the shared one. */ + await using var scratch = await ScratchPostgres.CreateAsync(cs!, ct); + + var before = PublicConnectGuardAttribute.PublicConnectOn(cs!, scratch.DatabaseName, "database_that_does_not_exist_5618"); + Assert.Equal([scratch.DatabaseName], before.Keys.ToArray()); + Assert.True(before[scratch.DatabaseName], "a new database grants CONNECT to PUBLIC"); + + await using (var admin = new NpgsqlConnection(cs)) + { + await admin.OpenAsync(ct); + await using var revoke = new NpgsqlCommand($"REVOKE ALL ON DATABASE \"{scratch.DatabaseName}\" FROM PUBLIC", admin); + await revoke.ExecuteNonQueryAsync(ct); + } + + var after = PublicConnectGuardAttribute.PublicConnectOn(cs!, scratch.DatabaseName); + Assert.False(after[scratch.DatabaseName]); + Assert.Equal([scratch.DatabaseName], PublicConnectDelta.DatabasesThatLostPublicConnect(before, after)); + + /* The default read names the connection's own database, as CI has it. */ + var shared = PublicConnectGuardAttribute.PublicConnectOn(cs!); + Assert.Contains(new NpgsqlConnectionStringBuilder(cs).Database!, shared.Keys); + } +} diff --git a/Darling/Darling.Tests/PublicConnectDelta.cs b/Darling/Darling.Tests/PublicConnectDelta.cs new file mode 100644 index 0000000000..bb53580884 --- /dev/null +++ b/Darling/Darling.Tests/PublicConnectDelta.cs @@ -0,0 +1,30 @@ +/* + * 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.Linq; + +namespace Darling.Tests; + +/// +/// The comparison the PUBLIC-CONNECT guard makes between two reads of PUBLIC's CONNECT (#5618). Its own +/// type, and free of any store access, so a guard-stage test can pin it without a PostgreSQL server. +/// +internal static class PublicConnectDelta +{ + /// The databases whose PUBLIC CONNECT was granted in and is revoked in + /// , in name order. A database missing from either side, or revoked in both, is not a change. + internal static IReadOnlyList DatabasesThatLostPublicConnect( + IReadOnlyDictionary before, IReadOnlyDictionary after) => + before + .Where(b => b.Value && after.TryGetValue(b.Key, out var now) && !now) + .Select(b => b.Key) + .OrderBy(name => name, StringComparer.Ordinal) + .ToList(); +} diff --git a/Darling/Darling.Tests/PublicConnectGuardAttribute.cs b/Darling/Darling.Tests/PublicConnectGuardAttribute.cs new file mode 100644 index 0000000000..d863e62690 --- /dev/null +++ b/Darling/Darling.Tests/PublicConnectGuardAttribute.cs @@ -0,0 +1,108 @@ +/* + * 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.Linq; +using System.Reflection; +using Npgsql; +using PerformanceMonitor.Darling.Service; +using Xunit; +using Xunit.v3; + +namespace Darling.Tests; + +/// +/// Fails the test that leaves PUBLIC without CONNECT on the shared test database (#5618). +/// +/// Why it exists. The product's managed provisioning batch writes +/// REVOKE ALL ON DATABASE darling FROM PUBLIC, and database privileges live in pg_database, which every +/// database in the cluster shares. A live test that ran that batch inside its own scratch database still took CONNECT +/// away from PUBLIC on the cluster's darling database, and nothing gave it back. The next test that opened a +/// connection as a freshly created role (a role has CONNECT only through PUBLIC) failed with 42501, and it was +/// that victim, not the test that did it, whose name appeared in the nightly run. It took a day of measuring to find +/// the cause. This puts the failure on the test that does it. +/// +/// What it compares. PUBLIC's CONNECT on each database before the test against the same read after it, +/// and fails only a change from granted to revoked. It does not demand that PUBLIC hold CONNECT: a developer's own +/// managed store has it revoked on purpose, and a guard that failed there would fail every test in the collection. +/// The databases are the one DARLING_TEST_PG names and the managed store's fixed name darling, which are +/// one database in CI and two on a laptop that keeps its suite in another database. +/// +/// Where it runs. On , the collection for the tests that change +/// cluster-wide state under fixed names. That collection runs alone, so a change between one test's start and end is +/// that test's. keeps every other class that changes a database-level +/// privilege out of the parallel collections. Without DARLING_TEST_PG it does nothing, as the tests skip. +/// +/// #1776 own-store, not [Collection("live-postgres")]: it reads one cluster-wide catalog +/// (pg_database) and no row or relation of the shared store, so it has nothing to serialize. It runs around the +/// tests of the one collection that does. +/// +[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)] +public sealed class PublicConnectGuardAttribute : BeforeAfterTestAttribute +{ + /// The state read in . Static so it does not depend on whether xUnit reuses one attribute + /// instance; the collection it guards runs one test at a time, so one slot is enough. + private static Dictionary? s_before; + + public override void Before(MethodInfo methodUnderTest, IXunitTest test) + { + var cs = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + s_before = string.IsNullOrEmpty(cs) ? null : PublicConnectOn(cs); + } + + public override void After(MethodInfo methodUnderTest, IXunitTest test) + { + var cs = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + var before = s_before; + s_before = null; + if (string.IsNullOrEmpty(cs) || before is null) + { + return; + } + + var lost = PublicConnectDelta.DatabasesThatLostPublicConnect(before, PublicConnectOn(cs)); + if (lost.Count > 0) + { + throw new InvalidOperationException( + $"{test.TestDisplayName} left PUBLIC without CONNECT on database {string.Join(", ", lost.Select(d => "\"" + d + "\""))} " + + "(#5618). Database privileges live in the cluster's shared catalog, so a test that runs the managed provisioning batch " + + "(ProvisioningTarget.Managed, whose PUBLIC revoke names the shared database) takes CONNECT away " + + "from every later test that relies on it. Point the batch at the test's own scratch database " + + "(ProvisioningTarget.ComposeStore(owner, scratch.DatabaseName), or the managed text with its database-level statements retargeted), " + + "or grant CONNECT back in a finally."); + } + } + + /// + /// Whether PUBLIC holds CONNECT on each of that exists on the cluster + /// reaches (DARLING_TEST_PG's own database and the managed store's + /// fixed name when none is passed). Unpooled: a pooled session must not outlive a read of a shared catalog. + /// + internal static Dictionary PublicConnectOn(string connectionString, params string[] databases) + { + var builder = new NpgsqlConnectionStringBuilder(connectionString) { Pooling = false }; + var names = databases.Length > 0 + ? databases + : new[] { builder.Database ?? string.Empty, ProvisioningTarget.Managed.DatabaseIdentifier }; + + using var connection = new NpgsqlConnection(builder.ConnectionString); + connection.Open(); + using var command = new NpgsqlCommand( + "SELECT datname, has_database_privilege('public', datname, 'CONNECT') FROM pg_database WHERE datname = ANY (@names)", connection); + command.Parameters.AddWithValue("names", names.Distinct(StringComparer.Ordinal).ToArray()); + var state = new Dictionary(StringComparer.Ordinal); + using var reader = command.ExecuteReader(); + while (reader.Read()) + { + state[reader.GetString(0)] = reader.GetBoolean(1); + } + + return state; + } +} diff --git a/Darling/Darling.Tests/PublicConnectGuardTests.cs b/Darling/Darling.Tests/PublicConnectGuardTests.cs new file mode 100644 index 0000000000..d395ac1f13 --- /dev/null +++ b/Darling/Darling.Tests/PublicConnectGuardTests.cs @@ -0,0 +1,46 @@ +/* + * 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.Collections.Generic; +using Xunit; + +namespace Darling.Tests; + +/// +/// The guard that fails the test which leaves PUBLIC without CONNECT on the shared test database (#5618), and its +/// comparison. No store is touched. +/// +[Trait("Stage", "Guard")] +public sealed class PublicConnectGuardTests +{ + [Fact] + public void OnlyAGrantedToRevokedChange_IsReported() + { + var before = new Dictionary + { + ["darling"] = true, + ["suite"] = true, + ["already_revoked"] = false, + ["regranted"] = false, + ["vanished"] = true, + }; + var after = new Dictionary + { + ["darling"] = false, + ["suite"] = true, + ["already_revoked"] = false, + ["regranted"] = true, + ["newcomer"] = false, + }; + + /* "darling" lost it; "suite" kept it; "already_revoked" was not granted to begin with (a developer's own managed + store), so it is not this test's doing; "regranted" went the other way; a database that appears or disappears + between the two reads is not a change of the privilege. */ + Assert.Equal(["darling"], PublicConnectDelta.DatabasesThatLostPublicConnect(before, after)); + } +} From e95b5d6e18b537ab54428ed4619d73b607a789fe Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 9 Oct 2026 01:33:25 -0400 Subject: [PATCH 2/2] Guard comment: drop an untrue line about how long the cause took to find --- Darling/Darling.Tests/PublicConnectGuardAttribute.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Darling/Darling.Tests/PublicConnectGuardAttribute.cs b/Darling/Darling.Tests/PublicConnectGuardAttribute.cs index d863e62690..72e0ff60e3 100644 --- a/Darling/Darling.Tests/PublicConnectGuardAttribute.cs +++ b/Darling/Darling.Tests/PublicConnectGuardAttribute.cs @@ -25,8 +25,8 @@ namespace Darling.Tests; /// database in the cluster shares. A live test that ran that batch inside its own scratch database still took CONNECT /// away from PUBLIC on the cluster's darling database, and nothing gave it back. The next test that opened a /// connection as a freshly created role (a role has CONNECT only through PUBLIC) failed with 42501, and it was -/// that victim, not the test that did it, whose name appeared in the nightly run. It took a day of measuring to find -/// the cause. This puts the failure on the test that does it. +/// that victim, not the test that did it, whose name appeared in the nightly run. This puts the failure on the test +/// that does it. /// /// What it compares. PUBLIC's CONNECT on each database before the test against the same read after it, /// and fails only a change from granted to revoked. It does not demand that PUBLIC hold CONNECT: a developer's own