diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index adf36346..2da8f546 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -87,6 +87,24 @@ jobs: - run: npx vitest run - run: npm run build + agent: + # Semantic checks on the endpoint agent's pure logic. Until this existed the + # agent had no tests at all, so a change to what an endpoint watches - or to + # the guard that decides which paths it will execute - was reviewed by + # reading alone. + # + # The project targets net8.0 and links the platform-pure sources rather than + # referencing ShopDbEndpointAgent.Core, which is net8.0-windows and cannot + # run here. See the csproj header for why, and for what that trades away. + # Runs in well under a second; it does not meaningfully add to gate time. + runs-on: arc-runner-set + steps: + - uses: actions/checkout@v5 + - uses: actions/setup-dotnet@v4 + with: + dotnet-version: '8.0.x' + - run: dotnet test agent/ShopDbEndpointAgent.Core.Tests/ShopDbEndpointAgent.Core.Tests.csproj --nologo + lean-build: # ADR-013 Phase 5: prove a per-site build carries only its chosen plugins. # The profile is the WJ plugin set, so this job proves the build this branch diff --git a/agent/ShopDbEndpointAgent.Core.Tests/LaunchPathPolicyTests.cs b/agent/ShopDbEndpointAgent.Core.Tests/LaunchPathPolicyTests.cs new file mode 100644 index 00000000..fdfa85f1 --- /dev/null +++ b/agent/ShopDbEndpointAgent.Core.Tests/LaunchPathPolicyTests.cs @@ -0,0 +1,312 @@ +using ShopDbEndpointAgent.Core; +using Xunit; + +namespace ShopDbEndpointAgent.Core.Tests; + +// Pins LaunchPathPolicy, the safeguard that lets the agent honor a launch path +// chosen in the WebUI without turning "write access to a file" into "remote code +// execution as LocalSystem". +// +// These tests build a real directory tree under the temp dir and point the +// policy at it via the approved-roots override, rather than asserting against +// hardcoded C:\Program Files strings. That keeps them honest about the checks +// (which touch the disk: File.Exists, link resolution) while staying runnable on +// the Linux CI runners. Cases that genuinely require Windows semantics, notably +// NTFS alternate data streams and junctions, are called out where they appear. +public sealed class LaunchPathPolicyTests : IDisposable +{ + private readonly string root; + + public LaunchPathPolicyTests() + { + root = Path.Combine(Path.GetTempPath(), + "shopdb-launchpolicy-" + Guid.NewGuid().ToString("n")); + Directory.CreateDirectory(root); + } + + public void Dispose() + { + try { Directory.Delete(root, recursive: true); } + catch (IOException) { } + catch (UnauthorizedAccessException) { } + } + + private LaunchPathPolicy PolicyForRoot() + { + return new LaunchPathPolicy(new[] { root }); + } + + private string CreateFile(string relativePath) + { + var full = Path.Combine(root, relativePath); + Directory.CreateDirectory(Path.GetDirectoryName(full)!); + File.WriteAllText(full, ""); + return full; + } + + [Fact] + public void An_approved_executable_under_an_approved_root_is_accepted() + { + var target = CreateFile(Path.Combine("UDC", "udc.exe")); + + var accepted = PolicyForRoot().TryValidate(target, out var fullPath, out var reason); + + Assert.True(accepted, reason); + Assert.Equal(target, fullPath); + } + + [Theory] + [InlineData("udc.exe")] + [InlineData("run.bat")] + [InlineData("run.cmd")] + public void Every_launchable_extension_is_accepted(string fileName) + { + // .bat and .cmd are deliberate: some shop-floor apps ship a launcher + // script rather than an exe. + var target = CreateFile(fileName); + + var accepted = PolicyForRoot().TryValidate(target, out _, out var reason); + + Assert.True(accepted, reason); + } + + [Theory] + [InlineData("payload.dll")] + [InlineData("payload.ps1")] + [InlineData("readme.txt")] + [InlineData("payload.vbs")] + public void A_non_launchable_extension_is_rejected(string fileName) + { + var target = CreateFile(fileName); + + var accepted = PolicyForRoot().TryValidate(target, out _, out var reason); + + Assert.False(accepted); + Assert.Contains("not launchable", reason); + } + + [Fact] + public void A_path_outside_every_approved_root_is_rejected() + { + var outside = Path.Combine(Path.GetTempPath(), + "shopdb-outside-" + Guid.NewGuid().ToString("n")); + Directory.CreateDirectory(outside); + try + { + var target = Path.Combine(outside, "evil.exe"); + File.WriteAllText(target, ""); + + var accepted = PolicyForRoot().TryValidate(target, out _, out var reason); + + Assert.False(accepted); + Assert.Contains("outside every approved launch root", reason); + } + finally + { + Directory.Delete(outside, recursive: true); + } + } + + [Fact] + public void A_sibling_root_with_a_shared_prefix_is_rejected() + { + // Guards the root comparison: "" must not also match + // "Evil", which a naive StartsWith without a separator allows. + var sibling = root + "Evil"; + Directory.CreateDirectory(sibling); + try + { + var target = Path.Combine(sibling, "evil.exe"); + File.WriteAllText(target, ""); + + var accepted = PolicyForRoot().TryValidate(target, out _, out var reason); + + Assert.False(accepted); + Assert.Contains("outside every approved launch root", reason); + } + finally + { + Directory.Delete(sibling, recursive: true); + } + } + + [Fact] + public void A_traversal_escaping_an_approved_root_is_rejected() + { + // GetFullPath normalizes the "..", so the check sees where the path + // actually lands rather than the approved-looking prefix it started + // with. + var target = Path.Combine(root, "..", "escaped.exe"); + + var accepted = PolicyForRoot().TryValidate(target, out _, out var reason); + + Assert.False(accepted); + Assert.Contains("outside every approved launch root", reason); + } + + [Fact] + public void A_path_that_does_not_exist_is_rejected() + { + var target = Path.Combine(root, "missing.exe"); + + var accepted = PolicyForRoot().TryValidate(target, out _, out var reason); + + Assert.False(accepted); + Assert.Contains("does not exist", reason); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void An_empty_candidate_is_rejected(string? candidate) + { + // This is the common case, not an edge case: most monitors carry no + // launch path at all, and the caller must fail closed rather than + // treating "no path" as "any path". + var accepted = PolicyForRoot().TryValidate(candidate, out _, out var reason); + + Assert.False(accepted); + Assert.Equal("empty path", reason); + } + + [Fact] + public void A_rejected_path_never_reports_a_usable_full_path() + { + // The caller keys off the bool but passes fullPath onward, so a + // rejection must not leave anything launchable in it. + var accepted = PolicyForRoot().TryValidate( + Path.Combine(root, "missing.exe"), out var fullPath, out _); + + Assert.False(accepted); + Assert.Equal("", fullPath); + } + + [Fact] + public void An_alternate_data_stream_is_rejected() + { + // Windows-only semantics. On net8.0 GetFullPath no longer throws on an + // interior colon, so "readme.txt:payload.exe" would otherwise report an + // .exe extension, pass the root check, and exist as a stream. + if (!OperatingSystem.IsWindows()) + return; + + CreateFile("readme.txt"); + var target = Path.Combine(root, "readme.txt:payload.exe"); + + var accepted = PolicyForRoot().TryValidate(target, out _, out var reason); + + Assert.False(accepted); + Assert.Contains("alternate data stream", reason); + } + + [Fact] + public void A_symlink_pointing_outside_an_approved_root_is_rejected() + { + // The lexical checks constrain the NAME; this pins that the policy also + // resolves what the name actually refers to. Creating a link needs no + // privilege beyond write access to the parent, and vendor install trees + // under Program Files are not reliably locked down. + var outside = Path.Combine(Path.GetTempPath(), + "shopdb-linktarget-" + Guid.NewGuid().ToString("n")); + Directory.CreateDirectory(outside); + try + { + var realTarget = Path.Combine(outside, "payload.exe"); + File.WriteAllText(realTarget, ""); + + var link = Path.Combine(root, "innocent.exe"); + try + { + File.CreateSymbolicLink(link, realTarget); + } + catch (UnauthorizedAccessException) + { + return; + } + catch (IOException) + { + return; + } + + var accepted = PolicyForRoot().TryValidate(link, out _, out var reason); + + Assert.False(accepted); + Assert.Contains("resolves outside every approved launch root", reason); + } + finally + { + Directory.Delete(outside, recursive: true); + } + } + + [Fact] + public void An_empty_roots_override_falls_back_to_the_shipped_defaults() + { + // A site that configures no roots must not end up with an empty + // allowlist, which would fail every launch closed. + var policy = new LaunchPathPolicy(Array.Empty()); + var target = CreateFile("udc.exe"); + + var accepted = policy.TryValidate(target, out _, out var reason); + + Assert.False(accepted); + Assert.Contains("outside every approved launch root", reason); + } + + [Fact] + public void A_whitespace_only_root_override_is_ignored() + { + var policy = new LaunchPathPolicy(new[] { " ", "" }); + var target = CreateFile("udc.exe"); + + var accepted = policy.TryValidate(target, out _, out _); + + Assert.False(accepted); + } + + [Fact] + public void ProgramData_is_an_approved_root_so_MTConnect_can_start() + { + // MTConnect installs to ProgramData\udc\mtconnect_udc\agent and starts + // through run.bat. With only the Program Files roots approved, its + // server-provisioned launch path was rejected outright, which is why + // remote start only ever worked through a local launchpaths pin that + // skipped validation. Deliberately broad for now - see BuildDefaultRoots. + var programData = Environment.GetFolderPath( + Environment.SpecialFolder.CommonApplicationData); + Assert.False(string.IsNullOrWhiteSpace(programData)); + + Assert.Contains( + LaunchPathPolicy.DefaultApprovedRoots, + root => root.StartsWith(programData, StringComparison.OrdinalIgnoreCase)); + } + + [Fact] + public void The_Program_Files_roots_are_still_approved() + { + // Widening to ProgramData must not have replaced the original roots: + // eDNC and UDC both install under Program Files. + var programFiles = Environment.GetFolderPath( + Environment.SpecialFolder.ProgramFiles); + Assert.False(string.IsNullOrWhiteSpace(programFiles)); + + Assert.Contains( + LaunchPathPolicy.DefaultApprovedRoots, + root => root.StartsWith(programFiles, StringComparison.OrdinalIgnoreCase)); + } + + [Fact] + public void A_launcher_script_under_an_approved_root_is_accepted() + { + // The run.bat pattern: no directly launchable .exe, so the approved + // extension set has to keep covering .bat. + var policy = PolicyForRoot(); + var target = CreateFile("run.bat"); + + var accepted = policy.TryValidate(target, out var validated, out var reason); + + Assert.True(accepted, reason); + Assert.EndsWith("run.bat", validated, StringComparison.OrdinalIgnoreCase); + } +} diff --git a/agent/ShopDbEndpointAgent.Core.Tests/LaunchPathResolutionTests.cs b/agent/ShopDbEndpointAgent.Core.Tests/LaunchPathResolutionTests.cs new file mode 100644 index 00000000..97758185 --- /dev/null +++ b/agent/ShopDbEndpointAgent.Core.Tests/LaunchPathResolutionTests.cs @@ -0,0 +1,71 @@ +using ShopDbEndpointAgent.Core; +using Xunit; + +namespace ShopDbEndpointAgent.Core.Tests; + +// Pins the local launch map, which decides where process_start looks for an +// executable before it consults the server. +// +// The rule these tests protect: launch paths are SERVER authoritative. A local +// entry wins over the server-provisioned path and bypasses LaunchPathPolicy +// validation entirely, so anything shipped here is both unvalidated and immune +// to WebUI edits. That combination is what left stale pins on endpoints, so the +// compiled defaults are empty on purpose and a test guards that. +public class LaunchPathResolutionTests +{ + [Fact] + public void No_launch_paths_are_compiled_into_the_agent() + { + // A compiled pin cannot be changed from the WebUI and skips validation. + // eDNC, UDC and MTConnect all carry their launchpath on the monitor row. + Assert.Empty(AgentConfig.DefaultLaunchPaths); + } + + [Fact] + public void A_config_without_launch_paths_resolves_to_an_empty_map() + { + // Empty here is what makes ResolveLaunchPath fall through to the + // server-provisioned path on the monitor row. + var config = new AgentConfig(); + + Assert.Empty(config.ResolveLaunchPaths()); + } + + [Fact] + public void A_local_override_still_wins_when_a_site_sets_one() + { + // The escape hatch stays available for an endpoint that cannot use the + // server value; it just is not shipped by default any more. + var config = new AgentConfig(); + config.LaunchPaths["UDC.exe"] = @"D:\Custom\UDC.exe"; + + var resolved = config.ResolveLaunchPaths(); + + Assert.Equal(@"D:\Custom\UDC.exe", resolved["UDC"]); + } + + [Fact] + public void A_local_override_key_is_normalized_to_a_bare_image_name() + { + // Keys are stored bare and the map is case-insensitive; CommandExecutor + // normalizes the lookup side the same way, so '.exe' or not both hit. + var config = new AgentConfig(); + config.LaunchPaths["DncMain.exe"] = @"D:\Dnc\DncMain.exe"; + + var resolved = config.ResolveLaunchPaths(); + + Assert.True(resolved.ContainsKey("DncMain")); + Assert.True(resolved.ContainsKey("dncmain")); + Assert.False(resolved.ContainsKey("DncMain.exe")); + } + + [Fact] + public void Blank_launch_path_entries_are_ignored() + { + var config = new AgentConfig(); + config.LaunchPaths["UDC.exe"] = " "; + config.LaunchPaths[" "] = @"D:\Stray.exe"; + + Assert.Empty(config.ResolveLaunchPaths()); + } +} diff --git a/agent/ShopDbEndpointAgent.Core.Tests/MonitorConfigMergeTests.cs b/agent/ShopDbEndpointAgent.Core.Tests/MonitorConfigMergeTests.cs new file mode 100644 index 00000000..c55dd8e6 --- /dev/null +++ b/agent/ShopDbEndpointAgent.Core.Tests/MonitorConfigMergeTests.cs @@ -0,0 +1,203 @@ +using ShopDbEndpointAgent.Core; +using Xunit; + +namespace ShopDbEndpointAgent.Core.Tests; + +// Pins MonitorConfig.Merge, which decides what an endpoint watches. +// +// The rule these tests exist to protect: the SERVER is authoritative when it +// answers, and a non-answer is not an answer. Concretely, null means "the fetch +// failed, keep what we have" while an empty-but-present list means "watch +// nothing". Collapsing those two cases is the bug this suite is here to catch, +// and it is a bug in both directions. Treat null as empty and one failed HTTP +// call blanks monitoring on every endpoint at once. Treat empty as null and +// detaching the last monitor in the WebUI silently does nothing forever. +public class MonitorConfigMergeTests +{ + private static MonitorConfig Local(params string[] processNames) + { + return new MonitorConfig + { + Processes = processNames + .Select(name => new MonitorTarget { Name = name }) + .ToArray(), + }; + } + + private static string[] Names(MonitorTarget[] targets) + { + return targets.Select(target => target.Name).OrderBy(name => name).ToArray(); + } + + [Fact] + public void A_server_list_replaces_the_local_one_rather_than_unioning() + { + // The regression that motivated the change: dncMain.exe was baked into + // appsettings.json by the installer and kept being reported on every + // endpoint even though no policy attached it. Under the old union it + // survived forever because the server had no way to retract it. + var local = Local("dncMain.exe", "UDC.exe"); + var server = new ResolvedMonitors + { + Processes = new[] { new MonitorTarget { Name = "notepad.exe" } }, + }; + + var merged = MonitorConfig.Merge(local, server); + + Assert.Equal(new[] { "notepad.exe" }, Names(merged.Processes)); + } + + [Fact] + public void A_null_server_response_keeps_the_current_watch_list() + { + // FetchResolvedMonitorsAsync returns null on every failure path. A + // transient blip must not blank monitoring fleet-wide. + var local = Local("dncMain.exe"); + + var merged = MonitorConfig.Merge(local, null); + + Assert.Equal(new[] { "dncMain.exe" }, Names(merged.Processes)); + } + + [Fact] + public void An_empty_server_response_clears_the_watch_list() + { + // The counterpart to the test above, and the reason null and empty must + // stay distinguishable: detaching the last monitor has to take effect. + var local = Local("dncMain.exe"); + var server = new ResolvedMonitors(); + + var merged = MonitorConfig.Merge(local, server); + + Assert.Empty(merged.Processes); + Assert.Empty(merged.Services); + } + + [Fact] + public void Services_are_replaced_independently_of_processes() + { + var local = new MonitorConfig + { + Processes = new[] { new MonitorTarget { Name = "dncMain.exe" } }, + Services = new[] { new MonitorTarget { Name = "MTConnectAgent" } }, + }; + var server = new ResolvedMonitors + { + Processes = new[] { new MonitorTarget { Name = "udc.exe" } }, + }; + + var merged = MonitorConfig.Merge(local, server); + + Assert.Equal(new[] { "udc.exe" }, Names(merged.Processes)); + Assert.Empty(merged.Services); + } + + [Fact] + public void A_null_local_config_with_a_null_server_response_is_not_a_crash() + { + // Defensive: the worker holds a singleton AgentConfig whose Monitors + // property is assignable, so null is reachable. + var merged = MonitorConfig.Merge(null, null); + + Assert.Empty(merged.Processes); + Assert.Empty(merged.Services); + } + + [Fact] + public void Server_entries_are_deduplicated_case_insensitively_by_name() + { + // Names are matched by string equality downstream, in the collector and + // in the launch-path lookup, so a duplicate would be reported twice. + var server = new ResolvedMonitors + { + Processes = new[] + { + new MonitorTarget { Name = "UDC.exe", App = "first" }, + new MonitorTarget { Name = "udc.exe", App = "second" }, + }, + }; + + var merged = MonitorConfig.Merge(Local(), server); + + var single = Assert.Single(merged.Processes); + Assert.Equal("second", single.App); + } + + [Fact] + public void Blank_server_entries_are_dropped() + { + var server = new ResolvedMonitors + { + Processes = new[] + { + new MonitorTarget { Name = " " }, + new MonitorTarget { Name = "" }, + new MonitorTarget { Name = "udc.exe" }, + }, + }; + + var merged = MonitorConfig.Merge(Local(), server); + + Assert.Equal(new[] { "udc.exe" }, Names(merged.Processes)); + } + + [Fact] + public void Server_names_are_trimmed_so_downstream_matching_works() + { + // The collector compares the reported name against enumerated process + // images by exact string equality; an untrimmed name never matches and + // the monitor silently reports stopped forever. + var server = new ResolvedMonitors + { + Processes = new[] { new MonitorTarget { Name = " udc.exe " } }, + }; + + var merged = MonitorConfig.Merge(Local(), server); + + Assert.Equal("udc.exe", Assert.Single(merged.Processes).Name); + } + + [Fact] + public void A_server_provisioned_launch_path_survives_the_replace() + { + // CommandExecutor.ServerLaunchPathFor reads the merged watch list to + // find launch paths, so dropping LaunchPath here would break + // process_start for policy-provisioned apps. + var server = new ResolvedMonitors + { + Processes = new[] + { + new MonitorTarget + { + Name = "udc.exe", + App = "UDC", + LaunchPath = @"C:\Program Files\UDC\udc.exe", + }, + }, + }; + + var merged = MonitorConfig.Merge(Local(), server); + + var target = Assert.Single(merged.Processes); + Assert.Equal(@"C:\Program Files\UDC\udc.exe", target.LaunchPath); + Assert.Equal("UDC", target.App); + } + + [Fact] + public void A_local_only_monitor_does_not_survive_a_successful_fetch() + { + // The security half of the change. A tampered appsettings.json could + // otherwise add a never-running entry and park the asset at a permanent + // condition, training operators to ignore a genuinely amber endpoint. + var local = Local("fabricated-never-running.exe"); + var server = new ResolvedMonitors + { + Processes = new[] { new MonitorTarget { Name = "udc.exe" } }, + }; + + var merged = MonitorConfig.Merge(local, server); + + Assert.DoesNotContain(merged.Processes, + target => target.Name == "fabricated-never-running.exe"); + } +} diff --git a/agent/ShopDbEndpointAgent.Core.Tests/ShopDbEndpointAgent.Core.Tests.csproj b/agent/ShopDbEndpointAgent.Core.Tests/ShopDbEndpointAgent.Core.Tests.csproj new file mode 100644 index 00000000..a5a20f2c --- /dev/null +++ b/agent/ShopDbEndpointAgent.Core.Tests/ShopDbEndpointAgent.Core.Tests.csproj @@ -0,0 +1,60 @@ + + + + + + net8.0 + enable + enable + ShopDbEndpointAgent.Core.Tests + ShopDbEndpointAgent.Core.Tests + true + false + + + + + + + + + + + + + + + diff --git a/agent/ShopDbEndpointAgent.Core/AgentConfig.cs b/agent/ShopDbEndpointAgent.Core/AgentConfig.cs index 9ce09fd5..ed91c0e5 100644 --- a/agent/ShopDbEndpointAgent.Core/AgentConfig.cs +++ b/agent/ShopDbEndpointAgent.Core/AgentConfig.cs @@ -81,22 +81,27 @@ public sealed class AgentConfig [JsonPropertyName("approvedlaunchroots")] public List ApprovedLaunchRoots { get; set; } = new(); - // Shipped defaults for the org's common shop-floor apps. A site's - // appsettings.json LaunchPaths entries win over these on a per-image basis. - // MTConnect is intentionally absent here: it is a Windows service controlled - // by the fixed mtconnect_* handler (install.bat + service control), not the - // generic process_start path. eDNC and UDC launch their runtime executables - // directly. + // Intentionally EMPTY: launch paths are server authoritative. + // + // Launch paths now live on the monitor rows the WebUI owns, the same place + // the watch list comes from, so editing a path in the WebUI reaches every + // endpoint with no rebuild and no redeploy. A compiled default here (or an + // appsettings.json launchpaths entry) still WINS over the server path and + // skips LaunchPathPolicy validation entirely, which is exactly how a stale + // pinned path would outlive the configuration that justified it. + // + // This stays empty on purpose. Populate it only as a deliberate local + // override for one endpoint that cannot use the server value, and expect + // that endpoint to stop tracking WebUI changes for that image. public static readonly IReadOnlyDictionary DefaultLaunchPaths = - new Dictionary(StringComparer.OrdinalIgnoreCase) - { - ["DncMain.exe"] = @"C:\Program Files (x86)\Dnc\bin\DncMain.exe", - ["UDC.exe"] = @"C:\Program Files\UDC\UDC.exe", - }; + new Dictionary(StringComparer.OrdinalIgnoreCase); - // The effective launch map: shipped defaults overlaid with appsettings.json - // entries (site wins). Keys are normalized to bare image names so a lookup - // tolerates 'DncMain' or 'DncMain.exe'. + // The effective launch map: shipped defaults (now empty) overlaid with + // appsettings.json entries. Normally empty, in which case process_start + // resolves through the server-provisioned path on the monitor row. Keys are + // normalized to bare image names so a lookup tolerates 'DncMain' or + // 'DncMain.exe'. Anything here BYPASSES LaunchPathPolicy - see + // DefaultLaunchPaths. public IReadOnlyDictionary ResolveLaunchPaths() { var resolved = new Dictionary(StringComparer.OrdinalIgnoreCase); @@ -212,38 +217,58 @@ public sealed class MonitorConfig [JsonPropertyName("services")] public MonitorTarget[] Services { get; set; } = Array.Empty(); - // Union the local (appsettings.json) watch list with the server-resolved one, - // keyed case-insensitively by wire name so a target watched both locally and - // by policy is not enumerated twice. Server entries win on the label (App) so - // a policy display name overrides a stale local one, but a local-only target - // is preserved - the server list augments, it does not replace. Nulls on - // either side degrade to the other, which is what keeps monitoring alive when - // a fetch fails. + // The SERVER decides what this endpoint watches. A server list replaces the + // local (appsettings.json) one outright rather than merging into it. + // + // This used to union the two, which made the effective watch list "whatever + // the server says PLUS whatever the box says", and the box half could not be + // retracted from the server at all. Two consequences, both bad. Operationally, + // monitors the installer had baked into appsettings.json kept being reported + // long after policy stopped attaching them, so detaching a monitor in the + // WebUI silently did nothing on already-deployed endpoints. Security-wise, it + // made a local file an input to server-side health: anyone able to edit it + // could add a never-running entry and park the asset at a permanent + // condition, training operators to ignore an endpoint that is amber for a + // fabricated reason. + // + // The caller MUST only call this when the fetch succeeded. A failed fetch + // returns null from FetchResolvedMonitorsAsync and the caller keeps the + // current list - that is what stops a transient network blip from blanking + // monitoring fleet-wide. A null server argument here is therefore treated as + // "no answer, keep local" rather than "watch nothing"; an EMPTY but present + // server list is a real answer and does clear the watch list, which is what + // makes detaching the last monitor take effect. public static MonitorConfig Merge(MonitorConfig? local, ResolvedMonitors? server) { + if (server is null) + return local ?? new MonitorConfig(); + return new MonitorConfig { - Processes = MergeTargets(local?.Processes, server?.Processes), - Services = MergeTargets(local?.Services, server?.Services), + Processes = Authoritative(server.Processes), + Services = Authoritative(server.Services), }; } - private static MonitorTarget[] MergeTargets( - MonitorTarget[]? local, MonitorTarget[]? server) + // Copies the server list, dropping unusable entries. Names are trimmed on + // the TARGET, not just on the dictionary key: downstream matching reads + // target.Name (the collector compares it against enumerated process images, + // and CommandExecutor.ServerLaunchPathFor matches it against the requested + // image), so a padded name that was only trimmed for de-duplication would + // never match anything and the monitor would report stopped forever. + private static MonitorTarget[] Authoritative(MonitorTarget[]? server) { + if (server is null || server.Length == 0) + return Array.Empty(); + var byName = new Dictionary( StringComparer.OrdinalIgnoreCase); - foreach (var target in local ?? Array.Empty()) + foreach (var target in server) { if (string.IsNullOrWhiteSpace(target?.Name)) continue; - byName[target.Name.Trim()] = target; - } - foreach (var target in server ?? Array.Empty()) - { - if (string.IsNullOrWhiteSpace(target?.Name)) - continue; - byName[target.Name.Trim()] = target; + target.Name = target.Name.Trim(); + byName[target.Name] = target; } return byName.Values.ToArray(); } @@ -258,12 +283,13 @@ public sealed class MonitorTarget // Human-facing application label, optional. Falls back to Name when empty. [JsonPropertyName("app")] public string App { get; set; } = ""; - // Optional server-provisioned launch path for a process monitor. Only set on - // process targets that a site leader configured a path for. The agent NEVER - // launches this blindly: LaunchPathPolicy validates it against an approved- - // root allowlist first, and a local appsettings.json launchpaths entry for - // the same image still wins. Null/empty means "no server path" - the agent - // falls back to its local launch map, and process_start fails closed when - // neither yields a validated path. + // Optional server-provisioned launch path for a process monitor. Set on + // process targets a site leader configured a path for, and now the PRIMARY + // source for process_start since the local launch map ships empty. The + // agent never launches this blindly: LaunchPathPolicy validates it against + // the approved-root allowlist, checks the extension, confirms it exists and + // re-checks it after resolving symlinks/junctions. A local appsettings.json + // launchpaths entry for the same image still wins if one is present. + // Null/empty means "no server path" and process_start fails closed. [JsonPropertyName("launchpath")] public string? LaunchPath { get; set; } } diff --git a/agent/ShopDbEndpointAgent.Core/Contracts.cs b/agent/ShopDbEndpointAgent.Core/Contracts.cs index ee7ace3f..99ed24b4 100644 --- a/agent/ShopDbEndpointAgent.Core/Contracts.cs +++ b/agent/ShopDbEndpointAgent.Core/Contracts.cs @@ -237,8 +237,10 @@ public sealed class CommandResult // The server-resolved watch list for this endpoint (processes and services the // site's policy attaches to the box's PC type), fetched each telemetry cycle so // a policy-attached monitor is enumerated without editing appsettings.json on -// the box. Same {name, app} shape as the local MonitorConfig, so the merge is a -// direct list union. Empty lists mean "watch nothing extra beyond local". +// the box. Same {name, app} shape as the local MonitorConfig, and it REPLACES +// that local list rather than unioning with it - the server is authoritative on +// what is watched. Empty lists mean "watch nothing", which is how detaching the +// last monitor takes effect; "no answer" is a null response, not an empty one. public sealed class ResolvedMonitors { [JsonPropertyName("processes")] public MonitorTarget[] Processes { get; set; } diff --git a/agent/ShopDbEndpointAgent.Core/LaunchPathPolicy.cs b/agent/ShopDbEndpointAgent.Core/LaunchPathPolicy.cs index 94755761..f93a248c 100644 --- a/agent/ShopDbEndpointAgent.Core/LaunchPathPolicy.cs +++ b/agent/ShopDbEndpointAgent.Core/LaunchPathPolicy.cs @@ -23,8 +23,10 @@ public sealed class LaunchPathPolicy private static readonly string[] ApprovedExtensions = { ".exe", ".bat", ".cmd" }; - // Shipped org defaults: the standard 64- and 32-bit install roots. Resolved - // from the environment so a non-C: system drive still matches. + // Shipped org defaults: the standard 64- and 32-bit install roots plus + // ProgramData. See BuildDefaultRoots for why ProgramData is included and + // why it should be narrowed later. Resolved from the environment so a + // non-C: system drive still matches. public static IReadOnlyList DefaultApprovedRoots { get; } = BuildDefaultRoots(); @@ -222,12 +224,37 @@ public sealed class LaunchPathPolicy return full.TrimEnd(Path.DirectorySeparatorChar); } + // REVISIT: ProgramData is deliberately broad here and should be narrowed. + // + // Shop-floor apps do not all install to Program Files. MTConnect lives at + // ProgramData\udc\mtconnect_udc\agent and starts through run.bat, so with + // only the Program Files roots approved, every server-provisioned launch + // path for it was rejected as "outside every approved launch root". That + // was previously masked because a local appsettings.json launchpaths entry + // bypasses validation entirely; now that launch paths are server + // authoritative, the root list is the only gate and it has to cover the + // real install locations. + // + // WARNING: ProgramData is world-writable by default on Windows. A + // non-administrator who can write under an approved ProgramData subtree can + // plant a .bat/.cmd/.exe there, and the agent runs launch paths as SYSTEM. + // The other gates still apply (the path must come from a monitor row an + // admin configured, must exist, must be a launchable extension, and is + // re-checked after symlink/junction resolution), so this is not an open + // door - but it is a wider blast radius than Program Files, whose ACLs + // require elevation to write. + // + // Narrow this to the specific subtrees in use (ProgramData\udc rather than + // all of ProgramData) once the full set of non-Program-Files install + // locations across the fleet is confirmed. A site can already do that today + // via AgentConfig.ApprovedLaunchRoots, which REPLACES this list. private static IReadOnlyList BuildDefaultRoots() { var candidates = new[] { Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles), Environment.GetFolderPath(Environment.SpecialFolder.ProgramFilesX86), + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), }; return candidates .Where(path => !string.IsNullOrWhiteSpace(path)) diff --git a/agent/ShopDbEndpointAgent.Core/TelemetryWorker.cs b/agent/ShopDbEndpointAgent.Core/TelemetryWorker.cs index 04f4cb20..5c6433b4 100644 --- a/agent/ShopDbEndpointAgent.Core/TelemetryWorker.cs +++ b/agent/ShopDbEndpointAgent.Core/TelemetryWorker.cs @@ -231,9 +231,11 @@ public sealed class TelemetryWorker : BackgroundService { // Refresh the watch list from server policy BEFORE collecting so a // policy-attached monitor (added in the WebUI, never in appsettings.json) - // is enumerated and reported this cycle. Server entries merge over the - // local list; a failed fetch returns null and Merge keeps the local list, - // so monitoring never blanks on a transient blip. + // is enumerated and reported this cycle. The server list REPLACES the + // local one, so detaching a monitor in the WebUI actually stops it being + // watched even on endpoints whose installer baked it into + // appsettings.json. A failed fetch returns null and Merge keeps the + // current list, so monitoring never blanks on a transient blip. var resolved = await client.FetchResolvedMonitorsAsync( Environment.MachineName, token); if (resolved is not null) diff --git a/plugins/endpoints/api/monitors.py b/plugins/endpoints/api/monitors.py index db73d185..1bf4c25d 100644 --- a/plugins/endpoints/api/monitors.py +++ b/plugins/endpoints/api/monitors.py @@ -28,22 +28,13 @@ from ..models import EndpointMonitor from .routes import endpoints_bp -# Which PC type the shipped monitors attach to, as a SETTING rather than a -# literal (ADR-015). PC-type vocabulary is per-site seed data: a plant whose -# type is called "Shop Floor Workstation" got no shipped monitors at all when -# this was hardcoded, and a plant that happens to use the name for something -# else got all of them. The default matches the seeded type so nothing changes -# for a site that uses it; a blank setting attaches the shipped bundle to -# nothing, which is the right neutral behaviour for a site that does not want -# it rather than a silent mismatch. +# RETIRED. This setting named the PC type the shipped monitors auto-attached +# to, defaulting to 'Shopfloor PC'. That auto-attach path is gone - policy is +# now the only attachment route (see _resolve_pctype_monitors) - so nothing +# reads this. The key is kept named here, not deleted, so an existing row in a +# site's settings table is recognisable as dead rather than mysterious, and so +# the key is not reused for something else later. SHIPPED_PCTYPE_SETTING = 'plugin.endpoints.shippedmonitorpctype' -SHIPPED_DEFAULT_PCTYPE = 'Shopfloor PC' - - -def shipped_pctype(): - """The PC type the shipped monitor bundle attaches to at this site.""" - configured = Setting.get(SHIPPED_PCTYPE_SETTING, default=SHIPPED_DEFAULT_PCTYPE) - return (configured or '').strip() _VALID_KINDS = ('process', 'service') @@ -170,20 +161,25 @@ def delete_monitor(monitor_id): def _resolve_pctype_monitors(pctype): - """Monitor definitions a PC type should watch: shipped defaults + policy. + """Monitor definitions a PC type should watch, from site policy alone. + + Policy is the ONLY attachment route. Shipped monitors used to also attach + automatically to whichever PC type the shipped-pctype setting named, which + defaulted to 'Shopfloor PC' - so eDNC, UDC and MTConnect were expected + monitors on every endpoint of that type at every site, with no policy + anywhere and no way to see why. 0019 split the shipped bundle into + per-monitor policies and removed the shipped SITE policy, intending + "off by default"; this branch survived and kept them attached, which is + why dncMain.exe kept appearing in Fleet health for all endpoints. + + Shipped monitors still exist and are still editable - a site composes them + through a Site Policy like anything else. What no longer happens is + attaching them because a PC type happens to carry a particular name. - Shipped monitors attach to the PC type named by the shipped-pctype setting; - policy-attached monitors come from the computers policy resolver by PC type. Returns a list of EndpointMonitor rows (active only), de-duped by id. """ selected = {} - shipped_for = shipped_pctype() - if shipped_for and pctype == shipped_for: - for monitor in EndpointMonitor.query.filter_by( - source='shipped', isactive=True).all(): - selected[monitor.endpointmonitorid] = monitor - computertypeid = _computertypeid_for(pctype) if computertypeid is not None: policy_ids = _policy_monitor_ids(computertypeid) diff --git a/plugins/endpoints/api/siteagent.py b/plugins/endpoints/api/siteagent.py index 897573f3..afcc2f55 100644 --- a/plugins/endpoints/api/siteagent.py +++ b/plugins/endpoints/api/siteagent.py @@ -201,21 +201,39 @@ _BASE_APPSETTINGS = { 'adoptonfirstrun': True, 'utilitiespath': '', 'logpath': '', + # Shipped EMPTY, deliberately. The server decides what an endpoint watches: + # the agent fetches its watch list from /api/endpoints/monitors/resolved + # every telemetry cycle and REPLACES this list with the answer, so anything + # seeded here is overwritten within one cadence anyway. + # + # It used to seed dncMain.exe, udc.exe, agent.exe and the MTConnect service, + # which is why every endpoint in the fleet reported eDNC regardless of what + # it actually ran. Back when the agent UNIONED the two lists, a baked-in + # entry could never be retracted from the server, so detaching a monitor in + # the WebUI silently did nothing on deployed endpoints and Fleet health + # surfaced dncMain everywhere. Seeding anything here re-creates that class of + # bug for as long as the affected packages stay in the field. + # + # To watch a process or service, attach a monitor to the PC type in the + # WebUI. That reaches every endpoint with no rebuild and no redeploy. 'monitors': { - 'processes': [ - {'name': 'dncMain.exe', 'app': 'eDNC'}, - {'name': 'udc.exe', 'app': 'Universal Data Collector'}, - {'name': 'agent.exe', 'app': 'MTConnect'}, - ], - 'services': [ - {'name': 'MTConnect UDC Agent', 'app': 'MTConnect'}, - ], - }, - 'launchpaths': { - 'DncMain.exe': 'C:\\Program Files (x86)\\Dnc\\bin\\DncMain.exe', - 'UDC.exe': 'C:\\Program Files\\UDC\\UDC.exe', - 'agent.exe': 'C:\\ProgramData\\udc\\mtconnect_udc\\agent\\run.bat', + 'processes': [], + 'services': [], }, + # Intentionally EMPTY, like monitors above, and for the same reason. Launch + # paths are server authoritative: they ride on the monitor rows the WebUI + # owns and reach the agent through /api/endpoints/monitors/resolved, so + # editing a path in the WebUI reaches every endpoint with no rebuild. + # + # A local entry here WINS over the server path AND bypasses the agent's + # LaunchPathPolicy validation (see CommandExecutor.ResolveLaunchPath), which + # is exactly how a stale pin outlives the configuration that justified it. + # eDNC, UDC and MTConnect all carry their launchpath on the shipped monitor + # rows instead. + # + # Seed an entry here only as a deliberate per-site override, accepting that + # the endpoint stops tracking WebUI changes for that image. + 'launchpaths': {}, # Approved scheduled task paths for scheduledtask_run. Shipped empty, which # means the agent falls back to ScheduledTaskPolicy.DefaultApprovedTaskPaths # (the Corporate Tools set). Listing entries here REPLACES those defaults, so diff --git a/plugins/endpoints/migrations/versions/0025_office_monitor_templates.py b/plugins/endpoints/migrations/versions/0025_office_monitor_templates.py new file mode 100644 index 00000000..d5eb6468 --- /dev/null +++ b/plugins/endpoints/migrations/versions/0025_office_monitor_templates.py @@ -0,0 +1,162 @@ +"""Seed the Office ProPlus LTSC monitor templates (x64 and x86). + +Revision ID: endpoints0025officemonitors +Revises: endpoints0024bootstraptokens +Create Date: 2026-09-19 + +Adds shipped process-monitor definitions for the Office ProPlus LTSC suite at +both install roots: + + C:\\Program Files\\Microsoft Office\\root\\Office16\\.EXE (x64) + C:\\Program Files (x86)\\Microsoft Office\\root\\Office16\\.EXE (x86) + +Apps: WINWORD, EXCEL, POWERPNT, OUTLOOK, MSACCESS. + +These are TEMPLATES, not an instruction to watch anything. Seeding a definition +only puts a row in the catalog; nothing is watched until a site policy attaches +it to a PC type. That separation is the whole point of the monitor rework - the +installer no longer bakes a watch list into appsettings.json, and the agent +replaces its list from the server each cycle - so adding rows here cannot +resurrect the "dncMain.exe on every endpoint" symptom. + +x64 and x86 are SEPARATE rows rather than one row with two paths. The launchpath +column holds a single path, and the two roots are genuinely different installs; +a site pins whichever one it actually deploys. The kind/displayname unique +constraint is what forces the architecture into the display name, which is also +what an operator needs to see to pick the right one. + +processname is the same image name for both (WINWORD.EXE and friends do not +differ by architecture), so these rows are distinguished by launchpath. A site +that runs both would attach only the row matching its install root. + +Idempotent on (kind, displayname): a re-run inserts nothing. Unlike 0006 this +does NOT bail out when the table is non-empty - that guard would make this a +no-op on every existing site, which is precisely where these templates are +wanted. Guards on the table being present and no-ops if it is not. + +Downgrade removes only the rows this added, and only when no policy action +references them, so a site that composed one into a policy does not lose it to a +rollback. +""" +from alembic import op + +import sqlalchemy as sa + +from datetime import datetime + + +# Alembic stores this in alembic_version.version_num, a VARCHAR(32). +revision = 'endpoints0025officemonitors' +down_revision = 'endpoints0024bootstraptokens' +branch_labels = None +depends_on = None +plugin_name = 'endpoints' + +MONITORS = 'endpointmonitors' + +_X64_ROOT = r'C:\Program Files\Microsoft Office\root\Office16' +_X86_ROOT = r'C:\Program Files (x86)\Microsoft Office\root\Office16' + +# (label, image name). The image name is identical across architectures. +_OFFICE_APPS = [ + ('Word', 'WINWORD.EXE'), + ('Excel', 'EXCEL.EXE'), + ('PowerPoint', 'POWERPNT.EXE'), + ('Outlook', 'OUTLOOK.EXE'), + ('Access', 'MSACCESS.EXE'), +] + + +def _rows(): + """The shipped Office rows: every app at both install roots.""" + for label, image in _OFFICE_APPS: + for architecture, root in (('x64', _X64_ROOT), ('x86', _X86_ROOT)): + yield { + 'kind': 'process', + 'displayname': 'Office {0} ({1})'.format(label, architecture), + 'processname': image, + 'servicename': None, + 'launchpath': '{0}\\{1}'.format(root, image), + 'datacollectorpath': None, + 'logcollectorpath': None, + 'source': 'shipped', + 'isactive': True, + } + + +def _inspector(): + from sqlalchemy import inspect + + return inspect(op.get_bind()) + + +def _has_table(name): + return _inspector().has_table(name) + + +def _existing(bind, kind, displayname): + return bind.execute( + sa.text('SELECT endpointmonitorid FROM endpointmonitors ' + 'WHERE kind = :kind AND displayname = :displayname'), + {'kind': kind, 'displayname': displayname}).scalar() + + +def upgrade(): + if not _has_table(MONITORS): + return + + bind = op.get_bind() + now = datetime.utcnow() + + monitors = sa.table( + MONITORS, + sa.column('kind', sa.String), + sa.column('displayname', sa.String), + sa.column('processname', sa.String), + sa.column('servicename', sa.String), + sa.column('launchpath', sa.String), + sa.column('datacollectorpath', sa.String), + sa.column('logcollectorpath', sa.String), + sa.column('source', sa.String), + sa.column('isactive', sa.Boolean), + sa.column('createdat', sa.DateTime), + ) + + pending = [] + for row in _rows(): + if _existing(bind, row['kind'], row['displayname']) is not None: + continue + record = dict(row) + record['createdat'] = now + pending.append(record) + + if pending: + op.bulk_insert(monitors, pending) + + +def downgrade(): + if not _has_table(MONITORS): + return + + bind = op.get_bind() + policies_present = _has_table('configurationpolicyactions') + + for row in _rows(): + monitorid = _existing(bind, row['kind'], row['displayname']) + if monitorid is None: + continue + + # Leave behind anything a site has already composed into a policy: + # dropping it would silently break that policy's action. + if policies_present: + referenced = bind.execute( + sa.text('SELECT COUNT(*) FROM configurationpolicyactions ' + 'WHERE action = :action'), + {'action': 'monitor:{0}'.format(monitorid)}).scalar() + if referenced: + continue + + bind.execute( + sa.text('DELETE FROM endpointmonitors ' + 'WHERE endpointmonitorid = :id'), + {'id': monitorid}) diff --git a/plugins/endpoints/plugin.py b/plugins/endpoints/plugin.py index 03276730..f6e82c33 100644 --- a/plugins/endpoints/plugin.py +++ b/plugins/endpoints/plugin.py @@ -1448,6 +1448,68 @@ class EndpointsPlugin(BasePlugin): '(concurrent update?)'.format(stored, skipped, len(to_add))) return stored + def _authoritative_services(self, assetid, reported): + """Keep only reported monitors the SERVER expects this asset to watch. + + The server is the authority on what is monitored. Without this, the + watch list is effectively whatever the endpoint says it is: the agent + merges its local appsettings.json into the server list, so anyone who + can edit that file on the box can add entries the server never + attached. Two attacks fall out of that, and the second is the one that + matters. Adding noise is merely annoying. Adding a NEVER-RUNNING entry + parks the asset at a permanent condition, and an operator who learns to + ignore a perpetually amber endpoint stops reading the signal that says + a real service died - the endpoint blinds the fleet view by talking, + not by going quiet. + + Resolution is by wire name, case-insensitively, because that is the + identity the agent reports and the identity the resolver hands out. + An asset with no resolved monitors keeps nothing, which is the same + "no monitors, no conditions" contract _service_conditions already has. + + Drops are logged rather than silent: an entry disappearing between the + endpoint and the page is exactly the kind of thing that is impossible + to diagnose from the UI alone. + """ + if not isinstance(reported, list): + return reported + + from flask import current_app + from .api.routes import _resolved_monitors_for_asset, _service_name + + try: + monitors = _resolved_monitors_for_asset(assetid) + except Exception: + # Never fail a check-in over the filter. A resolver error must not + # cost us the whole telemetry report, so fall back to storing what + # was reported and let the health layer apply its own rules. + current_app.logger.exception( + '[collector] asset_id=%s monitor_resolution_failed: ' + 'storing reported services unfiltered', assetid) + return reported + + expected = set() + for monitor in monitors: + for wirename in (monitor.processname, monitor.servicename): + if wirename and wirename.strip(): + expected.add(wirename.strip().lower()) + + kept, dropped = [], [] + for entry in reported: + if not isinstance(entry, dict): + continue + name = (_service_name(entry) or '').strip().lower() + if name in expected: + kept.append(entry) + else: + dropped.append(name) + + if dropped: + current_app.logger.info( + '[collector] asset_id=%s unmonitored_services_dropped: %s ' + '(not attached by any site policy)', assetid, ', '.join(dropped)) + return kept + def apply_collector_payload(self, payload: Dict) -> Dict: """Idempotently upsert one endpoint's state and append its events. @@ -1526,7 +1588,8 @@ class EndpointsPlugin(BasePlugin): if 'memorypercent' in payload: state.memorypercent = payload.get('memorypercent') if 'services' in payload: - state.servicesjson = _as_json(payload.get('services')) + state.servicesjson = _as_json( + self._authoritative_services(assetid, payload.get('services'))) if 'ports' in payload: state.portsjson = _as_json(payload.get('ports')) if 'interfaces' in payload: diff --git a/tests/test_plugin_migrations.py b/tests/test_plugin_migrations.py index c9de7065..a97870c4 100644 --- a/tests/test_plugin_migrations.py +++ b/tests/test_plugin_migrations.py @@ -123,7 +123,9 @@ EXPECTED_HEAD_REVISION['warranty'] = 'warranty0002proof' # endpointcredentials, endpointenrollrequests and endpointmigrationwaves. # 0024 adds endpointbootstraptokens: the short-lived, admin-minted tokens behind # the one-line bootstrap installer. -EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0024bootstraptokens' +# 0025 seeds the Office ProPlus LTSC monitor templates (x64 and x86). Catalog +# rows only: nothing is watched until a site policy attaches one. +EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0025officemonitors' # Plugins built after the cutover: their 0001 baseline really creates tables the # core chain never owned. diff --git a/tests/test_plugins/test_endpoints_ingest_authority.py b/tests/test_plugins/test_endpoints_ingest_authority.py new file mode 100644 index 00000000..7e580db3 --- /dev/null +++ b/tests/test_plugins/test_endpoints_ingest_authority.py @@ -0,0 +1,207 @@ +"""The server decides what is monitored, not the endpoint. + +The agent merges its local appsettings.json watch list into the server-resolved +one, so what an endpoint reports is not by itself evidence that the server ever +asked for it. Anyone able to edit that file on the box - or an old package with +monitors baked in by the installer - can put entries into the report that no +site policy attached. + +The interesting failure is not noise. It is a never-running entry that parks an +asset at a permanent condition: an operator who learns to ignore a perpetually +amber endpoint stops reading the signal that says a real service died. So the +endpoint can blind the fleet view by TALKING, not only by going quiet. + +These tests pin that reported entries are filtered against the server's own +resolved monitor list before they are stored. +""" + +import json + +from shopdb.core.models import Asset, AssetType, Setting + + +def _computer(db, hostname, computertype): + from plugins.computers.models import Computer, ComputerType + + atype = AssetType.query.filter_by(assettype='computer').first() + if not atype: + atype = AssetType(assettype='computer') + db.session.add(atype) + db.session.flush() + + ctype = ComputerType.query.filter_by(computertype=computertype).first() + if not ctype: + ctype = ComputerType(computertype=computertype) + db.session.add(ctype) + db.session.flush() + + asset = Asset(assetnumber=hostname, assettypeid=atype.assettypeid, + isactive=True) + db.session.add(asset) + db.session.flush() + comp = Computer(assetid=asset.assetid, hostname=hostname, + computertypeid=ctype.computertypeid) + db.session.add(comp) + db.session.flush() + return asset, ctype + + +def _attach_monitor_to_pctype(db, monitor, computertypeid): + from plugins.computers.models import ( + ConfigurationPolicy, ConfigurationPolicyAction, + BusinessPolicy, BusinessPolicyItem, + ) + + if not Setting.query.filter_by( + key='computers_configpolicies_enabled').first(): + db.session.add( + Setting(key='computers_configpolicies_enabled', value='true')) + + policy = ConfigurationPolicy( + name='Coverage {}'.format(monitor.endpointmonitorid), + scopejson={'type': 'pctype', 'id': computertypeid}, + ) + db.session.add(policy) + db.session.flush() + db.session.add(ConfigurationPolicyAction( + configurationpolicyid=policy.configurationpolicyid, + action='monitor:{}'.format(monitor.endpointmonitorid), + mode='gate', + )) + + business = BusinessPolicy(name='Site {}'.format( + monitor.endpointmonitorid), priority=100) + db.session.add(business) + db.session.flush() + db.session.add(BusinessPolicyItem( + businesspolicyid=business.businesspolicyid, + configurationpolicyid=policy.configurationpolicyid, + sortorder=0, + )) + db.session.commit() + + +def _stored_services(db, assetid): + from plugins.endpoints.models import EndpointState + + state = EndpointState.query.filter_by(assetid=assetid).first() + if state is None or not state.servicesjson: + return [] + return json.loads(state.servicesjson) + + +def _report(app, hostname, services): + from plugins.endpoints.plugin import EndpointsPlugin + + plugin = EndpointsPlugin() + return plugin.apply_collector_payload({ + 'hostname': hostname, + 'services': services, + }) + + +def test_an_unmonitored_reported_service_is_not_stored(client, db, app): + """The baked-in installer case: the endpoint reports dncMain.exe, but no + site policy attaches it, so it never reaches the stored snapshot.""" + asset, _ctype = _computer(db, 'INGEST1', 'Shopfloor PC') + db.session.commit() + + _report(app, 'INGEST1', [ + {'name': 'dncMain.exe', 'kind': 'process', 'state': 'stopped'}, + ]) + db.session.commit() + + assert _stored_services(db, asset.assetid) == [] + + +def test_a_policy_attached_service_is_stored(client, db, app): + """The control case: filtering must not throw away legitimate reports.""" + from plugins.endpoints.models import EndpointMonitor + + asset, ctype = _computer(db, 'INGEST2', 'Shopfloor PC') + monitor = EndpointMonitor( + kind='process', displayname='eDNC', processname='dncMain.exe', + source='shipped', isactive=True, + ) + db.session.add(monitor) + db.session.flush() + _attach_monitor_to_pctype(db, monitor, ctype.computertypeid) + + _report(app, 'INGEST2', [ + {'name': 'dncMain.exe', 'kind': 'process', 'state': 'stopped'}, + ]) + db.session.commit() + + stored = _stored_services(db, asset.assetid) + assert [entry['name'] for entry in stored] == ['dncMain.exe'] + + +def test_a_spoofed_never_running_service_cannot_park_the_asset_amber( + client, db, app): + """The attack that matters: an entry the server never attached cannot + manufacture a standing health condition.""" + from plugins.endpoints.services.health import _service_conditions + from plugins.endpoints.models import EndpointState + + asset, _ctype = _computer(db, 'INGEST3', 'Shopfloor PC') + db.session.commit() + + _report(app, 'INGEST3', [ + {'name': 'totally-made-up.exe', 'kind': 'process', 'state': 'stopped'}, + ]) + db.session.commit() + + state = EndpointState.query.filter_by(assetid=asset.assetid).first() + assert _service_conditions(state, None) == [] + + +def test_matching_ignores_case_on_the_wire_name(client, db, app): + """The agent reports whatever Windows hands it; 0006 seeds 'dncMain.exe' + while the launch path says 'DncMain.exe'. Identity is case-insensitive or + the filter would silently drop real monitors.""" + from plugins.endpoints.models import EndpointMonitor + + asset, ctype = _computer(db, 'INGEST4', 'Shopfloor PC') + monitor = EndpointMonitor( + kind='process', displayname='eDNC', processname='dncMain.exe', + source='shipped', isactive=True, + ) + db.session.add(monitor) + db.session.flush() + _attach_monitor_to_pctype(db, monitor, ctype.computertypeid) + + _report(app, 'INGEST4', [ + {'name': 'DNCMAIN.EXE', 'kind': 'process', 'state': 'running'}, + ]) + db.session.commit() + + stored = _stored_services(db, asset.assetid) + assert [entry['name'] for entry in stored] == ['DNCMAIN.EXE'] + + +def test_an_omitted_services_key_does_not_wipe_the_last_snapshot( + client, db, app): + """Filtering sits inside the existing presence guard: a report that omits + services leaves the previous snapshot standing.""" + from plugins.endpoints.models import EndpointMonitor + + asset, ctype = _computer(db, 'INGEST5', 'Shopfloor PC') + monitor = EndpointMonitor( + kind='service', displayname='MTConnect', servicename='MTConnect UDC Agent', + source='shipped', isactive=True, + ) + db.session.add(monitor) + db.session.flush() + _attach_monitor_to_pctype(db, monitor, ctype.computertypeid) + + _report(app, 'INGEST5', [ + {'name': 'MTConnect UDC Agent', 'kind': 'service', 'state': 'running'}, + ]) + db.session.commit() + + from plugins.endpoints.plugin import EndpointsPlugin + EndpointsPlugin().apply_collector_payload({'hostname': 'INGEST5'}) + db.session.commit() + + stored = _stored_services(db, asset.assetid) + assert [entry['name'] for entry in stored] == ['MTConnect UDC Agent'] diff --git a/tests/test_plugins/test_endpoints_office_monitor_templates.py b/tests/test_plugins/test_endpoints_office_monitor_templates.py new file mode 100644 index 00000000..a67e8807 --- /dev/null +++ b/tests/test_plugins/test_endpoints_office_monitor_templates.py @@ -0,0 +1,172 @@ +"""The Office ProPlus LTSC monitor templates seeded by endpoints 0025. + +Runs the migration against a bare engine and checks the rows it produces, rather +than asserting against the module's constants - a test that reads the same list +the code writes proves nothing about what lands in the table. + +The behaviour worth pinning here is that 0025 seeds on a site that ALREADY has +monitors. 0006 deliberately bails out when the catalog is non-empty (it is a +first-install seed), and copying that guard into 0025 would have made it a no-op +everywhere the templates are actually wanted. +""" +import importlib.util +from pathlib import Path + +from sqlalchemy import create_engine, text +from alembic.migration import MigrationContext +from alembic.operations import Operations + + +_ROOT = Path(__file__).resolve().parent.parent.parent +_ENDPOINTS_VERSIONS = _ROOT / 'plugins' / 'endpoints' / 'migrations' / 'versions' + +OFFICE_MIGRATION = '0025_office_monitor_templates.py' + +_EXPECTED_NAMES = { + 'Office Word (x64)', 'Office Word (x86)', + 'Office Excel (x64)', 'Office Excel (x86)', + 'Office PowerPoint (x64)', 'Office PowerPoint (x86)', + 'Office Outlook (x64)', 'Office Outlook (x86)', + 'Office Access (x64)', 'Office Access (x86)', +} + + +def _load(path, prefix): + spec = importlib.util.spec_from_file_location(prefix + path.stem, str(path)) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def _run(connection, *loaded): + operations = Operations(MigrationContext.configure(connection)) + operations._install_proxy() + try: + for module in loaded: + module.upgrade() + finally: + operations._remove_proxy() + connection.commit() + + +def _office_module(): + return _load(_ENDPOINTS_VERSIONS / OFFICE_MIGRATION, 'endpoints_mig_0025_') + + +def _seeded(connection): + _run( + connection, + _load(_ENDPOINTS_VERSIONS / '0006_endpoint_monitors.py', + 'endpoints_mig_0006_'), + _office_module(), + ) + + +def _monitors(connection): + rows = connection.execute(text( + 'SELECT displayname, kind, processname, launchpath, source ' + 'FROM endpointmonitors')).all() + return {row[0]: row for row in rows} + + +def _connection(): + return create_engine('sqlite://').connect() + + +def test_every_office_app_is_seeded_at_both_install_roots(): + with _connection() as connection: + _seeded(connection) + by_name = _monitors(connection) + + assert _EXPECTED_NAMES.issubset(set(by_name)) + + +def test_access_is_included(): + """Called out explicitly because it is the one that gets forgotten.""" + with _connection() as connection: + _seeded(connection) + by_name = _monitors(connection) + + assert by_name['Office Access (x64)'][2] == 'MSACCESS.EXE' + assert by_name['Office Access (x86)'][2] == 'MSACCESS.EXE' + + +def test_the_architectures_point_at_different_install_roots(): + with _connection() as connection: + _seeded(connection) + by_name = _monitors(connection) + + x64 = by_name['Office Excel (x64)'][3] + x86 = by_name['Office Excel (x86)'][3] + + assert x64 == r'C:\Program Files\Microsoft Office\root\Office16\EXCEL.EXE' + assert x86 == (r'C:\Program Files (x86)\Microsoft Office\root\Office16' + r'\EXCEL.EXE') + + +def test_the_rows_are_shipped_process_monitors(): + with _connection() as connection: + _seeded(connection) + by_name = _monitors(connection) + + for name in _EXPECTED_NAMES: + assert by_name[name][1] == 'process', name + assert by_name[name][4] == 'shipped', name + + +def test_seeding_is_idempotent(): + """The kind/displayname unique constraint would raise on a re-run.""" + with _connection() as connection: + _seeded(connection) + _run(connection, _office_module()) + by_name = _monitors(connection) + + assert len([name for name in by_name if name.startswith('Office ')]) == 10 + + +def test_templates_are_seeded_on_a_site_that_already_has_monitors(): + """0006 bails out when the catalog is non-empty; 0025 must not. + + Every existing site has the four shipped monitors from 0006, so copying that + guard would make this migration a no-op exactly where it is needed. + """ + with _connection() as connection: + _run(connection, + _load(_ENDPOINTS_VERSIONS / '0006_endpoint_monitors.py', + 'endpoints_mig_0006_')) + before = _monitors(connection) + assert before, 'expected the 0006 shipped rows to be present' + + _run(connection, _office_module()) + after = _monitors(connection) + + assert _EXPECTED_NAMES.issubset(set(after)) + assert 'eDNC' in after + + +def test_seeding_does_not_disturb_the_existing_shipped_monitors(): + with _connection() as connection: + _seeded(connection) + by_name = _monitors(connection) + + assert by_name['eDNC'][2] == 'dncMain.exe' + assert by_name['Universal Data Collector'][2] == 'udc.exe' + + +def test_downgrade_removes_the_seeded_templates(): + module = _office_module() + with _connection() as connection: + _seeded(connection) + + operations = Operations(MigrationContext.configure(connection)) + operations._install_proxy() + try: + module.downgrade() + finally: + operations._remove_proxy() + connection.commit() + + by_name = _monitors(connection) + + assert not _EXPECTED_NAMES & set(by_name) + assert 'eDNC' in by_name diff --git a/tests/test_plugins/test_endpoints_policy_monitors.py b/tests/test_plugins/test_endpoints_policy_monitors.py index af4c38ed..a948c9b1 100644 --- a/tests/test_plugins/test_endpoints_policy_monitors.py +++ b/tests/test_plugins/test_endpoints_policy_monitors.py @@ -253,30 +253,69 @@ def test_a_shipped_monitor_keeps_the_identity_the_policy_matches_on( assert monitor.displayname == 'eDNC' -# --- which PC type the shipped bundle attaches to is a setting --------------- +# --- enabling a monitor must never require a new agent ---------------------- -def test_shipped_monitors_follow_the_configured_pctype(client, db, app): - from plugins.endpoints.api.monitors import ( - SHIPPED_PCTYPE_SETTING, _resolve_pctype_monitors) +def test_a_monitor_created_today_reaches_the_agent_watch_list(client, db, app, + auth_headers): + """Create a monitor over the API, attach it by policy, and it is in the + agent's watch list on the next fetch. + + This is the no-redeploy contract, and it is the property most easily lost + in a refactor. The agent reads this route every cycle and merges the result + over appsettings.json, so a monitor that appears here is live on the floor + without shipping a new exe. Nothing in this test touches the agent. + """ + asset, ctype = _computer(db, 'NOREDEPLOY1', 'CMM PC') + + created = client.post('/api/endpoints/monitors', json={ + 'kind': 'service', 'displayname': 'Print Spooler', + 'servicename': 'Spooler', + }, headers=auth_headers) + assert created.status_code in (200, 201) + + from plugins.endpoints.models import EndpointMonitor + monitor = EndpointMonitor.query.filter_by(servicename='Spooler').first() + assert monitor is not None + _attach_monitor_to_pctype(db, monitor, ctype.computertypeid) + + response = client.get(RESOLVED_URL, query_string={'hostname': 'NOREDEPLOY1'}, + headers=_collector_headers(app)) + assert response.status_code == 200 + data = response.get_json()['data'] + assert {'name': 'Spooler', 'app': 'Print Spooler'} in data['services'] + + +# --- shipped monitors attach by POLICY ONLY --------------------------------- + +def test_shipped_monitors_are_not_attached_by_pc_type_name(client, db, app): + """The auto-attach path is gone: a shipped monitor is not expected anywhere + until a Site Policy composes it. + + Replaces two tests that pinned the old behaviour - that shipped monitors + followed a configurable PC type name, and that blanking the setting turned + them off. Both described a route that should not exist. Keeping them would + have meant keeping the bug they documented: every endpoint typed + 'Shopfloor PC' silently expected eDNC, UDC and MTConnect, which is what put + dncMain.exe in Fleet health for all endpoints. + """ + from plugins.endpoints.api.monitors import _resolve_pctype_monitors _shipped(db) - Setting.set(SHIPPED_PCTYPE_SETTING, 'Shop Floor Workstation') db.session.commit() - # The plant's own name for the type resolves the bundle... + assert _resolve_pctype_monitors('Shopfloor PC') == [] + assert _resolve_pctype_monitors('Shop Floor Workstation') == [] + + +def test_a_shipped_monitor_still_resolves_once_policy_attaches_it( + client, db, app): + """Removing auto-attach must not make shipped monitors unusable - a site + composes them like any other monitor.""" + from plugins.endpoints.api.monitors import _resolve_pctype_monitors + + asset, ctype = _computer(db, 'SHIPPEDPC1', 'Shopfloor PC') + monitor = _shipped(db) + _attach_monitor_to_pctype(db, monitor, ctype.computertypeid) + assert [m.displayname for m in - _resolve_pctype_monitors('Shop Floor Workstation')] == ['eDNC'] - # ...and the name this shipped with no longer does. - assert _resolve_pctype_monitors('Shopfloor PC') == [] - - -def test_a_blank_shipped_pctype_attaches_the_bundle_to_nothing(client, db, app): - """The neutral behaviour for a site that does not want the bundle: off, - rather than silently attached to whatever happens to match.""" - from plugins.endpoints.api.monitors import ( - SHIPPED_PCTYPE_SETTING, _resolve_pctype_monitors) - - _shipped(db) - Setting.set(SHIPPED_PCTYPE_SETTING, '') - db.session.commit() - assert _resolve_pctype_monitors('Shopfloor PC') == [] + _resolve_pctype_monitors('Shopfloor PC')] == ['eDNC'] diff --git a/tests/test_plugins/test_endpoints_siteagent_build.py b/tests/test_plugins/test_endpoints_siteagent_build.py index b5c0f236..74f39da4 100644 --- a/tests/test_plugins/test_endpoints_siteagent_build.py +++ b/tests/test_plugins/test_endpoints_siteagent_build.py @@ -351,3 +351,33 @@ def test_install_script_helper_without_pins_or_key(): assert '[string[]] $ServerPins = @()' in script assert 'CollectorKey' not in script assert 'ServerPins left as is' in script + + +def test_the_package_ships_no_baked_in_monitors(client, db, auth_headers, servercert): + """The server owns the watch list, so the installer must seed nothing. + + Seeding monitors here is what put dncMain.exe on every endpoint in Fleet + health: the entry rode into appsettings.json on every package, and + appsettings.json is never rewritten on the box, so it outlived any policy + that justified it. The agent now replaces its watch list from + /api/endpoints/monitors/resolved, which makes a seeded entry pointless as + well as harmful. + """ + settings = json.loads(_files(_build( + client, auth_headers, collectorkey=SECRET))['appsettings.json']) + + assert settings['monitors'] == {'processes': [], 'services': []} + + +def test_the_package_ships_no_baked_in_launch_paths(client, db, auth_headers, servercert): + """Launch paths are server authoritative, so the installer seeds none. + + A local launchpaths entry wins over the server-provisioned path AND skips + LaunchPathPolicy validation, so a seeded entry both outlives the config that + justified it and makes WebUI edits never reach the endpoint. eDNC, UDC and + MTConnect carry their launchpath on the shipped monitor rows instead. + """ + settings = json.loads(_files(_build( + client, auth_headers, collectorkey=SECRET))['appsettings.json']) + + assert settings['launchpaths'] == {}