From 6563511b5fe6748c54b1ea528e358f1c3aab71b2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 21:22:01 -0400 Subject: [PATCH] =?UTF-8?q?fix(host):=20resolve=20Host-003,004=20=E2=80=94?= =?UTF-8?q?=20replace=20plaintext=20secrets=20with=20env=20placeholders,?= =?UTF-8?q?=20validate=20site=20seed-node=20ports;=20re-triage=20Host-002?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/Host/findings.md | 68 ++++++++++++-- src/ScadaLink.Host/StartupValidator.cs | 42 +++++++-- src/ScadaLink.Host/appsettings.Central.json | 9 +- src/ScadaLink.Host/appsettings.Site.json | 2 +- .../CentralDbTestEnvironment.cs | 41 +++++++++ .../ConfigSecretsTests.cs | 89 +++++++++++++++++++ .../ScadaLink.Host.Tests/HealthCheckTests.cs | 6 ++ .../ScadaLink.Host.Tests/HostStartupTests.cs | 2 + .../StartupValidatorTests.cs | 56 ++++++++++++ 9 files changed, 297 insertions(+), 18 deletions(-) create mode 100644 tests/ScadaLink.Host.Tests/CentralDbTestEnvironment.cs create mode 100644 tests/ScadaLink.Host.Tests/ConfigSecretsTests.cs diff --git a/code-reviews/Host/findings.md b/code-reviews/Host/findings.md index b13ecc2..0d78d15 100644 --- a/code-reviews/Host/findings.md +++ b/code-reviews/Host/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 10 | +| Open findings | 8 | ## Summary @@ -98,10 +98,10 @@ response body; it failed before the fix and passes after. Full Host suite green | | | |--|--| -| Severity | Medium | +| Severity | Medium — re-triaged: stale design doc, Host code is correct | | Category | Design-document adherence | | Status | Open | -| Location | `src/ScadaLink.Host/Actors/AkkaHostedService.cs:70-108` | +| Location | `src/ScadaLink.Host/Actors/AkkaHostedService.cs:70-108`, `docs/requirements/Component-Host.md` REQ-HOST-6 | **Description** @@ -126,7 +126,21 @@ add the plugin packages and HOCON. Either way, code and doc must agree. **Resolution** -_Unresolved._ +_Verified 2026-05-16, left Open — re-triaged._ The finding is accurate: a repo-wide +search confirms there are **no** `PersistentActor` / `ReceivePersistentActor` +subclasses anywhere in `src/`, no `akka.persistence` section in the HOCON built by +`AkkaHostedService.StartAsync`, and `ScadaLink.Host.csproj` references no persistence +plugin packages. The system deliberately uses component-owned SQLite storage +services instead. The **Host code is therefore correct and internally consistent** — +the only defect is that `docs/requirements/Component-Host.md` REQ-HOST-6 and its +Dependencies list still mandate Akka.Persistence, a subsystem that does not (and is +not intended to) exist. The sole fix is editing that design document, which lies +outside this resolution task's permitted edit scope (`src/ScadaLink.Host`, +`tests/ScadaLink.Host.Tests`, `code-reviews/Host/findings.md`). No code or test +change can resolve a stale-doc finding. Left **Open** and surfaced for the design-doc +owner: REQ-HOST-6 must drop the Akka.Persistence requirement (and the +`Akka.Persistence.Hosting` Dependencies entry), stating that node-local persistence +is provided by component-owned SQLite storage services. ### Host-003 — Secrets committed in plaintext in `appsettings.Central.json` @@ -134,7 +148,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Host/appsettings.Central.json:20-31` | **Description** @@ -159,7 +173,29 @@ environment. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`). Root cause confirmed against +`appsettings.Central.json`: the committed file carried real-looking secrets in +plaintext — SQL Server passwords (`Password=ScadaLink_Dev1#`) in both connection +strings, an LDAP service-account password, and a JWT signing key. Fix: all four +secrets were removed from the committed file and replaced with non-functional +`${...}` placeholder references (`ConfigurationDb` / `MachineDataDb`, +`LdapServiceAccountPassword`, `JwtSigningKey`). A new top-level `_secrets` note +documents that the Host's configuration builder (`AddEnvironmentVariables()`) +overlays the real values supplied via environment variables +(`ScadaLink__Database__ConfigurationDb`, `ScadaLink__Database__MachineDataDb`, +`ScadaLink__Security__LdapServiceAccountPassword`, +`ScadaLink__Security__JwtSigningKey`); the placeholders are intentionally invalid so +a misconfigured deployment fails loudly rather than silently using a committed key. +Regression test class `ConfigSecretsTests` asserts the committed file carries no +plaintext `Password=` value, no committed LDAP service-account password, and no +committed JWT signing key; all three tests failed before the fix and pass after. +Tests that drive the full `Program` startup pipeline against the real SQL provider +(`HealthCheckTests`, `HostStartupTests.CentralRole_StartsWithoutError`) were adapted +to supply the local dev connection strings themselves via the new +`CentralDbTestEnvironment` test helper (environment variables) — they must no longer +depend on committed secrets. Note: the `docker/central-node-*/appsettings.Central.json` +files still contain the same dev secrets but lie outside this task's permitted edit +scope; they should receive the same treatment in a follow-up. ### Host-004 — Site seed-node list points at the gRPC port, not a remoting port @@ -167,7 +203,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Host/appsettings.Site.json:10-19` | **Description** @@ -190,7 +226,23 @@ to reject a seed node whose port equals this node's `GrpcPort`. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`). Root cause confirmed against +`appsettings.Site.json`: with `Node:RemotingPort = 8082` and `Node:GrpcPort = 8083`, +the second `Cluster:SeedNodes` entry was `akka.tcp://scadalink@localhost:8083` — the +Kestrel HTTP/2 gRPC port, not an Akka.Remote endpoint. `StartupValidator` only +checked seed-node *count* (≥2), so the misconfiguration passed silently. Fix, two +parts: (1) the shipped `appsettings.Site.json` second seed entry was corrected to a +remoting port (`localhost:8084`); (2) `StartupValidator.Validate` was extended — for +`Site` nodes it now parses each seed node's TCP port (via a new `SeedNodePort` +helper) and rejects any entry whose port equals the node's `GrpcPort`, using the +resolved GrpcPort including the `8083` `NodeOptions` default when the key is absent. +The seed-node-count check was hoisted above the Site block so the new rule can reuse +the parsed list. Regression tests in `StartupValidatorTests`: +`Site_SeedNodeOnGrpcPort_FailsValidation`, +`Site_SeedNodeOnDefaultGrpcPort_FailsValidation` (default-8083 path), +`Site_SeedNodesOnRemotingPort_PassesValidation`, and +`Central_SeedNodeOnPort8083_PassesValidation` (rule is Site-only) — all failed +appropriately before the fix and pass after. ### Host-005 — Blocking sync-over-async (`GetAwaiter().GetResult()`) inside `StartAsync` diff --git a/src/ScadaLink.Host/StartupValidator.cs b/src/ScadaLink.Host/StartupValidator.cs index da7dfec..1c3c08b 100644 --- a/src/ScadaLink.Host/StartupValidator.cs +++ b/src/ScadaLink.Host/StartupValidator.cs @@ -40,23 +40,55 @@ public static class StartupValidator errors.Add("ScadaLink:Security:JwtSigningKey required for Central"); } + var seedNodes = configuration.GetSection("ScadaLink:Cluster:SeedNodes").Get>(); + if (seedNodes == null || seedNodes.Count < 2) + errors.Add("ScadaLink:Cluster:SeedNodes must have at least 2 entries"); + if (role == "Site") { var grpcPortStr = nodeSection["GrpcPort"]; - if (grpcPortStr != null && (!int.TryParse(grpcPortStr, out var gp) || gp < 1 || gp > 65535)) + int grpcPort = 8083; // NodeOptions default when the key is absent + if (grpcPortStr != null && (!int.TryParse(grpcPortStr, out grpcPort) || grpcPort < 1 || grpcPort > 65535)) errors.Add("ScadaLink:Node:GrpcPort must be 1-65535"); var dbSection = configuration.GetSection("ScadaLink:Database"); if (string.IsNullOrEmpty(dbSection["SiteDbPath"])) errors.Add("ScadaLink:Database:SiteDbPath required for Site nodes"); - } - var seedNodes = configuration.GetSection("ScadaLink:Cluster:SeedNodes").Get>(); - if (seedNodes == null || seedNodes.Count < 2) - errors.Add("ScadaLink:Cluster:SeedNodes must have at least 2 entries"); + // Host-004: a seed node must reference an Akka.Remote endpoint, never the + // Kestrel HTTP/2 gRPC port. A seed entry whose port equals this node's + // GrpcPort would make a joining node attempt an Akka.Remote TCP + // association against the gRPC listener and fail. + if (seedNodes != null) + { + foreach (var seed in seedNodes) + { + if (SeedNodePort(seed) == grpcPort) + errors.Add( + $"ScadaLink:Cluster:SeedNodes entry '{seed}' must not target the gRPC port " + + $"({grpcPort}); seed nodes must reference Akka remoting ports"); + } + } + } if (errors.Count > 0) throw new InvalidOperationException( $"Configuration validation failed:\n{string.Join("\n", errors.Select(e => $" - {e}"))}"); } + + /// + /// Extracts the TCP port from an Akka seed-node address of the form + /// akka.tcp://system@host:port. Returns -1 when no port can be parsed. + /// + private static int SeedNodePort(string seedNode) + { + if (string.IsNullOrWhiteSpace(seedNode)) + return -1; + + var lastColon = seedNode.LastIndexOf(':'); + if (lastColon < 0 || lastColon == seedNode.Length - 1) + return -1; + + return int.TryParse(seedNode[(lastColon + 1)..], out var port) ? port : -1; + } } diff --git a/src/ScadaLink.Host/appsettings.Central.json b/src/ScadaLink.Host/appsettings.Central.json index ce5ad38..a38dd24 100644 --- a/src/ScadaLink.Host/appsettings.Central.json +++ b/src/ScadaLink.Host/appsettings.Central.json @@ -16,9 +16,10 @@ "FailureDetectionThreshold": "00:00:10", "MinNrOfMembers": 1 }, + "_secrets": "Host-003: Secrets are NOT committed in this file. Supply them via environment variables, which the Host's configuration builder (AddEnvironmentVariables) overlays over this file. Required: ScadaLink__Database__ConfigurationDb, ScadaLink__Database__MachineDataDb, ScadaLink__Security__LdapServiceAccountPassword, ScadaLink__Security__JwtSigningKey. The ${...} placeholders below are intentionally non-functional and must be overridden per environment.", "Database": { - "ConfigurationDb": "Server=localhost,1433;Database=ScadaLinkConfig;User Id=scadalink_app;Password=ScadaLink_Dev1#;TrustServerCertificate=true", - "MachineDataDb": "Server=localhost,1433;Database=ScadaLinkMachineData;User Id=scadalink_app;Password=ScadaLink_Dev1#;TrustServerCertificate=true" + "ConfigurationDb": "${SCADALINK_CONFIGURATIONDB_CONNECTION_STRING}", + "MachineDataDb": "${SCADALINK_MACHINEDATADB_CONNECTION_STRING}" }, "Security": { "LdapServer": "localhost", @@ -27,8 +28,8 @@ "AllowInsecureLdap": true, "LdapSearchBase": "dc=scadalink,dc=local", "LdapServiceAccountDn": "cn=admin,dc=scadalink,dc=local", - "LdapServiceAccountPassword": "password", - "JwtSigningKey": "scadalink-dev-jwt-signing-key-must-be-at-least-32-characters-long", + "LdapServiceAccountPassword": "${SCADALINK_LDAP_SERVICE_ACCOUNT_PASSWORD}", + "JwtSigningKey": "${SCADALINK_JWT_SIGNING_KEY}", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30 }, diff --git a/src/ScadaLink.Host/appsettings.Site.json b/src/ScadaLink.Host/appsettings.Site.json index b22b999..87a6a34 100644 --- a/src/ScadaLink.Host/appsettings.Site.json +++ b/src/ScadaLink.Host/appsettings.Site.json @@ -10,7 +10,7 @@ "Cluster": { "SeedNodes": [ "akka.tcp://scadalink@localhost:8082", - "akka.tcp://scadalink@localhost:8083" + "akka.tcp://scadalink@localhost:8084" ], "SplitBrainResolverStrategy": "keep-oldest", "StableAfter": "00:00:15", diff --git a/tests/ScadaLink.Host.Tests/CentralDbTestEnvironment.cs b/tests/ScadaLink.Host.Tests/CentralDbTestEnvironment.cs new file mode 100644 index 0000000..5077c4b --- /dev/null +++ b/tests/ScadaLink.Host.Tests/CentralDbTestEnvironment.cs @@ -0,0 +1,41 @@ +namespace ScadaLink.Host.Tests; + +/// +/// Host-003: appsettings.Central.json no longer commits database connection +/// strings — they are externalised to environment variables. Tests that exercise the +/// full Program startup pipeline against the real SQL provider must therefore +/// supply the local dev connection strings the way a deployment would: via +/// environment variables (Program's configuration builder calls +/// AddEnvironmentVariables()). +/// +/// Dispose restores the previous values so tests stay isolated. +/// +internal sealed class CentralDbTestEnvironment : IDisposable +{ + // Local dev/test database — same credentials the infra docker-compose stack uses. + // This is a test fixture value, not a committed production secret. + private const string ConfigurationDb = + "Server=localhost,1433;Database=ScadaLinkConfig;User Id=scadalink_app;Password=ScadaLink_Dev1#;TrustServerCertificate=true"; + private const string MachineDataDb = + "Server=localhost,1433;Database=ScadaLinkMachineData;User Id=scadalink_app;Password=ScadaLink_Dev1#;TrustServerCertificate=true"; + + private const string ConfigKey = "ScadaLink__Database__ConfigurationDb"; + private const string MachineKey = "ScadaLink__Database__MachineDataDb"; + + private readonly string? _previousConfig; + private readonly string? _previousMachine; + + public CentralDbTestEnvironment() + { + _previousConfig = Environment.GetEnvironmentVariable(ConfigKey); + _previousMachine = Environment.GetEnvironmentVariable(MachineKey); + Environment.SetEnvironmentVariable(ConfigKey, ConfigurationDb); + Environment.SetEnvironmentVariable(MachineKey, MachineDataDb); + } + + public void Dispose() + { + Environment.SetEnvironmentVariable(ConfigKey, _previousConfig); + Environment.SetEnvironmentVariable(MachineKey, _previousMachine); + } +} diff --git a/tests/ScadaLink.Host.Tests/ConfigSecretsTests.cs b/tests/ScadaLink.Host.Tests/ConfigSecretsTests.cs new file mode 100644 index 0000000..4f34a02 --- /dev/null +++ b/tests/ScadaLink.Host.Tests/ConfigSecretsTests.cs @@ -0,0 +1,89 @@ +using System.Reflection; +using System.Text.Json; + +namespace ScadaLink.Host.Tests; + +/// +/// Host-003 regression: secrets must not be committed in plaintext in the +/// shipped appsettings.Central.json. Connection-string passwords, the LDAP +/// service-account password and the JWT signing key must be supplied via +/// environment variables (or another secret store) at deployment time — the +/// committed file may only carry non-sensitive structural defaults or +/// placeholder values. +/// +public class ConfigSecretsTests +{ + private static string FindHostProjectDirectory() + { + var assemblyDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!; + var dir = new DirectoryInfo(assemblyDir); + while (dir != null) + { + var hostPath = Path.Combine(dir.FullName, "src", "ScadaLink.Host"); + if (Directory.Exists(hostPath)) + return hostPath; + dir = dir.Parent; + } + throw new DirectoryNotFoundException("Could not locate src/ScadaLink.Host"); + } + + private static JsonElement ScadaLinkSection() + { + var path = Path.Combine(FindHostProjectDirectory(), "appsettings.Central.json"); + var json = File.ReadAllText(path); + using var doc = JsonDocument.Parse(json); + return doc.RootElement.GetProperty("ScadaLink").Clone(); + } + + [Fact] + public void CentralConfig_ConnectionStrings_ContainNoPlaintextPassword() + { + var db = ScadaLinkSection().GetProperty("Database"); + foreach (var prop in db.EnumerateObject()) + { + var value = prop.Value.GetString() ?? string.Empty; + // A committed connection string must not carry a literal Password= value. + // Either the password is delivered via an environment variable or the + // whole connection string is. A placeholder reference is acceptable. + var idx = value.IndexOf("Password=", StringComparison.OrdinalIgnoreCase); + if (idx >= 0) + { + var after = value[(idx + "Password=".Length)..]; + var literal = after.Split(';')[0]; + Assert.True( + literal.Length == 0 || literal.Contains('{') || literal.Contains('$'), + $"appsettings.Central.json '{prop.Name}' contains a plaintext Password value '{literal}'. " + + "Move the secret to an environment variable."); + } + } + } + + [Fact] + public void CentralConfig_LdapServiceAccountPassword_IsNotCommitted() + { + var security = ScadaLinkSection().GetProperty("Security"); + if (security.TryGetProperty("LdapServiceAccountPassword", out var pw)) + { + var value = pw.GetString() ?? string.Empty; + Assert.True( + value.Length == 0 || value.Contains('{') || value.Contains('$'), + $"appsettings.Central.json carries a plaintext LdapServiceAccountPassword '{value}'. " + + "Move it to an environment variable."); + } + } + + [Fact] + public void CentralConfig_JwtSigningKey_IsNotCommitted() + { + var security = ScadaLinkSection().GetProperty("Security"); + if (security.TryGetProperty("JwtSigningKey", out var key)) + { + var value = key.GetString() ?? string.Empty; + Assert.True( + value.Length == 0 || value.Contains('{') || value.Contains('$'), + $"appsettings.Central.json carries a committed JwtSigningKey '{value}'. " + + "A committed signing key lets anyone with repo access forge session tokens. " + + "Move it to an environment variable."); + } + } +} diff --git a/tests/ScadaLink.Host.Tests/HealthCheckTests.cs b/tests/ScadaLink.Host.Tests/HealthCheckTests.cs index 33dfe49..11d32b8 100644 --- a/tests/ScadaLink.Host.Tests/HealthCheckTests.cs +++ b/tests/ScadaLink.Host.Tests/HealthCheckTests.cs @@ -11,6 +11,12 @@ public class HealthCheckTests : IDisposable { private readonly List _disposables = new(); + public HealthCheckTests() + { + // Host-003: connection strings are externalised; supply them via env vars. + _disposables.Add(new CentralDbTestEnvironment()); + } + public void Dispose() { foreach (var d in _disposables) diff --git a/tests/ScadaLink.Host.Tests/HostStartupTests.cs b/tests/ScadaLink.Host.Tests/HostStartupTests.cs index e3195e4..4a8b741 100644 --- a/tests/ScadaLink.Host.Tests/HostStartupTests.cs +++ b/tests/ScadaLink.Host.Tests/HostStartupTests.cs @@ -28,6 +28,8 @@ public class HostStartupTests : IDisposable // Set the environment to Central so appsettings.Central.json is loaded, // and set DOTNET_ENVIRONMENT before the factory creates the host. var previousEnv = Environment.GetEnvironmentVariable("DOTNET_ENVIRONMENT"); + // Host-003: connection strings are externalised; supply them via env vars. + using var dbEnv = new CentralDbTestEnvironment(); try { Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", "Central"); diff --git a/tests/ScadaLink.Host.Tests/StartupValidatorTests.cs b/tests/ScadaLink.Host.Tests/StartupValidatorTests.cs index 422a301..3b14787 100644 --- a/tests/ScadaLink.Host.Tests/StartupValidatorTests.cs +++ b/tests/ScadaLink.Host.Tests/StartupValidatorTests.cs @@ -254,6 +254,62 @@ public class StartupValidatorTests Assert.Null(ex); } + [Fact] + public void Site_SeedNodeOnGrpcPort_FailsValidation() + { + // Host-004 regression: a site seed node must reference an Akka remoting + // endpoint, never the Kestrel HTTP/2 gRPC port. A seed node whose port + // equals this node's GrpcPort would make a joining node attempt an + // Akka.Remote TCP association against the gRPC listener and fail. + var values = ValidSiteConfig(); + values["ScadaLink:Node:GrpcPort"] = "8083"; + values["ScadaLink:Cluster:SeedNodes:1"] = "akka.tcp://scadalink@site-a-node1:8083"; + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + Assert.Contains("must not target the gRPC port", ex.Message); + } + + [Fact] + public void Site_SeedNodeOnDefaultGrpcPort_FailsValidation() + { + // GrpcPort is absent here, so the NodeOptions default of 8083 applies. + // A seed node on 8083 must still be rejected. + var values = ValidSiteConfig(); + values["ScadaLink:Cluster:SeedNodes:1"] = "akka.tcp://scadalink@site-a-node2:8083"; + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + Assert.Contains("must not target the gRPC port", ex.Message); + } + + [Fact] + public void Site_SeedNodesOnRemotingPort_PassesValidation() + { + // Two distinct site nodes, both seed entries on the remoting port (8082). + var values = ValidSiteConfig(); + values["ScadaLink:Node:GrpcPort"] = "8083"; + values["ScadaLink:Cluster:SeedNodes:0"] = "akka.tcp://scadalink@site-a-node1:8082"; + values["ScadaLink:Cluster:SeedNodes:1"] = "akka.tcp://scadalink@site-a-node2:8082"; + var config = BuildConfig(values); + + var ex = Record.Exception(() => StartupValidator.Validate(config)); + Assert.Null(ex); + } + + [Fact] + public void Central_SeedNodeOnPort8083_PassesValidation() + { + // The gRPC-port rule applies to Site nodes only. A Central node has no + // GrpcPort, so a seed node on 8083 must not be rejected. + var values = ValidCentralConfig(); + values["ScadaLink:Cluster:SeedNodes:1"] = "akka.tcp://scadalink@central-node2:8083"; + var config = BuildConfig(values); + + var ex = Record.Exception(() => StartupValidator.Validate(config)); + Assert.Null(ex); + } + [Fact] public void MultipleErrors_AllReported() {