From 8ad26ae29b6b2e6b6b3bac8e84ba75cbd08578ea Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:30:29 -0400 Subject: [PATCH 1/4] Move firewall rule management to the elevated install path (#1771) The service runs as an unprivileged NT SERVICE virtual account that cannot modify Windows Firewall by design, but every start tried anyway: both New-NetFirewallRule and Remove-NetFirewallRule failed "Access is denied" for MCP (5152) and web (5153) on a production field instance. That was harmless there only because the rules pre-existed from a manual setup - on a fresh networked install nothing ever creates them, so remote clients are blocked outright, and networked is the normal deployment mode. Rule creation moves to the elevated context that already exists. A new --configure-firewall verb reconciles all three scoped rules (store, MCP, web) from darling.json in one pass; install-darling.ps1 runs it before the first start and again after -Network, and uninstall-darling.ps1 removes the rules by their shared DisplayName prefix. The verb reads only darling.json - no store, no credentials - so it works before the store has ever booted, and it is idempotent so an upgrade re-running it is a no-op. Exposure is decided by the same resolvers the running service fail-closes on, not by re-reading listen/allowFrom, so a config the service degrades to loopback never gets an open port and the service's own check cannot contradict what the installer just did. At runtime the write degrades to a read. Get-NetFirewallRule needs no elevation (verified on Windows 11 26200 under a restricted token: reads succeed, writes return PermissionDenied), so the service probes and reports once per state change rather than once per supervisor retry. A loopback-only install with no rule stays silent; an exposed endpoint missing its rule gets one WARN naming the exact command, built by the same builder the elevated verb runs. The bundled store had the identical bug on its own port and is fixed the same way. A service stop no longer attempts to remove the rule. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 14 + .../DarlingEndpointToggleCliTests.cs | 5 +- .../DarlingFirewallCheckTests.cs | 401 ++++++++++++++++++ .../DarlingCliCommands.cs | 248 ++++++++++- .../DarlingConfig.cs | 3 +- .../DarlingFirewallCheck.cs | 249 +++++++++++ .../DarlingManagedPostgres.cs | 101 ++--- .../Hosting/DarlingHostBinding.cs | 69 +-- .../Mcp/DarlingMcpHostService.cs | 58 +-- .../Mcp/DarlingWebHostService.cs | 47 +- .../Program.cs | 17 + Darling/README.md | 28 +- Darling/tools/install-darling.ps1 | 37 ++ Darling/tools/uninstall-darling.ps1 | 30 +- 14 files changed, 1117 insertions(+), 190 deletions(-) create mode 100644 Darling/Darling.Tests/DarlingFirewallCheckTests.cs create mode 100644 Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b641aa46a..eb961c06e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The service tried to open its own firewall on every start, could never succeed, and a fresh networked install got no rule at all** ([#1773], closes [#1771]) - a production field instance logged both halves of the firewall self-heal failing for BOTH endpoints on EVERY service start: `Remove-NetFirewallRule : Access is denied` and `New-NetFirewallRule : Access is denied`, for 5152 (MCP) and 5153 (web). It was harmless *there* only because the rules already existed from an earlier manual setup. The real defect is what happens without that accident: on a genuinely fresh networked install nothing ever creates the rule, so remote MCP and dashboard clients are firewalled off with only a warning in a log file to say why - and **networked is the normal deployment mode for this product, not the exception**. The cause is structural rather than a bug in the reconcile: the service runs as the `NT SERVICE\PerformanceMonitor Darling` virtual account, which cannot modify Windows Firewall **by design**, and granting it that privilege is the wrong fix - a monitoring service should not hold it. + + **Rule management moves to the elevated context that already existed.** A new `--configure-firewall` verb reconciles all three scoped rules - store, MCP, web - against `darling.json` in one pass, and `install-darling.ps1` runs it (before the first start, and again after `-Network`, since the wizard rewrites the exposure it just acted on); `uninstall-darling.ps1` removes them. The verb reads **only** `darling.json` - no store connection, no credentials - which is what lets it run at install time before the store has ever booted, unlike `--enable-mcp`/`--enable-web`, which write the control-plane store and need it initialized. It is idempotent, so an upgrade re-running it is a no-op. + + **The exposure decision is delegated, not re-derived, and that is the load-bearing choice.** The planner asks the same resolvers the running service fail-closes on (`ResolveNetworkExposure`, `ResolveMcpBind`, `ResolveWebBind`) rather than reading `listen` at face value. A config the service degrades to loopback - an unparseable `listen`, a missing or invalid `allowFrom`, a mismatched address family, a missing token, BYO mode - gets **no open port**, and the CIDR that reaches the rule is the parser's canonical output, never the operator's raw string (`IPNetwork.TryParse` *masks* host bits rather than rejecting them, so `192.168.1.77/24` opens `192.168.1.0/24`). Re-deriving it would have been worse than duplicated logic: the installer would open a port for an endpoint the service binds loopback-only, and then the service's own start-up check would flag, every start, a rule the installer had just deliberately created. + + **At runtime the write degrades to a read.** `Get-NetFirewallRule` needs no elevation - verified on Windows 11 26200 under a restricted token, where reads succeed and `New-NetFirewallRule` returns `PermissionDenied: Access is denied`, the exact field signature - so the service still tells the operator the truth without holding the privilege. It probes, then reports **once per state, not once per attempt** (the MCP and web hosts retry a failed start on a 30-second backoff, so a check wired where the reconcile sat would have traded access-denied spam for warning spam). A loopback-only install with no rule is the healthy default and logs **nothing at all**; an exposed endpoint whose rule is present logs one INFO; an exposed endpoint with no rule logs one WARN naming the exact `New-NetFirewallRule` to run, built by the same builder the elevated verb runs so the printed and applied commands cannot drift; a loopback-only endpoint with a rule still open logs one WARN naming the removal. An unreadable probe result is its own verdict rather than collapsing into "absent", which would manufacture a missing-rule warning out of a broken probe. The probe is also shaped so "absent" is an ANSWER and not a failure: an exact `-DisplayName` matching nothing is an `ObjectNotFound` **error**, and a suppressed error still exits 1, so the explicit `exit 0` is load-bearing - the same trap `BuildFirewallDisableCommand` already documents. + + **The bundled store had the identical bug on its own port** and is fixed the same way, though the issue named only 5152/5153. Two behaviors deliberately end: a service stop no longer removes the rule (it could never succeed, and the rule is install-managed now - it is scoped to a port nothing is listening on, and the next start reports it as stale), and `_runningNetworkMode`, which existed only to drive that removal, is gone. + + **Verified live, not just green.** The verb was run against real configs on a real firewall: an exposed MCP config created exactly one rule (`TCP 5152, inbound, RemoteAddress 192.168.1.0/24`) and left the other two absent; a fail-closed config (`allowFrom: "not-a-cidr"`) opened nothing, said why, and removed the stale rule; a loopback config removed all three; the machine was left with zero Darling rules. The exact probe command was run under a **non-elevated** token against both a present and an absent rule, returning `1` and `0` with a clean exit each time. Every guard was then verified RED by mutation, including one that first came back GREEN and exposed a weak test: restoring the runtime `New-NetFirewallRule` write; planning exposure from `listen` alone instead of the resolver; dropping the probe's `exit 0`; treating an unparseable probe as "absent"; reporting on every tick; emitting an open command with no CIDR; renaming the shared rule prefix that uninstall sweeps by; deleting the installer's invocation; dropping the verb from the dispatch allow-list; and reverting the store to writing its own rule. The installer pin **survived** its first mutation - `Contains("--configure-firewall")` stayed green with the real call deleted, because the script also names the verb in its help text and its error message - so it now pins the invocation form and proves the wrapper is called somewhere other than its own definition, and goes red both when the call is replaced and when every call site is removed + - **Anomaly baselines were being computed from a fraction of their intended history, silently** ([#1762]) - closing [#1757]. `PgBaselineProvider` asks for 30 days. `query_stats` keeps 4 under tiered retention, and the other eight baseline sources keep exactly 30, so on every tiered store the thresholds that decide what counts as an anomaly were built from far less data than designed with **no error raised**. The client-side timeouts that got this reported were the loud phase a store passes through on its way to that silence: once retention reaches a box the query stops timing out, because there is only 4 days left to scan. Severity was corrected during implementation - only `query_stats` sits behind the 4-day horizon (the TimescaleDB raw retention policy covers just `query_stats`, `procedure_stats` and `query_store_stats`, and only the first is a baseline source), so `query_duration` is a cliff at 4-vs-30 and the other ten families are a knife edge at 30-vs-30 - not broken today, but racing the purge at the oldest bucket and one config change away from the same degradation. **Nine continuous aggregates at the COLLECTION grain** now serve all eleven families: the hourly bucket is purely a partitioning and retention key and `collection_time` is a second GROUP BY column carrying the grain, so `AVG`, `STDDEV_SAMP` and the restart-exclusion `LAG` stay numerically identical instead of becoming a different statistic at a different scale. The existing hourly CAGG ladder **cannot** serve this for two independent reasons - wrong unit of observation, and it stores no sum-of-squares, so `AVG` reconstructs from sums and counts but standard deviation does not, and standard deviation is what sets the threshold. `cpu` and `file_io` therefore store sufficient statistics rather than an average: neither is one row per collection, because `CpuUtilizationCollector` issues `SELECT TOP (60)` over `RING_BUFFER_SCHEDULER_MONITOR` and appends every row under a single `collection_time`, so averaging first would silently redefine `sample_count` from samples to collections. The tier carries its own 35-day retention, deliberately longer than the 30-day window, and that relation is **pinned statically** because Storage cannot reference Analysis and it has no compile-time home - drop it below the window and the bug returns, silently. Two properties are required for the aggregates to actually hold history, both verified against TimescaleDB source rather than the docs: real-time aggregation is **opted into explicitly** (it has defaulted OFF since 2.13), and a coverage-gated backfill materializes whatever history the store already had. That backfill **verifies coverage after refreshing** instead of trusting that a successful `CALL` filled the aggregate, escalating to a forced refresh only on evidence: a refresh consumes invalidations, so a backfill cut short by a shutdown leaves a hole that a later plain refresh no-ops over while reporting success. That matters because the failure mode is **silence, not an error** - with real-time aggregation the watermark is a hard partition (`materialized WHERE time < watermark UNION ALL raw WHERE time >= watermark`), so an un-materialized region older than the watermark reads as absent rather than falling back to raw. No hand-rolled time slicing: manual `refresh_continuous_aggregate` already batches internally, each batch its own transaction, so partial progress survives an abort. The backfill is **launched, not awaited** - the composer tuning, delta re-seed and collection loop are all sequenced behind the TimescaleDB block, so awaiting it would take a restarted service dark for its whole duration, exactly when an operator is most likely to be restarting it - and a source-level test pins that, because adding an `await` there compiles, passes every functional test, and is invisible except on a large store. **No `CommandTimeout` bump anywhere**: the baseline queries keep the default, the fix is that they now read a tier sized for the question. Lite has no horizon bug (all nine of its baseline sources retain 30 days) so it keeps its raw-scan shape, but it gains the warning - lowering a source collector's retention below the baseline window would reintroduce the same silent degradation, and now says so once per metric. The mutation check is the test that earns its keep: point any family back at a raw `v_*` passthrough and the suite goes red, verified by actually reverting one. One more failure mode was caught by the live-PostgreSQL CI job, which is the only place it could show: without TimescaleDB the aggregates cannot exist, and the provider reads those relations **by name**, so a missing one throws, gets swallowed, and logs that same "Failed to compute baselines" line for **every** family - strictly worse than the bug being fixed. The nine relations are therefore also creatable as ordinary views, **derived from the aggregate's own select body** rather than written out again so the two supplies cannot drift, with `date_trunc('hour', ...)` standing in for the one TimescaleDB-only construct. The reverse transition is handled too - a store that later gains TimescaleDB has the stale fallback dropped first, guarded, because a continuous aggregate is itself a `relkind='v'` view and an unguarded `DROP VIEW` would destroy a materialized tier. - **The Lite AG alert guard now captures the data service before checking it** ([#1756]) - carries the guard refinement from [#1754], which was closed as superseded and would have lost it; the improvement is that PR author's. `EvaluateAvailabilityGroupAlertsAsync` runs UNAWAITED off a UI-timer tick and awaits twice, so checking the nullable field and then dereferencing it leaves a gap where it can be reassigned - and an NRE on a fire-and-forget task is an unobserved exception nobody sees, so the symptom would have been AG alerts silently not evaluating on some launches with nothing in the log. Capturing into a local first also matches the idiom the same file already uses for the tray service. ([#1755] carries the remaining `xUnit2031` holdout separately, under its own author.) - **A package with an OLDER PostgreSQL major no longer replaces a working newer runtime and takes the store down** ([#1752]) - closing [#1738] and [#1737] - `TryAdvanceRuntimeAsync` treated "the package's major DIFFERS from the extracted runtime's" as a reason to update, without checking which direction the difference went. A PostgreSQL 17 package landed beside an 18 store on DARLING01, the runtime was swapped, and the store was down about seven minutes until the previous runtime was restored by hand: no older postmaster can open a newer cluster. Nothing reached pg_upgrade, so no data was ever at risk - the store simply could not start. There is now a **direction check** against the data directory's own `PG_VERSION`, which is the authority on what the store needs AND is readable without executing anything - the reason the old checks all failed open is that they asked the binaries what they were, and on that host the binaries could not launch at all (`STATUS_DLL_NOT_FOUND`). It sits deliberately OUTSIDE the no-stamp branch, because the stamp records only that the zip CHANGED, never which way, so a stamped host receiving an older package would downgrade exactly the same. A refused package is left in place and its hash is deliberately NOT recorded, so the critical log repeats every start until the right zip is shipped rather than going quiet on the second one. **The second half of the same incident is also fixed**: the version check that should have caught it logged "data directory: 18, bundled runtime: unreadable - skipping the runtime version check. The store starts normally" one second before the bootstrap died. That degrade was backwards - the check could not run BECAUSE the binaries could not run, which is the strongest possible evidence they must not be used - so a KNOWN store major paired with an unidentifiable runtime now refuses to start, with a message naming the rescued runtime to restore instead of a raw Win32 status code. Also lands [#1737]'s deferred item: `RevertRuntime` now takes the data directory and the major it assumes, and refuses when `PG_VERSION` has already moved forward - redundant against today's two callers, and defence in depth against the third that forgets the flag, which is how #1738 happened by a different route. Every one of these is pinned, and the pins were mutation-checked: deleting the guard call, moving it inside the no-stamp branch, inverting either comparison, and deleting the revert guard each turn a test red - the direction comparison and the unidentifiable-runtime rule were extracted as PURE predicates precisely because inverting them in place left the entire suite green. @@ -1783,3 +1795,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1759]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1759 [#1767]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1767 [#1768]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1768 +[#1771]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1771 +[#1773]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1773 diff --git a/Darling/Darling.Tests/DarlingEndpointToggleCliTests.cs b/Darling/Darling.Tests/DarlingEndpointToggleCliTests.cs index c40378de8d..631726b242 100644 --- a/Darling/Darling.Tests/DarlingEndpointToggleCliTests.cs +++ b/Darling/Darling.Tests/DarlingEndpointToggleCliTests.cs @@ -239,8 +239,9 @@ public void OnlyAParsedCidrEverReachesTheFirewallCommand(string? allowFrom) [Fact] public void FirewallRuleNames_MatchTheDocumentedScopedDisplayNames() { - /* The CLI toggle verbs open/close these by DisplayName; the hosts self-reconcile the SAME names, so an - enable followed by a service restart is idempotent (no duplicate rule). Pin the exact strings. */ + /* The CLI toggle verbs and --configure-firewall open/close these by DisplayName, and the hosts CHECK + the SAME names at start-up (#1771), so an enable followed by a service restart is idempotent (no + duplicate rule) and the check never reports a rule the installer just created. Pin the exact strings. */ Assert.Equal("PerformanceMonitor Darling MCP (port 5152)", DarlingMcpHostService.McpFirewallRuleName(5152)); Assert.Equal("PerformanceMonitor Darling Web (port 5153)", DarlingWebHostService.WebFirewallRuleName(5153)); } diff --git a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs new file mode 100644 index 0000000000..56dff3f80a --- /dev/null +++ b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs @@ -0,0 +1,401 @@ +/* + * 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.Linq; +using PerformanceMonitor.Darling.Service; +using PerformanceMonitor.Darling.Service.Mcp; +using Xunit; + +namespace Darling.Tests; + +/// +/// #1771 — the service used to try to CREATE its own firewall rules from an unprivileged virtual account. +/// Every attempt failed "Access is denied" on a hardened box, and on a fresh networked install (the normal +/// deployment mode) the rule was never created at all, so remote clients were blocked outright. +/// +/// Rule management moved to the elevated install path; the runtime only VERIFIES. These pin both +/// halves: the read-only probe/verdict/remedy logic, the pure desired-state planner the elevated verb runs, +/// and — via source scans — the seams that would silently regress (a restored runtime write, a renamed rule +/// prefix that orphans rules on uninstall, an installer that stops calling the verb). +/// +public class DarlingFirewallCheckTests +{ + /* ---- the probe: read-only, and shaped so "absent" is an ANSWER, not a failure ---- */ + + [Fact] + public void BuildProbeCommand_ReadsOnly_AndNeverWrites() + { + var command = DarlingFirewallCheck.BuildProbeCommand("PerformanceMonitor Darling MCP (port 5152)"); + + Assert.Contains("Get-NetFirewallRule", command, StringComparison.Ordinal); + Assert.Contains("-DisplayName 'PerformanceMonitor Darling MCP (port 5152)'", command, StringComparison.Ordinal); + + /* The whole point of #1771: the running service issues NO firewall write. */ + Assert.DoesNotContain("New-NetFirewallRule", command, StringComparison.Ordinal); + Assert.DoesNotContain("Remove-NetFirewallRule", command, StringComparison.Ordinal); + Assert.DoesNotContain("Set-NetFirewallRule", command, StringComparison.Ordinal); + } + + [Fact] + public void BuildProbeCommand_TreatsAnAbsentRuleAsAnAnswer_NotAFailure() + { + var command = DarlingFirewallCheck.BuildProbeCommand("PerformanceMonitor Darling Web (port 5153)"); + + /* An exact DisplayName matching nothing is an ObjectNotFound ERROR from Get-NetFirewallRule (verified + on Windows 11 26200), and a suppressed error still exits 1 — the same trap the disable builder + documents. Without BOTH of these, every absent rule would read as a broken probe and warn. */ + Assert.Contains("-ErrorAction SilentlyContinue", command, StringComparison.Ordinal); + Assert.Contains("exit 0", command, StringComparison.Ordinal); + + /* One DisplayName can match several rules (Windows stores one per profile), and a single match has no + .Count without the array wrapper. */ + Assert.Contains("@(Get-NetFirewallRule", command, StringComparison.Ordinal); + Assert.Contains(DarlingFirewallCheck.ProbeSentinel, command, StringComparison.Ordinal); + } + + [Theory] + [InlineData("rule'; whoami; '")] + [InlineData("$(whoami)")] + [InlineData("x'; New-NetFirewallRule -DisplayName 'pwn' -RemoteAddress Any; '")] + public void BuildProbeCommand_KeepsAnyRuleNameInsideOneSingleQuotedLiteral(string hostile) + { + var command = DarlingFirewallCheck.BuildProbeCommand(hostile); + + /* Inside '…' PowerShell expands nothing, so the ONE way out is an unescaped quote: an odd total means + the value broke out. Same property the write builders carry (#1646), enforced on the read path too. */ + Assert.True((command.Split('\'').Length - 1) % 2 == 0, $"unbalanced quoting: {command}"); + Assert.Contains( + "-DisplayName '" + hostile.Replace("'", "''", StringComparison.Ordinal) + "'", + command, + StringComparison.Ordinal); + } + + [Theory] + [InlineData("DarlingFirewallProbe=0", false)] + [InlineData("DarlingFirewallProbe=1", true)] + [InlineData("DarlingFirewallProbe=3", true)] + [InlineData("noise before\r\nDarlingFirewallProbe=2\r\nnoise after", true)] + public void TryParseProbeOutput_ReadsTheCount(string output, bool expected) + => Assert.Equal(expected, DarlingFirewallCheck.TryParseProbeOutput(output)); + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("Get-NetFirewallRule : The term is not recognized")] + [InlineData("DarlingFirewallProbe=")] + [InlineData("DarlingFirewallProbe=x")] + public void TryParseProbeOutput_IsNullWhenItCannotTell(string? output) + { + /* Null must NOT collapse into "absent": that would manufacture a "rule is MISSING" warning out of a + broken probe. It becomes ProbeFailed, which says exactly what happened. */ + Assert.Null(DarlingFirewallCheck.TryParseProbeOutput(output)); + Assert.Equal( + FirewallRuleVerdict.ProbeFailed, + DarlingFirewallCheck.Classify(exposed: true, DarlingFirewallCheck.TryParseProbeOutput(output))); + } + + /* ---- the verdict table: the loopback-vs-networked gate ---- */ + + [Theory] + [InlineData(true, true, FirewallRuleVerdict.ExposedRulePresent)] + [InlineData(true, false, FirewallRuleVerdict.ExposedRuleMissing)] + [InlineData(false, true, FirewallRuleVerdict.LoopbackStaleRule)] + [InlineData(false, false, FirewallRuleVerdict.LoopbackNoRule)] + public void Classify_MapsExposureAndPresence(bool exposed, bool present, FirewallRuleVerdict expected) + => Assert.Equal(expected, DarlingFirewallCheck.Classify(exposed, present)); + + [Fact] + public void Classify_LoopbackWithNoRule_IsTheSilentHealthyDefault() + { + /* The constraint from #1771: a loopback-only install needs no rule and must produce NO warning. The + verdict itself carries that (it is the one with no remedy and no log line). */ + var verdict = DarlingFirewallCheck.Classify(exposed: false, present: false); + + Assert.Equal(FirewallRuleVerdict.LoopbackNoRule, verdict); + Assert.Null(DarlingFirewallCheck.BuildRemedyCommand(verdict, "PerformanceMonitor Darling MCP (port 5152)", 5152, null)); + } + + /* ---- the remedy: a WARN that names the exact command ---- */ + + [Fact] + public void BuildRemedyCommand_ForAMissingRule_IsTheExactScopedOpenCommand() + { + var remedy = DarlingFirewallCheck.BuildRemedyCommand( + FirewallRuleVerdict.ExposedRuleMissing, "PerformanceMonitor Darling MCP (port 5152)", 5152, "192.168.1.0/24"); + + Assert.NotNull(remedy); + Assert.Contains("New-NetFirewallRule", remedy, StringComparison.Ordinal); + Assert.Contains("-LocalPort 5152", remedy, StringComparison.Ordinal); + Assert.Contains("-RemoteAddress '192.168.1.0/24'", remedy, StringComparison.Ordinal); + + /* Built by the SAME builder the elevated verb runs, so the printed command and the applied one cannot + drift into being different rules. */ + Assert.Equal( + DarlingManagedPostgres.BuildFirewallEnableCommand("PerformanceMonitor Darling MCP (port 5152)", 5152, "192.168.1.0/24"), + remedy); + } + + [Fact] + public void BuildRemedyCommand_ForAStaleRule_RemovesOnly() + { + var remedy = DarlingFirewallCheck.BuildRemedyCommand( + FirewallRuleVerdict.LoopbackStaleRule, "PerformanceMonitor Darling Web (port 5153)", 5153, null); + + Assert.NotNull(remedy); + Assert.Contains("Remove-NetFirewallRule", remedy, StringComparison.Ordinal); + Assert.DoesNotContain("New-NetFirewallRule", remedy, StringComparison.Ordinal); + } + + [Theory] + [InlineData(FirewallRuleVerdict.ExposedRulePresent)] + [InlineData(FirewallRuleVerdict.LoopbackNoRule)] + [InlineData(FirewallRuleVerdict.ProbeFailed)] + public void BuildRemedyCommand_IsNullWhenThereIsNothingToFix(FirewallRuleVerdict verdict) + => Assert.Null(DarlingFirewallCheck.BuildRemedyCommand(verdict, "PerformanceMonitor Darling store (port 5641)", 5641, "10.0.0.0/8")); + + [Fact] + public void BuildRemedyCommand_RefusesToBuildAnOpenCommandWithoutACidr() + { + /* An exposed-but-CIDR-less config is one the service fail-closes anyway. Emitting an open command with + an empty -RemoteAddress would hand the operator a rule that opens the port to the world. */ + Assert.Null(DarlingFirewallCheck.BuildRemedyCommand( + FirewallRuleVerdict.ExposedRuleMissing, "PerformanceMonitor Darling MCP (port 5152)", 5152, null)); + Assert.Null(DarlingFirewallCheck.BuildRemedyCommand( + FirewallRuleVerdict.ExposedRuleMissing, "PerformanceMonitor Darling MCP (port 5152)", 5152, " ")); + } + + /* ---- no retry spam: report once per state, not once per supervisor tick ---- */ + + [Fact] + public void ShouldReport_OnlyWhenTheStateChanges() + { + const string rule = "PerformanceMonitor Darling MCP (port 5152)"; + + /* First observation always reports. */ + Assert.True(DarlingFirewallCheck.ShouldReport(null, null, rule, FirewallRuleVerdict.ExposedRuleMissing)); + + /* The supervisor re-attempts a failed start every 30s; the same verdict must stay quiet. */ + Assert.False(DarlingFirewallCheck.ShouldReport(rule, FirewallRuleVerdict.ExposedRuleMissing, rule, FirewallRuleVerdict.ExposedRuleMissing)); + + /* A real transition reports again — an admin created the rule. */ + Assert.True(DarlingFirewallCheck.ShouldReport(rule, FirewallRuleVerdict.ExposedRuleMissing, rule, FirewallRuleVerdict.ExposedRulePresent)); + + /* A port change makes a DIFFERENT rule; the new rule's state has never been stated. */ + Assert.True(DarlingFirewallCheck.ShouldReport( + rule, FirewallRuleVerdict.ExposedRulePresent, "PerformanceMonitor Darling MCP (port 6000)", FirewallRuleVerdict.ExposedRulePresent)); + } + + /* ---- the elevated planner: desired state per surface, from darling.json alone ---- */ + + [Fact] + public void PlanFirewallRules_LoopbackOnlyManagedConfig_OpensNothing() + { + var plans = DarlingCliCommands.PlanFirewallRules(ManagedConfig()); + + Assert.All(plans, p => Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, p.Action)); + + /* Removal is idempotent, so the loopback default is a clean no-op that also sweeps up a rule left + behind by an exposure that was turned off. */ + Assert.All(plans, p => Assert.Null(p.Cidr)); + } + + [Fact] + public void PlanFirewallRules_ExposedMcp_OpensTheScopedRule() + { + var config = ManagedConfig(); + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + var mcp = Assert.Single(PlansFor(config, "MCP")); + + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Open, mcp.Action); + Assert.Equal(DarlingMcpHostService.McpFirewallRuleName(config.Mcp.Port), mcp.RuleName); + Assert.Equal(config.Mcp.Port, mcp.Port); + Assert.Equal("192.168.1.0/24", mcp.Cidr); + + /* The other two surfaces are untouched by an MCP exposure. */ + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, Assert.Single(PlansFor(config, "web dashboard")).Action); + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, Assert.Single(PlansFor(config, "store")).Action); + } + + [Fact] + public void PlanFirewallRules_UsesTheParsersCanonicalCidr_NotTheRawString() + { + /* IPNetwork.TryParse MASKS host bits rather than rejecting them, so "validate then use the original" + would open a rule for a CIDR the parser had already decided meant something else. */ + var config = ManagedConfig(); + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.77/24", Token = "t" }; + + Assert.Equal("192.168.1.0/24", Assert.Single(PlansFor(config, "MCP")).Cidr); + } + + [Theory] + [InlineData(null, "t")] /* exposed listen, NO allowFrom -> the service fail-closes to loopback */ + [InlineData("not-a-cidr", "t")] /* exposed listen, junk allowFrom -> fail-closed */ + [InlineData("2001:db8::/32", "t")] /* address family disagrees with an IPv4 listen -> fail-closed */ + [InlineData("192.168.1.0/24", null)] /* no token -> the endpoint refuses to expose */ + public void PlanFirewallRules_NeverOpensAPortTheServiceWillNotExpose(string? allowFrom, string? token) + { + /* THE gate. The planner asks the SAME resolver the running service fail-closes on, so a config the + service degrades to loopback gets no open port — and the service's own start-up check reaches the + same verdict, instead of flagging a rule the installer had just created. */ + var config = ManagedConfig(); + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = allowFrom, Token = token }; + + var mcp = Assert.Single(PlansFor(config, "MCP")); + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, mcp.Action); + Assert.Null(mcp.Cidr); + Assert.NotNull(mcp.Note); /* and it says WHY, rather than silently doing nothing */ + } + + [Fact] + public void PlanFirewallRules_ByoMode_OpensNothingAndSkipsTheStore() + { + /* LAN exposure for MCP/web is managed-mode only, and in BYO the operator's own PostgreSQL governs the + store's exposure — so the installer must not open ports for a store it does not run. */ + var config = ManagedConfig(); + config.Postgres!.Managed = false; + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + var all = DarlingCliCommands.PlanFirewallRules(config); + + Assert.DoesNotContain(all, p => p.Surface == "store"); + Assert.All(all, p => Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, p.Action)); + Assert.NotNull(Assert.Single(PlansFor(config, "MCP")).Note); + } + + [Fact] + public void PlanFirewallRules_CoversAllThreeSurfaces_WithDistinctPortSpecificRules() + { + /* The issue named 5152/5153, but the bundled store had the identical structural bug on its own port. */ + var all = DarlingCliCommands.PlanFirewallRules(ManagedConfig()); + + Assert.Equal(3, all.Count); + Assert.Equal(3, all.Select(p => p.RuleName).Distinct(StringComparer.Ordinal).Count()); + Assert.All(all, p => Assert.StartsWith(DarlingFirewallCheck.RuleNamePrefix, p.RuleName, StringComparison.Ordinal)); + Assert.All(all, p => Assert.Contains($"(port {p.Port})", p.RuleName, StringComparison.Ordinal)); + } + + /* ---- drift guards: the seams a rename or a deleted call would break silently ---- */ + + [Fact] + public void EveryRuleNameBuilder_CarriesTheSharedPrefix_ThatUninstallSweepsBy() + { + Assert.StartsWith(DarlingFirewallCheck.RuleNamePrefix, DarlingManagedPostgres.StoreFirewallRuleName(5641), StringComparison.Ordinal); + Assert.StartsWith(DarlingFirewallCheck.RuleNamePrefix, DarlingMcpHostService.McpFirewallRuleName(5152), StringComparison.Ordinal); + Assert.StartsWith(DarlingFirewallCheck.RuleNamePrefix, DarlingWebHostService.WebFirewallRuleName(5153), StringComparison.Ordinal); + } + + [Fact] + public void UninstallScript_SweepsByTheSharedPrefix() + { + /* Uninstall removes rules by "*" so a rule from a PREVIOUS port is cleared too. That literal + lives in PowerShell, where no compiler checks it against the C# builders — this is that check. */ + var script = ReadRepoFile(Path.Combine("Darling", "tools", "uninstall-darling.ps1")); + + Assert.Contains($"Remove-NetFirewallRule -DisplayName '{DarlingFirewallCheck.RuleNamePrefix}*'", script, StringComparison.Ordinal); + } + + [Fact] + public void InstallScript_ActuallyInvokesTheFirewallVerb() + { + /* If the installer stops calling the verb, nothing creates the rules and #1771 is back — with the + runtime no longer even attempting it either. + This pins the INVOCATION, not a mention: the script also names --configure-firewall in its help + block and in the "re-run this by hand" error message, so a `Contains("--configure-firewall")` stays + green with the real call deleted. A mutation run caught exactly that, which is why the assertion is + the exact invocation form plus proof the wrapper is called somewhere other than its own definition. */ + var script = ReadRepoFile(Path.Combine("Darling", "tools", "install-darling.ps1")); + + Assert.Contains("& $serviceExe --configure-firewall", script, StringComparison.Ordinal); + + var mentions = script.Split("Invoke-FirewallReconcile", StringSplitOptions.None).Length - 1; + Assert.True(mentions >= 2, $"Invoke-FirewallReconcile is defined but never called (found {mentions} occurrence(s))"); + } + + [Theory] + [InlineData("DarlingMcpHostService.cs")] + [InlineData("DarlingWebHostService.cs")] + public void RuntimeHosts_VerifyTheFirewall_AndNeverWriteIt(string fileName) + { + /* The regression this whole change exists to prevent: a host that goes back to issuing the write it + cannot perform, producing access-denied on every start of every hardened install. */ + var source = ReadRepoFile(Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "Mcp", fileName)); + + Assert.Contains("DarlingFirewallCheck.CheckAsync", source, StringComparison.Ordinal); + Assert.DoesNotContain("BuildFirewallEnableCommand", source, StringComparison.Ordinal); + Assert.DoesNotContain("BuildFirewallDisableCommand", source, StringComparison.Ordinal); + Assert.DoesNotContain("ReconcileFirewallAsync", source, StringComparison.Ordinal); + } + + [Fact] + public void TheStoreBootstrap_VerifiesTheFirewall_AndNeverWritesItAtRuntime() + { + /* DarlingManagedPostgres still OWNS the pure command builders (the elevated verb calls them), so this + pins the runtime CALL SITE rather than the file's vocabulary: the bootstrap checks, and the old + "try to configure it myself" entry point is gone. */ + var source = ReadRepoFile(Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingManagedPostgres.cs")); + + Assert.Contains("CheckStoreFirewallAsync", source, StringComparison.Ordinal); + Assert.DoesNotContain("TryConfigureStoreFirewallAsync", source, StringComparison.Ordinal); + } + + [Fact] + public void ConfigureFirewallVerb_IsDispatchable() + { + /* A verb missing from the allow-list is classified UnknownOption and never reaches its handler — the + installer would then get "Unknown option" and exit non-zero on every install. */ + Assert.True(DarlingCliCommands.IsConfigureFirewallVerb("--configure-firewall")); + Assert.True(DarlingCliCommands.IsConfigureFirewallVerb("--CONFIGURE-FIREWALL")); + Assert.True(DarlingCliCommands.IsKnownVerb("--configure-firewall")); + Assert.Equal(StartupAction.RunKnownVerb, DarlingCliCommands.ClassifyStartupArgs(["--configure-firewall"])); + Assert.Contains("--configure-firewall", DarlingCliCommands.UsageText(), StringComparison.Ordinal); + } + + /* ---- helpers ---- */ + + /// A managed, fully loopback-only config — the default install, and the baseline every exposure + /// case above mutates one field of. + private static DarlingConfig ManagedConfig() + { + var config = new DarlingConfig(); + config.Postgres!.Managed = true; + return config; + } + + private static System.Collections.Generic.IEnumerable PlansFor(DarlingConfig config, string surface) + => DarlingCliCommands.PlanFirewallRules(config).Where(p => p.Surface == surface); + + private static string ReadRepoFile(string relativePath) + { + var root = FindRepoRoot(); + Assert.NotNull(root); + var path = Path.Combine(root, relativePath); + Assert.True(File.Exists(path), $"expected {path} to exist"); + return File.ReadAllText(path); + } + + /// Walks up from the test output directory to the repo root (the directory holding + /// PerformanceMonitor.sln) — the same idiom DocCommentHygieneTests uses. + private static string? FindRepoRoot() + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + for (var i = 0; i < 10 && directory is not null; i++) + { + if (File.Exists(Path.Combine(directory.FullName, "PerformanceMonitor.sln"))) + { + return directory.FullName; + } + + directory = directory.Parent; + } + + return null; + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index 97321d1f9f..203bf70d24 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -10,6 +10,7 @@ using System.Collections.Generic; using System.Globalization; using System.IO; +using System.Linq; using System.Net; using System.Net.NetworkInformation; using System.Net.Sockets; @@ -76,6 +77,12 @@ public static bool IsPrintViewerConnectionVerb(string arg) => public static bool IsConfigureNetworkVerb(string arg) => string.Equals(arg, "--configure-network", StringComparison.OrdinalIgnoreCase); + /// The verb handles — reconcile every scoped Darling firewall + /// rule from darling.json, elevated (#1771). This is the ONLY place rules are created; the service itself + /// only verifies them. + public static bool IsConfigureFirewallVerb(string arg) => + string.Equals(arg, "--configure-firewall", 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); @@ -114,6 +121,7 @@ public static bool IsKnownVerb(string arg) => || IsValidateConfigVerb(arg) || IsPrintViewerConnectionVerb(arg) || IsConfigureNetworkVerb(arg) + || IsConfigureFirewallVerb(arg) || IsEnableMcpVerb(arg) || IsDisableMcpVerb(arg) || IsEnableWebVerb(arg) @@ -181,6 +189,7 @@ public static string UsageText() => " PerformanceMonitor.Darling.Service.exe --encrypt-password Encrypt a SQL-auth password for darling.json (reads stdin)." + Environment.NewLine + " PerformanceMonitor.Darling.Service.exe --print-viewer-connection Print a remote-viewer connection string (managed store)." + 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 --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 + @@ -1424,7 +1433,7 @@ private static bool AskYesNo(TextReader input, TextWriter output, string label, --enable-mcp / --disable-mcp / --enable-web / --disable-web: headless endpoint bring-up. Two gaps these close on a headless box (no WPF Viewer, and the service runs as a virtual service - account that CANNOT modify Windows Firewall, so its best-effort self-reconcile silently fails): + account that CANNOT modify Windows Firewall, so the running service only VERIFIES its rules): (a) ENABLE/DISABLE an endpoint. After the first run mcp.enabled/web.enabled in darling.json are only a SEED; the store (config.config_service.mcp_enabled/web_enabled) is authoritative and is normally toggled only by the Viewer's Settings. These verbs write the store directly — a TARGETED UPDATE @@ -1691,7 +1700,7 @@ managed concerns. In BYO the operator's own PostgreSQL holds config_service — /// The firewall half of a toggle (defense-in-depth, never the boundary — pg_hba/token + the in-app CIDR /// check are). Only acts when the endpoint's darling.json network block opts into LAN exposure (a non-loopback /// listen, via the shared ). Uses the SAME scoped, - /// idempotent-by-DisplayName rule name the host's self-reconcile uses + /// idempotent-by-DisplayName rule name the host's start-up check looks for /// ( / ) /// and the SAME pure command builders. Elevated -> runs the rule; otherwise prints the exact elevated command /// — the store toggle already succeeded, so a non-elevated shell is a handoff, never a failure. A firewall @@ -1795,4 +1804,239 @@ private static async Task RunFirewallCommandAsync( error.WriteLine(" " + command); } } + + /* ================================================================================================ + --configure-firewall: the ELEVATED owner of every scoped Darling firewall rule (#1771). + + The service runs as an unprivileged virtual account and CANNOT create firewall rules. It used to try + on every start anyway, failing "Access is denied" each time; on a fresh networked install — the normal + deployment mode — that meant the rule was never created at all and remote clients were simply blocked. + Rule management therefore belongs to the elevated context that already exists: install-darling.ps1 + calls this verb, uninstall-darling.ps1 removes the rules, and the running service only VERIFIES + (DarlingFirewallCheck). + + It reconciles all THREE surfaces (store, MCP, web) in one pass, from darling.json alone: no store + connection, no credentials, so it is safe to run at install time before the store has ever booted — + unlike --enable-mcp/--enable-web, which write the control-plane store and therefore need it running. + ================================================================================================ */ + + /// What --configure-firewall will do to one surface's rule. + public enum FirewallRuleAction + { + /// The config exposes this surface on the LAN — create/refresh the scoped allow rule. + Open, + + /// The surface is loopback-only (by config, or fail-closed by its own resolver) — the desired + /// state is NO rule. Removal is idempotent, so this is also the no-op for a rule that never existed. + Remove, + } + + /// One surface's desired firewall state. is a human explanation printed + /// alongside, non-null only where the reason is not self-evident (a fail-closed exposure). + public readonly record struct FirewallRulePlan( + string Surface, string RuleName, int Port, FirewallRuleAction Action, string? Cidr, string? Note); + + /// + /// PURE desired-state for all three rules, so the whole decision pins without a live firewall. + /// Every surface is resolved by the SAME resolver the RUNNING service fail-closes on + /// (, + /// , ) + /// rather than by re-reading listen/allowFrom here. That is the load-bearing choice: a config + /// the service degrades to loopback (an unparseable listen, a mismatched address family, a missing CIDR) + /// must NOT get an open port, and the service's own start-up check must reach the same verdict this did — + /// otherwise every start would report a rule this verb had just deliberately created. + /// BYO mode resolves loopback for MCP/web (their resolvers take managed and refuse exposure + /// without it) and skips the store entirely, whose exposure the operator's own PostgreSQL governs. + /// + [SupportedOSPlatform("windows")] + public static IReadOnlyList PlanFirewallRules(DarlingConfig config) + { + var plans = new List(); + var managed = config.Postgres?.Managed ?? false; + + if (managed && config.Postgres is not null) + { + var (certPath, keyPath) = ResolveStoreCertPaths(config.Postgres); + var store = DarlingManagedPostgres.ResolveNetworkExposure(config.Postgres.Network, certPath, keyPath); + plans.Add(new FirewallRulePlan( + "store", + DarlingManagedPostgres.StoreFirewallRuleName(config.Postgres.Port), + config.Postgres.Port, + store.Exposed ? FirewallRuleAction.Open : FirewallRuleAction.Remove, + store.Exposed ? store.Cidr : null, + store.DegradeReason)); + } + + var mcpBind = DarlingMcpHostService.ResolveMcpBind(config.Mcp, managed); + var mcpExposed = mcpBind.Mode == DarlingMcpHostService.McpBindMode.NetworkAndLoopback; + plans.Add(new FirewallRulePlan( + "MCP", + DarlingMcpHostService.McpFirewallRuleName(config.Mcp.Port), + config.Mcp.Port, + mcpExposed ? FirewallRuleAction.Open : FirewallRuleAction.Remove, + mcpExposed ? CanonicalCidrOrNull(config.Mcp.Network?.AllowFrom) : null, + mcpExposed ? null : DescribeLoopbackReason(mcpBind.Reason, "mcp"))); + + var webBind = DarlingWebHostService.ResolveWebBind(config.Web, managed); + var webExposed = webBind.Mode == DarlingHostBinding.BindMode.NetworkAndLoopback; + plans.Add(new FirewallRulePlan( + "web dashboard", + DarlingWebHostService.WebFirewallRuleName(config.Web.Port), + config.Web.Port, + webExposed ? FirewallRuleAction.Open : FirewallRuleAction.Remove, + webExposed ? CanonicalCidrOrNull(config.Web.Network?.AllowFrom) : null, + webExposed ? null : DescribeLoopbackReason((DarlingMcpHostService.McpBindReason)webBind.Reason, "web"))); + + return plans; + } + + /// The parser's canonical CIDR, or null when it will not parse. The bind resolvers have already + /// refused exposure in the null case, so an Open plan always carries a real CIDR; this never forwards the + /// caller's raw string into a PowerShell command (#1646). + private static string? CanonicalCidrOrNull(string? allowFrom) => + ClassifyAllowFrom(allowFrom, out var canonical) == EndpointAllowFromVerdict.Valid ? canonical : null; + + /// Why a surface is loopback-only, when the reason is a DEGRADE worth printing. A plain + /// loopback-by-default config is the normal case and gets no note. + private static string? DescribeLoopbackReason(DarlingMcpHostService.McpBindReason reason, string section) => + reason switch + { + DarlingMcpHostService.McpBindReason.TokenMissing => + $"{section}.network is set but its token is missing or unreadable, so the service fail-closes this endpoint to loopback", + DarlingMcpHostService.McpBindReason.AllowFromInvalid => + $"{section}.network.allowFrom is missing or not a valid CIDR, so the service fail-closes this endpoint to loopback", + DarlingMcpHostService.McpBindReason.ManagedModeRequired => + $"{section}.network is set but postgres.managed = false; LAN exposure is managed-mode only and is ignored", + _ => 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")] + public static async Task ConfigureFirewallAsync( + string? configPath, TextWriter output, TextWriter error, CancellationToken cancellationToken) + { + DarlingConfig config; + try + { + config = DarlingConfig.Load(configPath); + } + catch (Exception ex) + { + error.WriteLine($"Could not load configuration: {ex.Message}"); + return 1; + } + + var plans = PlanFirewallRules(config); + var toOpen = plans.Count(p => p.Action == FirewallRuleAction.Open); + + output.WriteLine("Reconciling the scoped Windows Firewall rules to match darling.json."); + output.WriteLine( + "These rules are managed HERE, elevated. The service account cannot create them by design, so the " + + "running service only verifies them and reports what it finds."); + output.WriteLine(); + + if (!IsElevated()) + { + /* Not elevated. When nothing is exposed there is genuinely nothing to do and a hard failure would + be a lie (and would fail an otherwise fine loopback install); when something IS exposed this is a + real, actionable failure, so exit non-zero AND print every command to run by hand. */ + if (toOpen == 0) + { + output.WriteLine( + "Every endpoint is loopback-only, so no firewall rule is needed and none was changed. " + + "(This shell is not elevated, but there was nothing to do.)"); + return 0; + } + + error.WriteLine("This shell is not elevated, so NO firewall rule was changed. Run these in an ELEVATED PowerShell:"); + foreach (var plan in plans.Where(p => p.Action == FirewallRuleAction.Open)) + { + error.WriteLine(" " + DarlingManagedPostgres.BuildFirewallEnableCommand(plan.RuleName, plan.Port, plan.Cidr!)); + } + + return 1; + } + + var failures = 0; + foreach (var plan in plans) + { + if (plan.Note is not null) + { + output.WriteLine($"{plan.Surface}: {plan.Note}."); + } + + if (plan.Action == FirewallRuleAction.Remove) + { + /* Loopback-only: the desired state is no rule. Removal is idempotent (an absent rule exits 0), + so this both cleans up after an exposure that was turned off and no-ops on a fresh install. */ + if (!await TryRunFirewallStepAsync( + DarlingManagedPostgres.BuildFirewallDisableCommand(plan.RuleName), + $"{plan.Surface}: loopback-only — no rule needed (removed '{plan.RuleName}' if it existed).", + $"{plan.Surface}: could not remove the rule '{plan.RuleName}'", + output, error, cancellationToken)) + { + failures++; + } + + continue; + } + + if (!await TryRunFirewallStepAsync( + DarlingManagedPostgres.BuildFirewallEnableCommand(plan.RuleName, plan.Port, plan.Cidr!), + $"{plan.Surface}: opened '{plan.RuleName}' (TCP {plan.Port}, inbound, from {plan.Cidr}).", + $"{plan.Surface}: could not open the rule '{plan.RuleName}'", + output, error, cancellationToken)) + { + failures++; + } + } + + output.WriteLine(); + if (failures > 0) + { + error.WriteLine($"{failures} firewall rule(s) could not be reconciled — see the commands above."); + return 1; + } + + output.WriteLine(toOpen == 0 + ? "Done. Every endpoint is loopback-only, so no port was opened." + : $"Done. {toOpen} endpoint(s) exposed on the LAN; their scoped rules are in place."); + return 0; + } + + /// Runs one reconcile step and reports it. Returns false on failure, after printing the exact + /// command so an operator can finish by hand. Never throws except on cancellation. + [SupportedOSPlatform("windows")] + private static async Task TryRunFirewallStepAsync( + string command, string successLine, string failurePrefix, TextWriter output, TextWriter error, CancellationToken cancellationToken) + { + try + { + var (exitCode, psOutput) = await DarlingManagedPostgres.RunPowerShellAsync(command, cancellationToken); + if (exitCode == 0) + { + output.WriteLine(successLine); + return true; + } + + error.WriteLine($"{failurePrefix} (exit {exitCode}: {psOutput}). Run this by hand:"); + error.WriteLine(" " + command); + return false; + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) + { + error.WriteLine($"{failurePrefix} ({ex.Message}). Run this by hand:"); + error.WriteLine(" " + command); + return false; + } + } } diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs index f095cfe4b7..674ebbaf67 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs @@ -558,7 +558,8 @@ public sealed class McpConfig /// service reconciles it on every start (managed mode only): listen_addresses gains the bind IP, a /// self-signed TLS cert is generated for verify-full, a marked /// hostssl darling <role> <allowFrom> scram-sha-256 pg_hba block is written + -/// reloaded, and a best-effort firewall rule is added. Reconciliation is symmetric (removing the +/// reloaded, and the scoped firewall rule (created by the elevated installer, #1771) is checked. +/// Reconciliation is symmetric (removing the /// block closes the box) and fail-closed: any invalid/incomplete field degrades the store to /// loopback + LogCritical, never exposed. These rules are enforced at the point of use, NOT in the /// all-fatal (D-validate). diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs new file mode 100644 index 0000000000..dc04c9c28c --- /dev/null +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs @@ -0,0 +1,249 @@ +/* + Firewall rule VERIFICATION for the running service (#1771). + + The service runs as the `NT SERVICE\PerformanceMonitor Darling` virtual account, which by design cannot + modify Windows Firewall. Every surface that opted into LAN exposure used to try anyway on each start — + New-NetFirewallRule / Remove-NetFirewallRule — and every attempt failed with "Access is denied" on a + hardened box. On a field instance that was merely log noise (the rules already existed from a manual + setup); on a genuinely FRESH networked install nothing would create the rule at all, so the endpoint + would be firewalled off with only a warning to say so. Networked is the normal deployment mode. + + The rules are therefore created by the ELEVATED install path (install-darling.ps1 -> the exe's + --configure-firewall verb) and removed by uninstall-darling.ps1. What is left here is a READ-ONLY probe: + Get-NetFirewallRule needs no elevation (verified on Windows 11 26200 under a restricted token: reads + succeed, writes return PermissionDenied), so the service can still tell the operator the truth without + holding a privilege it should not have. + + Everything except CheckAsync is pure, so the probe shape, the parse, the verdict table, and the remedy + command all pin without a live firewall. +*/ + +using System; +using System.Globalization; +using System.Runtime.Versioning; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; + +namespace PerformanceMonitor.Darling.Service; + +/// What a firewall probe found, against what the config asked for. Public (where the rest of +/// is internal) only so the test project can name it in a [Theory] +/// parameter — xUnit needs public test methods, and a public method cannot take an internal type. +public enum FirewallRuleVerdict +{ + /// Loopback-only and no rule exists — the healthy default install. Reported nowhere; there is + /// nothing for an operator to do and a line here would be noise on every start of every default box. + LoopbackNoRule, + + /// Loopback-only but a scoped rule is still open — a stale allow rule left behind by an exposure + /// that was turned off by hand. Nothing is listening on the network so it grants no access, but it is + /// drift worth one line naming the removal. + LoopbackStaleRule, + + /// Exposed and the rule is present — the healthy networked install. One INFO line, so an + /// operator can see the check ran and passed. + ExposedRulePresent, + + /// Exposed but NO rule exists — the case that used to fail silently on a fresh install. One WARN + /// naming the exact elevated command. + ExposedRuleMissing, + + /// The probe itself could not run or could not be parsed. Reported once, never retried. + ProbeFailed, +} + +/// +/// Read-only Windows Firewall verification for the three optionally-exposed Darling surfaces (the bundled +/// store, the MCP endpoint, the web dashboard). See the file header for why this replaced the old +/// attempt-and-fail self-heal. +/// Windows-only in whole: every member is about Windows Firewall, and the pure builders compose +/// 's own Windows-only command builders. The test project targets +/// net10.0-windows, so the pure members still pin directly. +/// +[SupportedOSPlatform("windows")] +internal static class DarlingFirewallCheck +{ + /// + /// The sentinel the probe prints, so parsing keys off something this code owns rather than off + /// PowerShell's object formatting (which is culture- and host-dependent). + /// + internal const string ProbeSentinel = "DarlingFirewallProbe="; + + /// + /// The shared DisplayName prefix every Darling firewall rule carries. Uninstall removes rules by + /// "{prefix}*" rather than reconstructing each port-specific name, so this constant is the seam — + /// DarlingFirewallCheckTests pins it against all three real rule-name builders AND against the + /// literal in uninstall-darling.ps1, so a rename cannot silently orphan rules on uninstall. + /// + internal const string RuleNamePrefix = "PerformanceMonitor Darling "; + + /// + /// PURE probe command: count the rules with this DisplayName and print the count behind + /// . Deliberately shaped to ALWAYS exit 0 and never throw: + /// + /// An exact DisplayName that matches nothing is an ObjectNotFound ERROR from + /// Get-NetFirewallRule, not an empty result — "absent" is a normal, expected answer here, so it must not + /// look like a failure. -ErrorAction SilentlyContinue suppresses the text. + /// ...but a suppressed error still leaves powershell.exe exiting 1 (the same trap + /// documents), so the explicit + /// exit 0 is load-bearing: without it every absent rule would read as a failed probe. + /// The result is wrapped in @(...) because one DisplayName can match SEVERAL rules (Windows + /// stores one per profile) and a single match would otherwise have no .Count. + /// + /// The name goes through the same single-quoted-literal escaping as the write builders. + /// + internal static string BuildProbeCommand(string ruleName) => + $"$c = @(Get-NetFirewallRule -DisplayName {DarlingManagedPostgres.SingleQuotedPowerShell(ruleName)} " + + $"-ErrorAction SilentlyContinue).Count; Write-Output ('{ProbeSentinel}' + $c); exit 0"; + + /// + /// PURE parse of output: true/false when the sentinel carried a count, + /// null when it is absent or unparseable (PowerShell missing, the command died, a truncated pipe). A null + /// is , never a silent "absent" — guessing "absent" would + /// manufacture a bogus WARN, and guessing "present" would hide a real one. + /// + internal static bool? TryParseProbeOutput(string? output) + { + if (string.IsNullOrEmpty(output)) + { + return null; + } + + var start = output.IndexOf(ProbeSentinel, StringComparison.Ordinal); + if (start < 0) + { + return null; + } + + var digits = start + ProbeSentinel.Length; + var end = digits; + while (end < output.Length && char.IsAsciiDigit(output[end])) + { + end++; + } + + return end > digits + && int.TryParse(output[digits..end], NumberStyles.None, CultureInfo.InvariantCulture, out var count) + ? count > 0 + : null; + } + + /// PURE verdict table: what the config asked for vs. what the probe found. + internal static FirewallRuleVerdict Classify(bool exposed, bool? present) => + present is null ? FirewallRuleVerdict.ProbeFailed + : exposed + ? (present.Value ? FirewallRuleVerdict.ExposedRulePresent : FirewallRuleVerdict.ExposedRuleMissing) + : (present.Value ? FirewallRuleVerdict.LoopbackStaleRule : FirewallRuleVerdict.LoopbackNoRule); + + /// + /// PURE: the elevated command that fixes this verdict, or null when there is nothing to fix. This is what + /// makes the WARN actionable — the operator gets the exact rule to run, not a description of one. Built by + /// the SAME builders the elevated --configure-firewall verb runs, so the printed command and the + /// applied command can never drift. + /// + internal static string? BuildRemedyCommand(FirewallRuleVerdict verdict, string ruleName, int port, string? cidr) => + verdict switch + { + FirewallRuleVerdict.ExposedRuleMissing when !string.IsNullOrWhiteSpace(cidr) => + DarlingManagedPostgres.BuildFirewallEnableCommand(ruleName, port, cidr), + FirewallRuleVerdict.LoopbackStaleRule => + DarlingManagedPostgres.BuildFirewallDisableCommand(ruleName), + _ => null, + }; + + /// + /// PURE dedup gate: report only when the (rule, verdict) pair CHANGES. The MCP and web hosts run under a + /// supervisor that re-attempts a failed start with backoff, so a check wired where the old reconcile sat + /// would otherwise repeat its WARN on every attempt — the retry spam #1771 explicitly rules out. A steady + /// state is stated once; a real transition (rule created by an admin, exposure turned off) reports again. + /// + internal static bool ShouldReport(string? lastRuleName, FirewallRuleVerdict? lastVerdict, string ruleName, FirewallRuleVerdict verdict) => + lastVerdict != verdict || !string.Equals(lastRuleName, ruleName, StringComparison.Ordinal); + + /// + /// Runs the read-only probe and reports the verdict once per state change (see ). + /// NEVER writes a rule and NEVER throws except on cancellation — a monitoring service must not fail to + /// start because it could not read the firewall. Callers pass their own last-reported state and store the + /// returned value back. + /// + internal static async Task<(string RuleName, FirewallRuleVerdict Verdict)> CheckAsync( + string ruleName, + int port, + bool exposed, + string? cidr, + string? lastRuleName, + FirewallRuleVerdict? lastVerdict, + ILogger logger, + CancellationToken cancellationToken) + { + FirewallRuleVerdict verdict; + try + { + var (exitCode, output) = await DarlingManagedPostgres.RunPowerShellAsync( + BuildProbeCommand(ruleName), cancellationToken); + + /* The probe exits 0 for both answers, so a non-zero exit means the probe itself broke; do not read + a count out of a run that failed. */ + verdict = Classify(exposed, exitCode == 0 ? TryParseProbeOutput(output) : null); + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) + { + logger.LogDebug("Firewall probe for '{Rule}' could not run: {Message}", ruleName, ex.Message); + verdict = FirewallRuleVerdict.ProbeFailed; + } + + if (!ShouldReport(lastRuleName, lastVerdict, ruleName, verdict)) + { + return (ruleName, verdict); + } + + var remedy = BuildRemedyCommand(verdict, ruleName, port, cidr); + switch (verdict) + { + case FirewallRuleVerdict.ExposedRulePresent: + logger.LogInformation( + "Firewall rule '{Rule}' is present. The service verifies this rule but never modifies it — " + + "firewall rules are created by the elevated installer (install-darling.ps1).", + ruleName); + break; + + case FirewallRuleVerdict.ExposedRuleMissing: + logger.LogWarning( + "Firewall rule '{Rule}' is MISSING and this endpoint is configured for LAN access on port {Port}, " + + "so remote clients will be blocked. The service account cannot create it by design. Run " + + "PerformanceMonitor.Darling.Service.exe --configure-firewall from an ELEVATED PowerShell (it " + + "reconciles every Darling rule from darling.json), or run just this one:\n{Command}", + ruleName, port, remedy ?? "(no command — allowFrom is missing or not a valid CIDR; run --configure-network)"); + break; + + case FirewallRuleVerdict.LoopbackStaleRule: + logger.LogWarning( + "Firewall rule '{Rule}' is still open but this endpoint now binds LOOPBACK ONLY, so the rule is " + + "stale. Nothing is listening on the network, so it grants no access. Remove it with " + + "PerformanceMonitor.Darling.Service.exe --configure-firewall from an ELEVATED PowerShell, or " + + "run just this one:\n{Command}", + ruleName, remedy); + break; + + case FirewallRuleVerdict.ProbeFailed: + logger.LogWarning( + "Could not verify firewall rule '{Rule}' (the read-only probe did not return a usable answer). " + + "This does not affect the endpoint; verify the rule by hand with " + + "Get-NetFirewallRule -DisplayName '{Rule}'.", + ruleName, ruleName); + break; + + case FirewallRuleVerdict.LoopbackNoRule: + default: + /* Loopback-only with no rule is the healthy default — deliberately silent. */ + break; + } + + return (ruleName, verdict); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs index 3782f2c898..957940b414 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs @@ -1931,17 +1931,11 @@ private async Task ReconcileNetworkAsync( await GuardAdoptedListenAsync(ownerConnectionString, plan, cancellationToken); } - /* Defense-in-depth firewall rule (the boundary is pg_hba + TLS). Best-effort, never fatal. Enable - when exposed; on a disable EDGE (we just removed the block) also remove the rule. A store that - was never exposed touches the firewall not at all (the early return above). */ - if (exposed) - { - await TryConfigureStoreFirewallAsync(enable: true, plan.Cidr!, cancellationToken); - } - else - { - await TryConfigureStoreFirewallAsync(enable: false, cidr: null, cancellationToken); - } + /* Defense-in-depth firewall rule (the boundary is pg_hba + TLS). CHECKED, never written (#1771): + an exposed store reports a missing rule, and a disable EDGE (we just removed the block) reports + the now-stale rule, each naming the elevated command. A store that was never exposed touches the + firewall not at all (the early return above). */ + await CheckStoreFirewallAsync(exposed, exposed ? plan.Cidr : null, cancellationToken); } catch (OperationCanceledException) { @@ -2112,8 +2106,10 @@ private static bool LiveListenHasNonLoopback(string liveListenAddresses) return false; } - /// The scoped store firewall rule name (idempotent by DisplayName), port-specific. - private string StoreFirewallRuleName => $"PerformanceMonitor Darling store (port {_config.Port})"; + /// The scoped store firewall rule name (idempotent by DisplayName), port-specific. + /// internal static so the elevated --configure-firewall verb creates the SAME rule this + /// checks for, from darling.json alone and without a running store (#1771). + internal static string StoreFirewallRuleName(int port) => $"PerformanceMonitor Darling store (port {port})"; /// /// PowerShell single-quoted literal. Inside '…' PowerShell expands nothing — no $, no @@ -2122,15 +2118,17 @@ private static bool LiveListenHasNonLoopback(string liveListenAddresses) /// are then safe no matter what a caller hands them, INDEPENDENT of the caller-side CIDR parse that is /// the primary fix. The rule names are internally generated and contain no quotes, so quoting them /// leaves the emitted command byte-for-byte what it has always been. + /// internal so escapes the rule name + /// through this same one helper rather than growing a second, subtly different quoting rule (#1771). /// - private static string SingleQuoted(string value) + internal static string SingleQuotedPowerShell(string value) => "'" + value.Replace("'", "''", StringComparison.Ordinal) + "'"; /// Idempotent-named enable command (remove-by-name then add) — the exact scoped command the docs /// lead with (D1). Pure + testable. internal static string BuildFirewallEnableCommand(string ruleName, int port, string remoteCidr) - => $"Remove-NetFirewallRule -DisplayName {SingleQuoted(ruleName)} -ErrorAction SilentlyContinue; " + - $"New-NetFirewallRule -DisplayName {SingleQuoted(ruleName)} -Direction Inbound -Action Allow -Protocol TCP -LocalPort {port} -RemoteAddress {SingleQuoted(remoteCidr)} | Out-Null"; + => $"Remove-NetFirewallRule -DisplayName {SingleQuotedPowerShell(ruleName)} -ErrorAction SilentlyContinue; " + + $"New-NetFirewallRule -DisplayName {SingleQuotedPowerShell(ruleName)} -Direction Inbound -Action Allow -Protocol TCP -LocalPort {port} -RemoteAddress {SingleQuotedPowerShell(remoteCidr)} | Out-Null"; /// /// Idempotent-named disable command (remove-by-name). Pure + testable. @@ -2144,64 +2142,33 @@ internal static string BuildFirewallEnableCommand(string ruleName, int port, str /// The catch alone is not enough: a caught error leaves the exit state non-zero, hence the exit 0. /// internal static string BuildFirewallDisableCommand(string ruleName) - => $"try {{ Remove-NetFirewallRule -DisplayName {SingleQuoted(ruleName)} -ErrorAction Stop }} " + + => $"try {{ Remove-NetFirewallRule -DisplayName {SingleQuotedPowerShell(ruleName)} -ErrorAction Stop }} " + $"catch {{ if ($_.CategoryInfo.Category -ne 'ObjectNotFound') {{ throw }} }}; exit 0"; + /// Last (rule, verdict) reported, so a repeated network reconcile restates a steady firewall + /// state at most once (). + private string? _lastFirewallRule; + private FirewallRuleVerdict? _lastFirewallVerdict; + /// - /// Best-effort firewall reconcile (D1): add/remove the scoped, idempotent-named inbound rule via - /// PowerShell. The firewall is defense-in-depth, NOT the boundary (pg_hba + TLS are), so a failure (no - /// elevation, PowerShell missing) logs the exact scoped command for the operator to run by hand and - /// NEVER fails startup. + /// Read-only firewall VERIFICATION for the store's scoped rule (#1771). This used to add/remove the rule + /// itself, which the service account cannot do — the identical structural bug the MCP and web hosts had, + /// fixed the same way: the elevated installer creates the rule, and the running service only reports what + /// it finds. The firewall is defense-in-depth, NOT the boundary (pg_hba + TLS are), so a check that cannot + /// run NEVER fails startup. /// - private async Task TryConfigureStoreFirewallAsync(bool enable, string? cidr, CancellationToken cancellationToken) - { - var ruleName = StoreFirewallRuleName; - var command = enable - ? BuildFirewallEnableCommand(ruleName, _config.Port, cidr!) - : BuildFirewallDisableCommand(ruleName); - - try - { - var (exitCode, output) = await RunPowerShellAsync(command, cancellationToken); - if (exitCode == 0) - { - _logger.LogInformation("Firewall rule '{Rule}' {Verb}.", ruleName, enable ? "ensured" : "removed"); - } - else if (enable) - { - _logger.LogWarning( - "Could not configure the firewall automatically (exit {ExitCode}: {Output}). Run this in an elevated PowerShell:\n{Command}", - exitCode, output, command); - } - else - { - _logger.LogWarning("Could not remove the firewall rule automatically (exit {ExitCode}: {Output}).", exitCode, output); - } - } - catch (OperationCanceledException) - { - throw; - } - catch (Exception ex) - { - if (enable) - { - _logger.LogWarning( - "Could not configure the firewall automatically ({Message}). Run this in an elevated PowerShell:\n{Command}", - ex.Message, command); - } - else - { - _logger.LogWarning("Could not remove the firewall rule automatically ({Message}).", ex.Message); - } - } - } + [SupportedOSPlatform("windows")] + private async Task CheckStoreFirewallAsync(bool exposed, string? cidr, CancellationToken cancellationToken) + => (_lastFirewallRule, _lastFirewallVerdict) = await DarlingFirewallCheck.CheckAsync( + StoreFirewallRuleName(_config.Port), _config.Port, exposed, cidr, + _lastFirewallRule, _lastFirewallVerdict, _logger, cancellationToken); /// /// Runs a PowerShell command with captured, interleaved stdout+stderr and a timeout. internal - /// so the (separate) MCP host can reuse it for its own best-effort firewall reconcile - /// (darling-network-endpoints) instead of duplicating it — the firewall command shape is shared via the - /// pure / builders. + /// so the elevated --configure-firewall verb and the runtime firewall CHECK + /// () reuse it instead of duplicating it — the command shapes are shared + /// via the pure / + /// builders. /// is optional and defaults to the shared status timeout /// (); the --configure-network wizard passes a longer one for a /// service restart, which routinely exceeds the status budget. Existing callers are unaffected. diff --git a/Darling/PerformanceMonitor.Darling.Service/Hosting/DarlingHostBinding.cs b/Darling/PerformanceMonitor.Darling.Service/Hosting/DarlingHostBinding.cs index 26d0230f50..89ebb326e3 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Hosting/DarlingHostBinding.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Hosting/DarlingHostBinding.cs @@ -22,15 +22,15 @@ namespace PerformanceMonitor.Darling.Service.Hosting; /// The surface-agnostic bind/auth helpers shared by the two optional Kestrel hosts — the MCP host /// () and the web host (). /// Extracted so the LAN-exposure decision ladder, the in-app CIDR check, the loopback-listener collision -/// guard, the constant-time token compare, and the best-effort firewall reconcile live ONCE, with ONE test -/// suite, instead of being copy-pasted per surface (darling-network-endpoints anti-drift). The MCP host is -/// refactored onto this class (its behavior byte-for-byte unchanged — its pre-existing tests stay green -/// untouched, the refactor proof); the web host is built on it from the start. +/// guard, and the constant-time token compare live ONCE, with ONE test suite, instead of being copy-pasted +/// per surface (darling-network-endpoints anti-drift). The MCP host is refactored onto this class (its +/// behavior byte-for-byte unchanged — its pre-existing tests stay green untouched, the refactor proof); the +/// web host is built on it from the start. /// -/// Everything here is PURE (no logger, no server) except , which is -/// effectful but parameterized on the rule name + logger so both surfaces reconcile their own scoped rule. -/// The (mode, reason) split lets the caller map a reason to a log severity (Round-4 #7) while the pure ladder -/// stays free of side effects. +/// Everything here is PURE (no logger, no server). It used to also host the best-effort firewall +/// reconcile; that moved to when the runtime stopped writing rules and +/// started only verifying them (#1771). The (mode, reason) split lets the caller map a reason to a log +/// severity (Round-4 #7) while the pure ladder stays free of side effects. /// internal static class DarlingHostBinding { @@ -207,57 +207,4 @@ internal static bool FixedTimeTokenEquals(string? presented, string? expected) var presentedHash = SHA256.HashData(Encoding.UTF8.GetBytes(presented)); return CryptographicOperations.FixedTimeEquals(expectedHash, presentedHash); } - - /// - /// Best-effort firewall reconcile (defense-in-depth): add/remove the scoped, idempotent-named inbound rule - /// via PowerShell, reusing the store's tested pure command builders and its shared PowerShell runner (no - /// duplication, no timeout divergence). Parameterized on so each surface - /// reconciles its own port-specific rule. NEVER fatal — a failure (no elevation, PowerShell missing) logs - /// the exact scoped command for the operator to run by hand. The token + in-app CIDR are the boundary; the - /// firewall is defense-in-depth. - /// - [SupportedOSPlatform("windows")] - internal static async Task ReconcileFirewallAsync( - string ruleName, int port, bool enable, string? cidr, ILogger logger, CancellationToken cancellationToken) - { - var command = enable - ? DarlingManagedPostgres.BuildFirewallEnableCommand(ruleName, port, cidr!) - : DarlingManagedPostgres.BuildFirewallDisableCommand(ruleName); - - try - { - var (exitCode, output) = await DarlingManagedPostgres.RunPowerShellAsync(command, cancellationToken); - if (exitCode == 0) - { - logger.LogInformation("Firewall rule '{Rule}' {Verb}.", ruleName, enable ? "ensured" : "removed"); - } - else if (enable) - { - logger.LogWarning( - "Could not configure the firewall automatically (exit {ExitCode}: {Output}). Run this in an elevated PowerShell:\n{Command}", - exitCode, output, command); - } - else - { - logger.LogWarning("Could not remove the firewall rule automatically (exit {ExitCode}: {Output}).", exitCode, output); - } - } - catch (OperationCanceledException) - { - throw; - } - catch (Exception ex) - { - if (enable) - { - logger.LogWarning( - "Could not configure the firewall automatically ({Message}). Run this in an elevated PowerShell:\n{Command}", - ex.Message, command); - } - else - { - logger.LogWarning("Could not remove the firewall rule automatically ({Message}).", ex.Message); - } - } - } } diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs index 3507d68d72..04f6012a1a 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs @@ -49,9 +49,10 @@ namespace PerformanceMonitor.Darling.Service.Mcp; /// either way. Fail-closed, enforced HERE (the MCP host), NEVER in the all-fatal /// (the worker's abort would not stop this host). No TLS on MCP (a /// self-signed cert breaks real clients; the named MITM control is a TLS reverse proxy in front of the -/// endpoint) — the token travels cleartext on-segment, so own that residual with the reverse proxy. A -/// best-effort, scoped, idempotent firewall rule is added when exposed and removed when not -/// (defense-in-depth; the token + CIDR are the boundary, not the firewall). +/// endpoint) — the token travels cleartext on-segment, so own that residual with the reverse proxy. The +/// scoped, idempotent firewall rule is created by the ELEVATED installer (#1771 — this account cannot); +/// here it is only CHECKED and reported (defense-in-depth; the token + CIDR are the boundary, not the +/// firewall). /// /// Gated by darling.json's mcp.enabled (default OFF — a headless service should not open a /// port unless the operator asks); when disabled or when the config cannot load (the worker already logs @@ -78,7 +79,6 @@ public sealed class DarlingMcpHostService : BackgroundService private WebApplication? _app; private NpgsqlDataSource? _appDataSource; private int _runningPort; - private bool _runningNetworkMode; /// How often the supervisor re-reads the live control-plane state (#1560). internal static readonly TimeSpan SupervisorPollInterval = TimeSpan.FromSeconds(5); @@ -178,9 +178,11 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) } } - /// Stops and disposes the running app + its data source; in managed network mode also - /// reconciles the firewall rule off (symmetric with the start-side reconcile — disabling closes the - /// box). Safe to call when nothing is running. + /// Stops and disposes the running app + its data source. Safe to call when nothing is running. + /// It used to also remove the firewall rule here, mirroring the start-side reconcile. That write + /// could never succeed from this account (#1771) and the rule is now install-managed, so a stop leaves it + /// alone: it is scoped to a port nothing is listening on, an admin removes it with --configure-firewall or + /// uninstall-darling.ps1, and the start-side check reports it as stale until then. private async Task StopServerAsync(CancellationToken cancellationToken) { if (_app is null) @@ -206,12 +208,6 @@ private async Task StopServerAsync(CancellationToken cancellationToken) _appDataSource = null; } - if (_runningNetworkMode && OperatingSystem.IsWindows()) - { - await ReconcileMcpFirewallAsync(_runningPort, enable: false, null, cancellationToken); - } - - _runningNetworkMode = false; _runningPort = 0; } @@ -295,20 +291,21 @@ private async Task TryStartServerAsync(DarlingConfig config, int effective var primaryBind = networkMode ? networkListenIp! : IPAddress.Loopback; /* Port-in-use pre-check — Lite's StartMcpServerAsync guard, via the shared utility, against the - REAL bind address (D3-e: not always IPAddress.Loopback). Done before the firewall reconcile so a - bail here leaves the firewall untouched. */ + REAL bind address (D3-e: not always IPAddress.Loopback). Done before the firewall check so a + bail here reports nothing about the firewall. */ if (await PortUtilityService.IsTcpPortListeningAsync(effectivePort, primaryBind, stoppingToken)) { _logger.LogError("Port {Port} is already in use — MCP server not started this attempt; will retry", effectivePort); return false; } - /* Firewall reconcile (managed mode only; best-effort, never fatal). Symmetric like the store's D1 - reconcile: ensure the scoped rule when exposed, remove it when not (so disabling the config - closes the box). The token + in-app CIDR are the boundary; the firewall is defense-in-depth. */ + /* Firewall CHECK (managed mode only; read-only, never fatal) — #1771. Reports a missing rule when + exposed and a stale one when not, naming the elevated command either way; the rule itself is + created by the installer, because this process cannot create it. The token + in-app CIDR are the + boundary; the firewall is defense-in-depth. */ if (config.Postgres.Managed && OperatingSystem.IsWindows()) { - await ReconcileMcpFirewallAsync( + await CheckMcpFirewallAsync( effectivePort, networkMode, networkMode ? allowedCidr.ToString() : null, stoppingToken); } @@ -581,7 +578,6 @@ keep working. Both run BEFORE MapMcp (D3-b: "first ... before any handler/handsh serving until StopServerAsync (toggle-off, port change, or shutdown). */ await _app.StartAsync(stoppingToken); _runningPort = effectivePort; - _runningNetworkMode = networkMode; return true; } catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) @@ -785,19 +781,27 @@ private void LogBindReason(McpConfig mcp, McpBindReason reason) } /* --------------------------------------------------------------------------------------------------- - Best-effort MCP firewall reconcile (D1, defense-in-depth). Reuses the store's tested, pure command - builders (DarlingManagedPostgres.BuildFirewall*Command) for the exact scoped rule shape AND its shared - PowerShell runner (RunPowerShellAsync) — no duplication, no timeout divergence. Never fatal. + MCP firewall VERIFICATION (#1771). This used to reconcile the rule itself, which could never work: the + service runs as an unprivileged virtual account, so both halves failed "Access is denied" on every + start of every hardened install, and a fresh networked install got no rule at all. The rule is now + created by the elevated installer (--configure-firewall); this only reads and reports. --------------------------------------------------------------------------------------------------- */ /// The scoped MCP firewall rule name (idempotent by DisplayName), port-specific and distinct - /// from the store's rule so the two endpoints reconcile independently. internal so the headless - /// endpoint-toggle CLI verbs (--enable-mcp/--disable-mcp) reconcile the SAME rule by DisplayName. + /// from the store's rule so the two endpoints are managed independently. internal so the headless + /// endpoint-toggle CLI verbs (--enable-mcp/--disable-mcp) and --configure-firewall act on the SAME rule + /// by DisplayName. internal static string McpFirewallRuleName(int port) => $"PerformanceMonitor Darling MCP (port {port})"; + /// Last (rule, verdict) this host reported, so a supervisor retry loop restates a steady + /// firewall state at most once (). + private string? _lastFirewallRule; + private FirewallRuleVerdict? _lastFirewallVerdict; + [SupportedOSPlatform("windows")] - private Task ReconcileMcpFirewallAsync(int port, bool enable, string? cidr, CancellationToken cancellationToken) - => DarlingHostBinding.ReconcileFirewallAsync(McpFirewallRuleName(port), port, enable, cidr, _logger, cancellationToken); + private async Task CheckMcpFirewallAsync(int port, bool exposed, string? cidr, CancellationToken cancellationToken) + => (_lastFirewallRule, _lastFirewallVerdict) = await DarlingFirewallCheck.CheckAsync( + McpFirewallRuleName(port), port, exposed, cidr, _lastFirewallRule, _lastFirewallVerdict, _logger, cancellationToken); /// /// Managed mode's first-boot race, handled: the dedicated mcp-role credential appears only after diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingWebHostService.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingWebHostService.cs index a81ae9124a..5a5fdb79e2 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingWebHostService.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingWebHostService.cs @@ -37,7 +37,7 @@ namespace PerformanceMonitor.Darling.Service.Mcp; /// Shape mirrors the MCP host (5s supervisor poll, a pure decision /// table, 30s failed-start backoff, StartAsync not RunAsync so the supervisor owns the wait) and /// reuses the shared for the LAN-exposure ladder, the in-app CIDR check, the -/// loopback-listener guard, the constant-time token compare, and the firewall reconcile. The live +/// loopback-listener guard, and the constant-time token compare. The live /// enable/port state arrives via (the worker publishes it), so the viewer's /// Settings toggle starts/stops/rebinds the dashboard with no service restart. /// @@ -62,7 +62,6 @@ public sealed class DarlingWebHostService : BackgroundService private WebApplication? _app; private NpgsqlDataSource? _appDataSource; private int _runningPort; - private bool _runningNetworkMode; /// How often the supervisor re-reads the live control-plane state (#1562, mirrors the MCP host). internal static readonly TimeSpan SupervisorPollInterval = TimeSpan.FromSeconds(5); @@ -166,9 +165,11 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) } } - /// Stops and disposes the running app + its data source; in managed network mode also reconciles - /// the firewall rule off (symmetric with the start-side reconcile — disabling closes the box). Safe to call - /// when nothing is running. + /// Stops and disposes the running app + its data source. Safe to call when nothing is running. + /// It used to also remove the firewall rule here, mirroring the start-side reconcile. That write + /// could never succeed from this account (#1771) and the rule is now install-managed, so a stop leaves it + /// alone: it is scoped to a port nothing is listening on, an admin removes it with --configure-firewall or + /// uninstall-darling.ps1, and the start-side check reports it as stale until then. private async Task StopServerAsync(CancellationToken cancellationToken) { if (_app is null) @@ -194,12 +195,6 @@ private async Task StopServerAsync(CancellationToken cancellationToken) _appDataSource = null; } - if (_runningNetworkMode && OperatingSystem.IsWindows()) - { - await ReconcileWebFirewallAsync(_runningPort, enable: false, null, cancellationToken); - } - - _runningNetworkMode = false; _runningPort = 0; } @@ -303,18 +298,20 @@ private async Task TryStartServerAsync(DarlingConfig config, int effective var primaryBind = networkMode ? networkListenIp! : IPAddress.Loopback; /* Port-in-use pre-check via the shared utility against the REAL bind address. Done before the - firewall reconcile so a bail here leaves the firewall untouched. */ + firewall check so a bail here reports nothing about the firewall. */ if (await PortUtilityService.IsTcpPortListeningAsync(effectivePort, primaryBind, stoppingToken)) { _logger.LogError("Port {Port} is already in use — web dashboard not started this attempt; will retry", effectivePort); return false; } - /* Firewall reconcile (managed mode only; best-effort, never fatal): ensure the scoped rule when - exposed, remove it when not. The in-app CIDR is the boundary; the firewall is defense-in-depth. */ + /* Firewall CHECK (managed mode only; read-only, never fatal) — #1771. Reports a missing rule when + exposed and a stale one when not, naming the elevated command either way; the rule itself is + created by the installer, because this process cannot create it. The in-app CIDR is the + boundary; the firewall is defense-in-depth. */ if (config.Postgres.Managed && OperatingSystem.IsWindows()) { - await ReconcileWebFirewallAsync( + await CheckWebFirewallAsync( effectivePort, networkMode, networkMode ? allowedCidr.ToString() : null, stoppingToken); } @@ -469,7 +466,6 @@ only the network auth decision (session cookie / ?token= / in-CIDR). */ StopServerAsync (toggle-off, port change, or shutdown). */ await _app.StartAsync(stoppingToken); _runningPort = effectivePort; - _runningNetworkMode = networkMode; return true; } catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) @@ -742,7 +738,7 @@ private static byte[] Base64UrlDecode(string value) } /* --------------------------------------------------------------------------------------------------- - Bind-reason logging + managed-store credential wait + firewall reconcile (mirrors the MCP host). + Bind-reason logging + managed-store credential wait + firewall check (mirrors the MCP host). --------------------------------------------------------------------------------------------------- */ /// Emits the reason at its mapped severity. Silent for @@ -790,13 +786,22 @@ private void LogBindReason(WebConfig web, DarlingHostBinding.BindReason reason) } /// The scoped web firewall rule name (idempotent by DisplayName), port-specific and distinct from - /// the store's and MCP's rules so the endpoints reconcile independently. internal so the headless - /// endpoint-toggle CLI verbs (--enable-web/--disable-web) reconcile the SAME rule by DisplayName. + /// the store's and MCP's rules so the endpoints are managed independently. internal so the headless + /// endpoint-toggle CLI verbs (--enable-web/--disable-web) and --configure-firewall act on the SAME rule by + /// DisplayName. internal static string WebFirewallRuleName(int port) => $"PerformanceMonitor Darling Web (port {port})"; + /// Last (rule, verdict) this host reported, so a supervisor retry loop restates a steady + /// firewall state at most once (). + private string? _lastFirewallRule; + private FirewallRuleVerdict? _lastFirewallVerdict; + + /// Read-only firewall verification (#1771) — see for why this + /// no longer writes the rule. [SupportedOSPlatform("windows")] - private Task ReconcileWebFirewallAsync(int port, bool enable, string? cidr, CancellationToken cancellationToken) - => DarlingHostBinding.ReconcileFirewallAsync(WebFirewallRuleName(port), port, enable, cidr, _logger, cancellationToken); + private async Task CheckWebFirewallAsync(int port, bool exposed, string? cidr, CancellationToken cancellationToken) + => (_lastFirewallRule, _lastFirewallVerdict) = await DarlingFirewallCheck.CheckAsync( + WebFirewallRuleName(port), port, exposed, cidr, _lastFirewallRule, _lastFirewallVerdict, _logger, cancellationToken); /// /// Managed mode's first-boot race, handled: the VIEWER-role credential appears only after the worker's diff --git a/Darling/PerformanceMonitor.Darling.Service/Program.cs b/Darling/PerformanceMonitor.Darling.Service/Program.cs index c3a4a52526..91b8225330 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Program.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Program.cs @@ -109,6 +109,23 @@ explicit config path. Console.In is the scripted-input testability lever. */ configPath, Console.In, Console.Out, Console.Error, CancellationToken.None); } +/* CLI verb: --configure-firewall (#1771) — create/remove every scoped Darling firewall rule so the live + firewall matches darling.json. The service account cannot write firewall rules by design, so this ELEVATED + verb owns them: install-darling.ps1 runs it after the service is up, uninstall-darling.ps1 removes them, and + the running service only verifies. Reads darling.json only (no store, no credentials), so it works at + install time before the store has ever booted. Optional second arg = an explicit config path. */ +if (args.Length > 0 && DarlingCliCommands.IsConfigureFirewallVerb(args[0])) +{ + if (!OperatingSystem.IsWindows()) + { + Console.Error.WriteLine("--configure-firewall requires Windows (Windows Firewall)."); + return 1; + } + + var configPath = args.Length > 1 ? args[1] : null; + return await DarlingCliCommands.ConfigureFirewallAsync(configPath, Console.Out, Console.Error, CancellationToken.None); +} + /* 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 diff --git a/Darling/README.md b/Darling/README.md index cad599be5e..f485a56cdf 100644 --- a/Darling/README.md +++ b/Darling/README.md @@ -639,6 +639,20 @@ PerformanceMonitor.Darling.Service.exe --configure-network It shows the current exposure (read from the service's own resolvers), then walks you through the **store**, **MCP**, the **web dashboard**, any comma combination (e.g. `1,3`), or all three at once (or a **disable** that removes all exposure). Every answer is validated **by delegation to the exact checks the running service fail-closes on**, so the wizard can never write a config the service would refuse — it re-prompts with the resolver's own reason. It generates the MCP bearer / web access tokens for you (DPAPI-protected; each plaintext is printed once, so save it then), edits `darling.json` **in place preserving every comment** behind a timestamped `darling.json.bak-` backup, prints the scoped firewall command(s), the `--print-viewer-connection` handoff, and the web dashboard's browser login URL (`http://:/?token=...`), and offers to restart the service to apply. `install-darling.ps1 -Network` runs it automatically right after the install reaches Running. The manual field reference below documents exactly what it writes. +### Firewall rules (`--configure-firewall`) + +The service runs as `NT SERVICE\PerformanceMonitor Darling`, an unprivileged virtual account that **cannot create Windows Firewall rules** — and should not be able to. So the rules are managed from the elevated install instead: `install-darling.ps1` runs `--configure-firewall` for you (before the first start, and again after `-Network`), and `uninstall-darling.ps1` removes them. Run it by hand after any edit to a `network` block: + +``` +PerformanceMonitor.Darling.Service.exe --configure-firewall +``` + +Run **elevated**. It reconciles all three scoped rules — store, MCP, web dashboard — against `darling.json` in one pass: it opens the port for every surface that really is exposed and removes the rule for every surface that is not, so it also cleans up after an exposure you turned back off. It is idempotent (safe on every upgrade) and reads **only** `darling.json`, so it works before the store has ever booted — unlike `--enable-mcp` / `--enable-web`, which write the control-plane store and need the service to have initialized it. + +"Really exposed" is decided by the same resolvers the running service fail-closes on, not by reading `listen` at face value. A `network` block the service would degrade to loopback — an unparseable `listen`, a missing or invalid `allowFrom`, an address family that disagrees, a missing token, BYO mode — gets **no open port**, and the verb tells you why. + +The running service never touches these rules. It **checks** them on start and logs what it finds: nothing at all for the normal loopback-only install, one INFO line when an exposed endpoint's rule is present, and one WARN naming the exact command when an exposed endpoint's rule is missing or when a loopback-only endpoint still has a stale rule open. It states each verdict once, not once per retry. + ### Headless enable/disable + firewall (`--enable-mcp` / `--enable-web`) On a box with no Viewer, two things are otherwise awkward: the `enabled` flags in the `mcp` / `web` blocks below are only a **first-run seed** — after the first run the store (`config.config_service.mcp_enabled` / `web_enabled`) is authoritative and is normally flipped only from the Viewer's Settings — and the service account (`NT SERVICE\PerformanceMonitor Darling`) **cannot open the firewall itself**. Four verbs, run on the **service host**, close both in one elevated action: @@ -650,20 +664,20 @@ PerformanceMonitor.Darling.Service.exe --enable-web PerformanceMonitor.Darling.Service.exe --disable-web ``` -Each flips only its endpoint's **live store flag** with a targeted `config_service` write; the service **hot-reloads within one collection sweep — no restart.** If that endpoint's `network` block opts into LAN exposure (a non-loopback `listen`), the verb also reconciles the **same scoped, idempotent-by-name firewall rule the service would**: **run elevated**, it opens (or, on `--disable-*`, removes) the rule; **run non-elevated**, the store toggle still succeeds and it prints the exact elevated firewall command to run by hand (a loopback-only endpoint needs no rule and says so). Managed-mode only, Windows only. So the headless bring-up is: write the `network` block (the wizard above or the manual reference below), then `--enable-mcp` / `--enable-web` from an **elevated** shell. +Each flips only its endpoint's **live store flag** with a targeted `config_service` write; the service **hot-reloads within one collection sweep — no restart.** If that endpoint's `network` block opts into LAN exposure (a non-loopback `listen`), the verb also reconciles that endpoint's **scoped, idempotent-by-name firewall rule**: **run elevated**, it opens (or, on `--disable-*`, removes) the rule; **run non-elevated**, the store toggle still succeeds and it prints the exact elevated firewall command to run by hand (a loopback-only endpoint needs no rule and says so). Managed-mode only, Windows only. So the headless bring-up is: write the `network` block (the wizard above or the manual reference below), then `--enable-mcp` / `--enable-web` from an **elevated** shell. ### Verify it's actually reachable (and the two failures that look like bugs) Enabling an endpoint is **not** the same as reaching it, and both common failures leave the store flag reading `true`, so "it says enabled" is not proof. After `--enable-mcp` / `--enable-web`, verify on the **service host**: -1. **The listener is on the LAN address, not loopback.** `Get-NetTCPConnection -State Listen | Where-Object LocalPort -eq 5152` (or `5153` for web) must show the box's LAN IP, e.g. `10.0.0.5:5152` — **not** only `::1` / `127.0.0.1`. *Enabled but still loopback-bound* is the single most common failure: the store flag is on, but the service loaded `darling.json` **before** the `network` block existed. The block is read **once at service start** — the enable toggle stops/starts the endpoint with the already-loaded config and does **not** reload the file. **Restart the service** (`Restart-Service 'PerformanceMonitor Darling'`) so it re-reads the block, then re-check the listener; after the restart run `--enable-mcp` **elevated** only if the firewall rule is missing (the service self-reconciles the rule best-effort, but its service account cannot open the firewall itself). -2. **The scoped firewall rule exists and covers the client.** `Get-NetFirewallRule -DisplayName 'PerformanceMonitor Darling MCP (port 5152)'` (or `... Web (port 5153)`) should be `Enabled=True, Action=Allow`, scoped to the `network.allowFrom` CIDR. A non-elevated `--enable-*` skips this and prints the exact command to run by hand. +1. **The listener is on the LAN address, not loopback.** `Get-NetTCPConnection -State Listen | Where-Object LocalPort -eq 5152` (or `5153` for web) must show the box's LAN IP, e.g. `10.0.0.5:5152` — **not** only `::1` / `127.0.0.1`. *Enabled but still loopback-bound* is the single most common failure: the store flag is on, but the service loaded `darling.json` **before** the `network` block existed. The block is read **once at service start** — the enable toggle stops/starts the endpoint with the already-loaded config and does **not** reload the file. **Restart the service** (`Restart-Service 'PerformanceMonitor Darling'`) so it re-reads the block, then re-check the listener; after the restart run `--configure-firewall` **elevated** if the firewall rule is missing (the service account cannot create it, so the service only tells you it is missing). +2. **The scoped firewall rule exists and covers the client.** `Get-NetFirewallRule -DisplayName 'PerformanceMonitor Darling MCP (port 5152)'` (or `... Web (port 5153)`) should be `Enabled=True, Action=Allow`, scoped to the `network.allowFrom` CIDR. If it is absent, the service's own start-up log already says so and names the command; `--configure-firewall` elevated is the one-step fix. Reading rules needs no elevation, so this check works from any shell. Then from the **client** host: 3. **Connect to the box's LAN IP, never `localhost`.** Use `http://:5152/`. `localhost` / `127.0.0.1` only resolves *on the box itself*, so an off-box MCP client pointed at localhost fails silently — this is the number-one "MCP won't connect" cause. Send `Authorization: Bearer ` (the `network.token`), and do a **fresh** `initialize` + `tools/list` rather than trusting a cached tool list from a previous version. -**After a reinstall:** the installer replaces binaries but does **not** touch `darling.json` (the zip ships only `darling.sample.json`) or the store, so the `network` block and both live flags survive the upgrade — and the reinstall restarts the service, which re-reads the block. If MCP stops connecting afterward it is almost always failure 3 (the client pointed at `localhost`) or a missing firewall rule, **not** lost config: run `--enable-mcp` **elevated** to re-open the rule if check 2 comes up empty. A stale loopback bind is unlikely after a restart unless the block itself is invalid, in which case the endpoint fail-closes to loopback and logs a critical line saying why — fix the block and restart again. A full `--configure-network` re-run is only needed if the `network` block itself is gone. +**After a reinstall:** the installer replaces binaries but does **not** touch `darling.json` (the zip ships only `darling.sample.json`) or the store, so the `network` block and both live flags survive the upgrade — and the reinstall restarts the service, which re-reads the block. If MCP stops connecting afterward it is almost always failure 3 (the client pointed at `localhost`) or a missing firewall rule, **not** lost config: run `--configure-firewall` **elevated** to re-open the rule if check 2 comes up empty (the installer already does this, so an in-place upgrade normally leaves the rules correct). A stale loopback bind is unlikely after a restart unless the block itself is invalid, in which case the endpoint fail-closes to loopback and logs a critical line saying why — fix the block and restart again. A full `--configure-network` re-run is only needed if the `network` block itself is gone. ### Store endpoint (viewer over the LAN) @@ -681,14 +695,14 @@ Add a `network` block to `postgres` (managed mode): } ``` -On every start the service reconciles this against the live cluster: it adds the bind IP to `listen_addresses`, generates a self-signed TLS certificate (`server.crt` / `server.key` beside the data directory, with both an IP SAN for `listen` and a DNS SAN for the machine hostname), writes a marked `hostssl darling scram-sha-256` rule into `pg_hba.conf` and reloads, and best-effort adds a firewall rule. +On every start the service reconciles this against the live cluster: it adds the bind IP to `listen_addresses`, generates a self-signed TLS certificate (`server.crt` / `server.key` beside the data directory, with both an IP SAN for `listen` and a DNS SAN for the machine hostname), writes a marked `hostssl darling scram-sha-256` rule into `pg_hba.conf` and reloads, and **checks** (never creates — see [Firewall rules](#firewall-rules---configure-firewall)) that the store's scoped firewall rule matches. - **`role`** — the pg_hba login role the rule names: `"viewer"` (default, **read-only** — the secure default, covering a laptop reading every dashboard, chart, and finding) or `"admin"` (full remote **writes**; the service logs a warning because `admin` holds the `config_command` / `config_monitored_servers` / `config_notification` service-credential pivot). Never the superuser. This is **distinct from `postgres.connectAs`** (the *local* VM viewer's loopback role, default `admin`): `network.role` is the *remote* role and defaults to `viewer`, so the two have opposite defaults — the local seat is writable, the remote seat is read-only, unless you say otherwise. - **TLS is verify-full, not `require`.** Because Darling generates the cert, the client can pin it, so the connection string below uses `SSL Mode=VerifyFull` — which actually defends against an on-path MITM (`require` verifies nothing). The store's network pg_hba line is `hostssl`, so a non-TLS network client is refused. -- **The firewall is defense-in-depth, and you should set the scoped rule yourself** (the service's best-effort rule is a convenience). The store's real boundary is pg_hba + TLS: +- **The firewall is defense-in-depth, not the boundary** — pg_hba + TLS are. `--configure-firewall` (elevated) creates the store's scoped rule for you along with the other two; the equivalent by hand is: ``` - New-NetFirewallRule -DisplayName "Darling store (Postgres)" -Direction Inbound -Action Allow -Protocol TCP -LocalPort 5641 -RemoteAddress 192.168.1.0/24 + New-NetFirewallRule -DisplayName "PerformanceMonitor Darling store (port 5641)" -Direction Inbound -Action Allow -Protocol TCP -LocalPort 5641 -RemoteAddress 192.168.1.0/24 ``` **Remote-viewer handoff.** On the **service host**, run: diff --git a/Darling/tools/install-darling.ps1 b/Darling/tools/install-darling.ps1 index 00b9245c5a..6f64faca38 100644 --- a/Darling/tools/install-darling.ps1 +++ b/Darling/tools/install-darling.ps1 @@ -22,6 +22,9 @@ C:\PerformanceMonitorDarling). What it does, in order: 4b. Restricts darling.json to SYSTEM / Administrators / the service account, plus read for INTERACTIVE (the Viewer). It holds encrypted SQL passwords and the MCP/web access tokens, and an install folder under C:\ otherwise inherits read access for BUILTIN\Users. + 4c. Creates (or removes) the scoped Windows Firewall rules to match darling.json, via the exe's + --configure-firewall verb. This requires elevation, which is why it lives here: the service's + own unprivileged account cannot create them, and only verifies them at runtime. 5. Starts the service and confirms it reaches Running. 6. Creates 'Darling Viewer' shortcuts on the Desktop and in the Start Menu pointing at viewer\PerformanceMonitor.Darling.Viewer.exe. (Taskbar pinning is deliberately not @@ -217,6 +220,34 @@ if ($failed.Count -gt 0) { Write-Host '' } +# -- 4c. Firewall rules (#1771) -------------------------------------------------------------------- +# The service runs as an unprivileged virtual account that CANNOT create firewall rules. It used to try on +# every start and fail "Access is denied" each time - harmless where the rules already existed from a manual +# setup, but on a FRESH networked install nothing ever created them, so remote MCP/web/store clients were +# simply blocked. Networked is the normal deployment mode, so rule management belongs here, in the elevated +# install, and the running service only verifies. +# +# The exe does the work (--configure-firewall) rather than this script building rules itself: the rule names, +# the ports, and above all WHETHER a surface is really exposed come from the same resolvers the service +# fail-closes on, so a config the service degrades to loopback never gets an open port, and the service's own +# start-up check always reaches the same verdict the installer just acted on. Idempotent, so an upgrade +# re-running it is a no-op, and it needs only darling.json - no store, no credentials - so it is safe here, +# BEFORE the first start. +# +# Best-effort: a firewall failure warns rather than aborting an otherwise good install. Re-run it by hand. +function Invoke-FirewallReconcile { + & $serviceExe --configure-firewall + if ($LASTEXITCODE -ne 0) { + Write-Host '' + Write-Host "Firewall rules were not fully configured (exit $LASTEXITCODE). The service will still run, and" -ForegroundColor Yellow + Write-Host 'will log which rule is missing on each start. Re-run from an elevated prompt:' -ForegroundColor Yellow + Write-Host " & `"$serviceExe`" --configure-firewall" -ForegroundColor Yellow + Write-Host '' + } +} + +Invoke-FirewallReconcile + # -- 5. Start + confirm --------------------------------------------------------------------------- Start-Service -Name $serviceName (Get-Service -Name $serviceName).WaitForStatus('Running', [TimeSpan]::FromSeconds(60)) @@ -230,6 +261,12 @@ if ($Network) { Write-Host '' Write-Host 'Launching the guided network-exposure wizard (--configure-network)...' -ForegroundColor Cyan & $serviceExe --configure-network + + # The wizard just rewrote the exposure, so the rules created above no longer match darling.json - a newly + # exposed endpoint has no rule yet, and one turned back off has a stale one. Re-reconcile (idempotent). + Write-Host '' + Write-Host 'Re-applying the firewall rules to match the new exposure...' -ForegroundColor Cyan + Invoke-FirewallReconcile } # -- 6. Viewer shortcuts -------------------------------------------------------------------------- diff --git a/Darling/tools/uninstall-darling.ps1 b/Darling/tools/uninstall-darling.ps1 index cac066147f..ccd949d204 100644 --- a/Darling/tools/uninstall-darling.ps1 +++ b/Darling/tools/uninstall-darling.ps1 @@ -3,8 +3,9 @@ Uninstalls the PerformanceMonitor Darling service and removes the viewer shortcuts. .DESCRIPTION -Run from an ELEVATED PowerShell. Stops and deletes the Windows service and removes the Desktop / -Start Menu 'Darling Viewer' shortcuts. DELIBERATELY LEFT IN PLACE: the extracted program folder, +Run from an ELEVATED PowerShell. Stops and deletes the Windows service, removes the scoped Windows +Firewall rules it installed, and removes the Desktop / Start Menu 'Darling Viewer' shortcuts. +DELIBERATELY LEFT IN PLACE: the extracted program folder, darling.json, and everything under %ProgramData%\PerformanceMonitorDarling (the PostgreSQL store with all collected history, the DPAPI credential files, and the service logs) - so an uninstall is reversible by simply re-running install-darling.ps1. To destroy the collected data too, pass @@ -44,6 +45,31 @@ else { Write-Host "Service '$serviceName' is not installed - nothing to remove." } +# Scoped firewall rules (#1771). Created by install-darling.ps1 because the service account cannot create +# them; removed here for the same reason - nothing else ever will, and leaving an inbound allow rule behind +# for a service that no longer exists is exactly the litter an uninstall should clear. +# +# Matched by the shared DisplayName PREFIX rather than by rebuilding each port-specific name, so a store/MCP/ +# web rule left over from a PREVIOUS port is removed too - reconstructing names from the current darling.json +# would strand those, and darling.json may not even be readable at this point. The C# side pins this literal +# against all three real rule-name builders (DarlingFirewallCheck.RuleNamePrefix), so a rename cannot silently +# orphan rules here. A wildcard that matches nothing is not an error (verified), but the try/catch keeps a +# firewall hiccup from aborting the rest of an uninstall under ErrorActionPreference = 'Stop'. +try { + $rules = @(Get-NetFirewallRule -DisplayName 'PerformanceMonitor Darling *' -ErrorAction SilentlyContinue) + if ($rules.Count -gt 0) { + Remove-NetFirewallRule -DisplayName 'PerformanceMonitor Darling *' -ErrorAction Stop + Write-Host "Removed $($rules.Count) scoped firewall rule(s)." + } + else { + Write-Host 'No Darling firewall rules to remove.' + } +} +catch { + Write-Host "Could not remove the firewall rules ($($_.Exception.Message)). Remove them by hand:" -ForegroundColor Yellow + Write-Host " Remove-NetFirewallRule -DisplayName 'PerformanceMonitor Darling *'" -ForegroundColor Yellow +} + foreach ($lnkPath in @( (Join-Path ([Environment]::GetFolderPath('Desktop')) 'Darling Viewer.lnk'), (Join-Path ([Environment]::GetFolderPath('StartMenu')) 'Programs\Darling Viewer.lnk') From 84583966ba1309bf2c39d18b49f99e309c5aaafc Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:43:58 -0400 Subject: [PATCH 2/4] Sweep a surface's rules for every port, not just the current name (#1771) The port is part of the rule DisplayName, so changing a port does not update a rule - it creates a different one and strands the old as an inbound allow rule on a port nothing serves. Reconciling by exact name could never reach that, so --configure-firewall now sweeps a per-surface "... (port *)" wildcard before ensuring the current rule. This is the "update port if the rule exists with a different port" half of the fix, which the first pass missed. The wildcard is derived from the rule name, never assembled from an operator string, and a name with no port suffix is returned UNCHANGED rather than gaining a "*": a wildcard built from a malformed name could match, and therefore delete, rules this product does not own. The sweep runs as its OWN step rather than concatenated ahead of the open. It ends in "exit 0", so concatenating would terminate the shell before the rule was created while still reporting success - written, caught, and now pinned by a call-shape test. Verified live: with a leftover "MCP (port 5152)" rule present and mcp.port moved to 5199, one run left exactly one rule, on 5199. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 2 + .../DarlingFirewallCheckTests.cs | 66 +++++++++++++++++++ .../DarlingCliCommands.cs | 29 ++++---- .../DarlingFirewallCheck.cs | 20 ++++++ .../DarlingManagedPostgres.cs | 21 ++++++ 5 files changed, 127 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb961c06e1..addcfcf3e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,6 +91,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 **The bundled store had the identical bug on its own port** and is fixed the same way, though the issue named only 5152/5153. Two behaviors deliberately end: a service stop no longer removes the rule (it could never succeed, and the rule is install-managed now - it is scoped to a port nothing is listening on, and the next start reports it as stale), and `_runningNetworkMode`, which existed only to drive that removal, is gone. + **The port is part of the rule NAME**, so changing a port does not update a rule - it makes a different one and strands the old as an inbound allow rule on a port nothing serves. Reconciling by exact name could never reach that, so the verb sweeps a per-surface `... (port *)` wildcard before ensuring the current rule. The wildcard is derived, never assembled from an operator string, and a name with no port suffix is returned UNCHANGED rather than gaining a `*` - a wildcard built from a malformed name could match, and therefore delete, rules this product does not own. Verified live: with a leftover `MCP (port 5152)` rule present and `mcp.port` moved to 5199, one run left exactly one rule, on 5199. The sweep runs as its OWN step rather than concatenated ahead of the open, because it ends in `exit 0` and would otherwise terminate the shell before the rule was created - a bug written and caught here, now pinned by its own call-shape test. + **Verified live, not just green.** The verb was run against real configs on a real firewall: an exposed MCP config created exactly one rule (`TCP 5152, inbound, RemoteAddress 192.168.1.0/24`) and left the other two absent; a fail-closed config (`allowFrom: "not-a-cidr"`) opened nothing, said why, and removed the stale rule; a loopback config removed all three; the machine was left with zero Darling rules. The exact probe command was run under a **non-elevated** token against both a present and an absent rule, returning `1` and `0` with a clean exit each time. Every guard was then verified RED by mutation, including one that first came back GREEN and exposed a weak test: restoring the runtime `New-NetFirewallRule` write; planning exposure from `listen` alone instead of the resolver; dropping the probe's `exit 0`; treating an unparseable probe as "absent"; reporting on every tick; emitting an open command with no CIDR; renaming the shared rule prefix that uninstall sweeps by; deleting the installer's invocation; dropping the verb from the dispatch allow-list; and reverting the store to writing its own rule. The installer pin **survived** its first mutation - `Contains("--configure-firewall")` stayed green with the real call deleted, because the script also names the verb in its help text and its error message - so it now pins the invocation form and proves the wrapper is called somewhere other than its own definition, and goes red both when the call is replaced and when every call site is removed - **Anomaly baselines were being computed from a fraction of their intended history, silently** ([#1762]) - closing [#1757]. `PgBaselineProvider` asks for 30 days. `query_stats` keeps 4 under tiered retention, and the other eight baseline sources keep exactly 30, so on every tiered store the thresholds that decide what counts as an anomaly were built from far less data than designed with **no error raised**. The client-side timeouts that got this reported were the loud phase a store passes through on its way to that silence: once retention reaches a box the query stops timing out, because there is only 4 days left to scan. Severity was corrected during implementation - only `query_stats` sits behind the 4-day horizon (the TimescaleDB raw retention policy covers just `query_stats`, `procedure_stats` and `query_store_stats`, and only the first is a baseline source), so `query_duration` is a cliff at 4-vs-30 and the other ten families are a knife edge at 30-vs-30 - not broken today, but racing the purge at the oldest bucket and one config change away from the same degradation. **Nine continuous aggregates at the COLLECTION grain** now serve all eleven families: the hourly bucket is purely a partitioning and retention key and `collection_time` is a second GROUP BY column carrying the grain, so `AVG`, `STDDEV_SAMP` and the restart-exclusion `LAG` stay numerically identical instead of becoming a different statistic at a different scale. The existing hourly CAGG ladder **cannot** serve this for two independent reasons - wrong unit of observation, and it stores no sum-of-squares, so `AVG` reconstructs from sums and counts but standard deviation does not, and standard deviation is what sets the threshold. `cpu` and `file_io` therefore store sufficient statistics rather than an average: neither is one row per collection, because `CpuUtilizationCollector` issues `SELECT TOP (60)` over `RING_BUFFER_SCHEDULER_MONITOR` and appends every row under a single `collection_time`, so averaging first would silently redefine `sample_count` from samples to collections. The tier carries its own 35-day retention, deliberately longer than the 30-day window, and that relation is **pinned statically** because Storage cannot reference Analysis and it has no compile-time home - drop it below the window and the bug returns, silently. Two properties are required for the aggregates to actually hold history, both verified against TimescaleDB source rather than the docs: real-time aggregation is **opted into explicitly** (it has defaulted OFF since 2.13), and a coverage-gated backfill materializes whatever history the store already had. That backfill **verifies coverage after refreshing** instead of trusting that a successful `CALL` filled the aggregate, escalating to a forced refresh only on evidence: a refresh consumes invalidations, so a backfill cut short by a shutdown leaves a hole that a later plain refresh no-ops over while reporting success. That matters because the failure mode is **silence, not an error** - with real-time aggregation the watermark is a hard partition (`materialized WHERE time < watermark UNION ALL raw WHERE time >= watermark`), so an un-materialized region older than the watermark reads as absent rather than falling back to raw. No hand-rolled time slicing: manual `refresh_continuous_aggregate` already batches internally, each batch its own transaction, so partial progress survives an abort. The backfill is **launched, not awaited** - the composer tuning, delta re-seed and collection loop are all sequenced behind the TimescaleDB block, so awaiting it would take a restarted service dark for its whole duration, exactly when an operator is most likely to be restarting it - and a source-level test pins that, because adding an `await` there compiles, passes every functional test, and is invisible except on a large store. **No `CommandTimeout` bump anywhere**: the baseline queries keep the default, the fix is that they now read a tier sized for the question. Lite has no horizon bug (all nine of its baseline sources retain 30 days) so it keeps its raw-scan shape, but it gains the warning - lowering a source collector's retention below the baseline window would reintroduce the same silent degradation, and now says so once per metric. The mutation check is the test that earns its keep: point any family back at a raw `v_*` passthrough and the suite goes red, verified by actually reverting one. One more failure mode was caught by the live-PostgreSQL CI job, which is the only place it could show: without TimescaleDB the aggregates cannot exist, and the provider reads those relations **by name**, so a missing one throws, gets swallowed, and logs that same "Failed to compute baselines" line for **every** family - strictly worse than the bug being fixed. The nine relations are therefore also creatable as ordinary views, **derived from the aggregate's own select body** rather than written out again so the two supplies cannot drift, with `date_trunc('hour', ...)` standing in for the one TimescaleDB-only construct. The reverse transition is handled too - a store that later gains TimescaleDB has the stale fallback dropped first, guarded, because a continuous aggregate is itself a `relkind='v'` view and an unguarded `DROP VIEW` would destroy a materialized tier. diff --git a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs index 56dff3f80a..280f61b0e4 100644 --- a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs +++ b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs @@ -171,6 +171,72 @@ an empty -RemoteAddress would hand the operator a rule that opens the port to th FirewallRuleVerdict.ExposedRuleMissing, "PerformanceMonitor Darling MCP (port 5152)", 5152, " ")); } + /* ---- the per-surface sweep: a port change strands the OLD rule, and only a wildcard reaches it ---- */ + + [Theory] + [InlineData("PerformanceMonitor Darling MCP (port 5152)", "PerformanceMonitor Darling MCP (port *)")] + [InlineData("PerformanceMonitor Darling Web (port 5153)", "PerformanceMonitor Darling Web (port *)")] + [InlineData("PerformanceMonitor Darling store (port 5641)", "PerformanceMonitor Darling store (port *)")] + public void SurfaceRuleWildcard_CoversEveryPortOfOneSurface(string ruleName, string expected) + => Assert.Equal(expected, DarlingFirewallCheck.SurfaceRuleWildcard(ruleName)); + + [Fact] + public void SurfaceRuleWildcard_KeepsSurfacesApart() + { + /* The sweep must not reach across surfaces: clearing MCP's old ports must never touch the web rule. */ + var mcp = DarlingFirewallCheck.SurfaceRuleWildcard(DarlingMcpHostService.McpFirewallRuleName(5152)); + + Assert.DoesNotContain("Web", mcp, StringComparison.Ordinal); + Assert.DoesNotContain("store", mcp, StringComparison.Ordinal); + Assert.NotEqual(mcp, DarlingFirewallCheck.SurfaceRuleWildcard(DarlingWebHostService.WebFirewallRuleName(5153))); + } + + [Theory] + [InlineData("PerformanceMonitor Darling MCP")] + [InlineData("")] + [InlineData("something else entirely")] + public void SurfaceRuleWildcard_NeverInventsAWildcardFromAnUnrecognizedName(string ruleName) + { + /* Safety, not formality: a wildcard derived from a name with no port suffix would broaden the sweep to + whatever else happened to share the prefix — rules this product does not own and must not delete. */ + Assert.Equal(ruleName, DarlingFirewallCheck.SurfaceRuleWildcard(ruleName)); + Assert.DoesNotContain("*", DarlingFirewallCheck.SurfaceRuleWildcard(ruleName), StringComparison.Ordinal); + } + + [Fact] + public void BuildFirewallSweepCommand_IsStandalone_AndReportsRealFailuresLoudly() + { + var cmd = DarlingManagedPostgres.BuildFirewallSweepCommand("PerformanceMonitor Darling MCP (port *)"); + + Assert.Contains("Remove-NetFirewallRule -DisplayName 'PerformanceMonitor Darling MCP (port *)'", cmd, StringComparison.Ordinal); + Assert.DoesNotContain("New-NetFirewallRule", cmd, StringComparison.Ordinal); + + /* SilentlyContinue would hide the error TEXT but still exit 1, so access denied would surface as + "exit 1:" with an empty message — the trap BuildFirewallDisableCommand documents. */ + Assert.DoesNotContain("-ErrorAction SilentlyContinue", cmd, StringComparison.Ordinal); + Assert.Contains("-ErrorAction Stop", cmd, StringComparison.Ordinal); + Assert.Contains("ObjectNotFound", cmd, StringComparison.Ordinal); + Assert.Contains("throw", cmd, StringComparison.Ordinal); + + /* It ends in `exit 0`, which makes it a COMPLETE command: concatenating it ahead of another would + terminate the shell before that command ran. The verb must run it as its own step. */ + Assert.EndsWith("exit 0", cmd, StringComparison.Ordinal); + } + + [Fact] + public void ConfigureFirewall_RunsTheSweepAsItsOwnStep_NeverConcatenatedAheadOfTheOpen() + { + /* Pins the call shape, because the failure is silent and total: `sweep + "; " + enable` exits at the + sweep's `exit 0` and the rule is never created, while the verb still reports success. I wrote that + bug and caught it before it ran. */ + var source = ReadRepoFile(Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "DarlingCliCommands.cs")); + + Assert.DoesNotContain("BuildFirewallSweepCommand(wildcard) + ", source, StringComparison.Ordinal); + Assert.DoesNotContain("sweep + \"; \"", source, StringComparison.Ordinal); + Assert.Contains("BuildFirewallSweepCommand(wildcard)", source, StringComparison.Ordinal); + } + /* ---- no retry spam: report once per state, not once per supervisor tick ---- */ [Fact] diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index 203bf70d24..a72073e878 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -1970,19 +1970,26 @@ public static async Task ConfigureFirewallAsync( output.WriteLine($"{plan.Surface}: {plan.Note}."); } - if (plan.Action == FirewallRuleAction.Remove) + /* Sweep this surface's rules for EVERY port FIRST, as its own step. The port is part of the rule + name, so changing a port does not update a rule — it makes a different one and strands the old as + an inbound allow rule on a port nothing serves. Reconciling by exact name could never reach that. + Its own step, not concatenated ahead of the open below, because the sweep command ends in exit 0 + and would otherwise terminate the shell before the rule was created. */ + var wildcard = DarlingFirewallCheck.SurfaceRuleWildcard(plan.RuleName); + if (!await TryRunFirewallStepAsync( + DarlingManagedPostgres.BuildFirewallSweepCommand(wildcard), + plan.Action == FirewallRuleAction.Remove + ? $"{plan.Surface}: loopback-only — no rule needed (removed any '{wildcard}')." + : $"{plan.Surface}: cleared any previous rule matching '{wildcard}'.", + $"{plan.Surface}: could not remove the rule(s) matching '{wildcard}'", + output, error, cancellationToken)) { - /* Loopback-only: the desired state is no rule. Removal is idempotent (an absent rule exits 0), - so this both cleans up after an exposure that was turned off and no-ops on a fresh install. */ - if (!await TryRunFirewallStepAsync( - DarlingManagedPostgres.BuildFirewallDisableCommand(plan.RuleName), - $"{plan.Surface}: loopback-only — no rule needed (removed '{plan.RuleName}' if it existed).", - $"{plan.Surface}: could not remove the rule '{plan.RuleName}'", - output, error, cancellationToken)) - { - failures++; - } + failures++; + } + if (plan.Action == FirewallRuleAction.Remove) + { + /* Loopback-only: the desired state is no rule at all, on any port — the sweep WAS the work. */ continue; } diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs index dc04c9c28c..25187f3b3c 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs @@ -78,6 +78,26 @@ internal static class DarlingFirewallCheck /// internal const string RuleNamePrefix = "PerformanceMonitor Darling "; + /// The separator every scoped rule name puts before its port, and the seam this class derives a + /// per-surface wildcard from. + private const string PortSuffixSeparator = " (port "; + + /// + /// PURE: the DisplayName wildcard covering EVERY port of one surface — "PerformanceMonitor Darling MCP + /// (port *)" for any MCP rule. The port is part of the rule NAME, so changing a port does not update a + /// rule, it makes a DIFFERENT one and strands the old: an inbound allow rule left open on a port the + /// service no longer listens on. Reconciling by exact name alone could never clean that up, so the + /// elevated verb sweeps this wildcard per surface before ensuring the desired rule. + /// An unrecognized name (no port suffix) returns UNCHANGED rather than gaining a wildcard. That is + /// the safety property, not a formality: a wildcard derived from a malformed name could match — and + /// therefore delete — rules this product does not own. + /// + internal static string SurfaceRuleWildcard(string ruleName) + { + var separator = ruleName.IndexOf(PortSuffixSeparator, StringComparison.Ordinal); + return separator < 0 ? ruleName : ruleName[..separator] + PortSuffixSeparator + "*)"; + } + /// /// PURE probe command: count the rules with this DisplayName and print the count behind /// . Deliberately shaped to ALWAYS exit 0 and never throw: diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs index 957940b414..daf5a9ebe1 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs @@ -2145,6 +2145,27 @@ internal static string BuildFirewallDisableCommand(string ruleName) => $"try {{ Remove-NetFirewallRule -DisplayName {SingleQuotedPowerShell(ruleName)} -ErrorAction Stop }} " + $"catch {{ if ($_.CategoryInfo.Category -ne 'ObjectNotFound') {{ throw }} }}; exit 0"; + /// + /// Removes EVERY rule matching a DisplayName wildcard — how the elevated verb clears one surface's rules + /// for ALL ports before ensuring the current one (#1771). The port lives in the rule NAME, so a port change + /// does not update a rule, it strands the old one as an inbound allow rule on a port nothing serves; only a + /// wildcard sweep can reach it. + /// Shaped exactly like rather than with + /// -ErrorAction SilentlyContinue, and for the same reason: SilentlyContinue hides the error TEXT but + /// still exits 1, so a genuine failure (access denied, leaving a stale allow rule behind) would report as + /// "exit 1:" with an EMPTY message — the trap that builder documents. Catching ObjectNotFound and + /// rethrowing anything else keeps a real failure loud and a no-op quiet. A wildcard matching nothing is not + /// an error here anyway (verified on Windows 11 26200, where the exact-name form DOES raise + /// ObjectNotFound), but the shape costs nothing and does not depend on that. + /// This is a COMPLETE command, not a fragment: the trailing exit 0 means it must be run as its + /// own step and never string-concatenated ahead of another command, which would terminate the shell before + /// that command ran. Callers MUST pass a wildcard from + /// , never an operator-supplied string. + /// + internal static string BuildFirewallSweepCommand(string displayNameWildcard) + => $"try {{ Remove-NetFirewallRule -DisplayName {SingleQuotedPowerShell(displayNameWildcard)} -ErrorAction Stop }} " + + $"catch {{ if ($_.CategoryInfo.Category -ne 'ObjectNotFound') {{ throw }} }}; exit 0"; + /// Last (rule, verdict) reported, so a repeated network reconcile restates a steady firewall /// state at most once (). private string? _lastFirewallRule; From c0ca56f7078ca918462ee1e19d6fd6b3cf14c490 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:50:45 -0400 Subject: [PATCH 3/4] Pin the installer's firewall calls by REASON, not by occurrence count (#1771) Review catch. The pin asserted "Invoke-FirewallReconcile occurs at least twice", but the design carries a definition plus TWO calls, so deleting the post-wizard call at install-darling.ps1:269 left two occurrences and the pin stayed green. Measured, not assumed: with that call removed the string still occurs 2 times. That call is not decorative. -Network runs --configure-network AFTER the pre-start reconcile, and the wizard rewrites the very exposure that reconcile acted on, so a newly exposed endpoint has no rule and one turned back off has a stale one. Losing it silently reproduces a subset of #1771 on every networked install. Both calls are now pinned by what they are FOR: - a call inside the if ($Network) branch, found by brace-matching the block - a call before Start-Service, so the first start already sees the rules Same vacuous-pin shape as the earlier Contains() that matched help text. Tests only, no production change. Co-Authored-By: Claude Fable 5 --- .../DarlingFirewallCheckTests.cs | 57 ++++++++++++++++++- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs index 280f61b0e4..75da0f6ea4 100644 --- a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs +++ b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs @@ -380,9 +380,62 @@ the exact invocation form plus proof the wrapper is called somewhere other than var script = ReadRepoFile(Path.Combine("Darling", "tools", "install-darling.ps1")); Assert.Contains("& $serviceExe --configure-firewall", script, StringComparison.Ordinal); + } + + [Fact] + public void InstallScript_ReconcilesTheFirewallBeforeTheFirstStart() + { + /* The pre-start call. Rules must exist before the service ever binds, so the very first start already + sees them - otherwise a fresh networked install spends its first start reporting a missing rule. */ + var script = ReadRepoFile(Path.Combine("Darling", "tools", "install-darling.ps1")); + + var call = script.IndexOf("\nInvoke-FirewallReconcile", StringComparison.Ordinal); + var start = script.IndexOf("Start-Service -Name $serviceName", StringComparison.Ordinal); + + Assert.True(call >= 0, "install-darling.ps1 never calls Invoke-FirewallReconcile at top level"); + Assert.True(start >= 0, "install-darling.ps1 no longer starts the service"); + Assert.True(call < start, "the firewall reconcile must run BEFORE Start-Service, not after"); + } + + [Fact] + public void InstallScript_ReReconcilesInsideTheNetworkWizardBranch() + { + /* The SECOND call, pinned by its REASON rather than by counting occurrences. -Network runs + --configure-network AFTER the pre-start reconcile, and the wizard rewrites the very exposure that + reconcile acted on: a newly exposed endpoint has no rule yet, and one turned back off has a stale + one. Losing this call silently reproduces a subset of #1771 on every networked install. + A count-based pin does not hold here. The design carries a definition plus TWO calls, so "at least + two occurrences" stays GREEN with this one deleted - the exact hole a reviewer caught, and the same + vacuous-pin shape as the earlier Contains() that matched the help text. */ + var script = ReadRepoFile(Path.Combine("Darling", "tools", "install-darling.ps1")); + var branch = ExtractBracedBlock(script, "if ($Network) {"); + + Assert.Contains("--configure-network", branch, StringComparison.Ordinal); + Assert.Contains("Invoke-FirewallReconcile", branch, StringComparison.Ordinal); + } + + /// Returns the body of the first brace-balanced block introduced by . + private static string ExtractBracedBlock(string script, string header) + { + var start = script.IndexOf(header, StringComparison.Ordinal); + Assert.True(start >= 0, $"expected '{header}' in the script"); + + var open = script.IndexOf('{', start); + var depth = 0; + for (var i = open; i < script.Length; i++) + { + if (script[i] == '{') + { + depth++; + } + else if (script[i] == '}' && --depth == 0) + { + return script[(open + 1)..i]; + } + } - var mentions = script.Split("Invoke-FirewallReconcile", StringSplitOptions.None).Length - 1; - Assert.True(mentions >= 2, $"Invoke-FirewallReconcile is defined but never called (found {mentions} occurrence(s))"); + Assert.Fail($"unbalanced braces after '{header}'"); + return ""; } [Theory] From 4eb3b4943ed5b9e4e8b287c2107f05bc338ffda3 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:01:18 -0400 Subject: [PATCH 4/4] Prove the sweep's call shape instead of blacklisting the bug's spelling (#1771) Both from the delta review. The M13 pin forbade two literal spellings of the concatenation bug, making it a tripwire for the regression I happened to hit rather than a proof. It now asserts POSITIVELY that the sweep sits in TryRunFirewallStepAsync's command position, over whitespace-normalized source so reformatting cannot break it, and does the same for the open. Measured against four spellings of the same silent regression: all four now go red, and two of them - no spaces around the +, and string.Concat - the old pin would have missed. string.Concat would also have defeated a trailing-comma-only assertion, which is why the pin proves the argument POSITION rather than the punctuation around it. SurfaceRuleWildcard's remark now states the consequence rather than just the intent: on the no-port-suffix path the sweep degrades to the exact-name removal the caller already performed, so the change is strictly additive in blast radius - it can only ever widen a delete to other ports of the SAME surface, never to anything else. That is provable from the two lines above it, and it is the claim a reviewer needs to be able to make about a delete path. Comment and tests only; no behavior change. Co-Authored-By: Claude Fable 5 --- .../DarlingFirewallCheckTests.cs | 29 +++++++++++++++---- .../DarlingFirewallCheck.cs | 5 +++- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs index 75da0f6ea4..71e9ed027b 100644 --- a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs +++ b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs @@ -228,15 +228,32 @@ public void ConfigureFirewall_RunsTheSweepAsItsOwnStep_NeverConcatenatedAheadOfT { /* Pins the call shape, because the failure is silent and total: `sweep + "; " + enable` exits at the sweep's `exit 0` and the rule is never created, while the verb still reports success. I wrote that - bug and caught it before it ran. */ - var source = ReadRepoFile(Path.Combine( - "Darling", "PerformanceMonitor.Darling.Service", "DarlingCliCommands.cs")); + bug and caught it before it ran. + Asserts POSITIVELY that the sweep is the first argument of its own step, rather than blacklisting + the spellings of the bug. A review noted the earlier form only forbade two literal spellings, so a + missing space around the +, an intermediate variable, or string.Concat would each have slipped past; + requiring the call to sit directly in TryRunFirewallStepAsync's command position rules all of those + out at once. Whitespace is normalized so reformatting cannot break the pin. */ + var source = NormalizeWhitespace(ReadRepoFile(Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "DarlingCliCommands.cs"))); - Assert.DoesNotContain("BuildFirewallSweepCommand(wildcard) + ", source, StringComparison.Ordinal); - Assert.DoesNotContain("sweep + \"; \"", source, StringComparison.Ordinal); - Assert.Contains("BuildFirewallSweepCommand(wildcard)", source, StringComparison.Ordinal); + Assert.Contains( + "TryRunFirewallStepAsync( DarlingManagedPostgres.BuildFirewallSweepCommand(wildcard),", + source, + StringComparison.Ordinal); + + /* Same proof for the open: a complete argument, never folded into a larger string. */ + Assert.Contains( + "TryRunFirewallStepAsync( DarlingManagedPostgres.BuildFirewallEnableCommand(plan.RuleName, plan.Port, plan.Cidr!),", + source, + StringComparison.Ordinal); } + /// Collapses every run of whitespace to a single space, so a source-shape pin survives + /// reformatting and line rewrapping while still proving the call structure. + private static string NormalizeWhitespace(string source) + => System.Text.RegularExpressions.Regex.Replace(source, @"\s+", " "); + /* ---- no retry spam: report once per state, not once per supervisor tick ---- */ [Fact] diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs index 25187f3b3c..3ad5bf8a3b 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingFirewallCheck.cs @@ -90,7 +90,10 @@ internal static class DarlingFirewallCheck /// elevated verb sweeps this wildcard per surface before ensuring the desired rule. /// An unrecognized name (no port suffix) returns UNCHANGED rather than gaining a wildcard. That is /// the safety property, not a formality: a wildcard derived from a malformed name could match — and - /// therefore delete — rules this product does not own. + /// therefore delete — rules this product does not own. The consequence is worth stating exactly, because it + /// is stronger than "safe" and provable from these two lines: on that path the caller's sweep degrades to + /// the exact-name removal it already performed, so this change is strictly additive in blast radius — + /// it can only ever widen a delete to OTHER PORTS OF THE SAME SURFACE, never to anything else. /// internal static string SurfaceRuleWildcard(string ruleName) {