diff --git a/Darling/Darling.Tests/DarlingFirewallCheckTests.cs b/Darling/Darling.Tests/DarlingFirewallCheckTests.cs
index 2d93f94d37..73c3d4c926 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,261 @@ 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.
+ /// 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_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(
+ 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);
+
+ /* And it does NOT sweep — see the test below, which is the reason. */
+ 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
+ /// 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));
+ }
+
+ ///
+ /// 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 +816,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..e1f5f1b121 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs
@@ -2722,6 +2722,14 @@ 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 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.
@@ -2736,12 +2744,31 @@ 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 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);
+ 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 +2809,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 +2821,46 @@ 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", storeUnavailableReason)
+ : DescribeLoopbackReason(mcpBind.Reason, "mcp"),
+ mcpOpen
? DarlingHostBinding.DescribeFirewallPortAuthority(mcpToggle, "mcp", "MCP", config.Mcp.Port, storeUnavailableReason)
- : null));
+ : null,
+ /* 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);
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", storeUnavailableReason)
+ : DescribeLoopbackReason((DarlingMcpHostService.McpBindReason)webBind.Reason, "web"),
+ webOpen
? DarlingHostBinding.DescribeFirewallPortAuthority(webToggle, "web", "web dashboard", config.Web.Port, storeUnavailableReason)
- : null));
+ : null,
+ SweepOtherPorts: !webExposed || webToggle.Origin == DarlingHostBinding.EndpointToggleOrigin.ControlPlane));
return plans;
}
@@ -2825,6 +2871,47 @@ 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.
+ ///
+ /// 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, 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 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.
private static string? DescribeLoopbackReason(DarlingMcpHostService.McpBindReason reason, string section) =>
@@ -3095,11 +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)
{
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.)");
return 0;
}
@@ -3124,17 +3223,43 @@ 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.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)
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.