From 691170b9c17217c7eba194c8692d9671cbf69ac5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:10:06 +0100 Subject: [PATCH 1/5] Close the two firewall residues #2432 disclosed (#2436) Both were filed as "arguably deliberate, decide it explicitly." Reading the code decided them: each is a case where --configure-firewall contradicts a posture the rest of the product already holds, and one of them is destructive rather than merely untidy. ## A rule for a surface that is switched off darling.json's mcp.network says HOW an endpoint would be exposed; whether it runs at all is config_service.mcp_enabled, and the supervisor stops the server outright when that is false. The planner read only the network block, so the elevated verb opened an inbound allow rule on a port with nothing behind it -- the same shape #2414 was filed about, one field over. The defence for leaving it was that the rule is ready for the day the surface is enabled, since enabling it from the Viewer is a store write with no elevated step in it. That is a real convenience and it loses to three things. The product's posture everywhere else is that no listener means no rule: --disable-mcp sweeps the surface, and DarlingMcpHostService's stop path already documents an admin removing the rule with THIS verb. The two verbs actually disagreed -- --disable-mcp closed the port and the next --configure-firewall re-opened it, and every upgrade runs that verb, so a deliberately disabled endpoint got its port re-opened by an upgrade and the disable did not stay done. And the convenience is deferred rather than lost, to one elevated action the service already asks for by name when its start-up check finds the rule missing. The rejected alternative is written down beside the decision, because it is the argument the next reader will have again. The install-time half is the same rule and not merely symmetry: before the store exists, mcp.enabled IS the value config_service.mcp_enabled is seeded with, so a network block beside enabled = false describes an endpoint that will not start on the first run either. Opening its port was never "ready for later", it was ready for nothing. ## The upgrade that lands on the pre-change port The issue described this as the rule landing on the old port for one cycle. It is worse than that, and the sharper reading is what decided the fix over "keep warning". --configure-firewall sweeps the surface's rules for EVERY port before opening the current one, and install-darling.ps1 runs it with the service STOPPED -- which on a managed install means the store is down too, it being a child of the service. So on an upgrade of a box whose port was moved in the Viewer, the rule matching that wildcard is the WORKING one. The elevated verb an operator was told to run is what closes the live LAN surface. So the sweep is now conditional on the run being able to vouch for the port. Its entire justification is knowing which port is live; a run that fell back to darling.json's seed does not know, and there a rule on another port is as likely to be the one being served as it is to be stale. The rule for the file's port is still created -- that is the fresh-install case, where the file cannot be wrong -- and the enable command removes its own exact DisplayName first, so declining the sweep costs nothing in idempotence. It defers the cleanup to a run that can see the control plane. When the store DID answer, the sweep is authoritative again and nothing changes, or this would trade one residue for the one #2432 was filed to remove. The installer then makes that later run happen: an upgrade re-reconciles once the service is up and the store can be asked. Only an upgrade, and that is the point rather than an optimisation -- a fresh install's first start spends about two minutes on initdb and the first migration, so a second call there could not read the store either and would re-print the fallback disclosure about a port that is correct by definition. Skipping it by construction beats skipping it by hoping the timing works out. It is not a guarantee, and the comment says so: the store answers when it answers, the verb bounds its read to ten seconds, and if it still cannot the operator has exactly the remedy they had before. The window narrows; it does not close. ## Verification The suite targets net10.0-windows and cannot run on macOS, so the real test file was compiled into a net10.0 xUnit-shim harness and run against both branches. 68 passed / 0 failed here. Five of the new assertions are expressible against dev and ALL FIVE are red there -- the two control-plane-disabled cases, the file-seed-disabled case, the installer's post-start guard, and the source pin that the sweep step is gated at all. The three that pin the sweep permission itself cannot be compiled against dev, which is the stronger statement; with those removed dev runs 60 passed / 5 failed against the same file. Five existing fixtures gained mcp.enabled = true. That is the fixtures becoming faithful rather than the assertions weakening: they meant "this endpoint is serving the LAN", which now has to say both halves. All five stay green against dev with that line added, which is what shows it changed nothing about what they were testing. Co-Authored-By: Claude Opus 5 --- .../DarlingFirewallCheckTests.cs | 194 ++++++++++++++++++ .../DarlingCliCommands.cs | 122 ++++++++--- Darling/tools/install-darling.ps1 | 36 ++++ 3 files changed, 329 insertions(+), 23 deletions(-) diff --git a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs index 2d93f94d37..f1a1dd2630 100644 --- a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs +++ b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs @@ -293,6 +293,10 @@ behind by an exposure that was turned off. */ public void PlanFirewallRules_ExposedMcp_OpensTheScopedRule() { var config = ManagedConfig(); + /* #2436: enabled AND exposed. mcp.enabled defaults false, and since #2436 a surface the effective + toggle has switched off gets its rule removed rather than opened — so a fixture that means "this + endpoint is serving the LAN" has to say both halves. */ + config.Mcp.Enabled = true; 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")); @@ -313,6 +317,7 @@ 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.Enabled = true; 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); @@ -329,6 +334,7 @@ public void PlanFirewallRules_NeverOpensAPortTheServiceWillNotExpose(string? all 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.Enabled = true; config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = allowFrom, Token = token }; var mcp = Assert.Single(PlansFor(config, "MCP")); @@ -344,6 +350,7 @@ public void PlanFirewallRules_ByoMode_OpensNothingAndSkipsTheStore() 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.Enabled = true; config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; var all = DarlingCliCommands.PlanFirewallRules(config); @@ -445,6 +452,7 @@ public void PlanFirewallRules_TheStoreSurfacesPortStaysFileResolved_AndCarriesNo public void PlanFirewallRules_StoreUnreadable_UsesTheFilePort_AndSaysSoRatherThanFallingBackQuietly() { var config = ManagedConfig(); + config.Mcp.Enabled = true; config.Mcp.Port = 5152; config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; @@ -467,6 +475,166 @@ public void PlanFirewallRules_LoopbackPlansCarryNoPortNote() DarlingCliCommands.PlanFirewallRules(ManagedConfig(), mcpStore: (true, 5199), webStore: (true, 5188)), p => Assert.Null(p.PortNote)); + /* ---- #2436: a rule follows the ENABLE flag too, and the sweep follows what the run can vouch for ---- */ + + /// + /// The residue #2432 disclosed. mcp.network describes HOW the endpoint would be exposed; whether it + /// runs at all is config_service.mcp_enabled, and the supervisor stops the server outright when that + /// is false. The planner read only the network block, so the elevated verb opened an inbound allow rule on + /// a port with nothing behind it — the exact shape #2414 was filed about, one field over. + /// The concrete bug, rather than the principle: --disable-mcp sweeps the surface's rules, and + /// the very next --configure-firewall re-created one. install-darling.ps1 runs that verb on every + /// upgrade, so a deliberately disabled endpoint got its port re-opened by an upgrade. + /// + [Fact] + public void PlanFirewallRules_ControlPlaneHasTheSurfaceOff_RemovesTheRuleInsteadOfOpeningADeadPort() + { + var config = ManagedConfig(); + config.Mcp.Enabled = true; /* the file's seed says yes... */ + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + /* ...and the control plane, which is what the supervisor obeys, says no. */ + var mcp = Assert.Single( + DarlingCliCommands.PlanFirewallRules(config, mcpStore: (false, 5152)).Where(p => p.Surface == "MCP")); + + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, mcp.Action); + Assert.Null(mcp.Cidr); + + /* And it says WHY, because "your exposure config is fine but no rule appeared" is otherwise an hour + of looking at the CIDR. */ + Assert.NotNull(mcp.Note); + Assert.Contains("config.config_service.mcp_enabled = false", mcp.Note, StringComparison.Ordinal); + Assert.Contains("--enable-mcp", mcp.Note, StringComparison.Ordinal); + + /* A Remove always sweeps: "no rule on any port" needs no opinion about which port is live, and this is + what makes a --disable-mcp stay done across the upgrade that re-runs this verb. */ + Assert.True(mcp.SweepOtherPorts); + } + + /// The web dashboard is the byte-identical twin, as it is everywhere in this area. + [Fact] + public void PlanFirewallRules_ControlPlaneHasTheWebSurfaceOff_RemovesTheRuleToo() + { + var config = ManagedConfig(); + config.Web.Enabled = true; + config.Web.Network = new WebNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + var web = Assert.Single( + DarlingCliCommands.PlanFirewallRules(config, webStore: (false, 5153)).Where(p => p.Surface == "web dashboard")); + + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, web.Action); + Assert.Contains("config.config_service.web_enabled = false", web.Note, StringComparison.Ordinal); + } + + /// + /// The install-time half of the same rule, and the reason it is not merely symmetry: at install time the + /// store does not exist, so mcp.enabled IS the value config_service.mcp_enabled will be seeded + /// with. A network block beside enabled = false therefore describes an endpoint that will not start + /// on the first run either — opening its port was never "ready for later", it was ready for nothing. + /// + [Fact] + public void PlanFirewallRules_FileSeedSaysTheSurfaceIsOff_OpensNothing_AndNamesTheSeedRatherThanTheStore() + { + var config = ManagedConfig(); + config.Mcp.Enabled = false; + 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.Remove, mcp.Action); + Assert.NotNull(mcp.Note); + Assert.Contains("mcp.enabled = false", mcp.Note, StringComparison.Ordinal); + Assert.Contains("SEEDED", mcp.Note, StringComparison.Ordinal); + + /* No port note: this plan opened nothing, so it made no port decision to disclose. */ + Assert.Null(mcp.PortNote); + } + + /// + /// The other #2432 residue, and the sharper half of it. The sweep removes this surface's rules on ports + /// other than the one being opened — that is how a rule stranded by a port change gets collected, and its + /// entire justification is that the sweeper KNOWS which port is live. A run that fell back to darling.json's + /// seed does not: install-darling.ps1 calls this verb with the service stopped, and on a managed install the + /// store is a child of the service, so on an upgrade of a box whose port was moved in the Viewer the rule + /// matching the wildcard is the WORKING one. Sweeping it closes a live LAN surface — the elevated verb an + /// operator was told to run becomes the outage. + /// + [Fact] + public void PlanFirewallRules_StoreUnreadable_DoesNotSweepTheSurfacesOtherPorts() + { + var config = ManagedConfig(); + config.Mcp.Enabled = true; + config.Mcp.Port = 5152; + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + var mcp = Assert.Single( + DarlingCliCommands.PlanFirewallRules(config, storeUnavailableReason: "the store did not answer within 10 seconds") + .Where(p => p.Surface == "MCP")); + + /* The rule for the file's port is still created — that is the fresh-install case, where the file cannot + be wrong — and the enable command removes its own exact DisplayName first, so declining the sweep + costs nothing in idempotence. */ + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Open, mcp.Action); + Assert.False(mcp.SweepOtherPorts); + } + + /// + /// And when the control plane DID answer, the sweep is authoritative again: the port is known, so every + /// other rule on this surface really is stranded and collecting it is the #2414 fix working. Withholding it + /// permanently would trade one residue for the one #2432 was filed to remove. + /// + [Fact] + public void PlanFirewallRules_ControlPlaneAnswered_SweepsTheSurfacesOtherPorts() + { + var config = ManagedConfig(); + config.Mcp.Enabled = true; + config.Mcp.Port = 5152; + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + var mcp = Assert.Single( + DarlingCliCommands.PlanFirewallRules(config, mcpStore: (true, 5199)).Where(p => p.Surface == "MCP")); + + Assert.Equal(5199, mcp.Port); + Assert.True(mcp.SweepOtherPorts); + } + + /// + /// The store surface never withholds the sweep, and that is not an oversight to be made consistent later: + /// postgres.port is file-only, with no config_service column able to disagree with it, so this + /// surface's port is never a guess even when the store cannot be read. + /// + [Fact] + public void PlanFirewallRules_TheStoreSurfaceAlwaysSweeps_BecauseItsPortIsNeverAGuess() + { + var config = ManagedConfig(); + config.Postgres!.Network = new PostgresNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24" }; + + var store = Assert.Single( + DarlingCliCommands.PlanFirewallRules(config, storeUnavailableReason: "the store did not answer within 10 seconds") + .Where(p => p.Surface == "store")); + + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Open, store.Action); + Assert.True(store.SweepOtherPorts); + } + + /// + /// The plan carries the decision; this is that the verb obeys it. Source-shaped for the same reason its + /// sibling above is: whether a step ran is not observable without a live Windows Firewall, and a flag on a + /// record that no caller reads would be invisible to every behavioural test in this file. + /// + [Fact] + public void TheSweepStepIsGatedOnThePlansSweepPermission() + { + var source = NormalizeWhitespace(ReadRepoFile(Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "DarlingCliCommands.cs"))); + + Assert.Contains( + "if (plan.SweepOtherPorts) { if (!await TryRunFirewallStepAsync( " + + "DarlingManagedPostgres.BuildFirewallSweepCommand(wildcard),", + source, + StringComparison.Ordinal); + } + /* ---- drift guards: the seams a rename or a deleted call would break silently ---- */ /// @@ -553,6 +721,32 @@ sees them - otherwise a fresh networked install spends its first start reporting Assert.True(call < start, "the firewall reconcile must run BEFORE Start-Service, not after"); } + /// + /// #2436: the THIRD call, and the one whose absence is silent. The pre-start reconcile runs with the + /// service stopped, and on a managed install the store is a child of the service — so on an UPGRADE the + /// verb has nothing to ask and falls back to darling.json's seed, which is the wrong port on a box whose + /// port was later moved in the Viewer. This call is what asks the store once it can answer. + /// Pinned by its GUARD as well as its presence. Running it unconditionally would fire it during a + /// fresh install's ~2-minute first start, where the store cannot answer either and the file port is + /// correct by definition — a second copy of the fallback disclosure about a port that is not wrong. The + /// guard is the difference between narrowing the window by construction and hoping the timing works + /// out. + /// + [Fact] + public void InstallScript_ReReconcilesAfterTheFirstStart_ButOnlyOnAnUpgrade() + { + var script = ReadRepoFile(Path.Combine("Darling", "tools", "install-darling.ps1")); + var branch = ExtractBracedBlock(script, "if ($isUpgrade) {"); + + Assert.Contains("Invoke-FirewallReconcile", branch, StringComparison.Ordinal); + + /* And it is genuinely AFTER the start, or it is just the pre-start call with extra steps. */ + var start = script.IndexOf("Start-Service -Name $serviceName", StringComparison.Ordinal); + var guard = script.IndexOf("if ($isUpgrade) {", StringComparison.Ordinal); + Assert.True(start >= 0, "install-darling.ps1 no longer starts the service"); + Assert.True(guard > start, "the upgrade reconcile must run AFTER Start-Service, not before"); + } + [Fact] public void InstallScript_ReReconcilesInsideTheNetworkWizardBranch() { diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index b3f88587c8..1e1c0a6d03 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -2722,6 +2722,13 @@ It reconciles all THREE surfaces (store, MCP, web) in one pass. #2414 gave it ON and never a precondition — this verb still has to work at install time, before the store exists, where the file's port is the value the store will be seeded WITH. When the read fails, the verb says which port it used and why, and it never falls back silently: see TryReadEndpointTogglesAsync. + + #2436 followed the same read one step further, into what the verb is entitled to DO with it. The store + row carries an enable flag as well as a port, so a surface the control plane has switched off gets its + rule removed rather than opened — the posture --disable-mcp already had, which this verb used to undo on + every upgrade (see DescribeDisabledSurface). And when the read FAILS, the verb keeps creating the file's + rule but stops removing this surface's rules on other ports, because that removal's whole justification + is knowing which port is live (see FirewallRulePlan.SweepOtherPorts). ================================================================================================ */ /// What --configure-firewall will do to one surface's rule. @@ -2736,12 +2743,21 @@ public enum FirewallRuleAction } /// 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). + /// alongside, non-null only where the reason is not self-evident (a fail-closed exposure, or a surface the + /// control plane has switched off). /// (#2414) says which PLANE supplied and, when the store /// could not be read, what would make the chosen port wrong — non-null only on an Open plan, since a Remove - /// sweeps every port of the surface and does not depend on picking the right one. + /// sweeps every port of the surface and does not depend on picking the right one. + /// (#2436) is permission to remove this surface's rules on ports + /// OTHER than . That sweep is how a rule stranded by a port change gets collected, + /// and its whole justification is that the sweeper knows which port is live — so it is withheld on exactly + /// one combination: an Open plan whose port came from darling.json's seed because the store could not be + /// read. There, every other port is equally likely to be the port the endpoint is actually serving, and + /// removing it would close a working LAN surface to collect a rule that might not be stale at all. A + /// Remove plan always sweeps: "no rule on any port" needs no knowledge of which port is live. public readonly record struct FirewallRulePlan( - string Surface, string RuleName, int Port, FirewallRuleAction Action, string? Cidr, string? Note, string? PortNote); + string Surface, string RuleName, int Port, FirewallRuleAction Action, string? Cidr, string? Note, string? PortNote, + bool SweepOtherPorts = true); /// /// PURE desired-state for all three rules, so the whole decision pins without a live firewall. @@ -2782,7 +2798,10 @@ public static IReadOnlyList PlanFirewallRules( store.Exposed ? FirewallRuleAction.Open : FirewallRuleAction.Remove, store.Exposed ? store.Cidr : null, store.DegradeReason, - null)); + null, + /* postgres.port is file-only — there is no config_service column able to disagree with it — so + this surface's port is never a guess and the sweep is always authoritative. */ + SweepOtherPorts: true)); } /* #2414: the SAME resolver the MCP/web supervisors bind on, so the rule this verb writes is named for the @@ -2791,30 +2810,40 @@ describer turns into the loud fallback disclosure rather than a silent guess. */ var mcpToggle = DarlingHostBinding.ResolveEndpointToggle(mcpStore, config.Mcp.Enabled, config.Mcp.Port); var mcpBind = DarlingMcpHostService.ResolveMcpBind(config.Mcp, managed); var mcpExposed = mcpBind.Mode == DarlingMcpHostService.McpBindMode.NetworkAndLoopback; + var mcpOpen = mcpExposed && mcpToggle.Enabled; plans.Add(new FirewallRulePlan( "MCP", DarlingMcpHostService.McpFirewallRuleName(mcpToggle.Port), mcpToggle.Port, - mcpExposed ? FirewallRuleAction.Open : FirewallRuleAction.Remove, - mcpExposed ? CanonicalCidrOrNull(config.Mcp.Network?.AllowFrom) : null, - mcpExposed ? null : DescribeLoopbackReason(mcpBind.Reason, "mcp"), - mcpExposed + mcpOpen ? FirewallRuleAction.Open : FirewallRuleAction.Remove, + mcpOpen ? CanonicalCidrOrNull(config.Mcp.Network?.AllowFrom) : null, + mcpOpen + ? null + : mcpExposed ? DescribeDisabledSurface(mcpToggle, "mcp") : DescribeLoopbackReason(mcpBind.Reason, "mcp"), + mcpOpen ? DarlingHostBinding.DescribeFirewallPortAuthority(mcpToggle, "mcp", "MCP", config.Mcp.Port, storeUnavailableReason) - : null)); + : null, + SweepOtherPorts: !mcpOpen || mcpToggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane)); var webToggle = DarlingHostBinding.ResolveEndpointToggle(webStore, config.Web.Enabled, config.Web.Port); var webBind = DarlingWebHostService.ResolveWebBind(config.Web, managed); var webExposed = webBind.Mode == DarlingHostBinding.BindMode.NetworkAndLoopback; + var webOpen = webExposed && webToggle.Enabled; plans.Add(new FirewallRulePlan( "web dashboard", DarlingWebHostService.WebFirewallRuleName(webToggle.Port), webToggle.Port, - webExposed ? FirewallRuleAction.Open : FirewallRuleAction.Remove, - webExposed ? CanonicalCidrOrNull(config.Web.Network?.AllowFrom) : null, - webExposed ? null : DescribeLoopbackReason((DarlingMcpHostService.McpBindReason)webBind.Reason, "web"), - webExposed + webOpen ? FirewallRuleAction.Open : FirewallRuleAction.Remove, + webOpen ? CanonicalCidrOrNull(config.Web.Network?.AllowFrom) : null, + webOpen + ? null + : webExposed + ? DescribeDisabledSurface(webToggle, "web") + : DescribeLoopbackReason((DarlingMcpHostService.McpBindReason)webBind.Reason, "web"), + webOpen ? DarlingHostBinding.DescribeFirewallPortAuthority(webToggle, "web", "web dashboard", config.Web.Port, storeUnavailableReason) - : null)); + : null, + SweepOtherPorts: !webOpen || webToggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane)); return plans; } @@ -2825,6 +2854,32 @@ describer turns into the loud fallback disclosure rather than a silent guess. */ private static string? CanonicalCidrOrNull(string? allowFrom) => ClassifyAllowFrom(allowFrom, out var canonical) == EndpointAllowFromVerdict.Valid ? canonical : null; + /// + /// Why a surface that IS configured for LAN exposure still gets no rule: it is switched OFF (#2436), so + /// the supervisor never starts it and there is nothing behind the port. + /// + /// The decision, because the alternative is arguable. Leaving the rule open would mean an + /// operator who enables the endpoint from the Viewer's Settings — a store write the running service picks + /// up within one poll interval, with no elevated step anywhere in it — finds the LAN path already open. + /// That convenience is real, and it is what this verb used to do. It loses to three things. The product's + /// own posture everywhere else is that no listener means no rule: --disable-mcp sweeps the surface, + /// 's stop path documents an admin removing the rule with THIS verb, + /// and #2414 was fixed precisely because an inbound allow on a port nothing serves is the inverse of what + /// scoping a rule to a port is for. Worse, the two disagreed: --disable-mcp closed the port and the + /// next --configure-firewall — which every upgrade runs — silently re-opened it, so the disable verb + /// did not stay done. And the convenience is not lost, only deferred to one elevated action the service + /// already asks for by name: its start-up check reports the missing rule and prints the command. + /// + private static string DescribeDisabledSurface(DarlingHostBinding.EndpointToggle toggle, string section) + => toggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane + ? $"{section}.network exposes this endpoint, but the CONTROL PLANE has it off " + + $"(config.config_service.{section}_enabled = false), so the service does not start it and no rule " + + $"belongs on that port — turn it on with --enable-{section} or in the Viewer's Settings, then " + + "re-run --configure-firewall from an elevated prompt" + : $"{section}.network exposes this endpoint, but {section}.enabled = false, which is the value the " + + "store is SEEDED from, so the service will not start it and no rule belongs on that port — turn " + + $"it on with --enable-{section} (or set {section}.enabled before the first start)"; + /// 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) => @@ -3124,17 +3179,38 @@ these commands by hand can see which plane chose the ports baked into them. */ 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. */ + and would otherwise terminate the shell before the rule was created. + + #2436: and the sweep's authority to remove OTHER ports comes from knowing which port is live. On + an Open plan whose port fell back to darling.json's seed because the store could not answer, that + is precisely the knowledge missing — a rule on another port is as likely to be the one the + endpoint is being served on as it is to be stale, and removing it turns the elevated verb an + operator was told to run into an outage of the surface it was meant to repair. That is not + hypothetical: install-darling.ps1 runs this verb with the service STOPPED, so on an upgrade of a + box whose port was moved in the Viewer it is the working rule that matched the wildcard. The + enable command below removes its own exact DisplayName first, so declining the sweep costs + nothing in idempotence — it defers the cleanup to a run that can see the control plane, which is + what the installer's post-start reconcile is for. */ 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)) + if (plan.SweepOtherPorts) { - failures++; + if (!await TryRunFirewallStepAsync( + DarlingManagedPostgres.BuildFirewallSweepCommand(wildcard), + plan.Action == FirewallRuleAction.Remove + ? $"{plan.Surface}: no rule belongs on any port (removed any '{wildcard}')." + : $"{plan.Surface}: cleared any previous rule matching '{wildcard}'.", + $"{plan.Surface}: could not remove the rule(s) matching '{wildcard}'", + output, error, cancellationToken)) + { + failures++; + } + } + else + { + output.WriteLine( + $"{plan.Surface}: leaving any rule on another port ALONE. This run could not read the control " + + "plane, so it cannot tell a rule stranded by a port change from the rule the endpoint is " + + "actually being served on — re-run --configure-firewall with the service up to collect it."); } if (plan.Action == FirewallRuleAction.Remove) diff --git a/Darling/tools/install-darling.ps1 b/Darling/tools/install-darling.ps1 index 805b35dc9d..6fd7ca0b09 100644 --- a/Darling/tools/install-darling.ps1 +++ b/Darling/tools/install-darling.ps1 @@ -206,6 +206,12 @@ if (-not (Test-Path $serviceExe)) { # This runs BEFORE the pre-flight, the Event Log source, and service creation, so a doomed location costs # nothing and leaves nothing behind. $existing = Get-Service -Name $serviceName -ErrorAction SilentlyContinue + +# The same fact as a boolean, for the post-start firewall reconcile at 5a (#2436). Captured here with +# $existing because it stops being observable the moment sc.exe create runs, and named rather than reusing +# $existing at the far end of the script because what 5a actually depends on is not "a service object was +# found" but "a STORE already exists to be asked" - which is what an existing service implies. +$isUpgrade = $null -ne $existing # Classify the NORMALIZED spelling, but keep installing to $root exactly as given (#2348). The \\?\ prefix # instructs the path parser and is not part of where the install lives, so stripping it for the decision # changes which rules see the path and nothing about where files land. @@ -516,6 +522,12 @@ if ($failed.Count -gt 0) { # 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. # +# On a FRESH install that is not merely safe, it is the only correct time: config_service does not exist yet +# and is seeded FROM darling.json, so the file is the control plane's future answer and cannot be wrong. +# On an UPGRADE it can be: the store already holds a port that may have been moved in the Viewer, and the +# managed store is a child of the service, so with the service stopped there is nothing here to ask. That is +# what 5a below exists for - see the reasoning there. +# # Best-effort: a firewall failure warns rather than aborting an otherwise good install. Re-run it by hand. function Invoke-FirewallReconcile { & $serviceExe --configure-firewall @@ -536,6 +548,30 @@ Start-Service -Name $serviceName Write-Host "Service is Running. First start does real work (unpack pg-runtime, initdb, store migration, first collection cycle) - give it ~2 minutes." -ForegroundColor Green Write-Host "Primary log: %ProgramData%\PerformanceMonitorDarling\logs\darling-service_yyyyMMdd.log" +# -- 5a. Re-reconcile the firewall on an UPGRADE (#2436) ------------------------------------------- +# The 4c call ran with the service stopped, which on a managed install means the store was down too - it is +# a child of the service. So on an upgrade the verb fell back to darling.json's mcp.port / web.port, and on +# a box whose port was later moved in the Viewer's Settings that is the WRONG port: the rule lands where +# nothing listens while the served port stays shut. The verb says so when it falls back, and the running +# service's own start-up check then WARNs the exact command - so this always did self-heal with one operator +# action. This removes the need for that action in the case where it was never necessary, because the store +# is up now and can simply be asked. +# +# Only on an upgrade, and that is the point rather than an optimisation. On a fresh install the first start +# is doing initdb and the first migration for ~2 minutes, so a second call here could not read the store +# either - it would re-print the same fallback disclosure more loudly about a port that, on a fresh box, is +# by definition correct. Skipping it by construction beats skipping it by hoping the timing works out. +# +# Not a guarantee: the store answers when it answers, and the verb bounds its read to ten seconds. If it +# still cannot, this run says exactly what the 4c run said and the operator has the same remedy they had +# before - so the window narrows rather than closing, which is the honest claim. The verb is idempotent, so +# a redundant run costs nothing but output. +if ($isUpgrade) { + Write-Host '' + Write-Host 'Re-applying the firewall rules now that the store can answer (upgrade path)...' -ForegroundColor Cyan + Invoke-FirewallReconcile +} + # -- 5b. Optional guided network setup ------------------------------------------------------------ # Runs elevated (this whole script is), so the wizard's restart-to-apply works, and its restart is what # generates the store TLS cert on the first exposed start. Loopback-only stays the default without -Network. From 055ddbeb808fcdddd5124371fd4872582dce720e Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:21:02 +0100 Subject: [PATCH 2/5] Withhold the sweep on the enable flag too, not only the port (review) The first cut let every Remove plan sweep unconditionally, on the reasoning that "no rule on any port" needs no opinion about which port is live. That is true of the port and false of the flag, and review caught the difference. Enabling MCP is a store-only write by design (#2389): --enable-mcp and the Viewer never write back to darling.json, so on a long-lived box mcp.enabled = false is not a statement about anything, it is the seed nobody edited. Read as "off" on a run that could not reach the control plane, it now produced a Remove - which this change had just made possible - and the wildcard deleted the rule for the port the endpoint is actually serving on. install-darling.ps1 runs this verb with the service stopped, so that is the upgrade path rather than a corner, and it is the same outage this PR exists to prevent arriving through the other field. The fix is not "never sweep on a fallback", which would stop the installer collecting rules left behind by an exposure that was turned off. Whether a surface is exposed at ALL is mcp.network / web.network, which are file-only and have no config_service equivalent precisely so that exposure requires touching the host - the control plane can switch an exposed surface off, never switch an unexposed one on. So a bind that resolves loopback-only is loopback-only whatever the store would have said, and its sweep needs no knowledge this run is missing. The sweep is withheld for exactly the surfaces the FILE exposes on a run that could not read the control plane, where both store-backed values are a guess and either way of guessing wrong deletes a live rule. Nothing is exposed by deferring, and the message says so: the store is unreadable because the service is stopped, so no Darling endpoint is listening on any port until a run that can read the control plane is possible again. Two tests, one per direction. 70 pass / 0 fail on this branch through the net10.0 xUnit-shim harness; the same file with the five sweep-permission tests removed runs 60 / 5 against dev. Co-Authored-By: Claude Opus 5 --- .../DarlingFirewallCheckTests.cs | 59 +++++++++++++++++++ .../DarlingCliCommands.cs | 48 ++++++++++----- 2 files changed, 93 insertions(+), 14 deletions(-) diff --git a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs index f1a1dd2630..879e201ad8 100644 --- a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs +++ b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs @@ -548,6 +548,65 @@ public void PlanFirewallRules_FileSeedSaysTheSurfaceIsOff_OpensNothing_AndNamesT /* No port note: this plan opened nothing, so it made no port decision to disclose. */ Assert.Null(mcp.PortNote); + + /* And it does NOT sweep — see the test below, which is the reason. */ + Assert.False(mcp.SweepOtherPorts); + } + + /// + /// Caught in review on the first cut of this change, and it is the same outage arriving through the other + /// field. Enabling MCP is a store-only write by design (#2389) — --enable-mcp and the Viewer never + /// write back to darling.json — so on a long-lived box the file's mcp.enabled = false is not a + /// statement about anything, it is just the seed nobody edited. Read that as "off" on a run that could not + /// reach the control plane, and the plan becomes a Remove; let a Remove sweep unconditionally, on the + /// reasoning that "no rule on any port" needs no knowledge of which port is live, and the wildcard deletes + /// the rule for the port the endpoint IS serving on. install-darling.ps1 runs this verb with the service + /// stopped, so that is the upgrade path, not a corner. + /// The fix is not "never sweep on a fallback": whether a surface is exposed at ALL is + /// mcp.network, which is file-only and which the control plane can only ever switch OFF. So the + /// sweep is withheld for exactly the surfaces the file exposes, and the loopback cases below keep it. + /// + [Fact] + public void PlanFirewallRules_StoreUnreadable_DoesNotSweepAwayARuleTheControlPlaneMayBeServingOn() + { + var config = ManagedConfig(); + config.Mcp.Enabled = false; /* the untouched seed — --enable-mcp wrote the store, not this */ + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + var mcp = Assert.Single( + DarlingCliCommands.PlanFirewallRules(config, storeUnavailableReason: "the store did not answer within 10 seconds") + .Where(p => p.Surface == "MCP")); + + Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, mcp.Action); + Assert.False(mcp.SweepOtherPorts); + + /* The web twin has the identical shape and the identical hole. */ + var webConfig = ManagedConfig(); + webConfig.Web.Enabled = false; + webConfig.Web.Network = new WebNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + Assert.False( + Assert.Single( + DarlingCliCommands.PlanFirewallRules(webConfig, storeUnavailableReason: "the store did not answer within 10 seconds") + .Where(p => p.Surface == "web dashboard")) + .SweepOtherPorts); + } + + /// + /// The other side of that fix, and the reason it is not simply "never sweep on a fallback". A surface the + /// file does not expose on the LAN is loopback-only whatever the store would have said — mcp.network + /// has no config_service equivalent, by the #2389 design, precisely so that exposure requires touching the + /// host. So no rule belongs on any port, that conclusion needs nothing this run could not read, and the + /// installer keeps collecting rules left behind by an exposure that was removed from darling.json. + /// + [Fact] + public void PlanFirewallRules_ALoopbackSurfaceAlwaysSweeps_BecauseNetworkExposureIsFileOnly() + { + var plans = DarlingCliCommands.PlanFirewallRules( + ManagedConfig(), storeUnavailableReason: "the store did not answer within 10 seconds"); + + Assert.All(plans, p => Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, p.Action)); + Assert.All(plans, p => Assert.True(p.SweepOtherPorts)); } /// diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index 1e1c0a6d03..a873504afb 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -2726,9 +2726,10 @@ It reconciles all THREE surfaces (store, MCP, web) in one pass. #2414 gave it ON #2436 followed the same read one step further, into what the verb is entitled to DO with it. The store row carries an enable flag as well as a port, so a surface the control plane has switched off gets its rule removed rather than opened — the posture --disable-mcp already had, which this verb used to undo on - every upgrade (see DescribeDisabledSurface). And when the read FAILS, the verb keeps creating the file's - rule but stops removing this surface's rules on other ports, because that removal's whole justification - is knowing which port is live (see FirewallRulePlan.SweepOtherPorts). + every upgrade (see DescribeDisabledSurface). And when the read FAILS, the verb stops removing a + LAN-exposed surface's existing rules at all — neither the port nor the enable flag is knowable then, and + both are ways to end up deleting the rule the endpoint is actually being served on. It still creates the + file's rule, which is what a fresh install needs (see FirewallRulePlan.SweepOtherPorts). ================================================================================================ */ /// What --configure-firewall will do to one surface's rule. @@ -2750,11 +2751,21 @@ public enum FirewallRuleAction /// sweeps every port of the surface and does not depend on picking the right one. /// (#2436) is permission to remove this surface's rules on ports /// OTHER than . That sweep is how a rule stranded by a port change gets collected, - /// and its whole justification is that the sweeper knows which port is live — so it is withheld on exactly - /// one combination: an Open plan whose port came from darling.json's seed because the store could not be - /// read. There, every other port is equally likely to be the port the endpoint is actually serving, and - /// removing it would close a working LAN surface to collect a rule that might not be stale at all. A - /// Remove plan always sweeps: "no rule on any port" needs no knowledge of which port is live. + /// and its whole justification is that the sweeper knows what this surface is actually doing — so it is + /// withheld on exactly one combination: a surface darling.json exposes on the LAN, on a run that could not + /// read the control plane. There, BOTH store-backed values are a guess. The port may have moved, so another + /// port's rule may be the live one; and mcp_enabled may have been turned on with --enable-mcp + /// or in the Viewer, which never writes back to darling.json — so the file's enabled = false may be + /// years stale and the surface may be serving right now. Either way the wildcard would delete the rule the + /// endpoint is being served on, which is the outage this whole change exists to prevent. Nothing is exposed + /// by deferring: the store is unreadable because the service is stopped, so no Darling endpoint is + /// listening on any port until a run that CAN read the control plane is possible again. + /// The other half is certain and always sweeps. Whether a surface is network-exposed at all is + /// decided by mcp.network / web.network, which are FILE-ONLY and have no config_service + /// equivalent by design (#2389) — the control plane can switch an exposed surface off, never switch an + /// unexposed one on. So a bind that resolves loopback-only is loopback-only whatever the store would have + /// said, no rule belongs on any port, and collecting them needs no knowledge this run is missing. Same for + /// the store surface, whose postgres.port has no config_service column to disagree with it. public readonly record struct FirewallRulePlan( string Surface, string RuleName, int Port, FirewallRuleAction Action, string? Cidr, string? Note, string? PortNote, bool SweepOtherPorts = true); @@ -2823,7 +2834,11 @@ describer turns into the loud fallback disclosure rather than a silent guess. */ mcpOpen ? DarlingHostBinding.DescribeFirewallPortAuthority(mcpToggle, "mcp", "MCP", config.Mcp.Port, storeUnavailableReason) : null, - SweepOtherPorts: !mcpOpen || mcpToggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane)); + /* NOT !mcpOpen: a Remove reached because the FILE said disabled is exactly as much a guess as a + stale port, because --enable-mcp and the Viewer write only the store. mcpExposed is the half + that is certain — network.* is file-only, so the control plane can never expose what the file + does not. */ + SweepOtherPorts: !mcpExposed || mcpToggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane)); var webToggle = DarlingHostBinding.ResolveEndpointToggle(webStore, config.Web.Enabled, config.Web.Port); var webBind = DarlingWebHostService.ResolveWebBind(config.Web, managed); @@ -2843,7 +2858,7 @@ describer turns into the loud fallback disclosure rather than a silent guess. */ webOpen ? DarlingHostBinding.DescribeFirewallPortAuthority(webToggle, "web", "web dashboard", config.Web.Port, storeUnavailableReason) : null, - SweepOtherPorts: !webOpen || webToggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane)); + SweepOtherPorts: !webExposed || webToggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane)); return plans; } @@ -3207,10 +3222,15 @@ what the installer's post-start reconcile is for. */ } else { - output.WriteLine( - $"{plan.Surface}: leaving any rule on another port ALONE. This run could not read the control " + - "plane, so it cannot tell a rule stranded by a port change from the rule the endpoint is " + - "actually being served on — re-run --configure-firewall with the service up to collect it."); + output.WriteLine(plan.Action == FirewallRuleAction.Open + ? $"{plan.Surface}: leaving any rule on ANOTHER port alone. This run could not read the control " + + "plane, so it cannot tell a rule stranded by a port change from the rule the endpoint is " + + "actually being served on — re-run --configure-firewall with the service up to collect it." + : $"{plan.Surface}: leaving this surface's existing rules alone rather than removing them. " + + "darling.json exposes this endpoint and says it is switched off, but the --enable-* verbs and " + + "the Viewer write only the control plane, which this run could not read — so that off may be " + + "stale and the rule may be the one the endpoint is being served on. Nothing is listening while " + + "the service is stopped; re-run --configure-firewall with it up to reconcile."); } if (plan.Action == FirewallRuleAction.Remove) From 0b3fb8e023a56a46e806bddbca5ecf82ad5824e4 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:29:44 +0100 Subject: [PATCH 3/5] Say the disabled-surface note is a reading, not a fact (review) The File-origin half of DescribeDisabledSurface was written as though a File origin meant "fresh install". It does not: TryReadEndpointTogglesAsync collapses BYO, a missing credential, a ten-second timeout and every other connection failure into the same answer, so that branch is also reached on a long-lived box whose store is merely unreachable this minute - one that may already hold mcp_enabled = true. So the same run printed two lines that contradicted each other. The note said the endpoint is off and framed the value as something to set before the first start; the sweep-declined line a few lines later said the off may be stale and the rule may be the one the endpoint is being served on. The second line is the one that is right - it is the whole reason the sweep is being withheld - and the first actively misled an operator into believing a surface was definitely off in exactly the case where the code had just concluded it might not be. DescribeDisabledSurface now takes storeUnavailableReason and says the same three-state thing DescribeFirewallPortAuthority already says about the port: which plane answered, why it could do no better, and BOTH readings of the value it fell back on - correct on a box whose store has never been written, possibly stale on one that has run before, because --enable-mcp and the Viewer write only the control plane and never back to the file. The control-plane branch keeps its flat wording, because there it really is a fact, and a test pins that the two do not collapse into one hedged message that under-states a certain answer. 71 pass / 0 fail through the net10.0 xUnit-shim harness. Co-Authored-By: Claude Opus 5 --- .../DarlingFirewallCheckTests.cs | 40 ++++++++++++++++++- .../DarlingCliCommands.cs | 29 +++++++++++--- 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs index 879e201ad8..73c3d4c926 100644 --- a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs +++ b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs @@ -531,20 +531,38 @@ public void PlanFirewallRules_ControlPlaneHasTheWebSurfaceOff_RemovesTheRuleToo( /// store does not exist, so mcp.enabled IS the value config_service.mcp_enabled will be seeded /// with. A network block beside enabled = false therefore describes an endpoint that will not start /// on the first run either — opening its port was never "ready for later", it was ready for nothing. + /// Review's second finding, and the reason the note offers that as a READING rather than a fact: a + /// File origin is not "fresh install". Every store-read failure — BYO, a missing credential, a timeout — + /// collapses into it, so this branch is also reached on a long-lived box whose store is merely unreachable + /// this minute, where mcp.enabled may be a seed nobody has touched since --enable-mcp wrote the + /// store. Asserting the endpoint is off there would contradict the sweep-declined line the same run prints + /// a few lines later, which says the off may be stale and the rule may be the live one — and that line is + /// the one that is right. /// [Fact] - public void PlanFirewallRules_FileSeedSaysTheSurfaceIsOff_OpensNothing_AndNamesTheSeedRatherThanTheStore() + public void PlanFirewallRules_FileSeedSaysTheSurfaceIsOff_OpensNothing_AndOffersThatAsAReadingNotAFact() { var config = ManagedConfig(); config.Mcp.Enabled = false; 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")); + var mcp = Assert.Single( + DarlingCliCommands.PlanFirewallRules(config, storeUnavailableReason: "the store did not answer within 10 seconds") + .Where(p => p.Surface == "MCP")); Assert.Equal(DarlingCliCommands.FirewallRuleAction.Remove, mcp.Action); Assert.NotNull(mcp.Note); + + /* Which plane it went on and why it could not do better — the shape DescribeFirewallPortAuthority + already applies to the port, now applied to the flag. */ + Assert.Contains("could NOT be read", mcp.Note, StringComparison.Ordinal); + Assert.Contains("the store did not answer within 10 seconds", mcp.Note, StringComparison.Ordinal); Assert.Contains("mcp.enabled = false", mcp.Note, StringComparison.Ordinal); + + /* BOTH readings, so neither box's operator is misled: right at install time... */ Assert.Contains("SEEDED", mcp.Note, StringComparison.Ordinal); + /* ...and possibly stale on one that has run before. */ + Assert.Contains("may be stale", mcp.Note, StringComparison.Ordinal); /* No port note: this plan opened nothing, so it made no port decision to disclose. */ Assert.Null(mcp.PortNote); @@ -553,6 +571,24 @@ public void PlanFirewallRules_FileSeedSaysTheSurfaceIsOff_OpensNothing_AndNamesT Assert.False(mcp.SweepOtherPorts); } + /// The confirmed half stays a flat statement, because there it IS one: the control plane answered + /// and said off, so nothing is being inferred from a file the enable path never writes. Pinned so the two + /// wordings cannot be collapsed into one hedged message that under-states a certain answer. + [Fact] + public void PlanFirewallRules_ControlPlaneAnsweredOff_SaysSoWithoutAStalenessCaveat() + { + var config = ManagedConfig(); + config.Mcp.Enabled = true; + config.Mcp.Network = new McpNetworkConfig { Listen = "192.168.1.205", AllowFrom = "192.168.1.0/24", Token = "t" }; + + var note = Assert.Single( + DarlingCliCommands.PlanFirewallRules(config, mcpStore: (false, 5152)).Where(p => p.Surface == "MCP")).Note; + + Assert.Contains("the CONTROL PLANE has it off", note, StringComparison.Ordinal); + Assert.DoesNotContain("may be stale", note, StringComparison.Ordinal); + Assert.DoesNotContain("could NOT be read", note, StringComparison.Ordinal); + } + /// /// Caught in review on the first cut of this change, and it is the same outage arriving through the other /// field. Enabling MCP is a store-only write by design (#2389) — --enable-mcp and the Viewer never diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index a873504afb..ec30dfbfd7 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -2830,7 +2830,9 @@ describer turns into the loud fallback disclosure rather than a silent guess. */ mcpOpen ? CanonicalCidrOrNull(config.Mcp.Network?.AllowFrom) : null, mcpOpen ? null - : mcpExposed ? DescribeDisabledSurface(mcpToggle, "mcp") : DescribeLoopbackReason(mcpBind.Reason, "mcp"), + : mcpExposed + ? DescribeDisabledSurface(mcpToggle, "mcp", storeUnavailableReason) + : DescribeLoopbackReason(mcpBind.Reason, "mcp"), mcpOpen ? DarlingHostBinding.DescribeFirewallPortAuthority(mcpToggle, "mcp", "MCP", config.Mcp.Port, storeUnavailableReason) : null, @@ -2853,7 +2855,7 @@ does not. */ webOpen ? null : webExposed - ? DescribeDisabledSurface(webToggle, "web") + ? DescribeDisabledSurface(webToggle, "web", storeUnavailableReason) : DescribeLoopbackReason((DarlingMcpHostService.McpBindReason)webBind.Reason, "web"), webOpen ? DarlingHostBinding.DescribeFirewallPortAuthority(webToggle, "web", "web dashboard", config.Web.Port, storeUnavailableReason) @@ -2884,16 +2886,31 @@ does not. */ /// next --configure-firewall — which every upgrade runs — silently re-opened it, so the disable verb /// did not stay done. And the convenience is not lost, only deferred to one elevated action the service /// already asks for by name: its start-up check reports the missing rule and prints the command. + /// + /// Two messages, on the same three-state honesty + /// applies to the port — and for a reason review had to point out. Origin == File is NOT "fresh + /// install": collapses BYO, a missing credential, a timeout and + /// every other connection failure into the same answer, so this branch is also reached on a long-lived box + /// whose store is merely unreachable this minute and may well hold mcp_enabled = true. Asserting the + /// endpoint is off there would contradict the sweep-declined line printed a few lines later in the same + /// run, which says the opposite — that the off may be stale and the rule may be the live one — and it is + /// the sweep-declined line that is right. /// - private static string DescribeDisabledSurface(DarlingHostBinding.EndpointToggle toggle, string section) + private static string DescribeDisabledSurface( + DarlingHostBinding.EndpointToggle toggle, string section, string? storeUnavailableReason) => toggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane ? $"{section}.network exposes this endpoint, but the CONTROL PLANE has it off " + $"(config.config_service.{section}_enabled = false), so the service does not start it and no rule " + $"belongs on that port — turn it on with --enable-{section} or in the Viewer's Settings, then " + "re-run --configure-firewall from an elevated prompt" - : $"{section}.network exposes this endpoint, but {section}.enabled = false, which is the value the " - + "store is SEEDED from, so the service will not start it and no rule belongs on that port — turn " - + $"it on with --enable-{section} (or set {section}.enabled before the first start)"; + : $"{section}.network exposes this endpoint, but the control plane could NOT be read " + + $"({storeUnavailableReason ?? "reason unknown"}), so this run goes on darling.json's " + + $"{section}.enabled = false and opens nothing. On a box whose store has never been written — the " + + $"normal state at install time — that is right: config.config_service.{section}_enabled is SEEDED " + + "from this value, so the endpoint will not start. On a box that has run before it may be stale, " + + $"because --enable-{section} and the Viewer's Settings write only the control plane and never back " + + "to the file; if the endpoint IS enabled there, re-run --configure-firewall once the store is up " + + "and it will open the port"; /// 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. From f0a896c18717b928dc3876eca83a946355ab7025 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:36:53 +0100 Subject: [PATCH 4/5] Stop the not-elevated summary claiming loopback (review) "Every endpoint is loopback-only, so no firewall rule is needed" was true when the only way to reach that line was a bind that resolved loopback. This change added a second way: a surface fully configured for the LAN whose control plane has it switched off. The line then tells an operator their exposure config did not take, which is the opposite of what happened. It now says no endpoint wants an open port, and prints the plans' own notes - the same lines the elevated path already prints, which are where "the CONTROL PLANE has it off" or "allowFrom is not a valid CIDR" actually gets said. An operator who cannot elevate could not see either of those before, and they are the whole explanation for the summary above them. The remedy half stays out of scope and is filed instead: the not-elevated branch prints commands only for Open plans, so a Remove that wants its sweep - now including a deliberate --disable-mcp - still hands over nothing to run. That predates this change; what this change did was make it reachable more often. Co-Authored-By: Claude Opus 5 --- .../DarlingCliCommands.cs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index ec30dfbfd7..aa622850db 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -3184,9 +3184,19 @@ these commands by hand can see which plane chose the ports baked into them. */ real, actionable failure, so exit non-zero AND print every command to run by hand. */ if (toOpen == 0) { + /* #2436: "every endpoint is loopback-only" stopped being the only way to get here — a surface + the control plane has switched off is fully configured for the LAN and still wants no rule. + The plans' own notes say which it is, and they are the same lines the elevated path prints; + printing them here as well is what keeps an operator who cannot elevate from reading "no + rule is needed" as "your exposure config did not take". */ 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.)"); + "No endpoint wants an open port, so no rule was opened. (This shell is not elevated, but " + + "there was nothing to open.)"); + foreach (var plan in plans.Where(p => p.Note is not null)) + { + output.WriteLine($" {plan.Surface}: {plan.Note}."); + } + return 0; } From b002dc84e69056d2c4f812b36b376afb56c4d77e Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:45:10 +0100 Subject: [PATCH 5/5] Print the per-surface notes on the not-elevated path too (review) The previous commit printed them only where every plan was a removal, which left the mixed case exactly as it was: an exposed surface got a command, its disabled sibling got nothing, and the note explaining why - the only place a surface says it is fully LAN-configured and switched OFF - was dropped silently. That is the wrong half to have fixed, since the mixed case is the one where the two surfaces disagree and the operator most needs to be told which is which. The notes now print for every plan, before either branch. They are the same lines the elevated path already prints as it works; the person who could not act on them yet was simply the one who never saw them. The remedy half is still out of scope and filed as #2445: this path prints commands only for Open plans, so a Remove that wants its sweep still hands over nothing to run, and closing that honestly needs a read-only firewall probe here - which this branch currently makes none of - or an admitted approximation. Co-Authored-By: Claude Opus 5 --- .../DarlingCliCommands.cs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs index aa622850db..e1f5f1b121 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs @@ -3182,21 +3182,23 @@ these commands by hand can see which plane chose the ports baked into them. */ /* 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. */ + /* #2436: the per-surface notes, for EVERY plan and before either branch below. The elevated path + prints them as it works; this path used to print none at all, so the operator who most needs + them — the one who cannot act on them yet — was the one who never saw them. They are also the + only place a surface says it is fully LAN-configured and switched OFF, which is a state that did + not exist here before this change: "no rule is needed" would otherwise read as "your exposure + config did not take", and in the mixed case a disabled surface's stale rule would be dropped + silently while its exposed sibling got a command. */ + foreach (var plan in plans.Where(p => p.Note is not null)) + { + output.WriteLine($"{plan.Surface}: {plan.Note}."); + } + if (toOpen == 0) { - /* #2436: "every endpoint is loopback-only" stopped being the only way to get here — a surface - the control plane has switched off is fully configured for the LAN and still wants no rule. - The plans' own notes say which it is, and they are the same lines the elevated path prints; - printing them here as well is what keeps an operator who cannot elevate from reading "no - rule is needed" as "your exposure config did not take". */ output.WriteLine( "No endpoint wants an open port, so no rule was opened. (This shell is not elevated, but " + "there was nothing to open.)"); - foreach (var plan in plans.Where(p => p.Note is not null)) - { - output.WriteLine($" {plan.Surface}: {plan.Note}."); - } - return 0; }