From e50d3b3440dd015c74b5f86103a7f404e866b7c5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 19 Aug 2026 12:08:50 +0000 Subject: [PATCH] Add --harden-files, the actor that can apply the ACLs the service only knows The service computes the correct DACL and detects when the real one is wrong. What it lacks is authority: re-ACLing a file it does not own needs WRITE_DAC, and taking ownership needs a privilege a virtual service account is not granted. So it logs the remedy and carries on -- correctly, because a monitoring service must not refuse to monitor over a permissions problem -- and the only thing that ever APPLIED the rule to an existing install was install-darling.ps1. A box registered through the README's own sc create path left the operator hand-typing three icacls lines out of a CRITICAL log line (#2352). Seen on a real box today: darling.json and lp.secret both carried BUILTIN\Users:(RX) with inheritance unprotected, and every service start had been saying so, precisely and with the right commands, since the box was built. It VERIFIES rather than claims. Every target is re-read after the attempt and reported SECURED or STILL READABLE, and the exit code is 1 if anything is still exposed, so it can gate a provisioning script. "We tried" is not the same statement as "the secret is not readable" -- the distinction that let a permissions call which silently did nothing hide in the field for months. darling.json is the only target the interactive operator keeps read on: the Viewer and the CLI verbs run as that operator and must still read it. Nothing reads a backup (#1769), so the .bak-* siblings, the store directory and the DPAPI credentials get no interactive ACE. A config that will not load is a warning rather than a stop -- an unreadable darling.json is the very failure this repairs, so refusing to run without it would be refusing exactly when it is needed. Missing targets are skipped quietly: a BYO-Postgres install has no managed credential files and their absence is not a fault. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 2 + .../DarlingHardenFilesVerbTests.cs | 130 +++++++++++++ .../DarlingCliCommands.cs | 179 ++++++++++++++++++ .../Program.cs | 18 ++ 4 files changed, 329 insertions(+) create mode 100644 Darling/Darling.Tests/DarlingHardenFilesVerbTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index b96903b566..7a50f8f1f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **`--harden-files`: re-apply the secret-file ACLs from an elevated prompt** ([#2352]) - the service already computes the correct DACL and already detects when the real one is wrong, but cannot apply it: re-ACLing a file it does not own needs WRITE_DAC and taking ownership needs a privilege a virtual service account is not granted, so it could only log the remedy. Until now the only thing that applied it to an existing install was `install-darling.ps1`, leaving anyone who registered the exe by hand - the README's own `sc create` path - typing three `icacls` lines out of a log message. Covers darling.json (the only target the interactive operator keeps read on), its `.bak-*` siblings, the store directory and the DPAPI credentials; VERIFIES each one afterwards rather than claiming success, and exits non-zero if anything is still readable so it is usable in a provisioning script. Idempotent. - **Declared peer stores: a Darling MCP server that names its siblings instead of answering "unknown server"** ([#2339]) - tier 1 of the multi-store fix, disclosure only. With the fleet split across several boxes (one store each: SQL Server primaries on one, their readable replicas on another, PostgreSQL on a third) every box's MCP server answered over ITS store alone, so a server monitored by a sibling resolved as not-found - indistinguishable from a server nobody monitors, which with a deliberately-split fleet is now the normal case rather than an edge. An optional `peers` block in darling.json (`thisStoreCovers`, plus per-peer `name` / `covers` / optional `matches` name-substrings) is disclosed at the three places an agent forms its picture of the fleet: the MCP instructions gain a Fleet Coverage section above the tool census, `list_servers` gains `this_store_covers` + `peer_fleets` + a `peer_note` (its empty-registry answer is prose rather than JSON, and carries the peer list too - a store with nothing registered is a fresh or just-restarted box, the worst place to drop the disclosure), and the server-resolution miss appends "not monitored HERE - matches the declared coverage of " to the existing available-servers listing. There is NO credential, NO address and NO connectivity behind it: the service never contacts a peer and says so in every message, and the publish itself REFUSES a peers block whose text looks like a connection string or credential, since all of it is sent verbatim to every connected MCP client. That guard lives in the publish rather than only in config validation because the MCP host loads its own config and deliberately never validates it (its fail-closed checks are host-local), so validation alone would have left the one path that actually broadcasts uncovered; a refusal publishes nothing at all rather than the valid subset. An empty `peer_fleets` carries its own note - "this may be the only store, or nobody declared the siblings, and this server cannot tell those apart" - so absence never reads as "you are looking at the whole fleet". With nothing declared the PROSE surfaces are byte-for-byte unchanged (the instructions, the resolution miss, and the empty-registry sentence), with ONE deliberate exception: `list_servers`' JSON envelope carries `this_store_covers` / `peer_fleets` / `peer_note` on every response, declared or not, so a client comparing that tool's exact shape sees three new keys on upgrade. That is the point rather than an oversight - an empty `peer_fleets` means either "only store" or "nobody declared the siblings", and a conditional note would say nothing in exactly the case that produces the wrong conclusion. Lite has no peers concept and gets no twin. Federated cross-store reads stay unbuilt on purpose ### Changed @@ -2819,6 +2820,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#2348]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2348 [#2350]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2350 [#2353]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2353 +[#2352]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2352 [#2331]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2331 [#2181]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2181 [#2317]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2317 diff --git a/Darling/Darling.Tests/DarlingHardenFilesVerbTests.cs b/Darling/Darling.Tests/DarlingHardenFilesVerbTests.cs new file mode 100644 index 0000000000..020be148c8 --- /dev/null +++ b/Darling/Darling.Tests/DarlingHardenFilesVerbTests.cs @@ -0,0 +1,130 @@ +/* + * 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.IO; +using System.Runtime.CompilerServices; +using PerformanceMonitor.Darling.Service; +using Xunit; + +namespace Darling.Tests; + +/// +/// #2352: --harden-files — the actor that can actually apply the secret-file ACLs. +/// +/// Why the verb exists. The service already computes the correct DACL +/// (DarlingFileSecurity.HardenFile) and already detects when the real one is wrong +/// (IsReadableByOrdinaryUsers). What it lacks is authority: re-ACLing a file it does not own needs +/// WRITE_DAC, and taking ownership needs a privilege a virtual service account is not granted. So it logs the +/// remedy and continues — correctly, since a monitoring service must not refuse to monitor over a permissions +/// problem — and until now the only thing that ever APPLIED the rule to an existing install was +/// install-darling.ps1. A box registered by hand through the README's own sc create path left the +/// operator typing three icacls lines out of a log message. +/// +/// The ACL work itself is Windows-only and needs a real filesystem, so what is pinned here is everything +/// that decides whether the verb is REACHABLE and honest — the failure mode that shipped once already (#1912), +/// where a verb had a full dispatch block, appeared in the help text, and was bounced as "Unknown option" +/// because the allow-list never learned it. +/// +public class DarlingHardenFilesVerbTests +{ + [Theory] + [InlineData("--harden-files", true)] + [InlineData("--HARDEN-FILES", true)] + [InlineData("--Harden-Files", true)] + [InlineData("--harden", false)] + [InlineData("--harden-file", false)] + [InlineData("--configure-firewall", false)] + [InlineData("--nonsense", false)] + public void IsHardenFilesVerb_RecognizesTheVerb_CaseInsensitive(string arg, bool expected) + { + Assert.Equal(expected, DarlingCliCommands.IsHardenFilesVerb(arg)); + } + + /// + /// The allow-list must reach it or the Program.cs dispatch is dead code and the startup classifier answers + /// "Unknown option" instead. The generic reflection pin covers this too; naming it makes the intent local. + /// + [Fact] + public void IsKnownVerb_ReachesTheHardenVerb() + { + Assert.True(DarlingCliCommands.IsKnownVerb("--harden-files")); + } + + /// An operator who cannot find the verb does not have it. It is the remedy for a CRITICAL log line, + /// so it has to be listed where someone reading that line will look. + [Fact] + public void TheHelpText_ListsTheVerb_AndSaysItNeedsElevation() + { + var help = DarlingCliCommands.UsageText(); + + Assert.Contains("--harden-files", help, StringComparison.Ordinal); + Assert.Contains("elevated", help, StringComparison.OrdinalIgnoreCase); + } + + /// + /// Program.cs dispatches it, guarded on Windows, and calls it rather than something adjacent — pinned + /// structurally because the seam between the classifier and the dispatch is exactly where #1912's drift + /// lived, and no unit test that calls DarlingCliCommands directly can see it. + /// + [Fact] + public void ProgramDispatchesTheVerb_BehindAWindowsGuard() + { + var program = ReadRepoFile(Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "Program.cs")); + + var at = program.IndexOf("DarlingCliCommands.IsHardenFilesVerb(args[0])", StringComparison.Ordinal); + Assert.True(at >= 0, "Program.cs no longer dispatches --harden-files (#2352)"); + + var block = program[at..Math.Min(program.Length, at + 900)]; + + Assert.Contains("OperatingSystem.IsWindows()", block, StringComparison.Ordinal); + Assert.Contains("DarlingCliCommands.HardenFiles(", block, StringComparison.Ordinal); + } + + /// + /// The verdict is the RE-READ, never the call that returned without throwing. "We tried" is not the same + /// statement as "the secret is not readable" — the distinction that let a permissions call which silently + /// did nothing hide in the field. Pinned on the source because the behaviour needs Windows ACLs to observe. + /// + [Fact] + public void TheVerb_VerifiesEachTarget_AndFailsWhenAnythingIsStillReadable() + { + var source = ReadRepoFile(Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "DarlingCliCommands.cs")); + + var at = source.IndexOf("public static int HardenFiles(", StringComparison.Ordinal); + Assert.True(at >= 0, "HardenFiles is gone (#2352)"); + + var body = source[at..Math.Min(source.Length, at + 6000)]; + + /* It re-reads rather than trusting the call. */ + Assert.Contains("DarlingFileSecurity.IsReadableByOrdinaryUsers(", body, StringComparison.Ordinal); + + /* And a still-exposed target is a non-zero exit, so it is usable in a provisioning script. */ + Assert.Contains("STILL READABLE", body, StringComparison.Ordinal); + + /* The live config is the ONLY target the interactive operator keeps read on: the Viewer and the CLI + verbs run as that operator and must still read it. Nothing reads a backup (#1769). */ + Assert.Contains("AllowInteractive: true", body, StringComparison.Ordinal); + } + + private static string ReadRepoFile(string relative, [CallerFilePath] string thisFile = "") + { + for (var dir = new DirectoryInfo(Path.GetDirectoryName(thisFile)!); dir is not null; dir = dir.Parent) + { + var candidate = Path.Combine(dir.FullName, relative); + if (File.Exists(candidate)) + { + return File.ReadAllText(candidate); + } + } + + throw new FileNotFoundException($"Could not locate {relative} walking up from {thisFile}"); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index 64f6ee93cd..9b2cd1a403 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -91,6 +91,13 @@ public static bool IsConfigureNetworkVerb(string arg) => public static bool IsConfigureFirewallVerb(string arg) => string.Equals(arg, "--configure-firewall", StringComparison.OrdinalIgnoreCase); + /// The verb handles — re-apply the secret-file ACLs from an elevated + /// prompt (#2352). The running service knows the correct ACL and cannot apply it: re-ACLing a file it does + /// not own needs WRITE_DAC, and taking ownership needs a privilege a virtual service account is not + /// granted, so it can only log the remedy. This is the actor that carries it out. + public static bool IsHardenFilesVerb(string arg) => + string.Equals(arg, "--harden-files", StringComparison.OrdinalIgnoreCase); + /// The verb handles — enable the MCP endpoint in the store (+ firewall). public static bool IsEnableMcpVerb(string arg) => string.Equals(arg, "--enable-mcp", StringComparison.OrdinalIgnoreCase); @@ -154,6 +161,7 @@ public static bool IsKnownVerb(string arg) => || IsExportViewerConfigVerb(arg) || IsConfigureNetworkVerb(arg) || IsConfigureFirewallVerb(arg) + || IsHardenFilesVerb(arg) || IsEnableMcpVerb(arg) || IsDisableMcpVerb(arg) || IsEnableWebVerb(arg) @@ -227,6 +235,7 @@ public static string UsageText() => " PerformanceMonitor.Darling.Service.exe --export-viewer-config [dir] [--config ] Write a ready-to-copy viewer folder (darling.json + server.crt + README.txt)." + Environment.NewLine + " PerformanceMonitor.Darling.Service.exe --configure-network Interactive LAN-exposure wizard." + Environment.NewLine + " PerformanceMonitor.Darling.Service.exe --configure-firewall Create/remove the scoped firewall rules to match darling.json (run elevated)." + Environment.NewLine + + " PerformanceMonitor.Darling.Service.exe --harden-files Re-apply the ACLs on darling.json and the store credentials (run elevated)." + Environment.NewLine + " PerformanceMonitor.Darling.Service.exe --enable-mcp Enable the MCP endpoint in the store and open its firewall (run elevated)." + Environment.NewLine + " PerformanceMonitor.Darling.Service.exe --disable-mcp Disable the MCP endpoint in the store and remove its firewall rule (run elevated)." + Environment.NewLine + " PerformanceMonitor.Darling.Service.exe --enable-web Enable the web dashboard in the store and open its firewall (run elevated)." + Environment.NewLine + @@ -2740,6 +2749,176 @@ public static IReadOnlyList PlanFirewallRules(DarlingConfig co _ => null, }; + /// + /// Creates or removes every scoped Darling firewall rule so the live firewall matches darling.json. + /// Requires elevation (that is the entire point of the verb) and is idempotent — safe to re-run on every + /// upgrade, which is exactly how install-darling.ps1 uses it. Returns 0 when the firewall ends up matching + /// the config, 1 when it could not be made to. + /// + [SupportedOSPlatform("windows")] + /// + /// One target of : a path, whether the interactive operator legitimately reads it, + /// and what it is called in the report. Kept as data so the list is readable as a policy rather than as + /// control flow — which file gets INTERACTIVE read is the only judgement in this verb, and it should be + /// visible at a glance. + /// + private readonly record struct HardenTarget(string Path, bool AllowInteractive, bool IsDirectory, string What); + + /// + /// Re-applies the secret-file ACLs, elevated (#2352). + /// + /// Why this exists as a verb. The service already computes the correct DACL + /// () and already detects when the real one is wrong + /// (). What it lacks is authority: re-ACLing a + /// file it does not own needs WRITE_DAC, and taking ownership needs a privilege a virtual service account is + /// not granted, so it can only log the remedy and continue. Until now the only thing that ever APPLIED the + /// rule to an existing install was install-darling.ps1, which leaves anyone who registered the exe by + /// hand — the README's own sc create path — typing three icacls lines out of a log message. + /// + /// It verifies rather than claims. Every target is re-read after the attempt and reported as + /// SECURED or STILL READABLE, and the exit code is 1 if anything is still exposed. "We tried" is not the + /// same statement as "the secret is not readable" — a distinction 's own + /// contract already makes, and the reason a permissions call that silently did nothing was able to hide. + /// + /// Idempotent, so it is safe on a healthy box and can simply live in a runbook. Missing targets are + /// skipped quietly: a BYO-Postgres install has no managed credential files, and their absence is not a + /// fault. + /// + [SupportedOSPlatform("windows")] + public static int HardenFiles(string? configPath, TextWriter output, TextWriter error) + { + var resolvedConfig = DarlingConfig.ResolveConfigPath(configPath); + + /* The managed store's directory is derived from config when it loads, but this verb has to work when + darling.json is exactly what is unreadable — that is the failure it exists to repair. So a config that + will not load is a warning, not a stop: the config file itself is still hardened, and the store + targets fall back to the documented default location. */ + string? dataDirectory = null; + try + { + var config = DarlingConfig.Load(configPath); + dataDirectory = DarlingManagedPostgres.ResolveDataDirectory(config.Postgres); + } + catch (Exception ex) + { + output.WriteLine($"NOTE: could not load {resolvedConfig} ({ex.Message})."); + output.WriteLine(" Continuing with the documented default store location — hardening darling.json"); + output.WriteLine(" is very often what makes it loadable again."); + dataDirectory = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + "PerformanceMonitorDarling", "pg"); + } + + var storeRoot = Path.GetDirectoryName(Path.GetFullPath(dataDirectory)); + var targets = new List + { + /* INTERACTIVE read, alone in this list: the Viewer (ViewerSettings.ResolveConfigPath) and the CLI + verbs run as the operator and must still read the live config. Nothing reads a backup (#1769). */ + new(resolvedConfig, AllowInteractive: true, IsDirectory: false, "the live config"), + }; + + foreach (var backup in SafeEnumerate(Path.GetDirectoryName(resolvedConfig), "darling.json.bak-*")) + { + targets.Add(new(backup, AllowInteractive: false, IsDirectory: false, "a config backup")); + } + + if (!string.IsNullOrEmpty(storeRoot)) + { + targets.Add(new(storeRoot, AllowInteractive: false, IsDirectory: true, "the store directory")); + targets.Add(new(Path.Combine(storeRoot, "pg-credential.dpapi"), false, false, "the store credential")); + targets.Add(new(Path.Combine(storeRoot, "pg-admin-credential.dpapi"), false, false, "the admin credential")); + } + + output.WriteLine($"Hardening for service account: {DarlingFileSecurity.ServiceAccountDisplayName}"); + output.WriteLine(); + + var exposed = 0; + var touched = 0; + + foreach (var target in targets) + { + var exists = target.IsDirectory ? Directory.Exists(target.Path) : File.Exists(target.Path); + if (!exists) + { + continue; + } + + touched++; + + try + { + if (target.IsDirectory) + { + /* Traverse, not read: the operator's Viewer needs to walk to the config, never to read the + credential blobs sitting in here. Mirrors DarlingManagedPostgres' own call. */ + DarlingFileSecurity.HardenDirectory(target.Path, allowInteractiveTraverse: true); + } + else + { + DarlingFileSecurity.HardenFile(target.Path, target.AllowInteractive); + } + } + catch (Exception ex) + { + error.WriteLine($" FAILED {target.Path} ({target.What}): {ex.Message}"); + error.WriteLine($" {DarlingFileSecurity.DescribeOwnerAndExposure(target.Path)}"); + exposed++; + continue; + } + + /* The claim is the re-read, not the call that returned without throwing. */ + if (DarlingFileSecurity.IsReadableByOrdinaryUsers(target.Path)) + { + error.WriteLine($" STILL READABLE {target.Path} ({target.What})"); + error.WriteLine($" {DarlingFileSecurity.DescribeOwnerAndExposure(target.Path)}"); + exposed++; + } + else + { + output.WriteLine($" SECURED {target.Path} ({target.What})"); + } + } + + output.WriteLine(); + + if (touched == 0) + { + error.WriteLine("Nothing to harden: no config and no store files were found at the expected paths."); + error.WriteLine($"Looked for the config at {resolvedConfig}. Pass an explicit path as the second argument."); + return 1; + } + + if (exposed > 0) + { + error.WriteLine($"{exposed} of {touched} item(s) are STILL readable by ordinary users."); + error.WriteLine("Run this from an ELEVATED prompt. If it was already elevated, the owner is the"); + error.WriteLine("problem: ownership carries WRITE_DAC, so take ownership first, then re-run."); + return 1; + } + + output.WriteLine($"All {touched} item(s) secured. The service re-asserts these ACLs at every start."); + return 0; + } + + /// Directory enumeration that treats an unreadable or missing folder as empty — this verb runs + /// precisely when permissions are broken, so a throw here would defeat its purpose. + private static IEnumerable SafeEnumerate(string? directory, string pattern) + { + if (string.IsNullOrEmpty(directory) || !Directory.Exists(directory)) + { + return Array.Empty(); + } + + try + { + return Directory.EnumerateFiles(directory, pattern).ToList(); + } + catch (Exception) + { + return Array.Empty(); + } + } + /// /// Creates or removes every scoped Darling firewall rule so the live firewall matches darling.json. /// Requires elevation (that is the entire point of the verb) and is idempotent — safe to re-run on every diff --git a/Darling/PerformanceMonitor.Darling.Service/Program.cs b/Darling/PerformanceMonitor.Darling.Service/Program.cs index 1300bcaf2d..b46c94ba3e 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Program.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Program.cs @@ -156,6 +156,24 @@ the running service only verifies. Reads darling.json only (no store, no credent return await DarlingCliCommands.ConfigureFirewallAsync(configPath, Console.Out, Console.Error, CancellationToken.None); } +/* CLI verb: re-apply the secret-file ACLs, elevated (#2352). The running service computes the correct DACL and + detects when the real one is wrong, but cannot apply it — re-ACLing a file it does not own needs WRITE_DAC and + taking ownership needs a privilege a virtual service account is not granted — so it logs the remedy and carries + on. This is the actor with the authority. Verifies every target after the attempt and exits non-zero if + anything is still readable, so it is usable in a provisioning script. Windows-only: ACLs. + Optional second arg = an explicit config path. */ +if (args.Length > 0 && DarlingCliCommands.IsHardenFilesVerb(args[0])) +{ + if (!OperatingSystem.IsWindows()) + { + Console.Error.WriteLine("--harden-files requires Windows (NTFS ACLs)."); + return 1; + } + + var configPath = args.Length > 1 ? args[1] : null; + return DarlingCliCommands.HardenFiles(configPath, Console.Out, Console.Error); +} + /* CLI verbs: enable/disable the embedded MCP + web-dashboard endpoints on a HEADLESS managed deployment. Each flips the live switch in config.config_service (mcp_enabled/web_enabled — the store is authoritative after the first run; darling.json's enabled is only the seed) via a targeted UPDATE whose self-bump trigger makes the